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

[Nick Clifton]

Hi Ian,

Libiberty's demangler seems to be a favourite target of people looking
to file new CVEs by fuzzing strings and I have finally gotten tired of
reviewing them all. So I would like to propose a patch to add support
for placing a recursion limit on the demangling functions.

The patch adds a new demangling option - DMGL_RECURSE_LIMIT - which
needs to be present in order for the new code to take affect. So
current users of the libiberty library will not be affected[*].
The patch also adds a new function - cplus_demangle_set_recursion_limit()
- which can be used to probe and/or set the limit.

When the option is in effect a few of the demangler functions will use
static counters to make sure that they have not been called
recursively too many times. I only chose those functions for which I
could find filed PRs that triggered this kind of problem. But with
the stack limiting framework in place it would be easy to add checks
to other functions, should they prove necessary.

I also encountered one binary that could trigger stack exhaustion in
d_demangle_callback where it tries to allocate stack space to hold
component and substitution arrays. So the patch includes a check
for this situation as well.

There does not appear to be any error reporting framework for the
demangler functions, so when a recursion limit is reached the
functions just return a failure/NULL result.

I have tested the patch with a variety of different binutils builds
and also bootstrapped an x86_64-pc-linux-gnu toolchain. No new
failures were found.

What do you think, is this approach reasonable ?

Cheers
Nick

[*] Actually I also have a patch for the binutils to modify the
addr2line, c++filt, nm and objdump programs to make use of this new
feature, should it be accepted into libiberty.

Patches:

include/ChangeLog
2018-11-29 Nick Clifton <***@redhat.com>

* demangle.h (DMGL_RECURSE_LIMIT): Define.
(cplus_demangle_set_recursion_limit): Prototype.

libiberty/ChangeLog
2018-11-29 Nick Clifton <***@redhat.com>

PR 87675
PR 87636
* cp-demangle.c (demangle_recursion_limit): New static
variable.
(d_function_type): Add recursion counter. If the recursion
limit is enabled and reached, return with a failure result.
(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_demangle_set_recursion): New function. Sets and/or
returns the current stack recursion limit.
* cplus-dem.c (demangle_nested_args): Add recursion counter. If
the recursion limit is enabled and reached, return with a
failure result.

[Scott Gayou]

Thank you for looking into this Nick. I've been staring at a few of these
CVEs off-and-on for a few days, and the following CVEs all look like
duplicates:

CVE-2018-17985: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87335
CVE-2018-18484: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87636
CVE-2018-18701: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87675
CVE-2018-18700: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87681

There may be more. I think Mitre is scanning the gnu bugzilla and assigning
CVEs? This does look like a legitimate very low criticality "denial of
service", but generating new CVEs for every unique poc file against the
same root cause doesn't seem useful. Perhaps some of these should be
rejected?

[Nick Clifton]

Hi Scott,

Post by Scott Gayou
CVE-2018-17985: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87335
CVE-2018-18484: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87636
CVE-2018-18701: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87675
CVE-2018-18700: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87681

Yes, essentially they are. They actually trigger stack exhaustion at
different places inside libiberty, but the root cause is the same.
I am also happy to say that my proposed patch fixes *all* of these PRs.

Post by Scott Gayou
Perhaps some of these should be rejected?

That would nice, but I think that if the patch is accepted then the
issue should be resolved and we should stop seeing this kind of CVE.

(I must admit that my motivation for creating this patch in the first
place is that I am fed up with the amount of hassle that is involved
each time a new CVE is created. Especially when they are essentially
all the same bug).

Cheers
Nick

[Pedro Alves]

Hi Nick,

