Re: [PATCH v2] x86: avoid i386_dis_printf()'s staging area for potentially long strings

["H.J. Lu" <hjl.tools@gmail.com>]

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.