From 3f5184770d7291dedf46d61da3f989e3cc8414f2 Mon Sep 17 00:00:00 2001 From: Wiktor Garbacz Date: Mon, 20 Dec 2021 05:07:40 -0800 Subject: [PATCH] Introduce util::CharPtrArray with proper ownership semantics Replace existing calls to VecStringToCharPtrArr PiperOrigin-RevId: 417383812 Change-Id: Ibf9d878df5ada2cb3a0872f7ca7cab96c304a5c1 --- sandboxed_api/sandbox2/BUILD.bazel | 1 + sandboxed_api/sandbox2/CMakeLists.txt | 3 +- sandboxed_api/sandbox2/executor.cc | 4 +- sandboxed_api/sandbox2/forkserver.cc | 21 ++++---- sandboxed_api/sandbox2/forkserver.h | 4 +- sandboxed_api/sandbox2/sanitizer_test.cc | 5 +- sandboxed_api/sandbox2/util.cc | 68 +++++++++++++++++++----- sandboxed_api/sandbox2/util.h | 23 ++++++++ 8 files changed, 94 insertions(+), 35 deletions(-) diff --git a/sandboxed_api/sandbox2/BUILD.bazel b/sandboxed_api/sandbox2/BUILD.bazel index 959244b..63063ae 100644 --- a/sandboxed_api/sandbox2/BUILD.bazel +++ b/sandboxed_api/sandbox2/BUILD.bazel @@ -560,6 +560,7 @@ cc_library( "//sandboxed_api/util:fileops", "//sandboxed_api/util:raw_logging", "//sandboxed_api/util:status", + "@com_google_absl//absl/algorithm:container", "@com_google_absl//absl/base:core_headers", "@com_google_absl//absl/status", "@com_google_absl//absl/status:statusor", diff --git a/sandboxed_api/sandbox2/CMakeLists.txt b/sandboxed_api/sandbox2/CMakeLists.txt index 5b9213c..639a7b2 100644 --- a/sandboxed_api/sandbox2/CMakeLists.txt +++ b/sandboxed_api/sandbox2/CMakeLists.txt @@ -499,7 +499,8 @@ add_library(sandbox2_util ${SAPI_LIB_TYPE} ) add_library(sandbox2::util ALIAS sandbox2_util) target_link_libraries(sandbox2_util - PRIVATE absl::core_headers + PRIVATE absl::algorithm_container + absl::core_headers absl::str_format absl::strings sapi::config diff --git a/sandboxed_api/sandbox2/executor.cc b/sandboxed_api/sandbox2/executor.cc index 68f8594..4009989 100644 --- a/sandboxed_api/sandbox2/executor.cc +++ b/sandboxed_api/sandbox2/executor.cc @@ -38,9 +38,7 @@ namespace sandbox2 { namespace file_util = ::sapi::file_util; std::vector Executor::CopyEnviron() { - std::vector environ_copy; - util::CharPtrArrToVecString(environ, &environ_copy); - return environ_copy; + return util::CharPtrArray(environ).ToStringVector(); } pid_t Executor::StartSubProcess(int32_t clone_flags, const Namespace* ns, diff --git a/sandboxed_api/sandbox2/forkserver.cc b/sandboxed_api/sandbox2/forkserver.cc index a224f20..0eac04f 100644 --- a/sandboxed_api/sandbox2/forkserver.cc +++ b/sandboxed_api/sandbox2/forkserver.cc @@ -255,8 +255,6 @@ void ForkServer::LaunchChild(const ForkRequest& request, int execve_fd, // sandoxing can cause syscall violations (e.g. related to memory management). std::vector args; std::vector envs; - const char** argv = nullptr; - const char** envp = nullptr; if (will_execve) { PrepareExecveArgs(request, &args, &envs); } @@ -328,25 +326,24 @@ void ForkServer::LaunchChild(const ForkRequest& request, int execve_fd, c.PrepareEnvironment(); envs.push_back(c.GetFdMapEnvVar()); - // Convert argv and envs to const char **. No need to free it, as the - // code will either execve() or exit(). - argv = util::VecStringToCharPtrArr(args); - envp = util::VecStringToCharPtrArr(envs); + // Convert args and envs before enabling sandbox (it'll allocate which might + // be blocked). + util::CharPtrArray argv = util::CharPtrArray::FromStringVector(args); + util::CharPtrArray envp = util::CharPtrArray::FromStringVector(envs); c.EnableSandbox(); if (request.mode() == FORKSERVER_FORK_JOIN_SANDBOX_UNWIND) { exit(RunLibUnwindAndSymbolizer(&client_comms) ? EXIT_SUCCESS : EXIT_FAILURE); } else { - ExecuteProcess(execve_fd, argv, envp); + ExecuteProcess(execve_fd, argv.data(), envp.data()); } abort(); } if (will_execve) { - argv = util::VecStringToCharPtrArr(args); - envp = util::VecStringToCharPtrArr(envs); - ExecuteProcess(execve_fd, argv, envp); + ExecuteProcess(execve_fd, util::CharPtrArray::FromStringVector(args).data(), + util::CharPtrArray::FromStringVector(envs).data()); abort(); } } @@ -565,8 +562,8 @@ void ForkServer::SanitizeEnvironment() { absl::StrCat("while sanitizing process: ", status.message()).c_str()); } -void ForkServer::ExecuteProcess(int execve_fd, const char** argv, - const char** envp) { +void ForkServer::ExecuteProcess(int execve_fd, const char* const* argv, + const char* const* envp) { // Do not add any code before execve(), as it's subject to seccomp policies. // Indicate that it's a special execve(), by setting 4th, 5th and 6th syscall // argument to magic values. diff --git a/sandboxed_api/sandbox2/forkserver.h b/sandboxed_api/sandbox2/forkserver.h index 5410461..dfedcf6 100644 --- a/sandboxed_api/sandbox2/forkserver.h +++ b/sandboxed_api/sandbox2/forkserver.h @@ -72,8 +72,8 @@ class ForkServer { static void SanitizeEnvironment(); // Executes the sandboxee, or exit with Executor::kFailedExecve. - static void ExecuteProcess(int execve_fd, const char** argv, - const char** envp); + static void ExecuteProcess(int execve_fd, const char* const* argv, + const char* const* envp); // Runs namespace initializers for a sandboxee. static void InitializeNamespaces(const ForkRequest& request, uid_t uid, diff --git a/sandboxed_api/sandbox2/sanitizer_test.cc b/sandboxed_api/sandbox2/sanitizer_test.cc index e83b548..073f26c 100644 --- a/sandboxed_api/sandbox2/sanitizer_test.cc +++ b/sandboxed_api/sandbox2/sanitizer_test.cc @@ -51,18 +51,17 @@ namespace { // Runs a new process and returns 0 if the process terminated with 0. int RunTestcase(const std::string& path, const std::vector& args) { + util::CharPtrArray array = util::CharPtrArray::FromStringVector(args); pid_t pid = fork(); if (pid < 0) { PLOG(ERROR) << "fork()"; return 1; } - const char** argv = util::VecStringToCharPtrArr(args); if (pid == 0) { - execv(path.c_str(), const_cast(argv)); + execv(path.c_str(), const_cast(array.data())); PLOG(ERROR) << "execv('" << path << "')"; exit(EXIT_FAILURE); } - delete[] argv; for (;;) { int status; diff --git a/sandboxed_api/sandbox2/util.cc b/sandboxed_api/sandbox2/util.cc index 5ab866e..867990c 100644 --- a/sandboxed_api/sandbox2/util.cc +++ b/sandboxed_api/sandbox2/util.cc @@ -33,6 +33,7 @@ #include #include +#include "absl/algorithm/container.h" #include "absl/base/attributes.h" #include "absl/status/status.h" #include "absl/status/statusor.h" @@ -41,6 +42,7 @@ #include "absl/strings/numbers.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_format.h" +#include "absl/strings/str_join.h" #include "absl/strings/str_replace.h" #include "absl/strings/str_split.h" #include "absl/strings/string_view.h" @@ -57,10 +59,21 @@ namespace sandbox2::util { namespace file = ::sapi::file; namespace file_util = ::sapi::file_util; -void CharPtrArrToVecString(char* const* arr, std::vector* vec) { - for (int i = 0; arr[i]; ++i) { - vec->push_back(arr[i]); +namespace { + +std::string ConcatenateAll(char* const* arr) { + std::string result; + for (; *arr != nullptr; ++arr) { + size_t len = strlen(*arr); + result.append(*arr, len + 1); } + return result; +} + +} // namespace + +void CharPtrArrToVecString(char* const* arr, std::vector* vec) { + *vec = CharPtrArray(arr).ToStringVector(); } const char** VecStringToCharPtrArr(const std::vector& vec) { @@ -73,6 +86,39 @@ const char** VecStringToCharPtrArr(const std::vector& vec) { return arr; } +CharPtrArray::CharPtrArray(char* const* arr) : content_(ConcatenateAll(arr)) { + for (auto it = content_.begin(); it != content_.end(); + it += strlen(&*it) + 1) { + array_.push_back(&*it); + } + array_.push_back(nullptr); +} + +CharPtrArray::CharPtrArray(const std::vector& vec) + : content_(absl::StrJoin(vec, absl::string_view("\0", 1))) { + size_t len = 0; + array_.reserve(vec.size() + 1); + for (const std::string& str : vec) { + array_.push_back(&content_[len]); + len += str.size() + 1; + } + array_.push_back(nullptr); +} + +CharPtrArray CharPtrArray::FromStringVector( + const std::vector& vec) { + return CharPtrArray(vec); +} + +std::vector CharPtrArray::ToStringVector() const { + std::vector result; + result.reserve(array_.size() - 1); + for (size_t i = 0; i < array_.size() - 1; ++i) { + result.push_back(array_[i]); + } + return result; +} + std::string GetProgName(pid_t pid) { std::string fname = file::JoinPath("/proc", absl::StrCat(pid), "exe"); // Use ReadLink instead of RealPath, as for fd-based executables (e.g. created @@ -248,19 +294,13 @@ absl::StatusOr Communicate(const std::vector& argv, posix_spawn_file_actions_adddup2(&action, cout_pipe[1], 2); posix_spawn_file_actions_addclose(&action, cout_pipe[1]); - char** args = const_cast(util::VecStringToCharPtrArr(argv)); - char** envp = const_cast(util::VecStringToCharPtrArr(envv)); - struct ArgumentCleanup { - ~ArgumentCleanup() { - delete[] args_; - delete[] envp_; - } - char** args_; - char** envp_; - } args_cleanup{args, envp}; + CharPtrArray args = CharPtrArray::FromStringVector(argv); + CharPtrArray envp = CharPtrArray::FromStringVector(envv); pid_t pid; - if (posix_spawnp(&pid, args[0], &action, nullptr, args, envp) != 0) { + if (posix_spawnp(&pid, args.array()[0], &action, nullptr, + const_cast(args.data()), + const_cast(envp.data())) != 0) { return absl::UnknownError(sapi::OsErrorMessage(errno, "posix_spawnp()")); } diff --git a/sandboxed_api/sandbox2/util.h b/sandboxed_api/sandbox2/util.h index b09771c..4f9adb6 100644 --- a/sandboxed_api/sandbox2/util.h +++ b/sandboxed_api/sandbox2/util.h @@ -24,6 +24,7 @@ #include #include +#include "absl/base/attributes.h" #include "absl/base/macros.h" #include "absl/status/statusor.h" @@ -31,13 +32,35 @@ namespace sandbox2::util { // Converts an array of char* (terminated by a nullptr, like argv, or environ // arrays), to an std::vector. +ABSL_DEPRECATED("Use CharPtrArray(arr).ToStringVector() instead") void CharPtrArrToVecString(char* const* arr, std::vector* vec); // Converts a vector of strings to a newly allocated array. The array is limited // by the terminating nullptr entry (like environ or argv). It must be freed by // the caller. +ABSL_DEPRECATED("Use CharPtrArray class instead") const char** VecStringToCharPtrArr(const std::vector& vec); +// An char ptr array limited by the terminating nullptr entry (like environ +// or argv). +class CharPtrArray { + public: + CharPtrArray(char* const* array); + static CharPtrArray FromStringVector(const std::vector& vec); + + const std::vector& array() const { return array_; } + + const char* const* data() const { return array_.data(); } + + std::vector ToStringVector() const; + + private: + CharPtrArray(const std::vector& vec); + + const std::string content_; + std::vector array_; +}; + // Returns the program name (via /proc/self/comm) for a given PID. std::string GetProgName(pid_t pid);