fix end-of-string overrun problems in C++ demangler

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

fix end-of-string overrun problems in C++ demangler

Geoffrey Keating-2

cplus_demangle can read past the end of the string it's passed, if
that string is not a valid mangled name.  The attached changes to
test-demangle.c and demangle-expected show some cases where it would
do this.

I started to audit cp-demangle.c for cases where this would happen,
with the aim of fixing each problem individually.  However, once I
started working on the sixth example, and I was only about half-way
through the file, and I was pretty sure there were more that I hadn't
caught, I decided that fixing each case individually wasn't going to
work.  So, the change that actually fixes the problem is the change to
d_next_char; the invariant now is that di->n always points to a
character inside the string, so it is not incremented when it points
at the trailing '\0'.  d_check_char is just an optimisation, so as to
not introduce an unnecessary extra test in a lot of places.

I have verified that all uses of d_advance and d_peek_next_char are
safe in the current code-base.

Tested by running 'make check' in libiberty; I'm also running a full
GCC bootstrap and testrun just to be sure.

--
- Geoffrey Keating <[hidden email]>

===File ~/patches/libiberty-demangle-stringoverrun.patch====
2006-12-20  Geoffrey Keating  <[hidden email]>

        * cp-demangle.h: Add comment explaining what to do to avoid
        overrunning string.
        (d_check_char): New.
        (d_next_char): Don't advance past trailing '\0'.
        * cp-demangle.c (cplus_demangle_mangled_name): Use d_check_char.
        (d_nested_name): Likewise.
        (d_special_name): Likewise.
        (d_call_offset): Likewise.
        (d_function_type): Likewise.
        (d_array_type): Likewise.
        (d_pointer_to_member_type): Likewise.
        (d_template_param): Likewise.
        (d_template_args): Likewise.
        (d_template_arg): Likewise.
        (d_expr_primary): Likewise.
        (d_local_name): Likewise.
        (d_substitution): Likewise.
        (d_ctor_dtor_name): Use d_advance rather than d_next_char.
        * testsuite/test-demangle.c: Include sys/mman.h.
        (MAP_ANONYMOUS): Define.
        (protect_end): New.
        (main): Use protect_end.
        * testsuite/demangle-expected: Add testcases for overrunning
        the end of the string.

Index: libiberty/testsuite/test-demangle.c
===================================================================
--- libiberty/testsuite/test-demangle.c (revision 120067)
+++ libiberty/testsuite/test-demangle.c (working copy)
@@ -86,6 +86,50 @@
   buf->alloced = alloc;
 }
 
