Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Pedro Alves]

On 12/11/2018 02:25 PM, Ian Lance Taylor wrote:
> On Tue, Dec 11, 2018 at 3:05 AM Pedro Alves  wrote:

>> Ian earlier mentioned that we've wanted to avoid malloc because some
>> programs call the demangler from a signal handler, but it seems like
>> we already do, these functions already aren't safe to use from
>> signal handlers as is.  Where does the "we can't use malloc" idea
>> come from?  Is there some entry point that avoids
>> the malloc/realloc/free calls?
> 
> cplus_demangle_v3_callback and cplus_demangle_print_callback.

Ah, gotcha.  Thanks!  Interesting.

Pedro Alves

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Ian Lance Taylor via gcc-patches]

On Tue, Dec 11, 2018 at 3:05 AM Pedro Alves  wrote:
>
> I noticed that the comment on top of __cxa_demangle says:
>
>   "If OUTPUT_BUFFER is not long enough, it is expanded using realloc."
>
> and __cxa_demangle calls 'free'.
>
> And d_demangle, seemingly the workhorse for __cxa_demangle says:
>
> /* Entry point for the demangler.  If MANGLED is a g++ v3 ABI mangled
>name, return a buffer allocated with malloc holding the demangled
>name.  OPTIONS is the usual libiberty demangler options.  On
>success, this sets *PALC to the allocated size of the returned
>buffer.  On failure, this sets *PALC to 0 for a bad name, or 1 for
>a memory allocation failure, and returns NULL.  */
>
> cplus_demangle, the entry point that gdb uses, also relies on malloc.
>
> Ian earlier mentioned that we've wanted to avoid malloc because some
> programs call the demangler from a signal handler, but it seems like
> we already do, these functions already aren't safe to use from
> signal handlers as is.  Where does the "we can't use malloc" idea
> come from?  Is there some entry point that avoids
> the malloc/realloc/free calls?

cplus_demangle_v3_callback and cplus_demangle_print_callback.

Ian

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Pedro Alves]

On 12/11/2018 06:58 AM, Jakub Jelinek wrote:
> On Mon, Dec 10, 2018 at 05:33:19PM -0700, Jeff Law wrote:
 where di.num_comps is just strlen (mangled) * 2.  Without any analysis
 whatsoever, bumping the "recursion" limit will just mean we can process 1.5
 times long names.  Either we need more precise analysis on what we are
 looking for (how big arrays we'll need) or it needs to be an independent
 limit and certainly should allow say 10KB symbols too if they are
 reasonable.
>>>
>>> If the problem is alloca, we could avoid using alloca if the size
>>> passes a threshold.  Perhaps even use a better data structure than a
>>> preallocated array based on a guess about the number of components...
>> Actually I would strongly suggest avoiding alloca completely.  This
>> isn't particularly performance sensitive code and alloca can be abused
>> in all kinds of interesting ways.
> 
> We can't use malloc, 

I noticed that the comment on top of __cxa_demangle says:

  "If OUTPUT_BUFFER is not long enough, it is expanded using realloc."

and __cxa_demangle calls 'free'.

And d_demangle, seemingly the workhorse for __cxa_demangle says:

/* Entry point for the demangler.  If MANGLED is a g++ v3 ABI mangled
   name, return a buffer allocated with malloc holding the demangled
   name.  OPTIONS is the usual libiberty demangler options.  On
   success, this sets *PALC to the allocated size of the returned
   buffer.  On failure, this sets *PALC to 0 for a bad name, or 1 for
   a memory allocation failure, and returns NULL.  */

cplus_demangle, the entry point that gdb uses, also relies on malloc.

Ian earlier mentioned that we've wanted to avoid malloc because some
programs call the demangler from a signal handler, but it seems like
we already do, these functions already aren't safe to use from
signal handlers as is.  Where does the "we can't use malloc" idea
come from?  Is there some entry point that avoids
the malloc/realloc/free calls?

Not advocating for adding to the number of malloc calls willy-nilly BTW.
I'd prefer to reduce the number of mallocs/rellocs calls within the
demangler and also within the bfd_demangle wrapper, for performance
reasons, for example by making it possible to reuse a growing buffer
across demangling invocations.

> therefore on some targets alloca (or VLAs) are the only
> option, and for small sizes even if mmap is available using it is too
> costly.
> 
> Though, I like Jason's suggestion of just adding a maxinum of the number
> of components and number of substitutions and failing if we need more.
> 
>   Jakub

Thanks,
Pedro Alves

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Pedro Alves]

