Simplify Executor ctor hierarchy

Also accept `absl::string_view` and `absl::Span<const std::string>` arguments.

Drive-by:
 - Move using declaration into namespace
PiperOrigin-RevId: 354271016
Change-Id: Iadd873377e51cac7fa3800aab1f9e85ff94bd4e9

sandboxed_api/sandbox2/BUILD.bazel

@@ -263,6 +263,7 @@ cc_library(
         "@com_google_absl//absl/base:core_headers",
         "@com_google_absl//absl/memory",
         "@com_google_absl//absl/strings",
+        "@com_google_absl//absl/types:span",
     ],
 )
 
@@ -336,6 +337,7 @@ cc_library(
         "@com_google_absl//absl/synchronization",
         "@com_google_absl//absl/time",
         "@com_google_absl//absl/types:optional",
+        "@com_google_absl//absl/types:span",
         "@org_kernel_libcap//:libcap",
     ],
 )

sandboxed_api/sandbox2/CMakeLists.txt

@@ -263,8 +263,6 @@ add_library(sandbox2::executor ALIAS sandbox2_executor)
 target_link_libraries(sandbox2_executor
   PRIVATE absl::core_headers
           absl::memory
-          absl::strings
-          sapi::fileops
           sandbox2::forkserver_proto
           sandbox2::ipc
           sandbox2::limits
@@ -272,7 +270,10 @@ target_link_libraries(sandbox2_executor
           sandbox2::util
           sapi::base
           sapi::status_proto
-  PUBLIC glog::glog
+  PUBLIC absl::span
+         absl::strings
+         glog::glog
+         sapi::fileops
          sandbox2::fork_client
          sandbox2::global_forkserver
 )

sandboxed_api/sandbox2/executor.cc

@@ -33,32 +33,9 @@
 #include "sandboxed_api/sandbox2/util.h"
 #include "sandboxed_api/util/fileops.h"
 
-namespace file_util = ::sapi::file_util;
-
 namespace sandbox2 {
 
-// Delegate constructor that gets called by the public ones.
+namespace file_util = ::sapi::file_util;
-Executor::Executor(int exec_fd, const std::string& path,
-                   const std::vector<std::string>& argv,
-                   const std::vector<std::string>& envp,
-                   bool enable_sandboxing_pre_execve,
-                   pid_t libunwind_sbox_for_pid, ForkClient* fork_client)
-    : libunwind_sbox_for_pid_(libunwind_sbox_for_pid),
-      enable_sandboxing_pre_execve_(enable_sandboxing_pre_execve),
-      exec_fd_(exec_fd),
-      path_(path),
-      argv_(argv),
-      envp_(envp),
-      fork_client_(fork_client) {
-  if (fork_client_ != nullptr) {
-    CHECK(exec_fd == -1 && path.empty());
-  } else {
-    CHECK((exec_fd == -1 && (!path.empty() || libunwind_sbox_for_pid > 0)) ||
-          (exec_fd >= 0 && path.empty()));
-  }
-  SetUpServerSideCommsFd();
-  SetDefaultCwd();
-}
 
 Executor::~Executor() {
   if (client_comms_fd_ != -1) {

sandboxed_api/sandbox2/executor.h

@@ -20,14 +20,18 @@
 
 #include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include <glog/logging.h>
 #include "absl/base/macros.h"
+#include "absl/strings/string_view.h"
+#include "absl/types/span.h"
 #include "sandboxed_api/sandbox2/fork_client.h"
 #include "sandboxed_api/sandbox2/ipc.h"
 #include "sandboxed_api/sandbox2/limits.h"
 #include "sandboxed_api/sandbox2/namespace.h"
+#include "sandboxed_api/util/fileops.h"
 
 namespace sandbox2 {
 
@@ -40,37 +44,45 @@ class Executor final {
 
   // Initialized with a path to the process that the Executor class will
   // execute
-  Executor(const std::string& path, const std::vector<std::string>& argv)
+  Executor(
-      : Executor(/*exec_fd=*/-1, path, argv, CopyEnviron(),
+      absl::string_view path, absl::Span<const std::string> argv,
-                 /*enable_sandboxing_pre_execve=*/true,
+      absl::Span<const std::string> envp = absl::MakeConstSpan(CopyEnviron()))
-                 /*libunwind_sbox_for_pid=*/0,
+      : path_(std::string(path)),
-                 /*fork_client=*/nullptr) {}
+        argv_(argv.begin(), argv.end()),
+        envp_(envp.begin(), envp.end()) {
+    CHECK(!path_.empty());
+    SetUpServerSideCommsFd();
+  }
 
-  // As above, but takes an explicit environment
+  // As above, but with `const std::vector<std::string>&`
-  Executor(const std::string& path, const std::vector<std::string>& argv,
+  Executor(absl::string_view path, const std::vector<std::string>& argv,
-           const std::vector<std::string>& envp)
+           const std::vector<std::string>& envp = CopyEnviron())
-      : Executor(/*exec_fd=*/-1, path, argv, envp,
+      : Executor(path, absl::MakeSpan(argv), absl::MakeSpan(envp)) {}
-                 /*enable_sandboxing_pre_execve=*/true,
-                 /*libunwind_sbox_for_pid=*/0,
-                 /*fork_client=*/nullptr) {}
 
-  // As above, but takes a file-descriptor referring to an executable file.
   // Executor will own this file-descriptor, so if intend to use it, pass here
   // dup(fd) instead
+  Executor(int exec_fd, absl::Span<const std::string> argv,
+           absl::Span<const std::string> envp)
+      : exec_fd_(exec_fd),
+        argv_(argv.begin(), argv.end()),
+        envp_(envp.begin(), envp.end()) {
+    CHECK_GE(exec_fd, 0);
+    SetUpServerSideCommsFd();
+  }
+
+  // As above, but with `const std::vector<std::string>&`
   Executor(int exec_fd, const std::vector<std::string>& argv,
            const std::vector<std::string>& envp)
-      : Executor(exec_fd, /*path=*/"", argv, envp,
+      : Executor(exec_fd, absl::MakeSpan(argv), absl::MakeSpan(envp)) {}
-                 /*enable_sandboxing_pre_execve=*/true,
-                 /*libunwind_sbox_for_pid=*/0,
-                 /*fork_client=*/nullptr) {}
 
   // Uses a custom ForkServer (which the supplied ForkClient can communicate
   // with), which knows how to fork (or even execute) new sandboxed processes
   // (hence, no need to supply path/argv/envp here)
   explicit Executor(ForkClient* fork_client)
-      : Executor(/*exec_fd=*/-1, /*path=*/"", /*argv=*/{}, /*envp=*/{},
+      : enable_sandboxing_pre_execve_(false), fork_client_(fork_client) {
-                 /*enable_sandboxing_pre_execve=*/false,
+    CHECK(fork_client != nullptr);
-                 /*libunwind_sbox_for_pid=*/0, fork_client) {}
+    SetUpServerSideCommsFd();
+  }
 
   ~Executor();
 
@@ -101,17 +113,11 @@ class Executor final {
   // Internal constructor for executing libunwind on the given pid
   // enable_sandboxing_pre_execve=false as we are not going to execve.
   explicit Executor(pid_t libunwind_sbox_for_pid)
-      : Executor(/*exec_fd=*/-1, /*path=*/"", /*argv=*/{}, /*envp=*/{},
+      : libunwind_sbox_for_pid_(libunwind_sbox_for_pid),
-                 /*enable_sandboxing_pre_execve=*/false,
+        enable_sandboxing_pre_execve_(false) {
-                 /*libunwind_sbox_for_pid=*/libunwind_sbox_for_pid,
+    CHECK_GE(libunwind_sbox_for_pid_, 0);
-                 /*fork_client=*/nullptr) {}
+    SetUpServerSideCommsFd();
-
+  }
-  // Delegate constructor that gets called by the public ones.
-  Executor(int exec_fd, const std::string& path,
-           const std::vector<std::string>& argv,
-           const std::vector<std::string>& envp,
-           bool enable_sandboxing_pre_execve, pid_t libunwind_sbox_for_pid,
-           ForkClient* fork_client);
 
   // Creates a copy of the environment
   static std::vector<std::string> CopyEnviron();
@@ -120,14 +126,6 @@ class Executor final {
   // descriptor.
   void SetUpServerSideCommsFd();
 
-  // Sets the default value for cwd_
-  void SetDefaultCwd() {
-    char* cwd = get_current_dir_name();
-    PCHECK(cwd != nullptr);
-    cwd_ = cwd;
-    free(cwd);
-  }
-
   // Starts a new process which is connected with this Executor instance via a
   // Comms channel.
   // For clone_flags refer to Linux' 'man 2 clone'.
@@ -145,20 +143,24 @@ class Executor final {
 
   // If this executor is running the libunwind sandbox for a process,
   // this variable will hold the PID of the process. Otherwise it is zero.
-  pid_t libunwind_sbox_for_pid_;
+  pid_t libunwind_sbox_for_pid_ = 0;
 
   // Should the sandboxing be enabled before execve() occurs, or the binary will
   // do it by itself, using the Client object's methods
-  bool enable_sandboxing_pre_execve_;
+  bool enable_sandboxing_pre_execve_ = true;
 
   // Alternate (path/fd)/argv/envp to be used the in the __NR_execve call.
-  int exec_fd_;
+  int exec_fd_ = -1;
   std::string path_;
   std::vector<std::string> argv_;
   std::vector<std::string> envp_;
 
-  // chdir to cwd_, if set.
+  // chdir to cwd_, if set. Defaults to current working directory.
-  std::string cwd_;
+  std::string cwd_ = []() {
+    std::string cwd = sapi::file_util::fileops::GetCWD();
+    PCHECK(!cwd.empty());
+    return cwd;
+  }();
 
   // Server (sandbox) end-point of a socket-pair used to create Comms channel
   int server_comms_fd_ = -1;
@@ -166,7 +168,7 @@ class Executor final {
   int client_comms_fd_ = -1;
 
   // ForkClient connecting to the ForkServer - not owned by the object
-  ForkClient* fork_client_;
+  ForkClient* fork_client_ = nullptr;
 
   IPC ipc_;        // Used for communication with the sandboxee
   Limits limits_;  // Defines server- and client-side limits