Re: Fix for PR68159 in Libiberty Demangler (6)

[Jeff Law]

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 ();
+  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)

[Jakub Jelinek]

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 ();
> >>+  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)

[Jeff Law]

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 ();
+  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)

[Marcel Böhme]

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)

[Ian Lance Taylor]

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)

[Jakub Jelinek]

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)

[Marcel Böhme]

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 (, 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 ();
+  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 ();
+  return 0;
+}
 
-d_print_comp (, options, dc);
-  }
+  d_print_comp (, options, dc);
 
   d_print_flush ();
 
+  free (dpi.copy_templates);
+  free (dpi.saved_scopes);
+
   return ! d_print_saw_error ();
 }
 
@@ -5945,57 +5924,61 @@ d_demangle_callback (const char *mangled, int opti
 
   cplus_demangle_init_info (mangled, options, strlen (mangled), );
 
-  {
-#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

Re: Fix for PR68159 in Libiberty Demangler (6)

[Jakub Jelinek]

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 ();
> +  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)

[Marcel Böhme]

> 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 (, 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 ();
+  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 ();
+  return 0;
+}
 
-d_print_comp (, options, dc);
-  }
+  d_print_comp (, options, dc);
 
   d_print_flush ();
 
+  free (dpi.copy_templates);
+  free (dpi.saved_scopes);
+
   return ! d_print_saw_error ();
 }
 
@@ -5945,57 +5925,58 @@ d_demangle_callback (const char *mangled, int opti
 
   cplus_demangle_init_info (mangled, options, strlen (mangled), );
 
-  {
-#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 ();
+  break;
+case DCT_MANGLED:
+  dc = cplus_demangle_mangled_name (, 1);
+  break;
+case DCT_GLOBAL_CTORS:
+case DCT_GLOBAL_DTORS:
+  d_advance (, 11);
+  dc = d_make_comp (,
+   (type == DCT_GLOBAL_CTORS
+? DEMANGLE_COMPONENT_GLOBAL_CONSTRUCTORS
+: DEMANGLE_COMPONENT_GLOBAL_DESTRUCTORS),
+   d_make_demangle_mangled_name (, d_str ()),
+   NULL);
+  d_advance (, strlen (d_str ()));
+  break;
+default:
+  

Re: Fix for PR68159 in Libiberty Demangler (6)

[Jakub Jelinek]

On Fri, May 06, 2016 at 10:46:12PM +0800, Marcel Böhme wrote:
>d_print_init (, 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 ();
> +  return 0;
> +}

If one malloc succeeds and the other fails, you leak memory.

Jakub

Re: Fix for PR68159 in Libiberty Demangler (6)

[Marcel Böhme]

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 (, 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 ();
+  return 0;
+}
 
-dpi.saved_scopes = scopes;
-dpi.copy_templates 

Re: Fix for PR68159 in Libiberty Demangler (6)

[Jakub Jelinek]

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 (, 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)

[Marcel Böhme]

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 (, 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 (, options, dc);
 
-d_print_comp (, options, dc);
-  }
-
   d_print_flush ();
 
+  free(dpi.copy_templates);
+  free(dpi.saved_scopes);
+
   return ! d_print_saw_error ();
 }
 
@@ -5945,57 +5925,54 @@ d_demangle_callback (const char *mangled, int opti
 
   cplus_demangle_init_info (mangled, options, strlen (mangled), );
 
-  {
-#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));
 
-

Re: Fix for PR68159 in Libiberty Demangler (6)

[Jakub Jelinek]

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 (, 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)

[Marcel Böhme]

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 (, 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 (, options, dc);
 
-d_print_comp (, options, dc);
-  }
-
   d_print_flush ();
 
+  free(dpi.copy_templates);
+  free(dpi.saved_scopes);
+
   return ! d_print_saw_error ();
 }
 
@@ -5945,18 +5931,17 @@ d_demangle_callback (const char *mangled, int opti
 
   cplus_demangle_init_info (mangled, options, strlen (mangled), );
 
-  {
-#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