Clean up some uses of fork() in tests.

- Fix a few cases where async-signal-unsafe code is executed in a forked
  process pre-execve.

- Ensure that the return value of fork() is always checked.

PiperOrigin-RevId: 228949310
Change-Id: I3096cb7d7394b8d9ab81b0e0245f2060713ef589

7 files changed, 66 insertions, 32 deletions

diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD
index a8a6e15ee..41d476481 100644
--- a/test/syscalls/linux/BUILD
+++ b/test/syscalls/linux/BUILD
@@ -1460,7 +1460,6 @@ cc_binary(
         "//test/util:logging",
         "//test/util:multiprocess_util",
         "//test/util:signal_util",
-        "//test/util:test_main",
         "//test/util:test_util",
         "//test/util:thread_util",
         "@com_google_absl//absl/time",
diff --git a/test/syscalls/linux/concurrency.cc b/test/syscalls/linux/concurrency.cc
index 2c13b315c..f5a941ca8 100644
--- a/test/syscalls/linux/concurrency.cc
+++ b/test/syscalls/linux/concurrency.cc
@@ -72,8 +72,7 @@ TEST(ConcurrencyTest, MultiProcessMultithreaded) {
     }
   });
 
-  pid_t child_pid;
-  ASSERT_THAT(child_pid = fork(), SyscallSucceeds());
+  pid_t child_pid = fork();
   if (child_pid == 0) {
     // Busy wait without making any blocking syscalls.
     auto end = absl::Now() + absl::Seconds(5);
@@ -81,6 +80,7 @@ TEST(ConcurrencyTest, MultiProcessMultithreaded) {
     }
     _exit(0);
   }
+  ASSERT_THAT(child_pid, SyscallSucceeds());
 
   absl::SleepFor(absl::Seconds(1));
 
@@ -99,13 +99,13 @@ TEST(ConcurrencyTest, MultiProcessMultithreaded) {
 // never yields.
 TEST(ConcurrencyTest, MultiProcessConcurrency) {
 
-  pid_t child_pid;
-  ASSERT_THAT(child_pid = fork(), SyscallSucceeds());
+  pid_t child_pid = fork();
   if (child_pid == 0) {
     while (true) {
     }
     __builtin_unreachable();
   }
+  ASSERT_THAT(child_pid, SyscallSucceeds());
 
   absl::SleepFor(absl::Seconds(5));
 
diff --git a/test/syscalls/linux/kill.cc b/test/syscalls/linux/kill.cc
index 18ba8fb16..de50daf0b 100644
--- a/test/syscalls/linux/kill.cc
+++ b/test/syscalls/linux/kill.cc
@@ -70,7 +70,7 @@ TEST(KillTest, CanKillAllPIDs) {
     sa.sa_sigaction = SigHandler;
     sigfillset(&sa.sa_mask);
     sa.sa_flags = SA_SIGINFO;
-    auto cleanup = ASSERT_NO_ERRNO_AND_VALUE(ScopedSigaction(SIGWINCH, sa));
+    auto cleanup = ScopedSigaction(SIGWINCH, sa).ValueOrDie();
 
     // Indicate to the parent that we're ready.
     write_fd.reset();
@@ -81,7 +81,7 @@ TEST(KillTest, CanKillAllPIDs) {
     }
   }
 
-  EXPECT_THAT(pid, SyscallSucceeds());
+  ASSERT_THAT(pid, SyscallSucceeds());
 
   write_fd.reset();
 
@@ -111,7 +111,7 @@ TEST(KillTest, CannotKillInvalidPID) {
     _exit(0);
   }
 
-  EXPECT_THAT(fake_pid, SyscallSucceeds());
+  ASSERT_THAT(fake_pid, SyscallSucceeds());
 
   int status;
   ASSERT_THAT(RetryEINTR(waitpid)(fake_pid, &status, 0),
@@ -134,7 +134,7 @@ TEST(KillTest, CanKillRemoteProcess) {
     }
   }
 
-  EXPECT_THAT(pid, SyscallSucceeds());
+  ASSERT_THAT(pid, SyscallSucceeds());
 
   EXPECT_THAT(kill(pid, SIGKILL), SyscallSucceeds());
 
@@ -182,7 +182,7 @@ TEST(KillTest, SetPgid) {
       }
     }
 
-    EXPECT_THAT(pid, SyscallSucceeds());
+    ASSERT_THAT(pid, SyscallSucceeds());
 
     // Set the child's group and exit.
     ASSERT_THAT(setpgid(pid, pid), SyscallSucceeds());
@@ -208,7 +208,7 @@ TEST(KillTest, ProcessGroups) {
       pause();
     }
   }
