[PATCH] ui/console-vc: Silence warning about sprintf() on OpenBSD

[PATCH] ui/console-vc: Silence warning about sprintf() on OpenBSD

[Thomas Huth]

The linker on OpenBSD complains:

 ld: warning: console-vc.c:824 (../src/ui/console-vc.c:824)([...]):
 warning: sprintf() is often misused, please use snprintf()

Using snprintf() is certainly better here, so let's switch to that
function instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 ui/console-vc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ui/console-vc.c b/ui/console-vc.c
index 8393d532e7..336a1520eb 100644
--- a/ui/console-vc.c
+++ b/ui/console-vc.c
@@ -821,9 +821,9 @@ static void vc_putchar(VCChardev *vc, int ch)
                     break;
                 case 6:
                     /* report cursor position */
-                    sprintf(response, "\033[%d;%dR",
-                           (s->y_base + s->y) % s->total_height + 1,
-                            s->x + 1);
+                    snprintf(response, sizeof(response), "\033[%d;%dR",
+                             (s->y_base + s->y) % s->total_height + 1,
+                             s->x + 1);
                     vc_respond_str(vc, response);
                     break;
                 }
-- 
2.46.1

Re: [PATCH] ui/console-vc: Silence warning about sprintf() on OpenBSD

[Daniel P. Berrangé]

On Mon, Oct 14, 2024 at 05:10:23PM +0200, Thomas Huth wrote:
> The linker on OpenBSD complains:
> 
>  ld: warning: console-vc.c:824 (../src/ui/console-vc.c:824)([...]):
>  warning: sprintf() is often misused, please use snprintf()
> 
> Using snprintf() is certainly better here, so let's switch to that
> function instead.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  ui/console-vc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/ui/console-vc.c b/ui/console-vc.c
> index 8393d532e7..336a1520eb 100644
> --- a/ui/console-vc.c
> +++ b/ui/console-vc.c
> @@ -821,9 +821,9 @@ static void vc_putchar(VCChardev *vc, int ch)
>                      break;
>                  case 6:
>                      /* report cursor position */
> -                    sprintf(response, "\033[%d;%dR",
> -                           (s->y_base + s->y) % s->total_height + 1,
> -                            s->x + 1);
> +                    snprintf(response, sizeof(response), "\033[%d;%dR",
> +                             (s->y_base + s->y) % s->total_height + 1,
> +                             s->x + 1);
>                      vc_respond_str(vc, response);

These two lines are the only place in the code that uses the

   char response[40];

so even better than switching to snprintf, how about just taking
buffer size out of the picture:

  g_autofree *response =
      g_strdup_printf("\033[%d;%dR",
                      (s->y_base + s->y) % s->total_height + 1,
                      s->x + 1);
  vc_respond_str(vc, response);


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] ui/console-vc: Silence warning about sprintf() on OpenBSD

[Michael Tokarev]

On 14.10.2024 18:15, Daniel P. Berrangé wrote:

> These two lines are the only place in the code that uses the
> 
>     char response[40];
> 
> so even better than switching to snprintf, how about just taking
> buffer size out of the picture:
> 
>    g_autofree *response =
>        g_strdup_printf("\033[%d;%dR",
>                        (s->y_base + s->y) % s->total_height + 1,
>                        s->x + 1);
>    vc_respond_str(vc, response);

What's the reason to perform memory allocation in trivial places
like this?  If we're worrying about possible buffer size issue,
maybe asprintf() is a better alternative for such small things?
Fragmenting heap memory for no reason seems too much overkill.
But I'm old-scool, so.. :)

/mjt

Re: [PATCH] ui/console-vc: Silence warning about sprintf() on OpenBSD

[Alex Bennée]

Michael Tokarev <mjt@tls.msk.ru> writes:

> On 14.10.2024 18:15, Daniel P. Berrangé wrote:
>
>> These two lines are the only place in the code that uses the
>>     char response[40];
>> so even better than switching to snprintf, how about just taking
>> buffer size out of the picture:
>>    g_autofree *response =
>>        g_strdup_printf("\033[%d;%dR",
>>                        (s->y_base + s->y) % s->total_height + 1,
>>                        s->x + 1);
>>    vc_respond_str(vc, response);
>
> What's the reason to perform memory allocation in trivial places
> like this?  If we're worrying about possible buffer size issue,
> maybe asprintf() is a better alternative for such small things?
> Fragmenting heap memory for no reason seems too much overkill.
> But I'm old-scool, so.. :)

