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

[Richard Biener <richard.guenther@gmail.com>]

On Fri, Nov 30, 2018 at 6:41 PM Nick Clifton <nickc@redhat.com> 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  <nickc@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  <nickc@redhat.com>
>
>         * demangle.h (DMGL_RECURSE_LIMIT): Define.
>         (DEMANGLE_RECURSION_LIMIT): Prototype.
>
> binutils/ChangeLog
> 2018-11-29  Nick Clifton  <nickc@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.