Revert of monitor code update.

PiperOrigin-RevId: 247255592
Change-Id: I3656ea1628418321b1b8b02660b6a51a58c2c61f

sandboxed_api/sandbox2/monitor.cc

@@ -84,26 +84,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) {
-    PLOG(ERROR) << "ptrace(PTRACE_CONT, pid=" << pid << ", sig=" << signo
-                << ")";
-  }
-}
-
-void StopProcess(pid_t pid, int signo) {
-  if (ptrace(PTRACE_LISTEN, pid, 0, signo) == -1) {
-    PLOG(ERROR) << "ptrace(PTRACE_LISTEN, pid=" << pid << ", sig=" << signo
-                << ")";
-  }
-}
-
 }  // namespace
 
 Monitor::Monitor(Executor* executor, Policy* policy, Notify* notify)
@@ -112,7 +92,7 @@ Monitor::Monitor(Executor* executor, Policy* policy, Notify* notify)
       policy_(policy),
       comms_(executor_->ipc()->comms()),
       ipc_(executor_->ipc()),
-      setup_counter_(1),
+      setup_counter_(new absl::BlockingCounter(1)),
       done_(false),
       wait_for_execve_(executor->enable_sandboxing_pre_execve_) {
   std::string path =
@@ -142,9 +122,9 @@ void LogContainer(const std::vector<std::string>& container) {
 
 void Monitor::Run() {
   using DecrementCounter = decltype(setup_counter_);
-  std::unique_ptr<DecrementCounter, void (*)(DecrementCounter*)>
+  std::unique_ptr<DecrementCounter, std::function<void(DecrementCounter*)>>
       decrement_count{&setup_counter_, [](DecrementCounter* counter) {
-                        counter->DecrementCount();
+                        (*counter)->DecrementCount();
                       }};
 
   struct MonitorCleanup {
@@ -159,7 +139,7 @@ void Monitor::Run() {
   } monitor_cleanup{this};
 
   if (!InitSetupTimer()) {
-    SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_TIMERS);
+    result_.SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_TIMERS);
     return;
   }
 
@@ -167,7 +147,7 @@ void Monitor::Run() {
   // invocation, so do it once per Monitor.
   sigset_t sigtimedwait_sset;
   if (!InitSetupSignals(&sigtimedwait_sset)) {
-    SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_SIGNALS);
+    result_.SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_SIGNALS);
     return;
   }
 
@@ -200,43 +180,43 @@ void Monitor::Run() {
   }
 
   if (pid_ < 0) {
-    SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_SUBPROCESS);
+    result_.SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_SUBPROCESS);
     return;
   }
 
   if (!notify_->EventStarted(pid_, comms_)) {
-    SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_NOTIFY);
+    result_.SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_NOTIFY);
     return;
   }
   if (!InitAcceptConnection()) {
-    SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_CONNECTION);
+    result_.SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_CONNECTION);
     return;
   }
   if (!InitSendIPC()) {
-    SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_IPC);
+    result_.SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_IPC);
     return;
   }
   if (!InitSendCwd()) {
-    SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_CWD);
+    result_.SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_CWD);
     return;
   }
   if (!InitSendPolicy()) {
-    SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_POLICY);
+    result_.SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_POLICY);
     return;
   }
   if (!WaitForSandboxReady()) {
-    SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_WAIT);
+    result_.SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_WAIT);
     return;
   }
   if (!InitApplyLimits()) {
-    SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_LIMITS);
+    result_.SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_LIMITS);
     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.
   if (!InitPtraceAttach()) {
-    SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_PTRACE);
+    result_.SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_PTRACE);
     return;
   }
 
