From ec870c3d15d4f505b2c47541dd7bc2a848fcae8e Mon Sep 17 00:00:00 2001 From: Wiktor Garbacz Date: Thu, 28 Jan 2021 02:20:13 -0800 Subject: [PATCH] Simplify Executor ctor hierarchy Also accept `absl::string_view` and `absl::Span` arguments. Drive-by: - Move using declaration into namespace PiperOrigin-RevId: 354271016 Change-Id: Iadd873377e51cac7fa3800aab1f9e85ff94bd4e9 --- sandboxed_api/sandbox2/BUILD.bazel | 2 + sandboxed_api/sandbox2/CMakeLists.txt | 7 +- sandboxed_api/sandbox2/executor.cc | 25 +------- sandboxed_api/sandbox2/executor.h | 92 ++++++++++++++------------- 4 files changed, 54 insertions(+), 72 deletions(-) diff --git a/sandboxed_api/sandbox2/BUILD.bazel b/sandboxed_api/sandbox2/BUILD.bazel index fdfd7d3..1d988df 100644 --- a/sandboxed_api/sandbox2/BUILD.bazel +++ b/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", ], ) diff --git a/sandboxed_api/sandbox2/CMakeLists.txt b/sandboxed_api/sandbox2/CMakeLists.txt index 00cef53..e4cd9a1 100644 --- a/sandboxed_api/sandbox2/CMakeLists.txt +++ b/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 ) diff --git a/sandboxed_api/sandbox2/executor.cc b/sandboxed_api/sandbox2/executor.cc index 1b4bbff..0f0386e 100644 --- a/sandboxed_api/sandbox2/executor.cc +++ b/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. -Executor::Executor(int exec_fd, const std::string& path, - const std::vector& argv, - const std::vector& 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(); -} +namespace file_util = ::sapi::file_util; Executor::~Executor() { if (client_comms_fd_ != -1) { diff --git a/sandboxed_api/sandbox2/executor.h b/sandboxed_api/sandbox2/executor.h index 86dec1b..3031cb7 100644 --- a/sandboxed_api/sandbox2/executor.h +++ b/sandboxed_api/sandbox2/executor.h @@ -20,14 +20,18 @@ #include #include +#include #include #include #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& argv) - : Executor(/*exec_fd=*/-1, path, argv, CopyEnviron(), - /*enable_sandboxing_pre_execve=*/true, - /*libunwind_sbox_for_pid=*/0, - /*fork_client=*/nullptr) {} + Executor( + absl::string_view path, absl::Span argv, + absl::Span envp = absl::MakeConstSpan(CopyEnviron())) + : path_(std::string(path)), + argv_(argv.begin(), argv.end()), + envp_(envp.begin(), envp.end()) { + CHECK(!path_.empty()); + SetUpServerSideCommsFd(); + } - // As above, but takes an explicit environment - Executor(const std::string& path, const std::vector& argv, - const std::vector& envp) - : Executor(/*exec_fd=*/-1, path, argv, envp, - /*enable_sandboxing_pre_execve=*/true, - /*libunwind_sbox_for_pid=*/0, - /*fork_client=*/nullptr) {} + // As above, but with `const std::vector&` + Executor(absl::string_view path, const std::vector& argv, + const std::vector& envp = CopyEnviron()) + : Executor(path, absl::MakeSpan(argv), absl::MakeSpan(envp)) {} - // 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 argv, + absl::Span 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&` Executor(int exec_fd, const std::vector& argv, const std::vector& envp) - : Executor(exec_fd, /*path=*/"", argv, envp, - /*enable_sandboxing_pre_execve=*/true, - /*libunwind_sbox_for_pid=*/0, - /*fork_client=*/nullptr) {} + : Executor(exec_fd, absl::MakeSpan(argv), absl::MakeSpan(envp)) {} // 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, - /*libunwind_sbox_for_pid=*/0, fork_client) {} + : enable_sandboxing_pre_execve_(false), fork_client_(fork_client) { + CHECK(fork_client != nullptr); + 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=*/{}, - /*enable_sandboxing_pre_execve=*/false, - /*libunwind_sbox_for_pid=*/libunwind_sbox_for_pid, - /*fork_client=*/nullptr) {} - - // Delegate constructor that gets called by the public ones. - Executor(int exec_fd, const std::string& path, - const std::vector& argv, - const std::vector& 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_(false) { + CHECK_GE(libunwind_sbox_for_pid_, 0); + SetUpServerSideCommsFd(); + } // Creates a copy of the environment static std::vector 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 argv_; std::vector envp_; - // chdir to cwd_, if set. - std::string cwd_; + // chdir to cwd_, if set. Defaults to current working directory. + 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