[1/3] x86: work around compiler diagnosing dangling pointer
Checks
Commit Message
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.
Comments
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
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
>
@@ -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;