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