static struct demangle_component *
d_function_type (struct d_info *di)
{
- struct demangle_component *ret;
+ static unsigned long recursion_level = 0;

Did you consider making this be a part of struct d_info instead?
IIRC, d_info is like a "this" pointer, passed around pretty
much everywhere.

I think going in the direction of making the demangler harder to use
in an efficient thread-safe manner is undesirable, even if the feature
is optional. E.g., in GDB, loading big binaries, demangling is very high
in profiles, and so we've kicked around the desire to parallelize
it (e.g., by parallelizing the reading/interning of DSO files, instead of
reading all of them sequentially). Having to synchronize access to the
demangler would be quite unfortunate. If possible, it'd be great
to avoid making work toward that direction harder. (Keeping in mind that
if this recursion detection feature is useful for binutils, then it should
also be useful for GDB.)

Thanks,
Pedro Alves

[Tom Tromey]

Pedro> E.g., in GDB, loading big binaries, demangling is very high
Pedro> in profiles, and so we've kicked around the desire to parallelize
Pedro> it

More than kicked around -- I have a patch series that does this. It
isn't ready to submit, it requires a couple of other series that are
pending; but it is pretty far along. I would definitely appreciate not
introducing more thread-unsafety.

thanks,
Tom

[Nick Clifton]

Hi Pedro, Hi Tom,

Post by Tom Tromey
Pedro> E.g., in GDB, loading big binaries, demangling is very high
Pedro> in profiles, and so we've kicked around the desire to parallelize
Pedro> it

I did consider this, but I encountered two problems:

1. I wanted users of the demangling functions to be able to change
the recursion limit. So for example in environments with a very
limited amount of stack space the limit could be reduced. This
is one of the purposes of the cplus_demangle_set_recursion_limit()
function.

Since a new d_info structure is created every time a string is demangled
the only way to implement cplus_demangle_set_recursion_limit would
be to have it set the value that is used to initialize the field in
the structure, which is basically back to where we are now.

2. I wanted to be able to access the recursion limit from code
in cplus-dem.c, which does not use the d_info structure. This was
the other purpose of the cplus_demangler_set_recursion_limit()
function - it allows the limit to be read without changing it.

As an alternative I did consider adding an extra parameter to the
cplus_demangle(), cplus_demangle_opname() and related functions. But
this would make them non-backwards compatible and I did not want to
do that.

I would imagine that changing the recursion limit would be a very
rare event, possibly only ever done once in an executable's runtime,
so I doubt that there will ever be any real-life thread safety
problems associated with it. But I do understand that this is just
an assumption, not a guarantee.

Cheers
Nick

[Pedro Alves]

Post by Nick Clifton
Hi Pedro, Hi Tom,

Post by Tom Tromey
Pedro> E.g., in GDB, loading big binaries, demangling is very high
Pedro> in profiles, and so we've kicked around the desire to parallelize
Pedro> it

1. I wanted users of the demangling functions to be able to change
the recursion limit. So for example in environments with a very
limited amount of stack space the limit could be reduced. This
is one of the purposes of the cplus_demangle_set_recursion_limit()
function.
Since a new d_info structure is created every time a string is demangled
the only way to implement cplus_demangle_set_recursion_limit would
be to have it set the value that is used to initialize the field in
the structure, which is basically back to where we are now.

That's a lesser evil. With the proposed cplus_demangle_set_recursion_limit
interface, you essentially end up with an interface that makes all threads in
the process end up with the same limit. There's precedent for global options
in the current_demangling_style global, set by the cplus_demangle_set_style
function. That's one I recalled, there may be others. I'd prefer interfaces
that pass down the options as arguments, or a pointer to an options struct, but
that's not the main issue.

The main issue is the current recursion level counter.
That's the static variable that I had pointed at:

d_function_type (struct d_info *di)
{
[...]
+ static unsigned long recursion_level = 0;
[...]
+ recursion_level ++;
[...]
+ recursion_level --;
return ret;
}

Even if we go with a recursion limit per process, this recursion
level count must be moved to d_info for thread-safety without
synchronization.

Post by Nick Clifton
2. I wanted to be able to access the recursion limit from code
in cplus-dem.c, which does not use the d_info structure.

That should just mean a bit of plumbing is in order to pass down
either a d_info structure or something like it?

Post by Nick Clifton
This was
the other purpose of the cplus_demangler_set_recursion_limit()
function - it allows the limit to be read without changing it.
As an alternative I did consider adding an extra parameter to the
cplus_demangle(), cplus_demangle_opname() and related functions. But
this would make them non-backwards compatible and I did not want to
do that.

I'd say that ideally, we'd aim at the interface we want, and then go
from there. If changing interfaces is a problem, we can always add
a new entry point and keep the old as backward compatibility.
But is it (a problem)? Do we require ABI compatibility here?

But again, the main issue I'm seeing is the recursion level counter,
not the global limit.

Post by Nick Clifton
I would imagine that changing the recursion limit would be a very
rare event, possibly only ever done once in an executable's runtime,
so I doubt that there will ever be any real-life thread safety
problems associated with it. But I do understand that this is just
an assumption, not a guarantee.

See above.

Thanks,
Pedro Alves

[Ian Lance Taylor]

Post by Pedro Alves
Hi Nick,

static struct demangle_component *
d_function_type (struct d_info *di)
{
- struct demangle_component *ret;
+ static unsigned long recursion_level = 0;

Did you consider making this be a part of struct d_info instead?
IIRC, d_info is like a "this" pointer, passed around pretty
much everywhere.
I think going in the direction of making the demangler harder to use
in an efficient thread-safe manner is undesirable, even if the feature
is optional. E.g., in GDB, loading big binaries, demangling is very high
in profiles, and so we've kicked around the desire to parallelize
it (e.g., by parallelizing the reading/interning of DSO files, instead of
reading all of them sequentially). Having to synchronize access to the
demangler would be quite unfortunate. If possible, it'd be great
to avoid making work toward that direction harder. (Keeping in mind that
if this recursion detection feature is useful for binutils, then it should
also be useful for GDB.)

I agree. Using static variables here seems problematic. Right now as
far as I know the demangler has no static variables at all.

Ian

[Nick Clifton]

Hi Ian,

*sigh* it is always the way. You post a patch and five minutes later
you find a bug in it. In this case it turned out that I had forgotten
that gcc has its own copy of the libiberty sources, so the bootstrap
test that I had run did not use the patched sources. Doh. When I did
rerun the bootstrap with the patched sources it failed because I had
forgotten to use the CP_STATIC_IF_GLIBCPP_V3 macro when declaring the
new cplus_demangle_set_recursion_limit() function.

I am attaching a revised patch with this bug fixed, and an updated
changelog entry as I have found a few more CVE PRs that it fixes.

Also - Tom and Pedro have raised the issue that the patch introduces
a new static variable to the library that is not thread safe. I am
not sure of the best way to address this problem. Possibly the
variable could be made thread local ? Are there any other static
variables in libiberty that face the same issue ?

Cheers
Nick

libiberty/ChangeLog
2018-11-29 Nick Clifton <***@redhat.com>

PR 87681
PR 87675
PR 87636
PR 87335
* cp-demangle.c (demangle_recursion_limit): New static
variable.
(d_function_type): Add recursion counter. If the recursion
limit is enabled and reached, return with a failure result.
(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_demangle_set_recursion): New function. Sets and/or
returns the current stack recursion limit.
* cplus-dem.c (demangle_nested_args): Add recursion counter. If
the recursion limit is enabled and reached, return with a
failure result.

[Jakub Jelinek]

Post by Nick Clifton
Also - Tom and Pedro have raised the issue that the patch introduces
a new static variable to the library that is not thread safe. I am
not sure of the best way to address this problem. Possibly the
variable could be made thread local ? Are there any other static

Please don't. That has a cost for all the programs that link against
libstdc++ or any other library that includes the demangler, even when they
don't use the demangler at all (99.9% of the users).
Most of the demangler functions pass around a pointer to a struct with
context, can't this be added in there?

Jakub

[Nick Clifton]

Hi Jakub,

Post by Jakub Jelinek

Post by Nick Clifton
Also - Tom and Pedro have raised the issue that the patch introduces
a new static variable to the library that is not thread safe. I am
not sure of the best way to address this problem. Possibly the
variable could be made thread local ? Are there any other static

Please don't.

OK. :-)

Post by Jakub Jelinek
Most of the demangler functions pass around a pointer to a struct with
context, can't this be added in there?

Not without modifying the current demangling interface. The problem is
that the context structure is created for each invocation of a demangling
function (from outside the library), and no state is preserved across
demangling calls. Thus in order to have a recursion limit which is
configurable by the caller, you either need to have a global variable or
else extend the demangling interface to include a recursion limit parameter.

I did consider just having a fixed limit, that the user cannot change, but
I thought that this might be rejected by reviewers. (On the grounds that
different limits are appropriate to different execution environments).
Note - enabling or disabling the recursion limit is controlled by a separate
feature of the proposed patch, ie the new DMGL_RECURSE_LIMIT flag in the
options field of the cplus_demangleXXX() functions. But there is not enough
room in the options field to also include a recursion limit value.

Cheers
Nick

[Michael Matz]

Hi,

Post by Nick Clifton
Not without modifying the current demangling interface. The problem is
that the context structure is created for each invocation of a
demangling function (from outside the library), and no state is
preserved across demangling calls. Thus in order to have a recursion
limit which is configurable by the caller, you either need to have a
global variable or else extend the demangling interface to include a
recursion limit parameter.
I did consider just having a fixed limit, that the user cannot change,
but I thought that this might be rejected by reviewers. (On the grounds
that different limits are appropriate to different execution
environments). Note - enabling or disabling the recursion limit is
controlled by a separate feature of the proposed patch, ie the new
DMGL_RECURSE_LIMIT flag in the options field of the cplus_demangleXXX()
functions. But there is not enough room in the options field to also
include a recursion limit value.

Or we decide to not ignore this part of the GNU coding standard ...

Post by Nick Clifton
4.2 Writing Robust Programs
Avoid arbitrary limits on the length or number of any data structure,
including file names, lines, files, and symbols, by allocating all data
structures dynamically. In most Unix utilities, “long lines are silently
truncated”. This is not acceptable in a GNU utility.

... just because script kiddies do mindless fuzzing work. I realize that
you didn't implement a fixed limit, but IMHO it's bordering with that.

But re the static variables: you have two, once the recursion depth, and
once the limit.

The limit can be a static variable just fine, you state, as part of the
interface that it sets global state, and if that needs to happen from
multiple threads that the user must synchronize. This is fine because
there won't be many calls to constantly change that limit.

The current depth needs to be part of the d_info (resp. workstuff)
structure, avoiding the thread-unsafety.

Another crazy idea: do encode the limit in the option field. If you
declare that the limit is a power of two (which IMHO doesn't
materially change the usefullness of such limit) then you only need to
encode the log2 of it, for which 5 bits are plenty enough (you can encode
limits from 1 to 1<<32 then).


Ciao,
Michael.

[Ian Lance Taylor]

Post by Michael Matz

Post by Nick Clifton
Not without modifying the current demangling interface. The problem is
that the context structure is created for each invocation of a
demangling function (from outside the library), and no state is
preserved across demangling calls. Thus in order to have a recursion
limit which is configurable by the caller, you either need to have a
global variable or else extend the demangling interface to include a
recursion limit parameter.
I did consider just having a fixed limit, that the user cannot change,
but I thought that this might be rejected by reviewers. (On the grounds
that different limits are appropriate to different execution
environments). Note - enabling or disabling the recursion limit is
controlled by a separate feature of the proposed patch, ie the new
DMGL_RECURSE_LIMIT flag in the options field of the cplus_demangleXXX()
functions. But there is not enough room in the options field to also
include a recursion limit value.

Or we decide to not ignore this part of the GNU coding standard ...

Post by Nick Clifton
4.2 Writing Robust Programs
Avoid arbitrary limits on the length or number of any data structure,
including file names, lines, files, and symbols, by allocating all data
structures dynamically. In most Unix utilities, “long lines are silently
truncated”. This is not acceptable in a GNU utility.

... just because script kiddies do mindless fuzzing work. I realize that
you didn't implement a fixed limit, but IMHO it's bordering with that.

That section is "Writing Robust Programs." Robustness guarantees have
to be different for utilities and servers. A robust server doesn't
crash because of arbitrary user input, but there are servers that
demangle names that are provided by the user. So we need two modes for
the demangler: one that takes anything and sometimes crashes, for
utilities like c++filt, and one that doesn't crash, for servers. And it
seems like that is what Nick is suggesting.

Ian

[Cary Coutant]

Post by Ian Lance Taylor
That section is "Writing Robust Programs." Robustness guarantees have
to be different for utilities and servers. A robust server doesn't
crash because of arbitrary user input, but there are servers that
demangle names that are provided by the user. So we need two modes for
the demangler: one that takes anything and sometimes crashes, for
utilities like c++filt, and one that doesn't crash, for servers. And it
seems like that is what Nick is suggesting.

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.

-cary

[Nick Clifton]

Hi Cary,

Post by Cary Coutant
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

[Joseph Myers]

Post by Cary Coutant
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.)

