diff mbox series

RFA: Patch to fix severe recursion in d_count_templates_scopes (PR 89394)

Message ID 87imwctovn.fsf@redhat.com
State New
Headers show
Series RFA: Patch to fix severe recursion in d_count_templates_scopes (PR 89394) | expand

Commit Message

Nick Clifton March 21, 2019, 12:04 p.m. UTC 
Hi Ian,

  Attached is a proposed patch to fix PR 89394, which contains an
  artificial mangled name that triggers excessive recursion in
  d_count_templates_scopes.  The patch uses the same recursion limit
  that is already in place for d_print_comp, which I hope will be
  acceptable.

  There is one frag in the patch which is not directly related to this
  recursion problem however.  It extends the check in
  cplus_demangle_fill_name so that names with a negative length are
  rejected.  I had originally thought that the excessive recursion was
  due to a negative length string, although further investigation proved
  this guess to be wrong.  I felt that leaving the check in however
  would still be a good idea.

  Tested with no regressions with an x86_64-linux-gnu toolchain, as well
  as against the testcase in PR 89394.

  OK to apply ?

Cheers
  Nick

libiberty/ChangeLog
2019-03-21  Nick Clifton  <nickc@redhat.com>

	PR 89394
	* cp-demangle.c (cplus_demangle_fill_name): Reject negative
	lengths.
	(d_count_templates_scopes): Replace num_templates and num_scopes
	parameters with a struct d_print_info pointer parameter.  Adjust
	body of the function accordingly.  Add recursion counter and check
	that the recursion limit is not reached.
	(d_print_init): Pass dpi parameter to d_count_templates_scopes.
	Reset recursion counter afterwards, unless the recursion limit was
	reached.

Comments

Jeff Law March 29, 2019, 9:54 p.m. UTC | #1 
On 3/21/19 6:04 AM, Nick Clifton wrote:
> Hi Ian,
> 
>   Attached is a proposed patch to fix PR 89394, which contains an
>   artificial mangled name that triggers excessive recursion in
>   d_count_templates_scopes.  The patch uses the same recursion limit
>   that is already in place for d_print_comp, which I hope will be
>   acceptable.
> 
>   There is one frag in the patch which is not directly related to this
>   recursion problem however.  It extends the check in
>   cplus_demangle_fill_name so that names with a negative length are
>   rejected.  I had originally thought that the excessive recursion was
>   due to a negative length string, although further investigation proved
>   this guess to be wrong.  I felt that leaving the check in however
>   would still be a good idea.
> 
>   Tested with no regressions with an x86_64-linux-gnu toolchain, as well
>   as against the testcase in PR 89394.
> 
>   OK to apply ?
> 
> Cheers
>   Nick
> 
> libiberty/ChangeLog
> 2019-03-21  Nick Clifton  <nickc@redhat.com>
> 
> 	PR 89394
> 	* cp-demangle.c (cplus_demangle_fill_name): Reject negative
> 	lengths.
> 	(d_count_templates_scopes): Replace num_templates and num_scopes
> 	parameters with a struct d_print_info pointer parameter.  Adjust
> 	body of the function accordingly.  Add recursion counter and check
> 	that the recursion limit is not reached.
> 	(d_print_init): Pass dpi parameter to d_count_templates_scopes.
> 	Reset recursion counter afterwards, unless the recursion limit was
> 	reached.
> It's actually a fairly trivial patch once you know that d_print_init
sets up dpi :-)

Given some folks seem to think this is a security issue, I'm going to
ACK for gcc-9 even though it's not a regression.

Jeff
diff mbox series

Patch

