Re: [PATCH] GDBserver: Fix "Cond. jump or move depends on uninit value" in x87 code

[Simon Marchi <simon dot marchi at ericsson dot com>]

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