From 451c24c1c4c069fbd3e37e90e982cc6b88e9b03a Mon Sep 17 00:00:00 2001
From: Wiktor Garbacz <wiktorg@google.com>
Date: Mon, 11 Jan 2021 03:33:03 -0800
Subject: [PATCH] Fix fd leak

Support swapping and move assignment in FDCloser

PiperOrigin-RevId: 351119550
Change-Id: I9865d2fcad029a440cab60328b8731f8e1dc340f
---
 sandboxed_api/sandbox2/examples/tool/BUILD.bazel     |  1 +
 sandboxed_api/sandbox2/examples/tool/sandbox2tool.cc |  8 +++++---
 sandboxed_api/sandbox2/util/fileops.cc               |  4 ++--
 sandboxed_api/sandbox2/util/fileops.h                | 11 ++++++++++-
 4 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/sandboxed_api/sandbox2/examples/tool/BUILD.bazel b/sandboxed_api/sandbox2/examples/tool/BUILD.bazel
index ec8a786..475ab26 100644
--- a/sandboxed_api/sandbox2/examples/tool/BUILD.bazel
+++ b/sandboxed_api/sandbox2/examples/tool/BUILD.bazel
@@ -33,6 +33,7 @@ cc_binary(
         "//sandboxed_api/sandbox2",
         "//sandboxed_api/sandbox2:util",
         "//sandboxed_api/sandbox2/util:bpf_helper",
+        "//sandboxed_api/sandbox2/util:fileops",
         "//sandboxed_api/util:flags",
         "@com_google_absl//absl/memory",
         "@com_google_absl//absl/strings",
diff --git a/sandboxed_api/sandbox2/examples/tool/sandbox2tool.cc b/sandboxed_api/sandbox2/examples/tool/sandbox2tool.cc
index 01624f3..cc8defb 100644
--- a/sandboxed_api/sandbox2/examples/tool/sandbox2tool.cc
+++ b/sandboxed_api/sandbox2/examples/tool/sandbox2tool.cc
@@ -43,6 +43,7 @@
 #include "sandboxed_api/sandbox2/sandbox2.h"
 #include "sandboxed_api/sandbox2/util.h"
 #include "sandboxed_api/sandbox2/util/bpf_helper.h"
+#include "sandboxed_api/sandbox2/util/fileops.h"
 
 using std::string;
 
@@ -117,10 +118,11 @@ int main(int argc, char** argv) {
   }
   auto executor = absl::make_unique<sandbox2::Executor>(argv[1], args, envp);
 
-  int recv_fd1 = -1;
+  sandbox2::file_util::fileops::FDCloser recv_fd1;
   if (absl::GetFlag(FLAGS_sandbox2tool_redirect_fd1)) {
     // Make the sandboxed process' fd be available as fd in the current process.
-    recv_fd1 = executor->ipc()->ReceiveFd(STDOUT_FILENO);
+    recv_fd1 = sandbox2::file_util::fileops::FDCloser(
+        executor->ipc()->ReceiveFd(STDOUT_FILENO));
   }
 
   executor
@@ -206,7 +208,7 @@ int main(int argc, char** argv) {
       sleep(1);
       s2.DumpStackTrace();
     } else if (absl::GetFlag(FLAGS_sandbox2tool_redirect_fd1)) {
-      OutputFD(recv_fd1);
+      OutputFD(recv_fd1.get());
       // We couldn't receive more data from the sandboxee's STDOUT_FILENO, but
       // the process could still be running. Kill it unconditionally. A correct
       // final status code will be reported instead of Result::EXTERNAL_KILL.
diff --git a/sandboxed_api/sandbox2/util/fileops.cc b/sandboxed_api/sandbox2/util/fileops.cc
index 2132503..f158ebc 100644
--- a/sandboxed_api/sandbox2/util/fileops.cc
+++ b/sandboxed_api/sandbox2/util/fileops.cc
@@ -34,7 +34,7 @@ FDCloser::~FDCloser() { Close(); }
 
 bool FDCloser::Close() {
   int fd = Release();
-  if (fd == -1) {
+  if (fd == kCanonicalInvalidFd) {
     return false;
   }
   return close(fd) == 0 || errno == EINTR;
@@ -42,7 +42,7 @@ bool FDCloser::Close() {
 
 int FDCloser::Release() {
   int ret = fd_;
-  fd_ = -1;
+  fd_ = kCanonicalInvalidFd;
   return ret;
 }
 
diff --git a/sandboxed_api/sandbox2/util/fileops.h b/sandboxed_api/sandbox2/util/fileops.h
index 9d03fd1..0830ea5 100644
--- a/sandboxed_api/sandbox2/util/fileops.h
+++ b/sandboxed_api/sandbox2/util/fileops.h
@@ -16,6 +16,7 @@
 #define SANDBOXED_API_SANDBOX2_UTIL_FILEOPS_H_
 
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "absl/strings/string_view.h"
@@ -28,17 +29,25 @@ namespace fileops {
 // RAII helper class to automatically close file descriptors.
 class FDCloser {
  public:
-  explicit FDCloser(int fd) : fd_{fd} {}
+  explicit FDCloser(int fd = kCanonicalInvalidFd) : fd_{fd} {}
   FDCloser(const FDCloser&) = delete;
   FDCloser& operator=(const FDCloser&) = delete;
   FDCloser(FDCloser&& other) : fd_(other.Release()) {}
+  FDCloser& operator=(FDCloser&& other) {
+    Swap(other);
+    other.Close();
+    return *this;
+  }
   ~FDCloser();
 
   int get() const { return fd_; }
   bool Close();
+  void Swap(FDCloser& other) { std::swap(fd_, other.fd_); }
   int Release();
 
  private:
+  static constexpr int kCanonicalInvalidFd = -1;
+
   int fd_;
 };