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

[Sebastian Brzezinka <sebastian.brzezinka@intel.com>]

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