[binutils-gdb] gdb/remote: fix use after free bug

[binutils-gdb] gdb/remote: fix use after free bug

[Andrew Burgess]

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

commit 7a34f66b23d459d81315a4f7e63549eaa2f9cf51
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Thu Dec 2 11:05:17 2021 +0000

    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:
---
 gdb/remote.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

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;