Re: [PATCH 2/4] gdb: make remote target clear its async event handler in wait

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2020-11-30 11:52:49 -0500]:

> The remote target's remote_async_inferior_event_token field is a flag
> that tells when it has an event to report and it wants its wait method
> to be called so it can report it.  The flag in cleared the

"The flag is cleared in the ...."

> async_event_handler's callback (remote_async_inferior_event_handler),
> just before calling inferior_event_handler.  However, since
> inferior_event_handler may actually call another target's wait method,
> we are not sure if the remote target may still have an event to report,
> so we need check if we need to re-raise the flag after the
> inferior_event_handler call.
> 
> For various reasons, inferior_event_handler can lead to the remote
> target getting closed and deleted.  This can be because of an inferior
> exit, or a serial communication failure.  That leads to a
> use-after-free:
> 
>     static void
>     remote_async_inferior_event_handler (async_event_handler *handler,
>     				     gdb_client_data data)
>     {
>       clear_async_event_handler (handler);
>       inferior_event_handler (INF_REG_EVENT);  <== remote_target gets deleted
> 
>       remote_target *remote = (remote_target *) data;
>       remote_state *rs = remote->get_remote_state ();  <== dereferencing the deleted object
> 
> This is PR26614.  To fix this, move the responsibility of clearing the
> remote_async_inferior_event_token flag to remote_target::wait.  Upon
> return remote_target::wait should now leave the flag cleared or marked,
> depending on whether it has subsequent event to report.
> 
> In the case where remote_async_inferior_event_handler gets called but
> infrun calls another target's wait method, the flag just naturally
> stays marked because nothing touches it.
> 
> Note that this logic is already partially implemented in
> remote_target::wait, since the remote target may have multiple events to
> report (and it can only report one at the time):
> 
>   if (target_is_async_p ())
>     {
>       remote_state *rs = get_remote_state ();
> 
>       /* If there are are events left in the queue tell the event loop
> 	 to return here.  */
>       if (!rs->stop_reply_queue.empty ())
> 	mark_async_event_handler (rs->remote_async_inferior_event_token);
>     }
> 
> However, the code in remote_async_inferior_event_handler checks for for
> pending events in addition to whether the stop reply queue is empty, so
> I've made remote_target::wait check for that as well.  I'm not
> completely sure this is ok, since I don't understand very well yet how
> the pending events mechanism works.  But I figured it was safer to do
> this in order to avoid missing events, worst case it just leads to
> unnecessary calls to remote_target::wait.
> 
> gdb/ChangeLog:
> 
> 	PR gdb/26614
> 	* remote.c (remote_target::wait): Clear async event handler at
> 	beginning, mark if needed at the end.
> 	(remote_async_inferior_event_handler): Don't set or clear async
> 	event handler.
> 
> Change-Id: I20117f5b5acc8a9972c90f16280249b766c1bf37
> ---
>  gdb/remote.c | 26 +++++++++-----------------
>  1 file changed, 9 insertions(+), 17 deletions(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 23c1bab0b27..30694e99b31 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -8000,6 +8000,13 @@ ptid_t
>  remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
>  		     target_wait_flags options)
>  {
> +  remote_state *rs = get_remote_state ();
> +
> +  /* Start by clearing the flag that asks for our wait method to be called,
> +     we'll mark it again at the end if needed.  */
> +  if (target_is_async_p ())
> +    clear_async_event_handler (rs->remote_async_inferior_event_token);
> +
>    ptid_t event_ptid;
>  
>    if (target_is_non_stop_p ())
> @@ -8009,11 +8016,10 @@ remote_target::wait (ptid_t ptid, struct target_waitstatus *status,
>  
>    if (target_is_async_p ())
>      {
> -      remote_state *rs = get_remote_state ();
> -
>        /* If there are are events left in the queue tell the event loop
>  	 to return here.  */

It feels like this comment should be updated something like:

  /* If there are events left in the queue, or unacknowledged
      notifications, then tell the event loop to return here.  */

> -      if (!rs->stop_reply_queue.empty ())
> +      if (!rs->stop_reply_queue.empty ()
> +	  || rs->notif_state->pending_event[notif_client_stop.id] != nullptr)
>  	mark_async_event_handler (rs->remote_async_inferior_event_token);
>      }
>  
> @@ -14174,21 +14180,7 @@ remote_async_serial_handler (struct serial *scb, void *context)
>  static void
>  remote_async_inferior_event_handler (gdb_client_data data)
>  {
> -  remote_target *remote = (remote_target *) data;
> -  remote_state *rs = remote->get_remote_state ();
> -  clear_async_event_handler (rs->remote_async_inferior_event_token);
> -
>    inferior_event_handler (INF_REG_EVENT);
> -
> -  /* 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);

This code was all from commit 96118d114e3c5.  If you check that commit
for the file remote.c you'll see one extra hunk:

  diff --git a/gdb/remote.c b/gdb/remote.c
  index f7f99dc24fe..59075cb09f2 100644
  --- a/gdb/remote.c
  +++ b/gdb/remote.c
  @@ -5605,7 +5605,7 @@ remote_target::open_1 (const char *name, int from_tty, int extended_p)

     /* Register extra event sources in the event loop.  */
     rs->remote_async_inferior_event_token
  -    = create_async_event_handler (remote_async_inferior_event_handler, NULL);
  +    = create_async_event_handler (remote_async_inferior_event_handler, remote);
     rs->notif_state = remote_notif_state_allocate (remote);

     /* Reset the target state; these things will be queried either by

I think this should be reverted as part of this commit too.  Passing
the remote through as the gdb_client_data parameter is bad
(i.e. allows for use after free situations), so lets not do that.

Otherwise, LGTM.

Thanks,
Andreq

>  }
>  
>  int
> -- 
> 2.29.2
>