From 7a34f66b23d459d81315a4f7e63549eaa2f9cf51 Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Thu, 2 Dec 2021 11:05:17 +0000 Subject: [PATCH] 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. --- 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; -- 2.30.2