| Message ID | 20231120214520.59431-1-pbonzini@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] disas/cris: Pass buffer size to format_dec() to avoid overflow warning | expand |
On 2023/11/21 6:45, Paolo Bonzini wrote: > From: Philippe Mathieu-Daudé <philmd@linaro.org> > > Propagate the buffer size to format_dec() and use snprintf(). > > This should silence this UBSan -Wformat-overflow warning: > > In file included from /usr/include/stdio.h:906, > from include/qemu/osdep.h:114, > from ../disas/cris.c:21: > In function 'sprintf', > inlined from 'format_dec' at ../disas/cris.c:1737:3, > inlined from 'print_with_operands' at ../disas/cris.c:2477:12, > inlined from 'print_insn_cris_generic.constprop' at ../disas/cris.c:2690:8: > /usr/include/bits/stdio2.h:30:10: warning: null destination pointer [-Wformat-overflow=] > 30 | return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 31 | __glibc_objsize (__s), __fmt, > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 32 | __va_arg_pack ()); > | ~~~~~~~~~~~~~~~~~ > > Reported-by: Akihiko Odaki <akihiko.odaki@daynix.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Message-ID: <20231120132222.82138-1-philmd@linaro.org> > [Rewritten to fix logic and avoid repeated expression. - Paolo] > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > disas/cris.c | 26 ++++++++++++++++---------- > 1 file changed, 16 insertions(+), 10 deletions(-) > > diff --git a/disas/cris.c b/disas/cris.c > index 0b0a3fb9165..409a224c5d1 100644 > --- a/disas/cris.c > +++ b/disas/cris.c > @@ -1731,10 +1731,10 @@ format_hex (unsigned long number, > unsigned (== 0). */ > > static char * > -format_dec (long number, char *outbuffer, int signedp) > +format_dec (long number, char *outbuffer, size_t outsize, int signedp) > { > last_immediate = number; > - sprintf (outbuffer, signedp ? "%ld" : "%lu", number); > + snprintf (outbuffer, outsize, signedp ? "%ld" : "%lu", number); > > return outbuffer + strlen (outbuffer); > } > @@ -1876,6 +1876,12 @@ print_flags (struct cris_disasm_data *disdata, unsigned int insn, char *cp) > return cp; > } > > +#define FORMAT_DEC(number, tp, signedp) \ > + format_dec (number, tp, ({ \ Nit: the slash at the end is misaligned. Otherwise, Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com> > + assert(tp >= temp && tp <= temp + sizeof(temp)); \ > + temp + sizeof(temp) - tp; \ > + }), signedp) > + > /* Print out an insn with its operands, and update the info->insn_type > fields. The prefix_opcodep and the rest hold a prefix insn that is > supposed to be output as an address mode. */ > @@ -2105,7 +2111,7 @@ print_with_operands (const struct cris_opcode *opcodep, > if ((*cs == 'z' && (insn & 0x20)) > || (opcodep->match == BDAP_QUICK_OPCODE > && (nbytes <= 2 || buffer[1 + nbytes] == 0))) > - tp = format_dec (number, tp, signedp); > + tp = FORMAT_DEC (number, tp, signedp); > else > { > unsigned int highbyte = (number >> 24) & 0xff; > @@ -2241,7 +2247,7 @@ print_with_operands (const struct cris_opcode *opcodep, > with_reg_prefix); > if (number >= 0) > *tp++ = '+'; > - tp = format_dec (number, tp, 1); > + tp = FORMAT_DEC (number, tp, 1); > > info->flags |= CRIS_DIS_FLAG_MEM_TARGET_IS_REG; > info->target = (prefix_insn >> 12) & 15; > @@ -2340,7 +2346,7 @@ print_with_operands (const struct cris_opcode *opcodep, > { > if (number >= 0) > *tp++ = '+'; > - tp = format_dec (number, tp, 1); > + tp = FORMAT_DEC (number, tp, 1); > } > } > else > @@ -2397,7 +2403,7 @@ print_with_operands (const struct cris_opcode *opcodep, > break; > > case 'I': > - tp = format_dec (insn & 63, tp, 0); > + tp = FORMAT_DEC (insn & 63, tp, 0); > break; > > case 'b': > @@ -2426,11 +2432,11 @@ print_with_operands (const struct cris_opcode *opcodep, > break; > > case 'c': > - tp = format_dec (insn & 31, tp, 0); > + tp = FORMAT_DEC (insn & 31, tp, 0); > break; > > case 'C': > - tp = format_dec (insn & 15, tp, 0); > + tp = FORMAT_DEC (insn & 15, tp, 0); > break; > > case 'o': > @@ -2463,7 +2469,7 @@ print_with_operands (const struct cris_opcode *opcodep, > if (number > 127) > number = number - 256; > > - tp = format_dec (number, tp, 1); > + tp = FORMAT_DEC (number, tp, 1); > *tp++ = ','; > tp = format_reg (disdata, (insn >> 12) & 15, tp, with_reg_prefix); > } > @@ -2474,7 +2480,7 @@ print_with_operands (const struct cris_opcode *opcodep, > break; > > case 'i': > - tp = format_dec ((insn & 32) ? (insn & 31) | ~31L : insn & 31, tp, 1); > + tp = FORMAT_DEC ((insn & 32) ? (insn & 31) | ~31L : insn & 31, tp, 1); > break; > > case 'P':
On 20/11/23 22:45, Paolo Bonzini wrote: > From: Philippe Mathieu-Daudé <philmd@linaro.org> > > Propagate the buffer size to format_dec() and use snprintf(). > > This should silence this UBSan -Wformat-overflow warning: "produced when using GCC 12.1.0:" > > In file included from /usr/include/stdio.h:906, > from include/qemu/osdep.h:114, > from ../disas/cris.c:21: > In function 'sprintf', > inlined from 'format_dec' at ../disas/cris.c:1737:3, > inlined from 'print_with_operands' at ../disas/cris.c:2477:12, > inlined from 'print_insn_cris_generic.constprop' at ../disas/cris.c:2690:8: > /usr/include/bits/stdio2.h:30:10: warning: null destination pointer [-Wformat-overflow=] > 30 | return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 31 | __glibc_objsize (__s), __fmt, > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 32 | __va_arg_pack ()); > | ~~~~~~~~~~~~~~~~~ > > Reported-by: Akihiko Odaki <akihiko.odaki@daynix.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Message-ID: <20231120132222.82138-1-philmd@linaro.org> > [Rewritten to fix logic and avoid repeated expression. - Paolo] > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > disas/cris.c | 26 ++++++++++++++++---------- > 1 file changed, 16 insertions(+), 10 deletions(-) > > diff --git a/disas/cris.c b/disas/cris.c > index 0b0a3fb9165..409a224c5d1 100644 > --- a/disas/cris.c > +++ b/disas/cris.c > @@ -1731,10 +1731,10 @@ format_hex (unsigned long number, > unsigned (== 0). */ > > static char * > -format_dec (long number, char *outbuffer, int signedp) > +format_dec (long number, char *outbuffer, size_t outsize, int signedp) > { > last_immediate = number; > - sprintf (outbuffer, signedp ? "%ld" : "%lu", number); > + snprintf (outbuffer, outsize, signedp ? "%ld" : "%lu", number); > > return outbuffer + strlen (outbuffer); > } > @@ -1876,6 +1876,12 @@ print_flags (struct cris_disasm_data *disdata, unsigned int insn, char *cp) > return cp; > } > > +#define FORMAT_DEC(number, tp, signedp) \ > + format_dec (number, tp, ({ \ > + assert(tp >= temp && tp <= temp + sizeof(temp)); \ > + temp + sizeof(temp) - tp; \ > + }), signedp) Thank you Paolo for this respin! Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
diff --git a/disas/cris.c b/disas/cris.c index 0b0a3fb9165..409a224c5d1 100644 --- a/disas/cris.c +++ b/disas/cris.c @@ -1731,10 +1731,10 @@ format_hex (unsigned long number, unsigned (== 0). */ static char * -format_dec (long number, char *outbuffer, int signedp) +format_dec (long number, char *outbuffer, size_t outsize, int signedp) { last_immediate = number; - sprintf (outbuffer, signedp ? "%ld" : "%lu", number); + snprintf (outbuffer, outsize, signedp ? "%ld" : "%lu", number); return outbuffer + strlen (outbuffer); } @@ -1876,6 +1876,12 @@ print_flags (struct cris_disasm_data *disdata, unsigned int insn, char *cp) return cp; } +#define FORMAT_DEC(number, tp, signedp) \ + format_dec (number, tp, ({ \ + assert(tp >= temp && tp <= temp + sizeof(temp)); \ + temp + sizeof(temp) - tp; \ + }), signedp) + /* Print out an insn with its operands, and update the info->insn_type fields. The prefix_opcodep and the rest hold a prefix insn that is supposed to be output as an address mode. */ @@ -2105,7 +2111,7 @@ print_with_operands (const struct cris_opcode *opcodep, if ((*cs == 'z' && (insn & 0x20)) || (opcodep->match == BDAP_QUICK_OPCODE && (nbytes <= 2 || buffer[1 + nbytes] == 0))) - tp = format_dec (number, tp, signedp); + tp = FORMAT_DEC (number, tp, signedp); else { unsigned int highbyte = (number >> 24) & 0xff; @@ -2241,7 +2247,7 @@ print_with_operands (const struct cris_opcode *opcodep, with_reg_prefix); if (number >= 0) *tp++ = '+'; - tp = format_dec (number, tp, 1); + tp = FORMAT_DEC (number, tp, 1); info->flags |= CRIS_DIS_FLAG_MEM_TARGET_IS_REG; info->target = (prefix_insn >> 12) & 15; @@ -2340,7 +2346,7 @@ print_with_operands (const struct cris_opcode *opcodep, { if (number >= 0) *tp++ = '+'; - tp = format_dec (number, tp, 1); + tp = FORMAT_DEC (number, tp, 1); } } else @@ -2397,7 +2403,7 @@ print_with_operands (const struct cris_opcode *opcodep, break; case 'I': - tp = format_dec (insn & 63, tp, 0); + tp = FORMAT_DEC (insn & 63, tp, 0); break; case 'b': @@ -2426,11 +2432,11 @@ print_with_operands (const struct cris_opcode *opcodep, break; case 'c': - tp = format_dec (insn & 31, tp, 0); + tp = FORMAT_DEC (insn & 31, tp, 0); break; case 'C': - tp = format_dec (insn & 15, tp, 0); + tp = FORMAT_DEC (insn & 15, tp, 0); break; case 'o': @@ -2463,7 +2469,7 @@ print_with_operands (const struct cris_opcode *opcodep, if (number > 127) number = number - 256; - tp = format_dec (number, tp, 1); + tp = FORMAT_DEC (number, tp, 1); *tp++ = ','; tp = format_reg (disdata, (insn >> 12) & 15, tp, with_reg_prefix); } @@ -2474,7 +2480,7 @@ print_with_operands (const struct cris_opcode *opcodep, break; case 'i': - tp = format_dec ((insn & 32) ? (insn & 31) | ~31L : insn & 31, tp, 1); + tp = FORMAT_DEC ((insn & 32) ? (insn & 31) | ~31L : insn & 31, tp, 1); break; case 'P':