This is the mail archive of the gdb-cvs@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[binutils-gdb] File I/O file handles after target closes


https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=20db9c52a2073246a5c46a0274569e10b317866f

commit 20db9c52a2073246a5c46a0274569e10b317866f
Author: Pedro Alves <palves@redhat.com>
Date:   Wed Apr 11 11:35:58 2018 +0100

    File I/O file handles after target closes
    
    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  <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.

Diff:
---
 gdb/ChangeLog | 10 ++++++++++
 gdb/remote.c  |  3 +--
 gdb/target.c  | 32 +++++++++++++++++++++++++++++---
 3 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f4bfe78..04754ac 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,15 @@
 2018-04-11  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.
+
+2018-04-11  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.
diff --git a/gdb/remote.c b/gdb/remote.c
index a46f1f8..f54a388 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 eec4e58..e8d4ae7 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)


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]