Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v4]

2018-12-04 Thread Pedro Alves
On 12/04/2018 01:59 PM, Nick Clifton wrote:

> OK then, here is a fourth revision of the patch.

The issue pointed out by

 https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02592.html

is still present in this version.

Also, noticed a typo here:

> +/* If DMGL_NO_RECURE_LIMIT is not enabled, then this is the value used as

Typo: "RECURE"

> +   the maximum depth of recursion allowed.  It should be enough for any
> +   real-world mangled name.  */
> +#define DEMANGLE_RECURSION_LIMIT 1024
> +  

Thanks,
Pedro Alves


Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v4]

2018-12-04 Thread Nick Clifton
Hi Ian,

>>> Shouldn't we make it fool-proof by instead introducing a 
>>> DMGL_NO_RECURSION_LIMIT

> You don't need my blessing--I wrote that code ages ago--but I agree
> with Richard that in practice it's OK to limit recursion depth by
> default.  Real symbols have very limited recursion requirements.

OK then, here is a fourth revision of the patch.

In this version:

  * The demangler option has been renamed to DMGHL_NO_RECURSE_LIMIT
and if the option is not present then the limit is enforced.

  * I also found another PR that is fixed by the patch, although I had
to make sure that the affected code could handle NULL pointers 
properly afterwards.

  OK to apply ?

Cheers
  Nick


include/ChangeLog
2018-11-29  Nick Clifton  

* demangle.h (DMGL_NO_RECURSE_LIMIT): Define.
(DEMANGLE_RECURSION_LIMIT): Define

libiberty/ChangeLog
2018-11-29  Nick Clifton  

PR 87681
PR 87675
PR 87636
PR 87350
PR 87335
* cp-demangle.h (struct d_info): Add recursion_level field.
* cp-demangle.c (d_function_type): Add recursion counter.
If the recursion limit is reached and the check is not disabled,
then return with a failure result.
(cplus_demangle_init_info): Initialise the recursion_level field.
(d_demangle_callback): If the recursion limit is enabled, check
for a mangled string that is so long that there is not enough
stack space for the local arrays.
* cplus-dem.c (struct work): Add recursion_level field.
(squangle_mop_up): Set the numb and numk fields to zero.
(work_stuff_copy_to_from): Handle the case where a btypevec or 
ktypevec field is NULL.
(demangle_nested_args): Add recursion counter.  If
the recursion limit is not disabled and reached, return with a
failure result.
Index: include/demangle.h
===
--- include/demangle.h	(revision 266771)
+++ include/demangle.h	(working copy)
@@ -68,6 +68,17 @@
 /* If none of these are set, use 'current_demangling_style' as the default. */
 #define DMGL_STYLE_MASK (DMGL_AUTO|DMGL_GNU|DMGL_LUCID|DMGL_ARM|DMGL_HP|DMGL_EDG|DMGL_GNU_V3|DMGL_JAVA|DMGL_GNAT|DMGL_DLANG|DMGL_RUST)
 
+/* Disable a limit on the depth of recursion in mangled strings.
+   Note if this limit is disabled then stack exhaustion is possible when
+   demangling pathologically complicated strings.  Bug reports about stack
+   exhaustion when the option is enabled will be rejected.  */  
+#define DMGL_NO_RECURSE_LIMIT (1 << 18)	
+
+/* If DMGL_NO_RECURE_LIMIT is not enabled, then this is the value used as
+   the maximum depth of recursion allowed.  It should be enough for any
+   real-world mangled name.  */
+#define DEMANGLE_RECURSION_LIMIT 1024
+  
 /* Enumeration of possible demangling styles.
 
Lucid and ARM styles are still kept logically distinct, even though
Index: libiberty/cp-demangle.c
===
--- libiberty/cp-demangle.c	(revision 266771)
+++ libiberty/cp-demangle.c	(working copy)
@@ -2852,21 +2852,35 @@
 static struct demangle_component *
 d_function_type (struct d_info *di)
 {
-  struct demangle_component *ret;
+  struct demangle_component *ret = NULL;
 
-  if (! d_check_char (di, 'F'))
-return NULL;
-  if (d_peek_char (di) == 'Y')
+  if ((di->options & DMGL_NO_RECURSE_LIMIT) == 0)
 {
-  /* Function has C linkage.  We don't print this information.
-	 FIXME: We should print it in verbose mode.  */
-  d_advance (di, 1);
+  if (di->recursion_level > DEMANGLE_RECURSION_LIMIT)
+	/* FIXME: There ought to be a way to report
+	   that the recursion limit has been reached.  */
+	return NULL;
+
+  di->recursion_level ++;
 }
-  ret = d_bare_function_type (di, 1);
-  ret = d_ref_qualifier (di, ret);
 
-  if (! d_check_char (di, 'E'))
-return NULL;
+  if (d_check_char (di, 'F'))
+{
+  if (d_peek_char (di) == 'Y')
+	{
+	  /* Function has C linkage.  We don't print this information.
+	 FIXME: We should print it in verbose mode.  */
+	  d_advance (di, 1);
+	}
+  ret = d_bare_function_type (di, 1);
+  ret = d_ref_qualifier (di, ret);
+  
+  if (! d_check_char (di, 'E'))
+	ret = NULL;
+}
+
+  if ((di->options & DMGL_NO_RECURSE_LIMIT) == 0)
+di->recursion_level --;
   return ret;
 }
 
@@ -6203,6 +6217,7 @@
   di->expansion = 0;
   di->is_expression = 0;
   di->is_conversion = 0;
+  di->recursion_level = 0;
 }
 
 /* Internal implementation for the demangler.  If MANGLED is a g++ v3 ABI
@@ -6242,6 +6257,20 @@
 
   cplus_demangle_init_info (mangled, options, strlen (mangled), );
 
+  /* PR 87675 - Check for a mangled string that is so long
+ that we do not have enough stack space to demangle it.  */
+  if (((options & DMGL_NO_RECURSE_LIMIT) == 0)
+  /* This check is a bit arbitrary, since what we really want to do is to
+	 compare the sizes