Avoid hard failures in StartSubProcess

PiperOrigin-RevId: 433453289
Change-Id: Ib8b08ddd31c4daa9a377960d52f0a7eb7b17de19

sandboxed_api/sandbox2/BUILD.bazel

@@ -288,6 +288,7 @@ cc_library(
         ":util",
         "//sandboxed_api:config",
         "//sandboxed_api/util:fileops",
+        "//sandboxed_api/util:status",
         "@com_google_absl//absl/base:core_headers",
         "@com_google_absl//absl/memory",
         "@com_google_absl//absl/strings",

sandboxed_api/sandbox2/CMakeLists.txt

@@ -286,6 +286,7 @@ target_link_libraries(sandbox2_executor
          glog::glog
          sapi::config
          sapi::fileops
+         sapi::status
          sandbox2::fork_client
          sandbox2::global_forkserver
 )

sandboxed_api/sandbox2/executor.cc

@@ -36,6 +36,7 @@
 #include "sandboxed_api/sandbox2/ipc.h"
 #include "sandboxed_api/sandbox2/util.h"
 #include "sandboxed_api/util/fileops.h"
+#include "sandboxed_api/util/os_error.h"
 
 namespace sandbox2 {
 
@@ -80,19 +81,21 @@ std::vector<std::string> Executor::CopyEnviron() {
   return util::CharPtrArray(environ).ToStringVector();
 }
 
-pid_t Executor::StartSubProcess(int32_t clone_flags, const Namespace* ns,
+absl::StatusOr<Executor::Process> Executor::StartSubProcess(
-                                const std::vector<int>& caps,
+    int32_t clone_flags, const Namespace* ns, const std::vector<int>& caps) {
-                                pid_t* init_pid_out) {
   if (started_) {
-    LOG(ERROR) << "This executor has already been started";
+    return absl::FailedPreconditionError(
-    return -1;
+        "This executor has already been started");
   }
 
   if (!path_.empty()) {
     exec_fd_ = file_util::fileops::FDCloser(open(path_.c_str(), O_PATH));
     if (exec_fd_.get() < 0) {
-      PLOG(ERROR) << "Could not open file " << path_;
+      if (errno == ENOENT) {
-      return -1;
+        return absl::NotFoundError(sapi::OsErrorMessage(errno, path_));
+      }
+      return absl::InternalError(
+          sapi::OsErrorMessage(errno, "Could not open file ", path_));
     }
   }
 
@@ -155,33 +158,22 @@ pid_t Executor::StartSubProcess(int32_t clone_flags, const Namespace* ns,
     const std::string ns_path =
         absl::StrCat("/proc/", libunwind_sbox_for_pid_, "/ns/user");
     ns_fd = file_util::fileops::FDCloser(open(ns_path.c_str(), O_RDONLY));
-    PCHECK(ns_fd.get() != -1)
+    if (ns_fd.get() == -1) {
-        << "Could not open user ns fd (" << ns_path << ")";
+      return absl::InternalError(sapi::OsErrorMessage(
+          errno, "Could not open user ns fd (", ns_path, ")"));
+    }
   }
 
-  pid_t init_pid = -1;
+  Process process;
 
-  pid_t sandboxee_pid;
   if (fork_client_) {
-    sandboxee_pid = fork_client_->SendRequest(request, exec_fd_.get(),
+    process.main_pid = fork_client_->SendRequest(
-                                              client_comms_fd_.get(),
+        request, exec_fd_.get(), client_comms_fd_.get(), ns_fd.get(),
-                                              ns_fd.get(), &init_pid);
+        &process.init_pid);
   } else {
-    sandboxee_pid = GlobalForkClient::SendRequest(request, exec_fd_.get(),
+    process.main_pid = GlobalForkClient::SendRequest(
-                                                  client_comms_fd_.get(),
+        request, exec_fd_.get(), client_comms_fd_.get(), ns_fd.get(),
-                                                  ns_fd.get(), &init_pid);
+        &process.init_pid);
-  }
-
-  if (init_pid < 0) {
-    LOG(ERROR) << "Could not obtain init PID";
-  } else if (init_pid == 0 && request.clone_flags() & CLONE_NEWPID) {
-    LOG(FATAL)
-        << "No init process was spawned even though a PID NS was created, "
-        << "potential logic bug";
-  }
-
-  if (init_pid_out) {
-    *init_pid_out = init_pid;
   }
 
   started_ = true;
@@ -189,19 +181,19 @@ pid_t Executor::StartSubProcess(int32_t clone_flags, const Namespace* ns,
   client_comms_fd_.Close();
   exec_fd_.Close();
 
-  VLOG(1) << "StartSubProcess returned with: " << sandboxee_pid;
+  VLOG(1) << "StartSubProcess returned with: " << process.main_pid;
-  return sandboxee_pid;
+  return process;
 }
 
 std::unique_ptr<ForkClient> Executor::StartForkServer() {
   // This flag is set explicitly to 'true' during object instantiation, and
   // custom fork-servers should never be sandboxed.
   set_enable_sandbox_before_exec(false);
-  pid_t pid = StartSubProcess(0);
+  absl::StatusOr<Process> process = StartSubProcess(0);
-  if (pid == -1) {
+  if (!process.ok()) {
     return nullptr;
   }
-  return absl::make_unique<ForkClient>(pid, ipc_.comms());
+  return absl::make_unique<ForkClient>(process->main_pid, ipc_.comms());
 }
 
 void Executor::SetUpServerSideCommsFd() {

sandboxed_api/sandbox2/executor.h

@@ -39,6 +39,11 @@ namespace sandbox2 {
 // new processes which will be sandboxed.
 class Executor final {
  public:
+  struct Process {
+    pid_t init_pid;
+    pid_t main_pid;
+  };
+
   Executor(const Executor&) = delete;
   Executor& operator=(const Executor&) = delete;
 
@@ -120,11 +125,9 @@ class Executor final {
   //
   // caps is a vector of capabilities that are kept in the permitted set after
   // the clone, use with caution.
-  //
+  absl::StatusOr<Process> StartSubProcess(int clone_flags,
-  // Returns the same values as fork().
+                                          const Namespace* ns = nullptr,
-  pid_t StartSubProcess(int clone_flags, const Namespace* ns = nullptr,
+                                          const std::vector<int>& caps = {});
-                        const std::vector<int>& caps = {},
-                        pid_t* init_pid_out = nullptr);
 
   // Whether the Executor has been started yet
   bool started_ = false;

sandboxed_api/sandbox2/monitor.cc

@@ -276,13 +276,20 @@ void Monitor::Run() {
   }
 
   // Get PID of the sandboxee.
-  pid_t init_pid = 0;
   bool should_have_init = ns && (ns->GetCloneFlags() & CLONE_NEWPID);
-  pid_ = executor_->StartSubProcess(clone_flags, ns, policy_->capabilities(),
+  absl::StatusOr<Executor::Process> process =
-                                    &init_pid);
+      executor_->StartSubProcess(clone_flags, ns, policy_->capabilities());
 
-  if (init_pid > 0) {
+  if (!process.ok()) {
-    if (ptrace(PTRACE_SEIZE, init_pid, 0, PTRACE_O_EXITKILL) != 0) {
+    LOG(ERROR) << "Starting sandboxed subprocess failed: " << process.status();
+    SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_SUBPROCESS);
+    return;
+  }
+
+  pid_ = process->main_pid;
+
+  if (process->init_pid > 0) {
+    if (ptrace(PTRACE_SEIZE, process->init_pid, 0, PTRACE_O_EXITKILL) != 0) {
       if (errno == ESRCH) {
         SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_PTRACE);
         return;
@@ -291,7 +298,7 @@ void Monitor::Run() {
     }
   }
 
-  if (pid_ <= 0 || (should_have_init && init_pid <= 0)) {
+  if (pid_ <= 0 || (should_have_init && process->init_pid <= 0)) {
     SetExitStatusCode(Result::SETUP_ERROR, Result::FAILED_SUBPROCESS);
     return;
   }