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: RFA/RFC: Add stack recursion limit to libiberty's demangler [v3]

Nick Clifton
Hi Richard,

>>   * The description of the DMGL_RECURSE_LIMIT option in demangle.h has
>>     been enhanced to add a note that if the option is not used, then
>>     bug reports about stack overflows in the demangler will be rejected.
>
> Shouldn't we make it fool-proof by instead introducing a DMGL_NO_RECURSION_LIMIT
> flag and when not set default to limiting recursion?

Well I wanted the patch to be backwards compatible. Just on the
general principle of not surprising users/programmers by changing
things without telling them first.

I could change this of course, but I would rather have Ian's blessing
first.

Cheers
  Nick
Reply | Threaded
Open this post in threaded view
|

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

Nick Clifton
In reply to this post by Cary Coutant-3
Hi Cary,

> In order to handle arbitrary user input without crashing, perhaps the
> demangler should switch from recursive descent parsing to a state
> machine, where exhaustion of resources can be handled gracefully.

I think that that would be a better long term fix for the problem,
but it is not one that I have time to work on right now.

My main goal with this patch submission is to stop the flood of PR
and CVEs about mangled inputs that trigger stack exhaustion.  Being
able to properly demangle such inputs would be nice, but not something
that I think should be a priority.  I think that in real life no
program is ever going to generate a mangled name that is sufficiently
complex to trigger a seg-fault this way, so the only real purpose of
the patch is to resolve these PRs and stop more from being filed.

Cheers
  Nick


Reply | Threaded
Open this post in threaded view
|

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v3]

Sourceware - binutils list mailing list
In reply to this post by Nick Clifton
On Mon, Dec 3, 2018 at 6:45 AM, Nick Clifton <[hidden email]> wrote:

> Hi Richard,
>
>>>   * The description of the DMGL_RECURSE_LIMIT option in demangle.h has
>>>     been enhanced to add a note that if the option is not used, then
>>>     bug reports about stack overflows in the demangler will be rejected.
>>
>> Shouldn't we make it fool-proof by instead introducing a DMGL_NO_RECURSION_LIMIT
>> flag and when not set default to limiting recursion?
>
> Well I wanted the patch to be backwards compatible. Just on the
> general principle of not surprising users/programmers by changing
> things without telling them first.
>
> I could change this of course, but I would rather have Ian's blessing
> first.

You don't need my blessing--I wrote that code ages ago--but I agree
with Richard that in practice it's OK to limit recursion depth by
default.  Real symbols have very limited recursion requirements.

Ian
Reply | Threaded
Open this post in threaded view
|

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

Joseph Myers
In reply to this post by Cary Coutant-3
On Sat, 1 Dec 2018, Cary Coutant wrote:

> In order to handle arbitrary user input without crashing, perhaps the
> demangler should switch from recursive descent parsing to a state
> machine, where exhaustion of resources can be handled gracefully.

I've wondered if a GCC C/C++ extension could be defined that means
"convert this set of mutually recursive functions into a single function
with a state machine that allocates the equivalent of the stack manually".  
But such an extension would certainly be nontrivial to define.  (One use
for such an extension would be to avoid the GCC bugs that occasionally get
reported of the form "expressions with a million nested pairs of
parentheses make the compiler segfault", by using it to avoid recursion in
the parsers.)

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v4]

Nick Clifton
In reply to this post by Sourceware - binutils list mailing list
Hi Ian,

>>> Shouldn't we make it fool-proof by instead introducing a DMGL_NO_RECURSION_LIMIT

> You don't need my blessing--I wrote that code ages ago--but I agree
> with Richard that in practice it's OK to limit recursion depth by
> default.  Real symbols have very limited recursion requirements.

OK then, here is a fourth revision of the patch.

In this version:

  * The demangler option has been renamed to DMGHL_NO_RECURSE_LIMIT
    and if the option is not present then the limit is enforced.

  * I also found another PR that is fixed by the patch, although I had
    to make sure that the affected code could handle NULL pointers
    properly afterwards.

  OK to apply ?

