[PATCH v2] x86: avoid i386_dis_printf()'s staging area for potentially long strings
H.J. Lu
hjl.tools@gmail.com
Tue Aug 16 15:14:43 GMT 2022
On Mon, Aug 15, 2022 at 11:06 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 15.08.2022 21:19, H.J. Lu wrote:
> > On Mon, Aug 15, 2022 at 8:38 AM Jan Beulich <jbeulich@suse.com> 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+ <pr29483>:
> >> +[ ]*[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)
>
> But the precise representation of the "badness" isn't relevant to this
> testcase. All we care about is that there's no crash (and perhaps, see
> below, no overrun of the other buffer, but I view this as a separate
> aspect).
I want to avoid the output like
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
> As to the attachment - I figure honoring op_out[]'s 2nd dimension is
> a necessary thing. I don't think I agree with bumping the buffer size,
> though: 128 is as arbitrary as 100. Or else there would want to be a
> comment next to MAX_OPERAND_BUFFER_SIZE explaining how the value was
> determined and hence making clear under what conditions it might need
> further bumping.
When styling was added, the original 100-byte buffer size wasn't
increased. Increasing it by 28 bytes should cover styling characters.
> Yet you're the maintainer - if you want to go with your fix, I won't
> be able to stop you.
>
> Jan
I will post a patch.
--
H.J.
More information about the Binutils
mailing list