[Bug remote/26614] AddressSanitizer: heap-use-after-free of extended_remote_target in remote_async_inferior_event_handler

["vries at gcc dot gnu.org" <sourceware-bugzilla@sourceware.org>]

https://sourceware.org/bugzilla/show_bug.cgi?id=26614

--- Comment #14 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Simon Marchi from comment #12)
> It looks very similar to a problem I hit while doing some unrelated changes.
> I think the reason is:
> 
> static void
> remote_async_inferior_event_handler (gdb_client_data data)
> {
>   inferior_event_handler (INF_REG_EVENT);  <--- remote target can get
> destroyed anywhere here
> 
>   remote_target *remote = (remote_target *) data;
>   remote_state *rs = remote->get_remote_state ();  <--- we dereference the
> remote target here
> 
>   /* inferior_event_handler may have consumed an event pending on the
>      infrun side without calling target_wait on the REMOTE target, or
>      may have pulled an event out of a different target.  Keep trying
>      for this remote target as long it still has either pending events
>      or unacknowledged notifications.  */
> 
>   if (rs->notif_state->pending_event[notif_client_stop.id] != NULL
>       || !rs->stop_reply_queue.empty ())
>     mark_async_event_handler (rs->remote_async_inferior_event_token);
> }
> 
> 
> inferior_event_target can lead to the remote target to be destroyed.  An
> "exit" stop reply from the target can lead to it, but communication failure
> is another one.
> 

Agreed, it's the same problem.  Thanks for helping me out here.

> The reason why it is difficult to reproduce is that inferior_event_handler
> is also called from remote_async_serial_handler, which doesn't have the same
> code that dereferences the remote target.  So the bug can only trigger when
> inferior_event_handler is called through
> remote_async_inferior_event_handler, which likely happens less often than
> being called through remote_async_serial_handler.
> 

I see.

> I did write this patch below to make it so inferior_event_handler is always
> called through remote_async_inferior_event_handler, that automatically
> triggers the bug in the simplest cases (the inferior exits with "target
> remote").  It makes it so remote_async_serial_handler doesn't call
> remote_async_inferior_event_handler anymore, but just marks the remote
> target's async handler.
> 

Yep, works for me to trigger the problem.  Using this patch, I trigger the
original failure with -m32:
...
Running src/gdb/testsuite/gdb.multi/multi-target-continue.exp ...
ERROR: GDB process no longer exists
...
and this one with -m64:
...
Running src/gdb/testsuite/gdb.multi/multi-target-no-resumed.exp ...
ERROR: GDB process no longer exists
...
when running gdb.multi/*.exp.

> What I like from this patch is that there's now a single path from the
> remote target to inferior_event_handler, so less variations to consider,
> more determinism.

Agreed, that's a good thing :)

-- 
You are receiving this mail because:
You are on the CC list for the bug.