Re: [RFAv2] Fix buffer overflow regression due to minsym malloc-ed instead of obstack-ed.

[Philippe Waroquiers <philippe dot waroquiers at skynet dot be>]

On Mon, 2019-03-25 at 09:31 -0600, Tom Tromey wrote:
> Philippe> +  int n_after_msymbol = minsym.objfile->per_bfd->minimal_symbol_count
> Philippe> +    - (msymbol - minsym.objfile->per_bfd->msymbols.get ())
> Philippe> +    - 1;
> 
> What do you think of the appended instead?
> The idea is to make the last element more explicit.
Yes, that looks better, 2 minor comments below.
Thanks
Philippe
> 
> Tom
> 
> diff --git a/gdb/minsyms.c b/gdb/minsyms.c
> index b95e9ef6e8b..03743e3062b 100644
> --- a/gdb/minsyms.c
> +++ b/gdb/minsyms.c
> @@ -1480,11 +1480,10 @@ find_solib_trampoline_target (struct frame_info *frame, CORE_ADDR pc)
>  CORE_ADDR
>  minimal_symbol_upper_bound (struct bound_minimal_symbol minsym)
>  {
> -  int i;
>    short section;
>    struct obj_section *obj_section;
>    CORE_ADDR result;
> -  struct minimal_symbol *msymbol;
> +  struct minimal_symbol *iter, *msymbol;
>  
>    gdb_assert (minsym.minsym != NULL);
>  
> @@ -1499,21 +1498,24 @@ minimal_symbol_upper_bound (struct bound_minimal_symbol minsym)
>       other sections, to find the next symbol in this section with a
>       different address.  */
>  
> +  struct minimal_symbol *last
> +    = (minsym.objfile->per_bfd->msymbols.get ()
> +       + minsym.objfile->per_bfd->minimal_symbol_count);
Are the parenthesis needed here ?

Also, I find the name 'last' a little bit confusing,
as in the loop below, last is not handled.
Maybe last could be the 'real' last i.e. as:
   minsym.objfile->per_bfd->msymbols.get () +       
    + minsym.objfile->per_bfd->minimal_symbol_count - 1;

and have the '< last' conditions below then be '<= last'.

That makes more clear for me that we handle the last
element of the array.


>    msymbol = minsym.minsym;
>    section = MSYMBOL_SECTION (msymbol);
> -  for (i = 1; MSYMBOL_LINKAGE_NAME (msymbol + i) != NULL; i++)
> +  for (iter = msymbol + 1; iter < last; ++iter)
>      {
> -      if ((MSYMBOL_VALUE_RAW_ADDRESS (msymbol + i)
> +      if ((MSYMBOL_VALUE_RAW_ADDRESS (iter)
>  	   != MSYMBOL_VALUE_RAW_ADDRESS (msymbol))
> -	  && MSYMBOL_SECTION (msymbol + i) == section)
> +	  && MSYMBOL_SECTION (iter) == section)
>  	break;
>      }
>  
>    obj_section = MSYMBOL_OBJ_SECTION (minsym.objfile, minsym.minsym);
> -  if (MSYMBOL_LINKAGE_NAME (msymbol + i) != NULL
> -      && (MSYMBOL_VALUE_ADDRESS (minsym.objfile, msymbol + i)
> +  if (iter < last
> +      && (MSYMBOL_VALUE_ADDRESS (minsym.objfile, iter)
>  	  < obj_section_endaddr (obj_section)))
> -    result = MSYMBOL_VALUE_ADDRESS (minsym.objfile, msymbol + i);
> +    result = MSYMBOL_VALUE_ADDRESS (minsym.objfile, iter);
>    else
>      /* We got the start address from the last msymbol in the objfile.
>         So the end address is the end of the section.  */