@@ -259,59 +239,6 @@ bool Monitor::IsActivelyMonitoring() {
 
 void Monitor::SetActivelyMonitoring() { wait_for_execve_ = false; }
 
-void Monitor::SetExitStatusCode(Result::StatusEnum final_status,
-                                uintptr_t reason_code) {
-  CHECK(result_.final_status() == Result::UNSET);
-  result_.SetExitStatusCode(final_status, reason_code);
-}
-
-bool Monitor::ShouldCollectStackTrace() {
-  // Only get the stacktrace if we are not in the libunwind sandbox (avoid
-  // recursion).
-  bool stacktrace_collection_possible =
-      policy_->GetNamespace() && executor_->libunwind_sbox_for_pid_ == 0;
-  if (!stacktrace_collection_possible) {
-    LOG(ERROR) << "Cannot collect stack trace. Unwind pid "
-               << executor_->libunwind_sbox_for_pid_ << ", namespace "
-               << policy_->GetNamespace();
-    return false;
-  }
-  switch (result_.final_status()) {
-    case Result::EXTERNAL_KILL:
-      return policy_->collect_stacktrace_on_kill_;
-    case Result::TIMEOUT:
-      return policy_->collect_stacktrace_on_timeout_;
-    case Result::SIGNALED:
-      return policy_->collect_stacktrace_on_signal_;
-    case Result::VIOLATION:
-      return policy_->collect_stacktrace_on_violation_;
-    default:
-      return false;
-  }
-}
-
-void Monitor::SetAdditionalResultInfo(std::unique_ptr<Regs> regs) {
-  pid_t pid = regs->pid();
-  result_.SetRegs(std::move(regs));
-  result_.SetProgName(util::GetProgName(pid));
-  result_.SetProcMaps(ReadProcMaps(pid_));
-  if (ShouldCollectStackTrace()) {
-    result_.SetStackTrace(
-        GetStackTrace(result_.GetRegs(), policy_->GetNamespace()->mounts()));
-    LOG(ERROR) << "Stack trace: " << result_.GetStackTrace();
-  } else {
-    LOG(ERROR) << "Stack traces have been disabled";
-  }
-}
-
-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_;
-    SetExitStatusCode(Result::INTERNAL_ERROR, Result::FAILED_KILL);
-  }
-}
-
 void Monitor::MainSignals(int signo, siginfo_t* si) {
   VLOG(3) << "Signal '" << strsignal(signo) << "' (" << signo
           << ") received from PID: " << si->si_pid;
@@ -341,13 +268,11 @@ void Monitor::MainSignals(int signo, siginfo_t* si) {
   switch (signo) {
     case Monitor::kExternalKillSignal:
       VLOG(1) << "Will kill the main pid";
-      external_kill_ = true;
+      ActionProcessKill(pid_, Result::EXTERNAL_KILL, 0);
-      KillSandboxee();
       break;
     case Monitor::kTimerWallTimeSignal:
       VLOG(1) << "Sandbox process hit timeout due to the walltime timer";
-      timed_out_ = true;
+      ActionProcessKill(pid_, Result::TIMEOUT, 0);
-      KillSandboxee();
       break;
     case Monitor::kTimerSetSignal:
       VLOG(1) << "Will set the walltime timer to " << si->si_value.sival_int
@@ -357,7 +282,7 @@ void Monitor::MainSignals(int signo, siginfo_t* si) {
     case Monitor::kDumpStackSignal:
       VLOG(1) << "Dump the main pid's stack";
       should_dump_stack_ = true;
-      InterruptProcess(pid_);
+      PidInterrupt(pid_);
       break;
     default:
       LOG(ERROR) << "Unknown signal received: " << signo;
@@ -367,70 +292,86 @@ void Monitor::MainSignals(int signo, siginfo_t* si) {
 
 // Not defined in glibc.
 #define __WPTRACEEVENT(x) ((x & 0xff0000) >> 16)
-void Monitor::MainWait() {
+bool Monitor::MainWait() {
   // All possible process status change event must be checked as SIGCHLD
   // is reported once only for all events that arrived at the same time.
-  int ret;
-  int status;
   for (;;) {
+    int status;
     // It should be a non-blocking operation (hence WNOHANG), so this function
     // returns quickly if there are no events to be processed.
-    ret = waitpid(-1, &status, __WNOTHREAD | __WALL | WUNTRACED | WNOHANG);
+    int ret = waitpid(-1, &status, __WNOTHREAD | __WALL | WUNTRACED | WNOHANG);
 
     // No traced processes have changed their status yet.
     if (ret == 0) {
-      return;
+      return false;
     }
 
-    if (ret != -1) {
+    if (ret == -1 && errno == ECHILD) {
-      break;
-    }
-
-    if (errno == ECHILD) {
       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);
+      // We'll simply exit which will kill all remaining processes (if
-      return;
+      // there are any) because of the PTRACE_O_EXITKILL ptrace() flag.
+      return true;
     }
-
+    if (ret == -1 && errno == EINTR) {
-    if (errno == EINTR) {
       VLOG(3) << "waitpid() interruped with EINTR";
-    } else {
+      continue;
+    }
+    if (ret == -1) {
       PLOG(ERROR) << "waitpid() failed";
+      continue;
     }
-  }
 
-  VLOG(3) << "waitpid() returned with PID: " << ret << ", status: " << status;
+    VLOG(3) << "waitpid() returned with PID: " << ret << ", status: " << status;
 
-  if (WIFEXITED(status)) {
+    if (WIFEXITED(status)) {
-    VLOG(1) << "PID: " << ret << " finished with code: " << WEXITSTATUS(status);
+      VLOG(1) << "PID: " << ret
-    // That's the main process, set the exit code, and exit. It will kill
+              << " finished with code: " << WEXITSTATUS(status);
-    // all remaining processes (if there are any) because of the
+      // That's the main process, set the exit code, and exit. It will kill
-    // PTRACE_O_EXITKILL ptrace() flag.
+      // all remaining processes (if there are any) because of the
-    if (ret == pid_) {
+      // PTRACE_O_EXITKILL ptrace() flag.
-      if (IsActivelyMonitoring()) {
+      if (ret == pid_) {
-        SetExitStatusCode(Result::OK, WEXITSTATUS(status));
+        if (IsActivelyMonitoring()) {
-      } else {
+          result_.SetExitStatusCode(Result::OK, WEXITSTATUS(status));
-        SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_MONITOR);
+        } else {
+          result_.SetExitStatusCode(Result::SETUP_ERROR,
+                                    Result::FAILED_MONITOR);
+        }
+        return true;
       }
+    } else if (WIFSIGNALED(status)) {
+      VLOG(1) << "PID: " << ret << " terminated with signal: "
+              << util::GetSignalName(WTERMSIG(status));
+      if (ret == pid_) {
+        // That's the main process, depending on the result of the process take
+        // the register content and/or the stack trace. The death of this
+        // process will cause all remaining processes to be killed (if there are
+        // any), see the PTRACE_O_EXITKILL ptrace() flag.
+
+        // When the process is killed from a signal from within the result
+        // status will be still unset, fix this.
+        // The other cases should either be already handled, or (in the case of
+        // Result::OK) should be impossible to reach.
+        if (result_.final_status() == Result::UNSET) {
+          result_.SetExitStatusCode(Result::SIGNALED, WTERMSIG(status));
+        } else if (result_.final_status() == Result::OK) {
+          LOG(ERROR) << "Unexpected codepath taken";
+        }
+        return true;
+      }
+    } else if (WIFSTOPPED(status)) {
+      VLOG(2) << "PID: " << ret
+              << " received signal: " << util::GetSignalName(WSTOPSIG(status))
+              << " with event: " << __WPTRACEEVENT(status);
+      StateProcessStopped(ret, status);
+    } else if (WIFCONTINUED(status)) {
+      VLOG(2) << "PID: " << ret << " is being continued";
     }
-  } else if (WIFSIGNALED(status)) {
-    VLOG(1) << "PID: " << ret << " terminated with signal: "
-            << util::GetSignalName(WTERMSIG(status));
-  } else if (WIFSTOPPED(status)) {
-    VLOG(2) << "PID: " << ret
-            << " received signal: " << util::GetSignalName(WSTOPSIG(status))
-            << " with event: " << __WPTRACEEVENT(status);
-    StateProcessStopped(ret, status);
-  } else if (WIFCONTINUED(status)) {
-    VLOG(2) << "PID: " << ret << " is being continued";
   }
 }
 
 void Monitor::MainLoop(sigset_t* sset) {
-  // All possible still running children of main process, will be killed due to
+  for (;;) {
-  // PTRACE_O_EXITKILL ptrace() flag.
-  while (result_.final_status() == Result::UNSET) {
     // Use a time-out, so we can check for missed waitpid() events. It should
     // not happen during regular operations, so it's a defense-in-depth
     // mechanism against SIGCHLD signals being lost by the kernel (since these
@@ -447,12 +388,15 @@ void Monitor::MainLoop(sigset_t* sset) {
       MainSignals(ret, &si);
     }
 
+    // If CheckWait reported no more traced processes, or that
+    // the main pid had exited, we should break this loop (i.e. our job is
+    // done here).
     //
     // MainWait() should use a not-blocking (e.g. WNOHANG with waitpid())
     // syntax, so it returns quickly if there are not status changes in
     // traced processes.
-    if (result_.final_status() == Result::UNSET) {
+    if (MainWait()) {
-      MainWait();
+      return;
     }
   }
 }
@@ -784,6 +728,20 @@ bool Monitor::InitAcceptConnection() {
   return true;
 }
 
+void Monitor::ActionProcessContinue(pid_t pid, int signo) {
+  if (ptrace(PTRACE_CONT, pid, 0, signo) == -1) {
+    PLOG(ERROR) << "ptrace(PTRACE_CONT, pid=" << pid << ", sig=" << signo
+                << ")";
+  }
+}
+
+void Monitor::ActionProcessStop(pid_t pid, int signo) {
+  if (ptrace(PTRACE_LISTEN, pid, 0, signo) == -1) {
+    PLOG(ERROR) << "ptrace(PTRACE_LISTEN, pid=" << pid << ", sig=" << signo
+                << ")";
+  }
+}
+
 void Monitor::ActionProcessSyscall(Regs* regs, const Syscall& syscall) {
   // If the sandboxing is not enabled yet, allow the first __NR_execveat.
   if (syscall.nr() == __NR_execveat && !IsActivelyMonitoring()) {
@@ -791,7 +749,7 @@ void Monitor::ActionProcessSyscall(Regs* regs, const Syscall& syscall) {
             << "SYSCALL ::: PID: " << regs->pid() << ", PROG: '"
             << util::GetProgName(regs->pid())
             << "' : " << syscall.GetDescription();
-    ContinueProcess(regs->pid(), 0);
+    ActionProcessContinue(regs->pid(), 0);
     return;
   }
 
@@ -803,7 +761,7 @@ void Monitor::ActionProcessSyscall(Regs* regs, const Syscall& syscall) {
                  << ", PROG: '" << util::GetProgName(regs->pid())
                  << "' : " << syscall.GetDescription();
 
-    ContinueProcess(regs->pid(), 0);
+    ActionProcessContinue(regs->pid(), 0);
     return;
   }
 
@@ -814,12 +772,12 @@ void Monitor::ActionProcessSyscall(Regs* regs, const Syscall& syscall) {
     std::string syscall_description = syscall.GetDescription();
     PCHECK(absl::FPrintF(log_file_, "PID: %d %s\n", regs->pid(),
                          syscall_description) >= 0);
-    ContinueProcess(regs->pid(), 0);
+    ActionProcessContinue(regs->pid(), 0);
     return;
   }
 
   if (absl::GetFlag(FLAGS_sandbox2_danger_danger_permit_all)) {
-    ContinueProcess(regs->pid(), 0);
+    ActionProcessContinue(regs->pid(), 0);
     return;
   }
 
@@ -828,20 +786,40 @@ void Monitor::ActionProcessSyscall(Regs* regs, const Syscall& syscall) {
 
 void Monitor::ActionProcessSyscallViolation(Regs* regs, const Syscall& syscall,
                                             ViolationType violation_type) {
-  LogSyscallViolation(syscall);
+  pid_t pid = regs->pid();
+
+  LogAccessViolation(syscall);
   notify_->EventSyscallViolation(syscall, violation_type);
-  SetExitStatusCode(Result::VIOLATION, syscall.nr());
+  result_.SetExitStatusCode(Result::VIOLATION, syscall.nr());
   result_.SetSyscall(absl::make_unique<Syscall>(syscall));
-  SetAdditionalResultInfo(absl::make_unique<Regs>(*regs));
+  // Only get the stacktrace if we are not in the libunwind sandbox (avoid
-  // Rewrite the syscall argument to something invalid (-1).
+  // recursion).
-  // The process will be killed anyway so this is just a precaution.
+  if (executor_->libunwind_sbox_for_pid_ == 0 && policy_->GetNamespace()) {
+    if (policy_->collect_stacktrace_on_violation_) {
+      result_.SetStackTrace(
+          GetStackTrace(regs, policy_->GetNamespace()->mounts()));
+      LOG(ERROR) << "Stack trace: " << result_.GetStackTrace();
+    } else {
+      LOG(ERROR) << "Stack traces have been disabled";
+    }
+  }
+  // We make the result object create its own Reg instance. our regs is a
+  // pointer to a stack variable which might not live long enough.
+  result_.LoadRegs(pid);
+  result_.SetProgName(util::GetProgName(pid));
+  result_.SetProcMaps(ReadProcMaps(pid_));
+
+  // Rewrite the syscall argument to something invalid (-1). The process will
+  // be killed by ActionProcessKill(), so this is just a precaution.
   auto status = regs->SkipSyscallReturnValue(-ENOSYS);
   if (!status.ok()) {
     LOG(ERROR) << status;
   }
+
+  ActionProcessKill(pid, Result::VIOLATION, syscall.nr());
 }
 
-void Monitor::LogSyscallViolation(const Syscall& syscall) const {
+void Monitor::LogAccessViolation(const Syscall& syscall) {
   // Do not unwind libunwind.
   if (executor_->libunwind_sbox_for_pid_ != 0) {
     LOG(ERROR) << "Sandbox violation during execution of libunwind: "
@@ -849,16 +827,58 @@ void Monitor::LogSyscallViolation(const Syscall& syscall) const {
     return;
   }
 
+  uintptr_t syscall_nr = syscall.nr();
+  uintptr_t arg0 = syscall.args()[0];
+
   // So, this is an invalid syscall. Will be killed by seccomp-bpf policies as
   // well, but we should be on a safe side here as well.
   LOG(ERROR) << "SANDBOX VIOLATION : PID: " << syscall.pid() << ", PROG: '"
              << util::GetProgName(syscall.pid())
              << "' : " << syscall.GetDescription();
 
-  LogSyscallViolationExplanation(syscall);
+  // This follows policy in Policy::GetDefaultPolicy - keep it in sync.
+  if (syscall.arch() != Syscall::GetHostArch()) {
+    LOG(ERROR)
+        << "This is a violation because the syscall was issued because the"
+        << " sandboxee and executor architectures are different.";
+    return;
+  }
+
+  if (syscall_nr == __NR_ptrace) {
+    LOG(ERROR)
+        << "This is a violation because the ptrace syscall would be unsafe in"
+        << " sandbox2, so it has been blocked.";
+    return;
+  }
+  if (syscall_nr == __NR_bpf) {
+    LOG(ERROR)
+        << "This is a violation because the bpf syscall would be risky in"
+        << " a sandbox, so it has been blocked.";
+    return;
+  }
+
+  if (syscall_nr == __NR_clone && ((arg0 & CLONE_UNTRACED) != 0)) {
+    LOG(ERROR) << "This is a violation because calling clone with CLONE_UNTRACE"
+               << " would be unsafe in sandbox2, so it has been blocked.";
+    return;
+  }
+}
+
+void Monitor::ActionProcessKill(pid_t pid, Result::StatusEnum status,
+                                uintptr_t code) {
+  // Avoid overwriting result if we set it for instance after a violation.
+  if (result_.final_status() == Result::UNSET) {
+    result_.SetExitStatusCode(status, code);
+  }
+
+  VLOG(1) << "Sending SIGKILL to the PID: " << pid_;
+  if (kill(pid_, SIGKILL) != 0) {
+    LOG(FATAL) << "Could not send SIGKILL to PID " << pid_;
+  }
 }
 
 void Monitor::EventPtraceSeccomp(pid_t pid, int event_msg) {
+  VLOG(1) << "PID: " << pid << " violation uncovered via the SECCOMP_EVENT";
   // If the seccomp-policy is using RET_TRACE, we request that it returns the
   // syscall architecture identifier in the SECCOMP_RET_DATA.
   const auto syscall_arch = static_cast<Syscall::CpuArch>(event_msg);
@@ -866,7 +886,7 @@ void Monitor::EventPtraceSeccomp(pid_t pid, int event_msg) {
   auto status = regs.Fetch();
   if (!status.ok()) {
     LOG(ERROR) << status;
-    SetExitStatusCode(Result::INTERNAL_ERROR, Result::FAILED_FETCH);
+    ActionProcessKill(pid, Result::INTERNAL_ERROR, Result::FAILED_FETCH);
     return;
   }
 
@@ -887,48 +907,84 @@ void Monitor::EventPtraceExec(pid_t pid, int event_msg) {
             << ". SANDBOX ENABLED!";
     SetActivelyMonitoring();
   }
-  ContinueProcess(pid, 0);
+  ActionProcessContinue(pid, 0);
 }
 
 void Monitor::EventPtraceExit(pid_t pid, int event_msg) {
-  // A regular exit, let it continue (fast-path).
+  // A regular exit, let it continue.
   if (WIFEXITED(event_msg)) {
-    ContinueProcess(pid, 0);
+    ActionProcessContinue(pid, 0);
     return;
   }
 
-  // Fetch the registers as we'll need them to fill the result in any case
+  // Everything except the SECCOMP violation can continue.
-  auto regs = absl::make_unique<Regs>(pid);
+  if (!WIFSIGNALED(event_msg) || WTERMSIG(event_msg) != SIGSYS) {
-  auto status = regs->Fetch();
+    // Process is dying because it received a signal.
-  if (!status.ok()) {
+    // This can occur in three cases:
-    LOG(ERROR) << "failed to fetch regs: " << status;
+    // 1) Process was killed from the sandbox, in this case the result status
-    SetExitStatusCode(Result::INTERNAL_ERROR, Result::FAILED_FETCH);
+    //    was already set to Result::EXTERNAL_KILL. We do not get the stack
-    return;
+    //    trace in this case.
-  }
+    // 2) Process was killed because it hit a timeout. The result status is
-
+    //    also already set, however we are interested in the stack trace.
-  // Process signaled due to seccomp violation.
+    // 3) Regular signal. We need to obtain everything. The status will be set
-  if (WIFSIGNALED(event_msg) && WTERMSIG(event_msg) == SIGSYS) {
+    //    upon the process exit handler.
-    VLOG(1) << "PID: " << pid << " violation uncovered via the EXIT_EVENT";
+    if (pid == pid_) {
-    ActionProcessSyscallViolation(
+      result_.LoadRegs(pid_);
-        regs.get(), regs->ToSyscall(Syscall::GetHostArch()), kSyscallViolation);
+      result_.SetProgName(util::GetProgName(pid_));
-    return;
+      result_.SetProcMaps(ReadProcMaps(pid_));
-  }
+      bool stacktrace_collection_possible =
-
+          policy_->GetNamespace() && executor_->libunwind_sbox_for_pid_ == 0;
-  // This can be reached in three cases:
+      auto collect_stacktrace = [this]() {
-  // 1) Process was killed from the sandbox.
+        result_.SetStackTrace(GetStackTrace(result_.GetRegs(),
-  // 2) Process was killed because it hit a timeout.
+                                            policy_->GetNamespace()->mounts()));
-  // 3) Regular signal/other exit cause.
+      };
-  if (pid == pid_) {
+      switch (result_.final_status()) {
-    if (external_kill_) {
+        case Result::EXTERNAL_KILL:
-      SetExitStatusCode(Result::EXTERNAL_KILL, 0);
+          if (stacktrace_collection_possible &&
-    } else if (timed_out_) {
+              policy_->collect_stacktrace_on_kill_) {
-      SetExitStatusCode(Result::TIMEOUT, 0);
+            collect_stacktrace();
-    } else {
+          }
-      SetExitStatusCode(Result::SIGNALED, WTERMSIG(event_msg));
+          break;
+        case Result::TIMEOUT:
+          if (stacktrace_collection_possible &&
+              policy_->collect_stacktrace_on_timeout_) {
+            collect_stacktrace();
+          }
+          break;
+        case Result::VIOLATION:
+          break;
+        case Result::UNSET:
+          // Regular signal.
+          if (stacktrace_collection_possible &&
+              policy_->collect_stacktrace_on_signal_) {
+            collect_stacktrace();
+          }
+          break;
+        default:
+          LOG(ERROR) << "Unexpected codepath taken";
+          break;
+      }
     }
-    SetAdditionalResultInfo(std::move(regs));
+
+    ActionProcessContinue(pid, 0);
+    return;
   }
-  ContinueProcess(pid, 0);
+
+  VLOG(1) << "PID: " << pid << " violation uncovered via the EXIT_EVENT";
+
+  // We do not generate the stack trace in the SECCOMP case as it will be
+  // generated during ActionProcessSyscallViolation anyway.
+  Regs regs(pid);
+  auto status = regs.Fetch();
+  if (!status.ok()) {
+    LOG(ERROR) << status;
+    ActionProcessKill(pid, Result::INTERNAL_ERROR, Result::FAILED_FETCH);
+    return;
+  }
+
+  auto syscall = regs.ToSyscall(Syscall::GetHostArch());
+
+  ActionProcessSyscallViolation(&regs, syscall, kSyscallViolation);
 }
 
 void Monitor::EventPtraceStop(pid_t pid, int stopsig) {
@@ -936,13 +992,13 @@ void Monitor::EventPtraceStop(pid_t pid, int stopsig) {
   // flags to ptrace(PTRACE_SEIZE) might generate this event with SIGTRAP.
   if (stopsig != SIGSTOP && stopsig != SIGTSTP && stopsig != SIGTTIN &&
       stopsig != SIGTTOU) {
-    ContinueProcess(pid, 0);
+    ActionProcessContinue(pid, 0);
     return;
   }
   // It's our PID stop signal. Stop it.
   VLOG(2) << "PID: " << pid << " stopped due to "
           << util::GetSignalName(stopsig);
-  StopProcess(pid, 0);
+  ActionProcessStop(pid, 0);
 }
 
 void Monitor::StateProcessStopped(pid_t pid, int status) {
@@ -952,7 +1008,7 @@ void Monitor::StateProcessStopped(pid_t pid, int status) {
     VLOG(2) << "PID: " << pid
             << " received signal: " << util::GetSignalName(stopsig);
     notify_->EventSignal(pid, stopsig);
-    ContinueProcess(pid, stopsig);
+    ActionProcessContinue(pid, stopsig);
     return;
   }
 
@@ -965,13 +1021,12 @@ void Monitor::StateProcessStopped(pid_t pid, int status) {
       return;
     }
     PLOG(ERROR) << "ptrace(PTRACE_GETEVENTMSG, " << pid << ")";
-    SetExitStatusCode(Result::INTERNAL_ERROR, Result::FAILED_GETEVENT);
+    ActionProcessKill(pid, Result::INTERNAL_ERROR, Result::FAILED_GETEVENT);
     return;
   }
 
-  if (ABSL_PREDICT_FALSE(pid == pid_ && should_dump_stack_ &&
+  if (pid == pid_ && should_dump_stack_ &&
-                         executor_->libunwind_sbox_for_pid_ == 0 &&
+      executor_->libunwind_sbox_for_pid_ == 0 && policy_->GetNamespace()) {
-                         policy_->GetNamespace())) {
     Regs regs(pid);
     auto status = regs.Fetch();
     if (status.ok()) {
@@ -995,7 +1050,7 @@ void Monitor::StateProcessStopped(pid_t pid, int status) {
     case PTRACE_EVENT_CLONE:
       /* fall through */
     case PTRACE_EVENT_VFORK_DONE:
-      ContinueProcess(pid, 0);
+      ActionProcessContinue(pid, 0);
       break;
     case PTRACE_EVENT_EXEC:
       VLOG(2) << "PID: " << pid << " PTRACE_EVENT_EXEC, PID: " << event_msg;
@@ -1020,34 +1075,9 @@ void Monitor::StateProcessStopped(pid_t pid, int status) {
   }
 }
 
-void Monitor::LogSyscallViolationExplanation(const Syscall& syscall) const {
+void Monitor::PidInterrupt(pid_t pid) {
-  const uintptr_t syscall_nr = syscall.nr();
+  if (ptrace(PTRACE_INTERRUPT, pid, 0, 0) == -1) {
-  const uintptr_t arg0 = syscall.args()[0];
+    PLOG(WARNING) << "ptrace(PTRACE_INTERRUPT, pid=" << pid << ")";
-  const uintptr_t arg3 = syscall.args()[3];
-
-  // This follows policy in Policy::GetDefaultPolicy - keep it in sync.
-  if (syscall.arch() != Syscall::GetHostArch()) {
-    LOG(ERROR)
-        << "This is a violation because the syscall was issued because the"
-        << " sandboxee and executor architectures are different.";
-    return;
-  }
-  if (syscall_nr == __NR_ptrace) {
-    LOG(ERROR)
-        << "This is a violation because the ptrace syscall would be unsafe in"
-        << " sandbox2, so it has been blocked.";
-    return;
-  }
-  if (syscall_nr == __NR_bpf) {
-    LOG(ERROR)
-        << "This is a violation because the bpf syscall would be risky in"
-        << " a sandbox, so it has been blocked.";
-    return;
-  }
-  if (syscall_nr == __NR_clone && ((arg0 & CLONE_UNTRACED) != 0)) {
-    LOG(ERROR) << "This is a violation because calling clone with CLONE_UNTRACE"
-               << " would be unsafe in sandbox2, so it has been blocked.";
-    return;
   }
 }
 

sandboxed_api/sandbox2/monitor.h

@@ -83,6 +83,16 @@ class Monitor final {
   bool IsActivelyMonitoring();
   void SetActivelyMonitoring();
 
+  // Waits for events from monitored clients and signals from the main process.
+  void MainLoop(sigset_t* sset);
+
+  // Analyzes signals which Monitor might have already received.
+  void MainSignals(int signo, siginfo_t* si);
+
+  // Analyzes any possible children process status changes; returns 'true' if
+  // there are no more processes to track.
+  bool MainWait();
+
   // Sends Policy to the Client.
   // Returns success/failure status.
   bool InitSendPolicy();
@@ -127,41 +137,27 @@ class Monitor final {
   // Arms the walltime timer, absl::ZeroDuration() disarms the timer.
   bool TimerArm(absl::Duration duration);
 
-  // Kills the main traced PID with PTRACE_KILL.
+  // Final action with regard to PID.
-  void KillSandboxee();
+  // Continues PID with an optional signal.
+  void ActionProcessContinue(pid_t pid, int signo);
 
-  // Waits for events from monitored clients and signals from the main process.
+  // Stops the PID with an optional signal.
-  void MainLoop(sigset_t* sset);
+  void ActionProcessStop(pid_t pid, int signo);
-  // Analyzes signals which Monitor might have already received.
-  void MainSignals(int signo, siginfo_t* si);
-  // Analyzes any possible children process status changes
-  void MainWait();
-
-  // Process with given PID changed state to a stopped state.
-  void StateProcessStopped(pid_t pid, int status);
 
   // Logs the syscall violation and kills the process afterwards.
   void ActionProcessSyscallViolation(Regs* regs, const Syscall& syscall,
                                      ViolationType violation_type);
 
-  // PID called a traced syscall, or was killed due to syscall.
+  // Prints a SANDBOX VIOLATION message based on the registers.
+  // If the registers match something disallowed by Policy::GetDefaultPolicy,
+  // then it also prints a additional description of the reason.
+  void LogAccessViolation(const Syscall& syscall);
+
+  // PID called a syscall, or was killed due to syscall.
   void ActionProcessSyscall(Regs* regs, const Syscall& syscall);
 
-  // Sets basic info status and reason code in the result object.
+  // Kills the PID with PTRACE_KILL.
-  void SetExitStatusCode(Result::StatusEnum final_status,
+  void ActionProcessKill(pid_t pid, Result::StatusEnum status, uintptr_t code);
-                         uintptr_t reason_code);
-  // Whether a stack trace should be collected given the current status
-  bool ShouldCollectStackTrace();
-  // Sets additional information in the result object, such as program name,
-  // stack trace etc.
-  void SetAdditionalResultInfo(std::unique_ptr<Regs> regs);
-
-  // Logs a SANDBOX VIOLATION message based on the registers and additional
-  // explanation for the reason of the violation.
-  void LogSyscallViolation(const Syscall& syscall) const;
-  // Logs an additional explanation for the possible reason of the violation
-  // based on the registers.
-  void LogSyscallViolationExplanation(const Syscall& syscall) const;
 
   // Ptrace events:
   // Syscall violation processing path.
@@ -176,6 +172,14 @@ class Monitor final {
   // Processes stop path.
   void EventPtraceStop(pid_t pid, int stopsig);
 
+  // Changes the state of a given PID:
+  // Process is in a stopped state.
+  void StateProcessStopped(pid_t pid, int status);
+
+  // Helpers operating on PIDs.
+  // Interrupts the PID.
+  void PidInterrupt(pid_t pid);
+
   // Internal objects, owned by the Sandbox2 object.
   Executor* executor_;
   Notify* notify_;
@@ -189,7 +193,7 @@ class Monitor final {
   // Parent (the Sandbox2 object) waits on it, until we either enable
   // monitoring of a process (sandboxee) successfully, or the setup process
   // fails.
-  absl::BlockingCounter setup_counter_;
+  std::unique_ptr<absl::BlockingCounter> setup_counter_;
   // The Wall-Time timer for traced processes.
   std::unique_ptr<timer_t> walltime_timer_;
 
@@ -202,10 +206,6 @@ class Monitor final {
   absl::Mutex done_mutex_;
   // Should we dump the main sandboxed PID's stack?
   bool should_dump_stack_ = false;
-  // Was external kill request received
-  bool external_kill_ = false;
-  // Is the sandboxee timed out
-  bool timed_out_ = false;
 
   // Is the sandboxee actively monitored, or maybe we're waiting for execve()?
   bool wait_for_execve_;

sandboxed_api/sandbox2/result.cc

@@ -177,10 +177,6 @@ std::string Result::ReasonCodeEnumToString(ReasonCodeEnum value) {
       return "FAILED_GETEVENT";
     case sandbox2::Result::FAILED_MONITOR:
       return "FAILED_MONITOR";
-    case sandbox2::Result::FAILED_KILL:
-      return "FAILED_KILL";
-    case sandbox2::Result::FAILED_CHILD:
-      return "FAILED_CHILD";
     case sandbox2::Result::VIOLATION_SYSCALL:
       return "VIOLATION_SYSCALL";
     case sandbox2::Result::VIOLATION_ARCH:

sandboxed_api/sandbox2/result.h

@@ -77,8 +77,6 @@ class Result {
     FAILED_FETCH,
     FAILED_GETEVENT,
     FAILED_MONITOR,
-    FAILED_KILL,
-    FAILED_CHILD,
 
     // TODO(wiktorg) not used currently (syscall number stored insted) - need to
     // fix clients first
@@ -113,6 +111,13 @@ class Result {
     stack_trace_ = stack_trace;
   }
 
+  void LoadRegs(pid_t pid) {
+    auto regs = absl::make_unique<Regs>(pid);
+    if (regs->Fetch().ok()) {
+      SetRegs(std::move(regs));
+    }
+  }
+
   void SetRegs(std::unique_ptr<Regs> regs) { regs_ = std::move(regs); }
 
   void SetSyscall(std::unique_ptr<Syscall> syscall) {

sandboxed_api/sandbox2/sandbox2.cc

@@ -126,7 +126,7 @@ void Sandbox2::Launch() {
   // Wait for the Monitor to set-up the sandboxee correctly (or fail while
   // doing that). From here on, it is safe to use the IPC object for
   // non-sandbox-related data exchange.
-  monitor_->setup_counter_.Wait();
+  monitor_->setup_counter_->Wait();
 }
 
 }  // namespace sandbox2