diff mbox

Fix '-data-read-memory-bytes' typo/assertion

Message ID 1447894382-1469-1-git-send-email-donb@codesourcery.com
State New
Headers show

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

Simon Marchi Nov. 19, 2015, 1:27 p.m. UTC | #1
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.
Pedro Alves Nov. 19, 2015, 11:06 p.m. UTC | #2
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
Don Breazeal Nov. 20, 2015, 6:02 p.m. UTC | #3
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
>
Thomas Preud'homme Feb. 3, 2016, 12:37 p.m. UTC | #4
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
Don Breazeal Feb. 4, 2016, 6:23 p.m. UTC | #5
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
Don Breazeal Feb. 4, 2016, 10:54 p.m. UTC | #6
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
Pedro Alves Feb. 4, 2016, 11:02 p.m. UTC | #7
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
Thomas Preud'homme Feb. 5, 2016, 3:04 a.m. UTC | #8
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
Don Breazeal Feb. 11, 2016, 3:31 p.m. UTC | #9
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
Thomas Preud'homme Feb. 15, 2016, 4:58 a.m. UTC | #10
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 mbox

Patch

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