Message ID | 1447894382-1469-1-git-send-email-donb@codesourcery.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 80516 invoked by alias); 19 Nov 2015 00:53:08 -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 80479 invoked by uid 89); 19 Nov 2015 00:53:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 19 Nov 2015 00:53:06 +0000 Received: from svr-orw-fem-04.mgc.mentorg.com ([147.34.97.41]) by relay1.mentorg.com with esmtp id 1ZzDTD-0005Ke-MZ from Don_Breazeal@mentor.com for gdb-patches@sourceware.org; Wed, 18 Nov 2015 16:53:03 -0800 Received: from build4-lucid-cs (147.34.91.1) by svr-orw-fem-04.mgc.mentorg.com (147.34.97.41) with Microsoft SMTP Server id 14.3.224.2; Wed, 18 Nov 2015 16:53:03 -0800 Received: by build4-lucid-cs (Postfix, from userid 1905) id EB16141154; Wed, 18 Nov 2015 16:53:02 -0800 (PST) From: Don Breazeal <donb@codesourcery.com> To: <gdb-patches@sourceware.org> Subject: [PATCH] Fix '-data-read-memory-bytes' typo/assertion Date: Wed, 18 Nov 2015 16:53:02 -0800 Message-ID: <1447894382-1469-1-git-send-email-donb@codesourcery.com> MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes |
Commit Message
Don Breazeal
Nov. 19, 2015, 12:53 a.m. UTC
This patch fixes a typo in target.c:read_memory_robust, where it calls read_whatever_is_readable with the function arguments in the wrong order. Depending on the address being read, it can cause an xmalloc with a huge size, resulting in an assertion failure, or just read something other than what was requested. The problem only arises when GDB is handling an MI "-data-read-memory-bytes" request and the initial target_read returns an error status. Note that read_memory_robust is only called from the MI code. Function definition: static void read_whatever_is_readable (struct target_ops *ops, const ULONGEST begin, const ULONGEST end, int unit_size, VEC(memory_read_result_s) **result) Function call: read_whatever_is_readable (ops, offset + xfered_total, unit_size, offset + xfered_total + to_read, &result); If we debug gdb, generate the error, and break just before the call to read_whatever_is_readable, we see: # Generate an error by trying to read a bogus address in the GDB # that is under debug. (gdb) interpreter-exec mi "-data-read-memory-bytes 1073741752 216" # GDB-under-debug stops at the breakpoint on the call to # read_whatever_is_readable. Breakpoint 1, read_memory_robust (ops=0xe70150, offset=1073741752, len=216) at /scratch/dbreazea/sandbox/gdb-5611/binutils-gdb/gdb/target.c:1825 1825 read_whatever_is_readable (ops, offset + xfered_total, unit_size, (top) p unit_size $1 = 1 # Step into the function. (top) step read_whatever_is_readable (ops=0xe70150, begin=1073741752, end=1, unit_size=1073741968, result=0x7fffffffdd40) at /scratch/dbreazea/sandbox/gdb-5611/binutils-gdb/gdb/target.c:1658 1658 gdb_byte *buf = (gdb_byte *) xmalloc (end - begin); # unit_size was passed as 'end', and we are going to xmalloc a large # number and assert. (top) p end-begin $2 = 18446744072635809865 (top) c Continuing. "/scratch/dbreazea/sandbox/gdb-5611/binutils-gdb/gdb/utils.c:1072: internal-error: virtual memory exhausted.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable.\nQuit this debugging session? (y or n) " # With the fixed version, (end - begin) gives the 'len' passed to # read_memory_robust and specified by -data-read-memory-bytes (top) p end-begin $2 = 216 Tested on native x86_64 Linux with the gdb.mi tests. OK? thanks --Don gdb/ 2015-11-18 Don Breazeal <donb@codesourcery.com> * gdb/target.c (read_memory_robust): Call read_whatever_is_readable with arguments in the correct order. --- gdb/target.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Comments
On 15-11-18 07:53 PM, Don Breazeal wrote: > This patch fixes a typo in target.c:read_memory_robust, where > it calls read_whatever_is_readable with the function arguments > in the wrong order. Depending on the address being read, it > can cause an xmalloc with a huge size, resulting in an assertion > failure, or just read something other than what was requested. > > The problem only arises when GDB is handling an MI > "-data-read-memory-bytes" request and the initial target_read returns > an error status. Note that read_memory_robust is only called from > the MI code. > > Function definition: > static void > read_whatever_is_readable (struct target_ops *ops, > const ULONGEST begin, const ULONGEST end, > int unit_size, > VEC(memory_read_result_s) **result) > > Function call: > read_whatever_is_readable (ops, offset + xfered_total, unit_size, > offset + xfered_total + to_read, &result); > > If we debug gdb, generate the error, and break just before the call to > read_whatever_is_readable, we see: > > # Generate an error by trying to read a bogus address in the GDB > # that is under debug. > (gdb) interpreter-exec mi "-data-read-memory-bytes 1073741752 216" > > # GDB-under-debug stops at the breakpoint on the call to > # read_whatever_is_readable. > Breakpoint 1, read_memory_robust (ops=0xe70150, offset=1073741752, len=216) > at /scratch/dbreazea/sandbox/gdb-5611/binutils-gdb/gdb/target.c:1825 > 1825 read_whatever_is_readable (ops, offset + xfered_total, unit_size, > (top) p unit_size > $1 = 1 > > # Step into the function. > (top) step > read_whatever_is_readable (ops=0xe70150, begin=1073741752, end=1, > unit_size=1073741968, result=0x7fffffffdd40) > at /scratch/dbreazea/sandbox/gdb-5611/binutils-gdb/gdb/target.c:1658 > 1658 gdb_byte *buf = (gdb_byte *) xmalloc (end - begin); > > # unit_size was passed as 'end', and we are going to xmalloc a large > # number and assert. > (top) p end-begin > $2 = 18446744072635809865 > (top) c > Continuing. > "/scratch/dbreazea/sandbox/gdb-5611/binutils-gdb/gdb/utils.c:1072: internal-error: virtual memory exhausted.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable.\nQuit this debugging session? (y or n) " > > # With the fixed version, (end - begin) gives the 'len' passed to > # read_memory_robust and specified by -data-read-memory-bytes > (top) p end-begin > $2 = 216 > > Tested on native x86_64 Linux with the gdb.mi tests. > > OK? > thanks > --Don > > gdb/ > 2015-11-18 Don Breazeal <donb@codesourcery.com> > > * gdb/target.c (read_memory_robust): Call > read_whatever_is_readable with arguments in the correct order. > > --- > gdb/target.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/gdb/target.c b/gdb/target.c > index 93786c3..950a1b7 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -1822,8 +1822,9 @@ read_memory_robust (struct target_ops *ops, > /* Got an error reading full chunk. See if maybe we can read > some subrange. */ > xfree (buffer); > - read_whatever_is_readable (ops, offset + xfered_total, unit_size, > - offset + xfered_total + to_read, &result); > + read_whatever_is_readable (ops, offset + xfered_total, > + offset + xfered_total + to_read, > + unit_size, &result); > xfered_total += to_read; > } > else > Whoops. LGTM.
On 11/19/2015 12:53 AM, Don Breazeal wrote: > gdb/ > 2015-11-18 Don Breazeal <donb@codesourcery.com> > > * gdb/target.c (read_memory_robust): Call > read_whatever_is_readable with arguments in the correct order. > Please drop the "gdb/" in the file name. LGTM too. Obvious even. It'd be nice to have a testcase for this though. Seems like the simplest would be to just try e.g., "-data-read-memory-bytes 8 1" and make sure that fails with "Unable to read memory". We'd just need to skip the test if [is_address_zero_readable]. Thanks, Pedro Alves
On 11/19/2015 3:06 PM, Pedro Alves wrote: > On 11/19/2015 12:53 AM, Don Breazeal wrote: > >> gdb/ >> 2015-11-18 Don Breazeal <donb@codesourcery.com> >> >> * gdb/target.c (read_memory_robust): Call >> read_whatever_is_readable with arguments in the correct order. >> > > Please drop the "gdb/" in the file name. > > LGTM too. Obvious even. It'd be nice to have a testcase > for this though. Seems like the simplest would be to just try > e.g., "-data-read-memory-bytes 8 1" and make sure that fails > with "Unable to read memory". We'd just need to skip the > test if [is_address_zero_readable]. This is now pushed (e084c964d61e6f8582711c73738c4df132410597), ChangeLog is fixed. I'll put the test on my list of tests to implement. (I *will* get to these, incl. fork/exec tests, honest! :-) ) --Don On 11/18/2015 4:53 PM, Don Breazeal wrote: > This patch fixes a typo in target.c:read_memory_robust, where > it calls read_whatever_is_readable with the function arguments > in the wrong order. Depending on the address being read, it > can cause an xmalloc with a huge size, resulting in an assertion > failure, or just read something other than what was requested. > > The problem only arises when GDB is handling an MI > "-data-read-memory-bytes" request and the initial target_read returns > an error status. Note that read_memory_robust is only called from > the MI code. > > Function definition: > static void > read_whatever_is_readable (struct target_ops *ops, > const ULONGEST begin, const ULONGEST end, > int unit_size, > VEC(memory_read_result_s) **result) > > Function call: > read_whatever_is_readable (ops, offset + xfered_total, unit_size, > offset + xfered_total + to_read, &result); > > If we debug gdb, generate the error, and break just before the call to > read_whatever_is_readable, we see: > > # Generate an error by trying to read a bogus address in the GDB > # that is under debug. > (gdb) interpreter-exec mi "-data-read-memory-bytes 1073741752 216" > > # GDB-under-debug stops at the breakpoint on the call to > # read_whatever_is_readable. > Breakpoint 1, read_memory_robust (ops=0xe70150, offset=1073741752, len=216) > at /scratch/dbreazea/sandbox/gdb-5611/binutils-gdb/gdb/target.c:1825 > 1825 read_whatever_is_readable (ops, offset + xfered_total, unit_size, > (top) p unit_size > $1 = 1 > > # Step into the function. > (top) step > read_whatever_is_readable (ops=0xe70150, begin=1073741752, end=1, > unit_size=1073741968, result=0x7fffffffdd40) > at /scratch/dbreazea/sandbox/gdb-5611/binutils-gdb/gdb/target.c:1658 > 1658 gdb_byte *buf = (gdb_byte *) xmalloc (end - begin); > > # unit_size was passed as 'end', and we are going to xmalloc a large > # number and assert. > (top) p end-begin > $2 = 18446744072635809865 > (top) c > Continuing. > "/scratch/dbreazea/sandbox/gdb-5611/binutils-gdb/gdb/utils.c:1072: internal-error: virtual memory exhausted.\nA problem internal to GDB has been detected,\nfurther debugging may prove unreliable.\nQuit this debugging session? (y or n) " > > # With the fixed version, (end - begin) gives the 'len' passed to > # read_memory_robust and specified by -data-read-memory-bytes > (top) p end-begin > $2 = 216 > > Tested on native x86_64 Linux with the gdb.mi tests. > > OK? > thanks > --Don > > gdb/ > 2015-11-18 Don Breazeal <donb@codesourcery.com> > > * gdb/target.c (read_memory_robust): Call > read_whatever_is_readable with arguments in the correct order. > > --- > gdb/target.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/gdb/target.c b/gdb/target.c > index 93786c3..950a1b7 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -1822,8 +1822,9 @@ read_memory_robust (struct target_ops *ops, > /* Got an error reading full chunk. See if maybe we can read > some subrange. */ > xfree (buffer); > - read_whatever_is_readable (ops, offset + xfered_total, unit_size, > - offset + xfered_total + to_read, &result); > + read_whatever_is_readable (ops, offset + xfered_total, > + offset + xfered_total + to_read, > + unit_size, &result); > xfered_total += to_read; > } > else >
Hi Don, > From: gdb-patches-owner@sourceware.org [mailto:gdb-patches- > owner@sourceware.org] On Behalf Of Don Breazeal > > > This is now pushed (e084c964d61e6f8582711c73738c4df132410597), > ChangeLog is fixed. I believe this also affects the gdb 7.10 branch. Would you mind doing a backport for it? Best regards, Thomas
On 2/3/2016 4:37 AM, Thomas Preud'homme wrote: > Hi Don, > >> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches- >> owner@sourceware.org] On Behalf Of Don Breazeal >> >> >> This is now pushed (e084c964d61e6f8582711c73738c4df132410597), >> ChangeLog is fixed. > > I believe this also affects the gdb 7.10 branch. Would you mind doing a backport for it? > > Best regards, > > Thomas > > Hi Thomas, Maybe I don't understand something, but isn't the 7.10 branch essentially frozen, since there are no plans to generate another GDB release from that branch? The fix will be in GDB 7.11. --Don
On 2/4/2016 10:23 AM, Don Breazeal wrote: > On 2/3/2016 4:37 AM, Thomas Preud'homme wrote: >> Hi Don, >> >>> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches- >>> owner@sourceware.org] On Behalf Of Don Breazeal >>> >>> >>> This is now pushed (e084c964d61e6f8582711c73738c4df132410597), >>> ChangeLog is fixed. >> >> I believe this also affects the gdb 7.10 branch. Would you mind doing a backport for it? >> >> Best regards, >> >> Thomas >> >> > Hi Thomas, > Maybe I don't understand something, but isn't the 7.10 branch > essentially frozen, since there are no plans to generate another GDB > release from that branch? > > The fix will be in GDB 7.11. > --Don > I've been told off-list that back-porting to a stable branch like this can sometimes be appropriate. Pedro, you approved the original patch; OK to back-port this to the 7.10 branch? thanks --Don
On 02/04/2016 10:54 PM, Don Breazeal wrote: > > Pedro, you approved the original patch; OK to back-port this to the 7.10 > branch? OK. Thanks, Pedro Alves
On Thursday, February 04, 2016 11:02:02 PM Pedro Alves wrote: > On 02/04/2016 10:54 PM, Don Breazeal wrote: > > Pedro, you approved the original patch; OK to back-port this to the 7.10 > > branch? > > OK. Great! Many people build from stable branch rather than from tarball to get all the latest bugfixes. I know we do so as probably many others so a backport is definitely worthwhile I think. Thanks both of you. Best regards, Thomas
On 2/4/2016 7:04 PM, Thomas Preud'homme wrote: > On Thursday, February 04, 2016 11:02:02 PM Pedro Alves wrote: >> On 02/04/2016 10:54 PM, Don Breazeal wrote: >>> Pedro, you approved the original patch; OK to back-port this to the 7.10 >>> branch? >> >> OK. > > Great! Many people build from stable branch rather than from tarball to get > all the latest bugfixes. I know we do so as probably many others so a backport > is definitely worthwhile I think. > > Thanks both of you. > > Best regards, > > Thomas > This is now pushed to the 7.10 branch. Sorry for the delay. --Don
On Thursday, February 11, 2016 07:31:11 AM Don Breazeal wrote: > > This is now pushed to the 7.10 branch. Sorry for the delay. Not at all, thank you! Best regards, Thomas
diff --git a/gdb/target.c b/gdb/target.c index 93786c3..950a1b7 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -1822,8 +1822,9 @@ read_memory_robust (struct target_ops *ops, /* Got an error reading full chunk. See if maybe we can read some subrange. */ xfree (buffer); - read_whatever_is_readable (ops, offset + xfered_total, unit_size, - offset + xfered_total + to_read, &result); + read_whatever_is_readable (ops, offset + xfered_total, + offset + xfered_total + to_read, + unit_size, &result); xfered_total += to_read; } else