Audio modules fixes by lgirdwood · Pull Request #10752 · thesofproject/sof

lgirdwood · 2026-05-07T19:40:38Z

Some small fixes picked up by the audio ztest UTs.

Copilot

Pull request overview

This PR addresses a few robustness issues in the audio modules that were surfaced by the audio ztest unit tests, primarily by adding missing validation and making teardown paths safer.

Changes:

Add Zephyr device_is_ready() gating to mic privacy manager initialization and guard policy queries when the device isn’t initialized.
Add MFCC configuration validation to reject non-positive frame_shift.
Harden ASRC free/reset paths by adding NULL checks, clearing pointers after freeing, and resetting module private data.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`src/audio/mic_privacy_manager/mic_privacy_manager_intel.c`	Add device readiness check during init and a guard in `get_policy()` to avoid dereferencing an uninitialized device pointer.
`src/audio/mfcc/mfcc_setup.c`	Add basic validation for `frame_shift` to prevent invalid hop sizes.
`src/audio/asrc/asrc.c`	Make `free()`/`reset()` more defensive by checking allocations before releasing and nulling pointers after free.

kv2019i · 2026-05-11T10:45:06Z

+	if (!device_is_ready(mic_priv_dev)) {
+		LOG_ERR("mic_privacy device not ready");
+		return -ENODEV;
+	}


This is a valid concern. Maybe alternative is to drop mic privacy when running the ztest?

probably want to drop it for native_sim, but for qemu ACE its a doable test - the funny thing is copilot is reviewing itself here for this change.

Addressed — on the device-not-ready path init now clears mic_priv_dev (sets it NULL) before returning -ENODEV, so the other helpers take their existing !mic_priv_dev path instead of dereferencing a not-ready device. (Re the broader "drop mic privacy under ztest" idea — that is a separate test-config question; this change just removes the stale-pointer deref.)

lgirdwood · 2026-05-13T15:29:18Z

 	fft->fft_size = config->frame_length;
 	fft->fft_padded_size = 1 << (31 - norm_int32(fft->fft_size)); /* Round up to nearest 2^N */
+	if (config->frame_shift <= 0) {
+		comp_err(dev, "frame_shift must be positive");
+		return -EINVAL;
+	}


@singalsu fyi - these came up as crashes during the ztest UT as it was passing in zeros for stuff it did not know about. Will get this fixed.

Fixed — the guard now also rejects frame_shift > frame_length, so prev_data_size = fft_size - fft_hop_size can no longer underflow. Returns -EINVAL with both values logged.

kv2019i

Looks ok. Not 100% sure whether for mic privacy would be easier to just disable mic privacy for ztest runs. Otherwise you'd need add more checks (or make the dev null if init is not done).

kv2019i · 2026-05-11T10:45:06Z

+	if (!device_is_ready(mic_priv_dev)) {
+		LOG_ERR("mic_privacy device not ready");
+		return -ENODEV;
+	}


This is a valid concern. Maybe alternative is to drop mic privacy when running the ztest?

lyakh

please don't re-add function names to prints. As for adding all the checks, IIUC they can only trigger in ztests, right? Do we really want to add code to actual run-time paths to satisfy our ztests or should we rather adjust ztests to avoid impossible situations? I don't think adding such checks for our test flows is scalable - we can call all (non-static) functions with invalid arguments. Should we add checks everywhere for them?

lyakh · 2026-05-12T08:20:36Z

-	struct comp_dev *dev = mod->dev;

-	comp_dbg(dev, "entry");
+	comp_info(mod->dev, "asrc_free() entry");


we have removed all function names from all logging on purpose, let's not start re-adding them. Function names are printed automatically by Zephyr.

Done — dropped the function name, now just comp_dbg(mod->dev, "entry").

lyakh · 2026-05-12T08:22:43Z

+	comp_info(mod->dev, "asrc_free() entry");
+
+	if (!cd)
+		return 0;


can it be called with cd == NULL? Don't think so, .free() is probably only called when .init() was successful

