From: Andrew Burgess <aburgess@redhat.com>
Date: Thu, 2 Dec 2021 11:05:17 +0000 (+0000)
Subject: gdb/remote: fix use after free bug
X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=7a34f66b23d459d81315a4f7e63549eaa2f9cf51;p=binutils-gdb.git

gdb/remote: fix use after free bug

This commit:

  commit 288712bbaca36bff6578bc839ebcdc3707662f81
  Date:   Mon Nov 22 15:16:27 2021 +0000

      gdb/remote: use scoped_restore to control starting_up flag

introduced a use after free bug.  The scoped restore added in the
above commit resets a flag within a remote_target's remote_state
object.

However, in some situations, the remote_target can be unpushed before
the error is thrown.  If the only reference to the target is the one
in the target stack, then unpushing the target will cause the
remote_target to be deleted, which, in turn, will delete the
remote_state object.  The scoped restore will then try to reset the
flag within a deleted object.

This problem was caught in the gdb.server/server-connect.exp test,
which, when run with the address sanitizer enabled, highlights the
write after free bug described above.

This commit resolves this issue by adding a new class specifically for
the purpose of managing the starting_up flag.  As well as setting, and
then clearing the starting_up flag, this new class increments, and
then decrements the reference count on the remote_target object.  This
prevents the remote_target from being deleted until after the flag has
been reset.

The gdb.server/server-connect.exp now runs cleanly with the address
sanitizer enabled.
---

diff --git a/gdb/remote.c b/gdb/remote.c
index f53e31e126e..ebbc138b405 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4673,6 +4673,38 @@ remote_target::process_initial_stop_replies (int from_tty)
     }
 }
 
+/* Mark a remote_target as marking (by setting the starting_up flag within
+   its remote_state) for the lifetime of this object.  The reference count
+   on the remote target is temporarily incremented, to prevent the target
+   being deleted under our feet.  */
+
+struct scoped_mark_target_starting
+{
+  /* Constructor, TARGET is the target to be marked as starting, its
+     reference count will be incremented.  */
+  scoped_mark_target_starting (remote_target *target)
+    : m_remote_target (target)
+  {
+    m_remote_target->incref ();
+    remote_state *rs = m_remote_target->get_remote_state ();
+    rs->starting_up = true;
+  }
+
+  /* Destructor, mark the target being worked on as no longer starting, and
+     decrement the reference count.  */
+  ~scoped_mark_target_starting ()
+  {
+    remote_state *rs = m_remote_target->get_remote_state ();
+    rs->starting_up = false;
+    decref_target (m_remote_target);
+  }
+
+private:
+
+  /* The target on which we are operating.  */
+  remote_target *m_remote_target;
+};
+
 /* Helper for remote_target::start_remote, start the remote connection and
    sync state.  Return true if everything goes OK, otherwise, return false.
    This function exists so that the scoped_restore created within it will
@@ -4692,8 +4724,7 @@ remote_target::start_remote_1 (int from_tty, int extended_p)
      Ctrl-C before we're connected and synced up can't interrupt the
      target.  Instead, it offers to drop the (potentially wedged)
      connection.  */
-  scoped_restore restore_starting_up_flag
-    = make_scoped_restore (&rs->starting_up, true);
+  scoped_mark_target_starting target_is_starting (this);
 
   QUIT;