This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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 (0x00007ffe6053c000)
	libm.so.6 => /lib64/libm.so.6 (0x00007f9de23fb000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f9de203a000)
	/lib64/ld-linux-x86-64.so.2 (0x00005585ecc1d000)
	libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007f9de1e22000)
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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]