| Message ID | 20230818155846.1651287-2-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | audio/jackaudio: avoid dynamic stack allocations | expand |
On [2023 Aug 18] Fri 16:58:45, 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). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Francisco Iglesias <frasse.iglesias@gmail.com> > --- > 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 */ > + 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()); > > -- > 2.34.1 > >
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
On Mon, 21 Aug 2023 at 09:01, Christian Schoenebeck <qemu_oss@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.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. 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_oss@crudebyte.com> thanks -- PMM
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 */ + 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());
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). Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- audio/jackaudio.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)