Re: [PATCH v1 2/2] tests/chamelium/audio: Fix resource leaks on assertion failure

Sebastian Brzezinka
2026-03-05T06:55:26Z

On Wed Mar 4, 2026 at 6:36 PM CET, Mohammed Bilal wrote:
> Hi Sebastien ,
>
>> -----Original Message-----
>> From: Brzezinka, Sebastian <sebastian.brzezinka@intel.com>
>> Sent: 04 March 2026 17:48
>> To: Bilal, Mohammed <mohammed.bilal@intel.com>; igt-
>> dev@lists.freedesktop.org
>> Cc: B, Jeevan <jeevan.b@intel.com>; Joshi, Kunal1 <kunal1.joshi@intel.com>;
>> Brzezinka, Sebastian <sebastian.brzezinka@intel.com>
>> Subject: Re: [PATCH v1 2/2] tests/chamelium/audio: Fix resource leaks on
>> assertion failure
>>
>> On Wed Mar 4, 2026 at 11:05 AM CET, Mohammed Bilal wrote:
>> > Fatal assertions can bypass cleanup paths, causing resources to remain
>> > unreleased when failures occur during audio tests.
>> >
>> > Replace fatal assertions with non-fatal checks and route failures
>> > through a common cleanup path to ensure proper cleanup.
>> >
>> > Signed-off-by: Mohammed Bilal <mohammed.bilal@intel.com>
>> > ---
>> > tests/chamelium/kms_chamelium_audio.c | 43
>> > ++++++++++++++++++++-------
>> > 1 file changed, 33 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/tests/chamelium/kms_chamelium_audio.c
>> > b/tests/chamelium/kms_chamelium_audio.c
>> > index 2967e3c50..41cbe92d9 100644
>> > --- a/tests/chamelium/kms_chamelium_audio.c
>> > +++ b/tests/chamelium/kms_chamelium_audio.c
>> > @@ -426,9 +426,21 @@ static bool test_audio_frequencies(struct
>> > audio_state *state)
>> >
>> > audio_state_start(state, "frequencies");
>> >
>> > - igt_assert_f(state->capture.rate == state->playback.rate,
>> > - "Capture rate (%dHz) doesn't match playback rate (%dHz)\n",
>> > - state->capture.rate, state->playback.rate);
>> > + /* Initialize heap pointers to NULL so the cleanup path can safely
>> > + * free them regardless of where an error occurs.
>> > + */
>> > + channel = NULL;
>> > + buf = NULL;
>> > + recv = NULL;
>> > + recv_len = 0;
>> > + success = false;
>> It feels a bit messy to assign NULL here, it's cleaner at declaration. Plus, we don't
>> really need the goto logic for an input check, the original assert was enough since
>> it's just verifying the function's parameters. Ideally, this whole block should be
>> moved to the beginning of the function, before any init.
>>
>
> Thanks for the suggestion on moving NULL inits to declarations . I will address that in v2.
>
> Regarding the capture rate check we can't move it before audio_state_start() because the capture rate is only available after audio playback begins. There's even a comment in audio_state_start() regarding this .
>
> By the time we can check the rate, audio_state_start() has already launched a pthread and started the chamelium stream/capture. If we use igt_assert_f here and it fails, siglongjmp will skip audio_state_stop() leaving the pthread running and the stream/capture active. That's the exact resource leak this patch is fixing.
You’re absolutely right. I totally missed audio_state_start().
>
>> > +
>> > + if (state->capture.rate != state->playback.rate) {
>> > + igt_critical("Capture rate (%dHz) doesn't match playback "
>> > + "rate (%dHz)\n",
>> > + state->capture.rate, state->playback.rate);
>> > + goto out;
>> > + }
>> >
>> > /* Needs to be a multiple of 128, because that's the number of samples
>> > * we get per channel each time we receive an audio page from the @@
>> > -447,10 +459,6 @@ static bool test_audio_frequencies(struct audio_state
>> *state)
>> > buf = malloc(sizeof(int32_t) * buf_cap);
>> > buf_len = 0;
>> >
>> > - recv = NULL;
>> > - recv_len = 0;
>> > -
>> > - success = false;
>> > streak = 0;
>> > while (!success && state->msec < AUDIO_TIMEOUT) {
>> > audio_state_receive(state, &recv, &recv_len); @@ -460,13
>> +468,21 @@
>> > static bool test_audio_frequencies(struct audio_state *state)
>> >
>> > if (buf_len < buf_cap)
>> > continue;
>> > - igt_assert(buf_len == buf_cap);
>> > + if (buf_len != buf_cap) {
>> I see why you're using goto for the cleanup path, but let's tweak the condition so
>> it’s easier to read at a glance basically, failing if buf_len exceeds buf_cap. We
>> could also handle this by checking the divisibility of recv_len and buf_cap instead.
>>
>
> Good point . I'll change this to buf_len > buf_cap in v2 to make the overflow intent explicit. The buf_len < buf_cap case is already handled by the continue above, so by the time we reach this check only overflow is possible.
>
>> --
>> Best regards,
>> Sebastian
>
> Thanks ,
> Mohammed Bilal

--
Best regards,
Sebastian