RE: [PATCH] gdb: fix an issue with thread list corruption

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Hello Simon,

>>>>> On 5/4/26 3:16 AM, Markus Metzger wrote:
>>>>>> When resuming a target in non-stop mode with 'c -a', the continue
>>>>> command
>>>>>> uses for_each_thread() to proceed each stopped thread individually.
>This
>>>>>> uses an all_threads_safe() iteration.
>>>>>>
>>>>>> If one of the stopped threads does an inline step-over, since the target
>>>>>> is non-stop, we stop_all_threads(), which involves update_thread_list(),
>>>>>> which, in turn, may delete_thread().
>>>>>>
>>>>>> If this deleted the thread pointed to by the m_next safe iterator
>member,
>>>>>> the above all_threads_safe() iteration will be corrupted.
>>>>>>
>>>>>> The thread we're proceeding is stopped and there is no reason to
>delete
>>>>>> it.  Consequently, there is no reason for all_threads_safe(), which isn't
>>>>>> that safe in this scenario.
>>>>>>
>>>>>> Iterate using all_threads() and inline proceed_thread_callback().
>>>>>
>>>>> Just to try to make sure I understand the circumstances that lead the
>>>>> the failure correcty (and make sure this doesn't just cover up other
>>>>> problems):
>>>>>
>>>>> - thread 1 is stopped on a breakpoint
>>>>> - thread 2 is executing
>>>>> - displaced stepping is disabled
>>>>> - the target doesn't report thread events to the core
>>>>> - you "continue -a"
>>>>> - thread 1 starts an inline step-over, which calls stop_all_threads,
>>>>>   which calls update_thread_list
>>>>> - meanwhile, thread 2 exits
>>>>> - update_thread_list causes thread 2 to be deleted
>>>>> - the safe iterator's m_next field now points to a deleted thread info
>>>
>>> Can you comment on whether the sequence of events above sounds
>correct?
>>> If it is, I think it would be possible to write a CPU test to reproduce
>>> it, that would be helpful along with the fix.
>>
>> The sequence sounds good, but I wonder how we would ensure that thread
>2
>> exits just when we need it to.
>
>Usually, by hammering.  But I think it's not as difficult as it sounds.
>The state before the "continue -a" on thread N is that GDB knows about
>thread N+1.  Assuming that thread N+1 is a short-lived thread (one that
>does nothing but exit), it exits even before the "continue -a" is
>issued, but GDB doesn't know that until the next thread list update.
>When you "continue -a", GDB iterates using all_threads_safe, without
>doing any thread list update.  When iteration reaches thread N, the next
>thread is N+1, exited on the target but it still exists in GDB's mind.
>Thread N starts its inline step-over, and etc etc until the crash.
>
>> This is non-stop mode, so we should learn about the exit immediately.
>> The test would also need to rely on the ordering of threads in the
>> thread list.  And detecting the issue requires the next pointer of the
>> next element to be overwritten in a way that causes a deterministic
>> effect - like crashing GDB.  We wouldn't want to add another test that
>> fails sporadically.
>
>I wrote a test that fails consistently (at least with ASan) with
>the board native-extended-gdbserver:
>
>https://review.lttng.org/c/binutils-gdb/+/17704
>
>    (gdb) PASS: gdb.threads/continue-a-step-over-other-thread-exit.exp:
>step_over_thread_slot=1: continue to break_here
>    continue -a
>    Continuing.
>
>=================================================================
>    ==1250831==ERROR: AddressSanitizer: heap-use-after-free on address
>0x7d0984eb4480 at pc 0x564f8b3da4dd bp 0x7ffdad6a63d0 sp
>0x7ffdad6a63c0
>
>With your fixes, it passes.

And what happens without ASan?  Do we introduce yet another sporadically failing test?

If we only created two additional threads, we could vary which thread we're stepping
in two different runs of the test program, which makes the test program fairly trivial.

We'd need to step from one pthread_create() to the next to guarantee the order in
which GDB adds the threads.  As long as GDB adds them to either the front or the
back of the thread list consistently, we would get the same order in the two runs,
and by varying the thread we're stepping, we would cover both possible orders.

But we wouldn't be able to guarantee that the other thread exited at just the right
moment, would we?  We try to mitigate that by repeating this 100 times, but this is
just asking for the test to fail sporadically at a random iteration - or pass.

We have too many such tests already.  They may indicate an actual problem, or maybe
the test itself is broken or inherently non-deterministic.  What such tests definitely do is
create noise when trying to test a completely unrelated patch, thus slowing down GDB
development.

To get this deterministic, one would need to mock the remote target to guarantee
the ordering of thread exit and step completion events.  Something like gdbreplay,
but with hand-written logs, which would need to be maintained whenever anything
in the remote communication changes.  For more substantial changes to the remote
communication, this could become a significant burden.

And without additional tooling, like ASan, we still wouldn't be able to fail deterministically.

>Feel free to check it out and add it to your patch.  It will need a bit
>of cleanup, and there is a FIXME saying that it won't work with
>native-gdbserver because it requires inferior args.  I have another
>patch series that I need to send that add the ability to pass inferior
>args when using native-gdbserver, I'll try to get that done.
>
>Simon
Intel Deutschland GmbH
Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928