Re: [PATCH v2] [gdb] Fix heap-use-after-free in select_event_lwp

[Pedro Alves <pedro@palves.net>]

On 2024-01-23 11:48, Tom de Vries wrote:

> Since heap-use-after-free is essentially an address sanitizer complaint, I
> also tried building gdb with -O0 -fsanitize=address, but with this setup it
> doesn't seem to trigger (0 times out of 10).
> 
> The heap-use-after-free happens during the following scenario:
> - linux_nat_wait_1 selects an LWP thread T1 with a status to report.
> - it sets variable lp to point to the corresponding lwp_info.
> - it calls stop_callback and stop_wait_callback for all threads
>   (because !target_is_non_stop_p ()).
> - it calls select_event_lwp to maybe pick another thread than T1, to prevent
>   starvation.
> 
> The problem seems to be the following:
> - while calling stop_wait_callback for all threads, it also does this for T1.
>   While doing so, the corresponding lwp_info is deleted (callstack
>   stop_wait_callback -> wait_lwp -> exit_lwp -> delete_lwp), leaving variable
>   lp as a dangling pointer.
> - variable lp is passed to select_event_lwp, which derefences it, which causes
>   the heap-use-after-free.
> 
> Note that the comment here mentions "all other LWP's":
> ...
>       /* Now stop all other LWP's ...  */
>       iterate_over_lwps (minus_one_ptid, stop_callback);
>       /* ... and wait until all of them have reported back that
>         they're no longer running.  */
>       iterate_over_lwps (minus_one_ptid, stop_wait_callback);
> ...
> which presumably means other than the one in lp, but the iterators
> don't skip lp.

I think I'm missing something here.

The reason the comments say "all other LWP's", and don't bother filtering out LP is that
lp->stopped should be true at this point, and the callbacks (both stop_callback and stop_wait_callback)
check that flag, and do nothing if set.  I.e., they skip already-stopped threads, so they should
skip LP.

It sounds like we were about to report a stop for a thread that isn't marked as stopped?
Now it looks to me that _that_ would be the bug to fix.

Pedro Alves