Re: Fix fir PR71696 in Libiberty Demangler (6)
is_proctypevec = 1; + push_processed_type (work, n); + remembered_type = work->typevec[n]; mangled = &remembered_type; } break; @@ -3850,6 +3877,9 @@ do_type (struct work_stuff *work, const char **man string_delete (result); string_delete (&decl); + if (is_proctypevec) +pop_processed_type (work); + if (success) /* Assume an integral type, if we're not sure. */ return (int) ((tk == tk_none) ? tk_integral : tk); @@ -4263,6 +4293,41 @@ do_arg (struct work_stuff *work, const char **mang } static void +push_processed_type (struct work_stuff *work, int typevec_index) +{ + if (work->nproctypes >= work->proctypevec_size) +{ + if (!work->proctypevec_size) + { + work->proctypevec_size = 4; + work->proctypevec = XNEWVEC (int, work->proctypevec_size); + } + else + { + if (work->proctypevec_size < 16) + /* Double when small. */ + work->proctypevec_size *= 2; + else + { + /* Grow slower when large. */ + if (work->proctypevec_size > (INT_MAX / 3) * 2) +xmalloc_failed (INT_MAX); + work->proctypevec_size = (work->proctypevec_size * 3 / 2); + } + work->proctypevec += XRESIZEVEC (int, work->proctypevec, work->proctypevec_size); + } +} +work->proctypevec [work->nproctypes++] = typevec_index; +} + +static void +pop_processed_type (struct work_stuff *work) +{ + work->nproctypes--; +} + +static void remember_type (struct work_stuff *work, const char *start, int len) { char *tem; @@ -4526,10 +4591,13 @@ demangle_args (struct work_stuff *work, const char { string_append (declp, ", "); } + push_processed_type (work, t); if (!do_arg (work, &tem, &arg)) { + pop_processed_type (work); return (0); } + pop_processed_type (work); if (PRINT_ARG_TYPES) { string_appends (declp, &arg); Index: libiberty/testsuite/demangle-expected === --- libiberty/testsuite/demangle-expected (revision 239112) +++ libiberty/testsuite/demangle-expected (working copy) @@ -4587,3 +4587,8 @@ _Z808 __t2m05B50_ __t2m05B50_ +# +# Tests stack overflow PR71696 + +__10%0__S4_0T0T0 +%0<>::%0(%0<>) Index: libiberty/ChangeLog === --- libiberty/ChangeLog (revision 239112) +++ libiberty/ChangeLog (working copy) @@ -1,3 +1,20 @@ +2016-08-04 Marcel Böhme + + PR c++/71696 + * cplus-dem.c: Prevent infinite recursion when there is a cycle + in the referencing of remembered mangled types. + (work_stuff): New stack to keep track of the remembered mangled + types that are currently being processed. + (push_processed_type): New method to push currently processed + remembered type onto the stack. + (pop_processed_type): New method to pop currently processed + remembered type from the stack. + (work_stuff_copy_to_from): Copy values of new variables. + (delete_non_B_K_work_stuff): Free stack memory. + (demangle_args): Push/Pop currently processed remembered type. + (do_type): Do not demangle a cyclic reference and push/pop + referenced remembered type. + 2016-07-29 Aldy Hernandez * make-relative-prefix.c (make_relative_prefix_1): Fall back to @@ -16,7 +33,7 @@ (d_template_args_1): Split out from d_template_args. (d_args_length): New. -2016-07-13 Marcel BÃhme +2016-07-13 Marcel Böhme PR c++/70926 * cplus-dem.c: Handle large values and overflow when demangling Index: libiberty/cplus-dem.c === --- libiberty/cplus-dem.c (revision 239112) +++ libiberty/cplus-dem.c (working copy) @@ -144,6 +144,9 @@ struct work_stuff string* previous_argument; /* The last function argument demangled. */ int nrepeats; /* The number of times to repeat the previous argument. */ + int *proctypevec; /* Indices of currently processed remembered typevecs. */ + int proctypevec_size; + int nproctypes; }; #define PRINT_ANSI_QUALIFIERS (work -> options & DMGL_ANSI) @@ -436,6 +439,10 @@ iterate_demangle_function (struct work_stuff *, static void remember_type (struct work_stuff *, const char *, int); +static void push_processed_type (struct work_stuff *, int); + +static void pop_processed_type (struct work_stuff *); + static void remem
Re: Fix fir PR71696 in Libiberty Demangler (6)
Hi, This patch is still pending a full review. Best regards, - Marcel > On 4 Jul 2016, at 8:54 PM, Bernd Schmidt wrote: > > On 06/30/2016 08:46 AM, Marcel Böhme wrote: >> The attached patch fixes the stack overflow in the demangler due to >> cycles in the references of “remembered” mangled types >> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71696). >> >> The methods demangle_signature and do_arg in cplus-dem.c allow to >> “remember” mangled type names that can later be referenced and will >> also be demangled. The method demangle_args demangles those types >> following any references. So, if there is a cycle in the referencing >> (or in the simplest case a self-reference), the method enters >> infinite recursion. >> >> The patch tracks the mangled types that are currently being demangled >> in a new variable called work->proctypevec. If a referenced type is >> currently being demangled, the demangling is marked as not >> successful. > > I'll defer reviewing these to someone who understands demangling better. You > might want to Cc Jason. > > > Bernd > >
Re: Fix for PR70909 in Libiberty Demangler (4)
Hi, This patch is still pending a full review. Best regards, - Marcel > On 30 Jun 2016, at 12:09 AM, Pedro Alves wrote: > > On 06/29/2016 08:43 AM, Marcel Böhme wrote: >> Hi Jason, >> >> These test cases are generated by fuzzing which produces a lot of >> nonsensical input data. >> I think, "Garbage In, Garbage Out" is quite applicable here. >> With the patch at least it doesn’t crash and fixes the vulnerability. > > Note that demangling shows up high in gdb profiles when loading > huge programs. If we can avoid quadratic or worse complexity, > it'd preferred. > > Thanks, > Pedro Alves >
Fix fir PR71696 in Libiberty Demangler (6)
Hi, The attached patch fixes the stack overflow in the demangler due to cycles in the references of “remembered” mangled types (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71696). The methods demangle_signature and do_arg in cplus-dem.c allow to “remember” mangled type names that can later be referenced and will also be demangled. The method demangle_args demangles those types following any references. So, if there is a cycle in the referencing (or in the simplest case a self-reference), the method enters infinite recursion. The patch tracks the mangled types that are currently being demangled in a new variable called work->proctypevec. If a referenced type is currently being demangled, the demangling is marked as not successful. Bootstrapped and regression tested on x86_64-pc-linux-gnu. Test case added to libiberty/testsuite/demangler-expected and checked PR71696 is resolved. Index: libiberty/ChangeLog === --- libiberty/ChangeLog (revision 237852) +++ libiberty/ChangeLog (working copy) @@ -1,3 +1,21 @@ +2016-06-30 Marcel Böhme + + * cplus-dem.c: Prevent infinite recursion when there is a cycle + in the referencing of remembered mangled types. + (work_stuff): New stack to keep track of the remembered mangled + types that are currently being processed. + (push_processed_type): New method to push currently processed + remembered type onto the stack. + (pop_processed_type): New method to pop currently processed + remembered type from the stack. + (work_stuff_copy_to_from): Copy values of new variables. + (delete_non_B_K_work_stuff): Free stack memory. + (demangle_args): Push/Pop currently processed remembered type. + (do_type): Do not demangle a cyclic reference and push/pop + referenced remembered type. + + * testsuite/demangle-expected: Add tests. + 2016-05-31 Alan Modra * xmemdup.c (xmemdup): Use xmalloc rather than xcalloc. Index: libiberty/cplus-dem.c === --- libiberty/cplus-dem.c (revision 237852) +++ libiberty/cplus-dem.c (working copy) @@ -144,6 +144,9 @@ struct work_stuff string* previous_argument; /* The last function argument demangled. */ int nrepeats; /* The number of times to repeat the previous argument. */ + int *proctypevec; /* Indices of currently processed remembered typevecs. */ + int proctypevec_size; + int nproctypes; }; #define PRINT_ANSI_QUALIFIERS (work -> options & DMGL_ANSI) @@ -436,6 +439,10 @@ iterate_demangle_function (struct work_stuff *, static void remember_type (struct work_stuff *, const char *, int); +static void push_processed_type (struct work_stuff *, int); + +static void pop_processed_type (struct work_stuff *); + static void remember_Btype (struct work_stuff *, const char *, int, int); static int register_Btype (struct work_stuff *); @@ -1302,6 +1309,13 @@ work_stuff_copy_to_from (struct work_stuff *to, st memcpy (to->btypevec[i], from->btypevec[i], len); } + if (from->proctypevec) +{ + to->proctypevec = XNEWVEC (int, from->proctypevec_size); + memcpy (to->proctypevec, from->proctypevec, + from->proctypevec_size * sizeof(int)); +} + if (from->ntmpl_args) to->tmpl_argvec = XNEWVEC (char *, from->ntmpl_args); @@ -1336,6 +1350,12 @@ delete_non_B_K_work_stuff (struct work_stuff *work work -> typevec = NULL; work -> typevec_size = 0; } + if (work -> proctypevec != NULL) +{ + free (work -> proctypevec); + work -> proctypevec = NULL; + work -> proctypevec_size = 0; +} if (work->tmpl_argvec) { int i; @@ -3554,6 +3574,8 @@ static int do_type (struct work_stuff *work, const char **mangled, string *result) { int n; + int i; + int is_proctypevec; int done; int success; string decl; @@ -3566,6 +3588,7 @@ do_type (struct work_stuff *work, const char **man done = 0; success = 1; + is_proctypevec = 0; while (success && !done) { int member; @@ -3626,7 +3649,14 @@ do_type (struct work_stuff *work, const char **man success = 0; } else - { + for (i = 0; i < work -> nproctypes; i++) + if (work -> proctypevec [i] == n) + success = 0; + + if (success) + { + is_proctypevec = 1; + push_processed_type (work, n); remembered_type = work -> typevec[n]; mangled = &remembered_type; } @@ -3849,6 +3879,9 @@ do_type (struct work_stuff *work, const char **man string_delete (result); string_delete (&decl); + if (is_proctypevec) +pop_processed_type (work);
Re: Fix for PR70909 in Libiberty Demangler (4)
Hi Jason, These test cases are generated by fuzzing which produces a lot of nonsensical input data. I think, "Garbage In, Garbage Out" is quite applicable here. With the patch at least it doesn’t crash and fixes the vulnerability. Best regards, - Marcel > On 26 May 2016, at 10:05 PM, Jason Merrill wrote: > > It seems like in cases of malformed input we should return the input > again rather than produce garbage like "K> ". Maybe catch this sort of situation in > d_lookup_template_parameter? > > Jason > > > On Mon, May 2, 2016 at 11:21 AM, Marcel Böhme wrote: >> Hi, >> >> This fixes several stack overflows due to infinite recursion in d_print_comp >> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70909). >> >> The method d_print_comp in cp-demangle.c recursively constructs the >> d_print_info dpi from the demangle_component dc. The method >> d_print_comp_inner traverses dc as a graph. Now, dc can be a graph with >> cycles leading to infinite recursion in several distinct cases. The patch >> uses the component stack to find whether the current node dc has itself as >> ancestor more than once. >> >> Bootstrapped and regression tested on x86_64-pc-linux-gnu. Test cases added >> to libiberty/testsuite/demangler-expected and checked PR70909 and related >> stack overflows are resolved. >> >> Best regards, >> - Marcel >> >> >> >> Index: ChangeLog >> === >> --- ChangeLog (revision 235760) >> +++ ChangeLog (working copy) >> @@ -1,3 +1,19 @@ >> +2016-05-02 Marcel Böhme >> + >> + PR c++/70909 >> + PR c++/61460 >> + PR c++/68700 >> + PR c++/67738 >> + PR c++/68383 >> + PR c++/70517 >> + PR c++/61805 >> + PR c++/62279 >> + PR c++/67264 >> + * cp-demangle.c: Prevent infinite recursion when traversing cyclic >> + demangle component. >> + (d_print_comp): Return when demangle component has itself as ancistor >> + more than once. >> + >> 2016-04-30 Oleg Endo >> >>* configure: Remove SH5 support. >> Index: cp-demangle.c >> === >> --- cp-demangle.c (revision 235760) >> +++ cp-demangle.c (working copy) >> @@ -5436,6 +5436,24 @@ d_print_comp (struct d_print_info *dpi, int option >> { >> struct d_component_stack self; >> >> + self.parent = dpi->component_stack; >> + >> + while (self.parent) >> +{ >> + self.dc = self.parent->dc; >> + self.parent = self.parent->parent; >> + if (dc != NULL && self.dc == dc) >> + { >> + while (self.parent) >> + { >> + self.dc = self.parent->dc; >> + self.parent = self.parent->parent; >> + if (self.dc == dc) >> + return; >> + } >> + } >> +} >> + >> self.dc = dc; >> self.parent = dpi->component_stack; >> dpi->component_stack = &self; >> Index: testsuite/demangle-expected >> === >> --- testsuite/demangle-expected (revision 235760) >> +++ testsuite/demangle-expected (working copy) >> @@ -4431,3 +4431,69 @@ _Q.__0 >> >> _Q10-__9cafebabe. >> cafebabe.::-(void) >> +# >> +# Test demangler crash PR62279 >> + >> +_ZN5Utils9transformIPN15ProjectExplorer13BuildStepListEZNKS1_18BuildConfiguration14knownStepListsEvEUlS3_E_EE5QListIDTclfp0_cvT__RKS6_IS7_ET0_ >> +QList >> Utils::transform> ProjectExplorer::BuildConfiguration::knownStepLists() >> const::{lambda(ProjectExplorer::BuildStepList*)#1}>(ProjectExplorer::BuildConfiguration::knownStepLists() >> const::{lambda(ProjectExplorer::BuildStepList*)#1} const&, >> ProjectExplorer::BuildConfiguration::knownStepLists() >> const::{lambda(ProjectExplorer::BuildStepList*)#1}) >> +# >> + >> +_ZSt7forwardIKSaINSt6thread5_ImplISt12_Bind_simpleIFZN6WIM_DL5Utils9AsyncTaskC4IMNS3_8Hardware12FpgaWatchdogEKFvvEIPS8_EEEibOT_DpOT0_EUlvE_vEESD_RNSt16remove_referenceISC_E4typeE >> +std::allocator> (WIM_DL::Hardware::FpgaWatchdog::*)() const, >> WIM_DL::Hardware::FpgaWatchdog*>(int, bool, void >> (WIM_DL::Hardware::FpgaWatchdog::*&&)() const, >> WIM_DL::Hardware::FpgaWatchdog*&&)::{lambda()#1} ()> > >
Re: Fix for PR70926 in Libiberty Demangler (5)
Hi Jeff, On 23 Jun 2016, at 4:21 AM, Jeff Law wrote: > > OK for the trunk. Please install. > > Sorry for the delays. > > Jeff I might not have the access rights to commit to trunk. Best regards - Marcel
Re: Fix for PR70909 in Libiberty Demangler (4)
Hi, This patch is pending a careful review. Best regards, - Marcel > On 2 May 2016, at 11:21 PM, Marcel Böhme wrote: > > Hi, > > This fixes several stack overflows due to infinite recursion in d_print_comp > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70909). > > The method d_print_comp in cp-demangle.c recursively constructs the > d_print_info dpi from the demangle_component dc. The method > d_print_comp_inner traverses dc as a graph. Now, dc can be a graph with > cycles leading to infinite recursion in several distinct cases. The patch > uses the component stack to find whether the current node dc has itself as > ancestor more than once. > > Bootstrapped and regression tested on x86_64-pc-linux-gnu. Test cases added > to libiberty/testsuite/demangler-expected and checked PR70909 and related > stack overflows are resolved. > > Best regards, > - Marcel > > > > Index: ChangeLog > === > --- ChangeLog (revision 235760) > +++ ChangeLog (working copy) > @@ -1,3 +1,19 @@ > +2016-05-02 Marcel Böhme > + > + PR c++/70909 > + PR c++/61460 > + PR c++/68700 > + PR c++/67738 > + PR c++/68383 > + PR c++/70517 > + PR c++/61805 > + PR c++/62279 > + PR c++/67264 > + * cp-demangle.c: Prevent infinite recursion when traversing cyclic > + demangle component. > + (d_print_comp): Return when demangle component has itself as ancistor > + more than once. > + > 2016-04-30 Oleg Endo > > * configure: Remove SH5 support. > Index: cp-demangle.c > === > --- cp-demangle.c (revision 235760) > +++ cp-demangle.c (working copy) > @@ -5436,6 +5436,24 @@ d_print_comp (struct d_print_info *dpi, int option > { > struct d_component_stack self; > > + self.parent = dpi->component_stack; > + > + while (self.parent) > +{ > + self.dc = self.parent->dc; > + self.parent = self.parent->parent; > + if (dc != NULL && self.dc == dc) > + { > + while (self.parent) > + { > + self.dc = self.parent->dc; > + self.parent = self.parent->parent; > + if (self.dc == dc) > + return; > + } > + } > +} > + > self.dc = dc; > self.parent = dpi->component_stack; > dpi->component_stack = &self; > Index: testsuite/demangle-expected > === > --- testsuite/demangle-expected (revision 235760) > +++ testsuite/demangle-expected (working copy) > @@ -4431,3 +4431,69 @@ _Q.__0 > > _Q10-__9cafebabe. > cafebabe.::-(void) > +# > +# Test demangler crash PR62279 > + > +_ZN5Utils9transformIPN15ProjectExplorer13BuildStepListEZNKS1_18BuildConfiguration14knownStepListsEvEUlS3_E_EE5QListIDTclfp0_cvT__RKS6_IS7_ET0_ > +QList > Utils::transform ProjectExplorer::BuildConfiguration::knownStepLists() > const::{lambda(ProjectExplorer::BuildStepList*)#1}>(ProjectExplorer::BuildConfiguration::knownStepLists() > const::{lambda(ProjectExplorer::BuildStepList*)#1} const&, > ProjectExplorer::BuildConfiguration::knownStepLists() > const::{lambda(ProjectExplorer::BuildStepList*)#1}) > +# > + > +_ZSt7forwardIKSaINSt6thread5_ImplISt12_Bind_simpleIFZN6WIM_DL5Utils9AsyncTaskC4IMNS3_8Hardware12FpgaWatchdogEKFvvEIPS8_EEEibOT_DpOT0_EUlvE_vEESD_RNSt16remove_referenceISC_E4typeE > +std::allocator (WIM_DL::Hardware::FpgaWatchdog::*)() const, > WIM_DL::Hardware::FpgaWatchdog*>(int, bool, void > (WIM_DL::Hardware::FpgaWatchdog::*&&)() const, > WIM_DL::Hardware::FpgaWatchdog*&&)::{lambda()#1} ()> > > const&& > std::forward (WIM_DL::Hardware::FpgaWatchdog::*)() const, > WIM_DL::Hardware::FpgaWatchdog*>(int, bool, > std::allocator (WIM_DL::Hardware::FpgaWatchdog::*)() const, > WIM_DL::Hardware::FpgaWatchdog*>(int, bool, void > (WIM_DL::Hardware::FpgaWatchdog::*&&)() const, > WIM_DL::Hardware::FpgaWatchdog*&&)::{lambda()#1} ()> > > const&&, > WIM_DL::Hardware::FpgaWatchdog*&&)::{lambda()#1} ()> > > > const>(std::remove_reference (WIM_DL::Hardware::FpgaWatchdog::*)() const, > WIM_DL::Hardware::FpgaWatchdog*>(int, bool, void > (WIM_DL::Hardware::FpgaWatchdog::*&&)() const, > WIM_DL::Hardware::FpgaWatchdog*&&)::{lambda()#1} ()> > > const>::type&) > +# > +# Test demangler crash PR61805 > + > +_ZNK5niven5ColorIfLi4EEdvIfEENSt9enable_ifIXsrSt13is_arithmeticIT_E5valueEKNS0_IDTmlcvS5__Ec
Re: Fix for PR70926 in Libiberty Demangler (5)
Hi: Pending review. Best - Marcel > On 3 May 2016, at 10:40 PM, Marcel Böhme wrote: > > Hi, > > This fixes four access violations > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70926). > > Two of these first read the value of a length variable len from the mangled > string, then strncpy len characters from the mangled string; more than > necessary. > The other two read the value of an array index n from the mangled string, > which can be negative due to an overflow. > > Bootstrapped and regression tested on x86_64-pc-linux-gnu. Test cases added > to libiberty/testsuite/demangler-expected and checked PR70926 is resolved. > > Best regards, > - Marcel > > Index: libiberty/ChangeLog > === > --- libiberty/ChangeLog (revision 235801) > +++ libiberty/ChangeLog (working copy) > @@ -1,3 +1,12 @@ > +2016-05-03 Marcel Böhme > + > + PR c++/70926 > + * cplus-dem.c: Handle large values and overflow when demangling > + length variables. > + (demangle_template_value_parm): Read only until end of mangled string. > > + (do_hpacc_template_literal): Likewise. > + (do_type): Handle overflow when demangling array indices. > + > 2016-05-02 Marcel Böhme > > PR c++/70498 > Index: libiberty/cplus-dem.c > === > --- libiberty/cplus-dem.c (revision 235801) > +++ libiberty/cplus-dem.c (working copy) > @@ -2051,7 +2051,8 @@ demangle_template_value_parm (struct work_stuff *w > else > { > int symbol_len = consume_count (mangled); > - if (symbol_len == -1) > + if (symbol_len == -1 > + || symbol_len > (long) strlen (*mangled)) > return -1; > if (symbol_len == 0) > string_appendn (s, "0", 1); > @@ -3611,7 +3612,7 @@ do_type (struct work_stuff *work, const char **man > /* A back reference to a previously seen type */ > case 'T': > (*mangled)++; > - if (!get_count (mangled, &n) || n >= work -> ntypes) > + if (!get_count (mangled, &n) || n < 0 || n >= work -> ntypes) > { > success = 0; > } > @@ -3789,7 +3790,7 @@ do_type (struct work_stuff *work, const char **man > /* A back reference to a previously seen squangled type */ > case 'B': > (*mangled)++; > - if (!get_count (mangled, &n) || n >= work -> numb) > + if (!get_count (mangled, &n) || n < 0 || n >= work -> numb) > success = 0; > else > string_append (result, work->btypevec[n]); > @@ -4130,7 +4131,8 @@ do_hpacc_template_literal (struct work_stuff *work > > literal_len = consume_count (mangled); > > - if (literal_len <= 0) > + if (literal_len <= 0 > + || literal_len > (long) strlen (*mangled)) > return 0; > > /* Literal parameters are names of arrays, functions, etc. and the > Index: libiberty/testsuite/demangle-expected > === > --- libiberty/testsuite/demangle-expected (revision 235801) > +++ libiberty/testsuite/demangle-expected (working copy) > @@ -4441,3 +4441,16 @@ __vt_900cafebabe > > _Z808 > _Z808 > +# > +# Tests write access violation PR70926 > + > +0__Ot2m02R5T50 > +0__Ot2m02R5T50 > +# > + > +0__GT500_ > +0__GT500_ > +# > + > +__t2m05B50_ > +__t2m05B50_ >
Re: Fix for PR68159 in Libiberty Demangler (6)
Hi Ian, Stack overflows are a security concern and must be addressed. The Libiberty demangler is part of several tools, including binutils, gdb, valgrind, and many other libbfd-based tools that are used by the security community for the analysis of program binaries. Without a patch, the reverse engineering of untrusted binaries as well as determining whether an untrusted binary is malicious could cause serious damage. More details here: http://www.openwall.com/lists/oss-security/2016/05/05/3 > On 7 May 2016, at 12:16 AM, Ian Lance Taylor wrote: > > The function cplus_demangle_v3_callback must not call malloc. The > whole point of that function is to work when nothing else works. That > is why d_demangle_callback does not, and must not, call malloc. Point taken. In fact, I tracked down the patch submitted by Google's Simon Baldwin and the ensuing discussion from 2007: https://gcc.gnu.org/ml/gcc-patches/2007-01/msg01116.html (committed as revision 121305). In that thread, Mark Mitchell raised concerns about small stacks and large mangled names and suggested to focus on an allocation interface where the the caller provides "alloc" and "dealloc" functions (i.e., C++ allocators): https://gcc.gnu.org/ml/gcc-patches/2007-01/msg01904.html In the later patch to libstdc++ which has vterminate use the malloc-less demangler, Benjamin Kosnik raised similar concerns: https://gcc.gnu.org/ml/libstdc++/2007-03/msg00181.html Perhaps the allocation interface is the way to go? Best regards, - Marcel
Re: Fix for PR68159 in Libiberty Demangler (6)
Hi, This patch also removes the following part of the comment for method cplus_demangle_print_callback: "It does not use heap memory to build an output string, so cannot encounter memory allocation failure”. > On 6 May 2016, at 11:11 PM, Marcel Böhme wrote: > > >> If one malloc succeeds and the other fails, you leak memory. >> >> Jakub > Nice catch. Thanks! > > Bootstrapped and regression tested on x86_64-pc-linux-gnu. Best - Marcel Index: libiberty/ChangeLog === --- libiberty/ChangeLog (revision 235962) +++ libiberty/ChangeLog (working copy) @@ -1,3 +1,14 @@ +2016-05-06 Marcel Böhme + + PR c++/68159 + * cp-demangle.c: Allocate arrays of user-defined size on the heap, + not on the stack. Do not include . + (CP_DYNAMIC_ARRAYS): Remove definition. + (cplus_demangle_print_callback): Allocate memory for two arrays on + the heap. Free memory before return / exit. + (d_demangle_callback): Likewise. + (is_ctor_or_dtor): Likewise. + * testsuite/demangle-expected: Add regression test cases. + 2016-05-02 Marcel Böhme PR c++/70498 Index: libiberty/cp-demangle.c === --- libiberty/cp-demangle.c (revision 235962) +++ libiberty/cp-demangle.c (working copy) @@ -116,18 +116,6 @@ #include #endif -#ifdef HAVE_ALLOCA_H -# include -#else -# ifndef alloca -# ifdef __GNUC__ -# define alloca __builtin_alloca -# else -extern char *alloca (); -# endif /* __GNUC__ */ -# endif /* alloca */ -#endif /* HAVE_ALLOCA_H */ - #ifdef HAVE_LIMITS_H #include #endif @@ -186,20 +174,6 @@ static void d_init_info (const char *, int, size_t #define CP_STATIC_IF_GLIBCPP_V3 #endif /* ! defined(IN_GLIBCPP_V3) */ -/* See if the compiler supports dynamic arrays. */ - -#ifdef __GNUC__ -#define CP_DYNAMIC_ARRAYS -#else -#ifdef __STDC__ -#ifdef __STDC_VERSION__ -#if __STDC_VERSION__ >= 199901L -#define CP_DYNAMIC_ARRAYS -#endif /* __STDC__VERSION >= 199901L */ -#endif /* defined (__STDC_VERSION__) */ -#endif /* defined (__STDC__) */ -#endif /* ! defined (__GNUC__) */ - /* We avoid pulling in the ctype tables, to prevent pulling in additional unresolved symbols when this code is used in a library. FIXME: Is this really a valid reason? This comes from the original @@ -4112,9 +4086,7 @@ d_last_char (struct d_print_info *dpi) CALLBACK is a function to call to flush demangled string segments as they fill the intermediate buffer, and OPAQUE is a generalized callback argument. On success, this returns 1. On failure, - it returns 0, indicating a bad parse. It does not use heap - memory to build an output string, so cannot encounter memory - allocation failure. */ + it returns 0, indicating a bad parse. */ CP_STATIC_IF_GLIBCPP_V3 int @@ -4126,25 +4098,32 @@ cplus_demangle_print_callback (int options, d_print_init (&dpi, callback, opaque, dc); - { -#ifdef CP_DYNAMIC_ARRAYS -__extension__ struct d_saved_scope scopes[dpi.num_saved_scopes]; -__extension__ struct d_print_template temps[dpi.num_copy_templates]; + dpi.copy_templates += (struct d_print_template *) malloc (((size_t) dpi.num_copy_templates) + * sizeof (*dpi.copy_templates)); + if (! dpi.copy_templates) +{ + d_print_error (&dpi); + return 0; +} -dpi.saved_scopes = scopes; -dpi.copy_templates = temps; -#else -dpi.saved_scopes = alloca (dpi.num_saved_scopes - * sizeof (*dpi.saved_scopes)); -dpi.copy_templates = alloca (dpi.num_copy_templates -* sizeof (*dpi.copy_templates)); -#endif + dpi.saved_scopes += (struct d_saved_scope *) malloc (((size_t) dpi.num_saved_scopes) + * sizeof (*dpi.saved_scopes)); + if (! dpi.saved_scopes) +{ + free (dpi.copy_templates); + d_print_error (&dpi); + return 0; +} -d_print_comp (&dpi, options, dc); - } + d_print_comp (&dpi, options, dc); d_print_flush (&dpi); + free (dpi.copy_templates); + free (dpi.saved_scopes); + return ! d_print_saw_error (&dpi); } @@ -5945,57 +5924,61 @@ d_demangle_callback (const char *mangled, int opti cplus_demangle_init_info (mangled, options, strlen (mangled), &di); - { -#ifdef CP_DYNAMIC_ARRAYS -__extension__ struct demangle_component comps[di.num_comps]; -__extension__ struct demangle_component *subs[di.num_subs]; + di.comps = (struct demangle_component *) malloc (((size_t) di.num_comps) + * sizeof (*di.comps)); + if (! di.comps) +return 0; -di.comps = comps; -di.subs = subs; -#else -di.comps = alloca (di.num_comps * sizeof (*di.comps)); -di.subs = alloca (di
Re: Fix for PR68159 in Libiberty Demangler (6)
> If one malloc succeeds and the other fails, you leak memory. > > Jakub Nice catch. Thanks! Bootstrapped and regression tested on x86_64-pc-linux-gnu. Best - Marcel Index: libiberty/ChangeLog === --- libiberty/ChangeLog (revision 235962) +++ libiberty/ChangeLog (working copy) @@ -1,3 +1,14 @@ +2016-05-06 Marcel Böhme + + PR c++/68159 + * cp-demangle.c: Allocate arrays of user-defined size on the heap, + not on the stack. Do not include . + (CP_DYNAMIC_ARRAYS): Remove definition. + (cplus_demangle_print_callback): Allocate memory for two arrays on + the heap. Free memory before return / exit. + (d_demangle_callback): Likewise. + (is_ctor_or_dtor): Likewise. + * testsuite/demangle-expected: Add regression test cases. + 2016-05-02 Marcel Böhme PR c++/70498 Index: libiberty/cp-demangle.c === --- libiberty/cp-demangle.c (revision 235962) +++ libiberty/cp-demangle.c (working copy) @@ -116,18 +116,6 @@ #include #endif -#ifdef HAVE_ALLOCA_H -# include -#else -# ifndef alloca -# ifdef __GNUC__ -# define alloca __builtin_alloca -# else -extern char *alloca (); -# endif /* __GNUC__ */ -# endif /* alloca */ -#endif /* HAVE_ALLOCA_H */ - #ifdef HAVE_LIMITS_H #include #endif @@ -186,20 +174,6 @@ static void d_init_info (const char *, int, size_t #define CP_STATIC_IF_GLIBCPP_V3 #endif /* ! defined(IN_GLIBCPP_V3) */ -/* See if the compiler supports dynamic arrays. */ - -#ifdef __GNUC__ -#define CP_DYNAMIC_ARRAYS -#else -#ifdef __STDC__ -#ifdef __STDC_VERSION__ -#if __STDC_VERSION__ >= 199901L -#define CP_DYNAMIC_ARRAYS -#endif /* __STDC__VERSION >= 199901L */ -#endif /* defined (__STDC_VERSION__) */ -#endif /* defined (__STDC__) */ -#endif /* ! defined (__GNUC__) */ - /* We avoid pulling in the ctype tables, to prevent pulling in additional unresolved symbols when this code is used in a library. FIXME: Is this really a valid reason? This comes from the original @@ -4126,25 +4100,31 @@ cplus_demangle_print_callback (int options, d_print_init (&dpi, callback, opaque, dc); - { -#ifdef CP_DYNAMIC_ARRAYS -__extension__ struct d_saved_scope scopes[dpi.num_saved_scopes]; -__extension__ struct d_print_template temps[dpi.num_copy_templates]; + dpi.copy_templates += (struct d_print_template *) malloc (((size_t) dpi.num_copy_templates) + * sizeof (*dpi.copy_templates)); + if (! dpi.copy_templates) +{ + d_print_error (&dpi); + return 0; +} -dpi.saved_scopes = scopes; -dpi.copy_templates = temps; -#else -dpi.saved_scopes = alloca (dpi.num_saved_scopes - * sizeof (*dpi.saved_scopes)); -dpi.copy_templates = alloca (dpi.num_copy_templates -* sizeof (*dpi.copy_templates)); -#endif + dpi.saved_scopes += (struct d_saved_scope *) malloc (((size_t) dpi.num_saved_scopes) + * sizeof (*dpi.saved_scopes)); + if (! dpi.saved_scopes) +{ + d_print_error (&dpi); + return 0; +} -d_print_comp (&dpi, options, dc); - } + d_print_comp (&dpi, options, dc); d_print_flush (&dpi); + free (dpi.copy_templates); + free (dpi.saved_scopes); + return ! d_print_saw_error (&dpi); } @@ -5945,57 +5925,58 @@ d_demangle_callback (const char *mangled, int opti cplus_demangle_init_info (mangled, options, strlen (mangled), &di); - { -#ifdef CP_DYNAMIC_ARRAYS -__extension__ struct demangle_component comps[di.num_comps]; -__extension__ struct demangle_component *subs[di.num_subs]; + di.comps = (struct demangle_component *) malloc (((size_t) di.num_comps) + * sizeof (*di.comps)); + if (! di.comps) +return 0; -di.comps = comps; -di.subs = subs; -#else -di.comps = alloca (di.num_comps * sizeof (*di.comps)); -di.subs = alloca (di.num_subs * sizeof (*di.subs)); -#endif + di.subs = (struct demangle_component **) malloc (((size_t) di.num_subs) + * sizeof (*di.subs)); + if (! di.subs) +return 0; + + switch (type) +{ +case DCT_TYPE: + dc = cplus_demangle_type (&di); + break; +case DCT_MANGLED: + dc = cplus_demangle_mangled_name (&di, 1); + break; +case DCT_GLOBAL_CTORS: +case DCT_GLOBAL_DTORS: + d_advance (&di, 11); + dc = d_make_comp (&di, + (type == DCT_GLOBAL_CTORS +? DEMANGLE_COMPONENT_GLOBAL_CONSTRUCTORS +: DEMANGLE_COMPONENT_GLOBAL_DESTRUCTORS), + d_make_demangle_mangled_name (&di, d_str (&di)), + NULL); +
Re: Fix for PR68159 in Libiberty Demangler (6)
Hi Jakub, > On 6 May 2016, at 5:51 PM, Jakub Jelinek wrote: >> > > If you just want an array, restricting the size including the sizeof > to fit into int makes no sense, you want to guard it against overflows > during the multiplication. Okay, done. (Someone might want to substitute size_t with unsigned int if the former causes any problems). > Ian or Jason, can all the demangle users allocate heap memory or not? This question remains. > But much more importantly, you don't handle the allocation failure in > anyway, so if malloc fails, you'll just segfault. It is handled now. No abort. No overflow. I also checked: Even if num_saved_scopes or num_copy_templates happen to overflow in d_count_templates_scopes, that integer overflow won’t lead to a buffer overflow because of the checks (and only their uses) in lines 4292 and 4307. 4292: if (dpi->next_saved_scope >= dpi->num_saved_scopes) 4293: { 4294: d_print_error (dpi); 4295: return; 4296: } 4307: if (dpi->next_saved_scope >= dpi->num_saved_scopes) 4307:{ 4307: d_print_error (dpi); 4307: return; 4307:} As for your previous email: > On 6 May 2016, at 3:09 PM, Jakub Jelinek wrote: > > Furthermore, if I apply your patch and rebuild libstdc++, it stops working > altogether: > ldd -d -r ./libstdc++.so.6.0.22 > linux-vdso.so.1 (0x7ffe6053c000) > libm.so.6 => /lib64/libm.so.6 (0x7f9de23fb000) > libc.so.6 => /lib64/libc.so.6 (0x7f9de203a000) > /lib64/ld-linux-x86-64.so.2 (0x5585ecc1d000) > libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x7f9de1e22000) > undefined symbol: xmalloc (./libstdc++.so.6.0.22) > undefined symbol: xmalloc_failed (./libstdc++.so.6.0.22) Hmm. Just checked, xmalloc should be available through libiberty.h which is imported by cp-demangle.c. That earlier patch was successfully bootstrapped and regression tested on on x86_64-pc-linux-gnu from the sources in trunk. BTW: If I configure libstdc++-v3 directly, I receive an error message: ... ./config.status: creating include/Makefile ./config.status: line 2950: ./../../config-ml.in: No such file or directory Best regards, - Marcel Index: libiberty/ChangeLog === --- libiberty/ChangeLog (revision 235962) +++ libiberty/ChangeLog (working copy) @@ -1,3 +1,14 @@ +2016-05-06 Marcel Böhme + + PR c++/68159 + * cp-demangle.c: Allocate arrays of user-defined size on the heap, + not on the stack. Do not include . + (CP_DYNAMIC_ARRAYS): Remove definition. + (cplus_demangle_print_callback): Allocate memory for two arrays on + the heap. Free memory before return / exit. + (d_demangle_callback): Likewise. + (is_ctor_or_dtor): Likewise. + 2016-05-02 Marcel Böhme PR c++/70498 Index: libiberty/cp-demangle.c === --- libiberty/cp-demangle.c (revision 235962) +++ libiberty/cp-demangle.c (working copy) @@ -116,18 +116,6 @@ #include #endif -#ifdef HAVE_ALLOCA_H -# include -#else -# ifndef alloca -# ifdef __GNUC__ -# define alloca __builtin_alloca -# else -extern char *alloca (); -# endif /* __GNUC__ */ -# endif /* alloca */ -#endif /* HAVE_ALLOCA_H */ - #ifdef HAVE_LIMITS_H #include #endif @@ -186,20 +174,6 @@ static void d_init_info (const char *, int, size_t #define CP_STATIC_IF_GLIBCPP_V3 #endif /* ! defined(IN_GLIBCPP_V3) */ -/* See if the compiler supports dynamic arrays. */ - -#ifdef __GNUC__ -#define CP_DYNAMIC_ARRAYS -#else -#ifdef __STDC__ -#ifdef __STDC_VERSION__ -#if __STDC_VERSION__ >= 199901L -#define CP_DYNAMIC_ARRAYS -#endif /* __STDC__VERSION >= 199901L */ -#endif /* defined (__STDC_VERSION__) */ -#endif /* defined (__STDC__) */ -#endif /* ! defined (__GNUC__) */ - /* We avoid pulling in the ctype tables, to prevent pulling in additional unresolved symbols when this code is used in a library. FIXME: Is this really a valid reason? This comes from the original @@ -4126,25 +4100,26 @@ cplus_demangle_print_callback (int options, d_print_init (&dpi, callback, opaque, dc); - { -#ifdef CP_DYNAMIC_ARRAYS -__extension__ struct d_saved_scope scopes[dpi.num_saved_scopes]; -__extension__ struct d_print_template temps[dpi.num_copy_templates]; + dpi.copy_templates += (struct d_print_template *) malloc (((size_t) dpi.num_copy_templates) + * sizeof (*dpi.copy_templates)); + dpi.saved_scopes += (struct d_saved_scope *) malloc (((size_t) dpi.num_saved_scopes) + * sizeof (*dpi.saved_scopes)); + + if (! dpi.copy_templates || ! dpi.saved_scopes) +{ + d_print_error (&dpi); + return 0; +} -dp
Re: Fix for PR68159 in Libiberty Demangler (6)
Hi Jakub, The patch that is attached now is bootstrapped and regression tested on x86_64-pc-linux-gnu. > > This file is used not just in the various tools like binutils or gdb, but > also in libstdc++, where it used e.g. in the std::terminate handler, > which I think can't just xmalloc_failed, that function can be called already > in out of memory situation, where heap allocation is not possible. Earlier, I was working on libiberty/cplus-dem.c where xmalloc was explicitly available. So, I assumed it would be in libiberty/cp-demangle.c as well. > Why INT_MAX? > I'd have thought that the allocation size is computed in size_t and > thus it should be SIZE_MAX, (~(size_t) 0) or similar? In two separate patches (the first in cplus-dem.c and the second in cp-demangle.c) it was decided that we should import limit.h and otherwise define INT_MAX, then check against INT_MAX. However, I removed the overflow check since it is not clear what the behaviour should be when the integer actually overflows. Apparently, it can’t abort. Still, this remains an unresolved security concern if actually inputs can actually be generated that result in overflow. I also fixed some indentation issues caused by the removal of in-the-middle-of-the-method variable declarations. - { -#ifdef CP_DYNAMIC_ARRAYS -__extension__ struct d_saved_scope scopes[dpi.num_saved_scopes]; -__extension__ struct d_print_template temps[dpi.num_copy_templates]; Let me know if there are more concerns. There might be some more formatting issues lingering. Best regards, - Marcel Index: ChangeLog === --- ChangeLog (revision 235941) +++ ChangeLog (working copy) @@ -1,3 +1,14 @@ +2016-05-06 Marcel Böhme + + PR c++/68159 + * cp-demangle.c: Allocate arrays of user-defined size on the heap, + not on the stack. + (CP_DYNAMIC_ARRAYS): Remove redundant definition. + (cplus_demangle_print_callback): Allocate memory for two arrays on + the heap. Free memory before return / exit. + (d_demangle_callback): Likewise. + (is_ctor_or_dtor): Likewise. + 2016-05-02 Marcel Böhme PR c++/70498 Index: cp-demangle.c === --- cp-demangle.c (revision 235941) +++ cp-demangle.c (working copy) @@ -186,20 +186,6 @@ static void d_init_info (const char *, int, size_t #define CP_STATIC_IF_GLIBCPP_V3 #endif /* ! defined(IN_GLIBCPP_V3) */ -/* See if the compiler supports dynamic arrays. */ - -#ifdef __GNUC__ -#define CP_DYNAMIC_ARRAYS -#else -#ifdef __STDC__ -#ifdef __STDC_VERSION__ -#if __STDC_VERSION__ >= 199901L -#define CP_DYNAMIC_ARRAYS -#endif /* __STDC__VERSION >= 199901L */ -#endif /* defined (__STDC_VERSION__) */ -#endif /* defined (__STDC__) */ -#endif /* ! defined (__GNUC__) */ - /* We avoid pulling in the ctype tables, to prevent pulling in additional unresolved symbols when this code is used in a library. FIXME: Is this really a valid reason? This comes from the original @@ -4125,26 +4111,20 @@ cplus_demangle_print_callback (int options, struct d_print_info dpi; d_print_init (&dpi, callback, opaque, dc); + + dpi.copy_templates = (struct d_print_template *) + malloc (dpi.num_copy_templates * sizeof (*dpi.copy_templates)); - { -#ifdef CP_DYNAMIC_ARRAYS -__extension__ struct d_saved_scope scopes[dpi.num_saved_scopes]; -__extension__ struct d_print_template temps[dpi.num_copy_templates]; + dpi.saved_scopes = (struct d_saved_scope *) + malloc (dpi.num_saved_scopes * sizeof (*dpi.saved_scopes)); -dpi.saved_scopes = scopes; -dpi.copy_templates = temps; -#else -dpi.saved_scopes = alloca (dpi.num_saved_scopes - * sizeof (*dpi.saved_scopes)); -dpi.copy_templates = alloca (dpi.num_copy_templates -* sizeof (*dpi.copy_templates)); -#endif + d_print_comp (&dpi, options, dc); -d_print_comp (&dpi, options, dc); - } - d_print_flush (&dpi); + free(dpi.copy_templates); + free(dpi.saved_scopes); + return ! d_print_saw_error (&dpi); } @@ -5945,57 +5925,54 @@ d_demangle_callback (const char *mangled, int opti cplus_demangle_init_info (mangled, options, strlen (mangled), &di); - { -#ifdef CP_DYNAMIC_ARRAYS -__extension__ struct demangle_component comps[di.num_comps]; -__extension__ struct demangle_component *subs[di.num_subs]; + di.comps = (struct demangle_component *) malloc (di.num_comps + * sizeof (*di.comps)); -di.comps = comps; -di.subs = subs; -#else -di.comps = alloca (di.num_comps * sizeof (*di.comps)); -di.subs = alloca (di.num_subs * sizeof (*di.subs)); -#endif + di.subs = (struct demangle_component **) malloc (di.num_subs +
Fix for PR68159 in Libiberty Demangler (6)
Hi, This patches fixes * the stack overflow reported in PR68159 in cplus_demangle_print_callback, * a potential stack overflow in d_demangle_callback * a potential stack overflow in is_ctor_or_dtor, and * six potential buffer overflows (initialise less memory than needed due to integer overflow). The stack overflow reported in PR68159 occurs due to assigning an array too much memory from the stack (447kb). Similar stack overflows might occur in the remaining five dynamically allocated arrays in this and the other two functions. Since the array size is controlled from the mangled string, we better safeguard from integer overflows and thus buffer overflows for these six arrays. The patch allocates memory from the heap (xmalloc) instead of from the stack (dynamic arrays, alloca), checks for integer overflows, and frees the allocated memory before function return / abort. Best regards, - Marcel Index: ChangeLog === --- ChangeLog (revision 235941) +++ ChangeLog (working copy) @@ -1,3 +1,14 @@ +2016-05-06 Marcel Böhme + + PR c++/68159 + * cp-demangle.c: Check for overflow and allocate arrays of user-defined + size on the heap, not on the stack. + (CP_DYNAMIC_ARRAYS): Remove redundant definition. + (cplus_demangle_print_callback): Check for overflow. Allocate memory + for two arrays on the heap. Free memory before return / exit. + (d_demangle_callback): Likewise. + (is_ctor_or_dtor): Likewise. + 2016-05-02 Marcel Böhme PR c++/70498 Index: cp-demangle.c === --- cp-demangle.c (revision 235941) +++ cp-demangle.c (working copy) @@ -186,20 +186,6 @@ static void d_init_info (const char *, int, size_t #define CP_STATIC_IF_GLIBCPP_V3 #endif /* ! defined(IN_GLIBCPP_V3) */ -/* See if the compiler supports dynamic arrays. */ - -#ifdef __GNUC__ -#define CP_DYNAMIC_ARRAYS -#else -#ifdef __STDC__ -#ifdef __STDC_VERSION__ -#if __STDC_VERSION__ >= 199901L -#define CP_DYNAMIC_ARRAYS -#endif /* __STDC__VERSION >= 199901L */ -#endif /* defined (__STDC_VERSION__) */ -#endif /* defined (__STDC__) */ -#endif /* ! defined (__GNUC__) */ - /* We avoid pulling in the ctype tables, to prevent pulling in additional unresolved symbols when this code is used in a library. FIXME: Is this really a valid reason? This comes from the original @@ -4125,26 +4111,26 @@ cplus_demangle_print_callback (int options, struct d_print_info dpi; d_print_init (&dpi, callback, opaque, dc); + + if (dpi.num_copy_templates > INT_MAX / (int) sizeof (*dpi.copy_templates)) +xmalloc_failed(INT_MAX); + dpi.copy_templates = + (struct d_print_template *) xmalloc (dpi.num_copy_templates + * sizeof (*dpi.copy_templates)); - { -#ifdef CP_DYNAMIC_ARRAYS -__extension__ struct d_saved_scope scopes[dpi.num_saved_scopes]; -__extension__ struct d_print_template temps[dpi.num_copy_templates]; + if (dpi.num_saved_scopes > INT_MAX / (int) sizeof (*dpi.saved_scopes)) +xmalloc_failed(INT_MAX); + dpi.saved_scopes = + (struct d_saved_scope *) xmalloc (dpi.num_saved_scopes + * sizeof (*dpi.saved_scopes)); -dpi.saved_scopes = scopes; -dpi.copy_templates = temps; -#else -dpi.saved_scopes = alloca (dpi.num_saved_scopes - * sizeof (*dpi.saved_scopes)); -dpi.copy_templates = alloca (dpi.num_copy_templates -* sizeof (*dpi.copy_templates)); -#endif + d_print_comp (&dpi, options, dc); -d_print_comp (&dpi, options, dc); - } - d_print_flush (&dpi); + free(dpi.copy_templates); + free(dpi.saved_scopes); + return ! d_print_saw_error (&dpi); } @@ -5945,18 +5931,17 @@ d_demangle_callback (const char *mangled, int opti cplus_demangle_init_info (mangled, options, strlen (mangled), &di); - { -#ifdef CP_DYNAMIC_ARRAYS -__extension__ struct demangle_component comps[di.num_comps]; -__extension__ struct demangle_component *subs[di.num_subs]; + if (di.num_comps > INT_MAX / (int) sizeof (*di.comps)) +xmalloc_failed(INT_MAX); + di.comps = (struct demangle_component *) xmalloc (di.num_comps + * sizeof (*di.comps)); -di.comps = comps; -di.subs = subs; -#else -di.comps = alloca (di.num_comps * sizeof (*di.comps)); -di.subs = alloca (di.num_subs * sizeof (*di.subs)); -#endif + if (di.num_subs > INT_MAX / (int) sizeof (*di.subs)) +xmalloc_failed(INT_MAX); + di.subs = (struct demangle_component **) xmalloc (di.num_subs + * sizeof (*di.subs)); + { switch (type) { case DCT_TYPE: @@ -5977,6 +5962,8 @@ d_demangle_callback (const char *mangled, int opti
Fix for PR70926 in Libiberty Demangler (5)
Hi, This fixes four access violations (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70926). Two of these first read the value of a length variable len from the mangled string, then strncpy len characters from the mangled string; more than necessary. The other two read the value of an array index n from the mangled string, which can be negative due to an overflow. Bootstrapped and regression tested on x86_64-pc-linux-gnu. Test cases added to libiberty/testsuite/demangler-expected and checked PR70926 is resolved. Best regards, - Marcel Index: libiberty/ChangeLog === --- libiberty/ChangeLog (revision 235801) +++ libiberty/ChangeLog (working copy) @@ -1,3 +1,12 @@ +2016-05-03 Marcel Böhme + + PR c++/70926 + * cplus-dem.c: Handle large values and overflow when demangling + length variables. + (demangle_template_value_parm): Read only until end of mangled string. + (do_hpacc_template_literal): Likewise. + (do_type): Handle overflow when demangling array indices. + 2016-05-02 Marcel Böhme PR c++/70498 Index: libiberty/cplus-dem.c === --- libiberty/cplus-dem.c (revision 235801) +++ libiberty/cplus-dem.c (working copy) @@ -2051,7 +2051,8 @@ demangle_template_value_parm (struct work_stuff *w else { int symbol_len = consume_count (mangled); - if (symbol_len == -1) + if (symbol_len == -1 + || symbol_len > (long) strlen (*mangled)) return -1; if (symbol_len == 0) string_appendn (s, "0", 1); @@ -3611,7 +3612,7 @@ do_type (struct work_stuff *work, const char **man /* A back reference to a previously seen type */ case 'T': (*mangled)++; - if (!get_count (mangled, &n) || n >= work -> ntypes) + if (!get_count (mangled, &n) || n < 0 || n >= work -> ntypes) { success = 0; } @@ -3789,7 +3790,7 @@ do_type (struct work_stuff *work, const char **man /* A back reference to a previously seen squangled type */ case 'B': (*mangled)++; - if (!get_count (mangled, &n) || n >= work -> numb) + if (!get_count (mangled, &n) || n < 0 || n >= work -> numb) success = 0; else string_append (result, work->btypevec[n]); @@ -4130,7 +4131,8 @@ do_hpacc_template_literal (struct work_stuff *work literal_len = consume_count (mangled); - if (literal_len <= 0) + if (literal_len <= 0 + || literal_len > (long) strlen (*mangled)) return 0; /* Literal parameters are names of arrays, functions, etc. and the Index: libiberty/testsuite/demangle-expected === --- libiberty/testsuite/demangle-expected (revision 235801) +++ libiberty/testsuite/demangle-expected (working copy) @@ -4441,3 +4441,16 @@ __vt_900cafebabe _Z808 _Z808 +# +# Tests write access violation PR70926 + +0__Ot2m02R5T50 +0__Ot2m02R5T50 +# + +0__GT500_ +0__GT500_ +# + +__t2m05B50_ +__t2m05B50_
Fix for PR70909 in Libiberty Demangler (4)
Hi, This fixes several stack overflows due to infinite recursion in d_print_comp (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70909). The method d_print_comp in cp-demangle.c recursively constructs the d_print_info dpi from the demangle_component dc. The method d_print_comp_inner traverses dc as a graph. Now, dc can be a graph with cycles leading to infinite recursion in several distinct cases. The patch uses the component stack to find whether the current node dc has itself as ancestor more than once. Bootstrapped and regression tested on x86_64-pc-linux-gnu. Test cases added to libiberty/testsuite/demangler-expected and checked PR70909 and related stack overflows are resolved. Best regards, - Marcel Index: ChangeLog === --- ChangeLog (revision 235760) +++ ChangeLog (working copy) @@ -1,3 +1,19 @@ +2016-05-02 Marcel Böhme + + PR c++/70909 + PR c++/61460 + PR c++/68700 + PR c++/67738 + PR c++/68383 + PR c++/70517 + PR c++/61805 + PR c++/62279 + PR c++/67264 + * cp-demangle.c: Prevent infinite recursion when traversing cyclic + demangle component. + (d_print_comp): Return when demangle component has itself as ancistor + more than once. + 2016-04-30 Oleg Endo * configure: Remove SH5 support. Index: cp-demangle.c === --- cp-demangle.c (revision 235760) +++ cp-demangle.c (working copy) @@ -5436,6 +5436,24 @@ d_print_comp (struct d_print_info *dpi, int option { struct d_component_stack self; + self.parent = dpi->component_stack; + + while (self.parent) +{ + self.dc = self.parent->dc; + self.parent = self.parent->parent; + if (dc != NULL && self.dc == dc) + { + while (self.parent) + { + self.dc = self.parent->dc; + self.parent = self.parent->parent; + if (self.dc == dc) + return; + } + } +} + self.dc = dc; self.parent = dpi->component_stack; dpi->component_stack = &self; Index: testsuite/demangle-expected === --- testsuite/demangle-expected (revision 235760) +++ testsuite/demangle-expected (working copy) @@ -4431,3 +4431,69 @@ _Q.__0 _Q10-__9cafebabe. cafebabe.::-(void) +# +# Test demangler crash PR62279 + +_ZN5Utils9transformIPN15ProjectExplorer13BuildStepListEZNKS1_18BuildConfiguration14knownStepListsEvEUlS3_E_EE5QListIDTclfp0_cvT__RKS6_IS7_ET0_ +QList Utils::transform(ProjectExplorer::BuildConfiguration::knownStepLists() const::{lambda(ProjectExplorer::BuildStepList*)#1} const&, ProjectExplorer::BuildConfiguration::knownStepLists() const::{lambda(ProjectExplorer::BuildStepList*)#1}) +# + +_ZSt7forwardIKSaINSt6thread5_ImplISt12_Bind_simpleIFZN6WIM_DL5Utils9AsyncTaskC4IMNS3_8Hardware12FpgaWatchdogEKFvvEIPS8_EEEibOT_DpOT0_EUlvE_vEESD_RNSt16remove_referenceISC_E4typeE +std::allocator(int, bool, void (WIM_DL::Hardware::FpgaWatchdog::*&&)() const, WIM_DL::Hardware::FpgaWatchdog*&&)::{lambda()#1} ()> > > const&& std::forward(int, bool, std::allocator(int, bool, void (WIM_DL::Hardware::FpgaWatchdog::*&&)() const, WIM_DL::Hardware::FpgaWatchdog*&&)::{lambda()#1} ()> > > const&&, WIM_DL::Hardware::FpgaWatchdog*&&)::{lambda()#1} ()> > > const>(std::remove_reference(int, bool, void (WIM_DL::Hardware::FpgaWatchdog::*&&)() const, WIM_DL::Hardware::FpgaWatchdog*&&)::{lambda()#1} ()> > > const>::type&) +# +# Test demangler crash PR61805 + +_ZNK5niven5ColorIfLi4EEdvIfEENSt9enable_ifIXsrSt13is_arithmeticIT_E5valueEKNS0_IDTmlcvS5__Ecvf_EELi44typeES5_ +std::enable_if::value, niven::Color const>::type niven::Color::operator/(float) const +# +# Test recursion PR70517 + +_ZSt4moveIRZN11tconcurrent6futureIvE4thenIZ5awaitIS2_EDaOT_EUlRKS6_E_EENS1_INSt5decayIDTclfp_defpTEEE4typeEEES7_EUlvE_EONSt16remove_referenceIS6_E4typeES7_ +std::remove_reference::type> tconcurrent::future::then >(tconcurrent::future&&)::{lambda(tconcurrent::future::type> tconcurrent::future::then >()::{lambda( const&)#1}>( const)::{lambda()#1}& const&)#1}>(auto await >()::{lambda( const&)#1}&& const)::{lambda()#1}&>::type&& std::move::type> tconcurrent::future::then >(tconcurrent::future::type> tconcurrent::future::then >(tconcurrent::future&&)::{lambda(& const&)#1}>(auto await >()::{lambda( const&)#1}&& const)::{lambda()#1}&)::{lambda(tconcurrent::future::type> tconcurrent::future::then >(tconcurrent::future&&)::{lambda(& const&)#1}>(auto await >()::{lambda(&)#1}&&
Re: Fix for PR70498 in Libiberty Demangler
> On 28 Apr 2016, at 12:28 AM, Bernd Schmidt wrote: > > Please attach it (text/plain) instead. Done. Index: libiberty/ChangeLog === --- libiberty/ChangeLog (revision 235691) +++ libiberty/ChangeLog (working copy) @@ -1,3 +1,22 @@ +2016-04-16 Marcel Böhme + + PR c++/70498 + * cp-demangle.c: Parse numbers as integer instead of long to avoid + overflow after sanity checks. Include if available. + (INT_MAX): Define if necessary. + (d_make_template_param): Takes integer argument instead of long. + (d_make_function_param): Likewise. + (d_append_num): Likewise. + (d_identifier): Likewise. + (d_number): Parse as and return integer. + (d_compact_number): Handle overflow. + (d_source_name): Change variable type to integer for parsed number. + (d_java_resource): Likewise. + (d_special_name): Likewise. + (d_discriminator): Likewise. + (d_unnamed_type): Likewise. + * testsuite/demangle-expected: Add regression test cases. + 2016-04-30 Oleg Endo * configure: Remove SH5 support. Index: libiberty/cp-demangle.c === --- libiberty/cp-demangle.c (revision 235691) +++ libiberty/cp-demangle.c (working copy) @@ -128,6 +128,13 @@ # endif /* alloca */ #endif /* HAVE_ALLOCA_H */ +#ifdef HAVE_LIMITS_H +#include +#endif +#ifndef INT_MAX +# define INT_MAX (int)(((unsigned int) ~0) >> 1) /* 0x7FFF */ +#endif + #include "ansidecl.h" #include "libiberty.h" #include "demangle.h" @@ -398,7 +405,7 @@ struct demangle_component *); static struct demangle_component * -d_make_template_param (struct d_info *, long); +d_make_template_param (struct d_info *, int); static struct demangle_component * d_make_sub (struct d_info *, const char *, int); @@ -421,9 +428,9 @@ static struct demangle_component *d_source_name (struct d_info *); -static long d_number (struct d_info *); +static int d_number (struct d_info *); -static struct demangle_component *d_identifier (struct d_info *, long); +static struct demangle_component *d_identifier (struct d_info *, int); static struct demangle_component *d_operator_name (struct d_info *); @@ -1119,7 +1126,7 @@ /* Add a new template parameter. */ static struct demangle_component * -d_make_template_param (struct d_info *di, long i) +d_make_template_param (struct d_info *di, int i) { struct demangle_component *p; @@ -1135,7 +1142,7 @@ /* Add a new function parameter. */ static struct demangle_component * -d_make_function_param (struct d_info *di, long i) +d_make_function_param (struct d_info *di, int i) { struct demangle_component *p; @@ -1620,7 +1627,7 @@ static struct demangle_component * d_source_name (struct d_info *di) { - long len; + int len; struct demangle_component *ret; len = d_number (di); @@ -1633,12 +1640,12 @@ /* number ::= [n] <(non-negative decimal integer)> */ -static long +static int d_number (struct d_info *di) { int negative; char peek; - long ret; + int ret; negative = 0; peek = d_peek_char (di); @@ -1681,7 +1688,7 @@ /* identifier ::= <(unqualified source code identifier)> */ static struct demangle_component * -d_identifier (struct d_info *di, long len) +d_identifier (struct d_info *di, int len) { const char *name; @@ -1702,7 +1709,7 @@ /* Look for something which looks like a gcc encoding of an anonymous namespace, and replace it with a more user friendly name. */ - if (len >= (long) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2 + if (len >= (int) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2 && memcmp (name, ANONYMOUS_NAMESPACE_PREFIX, ANONYMOUS_NAMESPACE_PREFIX_LEN) == 0) { @@ -1870,7 +1877,7 @@ { struct demangle_component *p = NULL; struct demangle_component *next = NULL; - long len, i; + int len, i; char c; const char *str; @@ -2012,7 +2019,7 @@ case 'C': { struct demangle_component *derived_type; - long offset; + int offset; struct demangle_component *base_type; derived_type = cplus_demangle_type (di); @@ -2946,10 +2953,10 @@ /* _ */ -static long +static int d_compact_number (struct d_info *di) { - long num; + int num; if (d_peek_char (di) == '_') num = 0; else if (d_peek_char (di) == 'n') @@ -2957,7 +2964,7 @@ else num = d_number (di) + 1; - if (! d_check_char (di, '_')) + if (num < 0 || ! d_check_char (di, '_')) return -1; return num; } @@ -2969,7 +2976,7 @@ static struct demangle_component * d_template_param (struct d_info *di) { - long param; + int param; if (! d_check_char (di, 'T')) ret
Re: Fix for PR70498 in Libiberty Demangler
Hi Bernd, >>> index = d_compact_number (di) + 1; if (index == 0) return NULL; >>> >>> which probably ought to have the same kind of check (I'll note that >>> at this point we've accumulated two "+1"s, I'll assume that's what >>> we want). >> Yes. There should be an overflow check here. > > Could you update the patch for that? Sure. The updated patch, including Changelog and more test cases. Regression tested. Best regards, - Marcel Index: ChangeLog === --- ChangeLog (revision 235032) +++ ChangeLog (working copy) @@ -1,3 +1,22 @@ +2016-04-16 Marcel Böhme + + PR c++/70498 + * cp-demangle.c: Parse/handle numbers as integer instead of long. + Include if available. + (INT_MAX): Define if necessary. + (d_make_template_param): Takes integer argument instead of long. + (d_make_function_param): Likewise. + (d_append_num): Likewise. + (d_identifier): Likewise. + (d_number): Parse as and return as integer. + (d_compact_number): Handle overflow. + (d_expression_1): Likewise. + (d_source_name): Change variable type to integer for parsed number. + (d_java_resource): Likewise. + (d_special_name): Likewise. + (d_discriminator): Likewise. + (d_unnamed_type): Likewise. + * testsuite/demangle-expected: Add regression test cases. + 2016-04-08 Marcel Böhme PR c++/69687 Index: cp-demangle.c === --- cp-demangle.c (revision 235032) +++ cp-demangle.c (working copy) @@ -128,6 +128,13 @@ extern char *alloca (); # endif /* alloca */ #endif /* HAVE_ALLOCA_H */ +#ifdef HAVE_LIMITS_H +#include +#endif +#ifndef INT_MAX +# define INT_MAX (int)(((unsigned int) ~0) >> 1) /* 0x7FFF */ +#endif + #include "ansidecl.h" #include "libiberty.h" #include "demangle.h" @@ -398,7 +405,7 @@ d_make_dtor (struct d_info *, enum gnu_v struct demangle_component *); static struct demangle_component * -d_make_template_param (struct d_info *, long); +d_make_template_param (struct d_info *, int); static struct demangle_component * d_make_sub (struct d_info *, const char *, int); @@ -421,9 +428,9 @@ static struct demangle_component *d_unqu static struct demangle_component *d_source_name (struct d_info *); -static long d_number (struct d_info *); +static int d_number (struct d_info *); -static struct demangle_component *d_identifier (struct d_info *, long); +static struct demangle_component *d_identifier (struct d_info *, int); static struct demangle_component *d_operator_name (struct d_info *); @@ -1119,7 +1126,7 @@ d_make_dtor (struct d_info *di, enum gnu /* Add a new template parameter. */ static struct demangle_component * -d_make_template_param (struct d_info *di, long i) +d_make_template_param (struct d_info *di, int i) { struct demangle_component *p; @@ -1135,7 +1142,7 @@ d_make_template_param (struct d_info *di /* Add a new function parameter. */ static struct demangle_component * -d_make_function_param (struct d_info *di, long i) +d_make_function_param (struct d_info *di, int i) { struct demangle_component *p; @@ -1620,7 +1627,7 @@ d_unqualified_name (struct d_info *di) static struct demangle_component * d_source_name (struct d_info *di) { - long len; + int len; struct demangle_component *ret; len = d_number (di); @@ -1633,12 +1640,12 @@ d_source_name (struct d_info *di) /* number ::= [n] <(non-negative decimal integer)> */ -static long +static int d_number (struct d_info *di) { int negative; char peek; - long ret; + int ret; negative = 0; peek = d_peek_char (di); @@ -1681,7 +1688,7 @@ d_number_component (struct d_info *di) /* identifier ::= <(unqualified source code identifier)> */ static struct demangle_component * -d_identifier (struct d_info *di, long len) +d_identifier (struct d_info *di, int len) { const char *name; @@ -1702,7 +1709,7 @@ d_identifier (struct d_info *di, long le /* Look for something which looks like a gcc encoding of an anonymous namespace, and replace it with a more user friendly name. */ - if (len >= (long) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2 + if (len >= (int) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2 && memcmp (name, ANONYMOUS_NAMESPACE_PREFIX, ANONYMOUS_NAMESPACE_PREFIX_LEN) == 0) { @@ -1870,7 +1877,7 @@ d_java_resource (struct d_info *di) { struct demangle_component *p = NULL; struct demangle_component *next = NULL; - long len, i; + int len, i; char c; const char *str; @@ -2012,7 +2019,7 @@ d_special_name (struct d_info *di) case 'C': { struct demangle_component *derive
Re: Fix for PR70498 in Libiberty Demangler
Hi Bernd, > -static long > +static int > d_compact_number (struct d_info *di) > { > - long num; > + int num; >if (d_peek_char (di) == '_') > num = 0; >else if (d_peek_char (di) == 'n') > @@ -2957,7 +2957,7 @@ d_compact_number (struct d_info *di) >else > num = d_number (di) + 1; > > - if (! d_check_char (di, '_')) > + if (num < 0 || ! d_check_char (di, '_')) > return -1; >return num; > } > Shouldn't we check for overflows before performing the +1 addition (i.e. 0 <= > num < INT_MAX)? Ideally we'd also have a way to signal from d_number if we > had an overflow while parsing that number. Without an overflow signal, d_number will already be prone to return a negative number for supposedly non-negative numbers (those not preceded with ’n’). In that case an overflow check would be unnecessary in d_compact_number which is supposed to always return a positive number or a negative one (-1). If you decide in favour of an overflow signal, it must be handled by the call-sites. Not sure what the “default behaviour” should be then. Otherwise, we can simply assume that the call sites for d_number can handle negative numbers. Kindly advice. > > There's also this, in d_expression_1: > > index = d_compact_number (di) + 1; > if (index == 0) > return NULL; > > which probably ought to have the same kind of check (I'll note that at this > point we've accumulated two "+1"s, I'll assume that's what we want). Yes. There should be an overflow check here. Thanks! - Marcel
Re: Fix for PR70498 in Libiberty Demangler
> On 4 Apr 2016, at 9:24 PM, Bernd Schmidt wrote: > >> >> The patch now also accounts for overflows in d_compact_number which >> is supposed to return -1 in case of negative numbers. > > I take it this isn't for the normal 'n' case, but for instances where we > encounter overflows in d_number? Yes. Best regards, - Marcel
Re: Fix for PR70498 in Libiberty Demangler
> On 2 Apr 2016, at 1:44 AM, Bernd Schmidt wrote: > > On 04/01/2016 07:41 PM, Pedro Alves wrote: >> On 04/01/2016 11:21 AM, Marcel Böhme wrote: >>> static inline void >>> -d_append_num (struct d_print_info *dpi, long l) >>> +d_append_num (struct d_print_info *dpi, int l) >>> { >>>char buf[25]; >>>sprintf (buf,"%ld", l); >> >> Doesn't this warn about %ld format vs int type mismatch? > > Well spotted. Marcel, please correct this issue and check for other warnings. > Unless libiberty is somehow exempt from -Werror, this should have shown up in > a bootstrap. Bootstrapped and regression tested on x86_64-pc-linux-gnu. Test cases added + checked PR70498 is resolved. The patch now also accounts for overflows in d_compact_number which is supposed to return -1 in case of negative numbers. Thanks, - Marcel -- Index: libiberty/cp-demangle.c === --- libiberty/cp-demangle.c (revision 234663) +++ libiberty/cp-demangle.c (working copy) @@ -398,7 +398,7 @@ d_make_dtor (struct d_info *, enum gnu_v struct demangle_component *); static struct demangle_component * -d_make_template_param (struct d_info *, long); +d_make_template_param (struct d_info *, int); static struct demangle_component * d_make_sub (struct d_info *, const char *, int); @@ -421,9 +421,9 @@ static struct demangle_component *d_unqu static struct demangle_component *d_source_name (struct d_info *); -static long d_number (struct d_info *); +static int d_number (struct d_info *); -static struct demangle_component *d_identifier (struct d_info *, long); +static struct demangle_component *d_identifier (struct d_info *, int); static struct demangle_component *d_operator_name (struct d_info *); @@ -1119,7 +1119,7 @@ d_make_dtor (struct d_info *di, enum gnu /* Add a new template parameter. */ static struct demangle_component * -d_make_template_param (struct d_info *di, long i) +d_make_template_param (struct d_info *di, int i) { struct demangle_component *p; @@ -1135,7 +1135,7 @@ d_make_template_param (struct d_info *di /* Add a new function parameter. */ static struct demangle_component * -d_make_function_param (struct d_info *di, long i) +d_make_function_param (struct d_info *di, int i) { struct demangle_component *p; @@ -1620,7 +1620,7 @@ d_unqualified_name (struct d_info *di) static struct demangle_component * d_source_name (struct d_info *di) { - long len; + int len; struct demangle_component *ret; len = d_number (di); @@ -1633,12 +1633,12 @@ d_source_name (struct d_info *di) /* number ::= [n] <(non-negative decimal integer)> */ -static long +static int d_number (struct d_info *di) { int negative; char peek; - long ret; + int ret; negative = 0; peek = d_peek_char (di); @@ -1681,7 +1681,7 @@ d_number_component (struct d_info *di) /* identifier ::= <(unqualified source code identifier)> */ static struct demangle_component * -d_identifier (struct d_info *di, long len) +d_identifier (struct d_info *di, int len) { const char *name; @@ -1702,7 +1702,7 @@ d_identifier (struct d_info *di, long le /* Look for something which looks like a gcc encoding of an anonymous namespace, and replace it with a more user friendly name. */ - if (len >= (long) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2 + if (len >= (int) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2 && memcmp (name, ANONYMOUS_NAMESPACE_PREFIX, ANONYMOUS_NAMESPACE_PREFIX_LEN) == 0) { @@ -1870,7 +1870,7 @@ d_java_resource (struct d_info *di) { struct demangle_component *p = NULL; struct demangle_component *next = NULL; - long len, i; + int len, i; char c; const char *str; @@ -2012,7 +2012,7 @@ d_special_name (struct d_info *di) case 'C': { struct demangle_component *derived_type; - long offset; + int offset; struct demangle_component *base_type; derived_type = cplus_demangle_type (di); @@ -2946,10 +2946,10 @@ d_pointer_to_member_type (struct d_info /* _ */ -static long +static int d_compact_number (struct d_info *di) { - long num; + int num; if (d_peek_char (di) == '_') num = 0; else if (d_peek_char (di) == 'n') @@ -2957,7 +2957,7 @@ d_compact_number (struct d_info *di) else num = d_number (di) + 1; - if (! d_check_char (di, '_')) + if (num < 0 || ! d_check_char (di, '_')) return -1; return num; } @@ -2969,7 +2969,7 @@ d_compact_number (struct d_info *di) static struct demangle_component * d_template_param (struct d_info *di) { - long param; + int param; if (! d_check_char (di, 'T')) return NULL; @@ -3502,7 +3502,7 @@ d_local_name (struct d_i
Re: Proposed Patch for Bug 69687
> > Forgot about this issue, sorry. At least this needs guarding with #ifdef > HAVE_LIMITS_H, as in the other files in libiberty. Several of them also go to > trouble to define the macros if limits.h is missing; not sure how much of an > issue that is nowadays, but you might want to adapt something like the code > from strtol.c: > > #ifndef ULONG_MAX > #define ULONG_MAX ((unsigned long)(~0L)) /* 0x */ > #endif > > #ifndef LONG_MAX > #define LONG_MAX((long)(ULONG_MAX >> 1))/* 0x7FFF */ > #endif > > Mind trying that and doing a full test run as described in my other mail? Regression tested on x86_64-pc-linux-gnu (make check). Checked PR69687 is resolved (via binutils); even for our definition of INT_MAX. No test cases added since the test would take up to 2 minutes and end in xmalloc_failed even in the successful case. Thanks, - Marcel -- Index: libiberty/cplus-dem.c === --- libiberty/cplus-dem.c (revision 234663) +++ libiberty/cplus-dem.c (working copy) @@ -56,6 +56,13 @@ void * malloc (); void * realloc (); #endif +#ifdef HAVE_LIMITS_H +#include +#endif +#ifndef INT_MAX +# define INT_MAX (int)(((unsigned int) ~0) >> 1) /* 0x7FFF */ +#endif + #include #undef CURRENT_DEMANGLING_STYLE #define CURRENT_DEMANGLING_STYLE work->options @@ -4256,6 +4263,8 @@ remember_type (struct work_stuff *work, } else { + if (work -> typevec_size > INT_MAX / 2) + xmalloc_failed (INT_MAX); work -> typevec_size *= 2; work -> typevec = XRESIZEVEC (char *, work->typevec, work->typevec_size); @@ -4283,6 +4292,8 @@ remember_Ktype (struct work_stuff *work, } else { + if (work -> ksize > INT_MAX / 2) + xmalloc_failed (INT_MAX); work -> ksize *= 2; work -> ktypevec = XRESIZEVEC (char *, work->ktypevec, work->ksize); @@ -4312,6 +4323,8 @@ register_Btype (struct work_stuff *work) } else { + if (work -> bsize > INT_MAX / 2) + xmalloc_failed (INT_MAX); work -> bsize *= 2; work -> btypevec = XRESIZEVEC (char *, work->btypevec, work->bsize); @@ -4766,6 +4779,8 @@ string_need (string *s, int n) else if (s->e - s->p < n) { tem = s->p - s->b; + if (n > INT_MAX / 2 - tem) +xmalloc_failed (INT_MAX); n += tem; n *= 2; s->b = XRESIZEVEC (char, s->b, n);
Re: Fix for PR70498 in Libiberty Demangler
> Since d_identifier takes an int as length, d_identifier is called with a > negative length after the implicit cast: Sorry, d_make_name called from d_identifier in cp_demangle.c:1721 takes an int as length. Best regards - Marcel
Re: Fix for PR70498 in Libiberty Demangler
Hi Bernd, Thanks for the feedback! > Patches need to be bootstrapped and regression tested, and patch submissions > should include which target this was done on. > > Ideally you'd also want to include testcases along with your patches, > although I'm not entirely sure how we can arrange for this type of problem to > be tested. Regression tested on x86_64-pc-linux-gnu (make check). Test cases added to libiberty/testsuite/demangler-expected and checked PR70498 is resolved. Not sure how to bootstrap the patch. > Lastly, for this specific patch, I have trouble seeing how it fixes anything. > I'd need a more detailed explanation of how the problem happens in the first > place. In the patched version, the values wrap around when they are parsed in d_number. Since the mangled string may contain negative numbers, there is usually proper handling of negative numbers in the clients of d_number. Without the patch a value can become negative when cast from long to int *after* these checks. For instance, in d_source_name the length of the identifier is parsed as long from the mangled string and checked whether it is negative. Since d_identifier takes an int as length, d_identifier is called with a negative length after the implicit cast: static struct demangle_component * d_source_name (struct d_info *di) { int len; struct demangle_component *ret; len = d_number (di); if (len <= 0) return NULL; ret = d_identifier (di, len); di->last_name = ret; return ret; } -- Index: libiberty/cp-demangle.c === --- libiberty/cp-demangle.c (revision 234663) +++ libiberty/cp-demangle.c (working copy) @@ -398,7 +398,7 @@ struct demangle_component *); static struct demangle_component * -d_make_template_param (struct d_info *, long); +d_make_template_param (struct d_info *, int); static struct demangle_component * d_make_sub (struct d_info *, const char *, int); @@ -421,9 +421,9 @@ static struct demangle_component *d_source_name (struct d_info *); -static long d_number (struct d_info *); +static int d_number (struct d_info *); -static struct demangle_component *d_identifier (struct d_info *, long); +static struct demangle_component *d_identifier (struct d_info *, int); static struct demangle_component *d_operator_name (struct d_info *); @@ -1119,7 +1119,7 @@ /* Add a new template parameter. */ static struct demangle_component * -d_make_template_param (struct d_info *di, long i) +d_make_template_param (struct d_info *di, int i) { struct demangle_component *p; @@ -1135,7 +1135,7 @@ /* Add a new function parameter. */ static struct demangle_component * -d_make_function_param (struct d_info *di, long i) +d_make_function_param (struct d_info *di, int i) { struct demangle_component *p; @@ -1620,7 +1620,7 @@ static struct demangle_component * d_source_name (struct d_info *di) { - long len; + int len; struct demangle_component *ret; len = d_number (di); @@ -1633,12 +1633,12 @@ /* number ::= [n] <(non-negative decimal integer)> */ -static long +static int d_number (struct d_info *di) { int negative; char peek; - long ret; + int ret; negative = 0; peek = d_peek_char (di); @@ -1681,7 +1681,7 @@ /* identifier ::= <(unqualified source code identifier)> */ static struct demangle_component * -d_identifier (struct d_info *di, long len) +d_identifier (struct d_info *di, int len) { const char *name; @@ -1702,7 +1702,7 @@ /* Look for something which looks like a gcc encoding of an anonymous namespace, and replace it with a more user friendly name. */ - if (len >= (long) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2 + if (len >= ANONYMOUS_NAMESPACE_PREFIX_LEN + 2 && memcmp (name, ANONYMOUS_NAMESPACE_PREFIX, ANONYMOUS_NAMESPACE_PREFIX_LEN) == 0) { @@ -1870,7 +1870,7 @@ { struct demangle_component *p = NULL; struct demangle_component *next = NULL; - long len, i; + int len, i; char c; const char *str; @@ -2012,7 +2012,7 @@ case 'C': { struct demangle_component *derived_type; - long offset; + int offset; struct demangle_component *base_type; derived_type = cplus_demangle_type (di); @@ -2946,10 +2946,10 @@ /* _ */ -static long +static int d_compact_number (struct d_info *di) { - long num; + int num; if (d_peek_char (di) == '_') num = 0; else if (d_peek_char (di) == 'n') @@ -2969,7 +2969,7 @@ static struct demangle_component * d_template_param (struct d_info *di) { - long param; + int param; if (! d_check_char (di, 'T')) return NULL; @@ -3502,7 +3502,7 @@ static int d_discriminator (struct d_info *di) { - long discrim; + int discrim; if (d_peek_char (di) != '_') return 1; @@ -3558,7 +3558,7 @@ d_unnamed_type (struct d_info *di) { struct demangle_component *ret; - long num; + int num; if (! d_check_char (di, 'U')) return NULL; @@ -4086,7 +4086,7 @@
Fix for PR70498 in Libiberty Demangler
Hi, This fixes the write access violation detailed in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70498 (and a few other unreported cases). Sometimes length-variables for strings and arrays are of type long other times of type int. Since cp-demangle.h exports structs and methods with length-variables of type int, this has been made consistent in cp-demangle.c. Best regards, - Marcel Index: libiberty/cp-demangle.c === --- libiberty/cp-demangle.c (revision 234663) +++ libiberty/cp-demangle.c (working copy) @@ -398,7 +398,7 @@ struct demangle_component *); static struct demangle_component * -d_make_template_param (struct d_info *, long); +d_make_template_param (struct d_info *, int); static struct demangle_component * d_make_sub (struct d_info *, const char *, int); @@ -421,9 +421,9 @@ static struct demangle_component *d_source_name (struct d_info *); -static long d_number (struct d_info *); +static int d_number (struct d_info *); -static struct demangle_component *d_identifier (struct d_info *, long); +static struct demangle_component *d_identifier (struct d_info *, int); static struct demangle_component *d_operator_name (struct d_info *); @@ -1119,7 +1119,7 @@ /* Add a new template parameter. */ static struct demangle_component * -d_make_template_param (struct d_info *di, long i) +d_make_template_param (struct d_info *di, int i) { struct demangle_component *p; @@ -1135,7 +1135,7 @@ /* Add a new function parameter. */ static struct demangle_component * -d_make_function_param (struct d_info *di, long i) +d_make_function_param (struct d_info *di, int i) { struct demangle_component *p; @@ -1620,7 +1620,7 @@ static struct demangle_component * d_source_name (struct d_info *di) { - long len; + int len; struct demangle_component *ret; len = d_number (di); @@ -1633,12 +1633,12 @@ /* number ::= [n] <(non-negative decimal integer)> */ -static long +static int d_number (struct d_info *di) { int negative; char peek; - long ret; + int ret; negative = 0; peek = d_peek_char (di); @@ -1681,7 +1681,7 @@ /* identifier ::= <(unqualified source code identifier)> */ static struct demangle_component * -d_identifier (struct d_info *di, long len) +d_identifier (struct d_info *di, int len) { const char *name; @@ -1702,7 +1702,7 @@ /* Look for something which looks like a gcc encoding of an anonymous namespace, and replace it with a more user friendly name. */ - if (len >= (long) ANONYMOUS_NAMESPACE_PREFIX_LEN + 2 + if (len >= ANONYMOUS_NAMESPACE_PREFIX_LEN + 2 && memcmp (name, ANONYMOUS_NAMESPACE_PREFIX, ANONYMOUS_NAMESPACE_PREFIX_LEN) == 0) { @@ -1870,7 +1870,7 @@ { struct demangle_component *p = NULL; struct demangle_component *next = NULL; - long len, i; + int len, i; char c; const char *str; @@ -2012,7 +2012,7 @@ case 'C': { struct demangle_component *derived_type; - long offset; + int offset; struct demangle_component *base_type; derived_type = cplus_demangle_type (di); @@ -2946,10 +2946,10 @@ /* _ */ -static long +static int d_compact_number (struct d_info *di) { - long num; + int num; if (d_peek_char (di) == '_') num = 0; else if (d_peek_char (di) == 'n') @@ -2969,7 +2969,7 @@ static struct demangle_component * d_template_param (struct d_info *di) { - long param; + int param; if (! d_check_char (di, 'T')) return NULL; @@ -3502,7 +3502,7 @@ static int d_discriminator (struct d_info *di) { - long discrim; + int discrim; if (d_peek_char (di) != '_') return 1; @@ -3558,7 +3558,7 @@ d_unnamed_type (struct d_info *di) { struct demangle_component *ret; - long num; + int num; if (! d_check_char (di, 'U')) return NULL; @@ -4086,7 +4086,7 @@ } static inline void -d_append_num (struct d_print_info *dpi, long l) +d_append_num (struct d_print_info *dpi, int l) { char buf[25]; sprintf (buf,"%ld", l);
Fix for PR70492
Hi, This fixes the invalid write of size 8 detailed in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70492 Handle the special case when consume_count returns -1 due to an integer overflow when parsing the length of the virtual table qualifier in cplus-dem.c:2994 (gnu_special). Index: libiberty/cplus-dem.c === --- libiberty/cplus-dem.c (revision 234663) +++ libiberty/cplus-dem.c (working copy) @@ -3001,6 +3001,11 @@ gnu_special (work, mangled, declp) success = 1; break; } + else if (n == -1) +{ + success = 0; + break; +} } else {
Fix for PR70481 Libiberty Demangler
Hi, This fixes the use-after-free detailed in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70481 There is a variable ksize storing the amount of allocated memory for the array ktypevec. ksize being zero (0) indicates that some memory must be allocated upon the first write. When more memory is needed, both ksize and the memory are doubled during reallocation. At some point the memory for the array is freed (in squangle_mop_up) but the value of ksize remains. Since ksize is not 0, there is no indication that new memory must be allocated when there is another write to the array. The same holds for bsize and btypevec. Best regards, - Marcel Index: libiberty/cplus-dem.c === --- libiberty/cplus-dem.c (revision 234607) +++ libiberty/cplus-dem.c (working copy) @@ -1237,11 +1237,13 @@ squangle_mop_up (struct work_stuff *work) { free ((char *) work -> btypevec); work->btypevec = NULL; + work->bsize = 0; } if (work -> ktypevec != NULL) { free ((char *) work -> ktypevec); work->ktypevec = NULL; + work->ksize = 0; } }
Re: Proposed Patch for Bug 69687
Hi Bernd, > Are all the places being patched really problematic ones where an input file > could realistically cause an overflow, or just the string functions? The loop in demangle_args allows to call the patched register*- and remember*-methods arbitrarily often. So, those should also overflow at some point. Found a few other segmentation faults in libiberty that I’ll report and patch separately. > I'm concerned about just returning without any kind of error indication. Not > sure what we should be calling from libiberty, but I was thinking maybe > xmalloc_failed. Done. Now, clients of libiberty freeze for about 80 seconds and consume about 3GB of memory before exiting with "out of memory allocating 2147483647 bytes after a total of 3221147648 bytes”. > Might also want to guard against overflow from the first addition. Done. Index: libiberty/cplus-dem.c === --- libiberty/cplus-dem.c (revision 234607) +++ libiberty/cplus-dem.c (working copy) @@ -55,6 +55,7 @@ Boston, MA 02110-1301, USA. */ void * malloc (); void * realloc (); #endif +#include #include #undef CURRENT_DEMANGLING_STYLE @@ -4254,6 +4255,8 @@ remember_type (struct work_stuff *work, } else { + if (work -> typevec_size > INT_MAX / 2) + xmalloc_failed (INT_MAX); work -> typevec_size *= 2; work -> typevec = XRESIZEVEC (char *, work->typevec, work->typevec_size); @@ -4281,6 +4284,8 @@ remember_Ktype (struct work_stuff *work, } else { + if (work -> ksize > INT_MAX / 2) + xmalloc_failed (INT_MAX); work -> ksize *= 2; work -> ktypevec = XRESIZEVEC (char *, work->ktypevec, work->ksize); @@ -4310,6 +4315,8 @@ register_Btype (struct work_stuff *work) } else { + if (work -> bsize > INT_MAX / 2) + xmalloc_failed (INT_MAX); work -> bsize *= 2; work -> btypevec = XRESIZEVEC (char *, work->btypevec, work->bsize); @@ -4764,6 +4771,8 @@ string_need (string *s, int n) else if (s->e - s->p < n) { tem = s->p - s->b; + if (n > INT_MAX / 2 - tem) +xmalloc_failed (INT_MAX); n += tem; n *= 2; s->b = XRESIZEVEC (char, s->b, n);
Re: Proposed Patch for Bug 69687
Hi Bernd: I submitted the filled disclaimer form on March 4th. Now, a representative of FSF mentioned that the copyright assignment is ready on their end. Can someone please go ahead and review the patch? Best regards, - Marcel > On 4 Mar 2016, at 1:43 AM, Bernd Schmidt wrote: > > On 03/03/2016 04:18 PM, Mike Stump wrote: >> On Mar 3, 2016, at 6:55 AM, Marcel Böhme wrote: >>> I have revised the patch and removed the limits. >> >> I looked at the patch, I can find no more unreasonable limits! Wonderful. >> Hope someone will finish off the review and approve. > > Marcel, if you haven't contributed before, we'll need a copyright assignment > from you before we can accept the patch. Do you already have one on file? > > > Bernd >
Re: Proposed Patch for Bug 69687
On 4 Mar 2016, at 1:43 AM, Bernd Schmidt wrote: > > On 03/03/2016 04:18 PM, Mike Stump wrote: >> On Mar 3, 2016, at 6:55 AM, Marcel Böhme wrote: >>> I have revised the patch and removed the limits. >> >> I looked at the patch, I can find no more unreasonable limits! Wonderful. >> Hope someone will finish off the review and approve. > > Marcel, if you haven't contributed before, we'll need a copyright assignment > from you before we can accept the patch. Do you already have one on file? > > > Bernd > Hi Bernd, I have requested the disclaimer form as per /gd/gnuorg/Copyright/request-disclaim.changes Thanks, - Marcel
Re: Proposed Patch for Bug 69687
Thanks Mike. I have revised the patch and removed the limits. While perhaps less security critical, without the limits on the loop count (r) the test cases will still consume all your memory and effectively freeze GDB. * Before any realloc, check for overflow. * string_need now returns 1 if the allocation was successful. * all clients of string_need refrain from extending the string anything if string_need was unsuccessful. > better use unsigned values that are large enough to never overflow. Throughout cplus-dem.c, the length of a string is measured as pointer difference. So, technically length is of type ptrdiff_t which is signed. — a/libiberty/cplus-dem.c +++ b/libiberty/cplus-dem.c @@ -55,6 +55,7 @@ void * malloc (); void * realloc (); #endif +#include #include #undef CURRENT_DEMANGLING_STYLE @@ -379,7 +380,7 @@ static int arm_special (const char **, string *); -static void string_need (string *, int); +static int string_need (string *, int); static void string_delete (string *); @@ -4254,7 +4255,9 @@ } else { - work -> typevec_size *= 2; + if (work -> typevec_size > INT_MAX / 2) +return; + work -> typevec_size *= 2; work -> typevec = XRESIZEVEC (char *, work->typevec, work->typevec_size); } @@ -4281,7 +4284,9 @@ } else { - work -> ksize *= 2; + if (work -> ksize > INT_MAX / 2) +return; + work -> ksize *= 2; work -> ktypevec = XRESIZEVEC (char *, work->ktypevec, work->ksize); } @@ -4294,7 +4299,8 @@ /* Register a B code, and get an index for it. B codes are registered as they are seen, rather than as they are completed, so map > - registers map > as B0, and temp as B1 */ + registers map > as B0, and temp as B1. Returns -1 + if registration was unsuccessful. */ static int register_Btype (struct work_stuff *work) @@ -4310,7 +4316,9 @@ } else { - work -> bsize *= 2; + if (work -> bsize > INT_MAX / 2) +return -1; + work -> bsize *= 2; work -> btypevec = XRESIZEVEC (char *, work->btypevec, work->bsize); } @@ -4328,6 +4336,8 @@ { char *tem; + if (index < 0) +return; tem = XNEWVEC (char, len + 1); memcpy (tem, start, len); tem[len] = '\0'; @@ -4591,7 +4601,8 @@ const char *tem; string_appendn (declp, (*mangled), scan - (*mangled)); - string_need (declp, 1); + if (! string_need (declp, 1)) + return 0; *(declp -> p) = '\0'; /* Consume the function name, including the "__" separating the name @@ -4747,7 +4758,7 @@ /* a mini string-handling package */ -static void +static int string_need (string *s, int n) { int tem; @@ -4765,11 +4776,14 @@ { tem = s->p - s->b; n += tem; + if ( n > INT_MAX / 2) +return 0; n *= 2; s->b = XRESIZEVEC (char, s->b, n); s->p = s->b + tem; s->e = s->b + n; } +return 1; } static void @@ -4811,7 +4825,8 @@ if (s == NULL || *s == '\0') return; n = strlen (s); - string_need (p, n); + if (! string_need (p, n)) +return; memcpy (p->p, s, n); p->p += n; } @@ -4824,7 +4839,8 @@ if (s->b != s->p) { n = s->p - s->b; - string_need (p, n); + if (! string_need (p, n)) +return; memcpy (p->p, s->b, n); p->p += n; } @@ -4835,7 +4851,8 @@ { if (n != 0) { - string_need (p, n); + if (! string_need (p, n)) +return; memcpy (p->p, s, n); p->p += n; } @@ -4866,7 +4883,8 @@ if (n != 0) { - string_need (p, n); + if (! string_need (p, n)) +return; for (q = p->p - 1; q >= p->b; q--) { q[n] = q[0];
Proposed Patch for Bug 69687
Hi, Please find attached the proposed patch for Bug 69687: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69687 * Limiting the length of the mangled string to 264k characters. * Limiting the loop iterations to 256 (max. of C++ function parameters). --- a/libiberty/cplus-dem.c +++ b/libiberty/cplus-dem.c @@ -847,6 +847,13 @@ char * cplus_demangle (const char *mangled, int options) { + /** Limit the maximum length of mangled. + * C++ Std, Annex B (Implementation quantities): + * - Number of characters in an internal identifier or macro name [1 024]. + * - Arguments in one function call [256]. */ + if (mangled && strlen (mangled) > 263168) +return xstrdup (mangled); + char *ret; struct work_stuff work[1]; @@ -4488,6 +4495,8 @@ { return (0); } + /* C++ Standard Annex B: Parameters in one function definition [256].*/ + if (r > 256) r = 256; while (work->nrepeats > 0 || --r >= 0) { tem = work -> typevec[t]; Best regards, - Marcel