Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v3]
On Mon, Dec 3, 2018 at 6:45 AM, Nick Clifton 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
Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v3]
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
Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v3]
On Fri, Nov 30, 2018 at 6:41 PM Nick Clifton wrote: > > Hi Guys, > > >> 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. Shouldn't we make it fool-proof by instead introducing a DMGL_NO_RECURSION_LIMIT flag and when not set default to limiting recursion? > * 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 > > 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 > > * demangle.h (DMGL_RECURSE_LIMIT): Define. > (DEMANGLE_RECURSION_LIMIT): Prototype. > > binutils/ChangeLog > 2018-11-29 Nick Clifton > > * 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.
Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v3]
On 11/30/2018 05:41 PM, Nick Clifton wrote:
> @@ -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
Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v3]
Hi!
Just spelling nitpicking.
On Fri, Nov 30, 2018 at 05:41:35PM +, Nick Clifton wrote:
> + order to dmangle truely complicated names, but it also leaves the tools
truly? Multiple times.
> +The @option{-r} option is a snyonym for the
synonym? Multiple times.
Jakub
Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v3]
Hi Guys, >> 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 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 * demangle.h (DMGL_RECURSE_LIMIT): Define. (DEMANGLE_RECURSION_LIMIT): Prototype. binutils/ChangeLog 2018-11-29 Nick Clifton * 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. diff --git a/binutils/NEWS b/binutils/NEWS index a3ee86ef7f..5ed61c8513 100644 --- a/binutils/NEWS +++ b/binutils/NEWS @@ -1,5 +1,16 @@ -*- text -*- +* The addr2line, c++filt, nm and objdump tools now have a limit on the + maximum amount of recursion that is allowed whilst demangling strings. + The value for this limit is defined by the DEMANGLE_RECRUSE_LIMIT + constant declared in the include/demangle.h header file. At the time + of writing this constant has the value of 1024. + + The --no-recurse-limit option can be used to remove the limit, restoring + the behaviour of earlier versions of these tools. This may be needed in + order to dmangle truely complicated names, but it also leaves the tools + vulnerable to stack exhaustion from maliciously constructed mangled names. + * Objdump's --disassemble option can now take a parameter, specifying the starting symbol for disassembly. Disassembly will continue from this symbol up to the next symbol. diff --git a/binutils/addr2line.c b/binutils/addr2line.c index 008e62026e..3085806a4a 100644 --- a/binutils/addr2line.c +++ b/binutils/addr2line.c @@ -45,6 +45,9 @@ static bfd_boolean