On 12/11/2018 12:33 AM, Jeff Law wrote:

> Actually I would strongly suggest avoiding alloca completely.  This
> isn't particularly performance sensitive code 

On the contrary, the demangler is very performance-sensitive code for GDB.

See 
and Tromey's response.

> and alloca can be abused
> in all kinds of interesting ways.

Thanks,
Pedro Alves

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Jakub Jelinek]

On Mon, Dec 10, 2018 at 05:33:19PM -0700, Jeff Law wrote:
> >> where di.num_comps is just strlen (mangled) * 2.  Without any analysis
> >> whatsoever, bumping the "recursion" limit will just mean we can process 1.5
> >> times long names.  Either we need more precise analysis on what we are
> >> looking for (how big arrays we'll need) or it needs to be an independent
> >> limit and certainly should allow say 10KB symbols too if they are
> >> reasonable.
> > 
> > If the problem is alloca, we could avoid using alloca if the size
> > passes a threshold.  Perhaps even use a better data structure than a
> > preallocated array based on a guess about the number of components...
> Actually I would strongly suggest avoiding alloca completely.  This
> isn't particularly performance sensitive code and alloca can be abused
> in all kinds of interesting ways.

We can't use malloc, therefore on some targets alloca (or VLAs) are the only
option, and for small sizes even if mmap is available using it is too
costly.

Though, I like Jason's suggestion of just adding a maxinum of the number
of components and number of substitutions and failing if we need more.

Jakub

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Jeff Law]

On 12/10/18 8:34 AM, Jason Merrill wrote:
> On Mon, Dec 10, 2018 at 10:10 AM Jakub Jelinek  wrote:
>> On Mon, Dec 10, 2018 at 02:52:39PM +, Michael Matz wrote:
>>> On Fri, 7 Dec 2018, H.J. Lu wrote:
>>>
>> On Thu, Dec 6, 2018 at 3:12 AM Nick Clifton  wrote:
>>>
>>>   Is the patch OK with you ?
>>
>
> This caused:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88409
>

 Here is the fix.   OK for trunk?
>>>
>>> I think this points toward the limit being _much_ too low.  With template
>>> meta programming you easily get these mangled names, it's not even a
>>> particularly long one.  But I'm wondering a bit, without tracing the
>>> demangler, just looking at the symbol name and demangled result I don't
>>> readily see where the depth of recursion really is more than 1024, are
>>> there perhaps some recursion_level-- statements skipped?
>>
>> That is because the recursion_level limit isn't hit in this case at all (far
>> from it).
>>
>> What breaks it is this:
>>
>>   /* 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 of the di.comps and di.subs arrays against the
>>  amount of stack space remaining.  But there is no portable way to do
>>  this, so instead we use the recursion limit as a guide to the 
>> maximum
>>  size of the arrays.  */
>>   && (unsigned long) di.num_comps > DEMANGLE_RECURSION_LIMIT)
>> {
>>   /* FIXME: We need a way to indicate that a stack limit has been 
>> reached.  */
>>   return 0;
>> }
> 
>> where di.num_comps is just strlen (mangled) * 2.  Without any analysis
>> whatsoever, bumping the "recursion" limit will just mean we can process 1.5
>> times long names.  Either we need more precise analysis on what we are
>> looking for (how big arrays we'll need) or it needs to be an independent
>> limit and certainly should allow say 10KB symbols too if they are
>> reasonable.
> 
> If the problem is alloca, we could avoid using alloca if the size
> passes a threshold.  Perhaps even use a better data structure than a
> preallocated array based on a guess about the number of components...
Actually I would strongly suggest avoiding alloca completely.  This
isn't particularly performance sensitive code and alloca can be abused
in all kinds of interesting ways.

jeff

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Jason Merrill]

On Mon, Dec 10, 2018 at 1:55 PM Jakub Jelinek  wrote:
>
> On Mon, Dec 10, 2018 at 10:20:24AM -0800, Ian Lance Taylor wrote:
> > > I think it is a bad idea to use the same macro and value for both the
> > > recursion limit and essentially stack limit.  For the latter, it should
> > > actually compute expected stack size
> > > (i.e. di.num_comps * sizeof (*di.comps) + di.num_subs * sizeof (*di.subs))
> > > and rather than just giving up should say that memory up to 64KB this
> > > way will be handled via alloca, more through say mmap (I'd avoid malloc
> > > because some users are using these APIs in cases where malloc isn't 
> > > usable).
> > > And have only much larger limit, like say 1MB for these arrays on which to
> > > give up.
> >
> > That makes sense.
> >
> > We've wanted to avoid malloc in this code because some programs call
> > the demangler from a signal handler.  But using mmap should be fine in
> > practice.
>
> mmap is not available everywhere, but we could just have a smaller limit
> on how big mangled names we handle on targets where mmap isn't available or
> if it fails.
>
> And, the other option is just try to parse the string without doing all the
> processing first and compute (perhaps upper bound) on how many components
> and substitutions we need.  Many components have longish names, or numbers,
> etc. so the 2 * strlen is really very upper bound and strlen for number of
> substitutions too.

