Skip to content

Commit

Permalink
File I/O file handles after target closes
Browse files Browse the repository at this point in the history
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:
2018-04-11  Pedro Alves  <[email protected]>

	* 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.
  • Loading branch information
palves committed Apr 11, 2018
1 parent 5ff7930 commit 20db9c5
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 5 deletions.
10 changes: 10 additions & 0 deletions gdb/ChangeLog
Original file line number Diff line number Diff line change
@@ -1,3 +1,13 @@
2018-04-11 Pedro Alves <[email protected]>

* 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.

2018-04-11 Pedro Alves <[email protected]>

* target.c (fileio_fh_t): Make it a named struct instead of a
Expand Down
3 changes: 1 addition & 2 deletions gdb/remote.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
32 changes: 29 additions & 3 deletions gdb/target.c
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
}

Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 20db9c5

Please sign in to comment.