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

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2020-12-24 12:23 p.m., Andrew Burgess wrote:
> * 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 ...."

Fixed.

>> @@ -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.  */

Done.  I also changed "to return here" to "to call us again", because
I thought "to return here" was not very clear.

> 
>> -      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.

I don't think this is specifically what lead to the use-after-free, it was
more that we referred to the remote target after calling fetch_inferior_event.

But I agree that since it's now unused, we should not pass it anymore, so fixed.

> Otherwise, LGTM.

Thanks, I'll wait a bit to see if Pedro also has something to say about it.

Simon