Or, in that situation, we could put an upper bound on the number of
components and substitutions, and fail if we end up needing more than
that.

Jason

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Jakub Jelinek]

On Mon, Dec 10, 2018 at 10:20:24AM -0800, Ian Lance Taylor wrote:
> > I think it is a bad idea to use the same macro and value for both the
> > recursion limit and essentially stack limit.  For the latter, it should
> > actually compute expected stack size
> > (i.e. di.num_comps * sizeof (*di.comps) + di.num_subs * sizeof (*di.subs))
> > and rather than just giving up should say that memory up to 64KB this
> > way will be handled via alloca, more through say mmap (I'd avoid malloc
> > because some users are using these APIs in cases where malloc isn't usable).
> > And have only much larger limit, like say 1MB for these arrays on which to
> > give up.
> 
> That makes sense.
> 
> We've wanted to avoid malloc in this code because some programs call
> the demangler from a signal handler.  But using mmap should be fine in
> practice.

mmap is not available everywhere, but we could just have a smaller limit
on how big mangled names we handle on targets where mmap isn't available or
if it fails.

And, the other option is just try to parse the string without doing all the
processing first and compute (perhaps upper bound) on how many components
and substitutions we need.  Many components have longish names, or numbers,
etc. so the 2 * strlen is really very upper bound and strlen for number of
substitutions too.

Jakub

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Ian Lance Taylor via gcc-patches]

On Mon, Dec 10, 2018 at 7:35 AM Jakub Jelinek  wrote:
>
> On Mon, Dec 10, 2018 at 03:26:18PM +, Nick Clifton wrote:
> > >> My current suggestion
> > >> is to raise the limit to 2048, which allows the libiberty patch to
> > >> pass.  But do you have a feel for how much is a realistic limit ?
> > >
> > > For recursion limit I think that is fine.
> > > For just stack size limit, I think it is extremely small.
> > > I see that in the function it allocates on 64-bit 24 bytes times
> > > num_comps using alloca, so 48 bytes per character in the mangled name,
> > > and a pointer for each character in the mangled name.
> > > That is 112KB per 2048 bytes long mangled name.
> > >
> > > Dunno how much stack can we expect to be usable.
> >
> > Currently the patched libiberty uses the DEMANGLE_RECURSION_LIMIT value
> > in two ways.  The first is as a limit on the number of levels of recursion
> > of specific functions inside the demangler.  The second is as a check on
> > the number of component structures that will be allocated on the stack.
> > (See cp-demangle.c:d_demangle_callback).  One of the CVEs that I was 
> > checking
> > was triggering stack exhaustion this way, which is why I added the check.
> >
> > There is at least one other function where a similar stack allocation
> > happens (cplus_demangle_print_callback) but I was not sure if this could
> > be triggered with the other limits in place, and I did not have a reproducer
> > that touched it, so I left it alone.
>
> I think it is a bad idea to use the same macro and value for both the
> recursion limit and essentially stack limit.  For the latter, it should
> actually compute expected stack size
> (i.e. di.num_comps * sizeof (*di.comps) + di.num_subs * sizeof (*di.subs))
> and rather than just giving up should say that memory up to 64KB this
> way will be handled via alloca, more through say mmap (I'd avoid malloc
> because some users are using these APIs in cases where malloc isn't usable).
> And have only much larger limit, like say 1MB for these arrays on which to
> give up.

That makes sense.

We've wanted to avoid malloc in this code because some programs call
the demangler from a signal handler.  But using mmap should be fine in
practice.

Ian

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Jakub Jelinek]

