[PATCH] libdw: Undefined behavior in get_sleb128_step.

[PATCH] libdw: Undefined behavior in get_sleb128_step.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 3243 bytes --]

gcc -fsanitize=undefined pointed out that for too big sleb128 values we
could shift into the sign bit. So for sleb128 values that have to fit
in a (signed) int64_t variable reduce the max number of steps by one.

https://bugzilla.redhat.com/show_bug.cgi?id=1170810#c29

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libdw/ChangeLog       |  8 ++++++++
 libdw/memory-access.h | 23 +++++++++++++++++++----
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index 87c7d82..739e0f6 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,11 @@
+2015-04-22  Mark Wielaard  <mjw@redhat.com>
+
+	* memory-access.h (__libdw_max_len_leb128): Take type_len as argument.
+	(__libdw_max_len_uleb128): New function.
+	(__libdw_max_len_sleb128): Likewise.
+	(__libdw_get_uleb128): Use __libdw_max_len_uleb128.
+	(__libdw_get_sleb128): Use __libdw_max_len_sleb128.
+
 2015-04-21  Mark Wielaard  <mjw@redhat.com>
 
 	* dwarf_getmacros.c (read_macros): Allocate attributes dynamically
diff --git a/libdw/memory-access.h b/libdw/memory-access.h
index a53f791..a749b5a 100644
--- a/libdw/memory-access.h
+++ b/libdw/memory-access.h
@@ -40,13 +40,28 @@
 #define len_leb128(var) ((8 * sizeof (var) + 6) / 7)
 
 static inline size_t
-__libdw_max_len_leb128 (const unsigned char *addr, const unsigned char *end)
+__libdw_max_len_leb128 (const size_t type_len,
+			const unsigned char *addr, const unsigned char *end)
 {
-  const size_t type_len = len_leb128 (uint64_t);
   const size_t pointer_len = likely (addr < end) ? end - addr : 0;
   return likely (type_len <= pointer_len) ? type_len : pointer_len;
 }
 
+static inline size_t
+__libdw_max_len_uleb128 (const unsigned char *addr, const unsigned char *end)
+{
+  const size_t type_len = len_leb128 (uint64_t);
+  return __libdw_max_len_leb128 (type_len, addr, end);
+}
+
+static inline size_t
+__libdw_max_len_sleb128 (const unsigned char *addr, const unsigned char *end)
+{
+  /* Subtract one step, so we don't shift into sign bit.  */
+  const size_t type_len = len_leb128 (int64_t) - 1;
+  return __libdw_max_len_leb128 (type_len, addr, end);
+}
+
 #define get_uleb128_step(var, addr, nth)				      \
   do {									      \
     unsigned char __b = *(addr)++;					      \
@@ -64,7 +79,7 @@ __libdw_get_uleb128 (const unsigned char **addrp, const unsigned char *end)
      for the common single-byte case.  */
   get_uleb128_step (acc, *addrp, 0);
 
-  const size_t max = __libdw_max_len_leb128 (*addrp - 1, end);
+  const size_t max = __libdw_max_len_uleb128 (*addrp - 1, end);
   for (size_t i = 1; i < max; ++i)
     get_uleb128_step (acc, *addrp, i);
   /* Other implementations set VALUE to UINT_MAX in this
@@ -98,7 +113,7 @@ __libdw_get_sleb128 (const unsigned char **addrp, const unsigned char *end)
      for the common single-byte case.  */
   get_sleb128_step (acc, *addrp, 0);
 
-  const size_t max = __libdw_max_len_leb128 (*addrp - 1, end);
+  const size_t max = __libdw_max_len_sleb128 (*addrp - 1, end);
   for (size_t i = 1; i < max; ++i)
     get_sleb128_step (acc, *addrp, i);
   /* Other implementations set VALUE to INT_MAX in this
-- 
2.1.0

Re: [PATCH] libdw: Undefined behavior in get_sleb128_step.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 299 bytes --]

On Wed, 2015-04-22 at 12:46 +0200, Mark Wielaard wrote:
> gcc -fsanitize=undefined pointed out that for too big sleb128 values we
> could shift into the sign bit. So for sleb128 values that have to fit
> in a (signed) int64_t variable reduce the max number of steps by one.

Pushed to master.