Fix fir PR71696 in Libiberty Demangler (6)

Hi,

The attached patch fixes the stack overflow in the demangler due to cycles in the references of “remembered” mangled types (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71696).

The methods demangle_signature and do_arg in cplus-dem.c allow to “remember” mangled type names that can later be referenced and will also be demangled. The method demangle_args demangles those types following any references. So, if there is a cycle in the referencing (or in the simplest case a self-reference), the method enters infinite recursion.

The patch tracks the mangled types that are currently being demangled in a new variable called work->proctypevec. If a referenced type is currently being demangled, the demangling is marked as not successful.

Bootstrapped and regression tested on x86_64-pc-linux-gnu. Test case added to libiberty/testsuite/demangler-expected and checked PR71696 is resolved.

On 06/30/2016 08:46 AM, Marcel Böhme wrote:
> The attached patch fixes the stack overflow in the demangler due to
> cycles in the references of “remembered” mangled types
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71696).
>
> The methods demangle_signature and do_arg in cplus-dem.c allow to
> “remember” mangled type names that can later be referenced and will
> also be demangled. The method demangle_args demangles those types
> following any references. So, if there is a cycle in the referencing
> (or in the simplest case a self-reference), the method enters
> infinite recursion.
>
> The patch tracks the mangled types that are currently being demangled
> in a new variable called work->proctypevec. If a referenced type is
> currently being demangled, the demangling is marked as not
> successful.

I'll defer reviewing these to someone who understands demangling better. 
You might want to Cc Jason.


Bernd

Hi,

This patch is still pending a full review.

Best regards,
- Marcel

> On 4 Jul 2016, at 8:54 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> 
> On 06/30/2016 08:46 AM, Marcel Böhme wrote:
>> The attached patch fixes the stack overflow in the demangler due to
>> cycles in the references of “remembered” mangled types
>> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71696).
>> 
>> The methods demangle_signature and do_arg in cplus-dem.c allow to
>> “remember” mangled type names that can later be referenced and will
>> also be demangled. The method demangle_args demangles those types
>> following any references. So, if there is a cycle in the referencing
>> (or in the simplest case a self-reference), the method enters
>> infinite recursion.
>> 
>> The patch tracks the mangled types that are currently being demangled
>> in a new variable called work->proctypevec. If a referenced type is
>> currently being demangled, the demangling is marked as not
>> successful.
> 
> I'll defer reviewing these to someone who understands demangling better. You might want to Cc Jason.
> 
> 
> Bernd
> 
>

On 06/30/2016 12:46 AM, Marcel Böhme wrote:
> Hi,
>
> The attached patch fixes the stack overflow in the demangler due to cycles in the references of “remembered” mangled types (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71696).
>
> The methods demangle_signature and do_arg in cplus-dem.c allow to “remember” mangled type names that can later be referenced and will also be demangled. The method demangle_args demangles those types following any references. So, if there is a cycle in the referencing (or in the simplest case a self-reference), the method enters infinite recursion.
>
> The patch tracks the mangled types that are currently being demangled in a new variable called work->proctypevec. If a referenced type is currently being demangled, the demangling is marked as not successful.
>
> Bootstrapped and regression tested on x86_64-pc-linux-gnu. Test case added to libiberty/testsuite/demangler-expected and checked PR71696 is resolved.
>
>
> Index: libiberty/ChangeLog
> ===================================================================
> --- libiberty/ChangeLog	(revision 237852)
> +++ libiberty/ChangeLog	(working copy)
> @@ -1,3 +1,21 @@
> +2016-06-30  Marcel Böhme <boehme.marcel@gmail.com>
> +
> +	* cplus-dem.c: Prevent infinite recursion when there is a cycle
> +	in the referencing of remembered mangled types.
> +	(work_stuff): New stack to keep track of the remembered mangled
> +	types that are currently being processed.
> +	(push_processed_type): New method to push currently processed
> +	remembered type onto the stack.
> +	(pop_processed_type): New method to pop currently processed
> +	remembered type from the stack.
> +	(work_stuff_copy_to_from): Copy values of new variables.
> +	(delete_non_B_K_work_stuff): Free stack memory.
> +	(demangle_args): Push/Pop currently processed remembered type.
> +	(do_type): Do not demangle a cyclic reference and push/pop
> +	referenced remembered type.
> +
> +	* testsuite/demangle-expected: Add tests.
> +
> @@ -1302,6 +1309,13 @@ work_stuff_copy_to_from (struct work_stuff *to, st
>        memcpy (to->btypevec[i], from->btypevec[i], len);
>      }
>
> +  if (from->proctypevec)
> +    {
> +      to->proctypevec = XNEWVEC (int, from->proctypevec_size);
> +      memcpy (to->proctypevec, from->proctypevec,
> +	      from->proctypevec_size * sizeof(int));
> +    }
Isn't this

  to->proctypevec
    = XDUPVEC (int, from->proctypevec, from->proctypevec_size);


