From: Pedro Alves <pedro@palves.net>
Date: Tue, 7 Jul 2020 00:50:10 +0000 (+0100)
Subject: Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=1ad36a4b892fc4425d6f24c298713eeafece7b04;p=thirdparty%2Fbinutils-gdb.git

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.

gdb/ChangeLog:

	* thread.c
	(scoped_restore_current_thread::scoped_restore_current_thread):
	Incref the thread before calling get_frame_id instead of after.
	Let TARGET_CLOSE_ERROR propagate.
---

diff --git a/gdb/thread.c b/gdb/thread.c
index f0722d35888..a3c2be7dd0a 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1433,15 +1433,17 @@ scoped_restore_current_thread::~scoped_restore_current_thread ()
 
 scoped_restore_current_thread::scoped_restore_current_thread ()
 {
-  m_thread = NULL;
   m_inf = current_inferior ();
+  m_inf->incref ();
 
   if (inferior_ptid != null_ptid)
     {
-      thread_info *tp = inferior_thread ();
+      m_thread = inferior_thread ();
+      m_thread->incref ();
+
       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 +1468,18 @@ 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)
+	    {
+	      m_thread->decref ();
+	      m_inf->decref ();
+	      throw;
+	    }
+	}
     }
-
-  m_inf->incref ();
+  else
+    m_thread = NULL;
 }
 
 /* See gdbthread.h.  */