[2/3] Fix crash if connection drops in scoped_restore_current_thread's ctor, part 2

Message ID 20200708233125.1030-3-pedro@palves.net
State New
Headers show
Series
  • Fix crash if connection drops in scoped_restore_current_thread's ctor
Related show

Commit Message

Pedro Alves July 8, 2020, 11:31 p.m.
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.
---
 gdb/thread.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

-- 
2.14.5

Comments

Simon Marchi July 9, 2020, 3:31 a.m. | #1
On 2020-07-08 7:31 p.m., Pedro Alves wrote:
> 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.


So, when the target closes while we (scoped_restore_current_thread) own
a reference on the inferior and thread, the inferior and thread are still
destroyed, and so we shouldn't decref them?

Simon
Pedro Alves July 9, 2020, 11:12 a.m. | #2
On 7/9/20 4:31 AM, Simon Marchi wrote:
> On 2020-07-08 7:31 p.m., Pedro Alves wrote:

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

> 

> So, when the target closes while we (scoped_restore_current_thread) own

> a reference on the inferior and thread, the inferior and thread are still

> destroyed, and so we shouldn't decref them?


Aw, no, I got confused and misremembered how exceptions in ctors work.
The dtor for scoped_restore_current_thread isn't called, and I assumed
it was called.  We need to decref the inferior and thread before letting
the exception propagate, otherwise we leak them.  I'm testing the updated
version of the patch below, which does that:

	  /* Better let this propagate.  */
	  if (ex.error == TARGET_CLOSE_ERROR)
	    {
	      m_thread->decref ();
	      m_inf->decref ();
	      throw;
	    }

It passes multi-target.exp with Asan-enabled GDB.  Running the full
testsuite now.

From 1ad36a4b892fc4425d6f24c298713eeafece7b04 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>

Date: Tue, 7 Jul 2020 01:50:10 +0100
Subject: [PATCH] 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.
---
 gdb/thread.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/gdb/thread.c b/gdb/thread.c
index f0722d3588..a3c2be7dd0 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.  */

base-commit: ad8464f799a4c96c7ab8bdfec3f95846cf54f9b0
prerequisite-patch-id: 32ffdda7d7d774bc4df88bf848bcb796559b53ce
prerequisite-patch-id: 02021b74355b70debd344a6e445285c67dfef7d6
prerequisite-patch-id: c87fcf5a54f6805967cbf8ab107606c57d9ecf52
prerequisite-patch-id: ac7dee583d0ffa519c9d1cd89d27664bca68d8c1
prerequisite-patch-id: eac59ae2ea85d2d51e5be1b03e88a5641cc12c22
prerequisite-patch-id: 13da42ad04dc8e2e3bd6a556a0be0e17cf23669b
prerequisite-patch-id: fd3f09fdb58ddc1c595ea014716851f4c8fca48c
prerequisite-patch-id: 8230262cb24020a5b37e915843180e940807ba1f
-- 
2.14.5
Simon Marchi July 9, 2020, 2:16 p.m. | #3
On 2020-07-09 7:12 a.m., Pedro Alves wrote:
> Aw, no, I got confused and misremembered how exceptions in ctors work.

> The dtor for scoped_restore_current_thread isn't called, and I assumed

> it was called.  We need to decref the inferior and thread before letting

> the exception propagate, otherwise we leak them.  I'm testing the updated

> version of the patch below, which does that:

> 

> 	  /* Better let this propagate.  */

> 	  if (ex.error == TARGET_CLOSE_ERROR)

> 	    {

> 	      m_thread->decref ();

> 	      m_inf->decref ();

> 	      throw;

> 	    }

> 

> It passes multi-target.exp with Asan-enabled GDB.  Running the full

> testsuite now.


Ok, I also had the reasoning about exceptions thrown by constructors,
and thought it might be on purpose that it wasn't decref'ed.

If the references were kept by fields of scoped_restore_current_thread,
then it would work automatically, because their destructor would get
called.

Simon
Pedro Alves July 9, 2020, 5:23 p.m. | #4
On 7/9/20 3:16 PM, Simon Marchi wrote:
> On 2020-07-09 7:12 a.m., Pedro Alves wrote:

>> Aw, no, I got confused and misremembered how exceptions in ctors work.

>> The dtor for scoped_restore_current_thread isn't called, and I assumed

>> it was called.  We need to decref the inferior and thread before letting

>> the exception propagate, otherwise we leak them.  I'm testing the updated

>> version of the patch below, which does that:

>>

>> 	  /* Better let this propagate.  */

>> 	  if (ex.error == TARGET_CLOSE_ERROR)

>> 	    {

>> 	      m_thread->decref ();

>> 	      m_inf->decref ();

>> 	      throw;

>> 	    }

>>

>> It passes multi-target.exp with Asan-enabled GDB.  Running the full

>> testsuite now.

> 

> Ok, I also had the reasoning about exceptions thrown by constructors,

> and thought it might be on purpose that it wasn't decref'ed.

> 

> If the references were kept by fields of scoped_restore_current_thread,

> then it would work automatically, because their destructor would get

> called.


Yup.  I wasn't proposing that due to a header dependency issue which
I wasn't looking at addressing right now, but here's a version that
works around it.

From 55813e06b771787d3120f4a57f6e4f8ae25e5ebb Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>

Date: Thu, 9 Jul 2020 18:14:09 +0100
Subject: [PATCH] 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.
---
 gdb/gdbthread.h | 14 ++++++++++----
 gdb/thread.c    | 25 ++++++++++---------------
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 0166b2000f..ab5771fdb4 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -395,6 +395,13 @@ class thread_info : public refcounted_object
 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 @@ class scoped_restore_current_thread
   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 f0722d3588..4dce1ef82a 100644
--- 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.  */

base-commit: ad8464f799a4c96c7ab8bdfec3f95846cf54f9b0
prerequisite-patch-id: 32ffdda7d7d774bc4df88bf848bcb796559b53ce
prerequisite-patch-id: 02021b74355b70debd344a6e445285c67dfef7d6
prerequisite-patch-id: c87fcf5a54f6805967cbf8ab107606c57d9ecf52
prerequisite-patch-id: ac7dee583d0ffa519c9d1cd89d27664bca68d8c1
prerequisite-patch-id: eac59ae2ea85d2d51e5be1b03e88a5641cc12c22
prerequisite-patch-id: 13da42ad04dc8e2e3bd6a556a0be0e17cf23669b
prerequisite-patch-id: fd3f09fdb58ddc1c595ea014716851f4c8fca48c
prerequisite-patch-id: 8230262cb24020a5b37e915843180e940807ba1f
-- 
2.14.5
Simon Marchi July 9, 2020, 5:28 p.m. | #5
On 2020-07-09 1:23 p.m., Pedro Alves wrote:
> 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.


Cool, that's fine with me.

Simon

Patch

diff --git a/gdb/thread.c b/gdb/thread.c
index f0722d3588..1ec047e35b 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,14 @@  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 ();
+  else
+    m_thread = NULL;
 }
 
 /* See gdbthread.h.  */