Re: [igt-dev] [PATCH i-g-t v2 6/9] tests/kms_chamelium: add name parameter to audio_state_start

Martin Peres
2019-05-27T12:29:53Z

On 24/05/2019 18:03, Simon Ser wrote:
> This identifies the audio test name. This is required for adding multiple
> audio tests.
>
> Signed-off-by: Simon Ser <simon.ser@intel.com>
> ---
> tests/kms_chamelium.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> index 063840721f46..14262831c3ff 100644
> --- a/tests/kms_chamelium.c
> +++ b/tests/kms_chamelium.c
> @@ -824,6 +824,7 @@ struct audio_state {
> int rate;
> } playback, capture;
>
> + const char *name;
> struct audio_signal *signal;
> int channel_mapping[8];
>
> @@ -871,7 +872,7 @@ static void *run_audio_thread(void *data)
> return NULL;
> }
>
> -static void audio_state_start(struct audio_state *state)
> +static void audio_state_start(struct audio_state *state, const char *name)
> {
> int ret;
> bool ok;
> @@ -879,12 +880,13 @@ static void audio_state_start(struct audio_state *state)
> enum chamelium_stream_realtime_mode stream_mode;
> char dump_suffix[64];
>
> + state->name = name;

This is a little dangerous since the name could have been allocated on
the stack of the caller, which could lead to a use after free later on.

With this replaced with strdup and a free(state->name) in the fini
function, the patch is:

Reviewed-by: Martin Peres <martin.peres@linux.intel.com>

> state->recv_pages = 0;
> state->msec = 0;
>
> - igt_debug("Starting test with playback format %s, sampling rate %d Hz "
> - "and %d channels\n",
> - snd_pcm_format_name(state->playback.format),
> + igt_debug("Starting %s test with playback format %s, "
> + "sampling rate %d Hz and %d channels\n",
> + name, snd_pcm_format_name(state->playback.format),
> state->playback.rate, state->playback.channels);
>
> chamelium_start_capturing_audio(state->chamelium, state->port, false);
> @@ -930,8 +932,8 @@ static void audio_state_start(struct audio_state *state)
>
> if (igt_frame_dump_is_enabled()) {
> snprintf(dump_suffix, sizeof(dump_suffix),
> - "capture-%s-%dch-%dHz",
> - snd_pcm_format_name(state->playback.format),
> + "capture-%s-%s-%dch-%dHz",
> + name, snd_pcm_format_name(state->playback.format),
> state->playback.channels, state->playback.rate);
>
> state->dump_fd = audio_create_wav_file_s32_le(dump_suffix,
> @@ -1001,9 +1003,9 @@ static void audio_state_stop(struct audio_state *state, bool success)
> state->dump_path = NULL;
> }
>
> - igt_debug("Audio test result for format %s, sampling rate %d Hz and "
> - "%d channels: %s\n",
> - snd_pcm_format_name(state->playback.format),
> + igt_debug("Audio %s test result for format %s, sampling rate %d Hz "
> + "and %d channels: %s\n",
> + state->name, snd_pcm_format_name(state->playback.format),
> state->playback.rate, state->playback.channels,
> success ? "ALL GREEN" : "FAILED");
> }
> @@ -1062,7 +1064,7 @@ static bool test_audio_frequencies(struct audio_state *state)
> audio_output_frequencies_callback, state,
> PLAYBACK_SAMPLES);
>
> - audio_state_start(state);
> + audio_state_start(state, "frequencies");
>
> igt_assert(state->capture.rate == state->playback.rate);
>
>