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

[Simon Marchi]

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
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.  */
-      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);
 }
 
 int
-- 
2.29.2