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

["simark at simark dot ca" <sourceware-bugzilla@sourceware.org>]

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

Simon Marchi <simark at simark dot ca> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |simark at simark dot ca

--- Comment #12 from Simon Marchi <simark at simark dot ca> ---
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.

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

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.  In the patch log, I talk about an exit event, but you can replace
that in your head by "communication failure" which leads to the remote target
being destroyed, which is can really happen at any time.


>From 5c358d0604e0ac3e2bd9aca02234cb20f1caea42 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Fri, 27 Nov 2020 13:32:55 -0500
Subject: [PATCH] gdb: make remote_async_serial_handler just mark remote
 target's async event

The purpose of this patch is to uncover an existing bug that is
difficult to trigger otherwise.  But it makes the remote target flow a
bit simpler to understand, which is in my opinion a welcome change.

The bug is a possible use-after-free in
remote_async_inferior_event_handler:

    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 ();

      ...
    }

These are the steps that lead to it:

1. inferior_event_handler is called, deep down that calls
   remote_target::wait, then infrun handles the returned event
2. the event returned by remote_target::wait is an exit event
3. remote_target::mourn_inferior gets called, which leads to the remote
   target getting unpushed and deleted, because its refcount drops to 0
4. back to remote_async_inferior_event_handler, the
   `remote->get_remote_state ()` line uses the remote target object
   after it was deleted.

The reason why we don't see this bug on master (at least not often) is
that inferior_event_handler is most often called through
remote_async_inferior_event_handler, as a reaction of the file
descriptor used to talk to the remote side being readable:

    static void
    remote_async_serial_handler (struct serial *scb, void *context)
    {
      /* Don't propogate error information up to the client.  Instead let
         the client find out about the error by querying the target.  */
      inferior_event_handler (INF_REG_EVENT);
    }

I don't how exactly, but I am pretty sure (as I hit it in some rare
occasion) that some particular scheduling and event sequence can lead to
the exit event being handled through the inferior_event_handler call in
remote_async_inferior_event_token and to the bug described above.

Change-Id: If8d0daab97aa5338fe60d9e9b5a4ba3b5746eaea
---
 gdb/remote.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 71f814efb365..53ff8b63a1dc 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -14161,14 +14161,12 @@ remote_target::is_async_p ()
    will be able to delay notifying the client of an event until the
    point where an entire packet has been received.  */

-static serial_event_ftype remote_async_serial_handler;
-
 static void
 remote_async_serial_handler (struct serial *scb, void *context)
 {
-  /* Don't propogate error information up to the client.  Instead let
-     the client find out about the error by querying the target.  */
-  inferior_event_handler (INF_REG_EVENT);
+  remote_state *rs = (remote_state *) context;
+
+  mark_async_event_handler (rs->remote_async_inferior_event_token);
 }

 static void
-- 
2.29.2

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