diff mbox series

[01/12] ui/console-vc: Replace sprintf() by g_strdup_printf()

Message ID 20240410160614.90627-2-philmd@linaro.org
State New
Headers show
Series misc: Remove sprintf() due to macOS deprecation | expand

Commit Message

Philippe Mathieu-Daudé April 10, 2024, 4:06 p.m. UTC
sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
resulting in painful developper experience.

Replace sprintf() by g_strdup_printf() in order to avoid:

  [702/1310] Compiling C object libcommon.fa.p/ui_console-vc.c.o
  ui/console-vc.c:824:21: warning: 'sprintf' is deprecated:
    This function is provided for compatibility reasons only.
    Due to security concerns inherent in the design of sprintf(3),
    it is highly recommended that you use snprintf(3) instead.
    [-Wdeprecated-declarations]
                    sprintf(response, "\033[%d;%dR",
                    ^
  1 warning generated.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 ui/console-vc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Marc-André Lureau April 11, 2024, 6:58 a.m. UTC | #1
On Wed, Apr 10, 2024 at 8:06 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> sprintf() is deprecated on Darwin since macOS 13.0 / XCode 14.1,
> resulting in painful developper experience.
>
> Replace sprintf() by g_strdup_printf() in order to avoid:
>
>   [702/1310] Compiling C object libcommon.fa.p/ui_console-vc.c.o
>   ui/console-vc.c:824:21: warning: 'sprintf' is deprecated:
>     This function is provided for compatibility reasons only.
>     Due to security concerns inherent in the design of sprintf(3),
>     it is highly recommended that you use snprintf(3) instead.
>     [-Wdeprecated-declarations]
>                     sprintf(response, "\033[%d;%dR",
>                     ^
>   1 warning generated.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

> ---
>  ui/console-vc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ui/console-vc.c b/ui/console-vc.c
> index 899fa11c94..b455db436d 100644
> --- a/ui/console-vc.c
> +++ b/ui/console-vc.c
> @@ -648,7 +648,7 @@ static void vc_putchar(VCChardev *vc, int ch)
>      QemuTextConsole *s = vc->console;
>      int i;
>      int x, y;
> -    char response[40];
> +    g_autofree char *response = NULL;
>
>      switch(vc->state) {
>      case TTY_STATE_NORM:
> @@ -821,7 +821,7 @@ static void vc_putchar(VCChardev *vc, int ch)
>                      break;
>                  case 6:
>                      /* report cursor position */
> -                    sprintf(response, "\033[%d;%dR",
> +                    response = g_strdup_printf("\033[%d;%dR",
>                             (s->y_base + s->y) % s->total_height + 1,
>                              s->x + 1);
>                      vc_respond_str(vc, response);
> --
> 2.41.0
>
Gerd Hoffmann April 11, 2024, 7:47 a.m. UTC | #2
Hi,

>     Due to security concerns inherent in the design of sprintf(3),
>     it is highly recommended that you use snprintf(3) instead.

> -    char response[40];
> +    g_autofree char *response = NULL;

> -                    sprintf(response, "\033[%d;%dR",
> +                    response = g_strdup_printf("\033[%d;%dR",

Any specific reason why you don't go with the recommendation above?

While using g_strdup_printf() isn't wrong it allocates memory which
is not needed here because you can continue to use the stack buffer
this way:

	snprintf(response, sizeof(response), ...);

take care,
  Gerd
Philippe Mathieu-Daudé April 11, 2024, 9:36 a.m. UTC | #3
On 11/4/24 09:47, Gerd Hoffmann wrote:
>    Hi,
> 
>>      Due to security concerns inherent in the design of sprintf(3),
>>      it is highly recommended that you use snprintf(3) instead.
> 
>> -    char response[40];
>> +    g_autofree char *response = NULL;
> 
>> -                    sprintf(response, "\033[%d;%dR",
>> +                    response = g_strdup_printf("\033[%d;%dR",
> 
> Any specific reason why you don't go with the recommendation above?
> 
> While using g_strdup_printf() isn't wrong it allocates memory which
> is not needed here because you can continue to use the stack buffer
> this way:
> 
> 	snprintf(response, sizeof(response), ...);

I thought GLib/GString was recommended for formatting, so choose
this thinking mostly about style. Indeed in this case snprintf()
is sufficient. I'll respin, thanks.

> 
> take care,
>    Gerd
>
Gerd Hoffmann April 11, 2024, 10:02 a.m. UTC | #4
On Thu, Apr 11, 2024 at 11:36:10AM +0200, Philippe Mathieu-Daudé wrote:
> On 11/4/24 09:47, Gerd Hoffmann wrote:
> >    Hi,
> > 
> > >      Due to security concerns inherent in the design of sprintf(3),
> > >      it is highly recommended that you use snprintf(3) instead.
> > 
> > > -    char response[40];
> > > +    g_autofree char *response = NULL;
> > 
> > > -                    sprintf(response, "\033[%d;%dR",
> > > +                    response = g_strdup_printf("\033[%d;%dR",
> > 
> > Any specific reason why you don't go with the recommendation above?
> > 
> > While using g_strdup_printf() isn't wrong it allocates memory which
> > is not needed here because you can continue to use the stack buffer
> > this way:
> > 
> > 	snprintf(response, sizeof(response), ...);
> 
> I thought GLib/GString was recommended for formatting,

If you allocate the output buffer anyway (and there are patches in this
series where this is the case) it's clearly better to use
g_strdup_printf instead of malloc + snprintf.

In case a fixed-size buffer can be used I wouldn't switch to dynamic
allocation ...

take care,
  Gerd
diff mbox series

Patch

diff --git a/ui/console-vc.c b/ui/console-vc.c
index 899fa11c94..b455db436d 100644
--- a/ui/console-vc.c
+++ b/ui/console-vc.c
@@ -648,7 +648,7 @@  static void vc_putchar(VCChardev *vc, int ch)
     QemuTextConsole *s = vc->console;
     int i;
     int x, y;
-    char response[40];
+    g_autofree char *response = NULL;
 
     switch(vc->state) {
     case TTY_STATE_NORM:
@@ -821,7 +821,7 @@  static void vc_putchar(VCChardev *vc, int ch)
                     break;
                 case 6:
                     /* report cursor position */
-                    sprintf(response, "\033[%d;%dR",
+                    response = g_strdup_printf("\033[%d;%dR",
                            (s->y_base + s->y) % s->total_height + 1,
                             s->x + 1);
                     vc_respond_str(vc, response);