gdb: hold a target_ops_ref in scoped_finish_thread_state

  This commit fixes a use after free issue that was reported here:

  https://inbox.sourceware.org/gdb-patches/68354b98-795a-4b50-9eac-e54aa1d01b9d@simark.ca

This issue was exposed by the gdb.replay/missing-thread.exp test that
was added in this commit:

  commit 8bd08ee92c4a7bf2ad9e29c4da32a276ef2257fc
  Date:   Fri May 16 17:56:58 2025 +0100

      gdb: crash if thread unexpectedly disappears from thread list

It is worth pointing out that the use after free issue existed before
this commit, this commit just introduced a test that exposed the issue
when GDB is run with the address sanitizer.

It has taken a while to get this fix ready for upstream as this fix
depended on the recently committed patch:

  commit 43db8f70d86b2492b79f59342187b919fd58b3dd
  Date:   Thu Oct 23 16:34:20 2025 +0100

      gdbsupport: remove undefined behaviour from (forward_)scope_exit

The problem is that the first commit above introduces a test which
causes the remote target to disconnect while processing an inferior
stop event, specifically, within normal_stop (infrun.c), GDB calls
update_thread_list, and it is during this call that the inferior
disconnects.

When the remote target disconnects, GDB immediately unpushes the
remote target.  See remote_unpush_target and its uses in remote.c.

If this is the last use of the remote target, then unpushing it will
cause the target to be deleted.

This is a problem, because in normal_stop, we have an RAII variable
maybe_finish_thread_state, which is an optional
scoped_finish_thread_state, and in some cases, this will hold a
pointer to the process_startum_target which needs to be finished.

So the order of events is:

  1. Call to normal_stop.

  2. Create maybe_finish_thread_state with a pointer to the current
     remote_target object.

  3. Call update_thread_list.

  4. Remote disconnects, GDB unpushes and deletes the current
     remote_target object.  GDB throws an exception.

  5. The exception propagates back to normal_stop.

  6. The destructor for maybe_finish_thread_state runs, and tries to
     make use of its cached pointer to the (now deleted) remote_target
     object.  Badness ensues.

This bug isn't restricted to normal_stop.  If a remote target
disconnects anywhere where there is a scoped_finish_thread_state in
the call stack then this issue could arise.

I think what we need to do is to ensure that the remote_target is not
actually deleted until after the scoped_finish_thread_state has been
cleaned up.

And so, to achieve this, I propose changing scoped_finish_thread_state
to hold a target_ops_ref rather than a pointer to the target_ops
object.  Holding the reference will prevent the object from being
deleted.

The new scoped_finish_thread_state is defined within its own file, and
is a drop in replacement for the existing class.

On my local machine the gdb.replay/missing-thread.exp test passes
cleanly after this commit (with address sanitizers), but when I test
on some other machines with a more recent Fedora install, I'm still
seeing test failures (both before and after this patch), though not
relating to the address sanitizer (at least, I don't see an error from
the sanitizer).  I don't think these other issues are directly related
to the problem being addressed in this commit, and so I'm proposing
this patch for inclusion anyway.  I'll continue to look at the test
and see if I can fix the other failures too.  Or maybe I'll end up
having to back out the test.
---
 gdb/finish-thread-state.h | 64 +++++++++++++++++++++++++++++++++++++++
 gdb/gdbthread.h           |  5 ---
 gdb/infcmd.c              |  1 +
 gdb/infrun.c              |  1 +
 gdb/remote.c              |  1 +
 5 files changed, 67 insertions(+), 5 deletions(-)
 create mode 100644 gdb/finish-thread-state.h


base-commit: 5d8562e89a612f2d0e5c9e0813a0d37620eff78c
  

