From cb19713281c69c3625dd447a0c2ce539761989e2 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 11 Jul 2018 19:49:19 +0100 Subject: [PATCH] GDBserver: Fix "Cond. jump or move depends on uninit value" in x87 code 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}) (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}) (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: 2018-07-11 Pedro Alves * 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/ChangeLog | 8 +++++ gdb/gdbserver/i387-fp.c | 71 ++++++++++++++-------------------------- gdb/gdbserver/regcache.c | 10 ++++++ gdb/gdbserver/regcache.h | 7 ++++ 4 files changed, 50 insertions(+), 46 deletions(-) diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog index ac644ceb7f..7c482b0ddf 100644 --- a/gdb/gdbserver/ChangeLog +++ b/gdb/gdbserver/ChangeLog @@ -1,3 +1,11 @@ +2018-07-11 Pedro Alves + + * 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. + 2018-07-04 Vyacheslav Barinov Pedro Alves diff --git a/gdb/gdbserver/i387-fp.c b/gdb/gdbserver/i387-fp.c index bee1af9d8c..b06079ab42 100644 --- a/gdb/gdbserver/i387-fp.c +++ b/gdb/gdbserver/i387-fp.c @@ -149,38 +149,25 @@ i387_cache_to_fsave (struct regcache *regcache, void *buf) struct i387_fsave *fp = (struct i387_fsave *) buf; int i; int st0_regnum = find_regno (regcache->tdesc, "st0"); - unsigned long val, val2; + unsigned long val2; for (i = 0; i < 8; i++) 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; diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c index 735ce7bccf..0ffec534c3 100644 --- a/gdb/gdbserver/regcache.c +++ b/gdb/gdbserver/regcache.c @@ -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) { diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h index b4c4c20ebd..a3d8922bb1 100644 --- a/gdb/gdbserver/regcache.h +++ b/gdb/gdbserver/regcache.h @@ -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 */