Automated rollback of commit af2d5c4.

PiperOrigin-RevId: 624974126
Change-Id: I01b0aa6109745d37cfe717b5155ec302206378f4

sandboxed_api/BUILD

@@ -94,7 +94,6 @@ cc_library(
         "//sandboxed_api/sandbox2",
         "//sandboxed_api/sandbox2:client",
         "//sandboxed_api/sandbox2:comms",
-        "//sandboxed_api/sandbox2:util",
         "//sandboxed_api/util:file_base",
         "//sandboxed_api/util:fileops",
         "//sandboxed_api/util:runfiles",
@@ -168,7 +167,6 @@ cc_library(
         ":proto_arg_cc_proto",
         ":var_type",
         "//sandboxed_api/sandbox2:comms",
-        "//sandboxed_api/sandbox2:util",
         "//sandboxed_api/util:status",
         "@com_google_absl//absl/base:core_headers",
         "@com_google_absl//absl/log",
@@ -178,7 +176,6 @@ cc_library(
         "@com_google_absl//absl/strings",
         "@com_google_absl//absl/strings:str_format",
         "@com_google_absl//absl/synchronization",
-        "@com_google_absl//absl/types:span",
         "@com_google_absl//absl/utility",
     ],
 )

sandboxed_api/CMakeLists.txt

@@ -150,15 +150,13 @@ add_library(sapi_vars ${SAPI_LIB_TYPE}
 add_library(sapi::vars ALIAS sapi_vars)
 target_link_libraries(sapi_vars
   PRIVATE absl::core_headers
-          absl::span
           absl::status
           absl::statusor
           absl::str_format
           absl::strings
           absl::synchronization
           absl::utility
           sandbox2::comms
-          sandbox2::util
           sapi::base
           sapi::call
           sapi::lenval_core

sandboxed_api/sandbox.cc

@@ -19,7 +19,6 @@
 #include <sys/uio.h>
 #include <syscall.h>
 
-#include <cstddef>
 #include <cstdint>
 #include <cstdio>
 #include <initializer_list>
@@ -47,7 +46,6 @@
 #include "sandboxed_api/sandbox2/policybuilder.h"
 #include "sandboxed_api/sandbox2/result.h"
 #include "sandboxed_api/sandbox2/sandbox2.h"
-#include "sandboxed_api/sandbox2/util.h"
 #include "sandboxed_api/util/path.h"
 #include "sandboxed_api/util/runfiles.h"
 #include "sandboxed_api/util/status_macros.h"
@@ -452,11 +450,21 @@ absl::StatusOr<std::string> Sandbox::GetCString(const v::RemotePtr& str,
         absl::StrCat("Target string too large: ", len, " > ", max_length));
   }
   std::string buffer(len, '\0');
-  SAPI_ASSIGN_OR_RETURN(
-      size_t ret,
-      sandbox2::util::ReadBytesFromPidInto(
-          pid_, reinterpret_cast<uintptr_t>(str.GetValue()),
-          absl::MakeSpan(reinterpret_cast<char*>(buffer.data()), len)));
+  struct iovec local = {
+      .iov_base = &buffer[0],
+      .iov_len = len,
+  };
+  struct iovec remote = {
+      .iov_base = str.GetValue(),
+      .iov_len = len,
+  };
+
+  ssize_t ret = process_vm_readv(pid_, &local, 1, &remote, 1, 0);
+  if (ret == -1) {
+    PLOG(WARNING) << "reading c-string failed: process_vm_readv(pid: " << pid_
+                  << " raddr: " << str.GetValue() << " size: " << len << ")";
+    return absl::UnavailableError("process_vm_readv failed");
+  }
   if (ret != len) {
     LOG(WARNING) << "partial read when reading c-string: process_vm_readv(pid: "
                  << pid_ << " raddr: " << str.GetValue() << " size: " << len

sandboxed_api/sandbox2/BUILD

@@ -801,7 +801,6 @@ cc_library(
         "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
         "@com_google_absl//absl/strings:str_format",
-        "@com_google_absl//absl/types:span",
     ],
 )
 
@@ -1035,10 +1034,8 @@ cc_test(
         "//sandboxed_api:testing",
         "//sandboxed_api/util:status_matchers",
         "@com_google_absl//absl/cleanup",
-        "@com_google_absl//absl/log:check",
         "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
-        "@com_google_absl//absl/types:span",
         "@com_google_googletest//:gtest_main",
     ],
 )

sandboxed_api/sandbox2/CMakeLists.txt

@@ -700,8 +700,7 @@ target_link_libraries(sandbox2_util
           sapi::base
           sapi::raw_logging
           sapi::status
-  PUBLIC absl::span
-         absl::status
+  PUBLIC absl::status
          absl::statusor
 )
 target_compile_options(sandbox2_util PRIVATE
@@ -1117,9 +1116,7 @@ if(BUILD_TESTING AND SAPI_BUILD_TESTING)
     sandbox2::util
     absl::statusor
     absl::strings
-    absl::check
     absl::cleanup
-    absl::span
     sapi::status_matchers
     sapi::testing
     sapi::test_main

sandboxed_api/sandbox2/util.cc

@@ -14,7 +14,6 @@
 
 #include "sandboxed_api/sandbox2/util.h"
 
-#include <fcntl.h>
 #include <linux/limits.h>
 #include <sched.h>
 #include <spawn.h>
@@ -51,7 +50,6 @@
 #include "absl/strings/str_replace.h"
 #include "absl/strings/str_split.h"
 #include "absl/strings/string_view.h"
-#include "absl/types/span.h"
 #include "sandboxed_api/config.h"
 #include "sandboxed_api/util/file_helpers.h"
 #include "sandboxed_api/util/fileops.h"
@@ -412,178 +410,39 @@ std::string GetPtraceEventName(int event) {
   }
 }
 
-namespace {
-
-// Transfer memory via process_vm_readv/process_vm_writev.
-absl::StatusOr<size_t> ProcessVmTransfer(bool is_read, pid_t pid, uintptr_t ptr,
-                                         absl::Span<char> data) {
+absl::StatusOr<std::vector<uint8_t>> ReadBytesFromPid(pid_t pid, uintptr_t ptr,
+                                                      uint64_t size) {
   static const uintptr_t page_size = getpagesize();
   static const uintptr_t page_mask = page_size - 1;
 
   // Input sanity checks.
-  if (data.empty()) {
-    return 0;
+  if (size == 0) {
+    return std::vector<uint8_t>();
   }
 
-  iovec local_iov[] = {{data.data(), data.size()}};
+  // Allocate enough bytes to hold the entire size.
+  std::vector<uint8_t> bytes(size, 0);
+  iovec local_iov[] = {{bytes.data(), bytes.size()}};
   // Stores all the necessary iovecs to move memory.
   std::vector<iovec> remote_iov;
   // Each iovec should be contained to a single page.
   size_t consumed = 0;
-  while (consumed < data.size()) {
+  while (consumed < size) {
     // Read till the end of the page, at most the remaining number of bytes.
-    size_t chunk_size = std::min(data.size() - consumed,
-                                 page_size - ((ptr + consumed) & page_mask));
+    size_t chunk_size =
+        std::min(size - consumed, page_size - ((ptr + consumed) & page_mask));
     remote_iov.push_back({reinterpret_cast<void*>(ptr + consumed), chunk_size});
     consumed += chunk_size;
   }
 
-  ssize_t result =
-      is_read ? process_vm_readv(pid, local_iov, ABSL_ARRAYSIZE(local_iov),
-                                 remote_iov.data(), remote_iov.size(), 0)
-              : process_vm_writev(pid, local_iov, ABSL_ARRAYSIZE(local_iov),
-                                  remote_iov.data(), remote_iov.size(), 0);
+  ssize_t result = process_vm_readv(pid, local_iov, ABSL_ARRAYSIZE(local_iov),
+                                    remote_iov.data(), remote_iov.size(), 0);
   if (result < 0) {
     return absl::ErrnoToStatus(
-        errno, absl::StrFormat("transfer() failed for PID: %d at address: %#x",
-                               pid, ptr));
-  }
-  return result;
-}
-
-// Open /proc/pid/mem file descriptor.
-absl::StatusOr<file_util::fileops::FDCloser> OpenProcMem(pid_t pid,
-                                                         bool is_read) {
-  auto path = absl::StrFormat("/proc/%d/mem", pid);
-  auto closer = file_util::fileops::FDCloser(
-      open(path.c_str(), is_read ? O_RDONLY : O_WRONLY));
-  if (closer.get() == -1) {
-    return absl::ErrnoToStatus(
-        errno, absl::StrFormat("open() failed for PID: %d", pid));
-  }
-  return closer;
-}
-
-absl::StatusOr<size_t> ProcMemTransfer(bool is_read, pid_t pid, uintptr_t ptr,
-                                       absl::Span<char> data) {
-  if (data.empty()) {
-    return 0;
-  }
-
-  SAPI_ASSIGN_OR_RETURN(file_util::fileops::FDCloser fd_closer,
-                        OpenProcMem(pid, is_read));
-  size_t total_bytes_transferred = 0;
-  while (!data.empty()) {
-    ssize_t bytes_transfered =
-        is_read ? bytes_transfered =
-                      pread(fd_closer.get(), data.data(), data.size(), ptr)
-                : bytes_transfered =
-                      pwrite(fd_closer.get(), data.data(), data.size(), ptr);
-    if (bytes_transfered == 0) {
-      if (total_bytes_transferred == 0) {
-        return absl::NotFoundError(absl::StrFormat(
-            "Transfer was unsuccessful for PID: %d at address: %#x", pid, ptr));
-      }
-      break;
-    } else if (bytes_transfered < 0) {
-      if (total_bytes_transferred > 0) {
-        // Return number of bytes transferred until this error or end.
-        break;
-      }
-      // pread/write of /proc/<pid>mem returns EIO when ptr is unmapped.
-      if (errno == EIO) {
-        // Emulate returned error code from process_vm_readv.
-        errno = EFAULT;
-      }
-      return absl::ErrnoToStatus(
-          errno,
-          absl::StrFormat("transfer() failed for PID: %d at address: %#x", pid,
-                          ptr));
-    }
-    ptr += bytes_transfered;
-    data = data.subspan(bytes_transfered, data.size() - bytes_transfered);
-    total_bytes_transferred += bytes_transfered;
-  }
-  return total_bytes_transferred;
-}
-
-bool CheckIfProcessVmTransferWorks() {
-  // Fall-back to pread("/proc/$pid/mem") if process_vm_readv is unavailable.
-  static bool process_vm_transfer_works = []() {
-    constexpr char kMagic = 42;
-    char src = kMagic;
-    char dst = 0;
-    absl::StatusOr<size_t> read = internal::ReadBytesFromPidWithReadv(
-        getpid(), reinterpret_cast<uintptr_t>(&src), absl::MakeSpan(&dst, 1));
-    if (!read.ok() || *read != 1 || dst != kMagic) {
-      SAPI_RAW_LOG(WARNING,
-                   "This system does not seem to support the process_vm_readv()"
-                   " or process_vm_writev syscall. Falling back to transfers"
-                   " via /proc/pid/mem.");
-      return false;
-    }
-    return true;
-  }();
-  return process_vm_transfer_works;
-}
-
-}  // namespace
-
-namespace internal {
-
-absl::StatusOr<size_t> ReadBytesFromPidWithReadv(pid_t pid, uintptr_t ptr,
-                                                 absl::Span<char> data) {
-  return ProcessVmTransfer(true, pid, ptr, data);
-}
-
-absl::StatusOr<size_t> WriteBytesToPidWithWritev(pid_t pid, uintptr_t ptr,
-                                                 absl::Span<const char> data) {
-  return ProcessVmTransfer(
-      false, pid, ptr,
-      absl::MakeSpan(const_cast<char*>(data.data()), data.size()));
-}
-
-absl::StatusOr<size_t> ReadBytesFromPidWithProcMem(pid_t pid, uintptr_t ptr,
-                                                   absl::Span<char> data) {
-  return ProcMemTransfer(true, pid, ptr, data);
-}
-
-absl::StatusOr<size_t> WriteBytesToPidWithProcMem(pid_t pid, uintptr_t ptr,
-                                                  absl::Span<const char> data) {
-  return ProcMemTransfer(
-      false, pid, ptr,
-      absl::MakeSpan(const_cast<char*>(data.data()), data.size()));
-}
-
-}  // namespace internal
-
-absl::StatusOr<size_t> ReadBytesFromPidInto(pid_t pid, uintptr_t ptr,
-                                            absl::Span<char> data) {
-  if (CheckIfProcessVmTransferWorks()) {
-    return internal::ReadBytesFromPidWithReadv(pid, ptr, data);
-  } else {
-    return internal::ReadBytesFromPidWithProcMem(pid, ptr, data);
-  }
-}
-
-absl::StatusOr<size_t> WriteBytesToPidFrom(pid_t pid, uintptr_t ptr,
-                                           absl::Span<const char> data) {
-  if (CheckIfProcessVmTransferWorks()) {
-    return internal::WriteBytesToPidWithWritev(pid, ptr, data);
-  } else {
-    return internal::WriteBytesToPidWithProcMem(pid, ptr, data);
+        errno,
+        absl::StrFormat("process_vm_readv() failed for PID: %d at address: %#x",
+                        pid, ptr));
   }
-}
-
-absl::StatusOr<std::vector<uint8_t>> ReadBytesFromPid(pid_t pid, uintptr_t ptr,
-                                                      size_t size) {
-  // Allocate enough bytes to hold the entire size.
-  std::vector<uint8_t> bytes(size, 0);
-  SAPI_ASSIGN_OR_RETURN(
-      size_t result,
-      ReadBytesFromPidInto(
-          pid, ptr,
-          absl::MakeSpan(reinterpret_cast<char*>(bytes.data()), size)));
   // Ensure only successfully read bytes are returned.
   bytes.resize(result);
   return bytes;

sandboxed_api/sandbox2/util.h

@@ -20,15 +20,13 @@
 
 #include <sys/types.h>
 
-#include <cstddef>
 #include <cstdint>
 #include <string>
 #include <vector>
 
 #include "absl/base/attributes.h"
 #include "absl/base/macros.h"
 #include "absl/status/statusor.h"
-#include "absl/types/span.h"
 
 namespace sandbox2::util {
 
@@ -109,38 +107,9 @@ std::string GetRlimitName(int resource);
 // Returns ptrace event name
 std::string GetPtraceEventName(int event);
 
-namespace internal {
-// Reads `data`'s length of bytes from `ptr` in `pid`, returns number of bytes
-// read or an error.
-absl::StatusOr<size_t> ReadBytesFromPidWithReadv(pid_t pid, uintptr_t ptr,
-                                                 absl::Span<char> data);
-
-// Writes `data` to `ptr` in `pid`, returns number of bytes written or an error.
-absl::StatusOr<size_t> WriteBytesToPidWithWritev(pid_t pid, uintptr_t ptr,
-                                                 absl::Span<const char> data);
-
-// Reads `data`'s length of bytes from `ptr` in `pid`, returns number of bytes
-// read or an error.
-absl::StatusOr<size_t> ReadBytesFromPidWithProcMem(pid_t pid, uintptr_t ptr,
-                                                   absl::Span<char> data);
-
-// Writes `data` to `ptr` in `pid`, returns number of bytes written or an error.
-absl::StatusOr<size_t> WriteBytesToPidWithProcMem(pid_t pid, uintptr_t ptr,
-                                                  absl::Span<const char> data);
-};  // namespace internal
-
-// Reads `data`'s length of bytes from `ptr` in `pid`, returns number of bytes
-// read or an error.
-absl::StatusOr<size_t> ReadBytesFromPidInto(pid_t pid, uintptr_t ptr,
-                                            absl::Span<char> data);
-
-// Writes `data` to `ptr` in `pid`, returns number of bytes written or an error.
-absl::StatusOr<size_t> WriteBytesToPidFrom(pid_t pid, uintptr_t remote_ptr,
-                                           absl::Span<const char> data);
-
 // Reads `size` bytes from the given `ptr` address, or returns an error.
 absl::StatusOr<std::vector<uint8_t>> ReadBytesFromPid(pid_t pid, uintptr_t ptr,
-                                                      size_t size);
+                                                      uint64_t size);
 
 // Reads a path string (NUL-terminated, shorter than PATH_MAX) from another
 // process memory