On 12/23/25 1:48 PM, Andrew Burgess wrote:
> This commit fixes a use after free issue that was reported here:
>
>    https://inbox.sourceware.org/gdb-patches/68354b98-795a-4b50-9eac-e54aa1d01b9d@simark.ca
>
> This issue was exposed by the gdb.replay/missing-thread.exp test that
> was added in this commit:
>
>    commit 8bd08ee92c4a7bf2ad9e29c4da32a276ef2257fc
>    Date:   Fri May 16 17:56:58 2025 +0100
>
>        gdb: crash if thread unexpectedly disappears from thread list
>
> It is worth pointing out that the use after free issue existed before
> this commit, this commit just introduced a test that exposed the issue
> when GDB is run with the address sanitizer.
>
> It has taken a while to get this fix ready for upstream as this fix
> depended on the recently committed patch:
>
>    commit 43db8f70d86b2492b79f59342187b919fd58b3dd
>    Date:   Thu Oct 23 16:34:20 2025 +0100
>
>        gdbsupport: remove undefined behaviour from (forward_)scope_exit
>
> The problem is that the first commit above introduces a test which
> causes the remote target to disconnect while processing an inferior
> stop event, specifically, within normal_stop (infrun.c), GDB calls
> update_thread_list, and it is during this call that the inferior
> disconnects.
>
> When the remote target disconnects, GDB immediately unpushes the
> remote target.  See remote_unpush_target and its uses in remote.c.
>
> If this is the last use of the remote target, then unpushing it will
> cause the target to be deleted.
>
> This is a problem, because in normal_stop, we have an RAII variable
> maybe_finish_thread_state, which is an optional
> scoped_finish_thread_state, and in some cases, this will hold a
> pointer to the process_startum_target which needs to be finished.
>
> So the order of events is:
>
>    1. Call to normal_stop.
>
>    2. Create maybe_finish_thread_state with a pointer to the current
>       remote_target object.
>
>    3. Call update_thread_list.
>
>    4. Remote disconnects, GDB unpushes and deletes the current
>       remote_target object.  GDB throws an exception.
>
>    5. The exception propagates back to normal_stop.
>
>    6. The destructor for maybe_finish_thread_state runs, and tries to
>       make use of its cached pointer to the (now deleted) remote_target
>       object.  Badness ensues.
>
> This bug isn't restricted to normal_stop.  If a remote target
> disconnects anywhere where there is a scoped_finish_thread_state in
> the call stack then this issue could arise.
>
> I think what we need to do is to ensure that the remote_target is not
> actually deleted until after the scoped_finish_thread_state has been
> cleaned up.
>
> And so, to achieve this, I propose changing scoped_finish_thread_state
> to hold a target_ops_ref rather than a pointer to the target_ops
> object.  Holding the reference will prevent the object from being
> deleted.
>
> The new scoped_finish_thread_state is defined within its own file, and
> is a drop in replacement for the existing class.
>
> On my local machine the gdb.replay/missing-thread.exp test passes
> cleanly after this commit (with address sanitizers), but when I test
> on some other machines with a more recent Fedora install, I'm still
> seeing test failures (both before and after this patch), though not
> relating to the address sanitizer (at least, I don't see an error from
> the sanitizer).  I don't think these other issues are directly related
> to the problem being addressed in this commit, and so I'm proposing
> this patch for inclusion anyway.  I'll continue to look at the test
> and see if I can fix the other failures too.  Or maybe I'll end up
> having to back out the test.

Hi!

Thanks for the great explanation. I think this makes sense, and as much 
as I could follow the code, it looks pretty straight-forward

Reviewed-By: Guinevere Larsen <guinevere@redhat.com>
  

On 12/23/25 11:48 AM, Andrew Burgess wrote:
> This commit fixes a use after free issue that was reported here:
> 
>   https://inbox.sourceware.org/gdb-patches/68354b98-795a-4b50-9eac-e54aa1d01b9d@simark.ca
> 
> This issue was exposed by the gdb.replay/missing-thread.exp test that
> was added in this commit:
> 
>   commit 8bd08ee92c4a7bf2ad9e29c4da32a276ef2257fc
>   Date:   Fri May 16 17:56:58 2025 +0100
> 
>       gdb: crash if thread unexpectedly disappears from thread list
> 
> It is worth pointing out that the use after free issue existed before
> this commit, this commit just introduced a test that exposed the issue
> when GDB is run with the address sanitizer.
> 
> It has taken a while to get this fix ready for upstream as this fix
> depended on the recently committed patch:
> 
>   commit 43db8f70d86b2492b79f59342187b919fd58b3dd
>   Date:   Thu Oct 23 16:34:20 2025 +0100
> 
>       gdbsupport: remove undefined behaviour from (forward_)scope_exit
> 
> The problem is that the first commit above introduces a test which
> causes the remote target to disconnect while processing an inferior
> stop event, specifically, within normal_stop (infrun.c), GDB calls
> update_thread_list, and it is during this call that the inferior
> disconnects.
> 
> When the remote target disconnects, GDB immediately unpushes the
> remote target.  See remote_unpush_target and its uses in remote.c.
> 
> If this is the last use of the remote target, then unpushing it will
> cause the target to be deleted.
> 
> This is a problem, because in normal_stop, we have an RAII variable
> maybe_finish_thread_state, which is an optional
> scoped_finish_thread_state, and in some cases, this will hold a
> pointer to the process_startum_target which needs to be finished.
> 
> So the order of events is:
> 
>   1. Call to normal_stop.
> 
>   2. Create maybe_finish_thread_state with a pointer to the current
>      remote_target object.
> 
>   3. Call update_thread_list.
> 
>   4. Remote disconnects, GDB unpushes and deletes the current
>      remote_target object.  GDB throws an exception.
> 
>   5. The exception propagates back to normal_stop.
> 
>   6. The destructor for maybe_finish_thread_state runs, and tries to
>      make use of its cached pointer to the (now deleted) remote_target
>      object.  Badness ensues.
> 
> This bug isn't restricted to normal_stop.  If a remote target
> disconnects anywhere where there is a scoped_finish_thread_state in
> the call stack then this issue could arise.
> 
> I think what we need to do is to ensure that the remote_target is not
> actually deleted until after the scoped_finish_thread_state has been
> cleaned up.
> 
> And so, to achieve this, I propose changing scoped_finish_thread_state
> to hold a target_ops_ref rather than a pointer to the target_ops
> object.  Holding the reference will prevent the object from being
> deleted.
> 
> The new scoped_finish_thread_state is defined within its own file, and
> is a drop in replacement for the existing class.

