[PATCH] Linux: Avoid pread64/pwrite64 for high memory addresses (PR gdb/30525)

[PATCH] Linux: Avoid pread64/pwrite64 for high memory addresses (PR gdb/30525)

[Pedro Alves]

Since commit 05c06f318fd9 ("Linux: Access memory even if threads are
running"), GDB prefers pread64/pwrite64 to access inferior memory
instead of ptrace.  That change broke reading shared libraries on
SPARC Linux, as reported by PR gdb/30525 ("gdb cannot read shared
libraries on SPARC64").

On SPARC64 Linux, surprisingly (to me), userspace shared libraries are
mapped at high 64-bit addresses:

   (gdb) info sharedlibrary
   Cannot access memory at address 0xfff80001002011e0
   Cannot access memory at address 0xfff80001002011d8
   Cannot access memory at address 0xfff80001002011d8
   From                To                  Syms Read   Shared Object Library
   0xfff80001000010a0  0xfff8000100021f80  Yes (*)     /lib64/ld-linux.so.2
   (*): Shared library is missing debugging information.

Those addresses are 64-bit addresses with the high bits set.  When
interpreted as signed, they're negative.

The Linux kernel rejects pread64/pwrite64 if the offset argument of
type off_t (a signed type) is negative, which happens if the memory
address we're accessing has its high bit set.  See
linux/fs/read_write.c sys_pread64 and sys_pwrite64 in Linux.

Thanksfully, lseek does not fail in that situation.  So the fix is to
use the 'lseek + read|write' path if the offset would be negative.

Fix this in both native GDB and GDBserver.

Tested on a SPARC64 GNU/Linux and x86_64 GNU/Linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30525
Change-Id: I79c724f918037ea67b7396fadb521bc9d1b10dc5
---
 gdb/linux-nat.c        | 30 ++++++++++++++++++++----------
 gdbserver/linux-low.cc | 33 ++++++++++++++++++++-------------
 2 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 383ef58fa23..c5efacbb3fa 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -3909,18 +3909,28 @@ linux_proc_xfer_memory_partial_fd (int fd, int pid,
 
   gdb_assert (fd != -1);
 
-  /* Use pread64/pwrite64 if available, since they save a syscall and can
-     handle 64-bit offsets even on 32-bit platforms (for instance, SPARC
-     debugging a SPARC64 application).  */
+  /* Use pread64/pwrite64 if available, since they save a syscall and
+     can handle 64-bit offsets even on 32-bit platforms (for instance,
+     SPARC debugging a SPARC64 application).  But only use them if the
+     offset isn't so high that when cast to off_t it'd be negative, as
+     seen on SPARC64.  pread64/pwrite64 outright reject such offsets.
+     lseek does not.  */
 #ifdef HAVE_PREAD64
-  ret = (readbuf ? pread64 (fd, readbuf, len, offset)
-	 : pwrite64 (fd, writebuf, len, offset));
-#else
-  ret = lseek (fd, offset, SEEK_SET);
-  if (ret != -1)
-    ret = (readbuf ? read (fd, readbuf, len)
-	   : write (fd, writebuf, len));
+  if ((off_t) offset >= 0)
+    {
+      ret = (readbuf != nullptr
+	     ? pread64 (fd, readbuf, len, offset)
+	     : pwrite64 (fd, writebuf, len, offset));
+    }
+  else
 #endif
+    {
+      ret = lseek (fd, offset, SEEK_SET);
+      if (ret != -1)
+	ret = (readbuf
+	       ? read (fd, readbuf, len)
+	       : write (fd, writebuf, len));
+    }
 
   if (ret == -1)
     {
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 8ab16698632..63668d81b45 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -5377,21 +5377,28 @@ proc_xfer_memory (CORE_ADDR memaddr, unsigned char *readbuf,
     {
       int bytes;
 
-      /* If pread64 is available, use it.  It's faster if the kernel
-	 supports it (only one syscall), and it's 64-bit safe even on
-	 32-bit platforms (for instance, SPARC debugging a SPARC64
-	 application).  */
+      /* Use pread64/pwrite64 if available, since they save a syscall
+	 and can handle 64-bit offsets even on 32-bit platforms (for
+	 instance, SPARC debugging a SPARC64 application).  But only
+	 use them if the offset isn't so high that when cast to off_t
+	 it'd be negative, as seen on SPARC64.  pread64/pwrite64
+	 outright reject such offsets.  lseek does not.  */
 #ifdef HAVE_PREAD64
-      bytes = (readbuf != nullptr
-	       ? pread64 (fd, readbuf, len, memaddr)
-	       : pwrite64 (fd, writebuf, len, memaddr));
-#else
-      bytes = -1;
-      if (lseek (fd, memaddr, SEEK_SET) != -1)
-	bytes = (readbuf != nullptr
-		 ? read (fd, readbuf, len)
-		 : write (fd, writebuf, len));
+      if ((off_t) memaddr >= 0)
+	{
+	  bytes = (readbuf != nullptr
+		   ? pread64 (fd, readbuf, len, memaddr)
+		   : pwrite64 (fd, writebuf, len, memaddr));
+	}
+      else
 #endif
+	{
+	  bytes = -1;
+	  if (lseek (fd, memaddr, SEEK_SET) != -1)
+	    bytes = (readbuf != nullptr
+		     ? read (fd, readbuf, len)
+		     : write (fd, writebuf, len));
+	}
 
       if (bytes < 0)
 	return errno;

base-commit: a5042543d819df8a76ebec8c4d715f244efbab0a
-- 
2.34.1

Re: [PATCH] Linux: Avoid pread64/pwrite64 for high memory addresses (PR gdb/30525)

[Matt Turner]

Thank you!

Re: [PATCH] Linux: Avoid pread64/pwrite64 for high memory addresses (PR gdb/30525)

[Andrew Burgess]

Pedro Alves <pedro@palves.net> writes:

> Since commit 05c06f318fd9 ("Linux: Access memory even if threads are
> running"), GDB prefers pread64/pwrite64 to access inferior memory
> instead of ptrace.  That change broke reading shared libraries on
> SPARC Linux, as reported by PR gdb/30525 ("gdb cannot read shared
> libraries on SPARC64").
>
> On SPARC64 Linux, surprisingly (to me), userspace shared libraries are
> mapped at high 64-bit addresses:
>
>    (gdb) info sharedlibrary
>    Cannot access memory at address 0xfff80001002011e0
>    Cannot access memory at address 0xfff80001002011d8
>    Cannot access memory at address 0xfff80001002011d8
>    From                To                  Syms Read   Shared Object Library
>    0xfff80001000010a0  0xfff8000100021f80  Yes (*)     /lib64/ld-linux.so.2
>    (*): Shared library is missing debugging information.
>
> Those addresses are 64-bit addresses with the high bits set.  When
> interpreted as signed, they're negative.
>
> The Linux kernel rejects pread64/pwrite64 if the offset argument of
> type off_t (a signed type) is negative, which happens if the memory
> address we're accessing has its high bit set.  See
> linux/fs/read_write.c sys_pread64 and sys_pwrite64 in Linux.
>
> Thanksfully, lseek does not fail in that situation.  So the fix is to
> use the 'lseek + read|write' path if the offset would be negative.
>
> Fix this in both native GDB and GDBserver.
>
> Tested on a SPARC64 GNU/Linux and x86_64 GNU/Linux.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30525
> Change-Id: I79c724f918037ea67b7396fadb521bc9d1b10dc5
> ---
>  gdb/linux-nat.c        | 30 ++++++++++++++++++++----------
>  gdbserver/linux-low.cc | 33 ++++++++++++++++++++-------------
>  2 files changed, 40 insertions(+), 23 deletions(-)
>
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 383ef58fa23..c5efacbb3fa 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -3909,18 +3909,28 @@ linux_proc_xfer_memory_partial_fd (int fd, int pid,
>  
>    gdb_assert (fd != -1);
>  
> -  /* Use pread64/pwrite64 if available, since they save a syscall and can
> -     handle 64-bit offsets even on 32-bit platforms (for instance, SPARC
> -     debugging a SPARC64 application).  */
> +  /* Use pread64/pwrite64 if available, since they save a syscall and
> +     can handle 64-bit offsets even on 32-bit platforms (for instance,
> +     SPARC debugging a SPARC64 application).  But only use them if the
> +     offset isn't so high that when cast to off_t it'd be negative, as
> +     seen on SPARC64.  pread64/pwrite64 outright reject such offsets.
> +     lseek does not.  */
>  #ifdef HAVE_PREAD64
> -  ret = (readbuf ? pread64 (fd, readbuf, len, offset)
> -	 : pwrite64 (fd, writebuf, len, offset));
> -#else
> -  ret = lseek (fd, offset, SEEK_SET);
> -  if (ret != -1)
> -    ret = (readbuf ? read (fd, readbuf, len)
> -	   : write (fd, writebuf, len));
> +  if ((off_t) offset >= 0)
> +    {
> +      ret = (readbuf != nullptr
> +	     ? pread64 (fd, readbuf, len, offset)
> +	     : pwrite64 (fd, writebuf, len, offset));
> +    }

I haven't tested this, but the change looks good to me with one nit...

I think the '{' and '}' here (and in gdbserver below) aren't GDB style,
as there's only a single statement?

Otherwise,

Approved-By: Andrew Burgess <aburgess@redhat.com>

thanks,
Andrew

> +  else
>  #endif
> +    {
> +      ret = lseek (fd, offset, SEEK_SET);
> +      if (ret != -1)
> +	ret = (readbuf
> +	       ? read (fd, readbuf, len)
> +	       : write (fd, writebuf, len));
> +    }
>  
>    if (ret == -1)
>      {
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index 8ab16698632..63668d81b45 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -5377,21 +5377,28 @@ proc_xfer_memory (CORE_ADDR memaddr, unsigned char *readbuf,
>      {
>        int bytes;
>  
> -      /* If pread64 is available, use it.  It's faster if the kernel
> -	 supports it (only one syscall), and it's 64-bit safe even on
> -	 32-bit platforms (for instance, SPARC debugging a SPARC64
> -	 application).  */
> +      /* Use pread64/pwrite64 if available, since they save a syscall
> +	 and can handle 64-bit offsets even on 32-bit platforms (for
> +	 instance, SPARC debugging a SPARC64 application).  But only
> +	 use them if the offset isn't so high that when cast to off_t
> +	 it'd be negative, as seen on SPARC64.  pread64/pwrite64
> +	 outright reject such offsets.  lseek does not.  */
>  #ifdef HAVE_PREAD64
> -      bytes = (readbuf != nullptr
> -	       ? pread64 (fd, readbuf, len, memaddr)
> -	       : pwrite64 (fd, writebuf, len, memaddr));
> -#else
> -      bytes = -1;
> -      if (lseek (fd, memaddr, SEEK_SET) != -1)
> -	bytes = (readbuf != nullptr
> -		 ? read (fd, readbuf, len)
> -		 : write (fd, writebuf, len));
> +      if ((off_t) memaddr >= 0)
> +	{
> +	  bytes = (readbuf != nullptr
> +		   ? pread64 (fd, readbuf, len, memaddr)
> +		   : pwrite64 (fd, writebuf, len, memaddr));
> +	}
> +      else
>  #endif
> +	{
> +	  bytes = -1;
> +	  if (lseek (fd, memaddr, SEEK_SET) != -1)
> +	    bytes = (readbuf != nullptr
> +		     ? read (fd, readbuf, len)
> +		     : write (fd, writebuf, len));
> +	}
>  
>        if (bytes < 0)
>  	return errno;
>
> base-commit: a5042543d819df8a76ebec8c4d715f244efbab0a
> -- 
> 2.34.1

Re: [PATCH] Linux: Avoid pread64/pwrite64 for high memory addresses (PR gdb/30525)

[Pedro Alves]

Hi Andrew,

On 2023-07-05 18:59, Andrew Burgess wrote:
> Pedro Alves <pedro@palves.net> writes:
>> +  if ((off_t) offset >= 0)
>> +    {
>> +      ret = (readbuf != nullptr
>> +	     ? pread64 (fd, readbuf, len, offset)
>> +	     : pwrite64 (fd, writebuf, len, offset));
>> +    }
> 
> I haven't tested this, but the change looks good to me with one nit...
> 
> I think the '{' and '}' here (and in gdbserver below) aren't GDB style,
> as there's only a single statement?

I don't really mind much either way, as we're not super consistent, but note that
we do have a GDB-specific rule about this.  Here:

https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards

It says:

  "Any two or more lines in code should be wrapped in braces, even if they are comments, as they look like separate statements:"

As that says "Any two or more lines in code", I think it applies in this situation too.

Pedro Alves

Re: [PATCH] Linux: Avoid pread64/pwrite64 for high memory addresses (PR gdb/30525)

[Pedro Alves]

Hi again,

On 2023-07-06 14:43, Pedro Alves wrote:
> Hi Andrew,
> 
> On 2023-07-05 18:59, Andrew Burgess wrote:
>> Pedro Alves <pedro@palves.net> writes:
>>> +  if ((off_t) offset >= 0)
>>> +    {
>>> +      ret = (readbuf != nullptr
>>> +	     ? pread64 (fd, readbuf, len, offset)
>>> +	     : pwrite64 (fd, writebuf, len, offset));
>>> +    }
>>
>> I haven't tested this, but the change looks good to me with one nit...
>>
>> I think the '{' and '}' here (and in gdbserver below) aren't GDB style,
>> as there's only a single statement?
> 
> I don't really mind much either way, as we're not super consistent, but note that
> we do have a GDB-specific rule about this.  Here:
> 
> https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards
> 
> It says:
> 
>   "Any two or more lines in code should be wrapped in braces, even if they are comments, as they look like separate statements:"
> 
> As that says "Any two or more lines in code", I think it applies in this situation too.
> 

I just realized that the lseek path, which I just reindented, has an if branch
with more than one line which isn't using braces...  The patch should at least be
consistent in all the lines it touches...  :-)  If we follow the documented convention,
then here's what we get.

WDYT?  Should we tweak the convention instead?

From ab7a42f42e711f2a3565da4f53b0d2e6bea0039d Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Wed, 7 Jun 2023 10:38:14 +0100
Subject: [PATCH] Linux: Avoid pread64/pwrite64 for high memory addresses (PR
 gdb/30525)

Since commit 05c06f318fd9 ("Linux: Access memory even if threads are
running"), GDB prefers pread64/pwrite64 to access inferior memory
instead of ptrace.  That change broke reading shared libraries on
SPARC64 Linux, as reported by PR gdb/30525 ("gdb cannot read shared
libraries on SPARC64").

On SPARC64 Linux, surprisingly (to me), userspace shared libraries are
mapped at high 64-bit addresses:

   (gdb) info sharedlibrary
   Cannot access memory at address 0xfff80001002011e0
   Cannot access memory at address 0xfff80001002011d8
   Cannot access memory at address 0xfff80001002011d8
   From                To                  Syms Read   Shared Object Library
   0xfff80001000010a0  0xfff8000100021f80  Yes (*)     /lib64/ld-linux.so.2
   (*): Shared library is missing debugging information.

Those addresses are 64-bit addresses with the high bits set.  When
interpreted as signed, they're negative.

The Linux kernel rejects pread64/pwrite64 if the offset argument of
type off_t (a signed type) is negative, which happens if the memory
address we're accessing has its high bit set.  See
linux/fs/read_write.c sys_pread64 and sys_pwrite64 in Linux.

Thankfully, lseek does not fail in that situation.  So the fix is to
use the 'lseek + read|write' path if the offset would be negative.

Fix this in both native GDB and GDBserver.

Tested on a SPARC64 GNU/Linux and x86-64 GNU/Linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30525
Change-Id: I79c724f918037ea67b7396fadb521bc9d1b10dc5
---
 gdb/linux-nat.c        | 32 ++++++++++++++++++++++----------
 gdbserver/linux-low.cc | 35 ++++++++++++++++++++++-------------
 2 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 383ef58fa23..86f3ebd3573 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -3909,18 +3909,30 @@ linux_proc_xfer_memory_partial_fd (int fd, int pid,
 
   gdb_assert (fd != -1);
 
-  /* Use pread64/pwrite64 if available, since they save a syscall and can
-     handle 64-bit offsets even on 32-bit platforms (for instance, SPARC
-     debugging a SPARC64 application).  */
+  /* Use pread64/pwrite64 if available, since they save a syscall and
+     can handle 64-bit offsets even on 32-bit platforms (for instance,
+     SPARC debugging a SPARC64 application).  But only use them if the
+     offset isn't so high that when cast to off_t it'd be negative, as
+     seen on SPARC64.  pread64/pwrite64 outright reject such offsets.
+     lseek does not.  */
 #ifdef HAVE_PREAD64
-  ret = (readbuf ? pread64 (fd, readbuf, len, offset)
-	 : pwrite64 (fd, writebuf, len, offset));
-#else
-  ret = lseek (fd, offset, SEEK_SET);
-  if (ret != -1)
-    ret = (readbuf ? read (fd, readbuf, len)
-	   : write (fd, writebuf, len));
+  if ((off_t) offset >= 0)
+    {
+      ret = (readbuf != nullptr
+	     ? pread64 (fd, readbuf, len, offset)
+	     : pwrite64 (fd, writebuf, len, offset));
+    }
+  else
 #endif
+    {
+      ret = lseek (fd, offset, SEEK_SET);
+      if (ret != -1)
+	{
+	  ret = (readbuf != nullptr
+		 ? read (fd, readbuf, len)
+		 : write (fd, writebuf, len));
+	}
+    }
 
   if (ret == -1)
     {
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 8ab16698632..f580e4b7977 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -5377,21 +5377,30 @@ proc_xfer_memory (CORE_ADDR memaddr, unsigned char *readbuf,
     {
       int bytes;
 
-      /* If pread64 is available, use it.  It's faster if the kernel
-	 supports it (only one syscall), and it's 64-bit safe even on
-	 32-bit platforms (for instance, SPARC debugging a SPARC64
-	 application).  */
+      /* Use pread64/pwrite64 if available, since they save a syscall
+	 and can handle 64-bit offsets even on 32-bit platforms (for
+	 instance, SPARC debugging a SPARC64 application).  But only
+	 use them if the offset isn't so high that when cast to off_t
+	 it'd be negative, as seen on SPARC64.  pread64/pwrite64
+	 outright reject such offsets.  lseek does not.  */
 #ifdef HAVE_PREAD64
-      bytes = (readbuf != nullptr
-	       ? pread64 (fd, readbuf, len, memaddr)
-	       : pwrite64 (fd, writebuf, len, memaddr));
-#else
-      bytes = -1;
-      if (lseek (fd, memaddr, SEEK_SET) != -1)
-	bytes = (readbuf != nullptr
-		 ? read (fd, readbuf, len)
-		 : write (fd, writebuf, len));
+      if ((off_t) memaddr >= 0)
+	{
+	  bytes = (readbuf != nullptr
+		   ? pread64 (fd, readbuf, len, memaddr)
+		   : pwrite64 (fd, writebuf, len, memaddr));
+	}
+      else
 #endif
+	{
+	  bytes = -1;
+	  if (lseek (fd, memaddr, SEEK_SET) != -1)
+	    {
+	      bytes = (readbuf != nullptr
+		       ? read (fd, readbuf, len)
+		       : write (fd, writebuf, len));
+	    }
+	}
 
       if (bytes < 0)
 	return errno;

base-commit: a5042543d819df8a76ebec8c4d715f244efbab0a
-- 
2.34.1

Re: [PATCH] Linux: Avoid pread64/pwrite64 for high memory addresses (PR gdb/30525)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> I just realized that the lseek path, which I just reindented, has an if branch
Pedro> with more than one line which isn't using braces...  The patch should at least be
Pedro> consistent in all the lines it touches...  :-)  If we follow the documented convention,
Pedro> then here's what we get.

Pedro> WDYT?  Should we tweak the convention instead?

I tend to interpret this rule as "two statements and/or comments", so
braces are required if there's a comment but not for a single statement
that happens to be broken up across multiple lines.

Tom

Re: [PATCH] Linux: Avoid pread64/pwrite64 for high memory addresses (PR gdb/30525)

[Pedro Alves]

On 2023-07-06 16:25, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:
> 
> Pedro> I just realized that the lseek path, which I just reindented, has an if branch
> Pedro> with more than one line which isn't using braces...  The patch should at least be
> Pedro> consistent in all the lines it touches...  :-)  If we follow the documented convention,
> Pedro> then here's what we get.
> 
> Pedro> WDYT?  Should we tweak the convention instead?
> 
> I tend to interpret this rule as "two statements and/or comments", so
> braces are required if there's a comment but not for a single statement
> that happens to be broken up across multiple lines.
> 

I don't see how the current wording leaves any room for interpretation, but
I could definitely see that the wording may have overreached the original intention.

I remembered this rule being discussed on the list, (but not the discussion itself),
so I did some digging, and found it.

It all started here:

  https://inbox.sourceware.org/gdb-patches/20111218115931.GA22952@host2.jankratochvil.net/

and then a few weeks later another similar discussion happened, which led Jan proposing
adding formalizing the convention:

  https://inbox.sourceware.org/gdb-patches/CADPb22Q9qi3TsYw3ZFkAyuO6a0VEJ_wtM_4pe_tXzDW5myi29Q@mail.gmail.com/

Curiously, Eli had pointed out the "comments or more ?" issue here:

  https://inbox.sourceware.org/gdb-patches/831ur4kot7.fsf@gnu.org/

but sadly there was no answer.

From what I can tell in those two threads, this was really only about the comments case.  I'll
drop the braces from my patch and merge it.

As for the text in the wiki, how about something like below.  (Note this tweaks the existing
example to use "cond" instead of "i", since we don't do implicit bool conversions, and "i"
looks like an int.)

```
Per the GNU coding standards, single statements are not wrapped in braces, while multiple statements are.

Thus, if you need to wrap a statement, write:

if (cond)
  func (foo (),
        bar ());

and not:

if (cond)
  {
    func (foo (),
          bar ());
  }

As a GDB-specific rule, statements preceded by comments should also be wrapped in braces, as they look like separate statements:

if (cond)
  {
    /* Return success.  */
    return 0;
  }

and not:

if (cond)
  /* Return success.  */
  return 0;
```


Oh the irony:

 https://inbox.sourceware.org/gdb-patches/4F1011EA.1030509@redhat.com/

:-)

Re: [PATCH] Linux: Avoid pread64/pwrite64 for high memory addresses (PR gdb/30525)

[Pedro Alves]

On 2023-07-06 17:47, Pedro Alves wrote:

> From what I can tell in those two threads, this was really only about the comments case.  I'll
> drop the braces from my patch and merge it.

Done, like so.  Just realized I forgot to add Andrew's tag.  Sorry about that...

From 31a56a22c45d76df4c597439f337e3f75ac3065c Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Wed, 7 Jun 2023 10:38:14 +0100
Subject: [PATCH] Linux: Avoid pread64/pwrite64 for high memory addresses (PR
 gdb/30525)

Since commit 05c06f318fd9 ("Linux: Access memory even if threads are
running"), GDB prefers pread64/pwrite64 to access inferior memory
instead of ptrace.  That change broke reading shared libraries on
SPARC64 Linux, as reported by PR gdb/30525 ("gdb cannot read shared
libraries on SPARC64").

On SPARC64 Linux, surprisingly (to me), userspace shared libraries are
mapped at high 64-bit addresses:

   (gdb) info sharedlibrary
   Cannot access memory at address 0xfff80001002011e0
   Cannot access memory at address 0xfff80001002011d8
   Cannot access memory at address 0xfff80001002011d8
   From                To                  Syms Read   Shared Object Library
   0xfff80001000010a0  0xfff8000100021f80  Yes (*)     /lib64/ld-linux.so.2
   (*): Shared library is missing debugging information.

Those addresses are 64-bit addresses with the high bits set.  When
interpreted as signed, they're negative.

The Linux kernel rejects pread64/pwrite64 if the offset argument of
type off_t (a signed type) is negative, which happens if the memory
address we're accessing has its high bit set.  See
linux/fs/read_write.c sys_pread64 and sys_pwrite64 in Linux.

Thankfully, lseek does not fail in that situation.  So the fix is to
use the 'lseek + read|write' path if the offset would be negative.

Fix this in both native GDB and GDBserver.

Tested on a SPARC64 GNU/Linux and x86-64 GNU/Linux.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30525
Change-Id: I79c724f918037ea67b7396fadb521bc9d1b10dc5
---
 gdb/linux-nat.c        | 28 ++++++++++++++++++----------
 gdbserver/linux-low.cc | 29 +++++++++++++++++------------
 2 files changed, 35 insertions(+), 22 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 383ef58fa23..4f7b6f8194f 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -3909,18 +3909,26 @@ linux_proc_xfer_memory_partial_fd (int fd, int pid,
 
   gdb_assert (fd != -1);
 
-  /* Use pread64/pwrite64 if available, since they save a syscall and can
-     handle 64-bit offsets even on 32-bit platforms (for instance, SPARC
-     debugging a SPARC64 application).  */
+  /* Use pread64/pwrite64 if available, since they save a syscall and
+     can handle 64-bit offsets even on 32-bit platforms (for instance,
+     SPARC debugging a SPARC64 application).  But only use them if the
+     offset isn't so high that when cast to off_t it'd be negative, as
+     seen on SPARC64.  pread64/pwrite64 outright reject such offsets.
+     lseek does not.  */
 #ifdef HAVE_PREAD64
-  ret = (readbuf ? pread64 (fd, readbuf, len, offset)
-	 : pwrite64 (fd, writebuf, len, offset));
-#else
-  ret = lseek (fd, offset, SEEK_SET);
-  if (ret != -1)
-    ret = (readbuf ? read (fd, readbuf, len)
-	   : write (fd, writebuf, len));
+  if ((off_t) offset >= 0)
+    ret = (readbuf != nullptr
+	   ? pread64 (fd, readbuf, len, offset)
+	   : pwrite64 (fd, writebuf, len, offset));
+  else
 #endif
+    {
+      ret = lseek (fd, offset, SEEK_SET);
+      if (ret != -1)
+	ret = (readbuf != nullptr
+	       ? read (fd, readbuf, len)
+	       : write (fd, writebuf, len));
+    }
 
   if (ret == -1)
     {
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 8ab16698632..651f219b738 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -5377,21 +5377,26 @@ proc_xfer_memory (CORE_ADDR memaddr, unsigned char *readbuf,
     {
       int bytes;
 
-      /* If pread64 is available, use it.  It's faster if the kernel
-	 supports it (only one syscall), and it's 64-bit safe even on
-	 32-bit platforms (for instance, SPARC debugging a SPARC64
-	 application).  */
+      /* Use pread64/pwrite64 if available, since they save a syscall
+	 and can handle 64-bit offsets even on 32-bit platforms (for
+	 instance, SPARC debugging a SPARC64 application).  But only
+	 use them if the offset isn't so high that when cast to off_t
+	 it'd be negative, as seen on SPARC64.  pread64/pwrite64
+	 outright reject such offsets.  lseek does not.  */
 #ifdef HAVE_PREAD64
-      bytes = (readbuf != nullptr
-	       ? pread64 (fd, readbuf, len, memaddr)
-	       : pwrite64 (fd, writebuf, len, memaddr));
-#else
-      bytes = -1;
-      if (lseek (fd, memaddr, SEEK_SET) != -1)
+      if ((off_t) memaddr >= 0)
 	bytes = (readbuf != nullptr
-		 ? read (fd, readbuf, len)
-		 : write (fd, writebuf, len));
+		 ? pread64 (fd, readbuf, len, memaddr)
+		 : pwrite64 (fd, writebuf, len, memaddr));
+      else
 #endif
+	{
+	  bytes = -1;
+	  if (lseek (fd, memaddr, SEEK_SET) != -1)
+	    bytes = (readbuf != nullptr
+		     ? read (fd, readbuf, len)
+		     : write (fd, writebuf, len));
+	}
 
       if (bytes < 0)
 	return errno;

base-commit: c0c3bb70f2f13e07295041cdf24a4d2997fe99a4
-- 
2.34.1

Re: [PATCH] Linux: Avoid pread64/pwrite64 for high memory addresses (PR gdb/30525)

[Tom Tromey]

> As for the text in the wiki, how about something like below.  (Note
> this tweaks the existing example to use "cond" instead of "i", since
> we don't do implicit bool conversions, and "i" looks like an int.)

Looks good to me.

> Oh the irony:
>  https://inbox.sourceware.org/gdb-patches/4F1011EA.1030509@redhat.com/
> :-)

A bikeshed never really has a final color.

Tom