Patchwork GDBserver: Fix "Cond. jump or move depends on uninit value" in x87 code

login
register
mail settings
Submitter Pedro Alves
Date July 10, 2018, 4:45 p.m.
Message ID <20180710164511.29112-1-palves@redhat.com>
Download mbox | patch
Permalink /patch/28296/
State New
Headers show

Comments

Pedro Alves - July 10, 2018, 4:45 p.m.
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(-)
Simon Marchi - July 10, 2018, 6:58 p.m.
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
Pedro Alves - July 11, 2018, 6:55 p.m.
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

Patch

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 */