Skip to content

Audio modules fixes #10752

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lgirdwood wants to merge 3 commits into
base: main
Choose a base branch
from
+46 −11
Open

Audio modules fixes #10752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f32181
audio: asrc: Fix heap double-free during teardown
lrgirdwo May 6, 2026
6adec5b
audio: mfcc: Add frame_shift validation to prevent division by zero
lrgirdwo May 6, 2026
58d5b7d
audio: mic_privacy_manager: Fix NULL pointer dereference in ZTest
lrgirdwo May 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 26 additions & 11 deletions src/audio/asrc/asrc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,15 +315,25 @@ static void asrc_release_buffers(struct processing_module *mod, struct asrc_farr
static int asrc_free(struct processing_module *mod)
{
struct comp_data *cd = module_get_private_data(mod);
struct comp_dev *dev = mod->dev;

comp_dbg(dev, "entry");
comp_dbg(mod->dev, "entry");

if (!cd)
return 0;

@lyakh lyakh May 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lgirdwood lgirdwood Jun 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lyakh lyakh Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


mod_free(mod, cd->buf);
asrc_release_buffers(mod, cd->asrc_obj);
asrc_free_polyphase_filter(mod, cd->asrc_obj);
mod_free(mod, cd->asrc_obj);
cd->buf = NULL;

if (cd->asrc_obj) {

@lyakh lyakh May 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, can it be NULL here?

@lgirdwood lgirdwood Jun 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

asrc_release_buffers(mod, cd->asrc_obj);
asrc_free_polyphase_filter(mod, cd->asrc_obj);
mod_free(mod, cd->asrc_obj);
cd->asrc_obj = NULL;
}

mod_free(mod, cd);
module_set_private_data(mod, NULL);

return 0;
}

Expand Down Expand Up @@ -862,12 +872,17 @@ static int asrc_reset(struct processing_module *mod)
asrc_dai_stop_timestamp(cd);

/* Free the allocations those were done in prepare() */
asrc_release_buffers(mod, cd->asrc_obj);
asrc_free_polyphase_filter(mod, cd->asrc_obj);
mod_free(mod, cd->asrc_obj);
mod_free(mod, cd->buf);
cd->asrc_obj = NULL;
cd->buf = NULL;
if (cd->asrc_obj) {
asrc_release_buffers(mod, cd->asrc_obj);
asrc_free_polyphase_filter(mod, cd->asrc_obj);
mod_free(mod, cd->asrc_obj);
cd->asrc_obj = NULL;
}

if (cd->buf) {

@lyakh lyakh May 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for the above 2 added checks

@lgirdwood lgirdwood Jun 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

mod_free(mod, cd->buf);
cd->buf = NULL;
}

return 0;
}
Expand Down
8 changes: 8 additions & 0 deletions src/audio/mfcc/mfcc_setup.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,14 @@ int mfcc_setup(struct processing_module *mod, int max_frames, int sample_rate, i
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 */
/* frame_shift is the hop size and must be a positive value no larger
* than the frame/FFT length, otherwise prev_data_size below underflows
*/
if (config->frame_shift <= 0 || config->frame_shift > config->frame_length) {
comp_err(dev, "invalid frame_shift %d for frame_length %d",
config->frame_shift, config->frame_length);
return -EINVAL;
}
fft->fft_hop_size = config->frame_shift;
fft->half_fft_size = (fft->fft_padded_size >> 1) + 1;

Expand Down
12 changes: 12 additions & 0 deletions src/audio/mic_privacy_manager/mic_privacy_manager_intel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,15 @@ int mic_privacy_manager_init(void)
if (!mic_priv_dev)
return -EINVAL;

if (!device_is_ready(mic_priv_dev)) {
LOG_ERR("mic_privacy device not ready");
/* Clear the handle so later helpers take their !mic_priv_dev
* path instead of dereferencing a device that is not ready
*/
mic_priv_dev = NULL;
return -ENODEV;
}
Comment on lines +98 to +105

@kv2019i kv2019i May 11, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lgirdwood lgirdwood May 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lgirdwood lgirdwood Jun 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)


mic_privacy_api = (struct mic_privacy_api_funcs *)mic_priv_dev->api;
mic_privacy_policy = mic_privacy_api->get_policy();

Expand All @@ -109,6 +118,9 @@ int mic_privacy_manager_init(void)

int mic_privacy_manager_get_policy(void)
{
if (!mic_priv_dev)
return MIC_PRIVACY_HW_MANAGED;

mic_privacy_api = (struct mic_privacy_api_funcs *)mic_priv_dev->api;

return mic_privacy_api->get_policy();
Expand Down
Loading