File gdb-fix-heap-use-after-free-in-select_event_lwp... of Package gdb

Overview Repositories Revisions Requests Users Attributes Meta

File gdb-fix-heap-use-after-free-in-select_event_lwp.patch of Package gdb

From 350172ea215c7074601e8424ff636563612f91e8 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Wed, 21 Feb 2024 16:23:55 +0000
Subject: [PATCH 14/48] [gdb] Fix heap-use-after-free in select_event_lwp

PR gdb/31259 reveals one scenario where we run into a
heap-use-after-free reported by thread sanitizer, while running
gdb.base/vfork-follow-parent.exp.

The heap-use-after-free happens during the following scenario:

- linux_nat_wait_1 is about to return an event for T2.  It stops all
   other threads, and while doing so, stop_wait_callback -> wait_lwp
   sees T1 exit, and decides to leave the exit event pending.  It
   should have set the lp->stopped flag too, but does not -- this is
   the bug.

- The event for T2 is reported, is processed by infrun, and we're
   back at linux_nat_wait_1.

- linux_nat_wait_1 selects LWP T1 with the pending exit status to
   report.

- it sets variable lp to point to the corresponding lwp_info.

- it calls stop_callback and stop_wait_callback for all threads
   (because !target_is_non_stop_p ()).

- it calls select_event_lwp to maybe pick another thread than T1, to
   prevent starvation.

The problem is the following:

- while calling stop_wait_callback for all threads, it also does this
   for T1.  While doing so, the corresponding lwp_info is deleted
   (callstack stop_wait_callback -> wait_lwp -> exit_lwp ->
   delete_lwp), leaving variable lp as a dangling pointer.

- variable lp is passed to select_event_lwp, which derefences it,
   which causes the heap-use-after-free.

Note that the comment here mentions "all other LWP's":
...
      /* Now stop all other LWP's ...  */
      iterate_over_lwps (minus_one_ptid, stop_callback);
      /* ... and wait until all of them have reported back that
        they're no longer running.  */
      iterate_over_lwps (minus_one_ptid, stop_wait_callback);
...

The reason the comments say "all other LWP's", and doesn't bother
filtering out LP is that lp->stopped should be true at this point, and
the callbacks (both stop_callback and stop_wait_callback) check that
flag, and do nothing if set.  I.e., they skip already-stopped threads,
so they should skip LP.

In this particular scenario, though, we missed setting the stopped
flag right in the first step described above, so LP was iterated over
incorrectly.

The fix is to make wait_lwp set the lp->stopped flag when it decides
to leave the exit event pending.  However, going a bit further,
gdbserver has a mark_lwp_dead function to centralize setting up
various lwp flags such that the rest of the code doesn't mishandle
them, and it seems like a good idea to do a similar thing in gdb as
well.  That is what this patch does.

PR gdb/31259
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31259
Co-Authored-By: Tom de Vries <tdevries@suse.de>
Change-Id: I4a6169976f89bf714c478cbb2b7d4c32365e62a9
---
 gdb/linux-nat.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 7e36ced6292..ed445c5e5bb 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -2029,6 +2029,27 @@ wait_for_signal ()
     }
 }
 
+/* Mark LWP dead, with STATUS as exit status pending to report
+   later.  */
+
+static void
+mark_lwp_dead (lwp_info *lp, int status)
+{
+  /* Store the exit status lp->waitstatus, because lp->status would be
+     ambiguous (W_EXITCODE(0,0) == 0).  */
+  lp->waitstatus = host_status_to_waitstatus (status);
+
+  /* If we're processing LP's status, there should be no other event
+     already recorded as pending.  */
+  gdb_assert (lp->status == 0);
+
+  /* Dead LWPs aren't expected to report a pending sigstop.  */
+  lp->signalled = 0;
+
+  /* Prevent trying to stop it.  */
+  lp->stopped = 1;
+}
+
 /* Wait for LP to stop.  Returns the wait status, or 0 if the LWP has
    exited.  */
 
@@ -2114,9 +2135,8 @@ wait_lwp (struct lwp_info *lp)
 
 	      /* If this is the leader exiting, it means the whole
 		 process is gone.  Store the status to report to the
-		 core.  Store it in lp->waitstatus, because lp->status
-		 would be ambiguous (W_EXITCODE(0,0) == 0).  */
-	      lp->waitstatus = host_status_to_waitstatus (status);
+		 core.  */
+	      mark_lwp_dead (lp, status);
 	      return 0;
 	    }
 
@@ -2908,12 +2928,7 @@ linux_nat_filter_event (int lwpid, int status)
       linux_nat_debug_printf ("LWP %ld exited (resumed=%d)",
 			      lp->ptid.lwp (), lp->resumed);
 
-      /* Dead LWP's aren't expected to reported a pending sigstop.  */
-      lp->signalled = 0;
-
-      /* Store the pending event in the waitstatus, because
-	 W_EXITCODE(0,0) == 0.  */
-      lp->waitstatus = host_status_to_waitstatus (status);
+      mark_lwp_dead (lp, status);
       return;
     }
 
@@ -3248,6 +3263,7 @@ linux_nat_wait_1 (ptid_t ptid, struct target_waitstatus *ourstatus,
     }
 
   gdb_assert (lp);
+  gdb_assert (lp->stopped);
 
   status = lp->status;
   lp->status = 0;
-- 
2.35.3

Places

File gdb-fix-heap-use-after-free-in-select_event_lwp.patch of Package gdb

Places