Extract SandboxeeProcess and move it down the call chain

PiperOrigin-RevId: 507718207
Change-Id: Ia1f6fc2f09abbde5311f8dc0f596aa605989140d

sandboxed_api/sandbox2/BUILD.bazel

@@ -390,6 +390,7 @@ cc_library(
         ":client",
         ":comms",
         ":executor",
+        ":fork_client",
         ":ipc",
         ":limits",
         ":mounts",

sandboxed_api/sandbox2/CMakeLists.txt

@@ -349,28 +349,29 @@ add_library(sandbox2_monitor_base ${SAPI_LIB_TYPE}
 )
 add_library(sandbox2::monitor_base ALIAS sandbox2_monitor_base)
 target_link_libraries(sandbox2_monitor_base
-  PRIVATE absl::status
+  PRIVATE absl::cleanup
+          absl::status
+          absl::time
+          sandbox2::client
+          sandbox2::limits
+          sandbox2::mounts
+          sandbox2::namespace
+          sandbox2::util
+          sapi::file_helpers
+          sapi::temp_file
+          sapi::base
+          sapi::raw_logging
+  PUBLIC  absl::statusor
           absl::synchronization
           sandbox2::comms
           sandbox2::executor
+          sandbox2::fork_client
           sandbox2::ipc
           sandbox2::network_proxy_server
           sandbox2::notify
           sandbox2::policy
           sandbox2::result
           sandbox2::syscall
-          sapi::base
-          sapi::raw_logging
-  PUBLIC  absl::cleanup
-          absl::statusor
-          absl::time
-          sapi::file_helpers
-          sapi::temp_file
-          sandbox2::client
-          sandbox2::limits
-          sandbox2::mounts
-          sandbox2::namespace
-          sandbox2::util
 )
 
 # sandboxed_api/sandbox2:policybuilder

sandboxed_api/sandbox2/executor.cc

@@ -82,7 +82,7 @@ std::vector<std::string> Executor::CopyEnviron() {
   return util::CharPtrArray(environ).ToStringVector();
 }
 
-absl::StatusOr<Executor::Process> Executor::StartSubProcess(
+absl::StatusOr<SandboxeeProcess> Executor::StartSubProcess(
     int32_t clone_flags, const Namespace* ns) {
   if (started_) {
     return absl::FailedPreconditionError(
@@ -150,14 +150,14 @@ absl::StatusOr<Executor::Process> Executor::StartSubProcess(
 
   request.set_clone_flags(clone_flags);
 
-  Process process;
+  SandboxeeProcess process;
 
   if (fork_client_) {
-    process.main_pid = fork_client_->SendRequest(
+    process = fork_client_->SendRequest(request, exec_fd_.get(),
-        request, exec_fd_.get(), client_comms_fd_.get(), &process.init_pid);
+                                        client_comms_fd_.get());
   } else {
-    process.main_pid = GlobalForkClient::SendRequest(
+    process = GlobalForkClient::SendRequest(request, exec_fd_.get(),
-        request, exec_fd_.get(), client_comms_fd_.get(), &process.init_pid);
+                                            client_comms_fd_.get());
   }
 
   started_ = true;
@@ -173,7 +173,7 @@ 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);
-  absl::StatusOr<Process> process = StartSubProcess(0);
+  absl::StatusOr<SandboxeeProcess> process = StartSubProcess(0);
   if (!process.ok()) {
     return nullptr;
   }

sandboxed_api/sandbox2/executor.h

@@ -40,11 +40,6 @@ namespace sandbox2 {
 // new processes which will be sandboxed.
 class Executor final {
  public:
-  struct Process {
-    pid_t init_pid = -1;
-    pid_t main_pid = -1;
-  };
-
   Executor(const Executor&) = delete;
   Executor& operator=(const Executor&) = delete;
 
@@ -124,8 +119,8 @@ class Executor final {
   // Starts a new process which is connected with this Executor instance via a
   // Comms channel.
   // For clone_flags refer to Linux' 'man 2 clone'.
-  absl::StatusOr<Process> StartSubProcess(int clone_flags,
+  absl::StatusOr<SandboxeeProcess> StartSubProcess(
-                                          const Namespace* ns = nullptr);
+      int clone_flags, const Namespace* ns = nullptr);
 
   // Whether the Executor has been started yet
   bool started_ = false;

sandboxed_api/sandbox2/fork_client.cc

@@ -21,20 +21,24 @@
 
 namespace sandbox2 {
 
-pid_t ForkClient::SendRequest(const ForkRequest& request, int exec_fd,
+SandboxeeProcess ForkClient::SendRequest(const ForkRequest& request,
-                              int comms_fd, pid_t* init_pid) {
+                                         int exec_fd, int comms_fd) {
+  SandboxeeProcess process = {
+      .init_pid = -1,
+      .main_pid = -1,
+  };
   // Acquire the channel ownership for this request (transaction).
   absl::MutexLock l(&comms_mutex_);
 
   if (!comms_->SendProtoBuf(request)) {
     LOG(ERROR) << "Sending PB to the ForkServer failed";
-    return -1;
+    return process;
   }
   CHECK(comms_fd != -1) << "comms_fd was not properly set up";
   if (!comms_->SendFD(comms_fd)) {
     LOG(ERROR) << "Sending Comms FD (" << comms_fd
                << ") to the ForkServer failed";
-    return -1;
+    return process;
   }
   if (request.mode() == FORKSERVER_FORK_EXECVE ||
       request.mode() == FORKSERVER_FORK_EXECVE_SANDBOX) {
@@ -42,7 +46,7 @@ pid_t ForkClient::SendRequest(const ForkRequest& request, int exec_fd,
     if (!comms_->SendFD(exec_fd)) {
       LOG(ERROR) << "Sending Exec FD (" << exec_fd
                  << ") to the ForkServer failed";
-      return -1;
+      return process;
     }
   }
 
@@ -50,18 +54,17 @@ pid_t ForkClient::SendRequest(const ForkRequest& request, int exec_fd,
   // Receive init process ID.
   if (!comms_->RecvInt32(&pid)) {
     LOG(ERROR) << "Receiving init PID from the ForkServer failed";
-    return -1;
+    return process;
-  }
-  if (init_pid) {
-    *init_pid = static_cast<pid_t>(pid);
   }
+  process.init_pid = static_cast<pid_t>(pid);
 
   // Receive sandboxee process ID.
   if (!comms_->RecvInt32(&pid)) {
     LOG(ERROR) << "Receiving sandboxee PID from the ForkServer failed";
-    return -1;
+    return process;
   }
-  return static_cast<pid_t>(pid);
+  process.main_pid = static_cast<pid_t>(pid);
+  return process;
 }
 
 }  // namespace sandbox2

sandboxed_api/sandbox2/fork_client.h

@@ -28,6 +28,11 @@ constexpr inline char kForkServerDisableEnv[] = "SANDBOX2_NOFORKSERVER";
 class Comms;
 class ForkRequest;
 
+struct SandboxeeProcess {
+  pid_t init_pid = -1;
+  pid_t main_pid = -1;
+};
+
 class ForkClient {
  public:
   ForkClient(pid_t pid, Comms* comms) : pid_(pid), comms_(comms) {}
@@ -35,8 +40,8 @@ class ForkClient {
   ForkClient& operator=(const ForkClient&) = delete;
 
   // Sends the fork request over the supplied Comms channel.
-  pid_t SendRequest(const ForkRequest& request, int exec_fd, int comms_fd,
+  SandboxeeProcess SendRequest(const ForkRequest& request, int exec_fd,
-                    pid_t* init_pid = nullptr);
+                               int comms_fd);
 
   pid_t pid() { return pid_; }
 

sandboxed_api/sandbox2/forkserver_test.cc

@@ -62,14 +62,15 @@ pid_t TestSingleRequest(Mode mode, int exec_fd) {
   fork_req.add_args("/binary");
   fork_req.add_envs("FOO=1");
 
-  pid_t pid = GlobalForkClient::SendRequest(fork_req, exec_fd, sv[0]);
+  SandboxeeProcess process =
-  if (pid != -1) {
+      GlobalForkClient::SendRequest(fork_req, exec_fd, sv[0]);
-    VLOG(1) << "TestSingleRequest: Waiting for pid=" << pid;
+  if (process.main_pid != -1) {
-    waitpid(pid, nullptr, 0);
+    VLOG(1) << "TestSingleRequest: Waiting for pid=" << process.main_pid;
+    waitpid(process.main_pid, nullptr, 0);
   }
 
   close(sv[0]);
-  return pid;
+  return process.main_pid;
 }
 
 TEST(ForkserverTest, SimpleFork) {

sandboxed_api/sandbox2/global_forkclient.cc

@@ -289,15 +289,18 @@ void GlobalForkClient::Shutdown() {
   }
 }
 
-pid_t GlobalForkClient::SendRequest(const ForkRequest& request, int exec_fd,
+SandboxeeProcess GlobalForkClient::SendRequest(const ForkRequest& request,
-                                    int comms_fd, pid_t* init_pid) {
+                                               int exec_fd, int comms_fd) {
   absl::ReleasableMutexLock lock(&GlobalForkClient::instance_mutex_);
   EnsureStartedLocked(GlobalForkserverStartMode::kOnDemand);
   if (!instance_) {
-    return -1;
+    return {
+        .init_pid = -1,
+        .main_pid = -1,
+    };
   }
-  pid_t pid =
+  SandboxeeProcess process =
-      instance_->fork_client_.SendRequest(request, exec_fd, comms_fd, init_pid);
+      instance_->fork_client_.SendRequest(request, exec_fd, comms_fd);
   if (instance_->comms_.IsTerminated()) {
     LOG(ERROR) << "Global forkserver connection terminated";
     pid_t server_pid = instance_->fork_client_.pid();
@@ -308,7 +311,7 @@ pid_t GlobalForkClient::SendRequest(const ForkRequest& request, int exec_fd,
     lock.Release();
     WaitForForkserver(server_pid);
   }
-  return pid;
+  return process;
 }
 
 pid_t GlobalForkClient::GetPid() {

sandboxed_api/sandbox2/global_forkclient.h

@@ -44,8 +44,8 @@ class GlobalForkClient {
   GlobalForkClient(int fd, pid_t pid)
       : comms_(fd), fork_client_(pid, &comms_) {}
 
-  static pid_t SendRequest(const ForkRequest& request, int exec_fd,
+  static SandboxeeProcess SendRequest(const ForkRequest& request, int exec_fd,
-                           int comms_fd, pid_t* init_pid = nullptr)
+                                      int comms_fd)
       ABSL_LOCKS_EXCLUDED(instance_mutex_);
   static pid_t GetPid() ABSL_LOCKS_EXCLUDED(instance_mutex_);
 

sandboxed_api/sandbox2/monitor_base.cc

@@ -191,7 +191,7 @@ void MonitorBase::Launch() {
 
   // Get PID of the sandboxee.
   bool should_have_init = ns && (ns->GetCloneFlags() & CLONE_NEWPID);
-  absl::StatusOr<Executor::Process> process =
+  absl::StatusOr<SandboxeeProcess> process =
       executor_->StartSubProcess(clone_flags, ns);
 
   if (!process.ok()) {

sandboxed_api/sandbox2/monitor_base.h

@@ -29,6 +29,7 @@
 #include "absl/synchronization/notification.h"
 #include "sandboxed_api/sandbox2/comms.h"
 #include "sandboxed_api/sandbox2/executor.h"
+#include "sandboxed_api/sandbox2/fork_client.h"
 #include "sandboxed_api/sandbox2/ipc.h"
 #include "sandboxed_api/sandbox2/network_proxy/server.h"
 #include "sandboxed_api/sandbox2/notify.h"
@@ -82,7 +83,7 @@ class MonitorBase {
   Notify* notify_;
   Policy* policy_;
   // The sandboxee process.
-  Executor::Process process_;
+  SandboxeeProcess process_;
   Result result_;
   // Comms channel ptr, copied from the Executor object for convenience.
   Comms* comms_;