-  EXPECT_THAT(child, SyscallSucceeds());
+  ASSERT_THAT(child, SyscallSucceeds());
 
   pid_t other_child = fork();
   if (other_child == 0) {
@@ -216,6 +216,7 @@ TEST(KillTest, ProcessGroups) {
       pause();
     }
   }
+  ASSERT_THAT(other_child, SyscallSucceeds());
 
   // Ensure the kill does not succeed without the new group.
   EXPECT_THAT(kill(-child, SIGKILL), SyscallFailsWithErrno(ESRCH));
@@ -298,7 +299,7 @@ TEST(KillTest, ChildDropsPrivsCannotKill) {
     _exit(0);
   }
 
-  EXPECT_THAT(pid, SyscallSucceeds());
+  ASSERT_THAT(pid, SyscallSucceeds());
 
   int status;
   EXPECT_THAT(RetryEINTR(waitpid)(pid, &status, 0),
@@ -316,7 +317,7 @@ TEST(KillTest, CanSIGCONTSameSession) {
     _exit(0);
   }
 
-  EXPECT_THAT(stopped_child, SyscallSucceeds());
+  ASSERT_THAT(stopped_child, SyscallSucceeds());
 
   // Put the child in its own process group. The child and parent process
   // groups also share a session.
@@ -359,7 +360,7 @@ TEST(KillTest, CanSIGCONTSameSession) {
     _exit(0);
   }
 
-  EXPECT_THAT(stopped_child, SyscallSucceeds());
+  ASSERT_THAT(stopped_child, SyscallSucceeds());
 
   // Make sure child exited normally.
   EXPECT_THAT(RetryEINTR(waitpid)(stopped_child, &status, 0),
diff --git a/test/syscalls/linux/ptrace.cc b/test/syscalls/linux/ptrace.cc
index d3b3b8b02..f063cc92b 100644
--- a/test/syscalls/linux/ptrace.cc
+++ b/test/syscalls/linux/ptrace.cc
@@ -34,6 +34,11 @@
 #include "test/util/test_util.h"
 #include "test/util/thread_util.h"
 
+DEFINE_bool(ptrace_test_execve_child, false,
+            "If true, run the "
+            "PtraceExecveTest_Execve_GetRegs_PeekUser_SIGKILL_TraceClone_"
+            "TraceExit child workload.");
+
 namespace gvisor {
 namespace testing {
 
@@ -457,26 +462,18 @@ class PtraceExecveTest : public ::testing::TestWithParam<bool> {
 };
 
 TEST_P(PtraceExecveTest, Execve_GetRegs_PeekUser_SIGKILL_TraceClone_TraceExit) {
+  ExecveArray const owned_child_argv = {"/proc/self/exe",
+                                        "--ptrace_test_execve_child"};
+  char* const* const child_argv = owned_child_argv.get();
+
   pid_t const child_pid = fork();
   if (child_pid == 0) {
-    // In child process.
-
-    // Enable tracing, then raise SIGSTOP and expect our parent to suppress it.
-    TEST_PCHECK(ptrace(PTRACE_TRACEME, 0, 0, 0) == 0);
-    MaybeSave();
-    RaiseSignal(SIGSTOP);
-    MaybeSave();
-
-    // Call execve in a non-leader thread.
-    ExecveArray const owned_child_argv = {"/proc/self/exe"};
-    char* const* const child_argv = owned_child_argv.get();
-    ScopedThread t([&] {
-      execve(child_argv[0], child_argv, /* envp = */ nullptr);
-      TEST_CHECK_MSG(false, "Survived execve? (thread)");
-    });
-    t.Join();
-    TEST_CHECK_MSG(false, "Survived execve? (main)");
-    _exit(1);
+    // In child process. The test relies on calling execve() in a non-leader
+    // thread; pthread_create() isn't async-signal-safe, so the safest way to
+    // do this is to execve() first, then enable tracing and run the expected
+    // child process behavior in the new subprocess.
+    execve(child_argv[0], child_argv, /* envp = */ nullptr);
+    TEST_PCHECK_MSG(false, "Survived execve to test child");
   }
   // In parent process.
   ASSERT_THAT(child_pid, SyscallSucceeds());
@@ -595,6 +592,28 @@ TEST_P(PtraceExecveTest, Execve_GetRegs_PeekUser_SIGKILL_TraceClone_TraceExit) {
       << " status " << status;
 }
 
+[[noreturn]] void RunExecveChild() {
+  // Enable tracing, then raise SIGSTOP and expect our parent to suppress it.
+  TEST_PCHECK(ptrace(PTRACE_TRACEME, 0, 0, 0) == 0);
+  MaybeSave();
+  RaiseSignal(SIGSTOP);
+  MaybeSave();
+
+  // Call execve() in a non-leader thread. As long as execve() succeeds, what
+  // exactly we execve() shouldn't really matter, since the tracer should kill
+  // us after execve() completes.
+  ScopedThread t([&] {
+    ExecveArray const owned_child_argv = {"/proc/self/exe",
+                                          "--this_flag_shouldnt_exist"};
+    char* const* const child_argv = owned_child_argv.get();
+    execve(child_argv[0], child_argv, /* envp = */ nullptr);
+    TEST_PCHECK_MSG(false, "Survived execve? (thread)");
+  });
+  t.Join();
+  TEST_CHECK_MSG(false, "Survived execve? (main)");
+  _exit(1);
+}
+
 INSTANTIATE_TEST_CASE_P(TraceExec, PtraceExecveTest, ::testing::Bool());
 
 // This test has expectations on when syscall-enter/exit-stops occur that are
@@ -946,3 +965,13 @@ TEST(PtraceTest, ERESTART_NoRandomSave) {
 
 }  // namespace testing
 }  // namespace gvisor
+
+int main(int argc, char** argv) {
+  gvisor::testing::TestInit(&argc, &argv);
+
+  if (FLAGS_ptrace_test_execve_child) {
+    gvisor::testing::RunExecveChild();
+  }
+
+  return RUN_ALL_TESTS();
+}
diff --git a/test/syscalls/linux/sync_file_range.cc b/test/syscalls/linux/sync_file_range.cc
index ebe4ca171..d11f58481 100644
--- a/test/syscalls/linux/sync_file_range.cc
+++ b/test/syscalls/linux/sync_file_range.cc
@@ -70,6 +70,7 @@ TEST(SyncFileRangeTest, CannotSyncFileRangeOnUnopenedFd) {
     TEST_PCHECK(errno == EBADF);
     _exit(0);
   }
+  ASSERT_THAT(pid, SyscallSucceeds());
 
   int status = 0;
   ASSERT_THAT(waitpid(pid, &status, 0), SyscallSucceedsWithValue(pid));
diff --git a/test/syscalls/linux/timers.cc b/test/syscalls/linux/timers.cc
index dfe231575..14506eb12 100644
--- a/test/syscalls/linux/timers.cc
+++ b/test/syscalls/linux/timers.cc
@@ -92,6 +92,7 @@ TEST(TimerTest, ProcessKilledOnCPUSoftLimit) {
     for (;;) {
     }
   }
+  ASSERT_THAT(pid, SyscallSucceeds());
   auto c = Cleanup([pid] {
     int status;
     EXPECT_THAT(waitpid(pid, &status, 0), SyscallSucceedsWithValue(pid));
@@ -150,6 +151,7 @@ TEST(TimerTest, ProcessPingedRepeatedlyAfterCPUSoftLimit) {
     for (;;) {
     }
   }
+  ASSERT_THAT(pid, SyscallSucceeds());
   auto c = Cleanup([pid] {
     int status;
     EXPECT_THAT(waitpid(pid, &status, 0), SyscallSucceedsWithValue(pid));
@@ -195,6 +197,7 @@ TEST(TimerTest, ProcessKilledOnCPUHardLimit) {
     for (;;) {
     }
   }
+  ASSERT_THAT(pid, SyscallSucceeds());
   auto c = Cleanup([pid] {
     int status;
     EXPECT_THAT(waitpid(pid, &status, 0), SyscallSucceedsWithValue(pid));
diff --git a/test/syscalls/linux/wait.cc b/test/syscalls/linux/wait.cc
index 0a4ec7c6a..2da0bf130 100644
--- a/test/syscalls/linux/wait.cc
+++ b/test/syscalls/linux/wait.cc
@@ -547,6 +547,7 @@ TEST_P(WaitSpecificChildTest, AfterChildExecve) {
         const_cast<char**>(child_argv));
     _exit(errno);
   }
+  ASSERT_THAT(child, SyscallSucceeds());
   EXPECT_NO_ERRNO(WaitFor(child, 0));
 }