These guards are the actual double-free fix, not redundant checks. In the ZTest teardown that triggered this, free() runs more than once: the first call frees buf/asrc_obj, sets them NULL, frees cd and calls module_set_private_data(mod, NULL). The if (!cd) then makes the second free() a clean no-op instead of double-freeing cd. So it cannot be NULL on a normal single free after a successful init, but it can on the repeated-free path this fix targets.

so it cannot happen during normal usage? If so, does it make sense to have ztest flows that cannot happen in real situations and to have to modify the sources to support them?

lyakh · 2026-05-12T08:23:51Z

-	mod_free(mod, cd->asrc_obj);
+	cd->buf = NULL;
+
+	if (cd->asrc_obj) {


same, can it be NULL here?

Same rationale: reset() frees asrc_obj and sets it NULL, and free() can run afterwards. if (cd->asrc_obj) skips the second release rather than freeing an already-freed object. The NULL-after-free + guard pair is what removes the double-free across the reset()/free() ordering.

lyakh · 2026-05-12T08:25:37Z

+		cd->asrc_obj = NULL;
+	}
+
+	if (cd->buf) {


same for the above 2 added checks

Same here — reset() can be followed by free() (or another reset()), so guarding asrc_obj/buf with a non-NULL check and NULLing them after release prevents the second path from re-freeing. Kept deliberately for that reason.

lyakh · 2026-05-12T08:28:04Z

 	state->emph.coef = -config->preemphasis_coefficient; /* Negate config parameter */
 	fft->fft_size = config->frame_length;
 	fft->fft_padded_size = 1 << (31 - norm_int32(fft->fft_size)); /* Round up to nearest 2^N */
+	if (config->frame_shift <= 0) {


can this be hit in real pipelines or only in ztests?

It surfaced in the ztest UT (zeroed config), but it is reachable from a real pipeline too: frame_length/frame_shift come from the host topology blob, so a malformed or mis-generated topology could pass an out-of-range hop. Cheap to validate once at setup, so kept as a guard rather than a test-only assert.

lgirdwood · 2026-05-13T15:40:06Z

please don't re-add function names to prints. As for adding all the checks, IIUC they can only trigger in ztests, right? Do we really want to add code to actual run-time paths to satisfy our ztests or should we rather adjust ztests to avoid impossible situations? I don't think adding such checks for our test flows is scalable - we can call all (non-static) functions with invalid arguments. Should we add checks everywhere for them?

copilot fixed and added __func__, but this really means we have to adopt LOG() everywhere so that agents dont repeat.

We do need individual module UT ztests as we have no way to test in isolation, yes some of the data being passed in is coming out of band and not using normal runtime paths, but this is important as its obvious and now easy next step is to connect the modules APIs to fuzzing output in sim_native.

Properly track allocation state in the ASRC component lifecycle to prevent double-free of heap memory during module teardown in ZTest environments. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

Add input validation for the frame_shift configuration field in mfcc_setup(). A zero or negative value would cause a division by zero during STFT processing. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

Add null checks for hardware device pointers that may not be available when running in ZTest environments without real hardware. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

Copilot AI review requested due to automatic review settings May 7, 2026 19:40

lgirdwood requested review from dbaluta, kv2019i, lbetlej, mmaka1 and plbossart as code owners May 7, 2026 19:40

Copilot started reviewing on behalf of lgirdwood May 7, 2026 19:43 View session

Copilot AI reviewed May 7, 2026

View reviewed changes

kv2019i reviewed May 11, 2026

View reviewed changes

lyakh requested changes May 12, 2026

View reviewed changes

lrgirdwo added 3 commits June 22, 2026 13:41

audio: asrc: Fix heap double-free during teardown

1f32181

Properly track allocation state in the ASRC component lifecycle to prevent double-free of heap memory during module teardown in ZTest environments. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

audio: mic_privacy_manager: Fix NULL pointer dereference in ZTest

58d5b7d

Add null checks for hardware device pointers that may not be available when running in ZTest environments without real hardware. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

lgirdwood force-pushed the topic/audio-fixes branch from 1ebfd38 to 58d5b7d Compare June 22, 2026 14:44

Conversation

lgirdwood commented May 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented May 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants