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

[Simon Marchi <simark@simark.ca>]

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

I thought: don't we now ask the target to report thread exit events when
doing steps now (to handle step over exit)?  But we only ask for to
report the exit event for the stepping thread, so the target wouldn't
report the exit of thread 2.

If my understand above is corerct, then I agree with your reasoning.
The currently iterated on thread is stopped and should not disappear,
except maybe on a misbehaving target (in which case we'd fix the
target).  Maybe it could happen with a remote target where communication
breaks during this update_thread_list, but that is notoriously difficult
to handle correctly.

> @@ -762,7 +739,30 @@ continue_1 (int all_threads)
>        scoped_disable_commit_resumed disable_commit_resumed
>  	("continue all threads in non-stop");
>  
> -      for_each_thread (proceed_thread_callback);
> +      /* Do not use all_threads_safe in case threads get removed while
> +	 resuming THREAD.  */

I understand this comment because I just read your commit message, but I
don't think I would understand it in isolation.  Because
all_threads_safe is usually used to handle cases where threads (the
current one) get removed while iterating.  I suggest:

      /* Do not use all_threads_safe, because it's possible for the next thread
         to get removed while resuming THREAD.  We know that thread is stopped
	 and should not disappear under our feet.  */

Then if I wanted to understand more about the circumstances where this
check was added, I would do a git blame and find the commit message.

Simon