On Mon, Dec 10, 2018 at 03:26:18PM +, Nick Clifton wrote:
> >> My current suggestion
> >> is to raise the limit to 2048, which allows the libiberty patch to 
> >> pass.  But do you have a feel for how much is a realistic limit ?
> > 
> > For recursion limit I think that is fine.
> > For just stack size limit, I think it is extremely small.
> > I see that in the function it allocates on 64-bit 24 bytes times
> > num_comps using alloca, so 48 bytes per character in the mangled name,
> > and a pointer for each character in the mangled name.
> > That is 112KB per 2048 bytes long mangled name.
> > 
> > Dunno how much stack can we expect to be usable.
> 
> Currently the patched libiberty uses the DEMANGLE_RECURSION_LIMIT value
> in two ways.  The first is as a limit on the number of levels of recursion
> of specific functions inside the demangler.  The second is as a check on
> the number of component structures that will be allocated on the stack.
> (See cp-demangle.c:d_demangle_callback).  One of the CVEs that I was checking
> was triggering stack exhaustion this way, which is why I added the check.
> 
> There is at least one other function where a similar stack allocation
> happens (cplus_demangle_print_callback) but I was not sure if this could
> be triggered with the other limits in place, and I did not have a reproducer
> that touched it, so I left it alone.

I think it is a bad idea to use the same macro and value for both the
recursion limit and essentially stack limit.  For the latter, it should
actually compute expected stack size
(i.e. di.num_comps * sizeof (*di.comps) + di.num_subs * sizeof (*di.subs))
and rather than just giving up should say that memory up to 64KB this
way will be handled via alloca, more through say mmap (I'd avoid malloc
because some users are using these APIs in cases where malloc isn't usable).
And have only much larger limit, like say 1MB for these arrays on which to
give up.

Jakub

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Jason Merrill]

On Mon, Dec 10, 2018 at 10:10 AM Jakub Jelinek  wrote:
> On Mon, Dec 10, 2018 at 02:52:39PM +, Michael Matz wrote:
> > On Fri, 7 Dec 2018, H.J. Lu wrote:
> >
> > > > > On Thu, Dec 6, 2018 at 3:12 AM Nick Clifton  wrote:
> > > > > >
> > > > > >   Is the patch OK with you ?
> > > > >
> > > >
> > > > This caused:
> > > >
> > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88409
> > > >
> > >
> > > Here is the fix.   OK for trunk?
> >
> > I think this points toward the limit being _much_ too low.  With template
> > meta programming you easily get these mangled names, it's not even a
> > particularly long one.  But I'm wondering a bit, without tracing the
> > demangler, just looking at the symbol name and demangled result I don't
> > readily see where the depth of recursion really is more than 1024, are
> > there perhaps some recursion_level-- statements skipped?
>
> That is because the recursion_level limit isn't hit in this case at all (far
> from it).
>
> What breaks it is this:
>
>   /* 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 of the di.comps and di.subs arrays against the
>  amount of stack space remaining.  But there is no portable way to do
>  this, so instead we use the recursion limit as a guide to the maximum
>  size of the arrays.  */
>   && (unsigned long) di.num_comps > DEMANGLE_RECURSION_LIMIT)
> {
>   /* FIXME: We need a way to indicate that a stack limit has been 
> reached.  */
>   return 0;
> }

> where di.num_comps is just strlen (mangled) * 2.  Without any analysis
> whatsoever, bumping the "recursion" limit will just mean we can process 1.5
> times long names.  Either we need more precise analysis on what we are
> looking for (how big arrays we'll need) or it needs to be an independent
> limit and certainly should allow say 10KB symbols too if they are
> reasonable.

If the problem is alloca, we could avoid using alloca if the size
passes a threshold.  Perhaps even use a better data structure than a
preallocated array based on a guess about the number of components...

Jason

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Nick Clifton]

Hi David,

> Apologies in advance if this has been covered, as I've only been half-
> watching this thread, but is it always the case that the recursion
> depth can be bounded by some scalar multiple of the number of
> characters in the name?

Probably, but the point of this patch is to add a fixed limit that
prevents too much recursion from being performed.  The CVEs that I
have been trying to fix have been using mangled names with 20K-30K
characters in them, so creating a recursion limit based on the 
length of the input would not prevent the stack exhaustion. :-(

My hope is that we can choose a value that will allow any realistic
mangled name to be decoded, but which will prevent these fuzzers from
generating arbitrary length strings which exhaust the machines resources.

Cheers
  Nick

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Nick Clifton]

Hi Jakub,

