Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2

Running the testsuite against an Asan-enabled build of GDB makes
gdb.base/multi-target.exp expose this bug.

scoped_restore_current_thread's ctor calls get_frame_id to record the
selected frame's ID to restore later.  If the frame ID hasn't been
computed yet, it will be computed on the spot, and that will usually
require accessing the target's memory and registers.  If the remote
connection closes, while we're computing the frame ID, the remote
target exits its inferiors, unpushes itself, and throws a
TARGET_CLOSE_ERROR error.  Exiting the inferiors deletes the
inferior's threads.

scoped_restore_current_thread increments the current thread's refcount
to prevent the thread from being deleted from under its feet.
However, the code that does that isn't considering the case of the
thread being deleted from within get_frame_id.  It only increments the
refcount _after_ get_frame_id returns.  So if the current thread is
indeed deleted, the

     tp->incref ();

statement references a stale TP pointer.

Incrementing the refcounts earlier fixes it.

We should probably also let the TARGET_CLOSE_ERROR error propagate in
this case.  That alone would fix it, though it seems better to tweak
the refcount handling too.  And to avoid having to manually decref
before throwing, convert to use gdb::ref_ptr.

Unfortunately, we can't define inferior_ref in inferior.h and then use
it in scoped_restore_current_thread, because
scoped_restore_current_thread is defined before inferior is
(inferior.h includes gdbthread.h).  To break that dependency, we would
have to move scoped_restore_current_thread to its own header.  I'm not
doing that here.

gdb/ChangeLog:

	* gdbthread.h (inferior_ref): Define.
	(scoped_restore_current_thread) <m_thread>: Now a thread_info_ref.
	(scoped_restore_current_thread) <m_inf>: Now an inferior_ref.
	* thread.c
	(scoped_restore_current_thread::restore):
	Adjust to gdb::ref_ptr.
	(scoped_restore_current_thread::~scoped_restore_current_thread):
	Remove manual decref handling.
	(scoped_restore_current_thread::scoped_restore_current_thread):
	Adjust to use
	inferior_ref::new_reference/thread_info_ref::new_reference.
	Incref the thread before calling get_frame_id instead of after.
	Let TARGET_CLOSE_ERROR propagate.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 3cf2cf10bf93778d09f124ad966a958094366222..aafefd7a9252f3c1d14a46a04a7f2439ac3a32dc 100644 (file)
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,19 @@
+2020-07-10  Pedro Alves  <pedro@palves.net>
+
+       * gdbthread.h (inferior_ref): Define.
+       (scoped_restore_current_thread) <m_thread>: Now a thread_info_ref.
+       (scoped_restore_current_thread) <m_inf>: Now an inferior_ref.
+       * thread.c
+       (scoped_restore_current_thread::restore):
+       Adjust to gdb::ref_ptr.
+       (scoped_restore_current_thread::~scoped_restore_current_thread):
+       Remove manual decref handling.
+       (scoped_restore_current_thread::scoped_restore_current_thread):
+       Adjust to use
+       inferior_ref::new_reference/thread_info_ref::new_reference.
+       Incref the thread before calling get_frame_id instead of after.
+       Let TARGET_CLOSE_ERROR propagate.
+
 2020-07-10  Pedro Alves  <pedro@palves.net>
 
        * frame-tailcall.c (dwarf2_tailcall_sniffer_first): Only swallow

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 0166b2000fb14291fa3d1e2113410b1a5de849f9..ab5771fdb47c25c0bbf93becaebf1d27c62d987e 100644 (file)
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -395,6 +395,13 @@ public:
 using thread_info_ref
   = gdb::ref_ptr<struct thread_info, refcounted_object_ref_policy>;
 
+/* A gdb::ref_ptr pointer to an inferior.  This would ideally be in
+   inferior.h, but it can't due to header dependencies (inferior.h
+   includes gdbthread.h).  */
+
+using inferior_ref
+  = gdb::ref_ptr<struct inferior, refcounted_object_ref_policy>;
+
 /* Create an empty thread list, or empty the existing one.  */
 extern void init_thread_list (void);
 
@@ -660,10 +667,9 @@ private:
   void restore ();
 
   bool m_dont_restore = false;
-  /* Use the "class" keyword here, because of a clash with a "thread_info"
-     function in the Darwin API.  */
-  class thread_info *m_thread;
-  inferior *m_inf;
+  thread_info_ref m_thread;
+  inferior_ref m_inf;
+
   frame_id m_selected_frame_id;
   int m_selected_frame_level;
   bool m_was_stopped;

diff --git a/gdb/thread.c b/gdb/thread.c
index f0722d358888550d5786a3ff532717e07cf92846..4dce1ef82aaf5ad2dde8ea690f09047da22607ef 100644 (file)
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1396,9 +1396,9 @@ scoped_restore_current_thread::restore ()
         in the mean time exited (or killed, detached, etc.), then don't revert
         back to it, but instead simply drop back to no thread selected.  */
       && m_inf->pid != 0)
-    switch_to_thread (m_thread);
+    switch_to_thread (m_thread.get ());
   else
-    switch_to_inferior_no_thread (m_inf);
+    switch_to_inferior_no_thread (m_inf.get ());
 
   /* The running state of the originally selected thread may have
      changed, so we have to recheck it here.  */
@@ -1425,23 +1425,19 @@ scoped_restore_current_thread::~scoped_restore_current_thread ()
             but swallow the exception.  */
        }
     }
-
-  if (m_thread != NULL)
-    m_thread->decref ();
-  m_inf->decref ();
 }
 
 scoped_restore_current_thread::scoped_restore_current_thread ()
 {
-  m_thread = NULL;
-  m_inf = current_inferior ();
+  m_inf = inferior_ref::new_reference (current_inferior ());
 
   if (inferior_ptid != null_ptid)
     {
-      thread_info *tp = inferior_thread ();
+      m_thread = thread_info_ref::new_reference (inferior_thread ());
+
       struct frame_info *frame;
 
-      m_was_stopped = tp->state == THREAD_STOPPED;
+      m_was_stopped = m_thread->state == THREAD_STOPPED;
       if (m_was_stopped
          && target_has_registers
          && target_has_stack
@@ -1466,13 +1462,12 @@ scoped_restore_current_thread::scoped_restore_current_thread ()
        {
          m_selected_frame_id = null_frame_id;
          m_selected_frame_level = -1;
-       }
 
-      tp->incref ();
-      m_thread = tp;
+         /* Better let this propagate.  */
+         if (ex.error == TARGET_CLOSE_ERROR)
+           throw;
+       }
     }
-
-  m_inf->incref ();
 }
 
 /* See gdbthread.h.  */