Audio modules fixes#10752
Conversation
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>
lgirdwood
requested review from
dbaluta,
kv2019i,
lbetlej,
mmaka1 and
plbossart
as code owners
There was a problem hiding this comment.
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. |
| if (!device_is_ready(mic_priv_dev)) { | ||
| LOG_ERR("mic_privacy device not ready"); | ||
| return -ENODEV; | ||
| } |
There was a problem hiding this comment.
This is a valid concern. Maybe alternative is to drop mic privacy when running the ztest?
| 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; | ||
| } |
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
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).
| if (!device_is_ready(mic_priv_dev)) { | ||
| LOG_ERR("mic_privacy device not ready"); | ||
| return -ENODEV; | ||
| } |
There was a problem hiding this comment.
This is a valid concern. Maybe alternative is to drop mic privacy when running the ztest?
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
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?
| struct comp_dev *dev = mod->dev; | ||
|
|
||
| comp_dbg(dev, "entry"); | ||
| comp_info(mod->dev, "asrc_free() entry"); |
There was a problem hiding this comment.
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.
| comp_info(mod->dev, "asrc_free() entry"); | ||
|
|
||
| if (!cd) | ||
| return 0; |
There was a problem hiding this comment.
can it be called with cd == NULL? Don't think so, .free() is probably only called when .init() was successful
| mod_free(mod, cd->asrc_obj); | ||
| cd->buf = NULL; | ||
|
|
||
| if (cd->asrc_obj) { |
| cd->asrc_obj = NULL; | ||
| } | ||
|
|
||
| if (cd->buf) { |
There was a problem hiding this comment.
same for the above 2 added checks
| 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) { |
There was a problem hiding this comment.
can this be hit in real pipelines or only in ztests?
Some small fixes picked up by the audio ztest UTs.