On Mon, 21 Aug 2023 at 09:01, Christian Schoenebeck
<qemu_...@crudebyte.com> wrote:
>
> 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?
It's "-Wvla".
> > Signed-off-by: Peter Maydell <peter.mayd...@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.
Sure, I can do this. (I tend not to use 'const' except when
dealing with pointers, personally, but that's just habit.)
> > + 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_...@crudebyte.com>
thanks
-- PMM