>> My current suggestion
>> is to raise the limit to 2048, which allows the libiberty patch to 
>> pass.  But do you have a feel for how much is a realistic limit ?
> 
> For recursion limit I think that is fine.
> For just stack size limit, I think it is extremely small.
> I see that in the function it allocates on 64-bit 24 bytes times
> num_comps using alloca, so 48 bytes per character in the mangled name,
> and a pointer for each character in the mangled name.
> That is 112KB per 2048 bytes long mangled name.
> 
> Dunno how much stack can we expect to be usable.

Currently the patched libiberty uses the DEMANGLE_RECURSION_LIMIT value
in two ways.  The first is as a limit on the number of levels of recursion
of specific functions inside the demangler.  The second is as a check on
the number of component structures that will be allocated on the stack.
(See cp-demangle.c:d_demangle_callback).  One of the CVEs that I was checking
was triggering stack exhaustion this way, which is why I added the check.

There is at least one other function where a similar stack allocation
happens (cplus_demangle_print_callback) but I was not sure if this could
be triggered with the other limits in place, and I did not have a reproducer
that touched it, so I left it alone.

Cheers
  Nick

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[David Malcolm]

On Mon, 2018-12-10 at 14:52 +, Michael Matz wrote:
> Hi,
> 
> On Fri, 7 Dec 2018, H.J. Lu wrote:
> 
> > > > On Thu, Dec 6, 2018 at 3:12 AM Nick Clifton 
> > > > wrote:
> > > > > 
> > > > >   Is the patch OK with you ?
> > > 
> > > This caused:
> > > 
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88409
> > > 
> > 
> > Here is the fix.   OK for trunk?
> 
> I think this points toward the limit being _much_ too low.  With
> template 
> meta programming you easily get these mangled names, it's not even a 
> particularly long one.  But I'm wondering a bit, without tracing the 
> demangler, just looking at the symbol name and demangled result I
> don't 
> readily see where the depth of recursion really is more than 1024,
> are 
> there perhaps some recursion_level-- statements skipped?

Apologies in advance if this has been covered, as I've only been half-
watching this thread, but is it always the case that the recursion
depth can be bounded by some scalar multiple of the number of
characters in the name?

The name that's causing trouble is 654 characters long, and the
proposed limit of 1306 is slightly below double that.  There may well
be a bug in the implementation as Michael points out, but is the
recursion depth always guaranteed to be less than 2 * num_chars seen,
or somesuch limit.  If so, could the limit be dynamic, rather than
hardcoded?  That would trap cases where we're consuming stack frames
without consuming input characters, and eliminate having a hardcoded
limit that's bound to eventually become wrong as people write more and
more complicated C++ programs.

Hope this is constructive
Dave

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Jakub Jelinek]

On Mon, Dec 10, 2018 at 03:12:53PM +, Nick Clifton wrote:
> Hi Michael,
> 
> > I think this points toward the limit being _much_ too low.
> 
> Fair enough - several other people have said this as well.  So
> I have proposed an alternative patch instead.  My current suggestion
> is to raise the limit to 2048, which allows the libiberty patch to 
> pass.  But do you have a feel for how much is a realistic limit ?

For recursion limit I think that is fine.
For just stack size limit, I think it is extremely small.
I see that in the function it allocates on 64-bit 24 bytes times
num_comps using alloca, so 48 bytes per character in the mangled name,
and a pointer for each character in the mangled name.
That is 112KB per 2048 bytes long mangled name.

Dunno how much stack can we expect to be usable.

Jakub

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Nick Clifton]

Hi Michael,

> I think this points toward the limit being _much_ too low.

Fair enough - several other people have said this as well.  So
I have proposed an alternative patch instead.  My current suggestion
is to raise the limit to 2048, which allows the libiberty patch to 
pass.  But do you have a feel for how much is a realistic limit ?


> demangler, just looking at the symbol name and demangled result I don't 
> readily see where the depth of recursion really is more than 1024, are 
> there perhaps some recursion_level-- statements skipped?

I do not think so.  Unless there are some long jumps in the demangling code ?
I did a quick scan and did not find any, but I could have missed something.
Plus of course I cannot guarantee that my patch is bug free, although looking
at it again I do not see where I missed a level decrement.  

I think that the demangling code just is really recursive...

Cheers
  Nick

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Jakub Jelinek]