[Ian Lance Taylor]

Post by Nick Clifton
I did consider just having a fixed limit, that the user cannot change, but
I thought that this might be rejected by reviewers. (On the grounds that
different limits are appropriate to different execution environments).
Note - enabling or disabling the recursion limit is controlled by a separate
feature of the proposed patch, ie the new DMGL_RECURSE_LIMIT flag in the
options field of the cplus_demangleXXX() functions. But there is not enough
room in the options field to also include a recursion limit value.

I think it would be fine to have a large fixed limit plus a flag to
disable the limit. I can't think of any reason why a program would want
to change the limit unless it has complete trust in the symbols it is
demangling, and in that case it may as well simply disable the limit.

Ian

[Jakub Jelinek]

Post by Ian Lance Taylor

Post by Nick Clifton
I did consider just having a fixed limit, that the user cannot change, but
I thought that this might be rejected by reviewers. (On the grounds that
different limits are appropriate to different execution environments).
Note - enabling or disabling the recursion limit is controlled by a separate
feature of the proposed patch, ie the new DMGL_RECURSE_LIMIT flag in the
options field of the cplus_demangleXXX() functions. But there is not enough
room in the options field to also include a recursion limit value.

I think it would be fine to have a large fixed limit plus a flag to
disable the limit. I can't think of any reason why a program would want
to change the limit unless it has complete trust in the symbols it is
demangling, and in that case it may as well simply disable the limit.

