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

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

[Philippe Waroquiers]

Note: this is version 2.  The first RFA (RFQ) was not examining the last
element of the array, as the stop condition in the loop must be <=
instead of <.

Valgrind detects the following error in a bunch of tests,
e.g. in gdb.base/foll-fork.exp.

==15155== VALGRIND_GDB_ERROR_BEGIN
==15155== Invalid read of size 8
==15155==    at 0x55BE04: minimal_symbol_upper_bound(bound_minimal_symbol) (minsyms.c:1504)
==15155==    by 0x3B2E9C: find_pc_partial_function(unsigned long, char const**, unsigned long*, unsigned long*, block const**) (blockframe.c:340)
==15155==    by 0x3B3135: find_function_entry_range_from_pc(unsigned long, char const**, unsigned long*, unsigned long*) (blockframe.c:385)
==15155==    by 0x4F5597: fill_in_stop_func(gdbarch*, execution_control_state*) [clone .part.16] (infrun.c:4124)
==15155==    by 0x4FBE01: fill_in_stop_func (infrun.c:7636)
==15155==    by 0x4FBE01: process_event_stop_test(execution_control_state*) (infrun.c:6279)
...
==15155==  Address 0x715bec8 is 0 bytes after a block of size 2,952 alloc'd
==15155==    at 0x4C2E2B3: realloc (vg_replace_malloc.c:836)
==15155==    by 0x405F2C: xrealloc (common-utils.c:62)
==15155==    by 0x55BA4E: xresizevec<minimal_symbol> (poison.h:170)
==15155==    by 0x55BA4E: minimal_symbol_reader::install() (minsyms.c:1399)
==15155==    by 0x4981C7: elf_read_minimal_symbols (elfread.c:1165)
...

This seems to be a regression created by:
    commit 042d75e42c5572f333e0e06dabd3c5c4afab486c
    Author:     Tom Tromey <tom@tromey.com>
    AuthorDate: Sat Mar 2 12:29:48 2019 -0700
    Commit:     Tom Tromey <tom@tromey.com>
    CommitDate: Fri Mar 15 16:02:10 2019 -0600

        Allocate minimal symbols with malloc

Before this commit, the array of 'struct minimal_symbol'
contained a last element that was a "null symbol".  The comment in
minimal_symbol_reader::install was:
      /* We also terminate the minimal symbol table with a "null symbol",
         which is *not* included in the size of the table.  This makes it
         easier to find the end of the table when we are handed a pointer
         to some symbol in the middle of it.  Zero out the fields in the
         "null symbol" allocated at the end of the array.  Note that the
         symbol count does *not* include this null symbol, which is why it
         is indexed by mcount and not mcount-1.  */

      memset (&msymbols[mcount], 0, sizeof (struct minimal_symbol));

However, minimal_symbol_upper_bound was still based on the assumption
that the array of minsym is terminated by a minsym with a null symbol:
it is looping with:
  for (i = 1; MSYMBOL_LINKAGE_NAME (msymbol + i) != NULL; i++)

Replace this NULL comparison by a logic that calculates how
many msymbol are following the msymbols from which we are starting from.

(Re-)tested on debian/amd64, natively and under valgrind.

Note that a bunch of comments in minimal_symbol_reader::install
are still referring to allocations being done in obstack.  These
comments seem obsolete.  I have not fixed them, as I have not
understood what they are explaining (e.g. related to language
auto, demangling, etc : I have not seen where all this is done).

gdb/ChangeLog
2019-03-24  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* minsyms.c (minimal_symbol_upper_bound): Fix buffer overflow
	by computing the nr of symbols after msymbol.
---
 gdb/minsyms.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index b95e9ef6e8..3db21d1309 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1501,7 +1501,10 @@ minimal_symbol_upper_bound (struct bound_minimal_symbol minsym)
 
   msymbol = minsym.minsym;
   section = MSYMBOL_SECTION (msymbol);
-  for (i = 1; MSYMBOL_LINKAGE_NAME (msymbol + i) != NULL; i++)
+  int n_after_msymbol = minsym.objfile->per_bfd->minimal_symbol_count
+    - (msymbol - minsym.objfile->per_bfd->msymbols.get ())
+    - 1;
+  for (i = 1; i <= n_after_msymbol; i++)
     {
       if ((MSYMBOL_VALUE_RAW_ADDRESS (msymbol + i)
 	   != MSYMBOL_VALUE_RAW_ADDRESS (msymbol))
@@ -1510,7 +1513,7 @@ minimal_symbol_upper_bound (struct bound_minimal_symbol minsym)
     }
 
   obj_section = MSYMBOL_OBJ_SECTION (minsym.objfile, minsym.minsym);
-  if (MSYMBOL_LINKAGE_NAME (msymbol + i) != NULL
+  if (i <= n_after_msymbol
       && (MSYMBOL_VALUE_ADDRESS (minsym.objfile, msymbol + i)
 	  < obj_section_endaddr (obj_section)))
     result = MSYMBOL_VALUE_ADDRESS (minsym.objfile, msymbol + i);
-- 
2.20.1

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

[Tom Tromey]

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> Before this commit, the array of 'struct minimal_symbol'
Philippe> contained a last element that was a "null symbol".  The comment in
Philippe> minimal_symbol_reader::install was:

Sorry about this.

Philippe> Note that a bunch of comments in minimal_symbol_reader::install
Philippe> are still referring to allocations being done in obstack.  These
Philippe> comments seem obsolete.  I have not fixed them, as I have not
Philippe> understood what they are explaining (e.g. related to language
Philippe> auto, demangling, etc : I have not seen where all this is done).

The comment about language_auto is mildly incorrect, and I think
probably has been for quite some time.

There are some other incorrect comments in there.  I'll send a patch.

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.

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);
   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.  */

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

[Philippe Waroquiers]

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.  */

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

[Simon Marchi]

On 2019-03-25 3:54 p.m., Philippe Waroquiers wrote:
> 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.

I just wanted to mention that I just hit this bug, and that Tom's patch fixes it for me.

>> @@ -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 ?

It is mentioned here, search for "extra paren":

https://www.gnu.org/prep/standards/html_node/Formatting.html#Formatting

It's just there to please people who use Emacs :).

> 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.

This, or name the variable "past_the_end" or something like that.

Simon

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

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> and have the '< last' conditions below then be '<= last'.
>> 
>> That makes more clear for me that we handle the last
>> element of the array.

Simon> This, or name the variable "past_the_end" or something like that.

Perhaps I'll use past_the_end and then use !=, since that seems to be
the C++ iterator style.

Tom