On Mon, Dec 10, 2018 at 02:52:39PM +, Michael Matz wrote:
> On Fri, 7 Dec 2018, H.J. Lu wrote:
> 
> > > > On Thu, Dec 6, 2018 at 3:12 AM Nick Clifton  wrote:
> > > > >
> > > > >   Is the patch OK with you ?
> > > >
> > >
> > > This caused:
> > >
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88409
> > >
> > 
> > Here is the fix.   OK for trunk?
> 
> I think this points toward the limit being _much_ too low.  With template 
> meta programming you easily get these mangled names, it's not even a 
> particularly long one.  But I'm wondering a bit, without tracing the 
> demangler, just looking at the symbol name and demangled result I don't 
> readily see where the depth of recursion really is more than 1024, are 
> there perhaps some recursion_level-- statements skipped?

That is because the recursion_level limit isn't hit in this case at all (far
from it).

What breaks it is this:

  /* 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 of the di.comps and di.subs arrays against the
 amount of stack space remaining.  But there is no portable way to do
 this, so instead we use the recursion limit as a guide to the maximum
 size of the arrays.  */
  && (unsigned long) di.num_comps > DEMANGLE_RECURSION_LIMIT)
{
  /* FIXME: We need a way to indicate that a stack limit has been reached.  
*/
  return 0;
}

where di.num_comps is just strlen (mangled) * 2.  Without any analysis
whatsoever, bumping the "recursion" limit will just mean we can process 1.5
times long names.  Either we need more precise analysis on what we are
looking for (how big arrays we'll need) or it needs to be an independent
limit and certainly should allow say 10KB symbols too if they are
reasonable.

Jakub

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Michael Matz]

Hi,

On Fri, 7 Dec 2018, H.J. Lu wrote:

> > > On Thu, Dec 6, 2018 at 3:12 AM Nick Clifton  wrote:
> > > >
> > > >   Is the patch OK with you ?
> > >
> >
> > This caused:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88409
> >
> 
> Here is the fix.   OK for trunk?

I think this points toward the limit being _much_ too low.  With template 
meta programming you easily get these mangled names, it's not even a 
particularly long one.  But I'm wondering a bit, without tracing the 
demangler, just looking at the symbol name and demangled result I don't 
readily see where the depth of recursion really is more than 1024, are 
there perhaps some recursion_level-- statements skipped?


Ciao,
Michael.

[PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[H.J. Lu]

On Fri, Dec 7, 2018 at 8:17 AM H.J. Lu  wrote:
>
> On Thu, Dec 6, 2018 at 10:04 AM Ian Lance Taylor via gcc-patches
>  wrote:
> >
> > On Thu, Dec 6, 2018 at 3:12 AM Nick Clifton  wrote:
> > >
> > >   Is the patch OK with you ?
> >
>
> This caused:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88409
>

Here is the fix.   OK for trunk?

Thanks.

-- 
H.J.
From 676dc7f98d0c191e550f87df70393116d9e19ccb Mon Sep 17 00:00:00 2001
From: "H.J. Lu" 
Date: Fri, 7 Dec 2018 08:20:45 -0800
Subject: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

Define DEMANGLE_RECURSION_LIMIT to 1536 for

_ZN4modc6parser8sequenceINS_9astParser13LocatedParserINS0_9ParserRefINS2_UlRNS2_16TokenParserInputEE_EINS0_14OptionalParserINS2_18ListParserTemplateILNS_6tokens5Token4TypeE4EXadL_ZNSD_Ut_13parenthesized6ParserINS4_INS0_6ParserIS5_NS_3ast10ExpressionENSA_INS4_INS2_22OneOfKeywordsToTParserINSJ_5StyleEEENS0_14SequenceParserIS5_INS0_18ExactElementParserIS5_EENSA_ISM_ENS0_14RepeatedParserINS4_INS0_15TransformParserINSU_IS5_INS4_INSP_INSJ_10Annotation12RelationshipESX_EEENS2_UlNS2_3LocES12_ONS_5MaybeISK_EEE19_ELb0EENSU_INS0_17ExtractParserTypeIT_E9InputTypeEINS0_8MaybeRefIS1F_E4TypeEDpNS1I_IT0_E4TypeOS1F_DpOS1L_

the recursion level can reach 1306.

	PR other/88409
	* demangle.h (DEMANGLE_RECURSION_LIMIT): Set to 1536.
---
 include/demangle.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/demangle.h b/include/demangle.h
index 1e67fe2fb3..d9de074bef 100644
--- a/include/demangle.h
+++ b/include/demangle.h
@@ -77,7 +77,7 @@ extern "C" {
 /* If DMGL_NO_RECURSE_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
+#define DEMANGLE_RECURSION_LIMIT 1536
   
 /* Enumeration of possible demangling styles.
 
-- 
2.19.2