This seems fine to me.  I suppose that when this happens,
finish_thread_state will do nothing, because all of the threads owned by
that target will have been deleted.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

See comments below.

> diff --git a/gdb/finish-thread-state.h b/gdb/finish-thread-state.h
> new file mode 100644
> index 00000000000..f025b1221a6
> --- /dev/null
> +++ b/gdb/finish-thread-state.h
> @@ -0,0 +1,64 @@
> +/* Copyright (C) 2025 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "gdbsupport/scope-exit.h"
> +#include "gdbthread.h"
> +#include "target.h"

Missing include guards (caught by the check-include-guards pre-commit
check).

> +
> +namespace detail
> +{
> +
> +/* Policy class for use with scope_exit_base.  Calls finish_thread_state on
> +   scope exit, unless release() is called to disengage.  The release
> +   mechanism is supplied by scope_exit_base.  */
> +
> +struct scoped_finish_thread_state_policy
> +{
> +  /* Store TARG and PTID for a later call to finish_thread_state.  For the
> +     meaning of these arguments, see the comments on finish_thread_state.  */
> +  explicit scoped_finish_thread_state_policy (process_stratum_target *targ,
> +					      ptid_t ptid)
> +    : m_ptid (ptid)
> +  {
> +    if (targ != nullptr)
> +      m_target_ref = target_ops_ref::new_reference (targ);

clangd complains about this line (can't pass a `process_stratum_target
*` as a `target_ops *`), because when it analyzes this header file on
its own, it probably only sees a forward declaration for
process_stratum_target, not the full class, and therefore doesn't know
that it is a subclass of target_ops.  This probably compiles thanks for
transitive includes.  It would be good to add an include of
"process-stratum-target.h" to make it "include what you use".

> +  }
> +
> +  /* Called on scope exit unless release was called, see scope_exit_base
> +     for details.  Calls finish_thread_state with stored target and ptid.  */
> +  void on_exit ()
> +  {
> +    target_ops *t = m_target_ref.get ();
> +    process_stratum_target *p_target
> +      = gdb::checked_static_cast<process_stratum_target *> (t);

Please add an include for "gdbsupport/gdb-checked-static-cast.h",
otherwise, my clangd complains about checked_static_cast not existing
:).

Simo
  

Simon Marchi <simark@simark.ca> writes:

> On 12/23/25 11:48 AM, Andrew Burgess wrote:
>> This commit fixes a use after free issue that was reported here:
>> 
>>   https://inbox.sourceware.org/gdb-patches/68354b98-795a-4b50-9eac-e54aa1d01b9d@simark.ca
>> 
>> This issue was exposed by the gdb.replay/missing-thread.exp test that
>> was added in this commit:
>> 
>>   commit 8bd08ee92c4a7bf2ad9e29c4da32a276ef2257fc
>>   Date:   Fri May 16 17:56:58 2025 +0100
>> 
>>       gdb: crash if thread unexpectedly disappears from thread list
>> 
>> It is worth pointing out that the use after free issue existed before
>> this commit, this commit just introduced a test that exposed the issue
>> when GDB is run with the address sanitizer.
>> 
>> It has taken a while to get this fix ready for upstream as this fix
>> depended on the recently committed patch:
>> 
>>   commit 43db8f70d86b2492b79f59342187b919fd58b3dd
>>   Date:   Thu Oct 23 16:34:20 2025 +0100
>> 
>>       gdbsupport: remove undefined behaviour from (forward_)scope_exit
>> 
>> The problem is that the first commit above introduces a test which
>> causes the remote target to disconnect while processing an inferior
>> stop event, specifically, within normal_stop (infrun.c), GDB calls
>> update_thread_list, and it is during this call that the inferior
>> disconnects.
>> 
>> When the remote target disconnects, GDB immediately unpushes the
>> remote target.  See remote_unpush_target and its uses in remote.c.
>> 
>> If this is the last use of the remote target, then unpushing it will
>> cause the target to be deleted.
>> 
>> This is a problem, because in normal_stop, we have an RAII variable
>> maybe_finish_thread_state, which is an optional
>> scoped_finish_thread_state, and in some cases, this will hold a
>> pointer to the process_startum_target which needs to be finished.
>> 
>> So the order of events is:
>> 
>>   1. Call to normal_stop.
>> 
>>   2. Create maybe_finish_thread_state with a pointer to the current
>>      remote_target object.
>> 
>>   3. Call update_thread_list.
>> 
>>   4. Remote disconnects, GDB unpushes and deletes the current
>>      remote_target object.  GDB throws an exception.
>> 
>>   5. The exception propagates back to normal_stop.
>> 
>>   6. The destructor for maybe_finish_thread_state runs, and tries to
>>      make use of its cached pointer to the (now deleted) remote_target
>>      object.  Badness ensues.
>> 
>> This bug isn't restricted to normal_stop.  If a remote target
>> disconnects anywhere where there is a scoped_finish_thread_state in
>> the call stack then this issue could arise.
>> 
>> I think what we need to do is to ensure that the remote_target is not
>> actually deleted until after the scoped_finish_thread_state has been
>> cleaned up.
>> 
>> And so, to achieve this, I propose changing scoped_finish_thread_state
>> to hold a target_ops_ref rather than a pointer to the target_ops
>> object.  Holding the reference will prevent the object from being
>> deleted.
>> 
>> The new scoped_finish_thread_state is defined within its own file, and
>> is a drop in replacement for the existing class.
>
> This seems fine to me.  I suppose that when this happens,
> finish_thread_state will do nothing, because all of the threads owned by
> that target will have been deleted.
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
>
> See comments below.
>
>> diff --git a/gdb/finish-thread-state.h b/gdb/finish-thread-state.h
>> new file mode 100644
>> index 00000000000..f025b1221a6
>> --- /dev/null
>> +++ b/gdb/finish-thread-state.h
>> @@ -0,0 +1,64 @@
>> +/* Copyright (C) 2025 Free Software Foundation, Inc.
>> +
>> +   This file is part of GDB.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +#include "gdbsupport/scope-exit.h"
>> +#include "gdbthread.h"
>> +#include "target.h"
>
> Missing include guards (caught by the check-include-guards pre-commit
> check).
>
>> +
>> +namespace detail
>> +{
>> +
>> +/* Policy class for use with scope_exit_base.  Calls finish_thread_state on
>> +   scope exit, unless release() is called to disengage.  The release
>> +   mechanism is supplied by scope_exit_base.  */
>> +
>> +struct scoped_finish_thread_state_policy
>> +{
>> +  /* Store TARG and PTID for a later call to finish_thread_state.  For the
>> +     meaning of these arguments, see the comments on finish_thread_state.  */
>> +  explicit scoped_finish_thread_state_policy (process_stratum_target *targ,
>> +					      ptid_t ptid)
>> +    : m_ptid (ptid)
>> +  {
>> +    if (targ != nullptr)
>> +      m_target_ref = target_ops_ref::new_reference (targ);
>
> clangd complains about this line (can't pass a `process_stratum_target
> *` as a `target_ops *`), because when it analyzes this header file on
> its own, it probably only sees a forward declaration for
> process_stratum_target, not the full class, and therefore doesn't know
> that it is a subclass of target_ops.  This probably compiles thanks for
> transitive includes.  It would be good to add an include of
> "process-stratum-target.h" to make it "include what you use".
>
>> +  }
>> +
>> +  /* Called on scope exit unless release was called, see scope_exit_base
>> +     for details.  Calls finish_thread_state with stored target and ptid.  */
>> +  void on_exit ()
>> +  {
>> +    target_ops *t = m_target_ref.get ();
>> +    process_stratum_target *p_target
>> +      = gdb::checked_static_cast<process_stratum_target *> (t);
>
> Please add an include for "gdbsupport/gdb-checked-static-cast.h",
> otherwise, my clangd complains about checked_static_cast not existing
> :).

I made the fixes you pointed out, and pushed this patch.

Thanks,
Andrew