Cheers
  Nick


include/ChangeLog
2018-11-29  Nick Clifton  <[hidden email]>

        * demangle.h (DMGL_NO_RECURSE_LIMIT): Define.
        (DEMANGLE_RECURSION_LIMIT): Define

libiberty/ChangeLog
2018-11-29  Nick Clifton  <[hidden email]>

        PR 87681
        PR 87675
        PR 87636
        PR 87350
        PR 87335
        * cp-demangle.h (struct d_info): Add recursion_level field.
        * cp-demangle.c (d_function_type): Add recursion counter.
        If the recursion limit is reached and the check is not disabled,
        then return with a failure result.
        (cplus_demangle_init_info): Initialise the recursion_level field.
        (d_demangle_callback): If the recursion limit is enabled, check
        for a mangled string that is so long that there is not enough
        stack space for the local arrays.
        * cplus-dem.c (struct work): Add recursion_level field.
        (squangle_mop_up): Set the numb and numk fields to zero.
        (work_stuff_copy_to_from): Handle the case where a btypevec or
        ktypevec field is NULL.
        (demangle_nested_args): Add recursion counter.  If
        the recursion limit is not disabled and reached, return with a
        failure result.

libiberty-demangler-recursion-limit.4.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v4]

Pedro Alves-7
On 12/04/2018 01:59 PM, Nick Clifton wrote:

> OK then, here is a fourth revision of the patch.

The issue pointed out by

 https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02592.html

is still present in this version.

Also, noticed a typo here:

> +/* If DMGL_NO_RECURE_LIMIT is not enabled, then this is the value used as

Typo: "RECURE"

> +   the maximum depth of recursion allowed.  It should be enough for any
> +   real-world mangled name.  */
> +#define DEMANGLE_RECURSION_LIMIT 1024
> +  

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

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v5]

Nick Clifton
Hi Pedro,

> The issue pointed out by
>
>  https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02592.html
>
> is still present in this version.

Doh!  Yes I meant to fix that one too, but forgot.

