| Message ID | 20170503104441.1349-1-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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>
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> >
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
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> >>
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
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.
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
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 >
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
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,