[Qemu-devel] [PATCH] jazz_led: fix bad snprintf

[Qemu-devel] [PATCH] jazz_led: fix bad snprintf

[Paolo Bonzini]

Detected by GCC 7's -Wformat-truncation.  snprintf writes at most
2 bytes here including the terminating NUL, so the result is
truncated.  In addition, the newline at the end is pointless.
Fix the buffer size and the format string.
---
 hw/display/jazz_led.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/display/jazz_led.c b/hw/display/jazz_led.c
index b72fdb1717..3c97d56434 100644
--- a/hw/display/jazz_led.c
+++ b/hw/display/jazz_led.c
@@ -227,13 +227,13 @@ static void jazz_led_invalidate_display(void *opaque)
 static void jazz_led_text_update(void *opaque, console_ch_t *chardata)
 {
     LedState *s = opaque;
-    char buf[2];
+    char buf[3];
 
     dpy_text_cursor(s->con, -1, -1);
     qemu_console_resize(s->con, 2, 1);
 
     /* TODO: draw the segments */
-    snprintf(buf, 2, "%02hhx\n", s->segments);
+    snprintf(buf, 3, "%02hhx", s->segments);
     console_write_ch(chardata++, ATTR2CHTYPE(buf[0], QEMU_COLOR_BLUE,
                                              QEMU_COLOR_BLACK, 1));
     console_write_ch(chardata++, ATTR2CHTYPE(buf[1], QEMU_COLOR_BLUE,
-- 
2.12.2

Re: [Qemu-devel] [Qemu-trivial] [PATCH] jazz_led: fix bad snprintf

[Laurent Vivier]

On 03/05/2017 12:44, Paolo Bonzini wrote:
> Detected by GCC 7's -Wformat-truncation.  snprintf writes at most
> 2 bytes here including the terminating NUL, so the result is
> truncated.  In addition, the newline at the end is pointless.
> Fix the buffer size and the format string.
> ---
>  hw/display/jazz_led.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/jazz_led.c b/hw/display/jazz_led.c
> index b72fdb1717..3c97d56434 100644
> --- a/hw/display/jazz_led.c
> +++ b/hw/display/jazz_led.c
> @@ -227,13 +227,13 @@ static void jazz_led_invalidate_display(void *opaque)
>  static void jazz_led_text_update(void *opaque, console_ch_t *chardata)
>  {
>      LedState *s = opaque;
> -    char buf[2];
> +    char buf[3];
>  
>      dpy_text_cursor(s->con, -1, -1);
>      qemu_console_resize(s->con, 2, 1);
>  
>      /* TODO: draw the segments */
> -    snprintf(buf, 2, "%02hhx\n", s->segments);
> +    snprintf(buf, 3, "%02hhx", s->segments);
>      console_write_ch(chardata++, ATTR2CHTYPE(buf[0], QEMU_COLOR_BLUE,
>                                               QEMU_COLOR_BLACK, 1));
>      console_write_ch(chardata++, ATTR2CHTYPE(buf[1], QEMU_COLOR_BLUE,

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [Qemu-devel] [PATCH] jazz_led: fix bad snprintf

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Detected by GCC 7's -Wformat-truncation.  snprintf writes at most
> 2 bytes here including the terminating NUL, so the result is
> truncated.  In addition, the newline at the end is pointless.
> Fix the buffer size and the format string.
> ---
>  hw/display/jazz_led.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/display/jazz_led.c b/hw/display/jazz_led.c
> index b72fdb1717..3c97d56434 100644
> --- a/hw/display/jazz_led.c
> +++ b/hw/display/jazz_led.c
> @@ -227,13 +227,13 @@ static void jazz_led_invalidate_display(void *opaque)
>  static void jazz_led_text_update(void *opaque, console_ch_t *chardata)
>  {
>      LedState *s = opaque;
> -    char buf[2];
> +    char buf[3];
>  
>      dpy_text_cursor(s->con, -1, -1);
>      qemu_console_resize(s->con, 2, 1);
>  
>      /* TODO: draw the segments */
> -    snprintf(buf, 2, "%02hhx\n", s->segments);
> +    snprintf(buf, 3, "%02hhx", s->segments);
>      console_write_ch(chardata++, ATTR2CHTYPE(buf[0], QEMU_COLOR_BLUE,
>                                               QEMU_COLOR_BLACK, 1));
>      console_write_ch(chardata++, ATTR2CHTYPE(buf[1], QEMU_COLOR_BLUE,

Since we're only every interested in the first two characters, the
truncation is totally harmless.  Thus, your patch cleans doesn't really
"fix bad snprintf", it cleans up an unclean one.  Consider rewording the
commit message accordingly.

Regardless,
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] jazz_led: fix bad snprintf

[Paolo Bonzini]

On 03/05/2017 13:38, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Detected by GCC 7's -Wformat-truncation.  snprintf writes at most
>> 2 bytes here including the terminating NUL, so the result is
>> truncated.  In addition, the newline at the end is pointless.
>> Fix the buffer size and the format string.
>> ---
>>  hw/display/jazz_led.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/display/jazz_led.c b/hw/display/jazz_led.c
>> index b72fdb1717..3c97d56434 100644
>> --- a/hw/display/jazz_led.c
>> +++ b/hw/display/jazz_led.c
>> @@ -227,13 +227,13 @@ static void jazz_led_invalidate_display(void *opaque)
>>  static void jazz_led_text_update(void *opaque, console_ch_t *chardata)
>>  {
>>      LedState *s = opaque;
>> -    char buf[2];
>> +    char buf[3];
>>  
>>      dpy_text_cursor(s->con, -1, -1);
>>      qemu_console_resize(s->con, 2, 1);
>>  
>>      /* TODO: draw the segments */
>> -    snprintf(buf, 2, "%02hhx\n", s->segments);
>> +    snprintf(buf, 3, "%02hhx", s->segments);
>>      console_write_ch(chardata++, ATTR2CHTYPE(buf[0], QEMU_COLOR_BLUE,
>>                                               QEMU_COLOR_BLACK, 1));
>>      console_write_ch(chardata++, ATTR2CHTYPE(buf[1], QEMU_COLOR_BLUE,
> 
> Since we're only every interested in the first two characters, the
> truncation is totally harmless.  Thus, your patch cleans doesn't really
> "fix bad snprintf", it cleans up an unclean one.  Consider rewording the
> commit message accordingly.

My reading is that buf[1] is always '\0' in the current code: "snprintf
writes at most 2 bytes here including the terminating NUL, so the result
is truncated".

Paolo

> Regardless,
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH] jazz_led: fix bad snprintf

[Laurent Vivier]

On 03/05/2017 13:54, Paolo Bonzini wrote:
> 
> 
> On 03/05/2017 13:38, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Detected by GCC 7's -Wformat-truncation.  snprintf writes at most
>>> 2 bytes here including the terminating NUL, so the result is
>>> truncated.  In addition, the newline at the end is pointless.
>>> Fix the buffer size and the format string.
>>> ---
>>>  hw/display/jazz_led.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/display/jazz_led.c b/hw/display/jazz_led.c
>>> index b72fdb1717..3c97d56434 100644
>>> --- a/hw/display/jazz_led.c
>>> +++ b/hw/display/jazz_led.c
>>> @@ -227,13 +227,13 @@ static void jazz_led_invalidate_display(void *opaque)
>>>  static void jazz_led_text_update(void *opaque, console_ch_t *chardata)
>>>  {
>>>      LedState *s = opaque;
>>> -    char buf[2];
>>> +    char buf[3];
>>>  
>>>      dpy_text_cursor(s->con, -1, -1);
>>>      qemu_console_resize(s->con, 2, 1);
>>>  
>>>      /* TODO: draw the segments */
>>> -    snprintf(buf, 2, "%02hhx\n", s->segments);
>>> +    snprintf(buf, 3, "%02hhx", s->segments);
>>>      console_write_ch(chardata++, ATTR2CHTYPE(buf[0], QEMU_COLOR_BLUE,
>>>                                               QEMU_COLOR_BLACK, 1));
>>>      console_write_ch(chardata++, ATTR2CHTYPE(buf[1], QEMU_COLOR_BLUE,
>>
>> Since we're only every interested in the first two characters, the
>> truncation is totally harmless.  Thus, your patch cleans doesn't really
>> "fix bad snprintf", it cleans up an unclean one.  Consider rewording the
>> commit message accordingly.
> 
> My reading is that buf[1] is always '\0' in the current code: "snprintf
> writes at most 2 bytes here including the terminating NUL, so the result
> is truncated".

I agree, from printf(3):

"int snprintf(char *str, size_t size, const char *format, ...);
...
The  functions  snprintf()  and  vsnprintf()  write  at most size bytes
(including the terminating null byte ('\0')) to str."

Laurent

Re: [Qemu-devel] [PATCH] jazz_led: fix bad snprintf

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 03/05/2017 13:38, Markus Armbruster wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>>> Detected by GCC 7's -Wformat-truncation.  snprintf writes at most
>>> 2 bytes here including the terminating NUL, so the result is
>>> truncated.  In addition, the newline at the end is pointless.
>>> Fix the buffer size and the format string.
>>> ---
>>>  hw/display/jazz_led.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/display/jazz_led.c b/hw/display/jazz_led.c
>>> index b72fdb1717..3c97d56434 100644
>>> --- a/hw/display/jazz_led.c
>>> +++ b/hw/display/jazz_led.c
>>> @@ -227,13 +227,13 @@ static void jazz_led_invalidate_display(void *opaque)
>>>  static void jazz_led_text_update(void *opaque, console_ch_t *chardata)
>>>  {
>>>      LedState *s = opaque;
>>> -    char buf[2];
>>> +    char buf[3];
>>>  
>>>      dpy_text_cursor(s->con, -1, -1);
>>>      qemu_console_resize(s->con, 2, 1);
>>>  
>>>      /* TODO: draw the segments */
>>> -    snprintf(buf, 2, "%02hhx\n", s->segments);
>>> +    snprintf(buf, 3, "%02hhx", s->segments);
>>>      console_write_ch(chardata++, ATTR2CHTYPE(buf[0], QEMU_COLOR_BLUE,
>>>                                               QEMU_COLOR_BLACK, 1));
>>>      console_write_ch(chardata++, ATTR2CHTYPE(buf[1], QEMU_COLOR_BLUE,
>> 
>> Since we're only every interested in the first two characters, the
>> truncation is totally harmless.  Thus, your patch cleans doesn't really
>> "fix bad snprintf", it cleans up an unclean one.  Consider rewording the
>> commit message accordingly.
>
> My reading is that buf[1] is always '\0' in the current code: "snprintf
> writes at most 2 bytes here including the terminating NUL, so the result
> is truncated".

You're right; I forgot that snprintf() always adds a NUL.  So this *is*
broken: we write NUL instead of the second digit.  Mentioning this in
the commit message wouldn't hurt.

>
> Paolo
>
>> Regardless,
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> 

Re: [Qemu-devel] [PATCH] jazz_led: fix bad snprintf

[Paolo Bonzini]

On 03/05/2017 14:56, Markus Armbruster wrote:
>> snprintf writes at most 2 bytes here including the terminating NUL, so
>> the result is truncated".
>
> You're right; I forgot that snprintf() always adds a NUL.  So this *is*
> broken: we write NUL instead of the second digit.  Mentioning this in
> the commit message wouldn't hurt.

Well, it does, see the quote. :)

Paolo

Re: [Qemu-devel] [PATCH] jazz_led: fix bad snprintf

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 03/05/2017 14:56, Markus Armbruster wrote:
>>> snprintf writes at most 2 bytes here including the terminating NUL, so
>>> the result is truncated".
>>
>> You're right; I forgot that snprintf() always adds a NUL.  So this *is*
>> broken: we write NUL instead of the second digit.  Mentioning this in
>> the commit message wouldn't hurt.
>
> Well, it does, see the quote. :)

Yes, snprintf() truncates, but that's *internal* to
jazz_led_text_update().  The *external* effect isn't quite truncation,
it's writing NUL to the console instead of the second digit.  That's
truncation only if the console ignores NUL.  It might; I don't know.
Anyway, no big deal.

Re: [Qemu-devel] [PATCH] jazz_led: fix bad snprintf

[Michael Tokarev]

03.05.2017 13:44, Paolo Bonzini wrote:
> Detected by GCC 7's -Wformat-truncation.  snprintf writes at most
> 2 bytes here including the terminating NUL, so the result is
> truncated.  In addition, the newline at the end is pointless.
> Fix the buffer size and the format string.

Polo, that's quite a bit too bureaucratic,
but where's your s-o-b for this patch? :)

Thanks,

/mjt

Re: [Qemu-devel] [PATCH] jazz_led: fix bad snprintf

[Paolo Bonzini]

On 05/05/2017 08:24, Michael Tokarev wrote:
> 03.05.2017 13:44, Paolo Bonzini wrote:
>> Detected by GCC 7's -Wformat-truncation.  snprintf writes at most
>> 2 bytes here including the terminating NUL, so the result is
>> truncated.  In addition, the newline at the end is pointless.
>> Fix the buffer size and the format string.
> 
> Polo, that's quite a bit too bureaucratic,
> but where's your s-o-b for this patch? :)

Oops, here:

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

> 
> Thanks,
> 
> /mjt
> 

Re: [Qemu-devel] [PATCH] jazz_led: fix bad snprintf

[Michael Tokarev]

03.05.2017 13:44, Paolo Bonzini wrote:
> Detected by GCC 7's -Wformat-truncation.  snprintf writes at most
> 2 bytes here including the terminating NUL, so the result is
> truncated.  In addition, the newline at the end is pointless.
> Fix the buffer size and the format string.

Applied to -trivial, thanks!

/mjt