Message ID | 20230705134141.1441753-1-pedro@palves.net |
---|---|
State | New |
Headers |
Return-Path: <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 169853856969 for <patchwork@sourceware.org>; Wed, 5 Jul 2023 13:42:07 +0000 (GMT) X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from mail-wr1-f53.google.com (mail-wr1-f53.google.com [209.85.221.53]) by sourceware.org (Postfix) with ESMTPS id 3AB653858421 for <gdb-patches@sourceware.org>; Wed, 5 Jul 2023 13:41:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 3AB653858421 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-wr1-f53.google.com with SMTP id ffacd0b85a97d-3143b70d6easo2943699f8f.2 for <gdb-patches@sourceware.org>; Wed, 05 Jul 2023 06:41:52 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688564511; x=1691156511; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=bhjxVg0EAWd4KXPzLZ3WRsQVFqcsG2T0nwRqXiXFhPY=; b=iCcGyldt4Q0opCuXBfE+ENvXco22pbKGWjUXzlT5go3pikKLke+G8nSzgcCBGyuJ22 wcCZ80bg2j8ch1Q4bKmw5/pCO0UnJ6YTqNW9fSnrbMpPq66lMQWPh5g0a/Vwbs7cY0H+ RBVoxY4+4C1IRIqLbctFtYkKzaLBYYr9LU5xiBatU4Wv240AWyQ3dZFViTUH2INkTY70 HFeuDg2l0IboO9nSJZLeItpaA5MkFpCDgt990pQiDdeW4RRqQizxYI5bsD6d2cTTTuxR B+D8YADqI3bczqNlf055jlsfozKWVX4qQ+pZ3kjZfmMVtgSwTOlXDNYFaB/Lqi+kLGyA 20HA== X-Gm-Message-State: ABy/qLaSUjT5h/wp1tYW1tsfDx+JfmSwb9saXkfSX3sNx/nfLnRP4Ljm 9Spv+q048MAVbv63vL9rOYGHOPdCh9U= X-Google-Smtp-Source: APBJJlGWajFgrPxOx1FCtpPCjGH9JzkYv5B2eD6KX1ZKYm5Gi7+vO/xFRT3JdJA8goXgF4G7azT9WQ== X-Received: by 2002:adf:efcf:0:b0:312:74a9:8259 with SMTP id i15-20020adfefcf000000b0031274a98259mr12502016wrp.71.1688564510489; Wed, 05 Jul 2023 06:41:50 -0700 (PDT) Received: from localhost ([2001:8a0:f91d:bc00:2f8d:8b13:e93:bcfc]) by smtp.gmail.com with UTF8SMTPSA id e6-20020adfe386000000b0031437299fafsm8323194wrm.34.2023.07.05.06.41.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 05 Jul 2023 06:41:50 -0700 (PDT) From: Pedro Alves <pedro@palves.net> To: gdb-patches@sourceware.org Cc: Matt Turner <mattst88@gmail.com> Subject: [PATCH] Linux: Avoid pread64/pwrite64 for high memory addresses (PR gdb/30525) Date: Wed, 5 Jul 2023 14:41:41 +0100 Message-Id: <20230705134141.1441753-1-pedro@palves.net> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list <gdb-patches.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/gdb-patches>, <mailto:gdb-patches-request@sourceware.org?subject=subscribe> Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" <gdb-patches-bounces+patchwork=sourceware.org@sourceware.org> |
Series |
Linux: Avoid pread64/pwrite64 for high memory addresses (PR gdb/30525)
|
|
Commit Message
Pedro Alves
July 5, 2023, 1:41 p.m. UTC
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(-) base-commit: a5042543d819df8a76ebec8c4d715f244efbab0a
Comments
Thank you!
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
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
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
>>>>> "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
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/ :-)
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
> 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
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;