Skip to content

Commit

Permalink
GDBserver: Fix "Cond. jump or move depends on uninit value" in x87 code
Browse files Browse the repository at this point in the history 
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:
2018-07-11  Pedro Alves  <[email protected]>

	* 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.
  • Loading branch information
@palves
palves committed Jul 11, 2018
1 parent ad3c631 commit 82e080d
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 46 deletions.
8 changes: 8 additions & 0 deletions gdb/gdbserver/ChangeLog
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
2018-07-11 Pedro Alves <[email protected]>

* 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 <[email protected]>
Pedro Alves <[email protected]>

Expand Down
71 changes: 25 additions & 46 deletions gdb/gdbserver/i387-fp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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--)
{
Expand All @@ -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
Expand Down Expand Up @@ -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)
{
Expand All @@ -575,23 +555,22 @@ 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;
fp->fstat = val;
}

/* 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--)
{
Expand All @@ -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;
Expand Down
10 changes: 10 additions & 0 deletions gdb/gdbserver/regcache.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
7 changes: 7 additions & 0 deletions gdb/gdbserver/regcache.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 */

0 comments on commit 82e080d

Please sign in to comment.