* [PATCH 2/2] File I/O file handles after target closes
2018-04-10 10:11 [PATCH 0/2] Invalid file I/O file handles when the target closes Pedro Alves
@ 2018-04-10 10:11 ` Pedro Alves
2018-04-10 10:11 ` [PATCH 1/2] C++ify fileio_fh_t, replace VEC with std::vector Pedro Alves
2018-04-11 3:33 ` [PATCH 0/2] Invalid file I/O file handles when the target closes Simon Marchi
2 siblings, 0 replies; 4+ messages in thread
From: Pedro Alves @ 2018-04-10 10:11 UTC (permalink / raw)
To: gdb-patches
A future patch will propose making the remote target's target_ops be
heap-allocated (to make it possible to have multiple instances of
remote targets, for multiple simultaneous connections), and will
delete/destroy the remote target at target_close time.
That change trips on a latent problem, though. File I/O handles
remain open even after the target is gone, with a dangling pointer to
a target that no longer exists. This results in GDB crashing when it
calls the target_ops backend associated with the file handle:
(gdb) Disconnect
Ending remote debugging.
* GDB crashes deferencing a dangling pointer
Backtrace:
#0 0x00007f79338570a0 in main_arena () at /lib64/libc.so.6
#1 0x0000000000858bfe in target_fileio_close(int, int*) (fd=1, target_errno=0x7ffe0499a4c8)
at src/gdb/target.c:2980
#2 0x00000000007088bd in gdb_bfd_iovec_fileio_close(bfd*, void*) (abfd=0x1a631b0, stream=0x223c9d0)
at src/gdb/gdb_bfd.c:353
#3 0x0000000000930906 in opncls_bclose (abfd=0x1a631b0) at src/bfd/opncls.c:528
#4 0x0000000000930cf9 in bfd_close_all_done (abfd=0x1a631b0) at src/bfd/opncls.c:768
#5 0x0000000000930cb3 in bfd_close (abfd=0x1a631b0) at src/bfd/opncls.c:735
#6 0x0000000000708dc5 in gdb_bfd_close_or_warn(bfd*) (abfd=0x1a631b0) at src/gdb/gdb_bfd.c:511
#7 0x00000000007091a2 in gdb_bfd_unref(bfd*) (abfd=0x1a631b0) at src/gdb/gdb_bfd.c:615
#8 0x000000000079ed8e in objfile::~objfile() (this=0x2154730, __in_chrg=<optimized out>)
at src/gdb/objfiles.c:682
#9 0x000000000079fd1a in objfile_purge_solibs() () at src/gdb/objfiles.c:1065
#10 0x00000000008162ca in no_shared_libraries(char const*, int) (ignored=0x0, from_tty=1)
at src/gdb/solib.c:1251
#11 0x000000000073b89b in disconnect_command(char const*, int) (args=0x0, from_tty=1)
at src/gdb/infcmd.c:3035
This goes unnoticed in current master, because the current remote
target's target_ops is never destroyed nowadays, so we end up calling:
remote_hostio_close -> remote_hostio_send_command
which gracefully fails with FILEIO_ENOSYS if remote_desc is NULL
(because the target is closed).
Fix this by invalidating a target's file I/O handles when the target
is closed.
With this change, remote_hostio_send_command no longer needs to handle the
case of being called with a closed remote target, originally added here:
<https://sourceware.org/ml/gdb-patches/2008-08/msg00359.html>.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* target.c (fileio_fh_t::t): Add comment.
(target_fileio_pwrite, target_fileio_pread, target_fileio_fstat)
(target_fileio_close): Handle a NULL target.
(invalidate_fileio_fh): New.
(target_close): Call it.
* remote.c (remote_hostio_send_command): No longer check whether
remote_desc is open.
---
gdb/remote.c | 3 +--
gdb/target.c | 32 +++++++++++++++++++++++++++++---
2 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index a46f1f8ddb..f54a38833b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -11350,8 +11350,7 @@ remote_hostio_send_command (int command_bytes, int which_packet,
int ret, bytes_read;
char *attachment_tmp;
- if (!rs->remote_desc
- || packet_support (which_packet) == PACKET_DISABLE)
+ if (packet_support (which_packet) == PACKET_DISABLE)
{
*remote_errno = FILEIO_ENOSYS;
return -1;
diff --git a/gdb/target.c b/gdb/target.c
index eec4e58d38..e8d4ae7ea8 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2793,7 +2793,8 @@ default_fileio_target (void)
struct fileio_fh_t
{
- /* The target on which this file is open. */
+ /* The target on which this file is open. NULL if the target is
+ meanwhile closed while the handle is open. */
target_ops *target;
/* The file descriptor on the target. */
@@ -2818,6 +2819,20 @@ static std::vector<fileio_fh_t> fileio_fhandles;
list each time a new file is opened. */
static int lowest_closed_fd;
+/* Invalidate the target associated with open handles that were open
+ on target TARG, since we're about to close (and maybe destroy) the
+ target. The handles remain open from the client's perspective, but
+ trying to do anything with them other than closing them will fail
+ with EIO. */
+
+static void
+fileio_handles_invalidate_target (target_ops *targ)
+{
+ for (fileio_fh_t &fh : fileio_fhandles)
+ if (fh.target == targ)
+ fh.target = NULL;
+}
+
/* Acquire a target fileio file descriptor. */
static int
@@ -2933,6 +2948,8 @@ target_fileio_pwrite (int fd, const gdb_byte *write_buf, int len,
if (fh->is_closed ())
*target_errno = EBADF;
+ else if (fh->target == NULL)
+ *target_errno = EIO;
else
ret = fh->target->to_fileio_pwrite (fh->target, fh->target_fd, write_buf,
len, offset, target_errno);
@@ -2957,6 +2974,8 @@ target_fileio_pread (int fd, gdb_byte *read_buf, int len,
if (fh->is_closed ())
*target_errno = EBADF;
+ else if (fh->target == NULL)
+ *target_errno = EIO;
else
ret = fh->target->to_fileio_pread (fh->target, fh->target_fd, read_buf,
len, offset, target_errno);
@@ -2980,6 +2999,8 @@ target_fileio_fstat (int fd, struct stat *sb, int *target_errno)
if (fh->is_closed ())
*target_errno = EBADF;
+ else if (fh->target == NULL)
+ *target_errno = EIO;
else
ret = fh->target->to_fileio_fstat (fh->target, fh->target_fd,
sb, target_errno);
@@ -3003,8 +3024,11 @@ target_fileio_close (int fd, int *target_errno)
*target_errno = EBADF;
else
{
- ret = fh->target->to_fileio_close (fh->target, fh->target_fd,
- target_errno);
+ if (fh->target != NULL)
+ ret = fh->target->to_fileio_close (fh->target, fh->target_fd,
+ target_errno);
+ else
+ ret = 0;
release_fileio_fd (fd, fh);
}
@@ -3390,6 +3414,8 @@ target_close (struct target_ops *targ)
{
gdb_assert (!target_is_pushed (targ));
+ fileio_handles_invalidate_target (targ);
+
if (targ->to_xclose != NULL)
targ->to_xclose (targ);
else if (targ->to_close != NULL)
--
2.14.3
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH 1/2] C++ify fileio_fh_t, replace VEC with std::vector
2018-04-10 10:11 [PATCH 0/2] Invalid file I/O file handles when the target closes Pedro Alves
2018-04-10 10:11 ` [PATCH 2/2] File I/O file handles after " Pedro Alves
@ 2018-04-10 10:11 ` Pedro Alves
2018-04-11 3:33 ` [PATCH 0/2] Invalid file I/O file handles when the target closes Simon Marchi
2 siblings, 0 replies; 4+ messages in thread
From: Pedro Alves @ 2018-04-10 10:11 UTC (permalink / raw)
To: gdb-patches
Preparation for the next patch.
- Replace VEC with std::vector.
- Rewrite a couple macros as methods/functions.
- While at it, rename fileio_fh_t::fd as fileio_fh_t::target_fd to
avoid confusion between target and host file descriptors.
gdb/ChangeLog:
yyyy-mm-dd Pedro Alves <palves@redhat.com>
* target.c (fileio_fh_t): Make it a named struct instead of a
typedef.
(fileio_fh_t::is_closed): New method.
(DEF_VEC_O (fileio_fh_t)): Remove.
(fileio_fhandles): Now a std::vector.
(is_closed_fileio_fh): Delete.
(acquire_fileio_fd): Adjust. Rename parameters.
(release_fileio_fd): Adjust.
(fileio_fd_to_fh): Reimplement as a function instead of a macro.
(target_fileio_pwrite, target_fileio_pread, target_fileio_fstat)
(target_fileio_close): Adjust.
---
gdb/target.c | 80 +++++++++++++++++++++++++++++++-----------------------------
1 file changed, 42 insertions(+), 38 deletions(-)
diff --git a/gdb/target.c b/gdb/target.c
index 8acdd8092c..eec4e58d38 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2791,26 +2791,27 @@ default_fileio_target (void)
/* File handle for target file operations. */
-typedef struct
+struct fileio_fh_t
{
/* The target on which this file is open. */
- struct target_ops *t;
+ target_ops *target;
/* The file descriptor on the target. */
- int fd;
-} fileio_fh_t;
+ int target_fd;
-DEF_VEC_O (fileio_fh_t);
+ /* Check whether this fileio_fh_t represents a closed file. */
+ bool is_closed ()
+ {
+ return target_fd < 0;
+ }
+};
/* Vector of currently open file handles. The value returned by
target_fileio_open and passed as the FD argument to other
target_fileio_* functions is an index into this vector. This
vector's entries are never freed; instead, files are marked as
closed, and the handle becomes available for reuse. */
-static VEC (fileio_fh_t) *fileio_fhandles;
-
-/* Macro to check whether a fileio_fh_t represents a closed file. */
-#define is_closed_fileio_fh(fd) ((fd) < 0)
+static std::vector<fileio_fh_t> fileio_fhandles;
/* Index into fileio_fhandles of the lowest handle that might be
closed. This permits handle reuse without searching the whole
@@ -2820,27 +2821,25 @@ static int lowest_closed_fd;
/* Acquire a target fileio file descriptor. */
static int
-acquire_fileio_fd (struct target_ops *t, int fd)
+acquire_fileio_fd (target_ops *target, int target_fd)
{
- fileio_fh_t *fh;
-
- gdb_assert (!is_closed_fileio_fh (fd));
-
/* Search for closed handles to reuse. */
- for (;
- VEC_iterate (fileio_fh_t, fileio_fhandles,
- lowest_closed_fd, fh);
- lowest_closed_fd++)
- if (is_closed_fileio_fh (fh->fd))
- break;
+ for (; lowest_closed_fd < fileio_fhandles.size (); lowest_closed_fd++)
+ {
+ fileio_fh_t &fh = fileio_fhandles[lowest_closed_fd];
+
+ if (fh.is_closed ())
+ break;
+ }
/* Push a new handle if no closed handles were found. */
- if (lowest_closed_fd == VEC_length (fileio_fh_t, fileio_fhandles))
- fh = VEC_safe_push (fileio_fh_t, fileio_fhandles, NULL);
+ if (lowest_closed_fd == fileio_fhandles.size ())
+ fileio_fhandles.push_back (fileio_fh_t {target, target_fd});
+ else
+ fileio_fhandles[lowest_closed_fd] = {target, target_fd};
- /* Fill in the handle. */
- fh->t = t;
- fh->fd = fd;
+ /* Should no longer be marked closed. */
+ gdb_assert (!fileio_fhandles[lowest_closed_fd].is_closed ());
/* Return its index, and start the next lookup at
the next index. */
@@ -2852,14 +2851,17 @@ acquire_fileio_fd (struct target_ops *t, int fd)
static void
release_fileio_fd (int fd, fileio_fh_t *fh)
{
- fh->fd = -1;
+ fh->target_fd = -1;
lowest_closed_fd = std::min (lowest_closed_fd, fd);
}
/* Return a pointer to the fileio_fhandle_t corresponding to FD. */
-#define fileio_fd_to_fh(fd) \
- VEC_index (fileio_fh_t, fileio_fhandles, (fd))
+static fileio_fh_t *
+fileio_fd_to_fh (int fd)
+{
+ return &fileio_fhandles[fd];
+}
/* Helper for target_fileio_open and
target_fileio_open_warn_if_slow. */
@@ -2929,11 +2931,11 @@ target_fileio_pwrite (int fd, const gdb_byte *write_buf, int len,
fileio_fh_t *fh = fileio_fd_to_fh (fd);
int ret = -1;
- if (is_closed_fileio_fh (fh->fd))
+ if (fh->is_closed ())
*target_errno = EBADF;
else
- ret = fh->t->to_fileio_pwrite (fh->t, fh->fd, write_buf,
- len, offset, target_errno);
+ ret = fh->target->to_fileio_pwrite (fh->target, fh->target_fd, write_buf,
+ len, offset, target_errno);
if (targetdebug)
fprintf_unfiltered (gdb_stdlog,
@@ -2953,11 +2955,11 @@ target_fileio_pread (int fd, gdb_byte *read_buf, int len,
fileio_fh_t *fh = fileio_fd_to_fh (fd);
int ret = -1;
- if (is_closed_fileio_fh (fh->fd))
+ if (fh->is_closed ())
*target_errno = EBADF;
else
- ret = fh->t->to_fileio_pread (fh->t, fh->fd, read_buf,
- len, offset, target_errno);
+ ret = fh->target->to_fileio_pread (fh->target, fh->target_fd, read_buf,
+ len, offset, target_errno);
if (targetdebug)
fprintf_unfiltered (gdb_stdlog,
@@ -2976,10 +2978,11 @@ target_fileio_fstat (int fd, struct stat *sb, int *target_errno)
fileio_fh_t *fh = fileio_fd_to_fh (fd);
int ret = -1;
- if (is_closed_fileio_fh (fh->fd))
+ if (fh->is_closed ())
*target_errno = EBADF;
else
- ret = fh->t->to_fileio_fstat (fh->t, fh->fd, sb, target_errno);
+ ret = fh->target->to_fileio_fstat (fh->target, fh->target_fd,
+ sb, target_errno);
if (targetdebug)
fprintf_unfiltered (gdb_stdlog,
@@ -2996,11 +2999,12 @@ target_fileio_close (int fd, int *target_errno)
fileio_fh_t *fh = fileio_fd_to_fh (fd);
int ret = -1;
- if (is_closed_fileio_fh (fh->fd))
+ if (fh->is_closed ())
*target_errno = EBADF;
else
{
- ret = fh->t->to_fileio_close (fh->t, fh->fd, target_errno);
+ ret = fh->target->to_fileio_close (fh->target, fh->target_fd,
+ target_errno);
release_fileio_fd (fd, fh);
}
--
2.14.3
^ permalink raw reply [flat|nested] 4+ messages in thread