From patchwork Tue Jul 10 16:45:11 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 28296 Received: (qmail 49160 invoked by alias); 10 Jul 2018 16:46:09 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 48941 invoked by uid 89); 10 Jul 2018 16:45:50 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS, TIME_LIMIT_EXCEEDED autolearn=unavailable version=3.3.2 spammy=pedro, simplifying, Pedro, Alves X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 10 Jul 2018 16:45:26 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 343B340200A8 for ; Tue, 10 Jul 2018 16:45:13 +0000 (UTC) Received: from localhost.localdomain (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id B33AF2026D6B for ; Tue, 10 Jul 2018 16:45:12 +0000 (UTC) From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH] GDBserver: Fix "Cond. jump or move depends on uninit value" in x87 code Date: Tue, 10 Jul 2018 17:45:11 +0100 Message-Id: <20180710164511.29112-1-palves@redhat.com> 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: yyyy-mm-dd 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/i387-fp.c | 69 +++++++++++++++++------------------------------- gdb/gdbserver/regcache.c | 10 +++++++ gdb/gdbserver/regcache.h | 7 +++++ 3 files changed, 41 insertions(+), 45 deletions(-) diff --git a/gdb/gdbserver/i387-fp.c b/gdb/gdbserver/i387-fp.c index bee1af9d8c6..b55a28596da 100644 --- a/gdb/gdbserver/i387-fp.c +++ b/gdb/gdbserver/i387-fp.c @@ -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; diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c index 735ce7bccfc..0ffec534c36 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 b4c4c20ebd3..a3d8922bb10 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 */