More permissive ptrace handling in edge cases

This should make multithreaded sandboxees that exec (or send `SIGKILL`) behave more reliably.

PiperOrigin-RevId: 447458426
Change-Id: Ifdace340462199dc24c8cdf25d589ef6b24991e1

sandboxed_api/sandbox2/monitor.cc

@@ -97,12 +97,6 @@ std::string ReadProcMaps(pid_t pid) {
   return contents.str();
 }
 
-void InterruptProcess(pid_t pid) {
-  if (ptrace(PTRACE_INTERRUPT, pid, 0, 0) == -1) {
-    PLOG(WARNING) << "ptrace(PTRACE_INTERRUPT, pid=" << pid << ")";
-  }
-}
-
 void ContinueProcess(pid_t pid, int signo) {
   if (ptrace(PTRACE_CONT, pid, 0, signo) == -1) {
     if (errno == ESRCH) {
@@ -415,11 +409,18 @@ void Monitor::SetAdditionalResultInfo(std::unique_ptr<Regs> regs) {
 void Monitor::KillSandboxee() {
   VLOG(1) << "Sending SIGKILL to the PID: " << pid_;
   if (kill(pid_, SIGKILL) != 0) {
-    LOG(ERROR) << "Could not send SIGKILL to PID " << pid_;
+    PLOG(ERROR) << "Could not send SIGKILL to PID " << pid_;
     SetExitStatusCode(Result::INTERNAL_ERROR, Result::FAILED_KILL);
   }
 }
 
+void Monitor::InterruptSandboxee() {
+  if (ptrace(PTRACE_INTERRUPT, pid_, 0, 0) == -1) {
+    PLOG(ERROR) << "Could not send interrupt to pid=" << pid_;
+    SetExitStatusCode(Result::INTERNAL_ERROR, Result::FAILED_INTERRUPT);
+  }
+}
+
 // Not defined in glibc.
 #define __WPTRACEEVENT(x) ((x & 0xff0000) >> 16)
 
@@ -428,7 +429,7 @@ void Monitor::MainLoop(sigset_t* sset) {
   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) {
+  for (;;) {
     int64_t deadline = deadline_millis_.load(std::memory_order_relaxed);
     if (deadline != 0 && absl::Now() >= absl::FromUnixMillis(deadline)) {
       VLOG(1) << "Sandbox process hit timeout due to the walltime timer";
@@ -438,7 +439,7 @@ void Monitor::MainLoop(sigset_t* sset) {
 
     if (!dump_stack_request_flag_.test_and_set(std::memory_order_relaxed)) {
       should_dump_stack_ = true;
-      InterruptProcess(pid_);
+      InterruptSandboxee();
     }
 
     if (!external_kill_request_flag_.test_and_set(std::memory_order_relaxed)) {
@@ -454,6 +455,10 @@ void Monitor::MainLoop(sigset_t* sset) {
       KillSandboxee();
     }
 
+    if (result_.final_status() != Result::UNSET) {
+      break;
+    }
+
     // It should be a non-blocking operation (hence WNOHANG), so this function
     // returns quickly if there are no events to be processed.
     // Prioritize main pid to avoid resource starvation
@@ -873,7 +878,12 @@ void Monitor::EventPtraceSeccomp(pid_t pid, int event_msg) {
   Regs regs(pid);
   auto status = regs.Fetch();
   if (!status.ok()) {
-    LOG(ERROR) << status;
+    // Ignore if process is killed in the meanwhile
+    if (absl::IsNotFound(status)) {
+      LOG(WARNING) << "failed to fetch regs: " << status;
+      return;
+    }
+    LOG(ERROR) << "failed to fetch regs: " << status;
     SetExitStatusCode(Result::INTERNAL_ERROR, Result::FAILED_FETCH);
     return;
   }
@@ -900,7 +910,12 @@ void Monitor::EventSyscallExit(pid_t pid) {
   Regs regs(pid);
   auto status = regs.Fetch();
   if (!status.ok()) {
-    LOG(ERROR) << status;
+    // Ignore if process is killed in the meanwhile
+    if (absl::IsNotFound(status)) {
+      LOG(WARNING) << "failed to fetch regs: " << status;
+      return;
+    }
+    LOG(ERROR) << "failed to fetch regs: " << status;
     SetExitStatusCode(Result::INTERNAL_ERROR, Result::FAILED_FETCH);
     return;
   }
@@ -970,17 +985,22 @@ void Monitor::EventPtraceExit(pid_t pid, int event_msg) {
     return;
   }
 
+  const bool is_seccomp =
+      WIFSIGNALED(event_msg) && WTERMSIG(event_msg) == SIGSYS;
+
   // Fetch the registers as we'll need them to fill the result in any case
   auto regs = absl::make_unique<Regs>(pid);
-  auto status = regs->Fetch();
+  if (is_seccomp || pid == pid_) {
-  if (!status.ok()) {
+    auto status = regs->Fetch();
-    LOG(ERROR) << "failed to fetch regs: " << status;
+    if (!status.ok()) {
-    SetExitStatusCode(Result::INTERNAL_ERROR, Result::FAILED_FETCH);
+      LOG(ERROR) << "failed to fetch regs: " << status;
-    return;
+      SetExitStatusCode(Result::INTERNAL_ERROR, Result::FAILED_FETCH);
+      return;
+    }
   }
 
   // Process signaled due to seccomp violation.
-  if (WIFSIGNALED(event_msg) && WTERMSIG(event_msg) == SIGSYS) {
+  if (is_seccomp) {
     VLOG(1) << "PID: " << pid << " violation uncovered via the EXIT_EVENT";
     ActionProcessSyscallViolation(
         regs.get(), regs->ToSyscall(Syscall::GetHostArch()), kSyscallViolation);

sandboxed_api/sandbox2/monitor.h

@@ -99,6 +99,9 @@ class Monitor final {
   // Kills the main traced PID with PTRACE_KILL.
   void KillSandboxee();
 
+  // Interrupts the main traced PID with PTRACE_INTERRUPT.
+  void InterruptSandboxee();
+
   // Waits for events from monitored clients and signals from the main process.
   void MainLoop(sigset_t* sset);
 

sandboxed_api/sandbox2/result.cc

@@ -187,6 +187,8 @@ std::string Result::ReasonCodeEnumToString(ReasonCodeEnum value) {
       return "FAILED_MONITOR";
     case sandbox2::Result::FAILED_KILL:
       return "FAILED_KILL";
+    case sandbox2::Result::FAILED_INTERRUPT:
+      return "FAILED_INTERRUPT";
     case sandbox2::Result::FAILED_CHILD:
       return "FAILED_CHILD";
     case sandbox2::Result::FAILED_INSPECT:

sandboxed_api/sandbox2/result.h

@@ -79,6 +79,7 @@ class Result {
     FAILED_GETEVENT,
     FAILED_MONITOR,
     FAILED_KILL,
+    FAILED_INTERRUPT,
     FAILED_CHILD,
     FAILED_INSPECT,