From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 73506 invoked by alias); 30 Nov 2018 08:38:55 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 73001 invoked by uid 89); 30 Nov 2018 08:38:54 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.1 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=UD:cplus-dem.c, cplusdemc, cplus-dem.c, ought X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 30 Nov 2018 08:38:52 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1B8B930041CE; Fri, 30 Nov 2018 08:38:51 +0000 (UTC) Received: from comet.redhat.com (ovpn-116-107.ams2.redhat.com [10.36.116.107]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 9D49D60F89; Fri, 30 Nov 2018 08:38:49 +0000 (UTC) From: Nick Clifton To: ian@airs.com Subject: Re: RFA/RFC: Add stack recursion limit to libiberty's demangler CC: gcc-patches@gcc.gnu.org, binutils@sourceware.org, matz@gcc.gnu.org, sgayou@redhat.com, jason@redhat.com Date: Fri, 30 Nov 2018 08:38:00 -0000 Message-ID: <87muprdko7.fsf@redhat.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-IsSubscribed: yes X-SW-Source: 2018-11/txt/msg02535.txt.bz2 --=-=-= Content-Type: text/plain Content-length: 1718 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 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. --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=libiberty-demangler-recursion-limit.2.patch Content-length: 4725 Index: libiberty/cp-demangle.c =================================================================== --- libiberty/cp-demangle.c (revision 266657) +++ libiberty/cp-demangle.c (working copy) @@ -62,6 +62,7 @@ cplus_demangle_fill_dtor cplus_demangle_print cplus_demangle_print_callback + cplus_demangle_set_recursion_limit and other functions defined in the file cp-demint.c. This file also defines some other functions and variables which are @@ -181,6 +182,9 @@ #define cplus_demangle_init_info d_init_info static void d_init_info (const char *, int, size_t, struct d_info *); +#define cplus_demangle_set_recursion_limit d_set_recursion_level +static unsigned int d_set_recursion_limit (unsigned int); + #else /* ! defined(IN_GLIBCPP_V3) */ #define CP_STATIC_IF_GLIBCPP_V3 #endif /* ! defined(IN_GLIBCPP_V3) */ @@ -2852,21 +2856,34 @@ static struct demangle_component * d_function_type (struct d_info *di) { - struct demangle_component *ret; + static unsigned long recursion_level = 0; + struct demangle_component *ret = NULL; - if (! d_check_char (di, 'F')) - return NULL; - if (d_peek_char (di) == 'Y') + if ((di->options & DMGL_RECURSE_LIMIT) + && recursion_level > demangle_recursion_limit) { - /* Function has C linkage. We don't print this information. - FIXME: We should print it in verbose mode. */ - d_advance (di, 1); + /* FIXME: There ought to be a way to report that the recursion limit + has been reached. */ + return NULL; } - ret = d_bare_function_type (di, 1); - ret = d_ref_qualifier (di, ret); - if (! d_check_char (di, 'E')) - return NULL; + recursion_level ++; + if (d_check_char (di, 'F')) + { + if (d_peek_char (di) == 'Y') + { + /* Function has C linkage. We don't print this information. + FIXME: We should print it in verbose mode. */ + d_advance (di, 1); + } + ret = d_bare_function_type (di, 1); + ret = d_ref_qualifier (di, ret); + + if (! d_check_char (di, 'E')) + ret = NULL; + } + + recursion_level --; return ret; } @@ -6242,6 +6259,20 @@ cplus_demangle_init_info (mangled, options, strlen (mangled), &di); + /* PR 87675 - Check for a mangled string that is so long + that we do not have enough stack space to demangle it. */ + if ((options & DMGL_RECURSE_LIMIT) + /* This check is a bit arbitrary, since what we really want to do is to + compare the sizes of the di.comps and di.subs arrays against the + amount of stack space remaining. But there is no portable way to do + this, so instead we use the recursion limit as a guide to the maximum + size of the arrays. */ + && (unsigned long) di.num_comps > demangle_recursion_limit) + { + /* FIXME: We need a way to indicate that a stack limit has been reached. */ + return 0; + } + { #ifdef CP_DYNAMIC_ARRAYS __extension__ struct demangle_component comps[di.num_comps]; @@ -6324,6 +6355,23 @@ return dgs.buf; } +/* Set a limit on the amount of recursion allowed in mangled strings. + Only effective if the DMGL_RECURSE_LIMIT option has been set. + Returns the previous value of the recursion limit. + Ignores settings a limit of 0 or 1. + Note - setting the limit is not a thread-safe operation. */ + +CP_STATIC_IF_GLIBCPP_V3 +unsigned long +cplus_demangle_set_recursion_limit (unsigned long limit) +{ + unsigned long result = demangle_recursion_limit; + + if (limit > 1) + demangle_recursion_limit = limit; + return result; +} + #if defined(IN_LIBGCC2) || defined(IN_GLIBCPP_V3) extern char *__cxa_demangle (const char *, char *, size_t *, int *); Index: libiberty/cplus-dem.c =================================================================== --- libiberty/cplus-dem.c (revision 266657) +++ libiberty/cplus-dem.c (working copy) @@ -4693,10 +4693,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) + && recursion_level > cplus_demangle_set_recursion_limit (0)) + { + /* FIXME: There ought to be a way to report that the recursion limit + has been reached. */ + return 0; + } + + recursion_level ++; + /* The G++ name-mangling algorithm does not remember types on nested argument lists, unless -fsquangling is used, and in that case the type vector updated by remember_type is not used. So, we turn @@ -4723,6 +4734,7 @@ --work->forgetting_types; work->nrepeats = saved_nrepeats; + --recursion_level; return result; } --=-=-=--