Message ID | 20190323200635.29070-1-philippe.waroquiers@skynet.be |
---|---|
State | New, archived |
Headers |
Received: (qmail 82780 invoked by alias); 23 Mar 2019 20:06:45 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 82768 invoked by uid 89); 23 Mar 2019 20:06:45 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-22.3 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_STOCKGEN, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.1 spammy=RFQ, rfq, minimal_symbol, sk:042d75e X-HELO: mailsec119.isp.belgacom.be Received: from mailsec119.isp.belgacom.be (HELO mailsec119.isp.belgacom.be) (195.238.20.115) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 23 Mar 2019 20:06:44 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=skynet.be; i=@skynet.be; q=dns/txt; s=securemail; t=1553371602; x=1584907602; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=GNEe3sag07CEXCUVVcRM9OZF/vaOysu/aeA8PutHI8o=; b=G+AvpoWHPVBzXvP4avUkA4rMv5mKo5/9K+K7bP9n/WsPAQZOOipaJUJE ruHVNB2DNx3Oi36Pz+EcGgDHVMuPnA==; Received: from unknown (HELO md.home) ([109.130.122.147]) by relay.skynet.be with ESMTP/TLS/DHE-RSA-AES128-GCM-SHA256; 23 Mar 2019 21:06:40 +0100 From: Philippe Waroquiers <philippe.waroquiers@skynet.be> To: gdb-patches@sourceware.org Cc: Philippe Waroquiers <philippe.waroquiers@skynet.be> Subject: [RFQ] Fix buffer overflow regression due to minsym malloc-ed instead of obstack-ed. Date: Sat, 23 Mar 2019 21:06:35 +0100 Message-Id: <20190323200635.29070-1-philippe.waroquiers@skynet.be> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes |
Commit Message
Philippe Waroquiers
March 23, 2019, 8:06 p.m. UTC
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(-)
Comments
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);
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);