Introduce util::CharPtrArray with proper ownership semantics

Replace existing calls to VecStringToCharPtrArr

PiperOrigin-RevId: 417383812
Change-Id: Ibf9d878df5ada2cb3a0872f7ca7cab96c304a5c1

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",

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

sandboxed_api/sandbox2/executor.cc

@@ -38,9 +38,7 @@ namespace sandbox2 {
 namespace file_util = ::sapi::file_util;
 
 std::vector<std::string> Executor::CopyEnviron() {
-  std::vector<std::string> 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,

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<std::string> args;
   std::vector<std::string> 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.

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,

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<std::string>& 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<char**>(argv));
+    execv(path.c_str(), const_cast<char**>(array.data()));
     PLOG(ERROR) << "execv('" << path << "')";
     exit(EXIT_FAILURE);
   }
-  delete[] argv;
 
   for (;;) {
     int status;

sandboxed_api/sandbox2/util.cc

@@ -33,6 +33,7 @@
 #include <cstring>
 #include <memory>
 
+#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<std::string>* 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<std::string>* vec) {
+  *vec = CharPtrArray(arr).ToStringVector();
 }
 
 const char** VecStringToCharPtrArr(const std::vector<std::string>& vec) {
@@ -73,6 +86,39 @@ const char** VecStringToCharPtrArr(const std::vector<std::string>& 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<std::string>& 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<std::string>& vec) {
+  return CharPtrArray(vec);
+}
+
+std::vector<std::string> CharPtrArray::ToStringVector() const {
+  std::vector<std::string> 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<int> Communicate(const std::vector<std::string>& argv,
   posix_spawn_file_actions_adddup2(&action, cout_pipe[1], 2);
   posix_spawn_file_actions_addclose(&action, cout_pipe[1]);
 
-  char** args = const_cast<char**>(util::VecStringToCharPtrArr(argv));
-  char** envp = const_cast<char**>(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<char**>(args.data()),
+                   const_cast<char**>(envp.data())) != 0) {
     return absl::UnknownError(sapi::OsErrorMessage(errno, "posix_spawnp()"));
   }
 

sandboxed_api/sandbox2/util.h

@@ -24,6 +24,7 @@
 #include <string>
 #include <vector>
 
+#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<std::string>.
+ABSL_DEPRECATED("Use CharPtrArray(arr).ToStringVector() instead")
 void CharPtrArrToVecString(char* const* arr, std::vector<std::string>* 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<std::string>& 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<std::string>& vec);
+
+  const std::vector<const char*>& array() const { return array_; }
+
+  const char* const* data() const { return array_.data(); }
+
+  std::vector<std::string> ToStringVector() const;
+
+ private:
+  CharPtrArray(const std::vector<std::string>& vec);
+
+  const std::string content_;
+  std::vector<const char*> array_;
+};
+
 // Returns the program name (via /proc/self/comm) for a given PID.
 std::string GetProgName(pid_t pid);