Rework GetListOfFDs API

PiperOrigin-RevId: 395043959
Change-Id: I77ce13f0c786d3644971ed239f3106319667e979

sandboxed_api/sandbox2/BUILD.bazel

@@ -384,8 +384,11 @@ cc_library(
         "//sandboxed_api/util:file_helpers",
         "//sandboxed_api/util:fileops",
         "//sandboxed_api/util:raw_logging",
+        "//sandboxed_api/util:status",
         "//sandboxed_api/util:strerror",
         "@com_google_absl//absl/base:core_headers",
+        "@com_google_absl//absl/container:flat_hash_set",
+        "@com_google_absl//absl/status:statusor",
         "@com_google_absl//absl/strings",
         "@com_google_glog//:glog",
     ],
@@ -415,6 +418,7 @@ cc_library(
         "//sandboxed_api/util:status",
         "//sandboxed_api/util:strerror",
         "@com_google_absl//absl/container:flat_hash_map",
+        "@com_google_absl//absl/container:flat_hash_set",
         "@com_google_absl//absl/memory",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",

sandboxed_api/sandbox2/CMakeLists.txt

@@ -368,6 +368,7 @@ add_library(sandbox2_sanitizer ${SAPI_LIB_TYPE}
 add_library(sandbox2::sanitizer ALIAS sandbox2_sanitizer)
 target_link_libraries(sandbox2_sanitizer
   PRIVATE absl::core_headers
+          absl::flat_hash_set
           absl::strings
           sapi::file_helpers
           sapi::fileops
@@ -384,6 +385,7 @@ add_library(sandbox2_forkserver ${SAPI_LIB_TYPE}
 add_library(sandbox2::forkserver ALIAS sandbox2_forkserver)
 target_link_libraries(sandbox2_forkserver PRIVATE
   absl::flat_hash_map
+  absl::flat_hash_set
   absl::memory
   absl::status
   absl::statusor

sandboxed_api/sandbox2/forkserver.cc

@@ -35,6 +35,7 @@
 #include <cstring>
 
 #include "absl/container/flat_hash_map.h"
+#include "absl/container/flat_hash_set.h"
 #include "absl/memory/memory.h"
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
@@ -107,7 +108,7 @@ void MoveFDs(std::initializer_list<std::pair<int*, int>> move_fds,
   }
 }
 
-void RunInitProcess(std::set<int> open_fds) {
+void RunInitProcess(const absl::flat_hash_set<int>& open_fds) {
   if (prctl(PR_SET_NAME, "S2-INIT-PROC", 0, 0, 0) != 0) {
     SAPI_RAW_PLOG(WARNING, "prctl(PR_SET_NAME, 'S2-INIT-PROC')");
   }
@@ -266,9 +267,11 @@ void ForkServer::LaunchChild(const ForkRequest& request, int execve_fd,
 
   SanitizeEnvironment();
 
-  std::set<int> open_fds;
-  if (!sanitizer::GetListOfFDs(&open_fds)) {
-    SAPI_RAW_LOG(WARNING, "Could not get list of current open FDs");
+  absl::StatusOr<absl::flat_hash_set<int>> open_fds = sanitizer::GetListOfFDs();
+  if (!open_fds.ok()) {
+    SAPI_RAW_LOG(WARNING, "Could not get list of current open FDs: %s",
+                 std::string(open_fds.status().message()).c_str());
+    open_fds = absl::flat_hash_set<int>();
   }
 
   InitializeNamespaces(request, uid, gid, avoid_pivot_root);
@@ -297,7 +300,7 @@ void ForkServer::LaunchChild(const ForkRequest& request, int execve_fd,
       SAPI_RAW_PLOG(FATAL, "Could not spawn init process");
     }
     if (child != 0) {
-      RunInitProcess(open_fds);
+      RunInitProcess(*open_fds);
     }
     // Send sandboxee pid
     auto status = SendPid(signaling_fd);

sandboxed_api/sandbox2/sanitizer.cc

@@ -35,6 +35,7 @@
 #include <utility>
 #include <vector>
 
+#include "absl/container/flat_hash_set.h"
 #include "absl/strings/numbers.h"
 #include "absl/strings/str_cat.h"
 #include "absl/strings/str_split.h"
@@ -42,6 +43,7 @@
 #include "sandboxed_api/util/file_helpers.h"
 #include "sandboxed_api/util/fileops.h"
 #include "sandboxed_api/util/raw_logging.h"
+#include "sandboxed_api/util/status_macros.h"
 #include "sandboxed_api/util/strerror.h"
 
 namespace sandbox2::sanitizer {
@@ -52,55 +54,62 @@ namespace file_util = ::sapi::file_util;
 constexpr char kProcSelfFd[] = "/proc/self/fd";
 
 // Reads filenames inside the directory and converts them to numerical values.
-bool ListNumericalDirectoryEntries(const std::string& directory,
-                                   std::set<int>* nums) {
+absl::StatusOr<absl::flat_hash_set<int>> ListNumericalDirectoryEntries(
+    const std::string& directory) {
+  absl::flat_hash_set<int> result;
   std::vector<std::string> entries;
   std::string error;
   if (!file_util::fileops::ListDirectoryEntries(directory, &entries, &error)) {
-    SAPI_RAW_LOG(WARNING, "List directory entries for '%s' failed: %s",
-                 kProcSelfFd, error.c_str());
-    return false;
+    return absl::InternalError(absl::StrCat("List directory entries for '",
+                                            directory, "' failed: ", error));
   }
+  result.reserve(entries.size());
   for (const auto& entry : entries) {
     int num;
     if (!absl::SimpleAtoi(entry, &num)) {
-      SAPI_RAW_LOG(WARNING, "Cannot convert %s to a number", entry.c_str());
-      return false;
+      return absl::InternalError(
+          absl::StrCat("Cannot convert ", entry, " to a number"));
     }
-    nums->insert(num);
+    result.insert(num);
   }
-  return true;
+  return result;
 }
 
 }  // namespace
 
-bool GetListOfFDs(std::set<int>* fds) {
-  if (!ListNumericalDirectoryEntries(kProcSelfFd, fds)) {
-    return false;
-  }
-  // Exclude the dirfd which was opened in ListDirectoryEntries.
-  // Most probably will be the highest fd, hence the reverse order.
-  for (auto it = fds->rbegin(), end = fds->rend(); it != end; ++it) {
+absl::StatusOr<absl::flat_hash_set<int>> GetListOfFDs() {
+  SAPI_ASSIGN_OR_RETURN(absl::flat_hash_set<int> fds,
+                        ListNumericalDirectoryEntries(kProcSelfFd));
+
+  //  Exclude the dirfd which was opened in ListDirectoryEntries.
+  for (auto it = fds.begin(), end = fds.end(); it != end; ++it) {
     if (access(absl::StrCat(kProcSelfFd, "/", *it).c_str(), F_OK) != 0) {
-      fds->erase(*it);
+      fds.erase(it);
       break;
     }
   }
-  return true;
+  return fds;
 }
 
 bool GetListOfTasks(int pid, std::set<int>* tasks) {
   const std::string task_dir = absl::StrCat("/proc/", pid, "/task");
-  return ListNumericalDirectoryEntries(task_dir, tasks);
-}
-
-bool CloseAllFDsExcept(const std::set<int>& fd_exceptions) {
-  std::set<int> fds;
-  if (!GetListOfFDs(&fds)) {
+  auto task_entries = ListNumericalDirectoryEntries(task_dir);
+  if (!task_entries.ok()) {
     return false;
   }
 
-  for (auto fd : fds) {
+  tasks->clear();
+  tasks->insert(task_entries->begin(), task_entries->end());
+  return true;
+}
+
+bool CloseAllFDsExcept(const std::set<int>& fd_exceptions) {
+  absl::StatusOr<absl::flat_hash_set<int>> fds = GetListOfFDs();
+  if (!fds.ok()) {
+    return false;
+  }
+
+  for (auto fd : *fds) {
     if (fd_exceptions.find(fd) != fd_exceptions.end()) {
       continue;
     }
@@ -111,12 +120,12 @@ bool CloseAllFDsExcept(const std::set<int>& fd_exceptions) {
 }
 
 bool MarkAllFDsAsCOEExcept(const std::set<int>& fd_exceptions) {
-  std::set<int> fds;
-  if (!GetListOfFDs(&fds)) {
+  auto fds = GetListOfFDs();
+  if (!fds.ok()) {
     return false;
   }
 
-  for (auto fd : fds) {
+  for (auto fd : *fds) {
     if (fd_exceptions.find(fd) != fd_exceptions.end()) {
       continue;
     }

sandboxed_api/sandbox2/sanitizer.h

@@ -21,12 +21,14 @@
 #include <set>
 
 #include "absl/base/macros.h"
+#include "absl/container/flat_hash_set.h"
+#include "absl/status/statusor.h"
 
 namespace sandbox2 {
 namespace sanitizer {
 
 // Reads a list of open file descriptors in the current process.
-bool GetListOfFDs(std::set<int>* fds);
+absl::StatusOr<absl::flat_hash_set<int>> GetListOfFDs();
 
 // Closes all file descriptors in the current process except the ones in
 // fd_exceptions.