> @@ -1336,6 +1350,12 @@ delete_non_B_K_work_stuff (struct work_stuff *work
>        work -> typevec = NULL;
>        work -> typevec_size = 0;
>      }
> +  if (work -> proctypevec != NULL)
> +    {
> +      free (work -> proctypevec);
> +      work -> proctypevec = NULL;
> +      work -> proctypevec_size = 0;
> +    }
Whitespace nits.  No whitespace around the "->".  Can you fix the one 
immediately above your new code as well?  (there's many others in 
cplus-dem.c, feel free to fix those as an independent patch).

I see similar whitespace nits in your changes to do_type.


> @@ -3566,6 +3588,7 @@ do_type (struct work_stuff *work, const char **man
>
>    done = 0;
>    success = 1;
> +  is_proctypevec = 0;
>    while (success && !done)
>      {
>        int member;
> @@ -3626,7 +3649,14 @@ do_type (struct work_stuff *work, const char **man
>  	      success = 0;
>  	    }
>  	  else
> -	    {
> +	    for (i = 0; i < work -> nproctypes; i++)

> +	      if (work -> proctypevec [i] == n)
> +	        success = 0;
So presumably this doesn't happen all that often or this could get 
expensive and we'd want something more efficient for searching, right?

>  static void
> +push_processed_type (struct work_stuff *work, int typevec_index)
> +{
> +  if (work -> nproctypes >= work -> proctypevec_size)
> +    {
> +      if (work -> proctypevec_size == 0)
> +	{
> +	  work -> proctypevec_size = 3;
> +	  work -> proctypevec = XNEWVEC (int, work -> proctypevec_size);
> +	}
> +      else
> +	{
> +	  if (work -> proctypevec_size > INT_MAX / 2)
> +            xmalloc_failed (INT_MAX);
> +          work -> proctypevec_size *= 2;
> +          work -> proctypevec
> +            = XRESIZEVEC (int, work->proctypevec, work->proctypevec_size);
> +        }
> +    }
> +    work -> proctypevec [work -> nproctypes ++] = typevec_index;
"->" whitespace nits all over the place here.  I thought we had a better 
growing heuristic than just double every time..  Hmm, in gcc/vec.c we have:

   /* Exponential growth. */
   if (!alloc)
     alloc = 4;
   else if (alloc < 16)
     /* Double when small.  */
     alloc = alloc * 2;
   else
     /* Grow slower when large.  */
     alloc = (alloc * 3 / 2);

   /* If this is still too small, set it to the right size. */
   if (alloc < desired)
     alloc = desired;

Let's go with something similar here.



> +}
> +
> +static void
> +pop_processed_type (struct work_stuff *work)
> +{
> +  work -> nproctypes --;
No whitespace around the "->" or "--".

Can you take care of the minor issues above, retest & repost?

THanks,
Jeff