> Also, noticed a typo here:
>
>> +/* If DMGL_NO_RECURE_LIMIT is not enabled, then this is the value used as
>
> Typo: "RECURE"

Oops - thanks.

OK, revised (v5) patch attached.  Is this version acceptable to all ?

Cheers
  Nick


libiberty-demangler-recursion-limit.5.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v5]

Pedro Alves-7
On 12/04/2018 04:56 PM, Nick Clifton wrote:

> Hi Pedro,
>
>> The issue pointed out by
>>
>>  https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02592.html
>>
>> is still present in this version.
>
> Doh!  Yes I meant to fix that one too, but forgot.
>
>> Also, noticed a typo here:
>>
>>> +/* If DMGL_NO_RECURE_LIMIT is not enabled, then this is the value used as
>>
>> Typo: "RECURE"
>
> Oops - thanks.
>
> OK, revised (v5) patch attached.  Is this version acceptable to all ?
>
This is fine with me.

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

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v5]

Nick Clifton
Hi Ian,

  Is the patch OK with you ?

Cheers
  Nick
Reply | Threaded
Open this post in threaded view
|

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v5]

Jason Merrill
In reply to this post by Nick Clifton
On 12/4/18 11:56 AM, Nick Clifton wrote:
> OK, revised (v5) patch attached.  Is this version acceptable to all ?

Looks good to me.  Independently, do you see a reason not to disable the
old demangler entirely?

Jason
Reply | Threaded
Open this post in threaded view
|

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v5]

Sourceware - binutils list mailing list
In reply to this post by Nick Clifton
On Thu, Dec 6, 2018 at 3:12 AM Nick Clifton <[hidden email]> wrote:
>
>   Is the patch OK with you ?

Yes, thanks.

Ian
Reply | Threaded
Open this post in threaded view
|

RFC: libiberty PATCH to disable demangling of ancient mangling schemes

Jason Merrill
In reply to this post by Jason Merrill
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.

old-dem-off.diff (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Nick Clifton
Hi Jason,

>> 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.

I am not really familiar with this old scheme, so please excuse my ignorance
in asking these questions:

  * How likely is it that there are old toolchain in use out there that still
    use the v2 mangling ?  Ie I guess that I am asking "which generation(s)
    of gcc used v2 mangling ?"

  * If a user does try to demangle a v2 mangled name, what will happen ?
    Ie I guess I am asking if they will be given a hint that they need to use
    an older toolchain in order to demangle the names.

Cheers
  Nick



Reply | Threaded
Open this post in threaded view
|

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

Jakub Jelinek
On Fri, Dec 07, 2018 at 10:27:17AM +0000, Nick Clifton 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.
>
> I am not really familiar with this old scheme, so please excuse my ignorance
> in asking these questions:
>
>   * How likely is it that there are old toolchain in use out there that still
>     use the v2 mangling ?  Ie I guess that I am asking "which generation(s)
>     of gcc used v2 mangling ?"

GCC 3.0 and up used the new (Itanium C++ ABI) mangling, 2.95 and older used the old
mangling (2.96-RH used the new mangling I believe).
So you need compiler older than 17.5 years to have the old mangling.
Such a compiler didn't support most of the contemporarily used platforms
though at all (e.g. x86-64, powerpc64le, aarch64, I believe not even
powerpc64-linux).

        Jakub
Reply | Threaded
Open this post in threaded view
|

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

Richard Biener
In reply to this post by Jason Merrill
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?

Richard.
Reply | Threaded
Open this post in threaded view
|

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

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

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v5]

H.J. Lu-30
In reply to this post by Sourceware - binutils list mailing list
On Thu, Dec 6, 2018 at 10:04 AM Ian Lance Taylor via gcc-patches
<[hidden email]> 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


--
H.J.
Reply | Threaded
Open this post in threaded view
|

[PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

H.J. Lu-30
On Fri, Dec 7, 2018 at 8:17 AM H.J. Lu <[hidden email]> wrote:

>
> On Thu, Dec 6, 2018 at 10:04 AM Ian Lance Taylor via gcc-patches
> <[hidden email]> 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?

Thanks.

--
H.J.

0001-Set-DEMANGLE_RECURSION_LIMIT-to-1536.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Nick Clifton
In reply to this post by Jakub Jelinek
Hi Guys,

>>>> Looks good to me.  Independently, do you see a reason not to disable the
>>>> old demangler entirely?

>>   * How likely is it that there are old toolchain in use out there that still
>>     use the v2 mangling ?

> GCC 3.0 and up used the new (Itanium C++ ABI) mangling, 2.95 and older used the old
> mangling (2.96-RH used the new mangling I believe).
> So you need compiler older than 17.5 years to have the old mangling.
> Such a compiler didn't support most of the contemporarily used platforms
> though at all (e.g. x86-64, powerpc64le, aarch64, I believe not even
> powerpc64-linux).

Well that is good enough for me. :-)  I do not have the power to approve
the patch, but I would certainly be happy to see it go in.

Cheers
  Nick


Reply | Threaded
Open this post in threaded view
|

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

Jason Merrill
In reply to this post by Jakub Jelinek
On 12/7/18 12:48 PM, Tom Tromey wrote:

>>>>>> "Pedro" == Pedro Alves <[hidden email]> writes:
>
> Pedro> I would say that it's very, very unlikely, and not worth it of the
> Pedro> maintenance burden.
>
> Agreed, and especially true for the more unusual demanglings like Lucid
> or EDG.
>
> On the gdb side perhaps we can get rid of "demangle-style" now.  It
> probably hasn't worked properly in years, and after this it would be
> guaranteed not to.
So, here's the patch to tear out the old code, which passes the GCC
regression testsuite.  I also tried building binutils/gdb with it, and
both will need to remove code that calls cplus_mangle_opname for dealing
with the old mangling scheme.

Jason

old-dem-remove.diff (199K) Download Attachment
123