diff mbox

[3/7] Fix trinary op

Message ID 559F4E82.3020909@gmail.com
State New
Headers show

Commit Message

Mikhail Maltsev July 10, 2015, 4:48 a.m. UTC 
On 08.07.2015 13:55, Ian Lance Taylor wrote:
> I don't know of anybody who actually uses the DMGL_TYPES support.  I
> don't know why anybody would.
> 
> Ian
Thanks for pointing that out. I updated the testcases, so that now they
don't depend on DMGL_TYPES being used.

> But better still is to consider the larger context.  We want the
> demangler to work the same on all hosts, if at all possible.
> d_identifier is called exactly once.  Change it to take a parameter of
> type long.  Don't worry about changing d_source_name.
Fixed.

> Then look at the fact that d_number does not check for overflow.  We
> should consider changing d_number to limit itself to 32-bit integers,
> and to return an error indication on overflow.  From a quick glance I
> don't see any need for the demangler to support numbers larger than 32
> bits.  I think it's OK if we fail to demangle symbol names that are
> more than 2 billion characters long.
OK, but I think it'll be better to fix that in a separate patch.

The attached patch includes the changes mentioned above, there is also a
small change: I moved the comment for CHECK_DEMANGLER macro to
cp-demangle.c (it already contains a comment for other similar macros)
and replaced __builtin_abort() with abort(). For some reason I thought
that it might need an additional #include, but in reality libiberty (and
the demangler too) already use abort().
The changelog is also attached. OK for trunk after regtest?

Comments

Jeff Law July 10, 2015, 8:44 p.m. UTC | #1 
On 07/09/2015 10:48 PM, Mikhail Maltsev wrote:
> On 08.07.2015 13:55, Ian Lance Taylor wrote:
>> I don't know of anybody who actually uses the DMGL_TYPES support.  I
>> don't know why anybody would.
>>
>> Ian
> Thanks for pointing that out. I updated the testcases, so that now they
> don't depend on DMGL_TYPES being used.
>
>> But better still is to consider the larger context.  We want the
>> demangler to work the same on all hosts, if at all possible.
>> d_identifier is called exactly once.  Change it to take a parameter of
>> type long.  Don't worry about changing d_source_name.
> Fixed.
>
>> Then look at the fact that d_number does not check for overflow.  We
>> should consider changing d_number to limit itself to 32-bit integers,
>> and to return an error indication on overflow.  From a quick glance I
>> don't see any need for the demangler to support numbers larger than 32
>> bits.  I think it's OK if we fail to demangle symbol names that are
>> more than 2 billion characters long.
> OK, but I think it'll be better to fix that in a separate patch.
>
> The attached patch includes the changes mentioned above, there is also a
> small change: I moved the comment for CHECK_DEMANGLER macro to
> cp-demangle.c (it already contains a comment for other similar macros)
> and replaced __builtin_abort() with abort(). For some reason I thought
> that it might need an additional #include, but in reality libiberty (and
> the demangler too) already use abort().
> The changelog is also attached. OK for trunk after regtest?

OK after regression testing.

jeff
Mikhail Maltsev July 13, 2015, 5:54 a.m. UTC | #2 
On 07/10/2015 11:44 PM, Jeff Law wrote:
> 
> OK after regression testing.
> 
> jeff
> 
Bootstrapped and regtested on x86_64-unknown-linux-gnu. Applied as r225727.
diff mbox

Patch

diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c
index 2988b6b..8254100 100644
--- a/libiberty/cp-demangle.c
+++ b/libiberty/cp-demangle.c
@@ -93,7 +93,11 @@ 
    CP_DEMANGLE_DEBUG
       If defined, turns on debugging mode, which prints information on
       stdout about the mangled string.  This is not generally useful.
-*/
+
+   CHECK_DEMANGLER
+      If defined, additional sanity checks will be performed.  It will
+      cause some slowdown, but will allow to catch out-of-bound access
+      errors earlier.  This macro is intended for testing and debugging.  */
 
 #if defined (_AIX) && !defined (__GNUC__)
  #pragma alloca
@@ -419,7 +423,7 @@  static struct demangle_component *d_source_name (struct d_info *);
 
 static long d_number (struct d_info *);
 
-static struct demangle_component *d_identifier (struct d_info *, int);
+static struct demangle_component *d_identifier (struct d_info *, long);
 
 static struct demangle_component *d_operator_name (struct d_info *);
 
@@ -715,7 +719,7 @@  d_dump (struct demangle_component *dc, int indent)
     case DEMANGLE_COMPONENT_FIXED_TYPE:
       printf ("fixed-point type, accum? %d, sat? %d\n",
               dc->u.s_fixed.accum, dc->u.s_fixed.sat);
-      d_dump (dc->u.s_fixed.length, indent + 2)
+      d_dump (dc->u.s_fixed.length, indent + 2);
       break;
     case DEMANGLE_COMPONENT_ARGLIST:
       printf ("argument list\n");
@@ -1656,7 +1660,7 @@  d_number_component (struct d_info *di)
 /* identifier ::= <(unqualified source code identifier)>  */
 
 static struct demangle_component *
