This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFQ] Fix buffer overflow regression due to minsym malloc-ed instead of obstack-ed.
- From: Philippe Waroquiers <philippe dot waroquiers at skynet dot be>
- To: gdb-patches at sourceware dot org
- Date: Sat, 23 Mar 2019 21:10:34 +0100
- Subject: Re: [RFQ] Fix buffer overflow regression due to minsym malloc-ed instead of obstack-ed.
- References: <20190323200635.29070-1-philippe.waroquiers@skynet.be>
Typo in the subject: s/RFQ/RFA/
Thanks
Philippe
On Sat, 2019-03-23 at 21:06 +0100, Philippe Waroquiers wrote:
> 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.
>
> 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-23 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..f56663460d 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);