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

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

[Jan Beulich]

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

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

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 4514 bytes --]

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)

-- 
H.J.

[-- Attachment #2: p.diff --]
[-- Type: text/x-patch, Size: 1312 bytes --]

diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
index 1761df583da..c1166a4446c 100644
--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -119,6 +119,9 @@ static void DistinctDest_Fixup (instr_info *, int, int);
    buffers.  See oappend_insert_style for more details.  */
 #define STYLE_MARKER_CHAR '\002'
 
+/* The maximum operand buffer size.  */
+#define MAX_OPERAND_BUFFER_SIZE 128
+
 struct dis_private {
   /* Points to first byte not fetched.  */
   bfd_byte *max_fetched;
@@ -165,7 +168,7 @@ struct instr_info
      current instruction.  */
   int evex_used;
 
-  char obuf[100];
+  char obuf[MAX_OPERAND_BUFFER_SIZE];
   char *obufp;
   char *mnemonicendp;
   unsigned char *start_codep;
@@ -9275,7 +9278,7 @@ i386_dis_printf (instr_info *ins, enum disassembler_style style,
   va_list ap;
   enum disassembler_style curr_style = style;
   char *start, *curr;
-  char staging_area[100];
+  char staging_area[MAX_OPERAND_BUFFER_SIZE];
   int res;
 
   va_start (ap, fmt);
@@ -9377,7 +9380,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
     .last_seg_prefix = -1,
     .fwait_prefix = -1,
   };
-  char op_out[MAX_OPERANDS][100];
+  char op_out[MAX_OPERANDS][MAX_OPERAND_BUFFER_SIZE];
 
   priv.orig_sizeflag = AFLAG | DFLAG;
   if ((info->mach & bfd_mach_i386_i386) != 0)

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

[Jan Beulich]

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

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

[H.J. Lu]

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.