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 <ni...@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.
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;
}
