public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/3] x86: further disassembler tweaks
@ 2023-04-24  7:33 Jan Beulich
  2023-04-24  7:34 ` [PATCH 1/3] x86: work around compiler diagnosing dangling pointer Jan Beulich
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Jan Beulich @ 2023-04-24  7:33 UTC (permalink / raw)
  To: Binutils; +Cc: H.J. Lu

The first patch is an attempt to deal with recently uncovered (but
latently present for longer) dangling pointer warnings issued by gcc.
I'll commit this patch right away, in the hope that it helps. The
latter two patches address observations made while checking whether
the approach taken in patch 1 is actually safe. I'll keep these pending
for a few days, in case there are comments (or even objections).

1: work around compiler diagnosing dangling pointer
2: limit data passed to prefix_name()
3: limit data passed to i386_dis_printf()

Jan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/3] x86: work around compiler diagnosing dangling pointer
  2023-04-24  7:33 [PATCH 0/3] x86: further disassembler tweaks Jan Beulich
@ 2023-04-24  7:34 ` Jan Beulich
  2023-04-24 10:24   ` Alan Modra
  2023-04-24  7:35 ` [PATCH 2/3] x86: limit data passed to prefix_name() Jan Beulich
  2023-04-24  7:35 ` [PATCH 3/3] x86: limit data passed to i386_dis_printf() Jan Beulich
  2 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2023-04-24  7:34 UTC (permalink / raw)
  To: Binutils; +Cc: H.J. Lu

For quite come time print_insn() has been storing the address of a local
variable into info->private_data. Since the compiler can't know that the
field won't be accessed again after print_insn() returns, it may kind of
legitimately diagnose this. And recent enough gcc does as of the
introduction of the fetch_error() return paths (replacing setjmp()-based
error handling).

Utilizing that neither prefix_name() nor i386_dis_printf() actually use
info->private_data, zap the pointer in fetch_error(), after having
retrieved it for local use.
---
Let's hope that this addresses the observed issues, which I haven't been
seeing myself. And of course there are further return paths which may
(sooner or later) also have such a warning trigger.

--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -345,6 +345,12 @@ fetch_error (const instr_info *ins)
   const struct dis_private *priv = ins->info->private_data;
   const char *name = NULL;
 
+  /* Our caller has put a pointer to a local variable in info->private_data
+     and it is going to return right after this function has returned.  Some
+     compilers diagnose this as a dangling pointer.  Zap the pointer here to
+     avoid needing to do so on all involved return paths in the caller.  */
+  ins->info->private_data = NULL;
+
   if (ins->codep <= priv->the_buffer)
     return -1;
 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/3] x86: limit data passed to prefix_name()
  2023-04-24  7:33 [PATCH 0/3] x86: further disassembler tweaks Jan Beulich
  2023-04-24  7:34 ` [PATCH 1/3] x86: work around compiler diagnosing dangling pointer Jan Beulich
@ 2023-04-24  7:35 ` Jan Beulich
  2023-04-24  7:35 ` [PATCH 3/3] x86: limit data passed to i386_dis_printf() Jan Beulich
  2 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2023-04-24  7:35 UTC (permalink / raw)
  To: Binutils; +Cc: H.J. Lu

Make apparent that neither what "ins" points to nor, in particular, that
"ins->info->private_data" is actually used in the function.

--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -117,7 +117,6 @@ static bool PREFETCHI_Fixup (instr_info
 static void ATTRIBUTE_PRINTF_3 i386_dis_printf (const instr_info *,
 						enum disassembler_style,
 						const char *, ...);
-static const char *prefix_name (const instr_info *, int, int);
 
 /* This character is used to encode style information within the output
    buffers.  See oappend_insert_style for more details.  */
@@ -141,6 +140,8 @@ enum address_mode
   mode_64bit
 };
 