-d_identifier (struct d_info *di, int len)
+d_identifier (struct d_info *di, long len)
 {
   const char *name;
 
@@ -1677,7 +1681,7 @@  d_identifier (struct d_info *di, int len)
   /* Look for something which looks like a gcc encoding of an
      anonymous namespace, and replace it with a more user friendly
      name.  */
-  if (len >= (int) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2
+  if (len >= (long) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2
       && memcmp (name, ANONYMOUS_NAMESPACE_PREFIX,
 		 ANONYMOUS_NAMESPACE_PREFIX_LEN) == 0)
     {
@@ -3166,6 +3170,8 @@  d_expression_1 (struct d_info *di)
       struct demangle_component *type = NULL;
       if (peek == 't')
 	type = cplus_demangle_type (di);
+      if (!d_peek_next_char (di))
+	return NULL;
       d_advance (di, 2);
       return d_make_comp (di, DEMANGLE_COMPONENT_INITIALIZER_LIST,
 			  type, d_exprlist (di, 'E'));
@@ -3240,6 +3246,8 @@  d_expression_1 (struct d_info *di)
 	    struct demangle_component *left;
 	    struct demangle_component *right;
 
+	    if (code == NULL)
+	      return NULL;
 	    if (op_is_new_cast (op))
 	      left = cplus_demangle_type (di);
 	    else
@@ -3267,7 +3275,9 @@  d_expression_1 (struct d_info *di)
 	    struct demangle_component *second;
 	    struct demangle_component *third;
 
-	    if (!strcmp (code, "qu"))
+	    if (code == NULL)
+	      return NULL;
+	    else if (!strcmp (code, "qu"))
 	      {
 		/* ?: expression.  */
 		first = d_expression_1 (di);
@@ -4196,6 +4206,9 @@  d_find_pack (struct d_print_info *dpi,
     case DEMANGLE_COMPONENT_CHARACTER:
     case DEMANGLE_COMPONENT_FUNCTION_PARAM:
     case DEMANGLE_COMPONENT_UNNAMED_TYPE:
+    case DEMANGLE_COMPONENT_FIXED_TYPE:
+    case DEMANGLE_COMPONENT_DEFAULT_ARG:
+    case DEMANGLE_COMPONENT_NUMBER:
       return NULL;
 
     case DEMANGLE_COMPONENT_EXTENDED_OPERATOR:
@@ -4431,6 +4444,11 @@  d_print_comp_inner (struct d_print_info *dpi, int options,
 	    local_name = d_right (typed_name);
 	    if (local_name->type == DEMANGLE_COMPONENT_DEFAULT_ARG)
 	      local_name = local_name->u.s_unary_num.sub;
+	    if (local_name == NULL)
+	      {
+		d_print_error (dpi);
+		return;
+	      }
 	    while (local_name->type == DEMANGLE_COMPONENT_RESTRICT_THIS
 		   || local_name->type == DEMANGLE_COMPONENT_VOLATILE_THIS
 		   || local_name->type == DEMANGLE_COMPONENT_CONST_THIS
diff --git a/libiberty/cp-demangle.h b/libiberty/cp-demangle.h
index 6fce025..197883e 100644
--- a/libiberty/cp-demangle.h
+++ b/libiberty/cp-demangle.h
@@ -135,12 +135,37 @@  struct d_info
    - 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))
+#ifndef CHECK_DEMANGLER
+#  define d_peek_next_char(di) ((di)->n[1])
+#  define d_advance(di, i) ((di)->n += (i))
+#endif
 #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)
 
+#ifdef CHECK_DEMANGLER
+static inline char
+d_peek_next_char (const struct d_info *di)
+{
+  if (!di->n[0])
+    abort ();
+  return di->n[1];
+}
+
+static inline void
+d_advance (struct d_info *di, int i)
+{
+  if (i < 0)
+    abort ();
+  while (i--)
+    {
+      if (!di->n[0])
+	abort ();
+      di->n++;
+    }
+}
+#endif
+
 /* Functions and arrays in cp-demangle.c which are referenced by
    functions in cp-demint.c.  */
 #ifdef IN_GLIBCPP_V3
diff --git a/libiberty/testsuite/demangle-expected b/libiberty/testsuite/demangle-expected
index 6ea64ae..4c6359e 100644
--- a/libiberty/testsuite/demangle-expected
+++ b/libiberty/testsuite/demangle-expected
@@ -4091,6 +4091,36 @@  void g<1>(A<1>&, B<static_cast<bool>(1)>&)
 _ZNKSt7complexIiE4realB5cxx11Ev
 std::complex<int>::real[abi:cxx11]() const
 #
+# Some more crashes revealed by fuzz-testing:
+# Check for NULL pointer when demangling trinary operators
+--format=gnu-v3
+_Z1fAv32_f
+_Z1fAv32_f
+# Do not overflow when decoding identifier length
+--format=gnu-v3
+_Z11111111111
+_Z11111111111
+# Check out-of-bounds access when decoding braced initializer list
+--format=gnu-v3
+_ZDTtl
+_ZDTtl
+# Check for NULL pointer when demangling DEMANGLE_COMPONENT_LOCAL_NAME
+--format=gnu-v3
+_ZZN1fEEd_lEv
+_ZZN1fEEd_lEv
+# Handle DEMANGLE_COMPONENT_FIXED_TYPE in d_find_pack
+--format=gnu-v3
+_Z1fDpDFT_
+_Z1fDpDFT_
+# Likewise, DEMANGLE_COMPONENT_DEFAULT_ARG
+--format=gnu-v3
+_Z1fIDpZ1fEd_E
+_Z1fIDpZ1fEd_E
+# Likewise, DEMANGLE_COMPONENT_NUMBER
+--format=gnu-v3
+_Z1fDpDv1_c
+f((char __vector(1))...)
+#
 # Ada (GNAT) tests.
 #
 # Simple test.