Index: libiberty/cp-demangle.c
===================================================================
--- libiberty/cp-demangle.c	(revision 269832)
+++ libiberty/cp-demangle.c	(working copy)
@@ -861,7 +861,7 @@ 
 int
 cplus_demangle_fill_name (struct demangle_component *p, const char *s, int len)
 {
-  if (p == NULL || s == NULL || len == 0)
+  if (p == NULL || s == NULL || len <= 0)
     return 0;
   p->d_printing = 0;
   p->type = DEMANGLE_COMPONENT_NAME;
@@ -4061,7 +4061,7 @@ 
    are larger than the actual numbers encountered.  */
 
 static void
-d_count_templates_scopes (int *num_templates, int *num_scopes,
+d_count_templates_scopes (struct d_print_info *dpi,
 			  const struct demangle_component *dc)
 {
   if (dc == NULL)
@@ -4081,13 +4081,13 @@ 
       break;
 
     case DEMANGLE_COMPONENT_TEMPLATE:
-      (*num_templates)++;
+      dpi->num_copy_templates++;
       goto recurse_left_right;
 
     case DEMANGLE_COMPONENT_REFERENCE:
     case DEMANGLE_COMPONENT_RVALUE_REFERENCE:
       if (d_left (dc)->type == DEMANGLE_COMPONENT_TEMPLATE_PARAM)
-	(*num_scopes)++;
+	dpi->num_saved_scopes++;
       goto recurse_left_right;
 
     case DEMANGLE_COMPONENT_QUAL_NAME:
@@ -4152,42 +4152,42 @@ 
     case DEMANGLE_COMPONENT_TAGGED_NAME:
     case DEMANGLE_COMPONENT_CLONE:
     recurse_left_right:
-      d_count_templates_scopes (num_templates, num_scopes,
-				d_left (dc));
-      d_count_templates_scopes (num_templates, num_scopes,
-				d_right (dc));
+      /* PR 89394 - Check for too much recursion.  */
+      if (dpi->recursion > DEMANGLE_RECURSION_LIMIT)
+	/* FIXME: There ought to be a way to report to the
+	   user that the recursion limit has been reached.  */
+	return;
+
+      ++ dpi->recursion;
+      d_count_templates_scopes (dpi, d_left (dc));
+      d_count_templates_scopes (dpi, d_right (dc));
+      -- dpi->recursion;
       break;
 
     case DEMANGLE_COMPONENT_CTOR:
-      d_count_templates_scopes (num_templates, num_scopes,
-				dc->u.s_ctor.name);
+      d_count_templates_scopes (dpi, dc->u.s_ctor.name);
       break;
 
     case DEMANGLE_COMPONENT_DTOR:
-      d_count_templates_scopes (num_templates, num_scopes,
-				dc->u.s_dtor.name);
+      d_count_templates_scopes (dpi, dc->u.s_dtor.name);
       break;
 
     case DEMANGLE_COMPONENT_EXTENDED_OPERATOR:
-      d_count_templates_scopes (num_templates, num_scopes,
-				dc->u.s_extended_operator.name);
+      d_count_templates_scopes (dpi, dc->u.s_extended_operator.name);
       break;
 
     case DEMANGLE_COMPONENT_FIXED_TYPE:
-      d_count_templates_scopes (num_templates, num_scopes,
-                                dc->u.s_fixed.length);
+      d_count_templates_scopes (dpi, dc->u.s_fixed.length);
       break;
 
     case DEMANGLE_COMPONENT_GLOBAL_CONSTRUCTORS:
     case DEMANGLE_COMPONENT_GLOBAL_DESTRUCTORS:
-      d_count_templates_scopes (num_templates, num_scopes,
-				d_left (dc));
+      d_count_templates_scopes (dpi, d_left (dc));
       break;
 
     case DEMANGLE_COMPONENT_LAMBDA:
     case DEMANGLE_COMPONENT_DEFAULT_ARG:
-      d_count_templates_scopes (num_templates, num_scopes,
-				dc->u.s_unary_num.sub);
+      d_count_templates_scopes (dpi, dc->u.s_unary_num.sub);
       break;
     }
 }
@@ -4222,8 +4222,12 @@ 
   dpi->next_copy_template = 0;
   dpi->num_copy_templates = 0;
 
-  d_count_templates_scopes (&dpi->num_copy_templates,
-			    &dpi->num_saved_scopes, dc);
+  d_count_templates_scopes (dpi, dc);
+  /* If we did not reach the recursion limit, then reset the
+     current recursion value back to 0, so that we can print
+     the templates.  */
+  if (dpi->recursion < DEMANGLE_RECURSION_LIMIT)
+    dpi->recursion = 0;
   dpi->num_copy_templates *= dpi->num_saved_scopes;
 
   dpi->current_template = NULL;