diff mbox

[3/9] Avoid undefined behavior in extract_integer

Message ID 20180827145620.11055-4-tom@tromey.com
State New, archived
Headers

Commit Message

Tom Tromey Aug. 27, 2018, 2:56 p.m. UTC 
  -fsanitize=undefined showed that extract_integer could left-shift a
negative value, which is undefined.  This patch fixes the problem by
doing all the work in an unsigned type, and then using a static_cast
at the end of the function.  This relies on implementation-defined
behavior, but I tend to think we are on safe ground there.  (Also, if
need be, violations of this could probably be detected, either by
configure or by a static_assert.)

ChangeLog
2018-08-27  Tom Tromey  <tom@tromey.com>

	* findvar.c (extract_integer): Do work in an unsigned type and
	cast at the end.
---
 gdb/ChangeLog | 5 +++++
 gdb/findvar.c | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

Comments

Pedro Alves Aug. 28, 2018, 6:39 p.m. UTC | #1 
On 08/27/2018 03:56 PM, Tom Tromey wrote:
> -fsanitize=undefined showed that extract_integer could left-shift a
> negative value, which is undefined.  This patch fixes the problem by
> doing all the work in an unsigned type, and then using a static_cast
> at the end of the function.  This relies on implementation-defined
> behavior, but I tend to think we are on safe ground there.  (Also, if
> need be, violations of this could probably be detected, either by
> configure or by a static_assert.)
> 
> ChangeLog
> 2018-08-27  Tom Tromey  <tom@tromey.com>
> 
> 	* findvar.c (extract_integer): Do work in an unsigned type and
> 	cast at the end.

LGTM.

I suspect we assume two's complement in a good number of
places, and I don't think it's worth it to bother with anything
else.  There's even been discussion in the C++ committee about
baking the the assumption into the language.

Is the cast really necessary, though?  What error do you get?

Thanks,
Pedro Alves
Tom Tromey Aug. 29, 2018, 12:11 a.m. UTC | #2 
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> Is the cast really necessary, though?  What error do you get?

It's not necessary, so I've removed it.

Tom
diff mbox

Patch

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 9256833ab60..f2b84db82a1 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -50,7 +50,7 @@  template<typename T, typename>
 T
 extract_integer (const gdb_byte *addr, int len, enum bfd_endian byte_order)
 {
-  T retval = 0;
+  typename std::make_unsigned<T>::type retval = 0;
   const unsigned char *p;
   const unsigned char *startaddr = addr;
   const unsigned char *endaddr = startaddr + len;
@@ -86,7 +86,7 @@  That operation is not available on integers of more than %d bytes."),
       for (; p >= startaddr; --p)
 	retval = (retval << 8) | *p;
     }
-  return retval;
+  return static_cast<T> (retval);
 }
 
 /* Explicit instantiations.  */