Re: Fix for PR68159 in Libiberty Demangler (6)
On 05/16/2016 12:19 PM, Jakub Jelinek wrote: On Mon, May 16, 2016 at 12:12:38PM -0600, Jeff Law wrote: On 05/06/2016 09:19 AM, Jakub Jelinek wrote: On Fri, May 06, 2016 at 11:11:29PM +0800, Marcel Böhme wrote: + 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; +} Another thing to consider is if the common values of dpi.num_* and similarly in the other block are small enough, it might be desirable to just use an automatic fixed size array (or even alloca) and only fall back to malloc if it is too large. Please, no, don't fall back to alloca like this. That coding idiom has been the source of numerous security exploits in glibc. Experience shows us that we are not capable of doing that correctly on a consistent basis. Falling back to fixed size buffer is something we use heavily in gcc, and are able to get it right, there is nothing hard in it. Conceptually I agree, it ought not be that hard, in practice, it's been an absolute disaster in glibc. I've often wondered if the right model is to to use escape analysis along with the size of the object, loop analysis, etc and let the compiler figure this stuff out rather than leaving it to humans. For the cases where we can't use malloc at all and we'd need too much memory that it won't fit into the static buffer, I think all we can do is fall back into increasing the time complexity in the demangler by processing the string multiple times. Probably true. jeff
Re: Fix for PR68159 in Libiberty Demangler (6)
On Mon, May 16, 2016 at 12:12:38PM -0600, Jeff Law wrote: > On 05/06/2016 09:19 AM, Jakub Jelinek wrote: > >On Fri, May 06, 2016 at 11:11:29PM +0800, Marcel Böhme wrote: > >>+ 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; > >>+} > > > >Another thing to consider is if the common values of dpi.num_* > >and similarly in the other block are small enough, it might be desirable > >to just use an automatic fixed size array (or even alloca) and only > >fall back to malloc if it is too large. > Please, no, don't fall back to alloca like this. That coding idiom has been > the source of numerous security exploits in glibc. Experience shows us that > we are not capable of doing that correctly on a consistent basis. Falling back to fixed size buffer is something we use heavily in gcc, and are able to get it right, there is nothing hard in it. For the cases where we can't use malloc at all and we'd need too much memory that it won't fit into the static buffer, I think all we can do is fall back into increasing the time complexity in the demangler by processing the string multiple times. Jakub
Re: Fix for PR68159 in Libiberty Demangler (6)
On 05/06/2016 09:19 AM, Jakub Jelinek wrote: On Fri, May 06, 2016 at 11:11:29PM +0800, Marcel Böhme wrote: + 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; +} Another thing to consider is if the common values of dpi.num_* and similarly in the other block are small enough, it might be desirable to just use an automatic fixed size array (or even alloca) and only fall back to malloc if it is too large. Please, no, don't fall back to alloca like this. That coding idiom has been the source of numerous security exploits in glibc. Experience shows us that we are not capable of doing that correctly on a consistent basis. Jeff
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)
On Fri, May 6, 2016 at 2:51 AM, Jakub Jelinek wrote: > > Anyway, perhaps I'm misremembering, if there is a mode that really can't > fail due to allocation failures or not, we need to deal with that. > Ian or Jason, can all the demangle users allocate heap memory or not? > And, if __cxa_demangle can fail, there is some allocation_failure stuff > in the file. 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. Ian
Re: Fix for PR68159 in Libiberty Demangler (6)
On Sat, May 07, 2016 at 12:05:11AM +0800, Marcel Böhme wrote: > 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”. But that exactly is the thing I've talked about. Removing the comment doesn't make it right, supposedly it has been done that way for a reason. The file has lots of different entrypoints, some of them depend on various macros on what is it built for (libstdc++, libgcc, binutils/gdb/gcc in libiberty, ...). And some of them clearly can cope with memory allocation failures, but they should be turned into the allocation_failure flag setting. Others don't want any allocations. E.g. if you read the description of __cxa_demangle, there is *STATUS is set to one of the following values: 0: The demangling operation succeeded. -1: A memory allocation failure occurred. -2: MANGLED_NAME is not a valid name under the C++ ABI mangling rules. -3: One of the arguments is invalid. and thus, it should be ensured that we end up with *STATUS -1 even for the cases where malloc failed on those. But then look at e.g. __gcclibcxx_demangle_callback (but there are various others). Jakub
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.num_subs * sizeof (*di.subs)); -#endif + di.subs = (struct demangle_component **)
Re: Fix for PR68159 in Libiberty Demangler (6)
On Fri, May 06, 2016 at 11:11:29PM +0800, Marcel Böhme wrote: > + 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; > +} Another thing to consider is if the common values of dpi.num_* and similarly in the other block are small enough, it might be desirable to just use an automatic fixed size array (or even alloca) and only fall back to malloc if it is too large. Would be nice to say on some distro grab using nm and nm -D all _Z* symbols from all binaries and shared libraries and run the demangler with some statistics gathering. If say dpi.num_saved_scopes is <= 16 in 99.5% cases (completely random guess), it might be a useful optimization. Anyway, that is all from me, I'll defer to the demangler maintainers for the rest. Jakub
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); + d_advance (&di, strlen (d_str (&di))); + break; +default: +
Re: Fix for PR68159 in Libiberty Demangler (6)
On Fri, May 06, 2016 at 10:46:12PM +0800, Marcel Böhme wrote: >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; > +} If one malloc succeeds and the other fails, you leak memory. Jakub
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; +} -dpi.saved_scopes = scopes; -dpi.copy_templates = temps; -#else -dpi.saved_scopes = alloca (dpi.num_saved_scopes -
Re: Fix for PR68159 in Libiberty Demangler (6)
On Fri, May 06, 2016 at 05:01:14PM +0800, Marcel Böhme wrote: > 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. 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. Anyway, perhaps I'm misremembering, if there is a mode that really can't fail due to allocation failures or not, we need to deal with that. Ian or Jason, can all the demangle users allocate heap memory or not? And, if __cxa_demangle can fail, there is some allocation_failure stuff in the file. > @@ -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)); The indentation is still wrong. Either malloc would need to be below (struct or it should be dpi.copy_templates = (struct d_print_template *) malloc (...) But much more importantly, you don't handle the allocation failure in anyway, so if malloc fails, you'll just segfault. > + dpi.saved_scopes = (struct d_saved_scope *) > + malloc (dpi.num_saved_scopes * sizeof (*dpi.saved_scopes)); See above. > > + free(dpi.copy_templates); > + free(dpi.saved_scopes); Formatting, missing space before (. Jakub
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 + * sizeof (*di.subs)); -switch (type) - { -
Re: Fix for PR68159 in Libiberty Demangler (6)
On Fri, May 06, 2016 at 02:14:31PM +0800, Marcel Böhme wrote: > * 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. I think this is very problematic, but I'll let others comment on in detail. Have you tested the patch at all? 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. 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) (which is why I'm asking about what testing you've done). > @@ -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)); Ignoring the fact that this patch breaks libstdc++, there are also formatting and other issues. Missing space in between xmalloc_failed and (, = should not appear at the end of line, but on the start of next line and it should be indented by 2, not 4. 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? Jakub
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 d_advance (&di, strlen (d_str (&di)));