Re: [PATCH 1/2] audio/jackaudio: Avoid dynamic stack allocation in qjack_client_init
On Mon, 21 Aug 2023 at 09:01, Christian Schoenebeck
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
> > ---
> > 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
thanks
-- PMM
Re: [PATCH 1/2] audio/jackaudio: Avoid dynamic stack allocation in qjack_client_init
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
> ---
> 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
Best regards,
Christian Schoenebeck
Re: [PATCH 1/2] audio/jackaudio: Avoid dynamic stack allocation in qjack_client_init
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
Reviewed-by: Francisco Iglesias
> ---
> 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
>
>
[PATCH 1/2] audio/jackaudio: Avoid dynamic stack allocation in qjack_client_init
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
---
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
