GDBserver: Fix "Cond. jump or move depends on uninit value" in x87 code
Commit Message
Running gdbserver under Valgrind I get:
==26925== Conditional jump or move depends on uninitialised value(s)
==26925== at 0x473E7F: i387_cache_to_xsave(regcache*, void*) (i387-fp.c:579)
==26925== by 0x46E3ED: x86_fill_xstateregset(regcache*, void*) (linux-x86-low.c:418)
==26925== by 0x45E747: regsets_store_inferior_registers(regsets_info*, regcache*) (linux-low.c:5456)
==26925== by 0x45EEF8: linux_store_registers(regcache*, int) (linux-low.c:5731)
==26925== by 0x426441: regcache_invalidate_thread(thread_info*) (regcache.c:89)
==26925== by 0x45CCAF: linux_resume_one_lwp_throw(lwp_info*, int, int, siginfo_t*) (linux-low.c:4447)
==26925== by 0x45CE2A: linux_resume_one_lwp(lwp_info*, int, int, siginfo_t*) (linux-low.c:4519)
==26925== by 0x45E17C: proceed_one_lwp(thread_info*, lwp_info*) (linux-low.c:5216)
==26925== by 0x45DC81: linux_resume_one_thread(thread_info*, bool) (linux-low.c:5031)
==26925== by 0x45DD34: linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}::operator()(thread_info*) const (linux-low.c:5095)
==26925== by 0x462907: void for_each_thread<linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}>(linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}) (gdbthread.h:150)
==26925== by 0x45DE62: linux_resume(thread_resume*, unsigned long) (linux-low.c:5093)
==26925==
==26925== Conditional jump or move depends on uninitialised value(s)
==26925== at 0x473EBD: i387_cache_to_xsave(regcache*, void*) (i387-fp.c:586)
==26925== by 0x46E3ED: x86_fill_xstateregset(regcache*, void*) (linux-x86-low.c:418)
==26925== by 0x45E747: regsets_store_inferior_registers(regsets_info*, regcache*) (linux-low.c:5456)
==26925== by 0x45EEF8: linux_store_registers(regcache*, int) (linux-low.c:5731)
==26925== by 0x426441: regcache_invalidate_thread(thread_info*) (regcache.c:89)
==26925== by 0x45CCAF: linux_resume_one_lwp_throw(lwp_info*, int, int, siginfo_t*) (linux-low.c:4447)
==26925== by 0x45CE2A: linux_resume_one_lwp(lwp_info*, int, int, siginfo_t*) (linux-low.c:4519)
==26925== by 0x45E17C: proceed_one_lwp(thread_info*, lwp_info*) (linux-low.c:5216)
==26925== by 0x45DC81: linux_resume_one_thread(thread_info*, bool) (linux-low.c:5031)
==26925== by 0x45DD34: linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}::operator()(thread_info*) const (linux-low.c:5095)
==26925== by 0x462907: void for_each_thread<linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}>(linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}) (gdbthread.h:150)
==26925== by 0x45DE62: linux_resume(thread_resume*, unsigned long) (linux-low.c:5093)
The problem is a type/width mismatch in code like this, in
gdbserver/i387-fp.c:
/* Some registers are 16-bit. */
collect_register_by_name (regcache, "fctrl", &val);
fp->fctrl = val;
In the above code:
#1 - 'val' is a 64-bit unsigned long.
#2 - "fctrl" is 32-bit in the register cache, thus half of 'val' is
left uninitialized by collect_register_by_name, which works with
an untyped raw buffer output (i.e., void*).
#3 - fp->fctrl is an unsigned short (16-bit). For some such
registers we're masking off the uninitialized bits with 0xffff,
but not in all cases.
We end up in such a fragile situation because
collect_registers_by_name works with an untyped output buffer pointer,
making it easy to pass a pointer to a variable of the wrong size.
Fix this by using regcache_raw_get_unsigned instead (actually a new
regcache_raw_get_unsigned_by_name wrapper), which always returns a
zero-extended ULONGEST register value. It ends up simplifying the
i387-tdep.c code a bit, even.
gdb/gdbserver/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* i387-fp.c (i387_cache_to_fsave, cache_to_fxsave, i387_cache_to_xsave): Use
regcache_raw_get_unsigned_by_name instead of
collect_register_by_name.
* regcache.c (regcache_raw_get_unsigned_by_name): New.
* regcache.h (regcache_raw_get_unsigned_by_name): New.
---
gdb/gdbserver/i387-fp.c | 69 +++++++++++++++++-------------------------------
gdb/gdbserver/regcache.c | 10 +++++++
gdb/gdbserver/regcache.h | 7 +++++
3 files changed, 41 insertions(+), 45 deletions(-)
Comments
On 2018-07-10 12:45 PM, Pedro Alves wrote:
> Running gdbserver under Valgrind I get:
>
> ==26925== Conditional jump or move depends on uninitialised value(s)
> ==26925== at 0x473E7F: i387_cache_to_xsave(regcache*, void*) (i387-fp.c:579)
> ==26925== by 0x46E3ED: x86_fill_xstateregset(regcache*, void*) (linux-x86-low.c:418)
> ==26925== by 0x45E747: regsets_store_inferior_registers(regsets_info*, regcache*) (linux-low.c:5456)
> ==26925== by 0x45EEF8: linux_store_registers(regcache*, int) (linux-low.c:5731)
> ==26925== by 0x426441: regcache_invalidate_thread(thread_info*) (regcache.c:89)
> ==26925== by 0x45CCAF: linux_resume_one_lwp_throw(lwp_info*, int, int, siginfo_t*) (linux-low.c:4447)
> ==26925== by 0x45CE2A: linux_resume_one_lwp(lwp_info*, int, int, siginfo_t*) (linux-low.c:4519)
> ==26925== by 0x45E17C: proceed_one_lwp(thread_info*, lwp_info*) (linux-low.c:5216)
> ==26925== by 0x45DC81: linux_resume_one_thread(thread_info*, bool) (linux-low.c:5031)
> ==26925== by 0x45DD34: linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}::operator()(thread_info*) const (linux-low.c:5095)
> ==26925== by 0x462907: void for_each_thread<linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}>(linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}) (gdbthread.h:150)
> ==26925== by 0x45DE62: linux_resume(thread_resume*, unsigned long) (linux-low.c:5093)
> ==26925==
> ==26925== Conditional jump or move depends on uninitialised value(s)
> ==26925== at 0x473EBD: i387_cache_to_xsave(regcache*, void*) (i387-fp.c:586)
> ==26925== by 0x46E3ED: x86_fill_xstateregset(regcache*, void*) (linux-x86-low.c:418)
> ==26925== by 0x45E747: regsets_store_inferior_registers(regsets_info*, regcache*) (linux-low.c:5456)
> ==26925== by 0x45EEF8: linux_store_registers(regcache*, int) (linux-low.c:5731)
> ==26925== by 0x426441: regcache_invalidate_thread(thread_info*) (regcache.c:89)
> ==26925== by 0x45CCAF: linux_resume_one_lwp_throw(lwp_info*, int, int, siginfo_t*) (linux-low.c:4447)
> ==26925== by 0x45CE2A: linux_resume_one_lwp(lwp_info*, int, int, siginfo_t*) (linux-low.c:4519)
> ==26925== by 0x45E17C: proceed_one_lwp(thread_info*, lwp_info*) (linux-low.c:5216)
> ==26925== by 0x45DC81: linux_resume_one_thread(thread_info*, bool) (linux-low.c:5031)
> ==26925== by 0x45DD34: linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}::operator()(thread_info*) const (linux-low.c:5095)
> ==26925== by 0x462907: void for_each_thread<linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}>(linux_resume(thread_resume*, unsigned long)::{lambda(thread_info*)#2}) (gdbthread.h:150)
> ==26925== by 0x45DE62: linux_resume(thread_resume*, unsigned long) (linux-low.c:5093)
>
> The problem is a type/width mismatch in code like this, in
> gdbserver/i387-fp.c:
>
> /* Some registers are 16-bit. */
> collect_register_by_name (regcache, "fctrl", &val);
> fp->fctrl = val;
>
> In the above code:
>
> #1 - 'val' is a 64-bit unsigned long.
>
> #2 - "fctrl" is 32-bit in the register cache, thus half of 'val' is
> left uninitialized by collect_register_by_name, which works with
> an untyped raw buffer output (i.e., void*).
>
> #3 - fp->fctrl is an unsigned short (16-bit). For some such
> registers we're masking off the uninitialized bits with 0xffff,
> but not in all cases.
>
> We end up in such a fragile situation because
> collect_registers_by_name works with an untyped output buffer pointer,
> making it easy to pass a pointer to a variable of the wrong size.
>
> Fix this by using regcache_raw_get_unsigned instead (actually a new
> regcache_raw_get_unsigned_by_name wrapper), which always returns a
> zero-extended ULONGEST register value. It ends up simplifying the
> i387-tdep.c code a bit, even.
Hi Pedro,
That looks like a good improvement. This variable is now unused:
i387-fp.c:152:17: error: unused variable ‘val’ [-Werror=unused-variable]
Anytime we use collect_register* to write directly in a variable is potentially
dangerous. It's not really applicable in GDBserver, but in GDB there are also
endianness issues. Should we aim at making collect_register's parameter a
gdb_byte* instead, to help avoid this situation? Code such as the one you fixed
would not compile.
Simon
On 07/10/2018 07:58 PM, Simon Marchi wrote:
>
> Hi Pedro,
>
> That looks like a good improvement. This variable is now unused:
>
> i387-fp.c:152:17: error: unused variable ‘val’ [-Werror=unused-variable]
Thanks, I fixed that and pushed the patch in.
>
> Anytime we use collect_register* to write directly in a variable is potentially
> dangerous. It's not really applicable in GDBserver, but in GDB there are also
> endianness issues. Should we aim at making collect_register's parameter a
> gdb_byte* instead, to help avoid this situation? Code such as the one you fixed
> would not compile.
Indeed, that may be a good idea.
I think most current uses of collect_register_by_name would
fail to compile, but then most of those could be replaced
with regcache_raw_get_unsigned_by_name.
Thanks,
Pedro Alves
@@ -155,32 +155,19 @@ i387_cache_to_fsave (struct regcache *regcache, void *buf)
collect_register (regcache, i + st0_regnum,
((char *) &fp->st_space[0]) + i * 10);
- collect_register_by_name (regcache, "fioff", &fp->fioff);
- collect_register_by_name (regcache, "fooff", &fp->fooff);
-
+ fp->fioff = regcache_raw_get_unsigned_by_name (regcache, "fioff");
+ fp->fooff = regcache_raw_get_unsigned_by_name (regcache, "fooff");
+
/* This one's 11 bits... */
- collect_register_by_name (regcache, "fop", &val2);
+ val2 = regcache_raw_get_unsigned_by_name (regcache, "fop");
fp->fop = (val2 & 0x7FF) | (fp->fop & 0xF800);
/* Some registers are 16-bit. */
- collect_register_by_name (regcache, "fctrl", &val);
- fp->fctrl = val;
-
- collect_register_by_name (regcache, "fstat", &val);
- val &= 0xFFFF;
- fp->fstat = val;
-
- collect_register_by_name (regcache, "ftag", &val);
- val &= 0xFFFF;
- fp->ftag = val;
-
- collect_register_by_name (regcache, "fiseg", &val);
- val &= 0xFFFF;
- fp->fiseg = val;
-
- collect_register_by_name (regcache, "foseg", &val);
- val &= 0xFFFF;
- fp->foseg = val;
+ fp->fctrl = regcache_raw_get_unsigned_by_name (regcache, "fctrl");
+ fp->fstat = regcache_raw_get_unsigned_by_name (regcache, "fstat");
+ fp->ftag = regcache_raw_get_unsigned_by_name (regcache, "ftag");
+ fp->fiseg = regcache_raw_get_unsigned_by_name (regcache, "fiseg");
+ fp->foseg = regcache_raw_get_unsigned_by_name (regcache, "foseg");
}
void
@@ -237,24 +224,20 @@ i387_cache_to_fxsave (struct regcache *regcache, void *buf)
collect_register (regcache, i + xmm0_regnum,
((char *) &fp->xmm_space[0]) + i * 16);
- collect_register_by_name (regcache, "fioff", &fp->fioff);
- collect_register_by_name (regcache, "fooff", &fp->fooff);
- collect_register_by_name (regcache, "mxcsr", &fp->mxcsr);
+ fp->fioff = regcache_raw_get_unsigned_by_name (regcache, "fioff");
+ fp->fooff = regcache_raw_get_unsigned_by_name (regcache, "fooff");
+ fp->mxcsr = regcache_raw_get_unsigned_by_name (regcache, "mxcsr");
/* This one's 11 bits... */
- collect_register_by_name (regcache, "fop", &val2);
+ val2 = regcache_raw_get_unsigned_by_name (regcache, "fop");
fp->fop = (val2 & 0x7FF) | (fp->fop & 0xF800);
/* Some registers are 16-bit. */
- collect_register_by_name (regcache, "fctrl", &val);
- fp->fctrl = val;
-
- collect_register_by_name (regcache, "fstat", &val);
- fp->fstat = val;
+ fp->fctrl = regcache_raw_get_unsigned_by_name (regcache, "fctrl");
+ fp->fstat = regcache_raw_get_unsigned_by_name (regcache, "fstat");
/* Convert to the simplifed tag form stored in fxsave data. */
- collect_register_by_name (regcache, "ftag", &val);
- val &= 0xFFFF;
+ val = regcache_raw_get_unsigned_by_name (regcache, "ftag");
val2 = 0;
for (i = 7; i >= 0; i--)
{
@@ -265,11 +248,8 @@ i387_cache_to_fxsave (struct regcache *regcache, void *buf)
}
fp->ftag = val2;
- collect_register_by_name (regcache, "fiseg", &val);
- fp->fiseg = val;
-
- collect_register_by_name (regcache, "foseg", &val);
- fp->foseg = val;
+ fp->fiseg = regcache_raw_get_unsigned_by_name (regcache, "fiseg");
+ fp->foseg = regcache_raw_get_unsigned_by_name (regcache, "foseg");
}
void
@@ -566,7 +546,7 @@ i387_cache_to_xsave (struct regcache *regcache, void *buf)
}
/* This one's 11 bits... */
- collect_register_by_name (regcache, "fop", &val2);
+ val2 = regcache_raw_get_unsigned_by_name (regcache, "fop");
val2 = (val2 & 0x7FF) | (fp->fop & 0xF800);
if (fp->fop != val2)
{
@@ -575,14 +555,14 @@ i387_cache_to_xsave (struct regcache *regcache, void *buf)
}
/* Some registers are 16-bit. */
- collect_register_by_name (regcache, "fctrl", &val);
+ val = regcache_raw_get_unsigned_by_name (regcache, "fctrl");
if (fp->fctrl != val)
{
xstate_bv |= X86_XSTATE_X87;
fp->fctrl = val;
}
- collect_register_by_name (regcache, "fstat", &val);
+ val = regcache_raw_get_unsigned_by_name (regcache, "fstat");
if (fp->fstat != val)
{
xstate_bv |= X86_XSTATE_X87;
@@ -590,8 +570,7 @@ i387_cache_to_xsave (struct regcache *regcache, void *buf)
}
/* Convert to the simplifed tag form stored in fxsave data. */
- collect_register_by_name (regcache, "ftag", &val);
- val &= 0xFFFF;
+ val = regcache_raw_get_unsigned_by_name (regcache, "ftag");
val2 = 0;
for (i = 7; i >= 0; i--)
{
@@ -606,14 +585,14 @@ i387_cache_to_xsave (struct regcache *regcache, void *buf)
fp->ftag = val2;
}
- collect_register_by_name (regcache, "fiseg", &val);
+ val = regcache_raw_get_unsigned_by_name (regcache, "fiseg");
if (fp->fiseg != val)
{
xstate_bv |= X86_XSTATE_X87;
fp->fiseg = val;
}
- collect_register_by_name (regcache, "foseg", &val);
+ val = regcache_raw_get_unsigned_by_name (regcache, "foseg");
if (fp->foseg != val)
{
xstate_bv |= X86_XSTATE_X87;
@@ -448,6 +448,16 @@ regcache_raw_read_unsigned (struct regcache *regcache, int regnum,
#ifndef IN_PROCESS_AGENT
+/* See regcache.h. */
+
+ULONGEST
+regcache_raw_get_unsigned_by_name (struct regcache *regcache,
+ const char *name)
+{
+ return regcache_raw_get_unsigned (regcache,
+ find_regno (regcache->tdesc, name));
+}
+
void
collect_register_as_string (struct regcache *regcache, int n, char *buf)
{
@@ -131,4 +131,11 @@ void collect_register_as_string (struct regcache *regcache, int n, char *buf);
void collect_register_by_name (struct regcache *regcache,
const char *name, void *buf);
+/* Read a raw register as an unsigned integer. Convenience wrapper
+ around regcache_raw_get_unsigned that takes a register name instead
+ of a register number. */
+
+ULONGEST regcache_raw_get_unsigned_by_name (struct regcache *regcache,
+ const char *name);
+
#endif /* REGCACHE_H */