+/* If we have mmap() and mprotect(), copy the string S just before a
+   protected page, so that if the demangler runs over the end of the
+   string we'll get a fault, and return the address of the new string.
+   If no mmap, or it fails, or it looks too hard, just return S.  */
+
+#ifdef HAVE_SYS_MMAN_H
+#include <sys/mman.h>
+#endif
+#if defined(MAP_ANON) && ! defined (MAP_ANONYMOUS)
+#define MAP_ANONYMOUS MAP_ANON
+#endif
+
+static const char *
+protect_end (const char * s)
+{
+#if defined(HAVE_MMAP) && defined (MAP_ANONYMOUS)
+  size_t pagesize = getpagesize();
+  static char * buf;
+  size_t s_len = strlen (s);
+  char * result;
+  
+  /* Don't try if S is too long.  */
+  if (s_len >= pagesize)
+    return s;
+
+  /* Allocate one page of allocated space followed by an unmapped
+     page.  */
+  if (buf == NULL)
+    {
+      buf = mmap (NULL, pagesize * 2, PROT_READ | PROT_WRITE,
+  MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+      if (! buf)
+ return s;
+      munmap (buf + pagesize, pagesize);
+    }
+  
+  result = buf + (pagesize - s_len - 1);
+  memcpy (result, s, s_len + 1);
+  return result;
+#else
+  return s;
+#endif
+}
+
 static void
 fail (lineno, opts, in, out, exp)
      int lineno;
@@ -150,6 +194,8 @@
 
   for (;;)
     {
+      const char *inp;
+      
       getline (&format);
       if (feof (stdin))
  break;
@@ -157,6 +203,8 @@
       getline (&input);
       getline (&expect);
 
+      inp = protect_end (input.data);
+
       tests++;
 
       no_params = 0;
@@ -237,14 +285,14 @@
     {
       enum gnu_v3_ctor_kinds kc;
 
-      kc = is_gnu_v3_mangled_ctor (input.data);
+      kc = is_gnu_v3_mangled_ctor (inp);
       sprintf (buf, "%d", (int) kc);
     }
   else
     {
       enum gnu_v3_dtor_kinds kd;
 
-      kd = is_gnu_v3_mangled_dtor (input.data);
+      kd = is_gnu_v3_mangled_dtor (inp);
       sprintf (buf, "%d", (int) kd);
     }
 
@@ -259,7 +307,7 @@
 
       cplus_demangle_set_style (style);
 
-      result = cplus_demangle (input.data,
+      result = cplus_demangle (inp,
        DMGL_PARAMS|DMGL_ANSI|DMGL_TYPES
        |(ret_postfix ? DMGL_RET_POSTFIX : 0));
 
@@ -275,7 +323,7 @@
       if (no_params)
  {
   getline (&expect);
-  result = cplus_demangle (input.data, DMGL_ANSI|DMGL_TYPES);
+  result = cplus_demangle (inp, DMGL_ANSI|DMGL_TYPES);
 
   if (result
       ? strcmp (result, expect.data)
Index: libiberty/testsuite/demangle-expected
===================================================================
--- libiberty/testsuite/demangle-expected (revision 120067)
+++ libiberty/testsuite/demangle-expected (working copy)
@@ -3816,3 +3816,25 @@
 SASDASDFASDF_sdfsdf
 SASDASDFASDF_sdfsdf
 SASDASDFASDF_sdfsdf
+# These are all cases of invalid manglings where the demangler would read
+# past the end of the string.
+# d_name wasn't honouring a NULL from d_substitution
+--format=gnu-v3
+_ZSA
+_ZSA
+# d_expr_primary wasn't honouring NULL from cplus_demangle_mangled_name
+--format=gnu-v3
+_ZN1fIL_
+_ZN1fIL_
+# d_operator_name was taking two characters in a row
+--format=gnu-v3
+_Za
+_Za
+# d_prefix wasn't honouring NULL from d_substitution
+--format=gnu-v3
+_ZNSA
+_ZNSA
+# d_prefix wasn't honouring NULL from d_template_param
+--format=gnu-v3
+_ZNT
+_ZNT
Index: libiberty/cp-demangle.c
===================================================================
--- libiberty/cp-demangle.c (revision 120067)
+++ libiberty/cp-demangle.c (working copy)
@@ -913,9 +913,9 @@
 struct demangle_component *
 cplus_demangle_mangled_name (struct d_info *di, int top_level)
 {
-  if (d_next_char (di) != '_')
+  if (! d_check_char (di, '_'))
     return NULL;
-  if (d_next_char (di) != 'Z')
+  if (! d_check_char (di, 'Z'))
     return NULL;
   return d_encoding (di, top_level);
 }
@@ -1123,7 +1123,7 @@
   struct demangle_component *ret;
   struct demangle_component **pret;
 
-  if (d_next_char (di) != 'N')
+  if (! d_check_char (di, 'N'))
     return NULL;
 
   pret = d_cv_qualifiers (di, &ret, 1);
@@ -1134,7 +1134,7 @@
   if (*pret == NULL)
     return NULL;
 
-  if (d_next_char (di) != 'E')
+  if (! d_check_char (di, 'E'))
     return NULL;
 
   return ret;
@@ -1449,11 +1449,8 @@
 static struct demangle_component *
 d_special_name (struct d_info *di)
 {
-  char c;
-
   di->expansion += 20;
-  c = d_next_char (di);
-  if (c == 'T')
+  if (d_check_char (di, 'T'))
     {
       switch (d_next_char (di))
  {
@@ -1502,7 +1499,7 @@
     offset = d_number (di);
     if (offset < 0)
       return NULL;
-    if (d_next_char (di) != '_')
+    if (! d_check_char (di, '_'))
       return NULL;
     base_type = cplus_demangle_type (di);
     /* We don't display the offset.  FIXME: We should display
@@ -1523,7 +1520,7 @@
   return NULL;
  }
     }
-  else if (c == 'G')
+  else if (d_check_char (di, 'G'))
     {
       switch (d_next_char (di))
  {
@@ -1570,14 +1567,14 @@
   else if (c == 'v')
     {
       d_number (di);
-      if (d_next_char (di) != '_')
+      if (! d_check_char (di, '_'))
  return 0;
       d_number (di);
     }
   else
     return 0;
 
-  if (d_next_char (di) != '_')
+  if (! d_check_char (di, '_'))
     return 0;
 
   return 1;
@@ -1601,13 +1598,13 @@
       else if (di->last_name->type == DEMANGLE_COMPONENT_SUB_STD)
  di->expansion += di->last_name->u.s_string.len;
     }
-  switch (d_next_char (di))
+  switch (d_peek_char (di))
     {
     case 'C':
       {
  enum gnu_v3_ctor_kinds kind;
 
- switch (d_next_char (di))
+ switch (d_peek_next_char (di))
   {
   case '1':
     kind = gnu_v3_complete_object_ctor;
@@ -1621,6 +1618,7 @@
   default:
     return NULL;
   }
+ d_advance (di, 2);
  return d_make_ctor (di, kind, di->last_name);
       }
 
@@ -1628,7 +1626,7 @@
       {
  enum gnu_v3_dtor_kinds kind;
 
- switch (d_next_char (di))
+ switch (d_peek_next_char (di))
   {
   case '0':
     kind = gnu_v3_deleting_dtor;
@@ -1642,6 +1640,7 @@
   default:
     return NULL;
   }
+ d_advance (di, 2);
  return d_make_dtor (di, kind, di->last_name);
       }
 
@@ -1925,7 +1924,7 @@
 {
   struct demangle_component *ret;
 
-  if (d_next_char (di) != 'F')
+  if (! d_check_char (di, 'F'))
     return NULL;
   if (d_peek_char (di) == 'Y')
     {
@@ -1934,7 +1933,7 @@
       d_advance (di, 1);
     }
   ret = d_bare_function_type (di, 1);
-  if (d_next_char (di) != 'E')
+  if (! d_check_char (di, 'E'))
     return NULL;
   return ret;
 }
@@ -2021,7 +2020,7 @@
   char peek;
   struct demangle_component *dim;
 
-  if (d_next_char (di) != 'A')
+  if (! d_check_char (di, 'A'))
     return NULL;
 
   peek = d_peek_char (di);
@@ -2049,7 +2048,7 @@
  return NULL;
     }
 
-  if (d_next_char (di) != '_')
+  if (! d_check_char (di, '_'))
     return NULL;
 
   return d_make_comp (di, DEMANGLE_COMPONENT_ARRAY_TYPE, dim,
@@ -2065,7 +2064,7 @@
   struct demangle_component *mem;
   struct demangle_component **pmem;
 
-  if (d_next_char (di) != 'M')
+  if (! d_check_char (di, 'M'))
     return NULL;
 
   cl = cplus_demangle_type (di);
@@ -2109,7 +2108,7 @@
 {
   long param;
 
-  if (d_next_char (di) != 'T')
+  if (! d_check_char (di, 'T'))
     return NULL;
 
   if (d_peek_char (di) == '_')
@@ -2122,7 +2121,7 @@
       param += 1;
     }
 
-  if (d_next_char (di) != '_')
+  if (! d_check_char (di, '_'))
     return NULL;
 
   ++di->did_subs;
@@ -2144,7 +2143,7 @@
      constructor or destructor.  */
   hold_last_name = di->last_name;
 
-  if (d_next_char (di) != 'I')
+  if (! d_check_char (di, 'I'))
     return NULL;
 
   al = NULL;
@@ -2189,7 +2188,7 @@
     case 'X':
       d_advance (di, 1);
       ret = d_expression (di);
-      if (d_next_char (di) != 'E')
+      if (! d_check_char (di, 'E'))
  return NULL;
       return ret;
 
@@ -2316,7 +2315,7 @@
 {
   struct demangle_component *ret;
 
-  if (d_next_char (di) != 'L')
+  if (! d_check_char (di, 'L'))
     return NULL;
   if (d_peek_char (di) == '_')
     ret = cplus_demangle_mangled_name (di, 0);
@@ -2362,7 +2361,7 @@
  }
       ret = d_make_comp (di, t, type, d_make_name (di, s, d_str (di) - s));
     }
-  if (d_next_char (di) != 'E')
+  if (! d_check_char (di, 'E'))
     return NULL;
   return ret;
 }
@@ -2376,12 +2375,12 @@
 {
   struct demangle_component *function;
 
-  if (d_next_char (di) != 'Z')
+  if (! d_check_char (di, 'Z'))
     return NULL;
 
   function = d_encoding (di, 0);
 
-  if (d_next_char (di) != 'E')
+  if (! d_check_char (di, 'E'))
     return NULL;
 
   if (d_peek_char (di) == 's')
@@ -2486,7 +2485,7 @@
 {
   char c;
 
-  if (d_next_char (di) != 'S')
+  if (! d_check_char (di, 'S'))
     return NULL;
 
   c = d_next_char (di);
Index: libiberty/cp-demangle.h
===================================================================
--- libiberty/cp-demangle.h (revision 120067)
+++ libiberty/cp-demangle.h (working copy)
@@ -123,10 +123,16 @@
   int expansion;
 };
 
+/* To avoid running past the ending '\0', don't:
+   - call d_peek_next_char if d_peek_char returned '\0'
+   - call d_advance with an 'i' that is too large
+   - call d_check_char(di, '\0')
+   Everything else is safe.  */
 #define d_peek_char(di) (*((di)->n))
 #define d_peek_next_char(di) ((di)->n[1])
 #define d_advance(di, i) ((di)->n += (i))
-#define d_next_char(di) (*((di)->n++))
+#define d_check_char(di, c) (d_peek_char(di) == c ? ((di)->n++, 1) : 0)
+#define d_next_char(di) (d_peek_char(di) == '\0' ? '\0' : *((di)->n++))
 #define d_str(di) ((di)->n)
 
 /* Functions and arrays in cp-demangle.c which are referenced by
============================================================
Reply | Threaded
Open this post in threaded view
|

Re: fix end-of-string overrun problems in C++ demangler

Ian Lance Taylor-3
[hidden email] (Geoffrey Keating) writes:

> 2006-12-20  Geoffrey Keating  <[hidden email]>
>
> * cp-demangle.h: Add comment explaining what to do to avoid
> overrunning string.
> (d_check_char): New.
> (d_next_char): Don't advance past trailing '\0'.
> * cp-demangle.c (cplus_demangle_mangled_name): Use d_check_char.
> (d_nested_name): Likewise.
> (d_special_name): Likewise.
> (d_call_offset): Likewise.
> (d_function_type): Likewise.
> (d_array_type): Likewise.
> (d_pointer_to_member_type): Likewise.
> (d_template_param): Likewise.
> (d_template_args): Likewise.
> (d_template_arg): Likewise.
> (d_expr_primary): Likewise.
> (d_local_name): Likewise.
> (d_substitution): Likewise.
> (d_ctor_dtor_name): Use d_advance rather than d_next_char.
> * testsuite/test-demangle.c: Include sys/mman.h.
> (MAP_ANONYMOUS): Define.
> (protect_end): New.
> (main): Use protect_end.
> * testsuite/demangle-expected: Add testcases for overrunning
> the end of the string.

Looks good to me.  Thanks for cleaning this up.

Ian