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

[Jan Beulich <jbeulich@suse.com>]

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).

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.

Yet you're the maintainer - if you want to go with your fix, I won't
be able to stop you.

Jan