+static const char *prefix_name (enum address_mode, int, int);
+
 enum x86_64_isa
 {
   amd64 = 1,
@@ -355,7 +356,8 @@ fetch_error (const instr_info *ins)
     return -1;
 
   if (ins->prefixes || ins->fwait_prefix >= 0 || (ins->rex & REX_OPCODE))
-    name = prefix_name (ins, priv->the_buffer[0], priv->orig_sizeflag);
+    name = prefix_name (ins->address_mode, priv->the_buffer[0],
+			priv->orig_sizeflag);
   if (name != NULL)
     i386_dis_printf (ins, dis_style_mnemonic, "%s", name);
   else
@@ -8928,7 +8930,7 @@ ckprefix (instr_info *ins)
    prefix byte.  */
 
 static const char *
-prefix_name (const instr_info *ins, int pref, int sizeflag)
+prefix_name (enum address_mode mode, int pref, int sizeflag)
 {
   static const char *rexes [16] =
     {
@@ -8991,7 +8993,7 @@ prefix_name (const instr_info *ins, int
     case 0x66:
       return (sizeflag & DFLAG) ? "data16" : "data32";
     case 0x67:
-      if (ins->address_mode == mode_64bit)
+      if (mode == mode_64bit)
 	return (sizeflag & AFLAG) ? "addr32" : "addr64";
       else
 	return (sizeflag & AFLAG) ? "addr16" : "addr32";
@@ -9767,7 +9769,8 @@ print_insn (bfd_vma pc, disassemble_info
 	   i++)
 	i386_dis_printf (&ins, dis_style_mnemonic, "%s%s",
 			 (i == 0 ? "" : " "),
-			 prefix_name (&ins, ins.all_prefixes[i], sizeflag));
+			 prefix_name (ins.address_mode, ins.all_prefixes[i],
+				      sizeflag));
       return i;
 
     case ckp_fetch_error:
@@ -9788,7 +9791,8 @@ print_insn (bfd_vma pc, disassemble_info
       for (i = 0; i < ins.fwait_prefix && ins.all_prefixes[i];
 	   i++)
 	i386_dis_printf (&ins, dis_style_mnemonic, "%s ",
-			 prefix_name (&ins, ins.all_prefixes[i], sizeflag));
+			 prefix_name (ins.address_mode, ins.all_prefixes[i],
+				      sizeflag));
       i386_dis_printf (&ins, dis_style_mnemonic, "fwait");
       return i + 1;
     }
@@ -10025,8 +10029,9 @@ print_insn (bfd_vma pc, disassemble_info
   for (i = 0; i < (int) ARRAY_SIZE (ins.all_prefixes); i++)
     if (ins.all_prefixes[i])
       {
-	const char *name;
-	name = prefix_name (&ins, ins.all_prefixes[i], orig_sizeflag);
+	const char *name = prefix_name (ins.address_mode, ins.all_prefixes[i],
+					orig_sizeflag);
+
 	if (name == NULL)
 	  abort ();
 	prefix_length += strlen (name) + 1;


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 3/3] x86: limit data passed to i386_dis_printf()
  2023-04-24  7:33 [PATCH 0/3] x86: further disassembler tweaks Jan Beulich
  2023-04-24  7:34 ` [PATCH 1/3] x86: work around compiler diagnosing dangling pointer Jan Beulich
  2023-04-24  7:35 ` [PATCH 2/3] x86: limit data passed to prefix_name() Jan Beulich
@ 2023-04-24  7:35 ` Jan Beulich
  2 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2023-04-24  7:35 UTC (permalink / raw)
  To: Binutils; +Cc: H.J. Lu

The function doesn't use "ins" for other than retrieving "info". Remove
a thus pointless level of indirection.

--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -114,7 +114,7 @@ static bool MOVSXD_Fixup (instr_info *,
 static bool DistinctDest_Fixup (instr_info *, int, int);
 static bool PREFETCHI_Fixup (instr_info *, int, int);
 
-static void ATTRIBUTE_PRINTF_3 i386_dis_printf (const instr_info *,
+static void ATTRIBUTE_PRINTF_3 i386_dis_printf (const disassemble_info *,
 						enum disassembler_style,
 						const char *, ...);
 
@@ -359,12 +359,12 @@ fetch_error (const instr_info *ins)
     name = prefix_name (ins->address_mode, priv->the_buffer[0],
 			priv->orig_sizeflag);
   if (name != NULL)
-    i386_dis_printf (ins, dis_style_mnemonic, "%s", name);
+    i386_dis_printf (ins->info, dis_style_mnemonic, "%s", name);
   else
     {
       /* Just print the first byte as a .byte instruction.  */
-      i386_dis_printf (ins, dis_style_assembler_directive, ".byte ");
-      i386_dis_printf (ins, dis_style_immediate, "%#x",
+      i386_dis_printf (ins->info, dis_style_assembler_directive, ".byte ");
+      i386_dis_printf (ins->info, dis_style_immediate, "%#x",
 		       (unsigned int) priv->the_buffer[0]);
     }
 
@@ -9528,7 +9528,7 @@ oappend_register (instr_info *ins, const
    used in the next fprintf_styled_func call.  */
 
 static void ATTRIBUTE_PRINTF_3
-i386_dis_printf (const instr_info *ins, enum disassembler_style style,
+i386_dis_printf (const disassemble_info *info, enum disassembler_style style,
 		 const char *fmt, ...)
 {
   va_list ap;
@@ -9569,9 +9569,8 @@ i386_dis_printf (const instr_info *ins,
 	{
 	  /* Output content between our START position and CURR.  */
 	  int len = curr - start;
-	  int n = (*ins->info->fprintf_styled_func) (ins->info->stream,
-						     curr_style,
-						     "%.*s", len, start);
+	  int n = (*info->fprintf_styled_func) (info->stream, curr_style,
+						"%.*s", len, start);
 	  if (n < 0)
 	    break;
 
@@ -9722,7 +9721,7 @@ print_insn (bfd_vma pc, disassemble_info
 
   if (ins.address_mode == mode_64bit && sizeof (bfd_vma) < 8)
     {
-      i386_dis_printf (&ins, dis_style_text, _("64-bit address is disabled"));
+      i386_dis_printf (info, dis_style_text, _("64-bit address is disabled"));
       return -1;
     }
 
@@ -9767,7 +9766,7 @@ print_insn (bfd_vma pc, disassemble_info
       for (i = 0;
 	   i < (int) ARRAY_SIZE (ins.all_prefixes) && ins.all_prefixes[i];
 	   i++)
-	i386_dis_printf (&ins, dis_style_mnemonic, "%s%s",
+	i386_dis_printf (info, dis_style_mnemonic, "%s%s",
 			 (i == 0 ? "" : " "),
 			 prefix_name (ins.address_mode, ins.all_prefixes[i],
 				      sizeflag));
@@ -9790,10 +9789,10 @@ print_insn (bfd_vma pc, disassemble_info
       /* Handle ins.prefixes before fwait.  */
       for (i = 0; i < ins.fwait_prefix && ins.all_prefixes[i];
 	   i++)
-	i386_dis_printf (&ins, dis_style_mnemonic, "%s ",
+	i386_dis_printf (info, dis_style_mnemonic, "%s ",
 			 prefix_name (ins.address_mode, ins.all_prefixes[i],
 				      sizeflag));
-      i386_dis_printf (&ins, dis_style_mnemonic, "fwait");
+      i386_dis_printf (info, dis_style_mnemonic, "fwait");
       return i + 1;
     }
 
@@ -9939,14 +9938,14 @@ print_insn (bfd_vma pc, disassemble_info
      are all 0s in inverted form.  */
   if (ins.need_vex && ins.vex.register_specifier != 0)
     {
-      i386_dis_printf (&ins, dis_style_text, "(bad)");
+      i386_dis_printf (info, dis_style_text, "(bad)");
       return ins.end_codep - priv.the_buffer;
     }
 
   /* If EVEX.z is set, there must be an actual mask register in use.  */
   if (ins.vex.zeroing && ins.vex.mask_register_specifier == 0)
     {
-      i386_dis_printf (&ins, dis_style_text, "(bad)");
+      i386_dis_printf (info, dis_style_text, "(bad)");
       return ins.end_codep - priv.the_buffer;
     }
 
@@ -9957,7 +9956,7 @@ print_insn (bfd_vma pc, disassemble_info
 	 the encoding invalid.  Most other PREFIX_OPCODE rules still apply.  */
       if (ins.need_vex ? !ins.vex.prefix : !(ins.prefixes & PREFIX_DATA))
 	{
-	  i386_dis_printf (&ins, dis_style_text, "(bad)");
+	  i386_dis_printf (info, dis_style_text, "(bad)");
 	  return ins.end_codep - priv.the_buffer;
 	}
       ins.used_prefixes |= PREFIX_DATA;
@@ -9984,7 +9983,7 @@ print_insn (bfd_vma pc, disassemble_info
 	  || (ins.vex.evex && dp->prefix_requirement != PREFIX_DATA
 	      && !ins.vex.w != !(ins.used_prefixes & PREFIX_DATA)))
 	{
-	  i386_dis_printf (&ins, dis_style_text, "(bad)");
+	  i386_dis_printf (info, dis_style_text, "(bad)");
 	  return ins.end_codep - priv.the_buffer;
 	}
       break;
@@ -10035,13 +10034,13 @@ print_insn (bfd_vma pc, disassemble_info
 	if (name == NULL)
 	  abort ();
 	prefix_length += strlen (name) + 1;
-	i386_dis_printf (&ins, dis_style_mnemonic, "%s ", name);
+	i386_dis_printf (info, dis_style_mnemonic, "%s ", name);
       }
 
   /* Check maximum code length.  */
   if ((ins.codep - ins.start_codep) > MAX_CODE_LENGTH)
     {
-      i386_dis_printf (&ins, dis_style_text, "(bad)");
+      i386_dis_printf (info, dis_style_text, "(bad)");
       return MAX_CODE_LENGTH;
     }
 
@@ -10065,7 +10064,7 @@ print_insn (bfd_vma pc, disassemble_info
     i = 0;
 
   /* Print the instruction mnemonic along with any trailing whitespace.  */
-  i386_dis_printf (&ins, dis_style_mnemonic, "%s%*s", ins.obuf, i, "");
+  i386_dis_printf (info, dis_style_mnemonic, "%s%*s", ins.obuf, i, "");
 
   /* The enter and bound instructions are printed with operands in the same
      order as the intel book; everything else is printed in reverse order.  */
@@ -10120,7 +10119,7 @@ print_insn (bfd_vma pc, disassemble_info
 	    break;
 	  }
 	if (needcomma)
-	  i386_dis_printf (&ins, dis_style_text, ",");
+	  i386_dis_printf (info, dis_style_text, ",");
 	if (ins.op_index[i] != -1 && !ins.op_riprel[i])
 	  {
 	    bfd_vma target = (bfd_vma) ins.op_address[ins.op_index[i]];
@@ -10136,14 +10135,14 @@ print_insn (bfd_vma pc, disassemble_info
 	    (*info->print_address_func) (target, info);
 	  }
 	else
-	  i386_dis_printf (&ins, dis_style_text, "%s", op_txt[i]);
+	  i386_dis_printf (info, dis_style_text, "%s", op_txt[i]);
 	needcomma = 1;
       }
 
   for (i = 0; i < MAX_OPERANDS; i++)
     if (ins.op_index[i] != -1 && ins.op_riprel[i])
       {
-	i386_dis_printf (&ins, dis_style_comment_start, "        # ");
+	i386_dis_printf (info, dis_style_comment_start, "        # ");
 	(*info->print_address_func)
 	  ((bfd_vma)(ins.start_pc + (ins.codep - ins.start_codep)
 		     + ins.op_address[ins.op_index[i]]),


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] x86: work around compiler diagnosing dangling pointer
  2023-04-24  7:34 ` [PATCH 1/3] x86: work around compiler diagnosing dangling pointer Jan Beulich
@ 2023-04-24 10:24   ` Alan Modra
  2023-04-24 10:35     ` Jan Beulich
  0 siblings, 1 reply; 6+ messages in thread
From: Alan Modra @ 2023-04-24 10:24 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Binutils, H.J. Lu

On Mon, Apr 24, 2023 at 09:34:27AM +0200, Jan Beulich via Binutils wrote:
> For quite come time print_insn() has been storing the address of a local
> variable into info->private_data. Since the compiler can't know that the
> field won't be accessed again after print_insn() returns, it may kind of
> legitimately diagnose this. And recent enough gcc does as of the
> introduction of the fetch_error() return paths (replacing setjmp()-based
> error handling).
> 
> Utilizing that neither prefix_name() nor i386_dis_printf() actually use
> info->private_data, zap the pointer in fetch_error(), after having
> retrieved it for local use.
> ---
> Let's hope that this addresses the observed issues, which I haven't been
> seeing myself. And of course there are further return paths which may
> (sooner or later) also have such a warning trigger.

I'll be surprised if your patch is enough.  I have the following in my
local tree, tested to work with a freshly built gcc-13 compiler.
Would you like me to commit this (and revert your patch)?

opcodes/i386-dis.c: In function ‘print_insn’:
opcodes/i386-dis.c:9865:22: error: storing the address of local
variable ‘priv’ in ‘*info.private_data’ [-Werror=dangling-pointer=]

	* i386-dis.c (print_insn): Clear info->private_data before
	returning.

diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
index f021bdaa3e7..01e5ba81723 100644
--- a/opcodes/i386-dis.c
+++ b/opcodes/i386-dis.c
@@ -9731,6 +9731,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
 {
   const struct dis386 *dp;
   int i;
+  int ret;
   char *op_txt[MAX_OPERANDS];
   int needcomma;
   bool intel_swap_2_3;
@@ -9887,16 +9888,21 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
 	i386_dis_printf (&ins, dis_style_mnemonic, "%s%s",
 			 (i == 0 ? "" : " "),
 			 prefix_name (&ins, ins.all_prefixes[i], sizeflag));
-      return i;
+      ret = i;
+      goto out;
 
     case ckp_fetch_error:
-      return fetch_error (&ins);
+      goto fetch_error_out;
     }
 
   ins.insn_codep = ins.codep;
 
   if (!fetch_code (info, ins.codep + 1))
-    return fetch_error (&ins);
+    {
+    fetch_error_out:
+      ret = fetch_error (&ins);
+      goto out;
+    }
 
   ins.two_source_ops = (*ins.codep == 0x62) || (*ins.codep == 0xc8);
 
@@ -9909,7 +9915,8 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
 	i386_dis_printf (&ins, dis_style_mnemonic, "%s ",
 			 prefix_name (&ins, ins.all_prefixes[i], sizeflag));
       i386_dis_printf (&ins, dis_style_mnemonic, "fwait");
-      return i + 1;
+      ret = i + 1;
+      goto out;
     }
 
   if (*ins.codep == 0x0f)
@@ -9918,7 +9925,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
 
       ins.codep++;
       if (!fetch_code (info, ins.codep + 1))
-	return fetch_error (&ins);
+	goto fetch_error_out;
       threebyte = *ins.codep;
       dp = &dis386_twobyte[threebyte];
       ins.need_modrm = twobyte_has_modrm[threebyte];
@@ -9942,30 +9949,30 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
 
   ins.end_codep = ins.codep;
   if (ins.need_modrm && !fetch_modrm (&ins))
-    return fetch_error (&ins);
+    goto fetch_error_out;
 
   if (dp->name == NULL && dp->op[0].bytemode == FLOATCODE)
     {
       if (!get_sib (&ins, sizeflag)
 	  || !dofloat (&ins, sizeflag))
-	return fetch_error (&ins);
+	goto fetch_error_out;
     }
   else
     {
       dp = get_valid_dis386 (dp, &ins);
       if (dp == &err_opcode)
-	return fetch_error (&ins);
+	goto fetch_error_out;
       if (dp != NULL && putop (&ins, dp->name, sizeflag) == 0)
 	{
 	  if (!get_sib (&ins, sizeflag))
-	    return fetch_error (&ins);
+	    goto fetch_error_out;
 	  for (i = 0; i < MAX_OPERANDS; ++i)
 	    {
 	      ins.obufp = ins.op_out[i];
 	      ins.op_ad = MAX_OPERANDS - 1 - i;
 	      if (dp->op[i].rtn
 		  && !dp->op[i].rtn (&ins, dp->op[i].bytemode, sizeflag))
-		return fetch_error (&ins);
+		goto fetch_error_out;
 	      /* For EVEX instruction after the last operand masking
 		 should be printed.  */
 	      if (i == 0 && ins.vex.evex)
@@ -10055,14 +10062,16 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
   if (ins.need_vex && ins.vex.register_specifier != 0)
     {
       i386_dis_printf (&ins, dis_style_text, "(bad)");
-      return ins.end_codep - priv.the_buffer;
+      ret = ins.end_codep - priv.the_buffer;
+      goto out;
     }
 
   /* If EVEX.z is set, there must be an actual mask register in use.  */
   if (ins.vex.zeroing && ins.vex.mask_register_specifier == 0)
     {
       i386_dis_printf (&ins, dis_style_text, "(bad)");
-      return ins.end_codep - priv.the_buffer;
+      ret = ins.end_codep - priv.the_buffer;
+      goto out;
     }
 
   switch (dp->prefix_requirement)
@@ -10073,7 +10082,8 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
       if (ins.need_vex ? !ins.vex.prefix : !(ins.prefixes & PREFIX_DATA))
 	{
 	  i386_dis_printf (&ins, dis_style_text, "(bad)");
-	  return ins.end_codep - priv.the_buffer;
+	  ret = ins.end_codep - priv.the_buffer;
+	  goto out;
 	}
       ins.used_prefixes |= PREFIX_DATA;
       /* Fall through.  */
@@ -10100,7 +10110,8 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
 	      && !ins.vex.w != !(ins.used_prefixes & PREFIX_DATA)))
 	{
 	  i386_dis_printf (&ins, dis_style_text, "(bad)");
-	  return ins.end_codep - priv.the_buffer;
+	  ret = ins.end_codep - priv.the_buffer;
+	  goto out;
 	}
       break;
 
@@ -10156,7 +10167,8 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
   if ((ins.codep - ins.start_codep) > MAX_CODE_LENGTH)
     {
       i386_dis_printf (&ins, dis_style_text, "(bad)");
-      return MAX_CODE_LENGTH;
+      ret = MAX_CODE_LENGTH;
+      goto out;
     }
 
   /* Calculate the number of operands this instruction has.  */
@@ -10264,7 +10276,10 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
 	  info);
 	break;
       }
-  return ins.codep - priv.the_buffer;
+  ret = ins.codep - priv.the_buffer;
+ out:
+  info->private_data = NULL;
+  return ret;
 }
 
 /* Here for backwards compatibility.  When gdb stops using

-- 
Alan Modra
Australia Development Lab, IBM

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] x86: work around compiler diagnosing dangling pointer
  2023-04-24 10:24   ` Alan Modra
@ 2023-04-24 10:35     ` Jan Beulich
  0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2023-04-24 10:35 UTC (permalink / raw)
  To: Alan Modra; +Cc: Binutils, H.J. Lu

On 24.04.2023 12:24, Alan Modra wrote:
> On Mon, Apr 24, 2023 at 09:34:27AM +0200, Jan Beulich via Binutils wrote:
>> For quite come time print_insn() has been storing the address of a local
>> variable into info->private_data. Since the compiler can't know that the
>> field won't be accessed again after print_insn() returns, it may kind of
>> legitimately diagnose this. And recent enough gcc does as of the
>> introduction of the fetch_error() return paths (replacing setjmp()-based
>> error handling).
>>
>> Utilizing that neither prefix_name() nor i386_dis_printf() actually use
>> info->private_data, zap the pointer in fetch_error(), after having
>> retrieved it for local use.
>> ---
>> Let's hope that this addresses the observed issues, which I haven't been
>> seeing myself. And of course there are further return paths which may
>> (sooner or later) also have such a warning trigger.
> 
> I'll be surprised if your patch is enough.  I have the following in my
> local tree, tested to work with a freshly built gcc-13 compiler.

Well, I was working from the knowledge that previously the issue wasn't
diagnosed, so touching all return paths may not be needed (for now at
least). I'm also always hesitant to introduce any kind of "goto", but
...

> Would you like me to commit this (and revert your patch)?

... since you've gone that route and done the work, sure, feel free to
go ahead.

Thanks, Jan

> opcodes/i386-dis.c: In function ‘print_insn’:
> opcodes/i386-dis.c:9865:22: error: storing the address of local
> variable ‘priv’ in ‘*info.private_data’ [-Werror=dangling-pointer=]
> 
> 	* i386-dis.c (print_insn): Clear info->private_data before
> 	returning.
> 
> diff --git a/opcodes/i386-dis.c b/opcodes/i386-dis.c
> index f021bdaa3e7..01e5ba81723 100644
> --- a/opcodes/i386-dis.c
> +++ b/opcodes/i386-dis.c
> @@ -9731,6 +9731,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>  {
>    const struct dis386 *dp;
>    int i;
> +  int ret;
>    char *op_txt[MAX_OPERANDS];
>    int needcomma;
>    bool intel_swap_2_3;
> @@ -9887,16 +9888,21 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>  	i386_dis_printf (&ins, dis_style_mnemonic, "%s%s",
>  			 (i == 0 ? "" : " "),
>  			 prefix_name (&ins, ins.all_prefixes[i], sizeflag));
> -      return i;
> +      ret = i;
> +      goto out;
>  
>      case ckp_fetch_error:
> -      return fetch_error (&ins);
> +      goto fetch_error_out;
>      }
>  
>    ins.insn_codep = ins.codep;
>  
>    if (!fetch_code (info, ins.codep + 1))
> -    return fetch_error (&ins);
> +    {
> +    fetch_error_out:
> +      ret = fetch_error (&ins);
> +      goto out;
> +    }
>  
>    ins.two_source_ops = (*ins.codep == 0x62) || (*ins.codep == 0xc8);
>  
> @@ -9909,7 +9915,8 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>  	i386_dis_printf (&ins, dis_style_mnemonic, "%s ",
>  			 prefix_name (&ins, ins.all_prefixes[i], sizeflag));
>        i386_dis_printf (&ins, dis_style_mnemonic, "fwait");
> -      return i + 1;
> +      ret = i + 1;
> +      goto out;
>      }
>  
>    if (*ins.codep == 0x0f)
> @@ -9918,7 +9925,7 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>  
>        ins.codep++;
>        if (!fetch_code (info, ins.codep + 1))
> -	return fetch_error (&ins);
> +	goto fetch_error_out;
>        threebyte = *ins.codep;
>        dp = &dis386_twobyte[threebyte];
>        ins.need_modrm = twobyte_has_modrm[threebyte];
> @@ -9942,30 +9949,30 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>  
>    ins.end_codep = ins.codep;
>    if (ins.need_modrm && !fetch_modrm (&ins))
> -    return fetch_error (&ins);
> +    goto fetch_error_out;
>  
>    if (dp->name == NULL && dp->op[0].bytemode == FLOATCODE)
>      {
>        if (!get_sib (&ins, sizeflag)
>  	  || !dofloat (&ins, sizeflag))
> -	return fetch_error (&ins);
> +	goto fetch_error_out;
>      }
>    else
>      {
>        dp = get_valid_dis386 (dp, &ins);
>        if (dp == &err_opcode)
> -	return fetch_error (&ins);
> +	goto fetch_error_out;
>        if (dp != NULL && putop (&ins, dp->name, sizeflag) == 0)
>  	{
>  	  if (!get_sib (&ins, sizeflag))
> -	    return fetch_error (&ins);
> +	    goto fetch_error_out;
>  	  for (i = 0; i < MAX_OPERANDS; ++i)
>  	    {
>  	      ins.obufp = ins.op_out[i];
>  	      ins.op_ad = MAX_OPERANDS - 1 - i;
>  	      if (dp->op[i].rtn
>  		  && !dp->op[i].rtn (&ins, dp->op[i].bytemode, sizeflag))
> -		return fetch_error (&ins);
> +		goto fetch_error_out;
>  	      /* For EVEX instruction after the last operand masking
>  		 should be printed.  */
>  	      if (i == 0 && ins.vex.evex)
> @@ -10055,14 +10062,16 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>    if (ins.need_vex && ins.vex.register_specifier != 0)
>      {
>        i386_dis_printf (&ins, dis_style_text, "(bad)");
> -      return ins.end_codep - priv.the_buffer;
> +      ret = ins.end_codep - priv.the_buffer;
> +      goto out;
>      }
>  
>    /* If EVEX.z is set, there must be an actual mask register in use.  */
>    if (ins.vex.zeroing && ins.vex.mask_register_specifier == 0)
>      {
>        i386_dis_printf (&ins, dis_style_text, "(bad)");
> -      return ins.end_codep - priv.the_buffer;
> +      ret = ins.end_codep - priv.the_buffer;
> +      goto out;
>      }
>  
>    switch (dp->prefix_requirement)
> @@ -10073,7 +10082,8 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>        if (ins.need_vex ? !ins.vex.prefix : !(ins.prefixes & PREFIX_DATA))
>  	{
>  	  i386_dis_printf (&ins, dis_style_text, "(bad)");
> -	  return ins.end_codep - priv.the_buffer;
> +	  ret = ins.end_codep - priv.the_buffer;
> +	  goto out;
>  	}
>        ins.used_prefixes |= PREFIX_DATA;
>        /* Fall through.  */
> @@ -10100,7 +10110,8 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>  	      && !ins.vex.w != !(ins.used_prefixes & PREFIX_DATA)))
>  	{
>  	  i386_dis_printf (&ins, dis_style_text, "(bad)");
> -	  return ins.end_codep - priv.the_buffer;
> +	  ret = ins.end_codep - priv.the_buffer;
> +	  goto out;
>  	}
>        break;
>  
> @@ -10156,7 +10167,8 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>    if ((ins.codep - ins.start_codep) > MAX_CODE_LENGTH)
>      {
>        i386_dis_printf (&ins, dis_style_text, "(bad)");
> -      return MAX_CODE_LENGTH;
> +      ret = MAX_CODE_LENGTH;
> +      goto out;
>      }
>  
>    /* Calculate the number of operands this instruction has.  */
> @@ -10264,7 +10276,10 @@ print_insn (bfd_vma pc, disassemble_info *info, int intel_syntax)
>  	  info);
>  	break;
>        }
> -  return ins.codep - priv.the_buffer;
> +  ret = ins.codep - priv.the_buffer;
> + out:
> +  info->private_data = NULL;
> +  return ret;
>  }
>  
>  /* Here for backwards compatibility.  When gdb stops using
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-04-24 10:35 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-24  7:33 [PATCH 0/3] x86: further disassembler tweaks Jan Beulich
2023-04-24  7:34 ` [PATCH 1/3] x86: work around compiler diagnosing dangling pointer Jan Beulich
2023-04-24 10:24   ` Alan Modra
2023-04-24 10:35     ` Jan Beulich
2023-04-24  7:35 ` [PATCH 2/3] x86: limit data passed to prefix_name() Jan Beulich
2023-04-24  7:35 ` [PATCH 3/3] x86: limit data passed to i386_dis_printf() Jan Beulich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).