cgen: opcodes: Fix memory corruption in in lookup
Commit Message
The buf variable is used after it is free'd. This causes the lookups to
fail and also causes memory corruption.
Re-arrange the code a bit to make sure we always free memory before
returning. This was caught in openrisc testing, one of the only user of
this method.
opcodes/ChangeLog:
2017-02-09 Stafford Horne <shorne@gmail.com>
cgen-opc.c (cgen_lookup_insn): Fix memory corruption issue.
---
opcodes/cgen-opc.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
Comments
Hi Stafford,
opcodes/ changes should be reviewed in binutils@sourceware.org.
On Wed, Feb 8, 2017 at 4:40 PM, Stafford Horne <shorne@gmail.com> wrote:
> The buf variable is used after it is free'd. This causes the lookups to
> fail and also causes memory corruption.
>
> Re-arrange the code a bit to make sure we always free memory before
> returning. This was caught in openrisc testing, one of the only user of
> this method.
>
> opcodes/ChangeLog:
>
> 2017-02-09 Stafford Horne <shorne@gmail.com>
> cgen-opc.c (cgen_lookup_insn): Fix memory corruption issue.
ChangeLog format issue,
017-02-09 Stafford Horne <shorne@gmail.com>
* cgen-opc.c (cgen_lookup_insn): Fix memory corruption issue.
> ---
> opcodes/cgen-opc.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/opcodes/cgen-opc.c b/opcodes/cgen-opc.c
> index 72b4f05..40a6320 100644
> --- a/opcodes/cgen-opc.c
> +++ b/opcodes/cgen-opc.c
> @@ -463,7 +463,6 @@ cgen_lookup_insn (CGEN_CPU_DESC cd,
> buf = (unsigned char *) xmalloc (cd->max_insn_bitsize / 8);
> cgen_put_insn_value (cd, buf, length, insn_int_value);
> base_insn = insn_int_value;
> - free (buf);
> }
> else
> {
> @@ -475,7 +474,7 @@ cgen_lookup_insn (CGEN_CPU_DESC cd,
> base_insn = cgen_get_insn_value (cd, buf, length);
> }
>
> - if (!insn)
> + if (insn == NULL)
> {
> const CGEN_INSN_LIST *insn_list;
>
> @@ -505,7 +504,8 @@ cgen_lookup_insn (CGEN_CPU_DESC cd,
> /* sanity check */
> if (length != 0 && length != elength)
> abort ();
> - return insn;
> + /* found, done */
> + break;
> }
> }
> }
> @@ -530,10 +530,12 @@ cgen_lookup_insn (CGEN_CPU_DESC cd,
> Could relax this later if it ever proves useful. */
> if (length == 0)
> abort ();
> - return insn;
> }
>
> - return NULL;
> + if (cd->int_insn_p)
> + free (buf);
> +
> + return insn;
> }
>
> /* Fill in the operand instances used by INSN whose operands are FIELDS.
> --
> 2.9.3
>
Hi Yao,
On Wed, Feb 08, 2017 at 05:01:05PM +0000, Yao Qi wrote:
> Hi Stafford,
> opcodes/ changes should be reviewed in binutils@sourceware.org.
Right, it was late when I sent this and I was working on a gdb bug. So
just sent it here. Resent now to binutils list.
> On Wed, Feb 8, 2017 at 4:40 PM, Stafford Horne <shorne@gmail.com> wrote:
> > The buf variable is used after it is free'd. This causes the lookups to
> > fail and also causes memory corruption.
> >
> > Re-arrange the code a bit to make sure we always free memory before
> > returning. This was caught in openrisc testing, one of the only user of
> > this method.
> >
> > opcodes/ChangeLog:
> >
> > 2017-02-09 Stafford Horne <shorne@gmail.com>
> > cgen-opc.c (cgen_lookup_insn): Fix memory corruption issue.
>
> ChangeLog format issue,
>
> 017-02-09 Stafford Horne <shorne@gmail.com>
>
> * cgen-opc.c (cgen_lookup_insn): Fix memory corruption issue.
Thanks,
-Stafford
@@ -463,7 +463,6 @@ cgen_lookup_insn (CGEN_CPU_DESC cd,
buf = (unsigned char *) xmalloc (cd->max_insn_bitsize / 8);
cgen_put_insn_value (cd, buf, length, insn_int_value);
base_insn = insn_int_value;
- free (buf);
}
else
{
@@ -475,7 +474,7 @@ cgen_lookup_insn (CGEN_CPU_DESC cd,
base_insn = cgen_get_insn_value (cd, buf, length);
}
- if (!insn)
+ if (insn == NULL)
{
const CGEN_INSN_LIST *insn_list;
@@ -505,7 +504,8 @@ cgen_lookup_insn (CGEN_CPU_DESC cd,
/* sanity check */
if (length != 0 && length != elength)
abort ();
- return insn;
+ /* found, done */
+ break;
}
}
}
@@ -530,10 +530,12 @@ cgen_lookup_insn (CGEN_CPU_DESC cd,
Could relax this later if it ever proves useful. */
if (length == 0)
abort ();
- return insn;
}
- return NULL;
+ if (cd->int_insn_p)
+ free (buf);
+
+ return insn;
}
/* Fill in the operand instances used by INSN whose operands are FIELDS.