Re: Fix for PR68159 in Libiberty Demangler (6)

[Jakub Jelinek <jakub at redhat dot com>]

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