On Thu, Dec 6, 2018 at 10:04 AM Ian Lance Taylor via gcc-patches
wrote:
>
> On Thu, Dec 6, 2018 at 3:12 AM Nick Clifton wrote:
> >
> > Is the patch OK with you ?
>
This caused:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88409
--
H.J.
On Thu, Dec 6, 2018 at 3:12 AM Nick Clifton wrote:
>
> Is the patch OK with you ?
Yes, thanks.
Ian
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
Hi Ian,
Is the patch OK with you ?
Cheers
Nick
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
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
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
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
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
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.
>>
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
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
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
> 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
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
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
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 -
Michael Matz writes:
> On Fri, 30 Nov 2018, Nick Clifton wrote:
>
>> 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
On Fri, Nov 30, 2018 at 05:55:52AM -0800, Ian Lance Taylor wrote:
> Nick Clifton writes:
>
> > 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
Nick Clifton writes:
> 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
Hi,
On Fri, 30 Nov 2018, Nick Clifton wrote:
> 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
On 11/30/2018 08:26 AM, Nick Clifton wrote:
> Hi Pedro, Hi Tom,
>
>> 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
Hi Jakub,
>> 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
On Fri, Nov 30, 2018 at 08:38:48AM +, Nick Clifton wrote:
> 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
Hi Scott,
> 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:
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
Pedro Alves writes:
> Hi Nick,
>
> On 11/29/2018 03:01 PM, Nick Clifton wrote:
>> 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
Hi Nick,
On 11/29/2018 03:01 PM, Nick Clifton wrote:
> 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
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
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
30 matches
Mail list logo
