Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

["Ian Lance Taylor via binutils" <binutils at sourceware dot org>]

On Mon, Dec 10, 2018 at 7:35 AM Jakub Jelinek <jakub@redhat.com> 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