PtraceMonitor: Use deadline manager instead of sigtimedwait

As SIGCHLD is process directed in case where there are many sandboxees running
it is not reliably delivered (any monitor can get it). Thus the whole relies
on the monitors actually polling the status every 500ms. This causes bigger
latency and unneccessary CPU load.

PiperOrigin-RevId: 704670528
Change-Id: I1e7f8a116e271d61d357eab41f7f31ebbfe5417c

sandboxed_api/sandbox2/CMakeLists.txt

@@ -470,8 +470,7 @@ add_library(sandbox2_monitor_ptrace ${SAPI_LIB_TYPE}
 )
 add_library(sandbox2::monitor_ptrace ALIAS sandbox2_monitor_ptrace)
 target_link_libraries(sandbox2_monitor_ptrace
-  PRIVATE absl::core_headers
-          absl::cleanup
+  PRIVATE absl::cleanup
           absl::flat_hash_set
           absl::flags
           absl::log
@@ -486,14 +485,15 @@ target_link_libraries(sandbox2_monitor_ptrace
           sapi::status
           sandbox2::client
           sandbox2::comms
-          sandbox2::pid_waiter
           sandbox2::result
           sandbox2::sanitizer
           sandbox2::util
   PUBLIC absl::check
+         absl::core_headers
          sandbox2::executor
          sandbox2::monitor_base
          sandbox2::notify
+         sandbox2::pid_waiter
          sandbox2::policy
          sandbox2::regs
          sandbox2::syscall

sandboxed_api/sandbox2/monitor_ptrace.cc

@@ -200,14 +200,12 @@ bool PtraceMonitor::InterruptSandboxee() {
 #define __WPTRACEEVENT(x) ((x & 0xff0000) >> 16)
 
 void PtraceMonitor::NotifyMonitor() {
-  absl::ReaderMutexLock lock(&notify_mutex_);
-  if (thread_.IsJoinable()) {
-    pthread_kill(thread_.handle(), SIGCHLD);
-  }
+  absl::MutexLock lock(&notify_mutex_);
+  pid_waiter_.SetDeadline(absl::InfinitePast());
+  notified_ = true;
 }
 
 void PtraceMonitor::Join() {
-  absl::MutexLock lock(&notify_mutex_);
   if (thread_.IsJoinable()) {
     thread_.Join();
     CHECK(IsDone()) << "Monitor did not terminate";
@@ -232,12 +230,6 @@ void PtraceMonitor::Run() {
   };
 
   absl::Cleanup setup_notify = [this] { setup_notification_.Notify(); };
-  // It'd be costly to initialize the sigset_t for each sigtimedwait()
-  // invocation, so do it once per Monitor.
-  if (!InitSetupSignals()) {
-    SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_SIGNALS);
-    return;
-  }
   // This call should be the last in the init sequence, because it can cause the
   // sandboxee to enter ptrace-stopped state, in which it will not be able to
   // send any messages over the Comms channel.
@@ -251,11 +243,15 @@ void PtraceMonitor::Run() {
   std::move(setup_notify).Invoke();
 
   bool sandboxee_exited = false;
-  PidWaiter pid_waiter(process_.main_pid);
+  pid_waiter_.SetPriorityPid(process_.main_pid);
   int status;
   // All possible still running children of main process, will be killed due to
   // PTRACE_O_EXITKILL ptrace() flag.
   while (result().final_status() == Result::UNSET) {
+    {
+      absl::MutexLock lock(&notify_mutex_);
+      notified_ = false;
+    }
     if (absl::Now() >= hard_deadline_) {
       LOG(WARNING) << "Hard deadline exceeded (timed_out=" << timed_out_
                    << ", external_kill=" << external_kill_
@@ -295,13 +291,18 @@ void PtraceMonitor::Run() {
         break;
       }
     }
-
-    pid_t ret = pid_waiter.Wait(&status);
+    absl::Time effective_deadline = hard_deadline_;
+    if (deadline != 0 && hard_deadline_ == absl::InfiniteFuture()) {
+      effective_deadline = absl::FromUnixMillis(deadline);
+    }
+    {
+      absl::MutexLock lock(&notify_mutex_);
+      if (!notified_) {
+        pid_waiter_.SetDeadline(effective_deadline);
+      }
+    }
+    pid_t ret = pid_waiter_.Wait(&status);
     if (ret == 0) {
-      constexpr timespec ts = {kWakeUpPeriodSec, kWakeUpPeriodNSec};
-      int signo = sigtimedwait(&sset_, nullptr, &ts);
-      LOG_IF(ERROR, signo != -1 && signo != SIGCHLD)
-          << "Unknown signal received: " << signo;
       continue;
     }
 
@@ -310,7 +311,7 @@ void PtraceMonitor::Run() {
         LOG(ERROR) << "PANIC(). The main process has not exited yet, "
                    << "yet we haven't seen its exit event";
         SetExitStatusCode(Result::INTERNAL_ERROR, Result::FAILED_CHILD);
-      } else {
+      } else if (errno != EINTR) {
         PLOG(ERROR) << "waitpid() failed";
       }
       continue;
@@ -374,14 +375,17 @@ void PtraceMonitor::Run() {
                  absl::GetFlag(FLAGS_sandbox2_stack_traces_collection_timeout);
     }
     for (;;) {
-      auto left = deadline - absl::Now();
       if (absl::Now() >= deadline) {
         LOG(WARNING)
             << "Waiting for sandboxee exit timed out. Sandboxee result: "
             << result_.ToString();
         break;
       }
-      pid_t ret = pid_waiter.Wait(&status);
+      {
+        absl::MutexLock lock(&notify_mutex_);
+        pid_waiter_.SetDeadline(deadline);
+      }
+      pid_t ret = pid_waiter_.Wait(&status);
       if (ret == -1) {
         if (!log_stack_traces || ret != ECHILD) {
           PLOG(ERROR) << "waitpid() failed";
@@ -397,8 +401,6 @@ void PtraceMonitor::Run() {
       }
 
       if (ret == 0) {
-        auto ts = absl::ToTimespec(left);
-        sigtimedwait(&sset_, nullptr, &ts);
         continue;
       }
 
@@ -434,26 +436,6 @@ void PtraceMonitor::LogStackTraceOfPid(pid_t pid) {
   }
 }
 
-bool PtraceMonitor::InitSetupSignals() {
-  if (sigemptyset(&sset_) == -1) {
-    PLOG(ERROR) << "sigemptyset()";
-    return false;
-  }
-
-  // sigtimedwait will react (wake-up) to arrival of this signal.
-  if (sigaddset(&sset_, SIGCHLD) == -1) {
-    PLOG(ERROR) << "sigaddset(SIGCHLD)";
-    return false;
-  }
-
-  if (pthread_sigmask(SIG_BLOCK, &sset_, nullptr) == -1) {
-    PLOG(ERROR) << "pthread_sigmask(SIG_BLOCK, SIGCHLD)";
-    return false;
-  }
-
-  return true;
-}
-
 absl::Status TryAttach(const absl::flat_hash_set<int>& tasks,
                        absl::Time deadline,
                        absl::flat_hash_set<int>& tasks_attached) {

sandboxed_api/sandbox2/monitor_ptrace.h

@@ -22,6 +22,7 @@
 #include <cstdint>
 #include <memory>
 
+#include "absl/base/thread_annotations.h"
 #include "absl/container/flat_hash_map.h"
 #include "absl/log/log.h"
 #include "absl/synchronization/mutex.h"
@@ -34,6 +35,7 @@
 #include "sandboxed_api/sandbox2/policy.h"
 #include "sandboxed_api/sandbox2/regs.h"
 #include "sandboxed_api/sandbox2/syscall.h"
+#include "sandboxed_api/sandbox2/util/pid_waiter.h"
 #include "sandboxed_api/util/thread.h"
 
 namespace sandbox2 {
@@ -62,14 +64,11 @@ class PtraceMonitor : public MonitorBase {
       absl::Time deadline = absl::Now() + limit;
       deadline_millis_.store(absl::ToUnixMillis(deadline),
                              std::memory_order_relaxed);
+      NotifyMonitor();
     }
   }
 
  private:
-  // Timeout used with sigtimedwait (0.5s).
-  static constexpr int kWakeUpPeriodSec = 0L;
-  static constexpr int kWakeUpPeriodNSec = (500L * 1000L * 1000L);
-
   // Waits for events from monitored clients and signals from the main process.
   void RunInternal() override;
   void Join() override;
@@ -127,9 +126,6 @@ class PtraceMonitor : public MonitorBase {
   // Returns false if an error occurred and process could not be interrupted.
   bool InterruptSandboxee();
 
-  // Sets up required signal masks/handlers; prepare mask for sigtimedwait().
-  bool InitSetupSignals();
-
   // ptrace(PTRACE_SEIZE) to the Client.
   // Returns success/failure status.
   bool InitPtraceAttach();
@@ -159,12 +155,16 @@ class PtraceMonitor : public MonitorBase {
   sigset_t sset_;
   // Deadline after which sandboxee get terminated via PTRACE_O_EXITKILL.
   absl::Time hard_deadline_ = absl::InfiniteFuture();
+  // PidWaiter for waiting for sandboxee events.
+  PidWaiter pid_waiter_;
 
   // Monitor thread object.
   sapi::Thread thread_;
 
-  // Synchronizes monitor thread deletion and notifying the monitor.
+  // Synchronizes deadline setting and notifying the monitor.
   absl::Mutex notify_mutex_;
+  // True iff monitor thread is notified
+  bool notified_ ABSL_GUARDED_BY(notify_mutex_) = false;
 };
 
 }  // namespace sandbox2

sandboxed_api/sandbox2/sandbox2_test.cc

@@ -182,10 +182,13 @@ TEST_P(Sandbox2Test, SandboxeeTimeoutDisabledStacktraces) {
                                              .TryBuild());
   Sandbox2 sandbox(std::move(executor), std::move(policy));
   ASSERT_THAT(SetUpSandbox(&sandbox), IsOk());
+  absl::Time start_time = absl::Now();
   ASSERT_TRUE(sandbox.RunAsync());
   sandbox.set_walltime_limit(absl::Seconds(1));
   auto result = sandbox.AwaitResult();
   EXPECT_EQ(result.final_status(), Result::TIMEOUT);
+  auto elapsed = absl::Now() - start_time;
+  EXPECT_THAT(elapsed, Lt(absl::Seconds(2)));
   EXPECT_THAT(result.stack_trace(), IsEmpty());
 }
 

sandboxed_api/sandbox2/util/pid_waiter.cc

@@ -14,6 +14,8 @@
 
 #include "sandboxed_api/sandbox2/util/pid_waiter.h"
 
+#include <sys/wait.h>
+
 #include <cerrno>
 #include <memory>