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

Bilal, Mohammed
2026-03-04T17:36:08Z

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.

> > +
> > + 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