RFA/RFC: Add stack recursion limit to libiberty's demangler

classic Classic list List threaded Threaded
59 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Re: RFC: libiberty PATCH to disable demangling of ancient mangling schemes

Eric Gallager
On 12/7/18, Jason Merrill <[hidden email]> wrote:

> On 12/7/18 6:36 AM, Richard Biener wrote:
>> On Thu, Dec 6, 2018 at 10:22 PM Jason Merrill <[hidden email]> wrote:
>>>
>>> On Thu, Dec 6, 2018 at 11:14 AM Jason Merrill <[hidden email]> wrote:
>>>>
>>>> Looks good to me.  Independently, do you see a reason not to disable
>>>> the
>>>> old demangler entirely?
>>>
>>> Like so.  Does anyone object to this?  These mangling schemes haven't
>>> been relevant in decades.
>>
>> Why #ifdef the code?  Just rip it out?
>
> I was thinking as an intermediate measure in case some user wanted it
> for some reason, but I'd be fine with that as well.
>
> Jason
>

A compromise could be to do the #ifdef for GCC 9, see if anyone
complains, and then if no one complains, rip it out entirely for GCC
10.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

Michael Matz
In reply to this post by H.J. Lu-30
Hi,

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

> > > On Thu, Dec 6, 2018 at 3:12 AM Nick Clifton <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

Jakub Jelinek
On Mon, Dec 10, 2018 at 02:52:39PM +0000, Michael Matz wrote:

> On Fri, 7 Dec 2018, H.J. Lu wrote:
>
> > > > On Thu, Dec 6, 2018 at 3:12 AM Nick Clifton <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

Nick Clifton
In reply to this post by Michael Matz
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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

David Malcolm
In reply to this post by Michael Matz
On Mon, 2018-12-10 at 14:52 +0000, Michael Matz wrote:

> Hi,
>
> On Fri, 7 Dec 2018, H.J. Lu wrote:
>
> > > > On Thu, Dec 6, 2018 at 3:12 AM Nick Clifton <[hidden email]>
> > > > 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

Jakub Jelinek
In reply to this post by Nick Clifton
On Mon, Dec 10, 2018 at 03:12:53PM +0000, 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
Reply | Threaded
Open this post in threaded view
|

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


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

Nick Clifton
In reply to this post by David Malcolm
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






Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

Jason Merrill
In reply to this post by Jakub Jelinek
On Mon, Dec 10, 2018 at 10:10 AM Jakub Jelinek <[hidden email]> wrote:

> On Mon, Dec 10, 2018 at 02:52:39PM +0000, Michael Matz wrote:
> > On Fri, 7 Dec 2018, H.J. Lu wrote:
> >
> > > > > On Thu, Dec 6, 2018 at 3:12 AM Nick Clifton <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

Jakub Jelinek
In reply to this post by Nick Clifton
On Mon, Dec 10, 2018 at 03:26:18PM +0000, 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

Sourceware - binutils list mailing list
On Mon, Dec 10, 2018 at 7:35 AM Jakub Jelinek <[hidden email]> wrote:

>
> On Mon, Dec 10, 2018 at 03:26:18PM +0000, 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
Reply | Threaded
Open this post in threaded view
|

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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

Jason Merrill
On Mon, Dec 10, 2018 at 1:55 PM Jakub Jelinek <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

Jeff Law
In reply to this post by Jason Merrill
On 12/10/18 8:34 AM, Jason Merrill wrote:

> On Mon, Dec 10, 2018 at 10:10 AM Jakub Jelinek <[hidden email]> wrote:
>> On Mon, Dec 10, 2018 at 02:52:39PM +0000, Michael Matz wrote:
>>> On Fri, 7 Dec 2018, H.J. Lu wrote:
>>>
>>>>>> On Thu, Dec 6, 2018 at 3:12 AM Nick Clifton <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

Pedro Alves-7
In reply to this post by Jeff Law
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 <https://sourceware.org/ml/binutils/2018-11/msg00257.html>
and Tromey's response.

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

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

Pedro Alves-7
In reply to this post by Jakub Jelinek
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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

Sourceware - binutils list mailing list
On Tue, Dec 11, 2018 at 3:05 AM Pedro Alves <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

Pedro Alves-7
On 12/11/2018 02:25 PM, Ian Lance Taylor wrote:
> On Tue, Dec 11, 2018 at 3:05 AM Pedro Alves <[hidden email]> 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
123