signals: Fix races that prevent a SIGCONT from aborting a SIGSTOP

The target tg's leader's goroutine can set `groupStopDequeued` with the signals
lock held and then give up the lock to the SIGCONT-sending goroutine, which in
turn will return early from endGroupStopLocked() because a stop has not begun
yet. So we fix this bug by having endGroupStopLocked() always clear
`groupStopDequeued`. In other words, this fixes a race between:
 - deliverSingal() -> initiateGroupStop() and
 - applySignalSideEffectsLocked() -> endGroupStopLocked()

Now there's a second bug. It is a race between:
- runInterrupt.execute()
- applySignalSideEffectsLocked() -> endGroupStopLocked()
It occurs this way: in runInterrupt(), between participateGroupStopLocked()
and beginInternalStopLocked(), the signal mutex is dropped, allowing a SIGCONT
issuing thread to run its endGroupStopLocked() in the interim, which as it
turns out fails to abort the stop, for runInterrupt.execute() will not consult
`groupStopPending` again when it resumes. We fix this by not dropping the
signals mutex in runInterrupt().

PiperOrigin-RevId: 885288544

Author: shailend-g

pkg/sentry/kernel/task_signals.go

@@ -812,6 +812,9 @@ func (t *Task) initiateGroupStop(info *linux.SignalInfo) {
 func (tg *ThreadGroup) endGroupStopLocked(broadcast bool) {
 	// Discard all previously-queued stop signals.
 	linux.ForEachSignal(StopSignals, tg.discardSpecificLocked)
+	// Unsetting groupStopDequeued will cause racing calls to initiateGroupStop
+	// to recognize that the group stop has been cancelled.
+	tg.groupStopDequeued = false
 
 	if tg.groupStopPendingCount == 0 && !tg.groupStopComplete {
 		return
@@ -850,9 +853,6 @@ func (tg *ThreadGroup) endGroupStopLocked(broadcast bool) {
 		tg.groupContInterrupted = !tg.groupStopComplete
 		tg.groupContWaitable = true
 	}
-	// Unsetting groupStopDequeued will cause racing calls to initiateGroupStop
-	// to recognize that the group stop has been cancelled.
-	tg.groupStopDequeued = false
 	tg.groupStopSignal = 0
 	tg.groupStopPendingCount = 0
 	tg.groupStopComplete = false
@@ -986,14 +986,14 @@ func (*runInterrupt) execute(t *Task) taskRunState {
 		}
 		t.trapStopPending = false
 		t.trapNotifyPending = false
-		// Drop the signal mutex so we can take the TaskSet mutex.
-		t.tg.signalHandlers.mu.Unlock()
 
-		t.tg.pidns.owner.mu.RLock()
-		if t.tg.leader.parent == nil {
-			notifyParent = false
-		}
 		if tracer := t.Tracer(); tracer != nil {
+			// Drop the signal mutex so we can take the TaskSet mutex.
+			t.tg.signalHandlers.mu.Unlock()
+			t.tg.pidns.owner.mu.RLock()
+			if t.tg.leader.parent == nil {
+				notifyParent = false
+			}
 			if t.ptraceSeized {
 				if sig == 0 {
 					sig = linux.SIGTRAP
@@ -1026,11 +1026,15 @@ func (*runInterrupt) execute(t *Task) taskRunState {
 				}
 			}
 		} else {
-			t.tg.signalHandlers.mu.Lock()
 			if !t.killedLocked() {
 				t.beginInternalStopLocked((*groupStop)(nil))
 			}
+			// Drop the signal mutex so we can take the TaskSet mutex.
 			t.tg.signalHandlers.mu.Unlock()
+			t.tg.pidns.owner.mu.RLock()
+			if t.tg.leader.parent == nil {
+				notifyParent = false
+			}
 		}
 		if notifyParent {
 			t.tg.leader.parent.signalStop(t.tg.leader, linux.CLD_STOPPED, int32(sig))

test/syscalls/linux/BUILD

@@ -2628,6 +2628,7 @@ cc_binary(
     deps = select_gtest() + [
         "//test/util:multiprocess_util",
         "//test/util:posix_error",
+        "//test/util:save_util",
         "//test/util:test_util",
         "//test/util:thread_util",
         "@com_google_absl//absl/flags:flag",

test/syscalls/linux/sigstop.cc

@@ -17,12 +17,18 @@
 #include <sys/select.h>
 #include <time.h>
 
+#include <array>
+#include <atomic>
+#include <memory>
+
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include "absl/flags/flag.h"
 #include "absl/time/clock.h"
 #include "absl/time/time.h"
 #include "test/util/multiprocess_util.h"
 #include "test/util/posix_error.h"
+#include "test/util/save_util.h"
 #include "test/util/test_util.h"
 #include "test/util/thread_util.h"
 
@@ -130,6 +136,60 @@ TEST(SigstopTest, WaitidCorrectness) {
   EXPECT_EQ(info.si_code, CLD_KILLED);
 }
 
+TEST(SigstopTest, SIGCONTAbortsSIGSTOP) {
+  DisableSave ds;  // Forks a lot across many threads.
+  constexpr size_t kIterCount = 10;
+  std::array<std::unique_ptr<ScopedThread>, 100> threads;
+  std::atomic<bool> failed{false};
+
+  // The race takes a while to manifest, hence we use many threads.
+  for (size_t i = 0; i < threads.size(); ++i) {
+    threads[i] = std::make_unique<ScopedThread>([&failed] {
+      for (size_t j = 0; j < kIterCount; ++j) {
+        if (failed.load(std::memory_order_relaxed)) {
+          return;
+        }
+
+        int pid = fork();
+        ASSERT_GE(pid, 0);
+        if (pid == 0) {
+          // Child just sits and waits for signals.
+          // pause() is async-signal-safe and blocks until any signal arrives.
+          while (true) pause();
+        }
+
+        ASSERT_THAT(kill(pid, SIGSTOP), SyscallSucceeds());
+        ASSERT_THAT(kill(pid, SIGCONT), SyscallSucceeds());
+
+        // Fire a SIGTERM:
+        // - If the child is STOPPED, SIGTERM is queued.
+        // - If the child is RUNNING, it dies.
+        // Either way the waitpid below should not hang.
+        ASSERT_THAT(kill(pid, SIGTERM), SyscallSucceeds());
+
+        int status;
+        EXPECT_THAT(RetryEINTR(waitpid)(pid, &status, WSTOPPED),
+                    SyscallSucceeds());
+        if (WIFSTOPPED(status)) {
+          failed.store(true, std::memory_order_relaxed);
+          // Clean up: kill and reap the stuck child.
+          kill(pid, SIGKILL);
+          RetryEINTR(waitpid)(pid, &status, 0);
+          break;  // Test failed, don't continue this thread.
+        } else {
+          EXPECT_TRUE(WIFSIGNALED(status));
+          EXPECT_EQ(WTERMSIG(status), SIGTERM);
+        }
+      }
+    });
+  }
+
+  for (const auto& t : threads) {
+    t->Join();
+  }
+  EXPECT_FALSE(failed.load(std::memory_order_relaxed));
+}
+
 // Like base:SleepFor, but tries to avoid counting time spent stopped due to a
 // stop signal toward the sleep.
 //