Re: [PATCH 1/2] audio/jackaudio: Avoid dynamic stack allocation in qjack_client_init

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Friday, August 18, 2023 5:58:45 PM CEST Peter Maydell wrote:
> Avoid a dynamic stack allocation in qjack_client_init(), by using
> a g_autofree heap allocation instead.
> 
> (We stick with allocate + snprintf() because the JACK API requires
> the name to be no more than its maximum size, so g_strdup_printf()
> would require an extra truncation step.)
> 
> The codebase has very few VLAs, and if we can get rid of them all we
> can make the compiler error on new additions.  This is a defensive
> measure against security bugs where an on-stack dynamic allocation
> isn't correctly size-checked (e.g.  CVE-2021-3527).

Sounds good, what compiler flag will that be?

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  audio/jackaudio.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/audio/jackaudio.c b/audio/jackaudio.c
> index 5bdf3d7a78d..7cb2a49f971 100644
> --- a/audio/jackaudio.c
> +++ b/audio/jackaudio.c
> @@ -400,7 +400,8 @@ static void qjack_client_connect_ports(QJackClient *c)
>  static int qjack_client_init(QJackClient *c)
>  {
>      jack_status_t status;
> -    char client_name[jack_client_name_size()];
> +    int client_name_len = jack_client_name_size(); /* includes NUL */

I would add `const` here.

> +    g_autofree char *client_name = g_new(char, client_name_len);
>      jack_options_t options = JackNullOption;
>  
>      if (c->state == QJACK_STATE_RUNNING) {
> @@ -409,7 +410,7 @@ static int qjack_client_init(QJackClient *c)
>  
>      c->connect_ports = true;
>  
> -    snprintf(client_name, sizeof(client_name), "%s-%s",
> +    snprintf(client_name, client_name_len, "%s-%s",
>          c->out ? "out" : "in",
>          c->opt->client_name ? c->opt->client_name : audio_application_name());

Unrelated, but this could be shortened by Elvis operator BTW:

    c->opt->client_name ?: audio_application_name()

Anyway:

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

Best regards,
Christian Schoenebeck