On Mon, Aug 15, 2022 at 8:38 AM Jan Beulich wrote: > > PR binutils/29483 > > When print_insn() processes op_txt[], it may pass strings into > i386_dis_printf() which staging_area[] cannot fit; this was observed for > an invalid form of VPSCATTERDD (both broadcast and zeroing-masking bits > set). Rather than arbitrarily enlarging that local array, avoid its use > altogether when the format string is simply "%s". This merely requires > two local variables to have their type constified. > > While limiting the scope of "res" it became apparent that > - no caller cares about the function's return value, > - the comment about the return value was wrong, > - a particular positive return value would have been meaningless to the > caller. > Therefore convert the function to return "void" at the same time. > --- > An alternative to the special casing would be to introduce something > like i386_dis_puts(), then to be used by all call sites which currently > pass "%s" or format strings without any format characters at all (plus, > of course, i386_dis_printf() itself). > --- > v2: Add testcase. > > --- a/gas/testsuite/gas/i386/i386.exp > +++ b/gas/testsuite/gas/i386/i386.exp > @@ -1349,6 +1349,7 @@ if [gas_64_check] then { > run_dump_test ehinterp > } > run_dump_test pr27198 > + run_dump_test pr29483 > > set ASFLAGS "$old_ASFLAGS --64" > > --- /dev/null > +++ b/gas/testsuite/gas/i386/pr29483.d > @@ -0,0 +1,11 @@ > +#objdump: -dw > +#name: x86-64 PR binutils/29483 > + > +.*: +file format .* > + > +Disassembly of section .text: > + > +0+ : > +[ ]*[a-f0-9]+: 65 62 62 7d 97 a0 94 ff 20 20 20 ae vpscatterdd .* > + > +0x65,0x62,0x62,0x7d,0x97,0xa0,0x94,0xff,0x20,0x20,0x20,0xae > --- /dev/null > +++ b/gas/testsuite/gas/i386/pr29483.s > @@ -0,0 +1,5 @@ > + .text > +pr29483: > + # This (VPSCATTERDD with EVEX.br and EVEX.z invalidly set) should not > + # crash the disassembler. > + .byte 0x65,0x62,0x62,0x7d,0x97,0xa0,0x94,0xff,0x20,0x20,0x20,0xae > --- a/opcodes/i386-dis.c > +++ b/opcodes/i386-dis.c > @@ -9264,31 +9264,40 @@ oappend_register (instr_info *ins, const > STYLE is the default style to use in the fprintf_styled_func calls, > however, FMT might include embedded style markers (see oappend_style), > these embedded markers are not printed, but instead change the style > - used in the next fprintf_styled_func call. > + used in the next fprintf_styled_func call. */ > > - Return non-zero to indicate the print call was a success. */ > - > -static int ATTRIBUTE_PRINTF_3 > +static void ATTRIBUTE_PRINTF_3 > i386_dis_printf (instr_info *ins, enum disassembler_style style, > const char *fmt, ...) > { > va_list ap; > enum disassembler_style curr_style = style; > - char *start, *curr; > + const char *start, *curr; > char staging_area[100]; > - int res; > > va_start (ap, fmt); > - res = vsnprintf (staging_area, sizeof (staging_area), fmt, ap); > - va_end (ap); > + /* In particular print_insn()'s processing of op_txt[] can hand rather long > + strings here. Bypass vsnprintf() in such cases to avoid capacity issues > + with the staging area. */ > + if (strcmp (fmt, "%s")) > + { > + int res = vsnprintf (staging_area, sizeof (staging_area), fmt, ap); > > - if (res < 0) > - return res; > + va_end (ap); > > - if ((size_t) res >= sizeof (staging_area)) > - abort (); > + if (res < 0) > + return; > > - start = curr = staging_area; > + if ((size_t) res >= sizeof (staging_area)) > + abort (); > + > + start = curr = staging_area; > + } > + else > + { > + start = curr = va_arg (ap, const char *); > + va_end (ap); > + } > > do > { > @@ -9303,10 +9312,7 @@ i386_dis_printf (instr_info *ins, enum d > curr_style, > "%.*s", len, start); > if (n < 0) > - { > - res = n; > - break; > - } > + break; > > if (*curr == '\0') > break; > @@ -9340,8 +9346,6 @@ i386_dis_printf (instr_info *ins, enum d > ++curr; > } > while (true); > - > - return res; > } > > static int I prefer something like this with the disassembler output: 0: 65 62 62 7d 97 a0 94 ff 20 20 20 ae vpscatterdd %xmm26,%gs:-0x51dfdfe0(%rdi,%xmm23,8){bad}{%k7}{z}/(bad) -- H.J.