[PATCH 0/4] Clear target async event handlers in wait method

[PATCH 0/4] Clear target async event handlers in wait method

[Simon Marchi]

This change started out as part of some work I intend to submit at a
later time, related to how infrun consumes target events.  However, I
noticed it would address PR 26614 ("AddressSanitizer:
heap-use-after-free of extended_remote_target in
remote_async_inferior_event_handler").  And in my opinion it's a
desirable change no matter what, because it simplifies things a little
bit.  So I extracted it and am submitting it on its own.

Simon Marchi (4):
  gdb: make async event handlers clear themselves
  gdb: make remote target clear its async event handler in wait
  gdb: make record-btrace target clear its async event handler in wait
  gdb: make record-full target clear its async event handler in wait

 gdb/async-event.c   |  1 -
 gdb/async-event.h   |  9 +++++++++
 gdb/infrun.c        |  1 +
 gdb/record-btrace.c |  3 +++
 gdb/record-full.c   |  2 ++
 gdb/remote-notif.c  |  4 +++-
 gdb/remote.c        | 25 +++++++++----------------
 7 files changed, 27 insertions(+), 18 deletions(-)

-- 
2.29.2

[PATCH 1/4] gdb: make async event handlers clear themselves

[Simon Marchi]

The `ready` flag of async event handlers is cleared by the async event
handler system right before invoking the associated callback, in
check_async_event_handlers.

This is not ideal with how the infrun subsystem consumes events: all
targets' async event handler callbacks essentially just invoke
`inferior_event_handler`, which eventually calls `fetch_inferior_event`
and `do_target_wait`.  `do_target_wait` picks an inferior at random,
and thus a target at random (it could be the target whose `ready` flag
was cleared, or not), and pulls one event from it.

So it's possible that:

- the async event handler for a target A is called
- we end up consuming an event for target B
- all threads of target B are stopped, target_async(0) is called on it,
  so its async event handler is cleared (e.g.
  record_btrace_target::async)

As a result, target A still has events to report while its async event
handler is left unmarked, so these events are not consumed.  To counter
this, at the end of their async event handler callbacks, targets check
if they still have something to report and re-mark their async event
handler (e.g. remote_async_inferior_event_handler).

The linux_nat target does not suffer from this because it doesn't use an
async event handler at the moment.  It only uses a pipe registered with
the event loop.  It is written to in the SIGCHLD handler (and in other
spots that want to get target wait method called) and read from in
the target's wait method.  So if linux_nat happened to be target A in
the example above, the pipe would just stay readable, and the event loop
would wake up again, until linux_nat's wait method is finally called and
consumes the contents of the pipe.

I think it would be nicer if targets using async_event_handler worked in
a similar way, where the flag would stay set until the target's wait
method is actually called.  As a first step towards that, this patch
moves the responsibility of clearing the ready flags of async event
handlers to the invoked callback.

All async event handler callbacks are modified to clear their ready flag
before doing anything else.  So in practice, nothing changes with this
patch.  It's only the responsibility of clearing the flag that is
shifted toward the callee.

gdb/ChangeLog:

        * async-event.h (async_event_handler_func):  Add documentation.
        * async-event.c (check_async_event_handlers): Don't clear
	async_event_handler ready flag.
        * infrun.c (infrun_async_inferior_event_handler): Clear ready
	flag.
        * record-btrace.c (record_btrace_handle_async_inferior_event):
	Likewise.
        * record-full.c (record_full_async_inferior_event_handler):
	Likewise.
        * remote-notif.c (remote_async_get_pending_events_handler):
	Likewise.
        * remote.c (remote_async_inferior_event_handler): Likewise.

Change-Id: I179ef8e99580eae642d332846fd13664dbddc0c1
---
 gdb/async-event.c   | 1 -
 gdb/async-event.h   | 9 +++++++++
 gdb/infrun.c        | 1 +
 gdb/record-btrace.c | 1 +
 gdb/record-full.c   | 1 +
 gdb/remote-notif.c  | 4 +++-
 gdb/remote.c        | 5 +++--
 7 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/gdb/async-event.c b/gdb/async-event.c
index 4228dfb09e6..eb967b568c5 100644
--- a/gdb/async-event.c
+++ b/gdb/async-event.c
@@ -322,7 +322,6 @@ check_async_event_handlers ()
     {
       if (async_handler_ptr->ready)
 	{
-	  async_handler_ptr->ready = 0;
 	  event_loop_debug_printf ("invoking async event handler `%s`",
 				   async_handler_ptr->name);
 	  (*async_handler_ptr->proc) (async_handler_ptr->client_data);
diff --git a/gdb/async-event.h b/gdb/async-event.h
index 8f279d63d63..10b9ae85112 100644
--- a/gdb/async-event.h
+++ b/gdb/async-event.h
@@ -24,6 +24,15 @@
 struct async_signal_handler;
 struct async_event_handler;
 typedef void (sig_handler_func) (gdb_client_data);
+
+/* Type of async event handler callbacks.
+
+   DATA is the client data originally passed to create_async_event_handler.
+
+   The callback is called when the async event handler is marked.  The callback
+   is responsible for clearing the async event handler if it no longer needs
+   to be called.  */
+
 typedef void (async_event_handler_func) (gdb_client_data);
 
 extern struct async_signal_handler *
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 2e5e837452d..eee96816e3b 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -9158,6 +9158,7 @@ static const struct internalvar_funcs siginfo_funcs =
 static void
 infrun_async_inferior_event_handler (gdb_client_data data)
 {
+  clear_async_event_handler (infrun_async_inferior_event_token);
   inferior_event_handler (INF_REG_EVENT);
 }
 
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index dece9b60778..d5338c74aed 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -326,6 +326,7 @@ record_btrace_auto_disable (void)
 static void
 record_btrace_handle_async_inferior_event (gdb_client_data data)
 {
+  clear_async_event_handler (record_btrace_async_inferior_event_handler);
   inferior_event_handler (INF_REG_EVENT);
 }
 
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 3b5e6fee7cd..410028aed60 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -904,6 +904,7 @@ static struct async_event_handler *record_full_async_inferior_event_token;
 static void
 record_full_async_inferior_event_handler (gdb_client_data data)
 {
+  clear_async_event_handler (record_full_async_inferior_event_token);
   inferior_event_handler (INF_REG_EVENT);
 }
 
diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
index f18bc8678e3..3b81fd55557 100644
--- a/gdb/remote-notif.c
+++ b/gdb/remote-notif.c
@@ -108,8 +108,10 @@ remote_notif_process (struct remote_notif_state *state,
 static void
 remote_async_get_pending_events_handler (gdb_client_data data)
 {
+  remote_notif_state *notif_state = (remote_notif_state *) data;
+  clear_async_event_handler (notif_state->get_pending_events_token);
   gdb_assert (target_is_non_stop_p ());
-  remote_notif_process ((struct remote_notif_state *) data, NULL);
+  remote_notif_process (notif_state, NULL);
 }
 
 /* Remote notification handler.  Parse BUF, queue notification and
diff --git a/gdb/remote.c b/gdb/remote.c
index 71f814efb36..23c1bab0b27 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -14174,10 +14174,11 @@ remote_async_serial_handler (struct serial *scb, void *context)
 static void
 remote_async_inferior_event_handler (gdb_client_data data)
 {
-  inferior_event_handler (INF_REG_EVENT);
-
   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
-- 
2.29.2

[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

[PATCH 3/4] gdb: make record-btrace target clear its async event handler in wait

[Simon Marchi]

For the same reason explained in the previous patch (which was for the
remote target), move clearing of the async event handler of the
record-btrace target to the wait method.

The record-btrace target already re-sets its async event handler if
needed in its wait method, so that part doesn't need to be changed:

    /* In async mode, we need to announce further events.  */
    if (target_is_async_p ())
      record_btrace_maybe_mark_async_event (moving, no_history);

gdb/ChangeLog:

	* record-btrace.c (record_btrace_handle_async_inferior_event):
	Don't clear async event handler.
	(record_btrace_target::wait): Clear async event handler at
	beginning.

Change-Id: Ib32087a81bf94f1b884a938c8167ac8bbe09e362
---
 gdb/record-btrace.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index d5338c74aed..aca76dae0dd 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -326,7 +326,6 @@ record_btrace_auto_disable (void)
 static void
 record_btrace_handle_async_inferior_event (gdb_client_data data)
 {
-  clear_async_event_handler (record_btrace_async_inferior_event_handler);
   inferior_event_handler (INF_REG_EVENT);
 }
 
@@ -2543,6 +2542,9 @@ record_btrace_target::wait (ptid_t ptid, struct target_waitstatus *status,
   std::vector<thread_info *> moving;
   std::vector<thread_info *> no_history;
 
+  /* Clear this, if needed we'll re-mark it below.  */
+  clear_async_event_handler (record_btrace_async_inferior_event_handler);
+
   DEBUG ("wait %s (0x%x)", target_pid_to_str (ptid).c_str (),
 	 (unsigned) options);
 
-- 
2.29.2

[PATCH 4/4] gdb: make record-full target clear its async event handler in wait

[Simon Marchi]

For the same reason explained in the previous patch (which was for the
record-btrace target), move clearing of the async event handler of the
record-full target to the wait method.

I'm not sure if/where that target needs to re-set its async event
handler in the wait method.  Since it only supports a single thread,
there probably can't be multiple events to report at the same time.

gdb/ChangeLog:

	* record-full.c (record_full_async_inferior_event_handler):
	Don't clear async event handler.
	(record_full_base_target::wait): Clear async event handler at
	beginning.

Change-Id: I146fbdb53d99e3a32766ac7cd337ac5ed7fd9adf
---
 gdb/record-full.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gdb/record-full.c b/gdb/record-full.c
index 410028aed60..a7bd0bdcd62 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -904,7 +904,6 @@ static struct async_event_handler *record_full_async_inferior_event_token;
 static void
 record_full_async_inferior_event_handler (gdb_client_data data)
 {
-  clear_async_event_handler (record_full_async_inferior_event_token);
   inferior_event_handler (INF_REG_EVENT);
 }
 
@@ -1464,6 +1463,8 @@ record_full_base_target::wait (ptid_t ptid, struct target_waitstatus *status,
 {
   ptid_t return_ptid;
 
+  clear_async_event_handler (record_full_async_inferior_event_token);
+
   return_ptid = record_full_wait_1 (this, ptid, status, options);
   if (status->kind != TARGET_WAITKIND_IGNORE)
     {
-- 
2.29.2

Re: [PATCH 0/4] Clear target async event handlers in wait method

[Simon Marchi]

Ping.

On 2020-11-30 11:52 a.m., Simon Marchi via Gdb-patches wrote:
> This change started out as part of some work I intend to submit at a
> later time, related to how infrun consumes target events.  However, I
> noticed it would address PR 26614 ("AddressSanitizer:
> heap-use-after-free of extended_remote_target in
> remote_async_inferior_event_handler").  And in my opinion it's a
> desirable change no matter what, because it simplifies things a little
> bit.  So I extracted it and am submitting it on its own.
> 
> Simon Marchi (4):
>   gdb: make async event handlers clear themselves
>   gdb: make remote target clear its async event handler in wait
>   gdb: make record-btrace target clear its async event handler in wait
>   gdb: make record-full target clear its async event handler in wait
> 
>  gdb/async-event.c   |  1 -
>  gdb/async-event.h   |  9 +++++++++
>  gdb/infrun.c        |  1 +
>  gdb/record-btrace.c |  3 +++
>  gdb/record-full.c   |  2 ++
>  gdb/remote-notif.c  |  4 +++-
>  gdb/remote.c        | 25 +++++++++----------------
>  7 files changed, 27 insertions(+), 18 deletions(-)
> 

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

[Andrew Burgess]

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

Re: [PATCH 1/4] gdb: make async event handlers clear themselves

[Andrew Burgess]

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

> The `ready` flag of async event handlers is cleared by the async event
> handler system right before invoking the associated callback, in
> check_async_event_handlers.
> 
> This is not ideal with how the infrun subsystem consumes events: all
> targets' async event handler callbacks essentially just invoke
> `inferior_event_handler`, which eventually calls `fetch_inferior_event`
> and `do_target_wait`.  `do_target_wait` picks an inferior at random,
> and thus a target at random (it could be the target whose `ready` flag
> was cleared, or not), and pulls one event from it.
> 
> So it's possible that:
> 
> - the async event handler for a target A is called
> - we end up consuming an event for target B
> - all threads of target B are stopped, target_async(0) is called on it,
>   so its async event handler is cleared (e.g.
>   record_btrace_target::async)
> 
> As a result, target A still has events to report while its async event
> handler is left unmarked, so these events are not consumed.  To counter
> this, at the end of their async event handler callbacks, targets check
> if they still have something to report and re-mark their async event
> handler (e.g. remote_async_inferior_event_handler).
> 
> The linux_nat target does not suffer from this because it doesn't use an
> async event handler at the moment.  It only uses a pipe registered with
> the event loop.  It is written to in the SIGCHLD handler (and in other
> spots that want to get target wait method called) and read from in
> the target's wait method.  So if linux_nat happened to be target A in
> the example above, the pipe would just stay readable, and the event loop
> would wake up again, until linux_nat's wait method is finally called and
> consumes the contents of the pipe.
> 
> I think it would be nicer if targets using async_event_handler worked in
> a similar way, where the flag would stay set until the target's wait
> method is actually called.  As a first step towards that, this patch
> moves the responsibility of clearing the ready flags of async event
> handlers to the invoked callback.
> 
> All async event handler callbacks are modified to clear their ready flag
> before doing anything else.  So in practice, nothing changes with this
> patch.  It's only the responsibility of clearing the flag that is
> shifted toward the callee.
> 
> gdb/ChangeLog:
> 
>         * async-event.h (async_event_handler_func):  Add documentation.
>         * async-event.c (check_async_event_handlers): Don't clear
> 	async_event_handler ready flag.
>         * infrun.c (infrun_async_inferior_event_handler): Clear ready
> 	flag.
>         * record-btrace.c (record_btrace_handle_async_inferior_event):
> 	Likewise.
>         * record-full.c (record_full_async_inferior_event_handler):
> 	Likewise.
>         * remote-notif.c (remote_async_get_pending_events_handler):
> 	Likewise.
>         * remote.c (remote_async_inferior_event_handler): Likewise.
>

LGTM.

Thanks,
Andrew

> Change-Id: I179ef8e99580eae642d332846fd13664dbddc0c1
> ---
>  gdb/async-event.c   | 1 -
>  gdb/async-event.h   | 9 +++++++++
>  gdb/infrun.c        | 1 +
>  gdb/record-btrace.c | 1 +
>  gdb/record-full.c   | 1 +
>  gdb/remote-notif.c  | 4 +++-
>  gdb/remote.c        | 5 +++--
>  7 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/async-event.c b/gdb/async-event.c
> index 4228dfb09e6..eb967b568c5 100644
> --- a/gdb/async-event.c
> +++ b/gdb/async-event.c
> @@ -322,7 +322,6 @@ check_async_event_handlers ()
>      {
>        if (async_handler_ptr->ready)
>  	{
> -	  async_handler_ptr->ready = 0;
>  	  event_loop_debug_printf ("invoking async event handler `%s`",
>  				   async_handler_ptr->name);
>  	  (*async_handler_ptr->proc) (async_handler_ptr->client_data);
> diff --git a/gdb/async-event.h b/gdb/async-event.h
> index 8f279d63d63..10b9ae85112 100644
> --- a/gdb/async-event.h
> +++ b/gdb/async-event.h
> @@ -24,6 +24,15 @@
>  struct async_signal_handler;
>  struct async_event_handler;
>  typedef void (sig_handler_func) (gdb_client_data);
> +
> +/* Type of async event handler callbacks.
> +
> +   DATA is the client data originally passed to create_async_event_handler.
> +
> +   The callback is called when the async event handler is marked.  The callback
> +   is responsible for clearing the async event handler if it no longer needs
> +   to be called.  */
> +
>  typedef void (async_event_handler_func) (gdb_client_data);
>  
>  extern struct async_signal_handler *
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index 2e5e837452d..eee96816e3b 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -9158,6 +9158,7 @@ static const struct internalvar_funcs siginfo_funcs =
>  static void
>  infrun_async_inferior_event_handler (gdb_client_data data)
>  {
> +  clear_async_event_handler (infrun_async_inferior_event_token);
>    inferior_event_handler (INF_REG_EVENT);
>  }
>  
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index dece9b60778..d5338c74aed 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -326,6 +326,7 @@ record_btrace_auto_disable (void)
>  static void
>  record_btrace_handle_async_inferior_event (gdb_client_data data)
>  {
> +  clear_async_event_handler (record_btrace_async_inferior_event_handler);
>    inferior_event_handler (INF_REG_EVENT);
>  }
>  
> diff --git a/gdb/record-full.c b/gdb/record-full.c
> index 3b5e6fee7cd..410028aed60 100644
> --- a/gdb/record-full.c
> +++ b/gdb/record-full.c
> @@ -904,6 +904,7 @@ static struct async_event_handler *record_full_async_inferior_event_token;
>  static void
>  record_full_async_inferior_event_handler (gdb_client_data data)
>  {
> +  clear_async_event_handler (record_full_async_inferior_event_token);
>    inferior_event_handler (INF_REG_EVENT);
>  }
>  
> diff --git a/gdb/remote-notif.c b/gdb/remote-notif.c
> index f18bc8678e3..3b81fd55557 100644
> --- a/gdb/remote-notif.c
> +++ b/gdb/remote-notif.c
> @@ -108,8 +108,10 @@ remote_notif_process (struct remote_notif_state *state,
>  static void
>  remote_async_get_pending_events_handler (gdb_client_data data)
>  {
> +  remote_notif_state *notif_state = (remote_notif_state *) data;
> +  clear_async_event_handler (notif_state->get_pending_events_token);
>    gdb_assert (target_is_non_stop_p ());
> -  remote_notif_process ((struct remote_notif_state *) data, NULL);
> +  remote_notif_process (notif_state, NULL);
>  }
>  
>  /* Remote notification handler.  Parse BUF, queue notification and
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 71f814efb36..23c1bab0b27 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -14174,10 +14174,11 @@ remote_async_serial_handler (struct serial *scb, void *context)
>  static void
>  remote_async_inferior_event_handler (gdb_client_data data)
>  {
> -  inferior_event_handler (INF_REG_EVENT);
> -
>    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
> -- 
> 2.29.2
> 

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

[Simon Marchi]

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

Re: [PATCH 3/4] gdb: make record-btrace target clear its async event handler in wait

[Andrew Burgess]

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

> For the same reason explained in the previous patch (which was for the
> remote target), move clearing of the async event handler of the
> record-btrace target to the wait method.
> 
> The record-btrace target already re-sets its async event handler if
> needed in its wait method, so that part doesn't need to be changed:
> 
>     /* In async mode, we need to announce further events.  */
>     if (target_is_async_p ())
>       record_btrace_maybe_mark_async_event (moving, no_history);
> 
> gdb/ChangeLog:
> 
> 	* record-btrace.c (record_btrace_handle_async_inferior_event):
> 	Don't clear async event handler.
> 	(record_btrace_target::wait): Clear async event handler at
> 	beginning.

LGTM.

> 
> Change-Id: Ib32087a81bf94f1b884a938c8167ac8bbe09e362
> ---
>  gdb/record-btrace.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
> index d5338c74aed..aca76dae0dd 100644
> --- a/gdb/record-btrace.c
> +++ b/gdb/record-btrace.c
> @@ -326,7 +326,6 @@ record_btrace_auto_disable (void)
>  static void
>  record_btrace_handle_async_inferior_event (gdb_client_data data)
>  {
> -  clear_async_event_handler (record_btrace_async_inferior_event_handler);
>    inferior_event_handler (INF_REG_EVENT);
>  }
>  
> @@ -2543,6 +2542,9 @@ record_btrace_target::wait (ptid_t ptid, struct target_waitstatus *status,
>    std::vector<thread_info *> moving;
>    std::vector<thread_info *> no_history;
>  
> +  /* Clear this, if needed we'll re-mark it below.  */
> +  clear_async_event_handler (record_btrace_async_inferior_event_handler);
> +
>    DEBUG ("wait %s (0x%x)", target_pid_to_str (ptid).c_str (),
>  	 (unsigned) options);
>  
> -- 
> 2.29.2
> 

Re: [PATCH 4/4] gdb: make record-full target clear its async event handler in wait

[Andrew Burgess]

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

> For the same reason explained in the previous patch (which was for the
> record-btrace target), move clearing of the async event handler of the
> record-full target to the wait method.
> 
> I'm not sure if/where that target needs to re-set its async event
> handler in the wait method.  Since it only supports a single thread,
> there probably can't be multiple events to report at the same time.
> 
> gdb/ChangeLog:
> 
> 	* record-full.c (record_full_async_inferior_event_handler):
> 	Don't clear async event handler.
> 	(record_full_base_target::wait): Clear async event handler at
> 	beginning.

LGTM.

> 
> Change-Id: I146fbdb53d99e3a32766ac7cd337ac5ed7fd9adf
> ---
>  gdb/record-full.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/record-full.c b/gdb/record-full.c
> index 410028aed60..a7bd0bdcd62 100644
> --- a/gdb/record-full.c
> +++ b/gdb/record-full.c
> @@ -904,7 +904,6 @@ static struct async_event_handler *record_full_async_inferior_event_token;
>  static void
>  record_full_async_inferior_event_handler (gdb_client_data data)
>  {
> -  clear_async_event_handler (record_full_async_inferior_event_token);
>    inferior_event_handler (INF_REG_EVENT);
>  }
>  
> @@ -1464,6 +1463,8 @@ record_full_base_target::wait (ptid_t ptid, struct target_waitstatus *status,
>  {
>    ptid_t return_ptid;
>  
> +  clear_async_event_handler (record_full_async_inferior_event_token);
> +
>    return_ptid = record_full_wait_1 (this, ptid, status, options);
>    if (status->kind != TARGET_WAITKIND_IGNORE)
>      {
> -- 
> 2.29.2
> 

Re: [PATCH 1/4] gdb: make async event handlers clear themselves

[Pedro Alves]

On 30/11/20 16:52, Simon Marchi via Gdb-patches wrote:
> The `ready` flag of async event handlers is cleared by the async event
> handler system right before invoking the associated callback, in
> check_async_event_handlers.
> 
> This is not ideal with how the infrun subsystem consumes events: all
> targets' async event handler callbacks essentially just invoke
> `inferior_event_handler`, which eventually calls `fetch_inferior_event`
> and `do_target_wait`.  `do_target_wait` picks an inferior at random,
> and thus a target at random (it could be the target whose `ready` flag
> was cleared, or not), and pulls one event from it.
> 
> So it's possible that:
> 
> - the async event handler for a target A is called
> - we end up consuming an event for target B
> - all threads of target B are stopped, target_async(0) is called on it,
>   so its async event handler is cleared (e.g.
>   record_btrace_target::async)
> 
> As a result, target A still has events to report while its async event
> handler is left unmarked, so these events are not consumed.  To counter
> this, at the end of their async event handler callbacks, targets check
> if they still have something to report and re-mark their async event
> handler (e.g. remote_async_inferior_event_handler).
> 
> The linux_nat target does not suffer from this because it doesn't use an
> async event handler at the moment.  It only uses a pipe registered with
> the event loop.  It is written to in the SIGCHLD handler (and in other
> spots that want to get target wait method called) and read from in
> the target's wait method.  So if linux_nat happened to be target A in
> the example above, the pipe would just stay readable, and the event loop
> would wake up again, until linux_nat's wait method is finally called and
> consumes the contents of the pipe.
> 
> I think it would be nicer if targets using async_event_handler worked in
> a similar way, where the flag would stay set until the target's wait
> method is actually called.  As a first step towards that, this patch
> moves the responsibility of clearing the ready flags of async event
> handlers to the invoked callback.
> 
> All async event handler callbacks are modified to clear their ready flag
> before doing anything else.  So in practice, nothing changes with this
> patch.  It's only the responsibility of clearing the flag that is
> shifted toward the callee.
> 
> gdb/ChangeLog:
> 
>         * async-event.h (async_event_handler_func):  Add documentation.
>         * async-event.c (check_async_event_handlers): Don't clear
> 	async_event_handler ready flag.
>         * infrun.c (infrun_async_inferior_event_handler): Clear ready
> 	flag.
>         * record-btrace.c (record_btrace_handle_async_inferior_event):
> 	Likewise.
>         * record-full.c (record_full_async_inferior_event_handler):
> 	Likewise.
>         * remote-notif.c (remote_async_get_pending_events_handler):
> 	Likewise.
>         * remote.c (remote_async_inferior_event_handler): Likewise.

(Watch out for spaces/tabs in the ChangeLog.)

I like this.  OK.

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

[Pedro Alves]

On 30/11/20 16:52, Simon Marchi via Gdb-patches wrote:

> However, the code in remote_async_inferior_event_handler checks for for

s/for 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.

It won't hurt, but I think it shouldn't be necessary -- if putpkt_binary or
getpkt_or_notif_sane_1 see a notification (search for '%'), we end up in
handle_notification, which marks state->get_pending_events_token, which
then leads us to:

 -> remote_async_get_pending_events_handler
  -> remote_notif_get_pending_events
   -> remote_notif_stop_ack
    -> remote_target::push_stop_reply
     -> mark_async_event_handler (rs->remote_async_inferior_event_token);

which then ends up in target_target::wait.

I don't recall exactly why I had to put the check for the pending
event in remote_async_inferior_event_handler, though.  Maybe it was
necessary because events auto-cleared back then.

Anyhow, this LGTM.  Patches #3 and #4 LGTM too.

Thanks,
Pedro Alves

Re: [PATCH 1/4] gdb: make async event handlers clear themselves

[Simon Marchi]

On 2021-02-04 12:42 p.m., Pedro Alves wrote:
> On 30/11/20 16:52, Simon Marchi via Gdb-patches wrote:
>> The `ready` flag of async event handlers is cleared by the async event
>> handler system right before invoking the associated callback, in
>> check_async_event_handlers.
>>
>> This is not ideal with how the infrun subsystem consumes events: all
>> targets' async event handler callbacks essentially just invoke
>> `inferior_event_handler`, which eventually calls `fetch_inferior_event`
>> and `do_target_wait`.  `do_target_wait` picks an inferior at random,
>> and thus a target at random (it could be the target whose `ready` flag
>> was cleared, or not), and pulls one event from it.
>>
>> So it's possible that:
>>
>> - the async event handler for a target A is called
>> - we end up consuming an event for target B
>> - all threads of target B are stopped, target_async(0) is called on it,
>>   so its async event handler is cleared (e.g.
>>   record_btrace_target::async)
>>
>> As a result, target A still has events to report while its async event
>> handler is left unmarked, so these events are not consumed.  To counter
>> this, at the end of their async event handler callbacks, targets check
>> if they still have something to report and re-mark their async event
>> handler (e.g. remote_async_inferior_event_handler).
>>
>> The linux_nat target does not suffer from this because it doesn't use an
>> async event handler at the moment.  It only uses a pipe registered with
>> the event loop.  It is written to in the SIGCHLD handler (and in other
>> spots that want to get target wait method called) and read from in
>> the target's wait method.  So if linux_nat happened to be target A in
>> the example above, the pipe would just stay readable, and the event loop
>> would wake up again, until linux_nat's wait method is finally called and
>> consumes the contents of the pipe.
>>
>> I think it would be nicer if targets using async_event_handler worked in
>> a similar way, where the flag would stay set until the target's wait
>> method is actually called.  As a first step towards that, this patch
>> moves the responsibility of clearing the ready flags of async event
>> handlers to the invoked callback.
>>
>> All async event handler callbacks are modified to clear their ready flag
>> before doing anything else.  So in practice, nothing changes with this
>> patch.  It's only the responsibility of clearing the flag that is
>> shifted toward the callee.
>>
>> gdb/ChangeLog:
>>
>>         * async-event.h (async_event_handler_func):  Add documentation.
>>         * async-event.c (check_async_event_handlers): Don't clear
>> 	async_event_handler ready flag.
>>         * infrun.c (infrun_async_inferior_event_handler): Clear ready
>> 	flag.
>>         * record-btrace.c (record_btrace_handle_async_inferior_event):
>> 	Likewise.
>>         * record-full.c (record_full_async_inferior_event_handler):
>> 	Likewise.
>>         * remote-notif.c (remote_async_get_pending_events_handler):
>> 	Likewise.
>>         * remote.c (remote_async_inferior_event_handler): Likewise.
> 
> (Watch out for spaces/tabs in the ChangeLog.)

Fixed.

> I like this.  OK.

Pushed, thanks.

Simon

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

[Simon Marchi]

On 2021-02-04 1:00 p.m., Pedro Alves wrote:
> On 30/11/20 16:52, Simon Marchi via Gdb-patches wrote:
> 
>> However, the code in remote_async_inferior_event_handler checks for for
> 
> s/for 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.
> 
> It won't hurt, but I think it shouldn't be necessary -- if putpkt_binary or
> getpkt_or_notif_sane_1 see a notification (search for '%'), we end up in
> handle_notification, which marks state->get_pending_events_token, which
> then leads us to:
> 
>  -> remote_async_get_pending_events_handler
>   -> remote_notif_get_pending_events
>    -> remote_notif_stop_ack
>     -> remote_target::push_stop_reply
>      -> mark_async_event_handler (rs->remote_async_inferior_event_token);
> 
> which then ends up in target_target::wait.
> 
> I don't recall exactly why I had to put the check for the pending
> event in remote_async_inferior_event_handler, though.  Maybe it was
> necessary because events auto-cleared back then.

Ok, I'll push this patch it as-is then, but look into simplifying the code
as another patch.
 
> Anyhow, this LGTM.  Patches #3 and #4 LGTM too.

Thanks, will push.

Simon