Well, disabling the limit is what the people fuzzing it will use then
and report it still crashes.
We'd need to document that if somebody asks for no limit, then we don't
consider any cases of running as out of stack etc. as bugs, and simply
people shouldn't set that on when running on untrusted symbols.

Jakub

[Nick Clifton]

Hi Guys,

Post by Ian Lance Taylor
I think it would be fine to have a large fixed limit plus a flag to
disable the limit.

Great - in which case please may I present version 3 of the patch. In
this version:

* The cplus_demangle_set_recursion_limit() function has been removed
and instead a new constant - DEMANGLE_RECURSION_LIMIT - is defined in
demangle.h.

* The recursion counters in cp-demangle.c have been merged into one
counter, now contained in the d_info structure.

* In cplus-dem.c the recursion counter has been moved into the work
structure.

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

* The binutils patch has been updated to reflect these changes. The
addr2line, cxxfilt, nm and objdump programs now have two new options
--recurse-limit and --no-recurse-limit, with --recurse-limit being
the default. The documentation is updated to describe these options
and to also add a note about bug reports being rejected if
--no-recurse-limit is used.

What do you think, is this version acceptable ?

Cheers
Nick

libiberty/ChangeLog
2018-11-29 Nick Clifton <***@redhat.com>

PR 87681
PR 87675
PR 87636
PR 87335
* cp-demangle.h (struct d_info): Add recursion_limit field.
* cp-demangle.c (d_function_type): If the recursion limit is
enabled and reached, return with a failure result.
(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.
(demangle_nested_args): If the recursion limit is enabled and
reached, return with a failure result.

include/ChangeLog
2018-11-29 Nick Clifton <***@redhat.com>

* demangle.h (DMGL_RECURSE_LIMIT): Define.
(DEMANGLE_RECURSION_LIMIT): Prototype.

binutils/ChangeLog
2018-11-29 Nick Clifton <***@redhat.com>

* addr2line.c (demangle_flags): New static variable.
(long_options): Add --recurse-limit and --no-recurse-limit.
(translate_address): Pass demangle_flags to bfd_demangle.
(main): Handle --recurse-limit and --no-recurse-limit options.
* cxxfilt.c (flags): Add DMGL_RECURSE_LIMIT.
(long_options): Add --recurse-limit and --no-recurse-limit.
(main): Handle new options.
* dlltool.c (gen_def_file): Include DMGL_RECURSE_LIMIT in flags
passed to cplus_demangle.
* nm.c (demangle_flags): New static variable.
(long_options): Add --recurse-limit and --no-recurse-limit.
(main): Handle new options.
* objdump.c (demangle_flags): New static variable.
(usage): Add --recurse-limit and --no-recurse-limit.
(long_options): Likewise.
(objdump_print_symname): Pass demangle_flags to bfd_demangle.
(disassemble_section): Likewise.
(dump_dymbols): Likewise.
(main): Handle new options.
* prdbg.c (demangle_flags): New static variable.
(tg_variable): Pass demangle_flags to demangler.
(tg_start_function): Likewise.
* stabs.c (demangle_flags): New static variable.
(stab_demangle_template): Pass demangle_flags to demangler.
(stab_demangle_v3_argtypes): Likewise.
(stab_demangle_v3_arg): Likewise.
* doc/binutuls.texi: Document new command line options.
* NEWS: Mention the new feature.
* testsuite/config/default.exp (CXXFILT): Define if not already
defined.
(CXXFILTFLAGS): Likewise.
* testsuite/binutils-all/cxxfilt.exp: New file. Runs a few
simple tests of the cxxfilt program.

[Jakub Jelinek]

Hi!

Just spelling nitpicking.

+ order to dmangle truely complicated names, but it also leaves the tools

truly? Multiple times.
synonym? Multiple times.

Jakub

[Pedro Alves]

@@ -4693,10 +4694,21 @@
demangle_nested_args (struct work_stuff *work, const char **mangled,
string *declp)
{
+ static unsigned long recursion_level = 0;
string* saved_previous_argument;
int result;
int saved_nrepeats;
+ if ((work->options & DMGL_RECURSE_LIMIT)
+ && work->recursion_level > DEMANGLE_RECURSION_LIMIT)
+ {
+ /* FIXME: There ought to be a way to report that the recursion limit
+ has been reached. */
+ return 0;
+ }
+
+ recursion_level ++;
+

There's still a static recursion level counter here?

Thanks,
Pedro Alves

[Richard Biener]

Post by Nick Clifton
Hi Guys,

Post by Ian Lance Taylor
I think it would be fine to have a large fixed limit plus a flag to
disable the limit.

Great - in which case please may I present version 3 of the patch. In
* The cplus_demangle_set_recursion_limit() function has been removed
and instead a new constant - DEMANGLE_RECURSION_LIMIT - is defined in
demangle.h.
* The recursion counters in cp-demangle.c have been merged into one
counter, now contained in the d_info structure.
* In cplus-dem.c the recursion counter has been moved into the work
structure.
* 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?

Post by Nick Clifton
* The binutils patch has been updated to reflect these changes. The
addr2line, cxxfilt, nm and objdump programs now have two new options
--recurse-limit and --no-recurse-limit, with --recurse-limit being
the default. The documentation is updated to describe these options
and to also add a note about bug reports being rejected if
--no-recurse-limit is used.
What do you think, is this version acceptable ?
Cheers
Nick
libiberty/ChangeLog
PR 87681
PR 87675
PR 87636
PR 87335
* cp-demangle.h (struct d_info): Add recursion_limit field.
* cp-demangle.c (d_function_type): If the recursion limit is
enabled and reached, return with a failure result.
(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.
(demangle_nested_args): If the recursion limit is enabled and
reached, return with a failure result.
include/ChangeLog
* demangle.h (DMGL_RECURSE_LIMIT): Define.
(DEMANGLE_RECURSION_LIMIT): Prototype.
binutils/ChangeLog
* addr2line.c (demangle_flags): New static variable.
(long_options): Add --recurse-limit and --no-recurse-limit.
(translate_address): Pass demangle_flags to bfd_demangle.
(main): Handle --recurse-limit and --no-recurse-limit options.
* cxxfilt.c (flags): Add DMGL_RECURSE_LIMIT.
(long_options): Add --recurse-limit and --no-recurse-limit.
(main): Handle new options.
* dlltool.c (gen_def_file): Include DMGL_RECURSE_LIMIT in flags
passed to cplus_demangle.
* nm.c (demangle_flags): New static variable.
(long_options): Add --recurse-limit and --no-recurse-limit.
(main): Handle new options.
* objdump.c (demangle_flags): New static variable.
(usage): Add --recurse-limit and --no-recurse-limit.
(long_options): Likewise.
(objdump_print_symname): Pass demangle_flags to bfd_demangle.
(disassemble_section): Likewise.
(dump_dymbols): Likewise.
(main): Handle new options.
* prdbg.c (demangle_flags): New static variable.
(tg_variable): Pass demangle_flags to demangler.
(tg_start_function): Likewise.
* stabs.c (demangle_flags): New static variable.
(stab_demangle_template): Pass demangle_flags to demangler.
(stab_demangle_v3_argtypes): Likewise.
(stab_demangle_v3_arg): Likewise.
* doc/binutuls.texi: Document new command line options.
* NEWS: Mention the new feature.
* testsuite/config/default.exp (CXXFILT): Define if not already
defined.
(CXXFILTFLAGS): Likewise.
* testsuite/binutils-all/cxxfilt.exp: New file. Runs a few
simple tests of the cxxfilt program.

[Nick Clifton]

Hi Richard,

Post by Richard Biener

Post by Nick Clifton
* 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

[Ian Lance Taylor via binutils]

Post by Nick Clifton
Hi Richard,

Post by Richard Biener

Post by Nick Clifton
* 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

[Nick Clifton]

Hi Ian,

Post by Ian Lance Taylor via binutils

Post by Richard Biener
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 <***@redhat.com>

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

libiberty/ChangeLog
2018-11-29 Nick Clifton <***@redhat.com>

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.

[Pedro Alves]

Post by Nick Clifton
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.

Post by Nick Clifton
+/* If DMGL_NO_RECURE_LIMIT is not enabled, then this is the value used as

Typo: "RECURE"

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

Thanks,
Pedro Alves

[Nick Clifton]

Hi Pedro,

Post by Pedro Alves
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.

Post by Pedro Alves

Post by Nick Clifton
+/* 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

[Pedro Alves]

Post by Nick Clifton
Hi Pedro,

Post by Pedro Alves
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.

Post by Pedro Alves

Post by Nick Clifton
+/* 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

[Nick Clifton]

Hi Ian,

Is the patch OK with you ?

Cheers
Nick

[Ian Lance Taylor via binutils]

Post by Nick Clifton
Is the patch OK with you ?

Yes, thanks.

Ian

[H.J. Lu]

On Thu, Dec 6, 2018 at 10:04 AM Ian Lance Taylor via gcc-patches

Post by Nick Clifton
Is the patch OK with you ?

This caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88409

[H.J. Lu]

Post by H.J. Lu
On Thu, Dec 6, 2018 at 10:04 AM Ian Lance Taylor via gcc-patches

Post by Nick Clifton
Is the patch OK with you ?

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88409

Here is the fix. OK for trunk?

Thanks.

[Jason Merrill]

Post by Nick Clifton
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

[Jason Merrill]

Post by Jason Merrill
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.

[Nick Clifton]

Hi Jason,

Post by Jason Merrill

Post by Jason Merrill
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

[Jakub Jelinek]

Post by Nick Clifton

Post by Jason Merrill

Post by Jason Merrill
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
* 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

[Pedro Alves]

Adding gdb-patches, since demangling affects gdb.

Ref: https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00407.html

Post by Jakub Jelinek

Post by Nick Clifton

Post by Jason Merrill

Post by Jason Merrill
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
* 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).

Yeah.

I guess the question would be whether it is reasonable to expect
that people will still need to debug&inspect (with gdb, c++filt, etc.)
any such old binary, and that they will need to do it with with modern
tools, as opposed to sticking with older binutils&gdb, and how often
would that be needed.

I would say that it's very, very unlikely, and not worth it of the
maintenance burden.

Last I heard of 2.95-produced binaries I think was for some ancient gcc-2.95-based
cross compiler that was still being minimally maintained, because it was needed
to build&maintain some legacy stuff. That was maybe over 8 years ago, and
it was off trunk. It's probably dead by now. And if isn't dead,
whoever maintains the compiler off trunk certainly can also maintain old-ish
binutils & gdb off trunk.

Thanks,
Pedro Alves

[Tom Tromey]

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.

Tom

[Jason Merrill]

Post by Tom Tromey
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

[Nick Clifton]

Hi Guys,

Post by Jakub Jelinek

Post by Nick Clifton

Post by Jason Merrill
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

[Richard Biener]

Post by Jason Merrill

Post by Jason Merrill
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.

[Jason Merrill]

Post by Richard Biener

Post by Jason Merrill

Post by Jason Merrill
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