I doubt the allocate/free pair will cause much fragmentation but it
doesn't look like we are in any hot path here. Anyway snprintf is
certainly better than sprintf so:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

>
> /mjt

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] ui/console-vc: Silence warning about sprintf() on OpenBSD

[Daniel P. Berrangé]

On Mon, Oct 14, 2024 at 10:50:44PM +0300, Michael Tokarev wrote:
> On 14.10.2024 18:15, Daniel P. Berrangé wrote:
> 
> > These two lines are the only place in the code that uses the
> > 
> >     char response[40];
> > 
> > so even better than switching to snprintf, how about just taking
> > buffer size out of the picture:
> > 
> >    g_autofree *response =
> >        g_strdup_printf("\033[%d;%dR",
> >                        (s->y_base + s->y) % s->total_height + 1,
> >                        s->x + 1);
> >    vc_respond_str(vc, response);
> 
> What's the reason to perform memory allocation in trivial places
> like this?  If we're worrying about possible buffer size issue,
> maybe asprintf() is a better alternative for such small things?
> Fragmenting heap memory for no reason seems too much overkill.
> But I'm old-scool, so.. :)

This is not a performance sensitive path, and using g_strdup_printf
makes it robust against any futher changes in the future. In the
context of all the memory allocation QEMU does, I can't see this
making any difference to heap fragmentation whatsoever.

snprintf with fixed buffers should only be used where there's a
demonstratable performance win, and the return value actually
checked with an assert() to prove we're not overflowing.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] ui/console-vc: Silence warning about sprintf() on OpenBSD

[Thomas Huth]

On 15/10/2024 10.14, Daniel P. Berrangé wrote:
> On Mon, Oct 14, 2024 at 10:50:44PM +0300, Michael Tokarev wrote:
>> On 14.10.2024 18:15, Daniel P. Berrangé wrote:
>>
>>> These two lines are the only place in the code that uses the
>>>
>>>      char response[40];
>>>
>>> so even better than switching to snprintf, how about just taking
>>> buffer size out of the picture:
>>>
>>>     g_autofree *response =
>>>         g_strdup_printf("\033[%d;%dR",
>>>                         (s->y_base + s->y) % s->total_height + 1,
>>>                         s->x + 1);
>>>     vc_respond_str(vc, response);
>>
>> What's the reason to perform memory allocation in trivial places
>> like this?  If we're worrying about possible buffer size issue,
>> maybe asprintf() is a better alternative for such small things?
>> Fragmenting heap memory for no reason seems too much overkill.
>> But I'm old-scool, so.. :)
> 
> This is not a performance sensitive path, and using g_strdup_printf
> makes it robust against any futher changes in the future. In the
> context of all the memory allocation QEMU does, I can't see this
> making any difference to heap fragmentation whatsoever.
> 
> snprintf with fixed buffers should only be used where there's a
> demonstratable performance win, and the return value actually
> checked with an assert() to prove we're not overflowing.

While I'm obviously old-schooled, too (since I used snprintf here in the 
first place), I agree with Daniel that the few cycles of improved 
performance likely aren't justified in this case here, so g_strdup_printf() 
is a better choice here indeed. I just sent a v2 with that change.

  Thomas

Re: [PATCH] ui/console-vc: Silence warning about sprintf() on OpenBSD

[Marc-André Lureau]

On Mon, Oct 14, 2024 at 7:10 PM Thomas Huth <thuth@redhat.com> wrote:
>
> The linker on OpenBSD complains:
>
>  ld: warning: console-vc.c:824 (../src/ui/console-vc.c:824)([...]):
>  warning: sprintf() is often misused, please use snprintf()
>
> Using snprintf() is certainly better here, so let's switch to that
> function instead.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  ui/console-vc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/ui/console-vc.c b/ui/console-vc.c
> index 8393d532e7..336a1520eb 100644
> --- a/ui/console-vc.c
> +++ b/ui/console-vc.c
> @@ -821,9 +821,9 @@ static void vc_putchar(VCChardev *vc, int ch)
>                      break;
>                  case 6:
>                      /* report cursor position */
> -                    sprintf(response, "\033[%d;%dR",
> -                           (s->y_base + s->y) % s->total_height + 1,
> -                            s->x + 1);
> +                    snprintf(response, sizeof(response), "\033[%d;%dR",
> +                             (s->y_base + s->y) % s->total_height + 1,
> +                             s->x + 1);
>                      vc_respond_str(vc, response);
>                      break;
>                  }
> --
> 2.46.1
>