public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Andrew Burgess <aburgess@redhat.com>
To: gdb-patches@sourceware.org
Cc: Andrew Burgess <aburgess@redhat.com>
Subject: [PATCH 1/8] gdb/fileio: fix errno for packets where an attachment is expected
Date: Wed,  5 Jun 2024 14:15:08 +0100	[thread overview]
Message-ID: <ab03d77877b5ee6b39a6cd8efda9adc168ccca00.1717592684.git.aburgess@redhat.com> (raw)
In-Reply-To: <cover.1717592683.git.aburgess@redhat.com>

In remote.c lives remote_target::remote_hostio_send_command(), which
is used to handle sending a fileio packet to the remote, and for
parsing the reply packet.

Some commands, e.g. open, pwrite, close, send some arguments to the
remote, and then get back a single integer return value.

Other commands though, e.g. pread, readlink, fstat, send some
arguments and get back an integer return value and some additional
data.  This additional data is called the attachment.

Except, we only get the attachment if the command completes
successfully.  For example, calling readlink with a non existent path
will result in a return packet: 'F-1,2' with no attachment.  This is
as expected.

Within remote_hostio_send_command we call remote_hostio_parse_result,
this parses the status code (-1 in our example above) and
then parses the errno value (2 in our example above).

Back in remote_hostio_parse_result we then hit this block:

  /* Make sure we saw an attachment if and only if we expected one.  */
  if ((attachment_tmp == NULL && attachment != NULL)
      || (attachment_tmp != NULL && attachment == NULL))
    {
      *remote_errno = FILEIO_EINVAL;
      return -1;
    }

Which ensures that commands that expect an attachment, got an
attachment.

The problem is, we'll only get an attachment if the command
succeeded.  If it didn't, then there is no attachment, and that is as
expected.

As remote_hostio_parse_result always sets the returned error number to
FILEIO_SUCCESS unless the packet contained an actual error
number (e.g. 2 in our example above), I suggest we should return early
if remote_hostio_parse_result indicates an error packet.

I ran into this issue while working on another patch.  In that patch I
was checking the error code returned by a remote readlink call and
spotted that when I passed an invalid path I got EINVAL instead of
ENOENT.  This patch fixes this issue.

Unfortunately the patch I was working on evolved, and my need to check
the error code went away, and so, right now, I have no way to reveal
this bug.  But I think this is an obviously correct fix, and worth
merging even without a test.
---
 gdb/remote.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gdb/remote.c b/gdb/remote.c
index 349bc8cf005..241803ab2b3 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -12663,6 +12663,9 @@ remote_target::remote_hostio_send_command (int command_bytes, int which_packet,
       return -1;
     }
 
+  if (*remote_errno != FILEIO_SUCCESS)
+    return -1;
+
   /* Make sure we saw an attachment if and only if we expected one.  */
   if ((attachment_tmp == NULL && attachment != NULL)
       || (attachment_tmp != NULL && attachment == NULL))
-- 
2.25.4


  reply	other threads:[~2024-06-05 13:15 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-06-05 13:15 [PATCH 0/8] Extension for looking up debug info by build-id Andrew Burgess
2024-06-05 13:15 ` Andrew Burgess [this message]
2024-06-06 16:49   ` [PATCH 1/8] gdb/fileio: fix errno for packets where an attachment is expected Tom Tromey
2024-06-11 19:52     ` Andrew Burgess
2024-06-05 13:15 ` [PATCH 2/8] gdb: avoid duplicate search in build_id_to_bfd_suffix Andrew Burgess
2024-06-06 16:49   ` Tom Tromey
2024-06-11 19:52     ` Andrew Burgess
2024-06-05 13:15 ` [PATCH 3/8] gdb: warn of slow remote file reading only after a successful open Andrew Burgess
2024-06-06 16:58   ` Tom Tromey
2024-06-11 19:52     ` Andrew Burgess
2024-06-05 13:15 ` [PATCH 4/8] gdb: convert separate-debug-file to new(ish) debug scheme Andrew Burgess
2024-06-06 16:54   ` Tom Tromey
2024-06-11 19:52     ` Andrew Burgess
2024-06-05 13:15 ` [PATCH 5/8] gdb: add target_fileio_stat, but no implementations yet Andrew Burgess
2024-06-05 13:15 ` [PATCH 6/8] gdb: add GDB side target_ops::fileio_stat implementation Andrew Burgess
2024-06-05 14:21   ` Eli Zaretskii
2024-06-05 13:15 ` [PATCH 7/8] gdbserver: add gdbserver support for vFile::stat packet Andrew Burgess
2024-06-05 13:15 ` [PATCH 8/8] gdb: check for multiple matching build-id files Andrew Burgess
2024-06-13 10:29 ` [PATCH 0/8] Extension for looking up debug info by build-id Andrew Burgess
2024-06-13 10:29   ` [PATCHv2 1/4] gdb: add target_fileio_stat, but no implementations yet Andrew Burgess
2024-06-13 10:29   ` [PATCHv2 2/4] gdb: add GDB side target_ops::fileio_stat implementation Andrew Burgess
2024-06-13 10:29   ` [PATCHv2 3/4] gdbserver: add gdbserver support for vFile::stat packet Andrew Burgess
2024-06-13 10:29   ` [PATCHv2 4/4] gdb: check for multiple matching build-id files Andrew Burgess

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ab03d77877b5ee6b39a6cd8efda9adc168ccca00.1717592684.git.aburgess@redhat.com \
    --to=aburgess@redhat.com \
    --cc=gdb-patches@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).