libbrillo: process: close unused file descriptors inherited from parent

Currently, we're leaking all opened file descriptors from the
parent process to the child process. This causes undesired side
effects where the file descriptors in the parent might get into
a weird state (e.g. file is ready for read without any data).

Fix this by allowing the caller to set a flag, so that unused file
descriptors inherited from parent will get closed when starting
the child process.  Default it to false for now to avoid causing
any unexpected side effects for other callers.

Bug: None
BUG=chromium:567357
TEST=new unit tests
TEST=Manual test

Change-Id: Icf13c4832305096c54e013872360f1da6ee65fb8

4 files changed, 97 insertions, 1 deletions

diff --git a/brillo/process.cc b/brillo/process.cc
index 6199bc1..7edef02 100644
--- a/brillo/process.cc
+++ b/brillo/process.cc
@@ -17,6 +17,7 @@
 #include <base/files/file_util.h>
 #include <base/logging.h>
 #include <base/posix/eintr_wrapper.h>
+#include <base/process/process_metrics.h>
 #include <base/strings/string_number_conversions.h>
 #include <base/strings/string_util.h>
 #include <base/time/time.h>
@@ -49,7 +50,8 @@ ProcessImpl::ProcessImpl()
       gid_(-1),
       pre_exec_(base::Bind(&ReturnTrue)),
       search_path_(false),
-      inherit_parent_signal_mask_(false) {
+      inherit_parent_signal_mask_(false),
+      close_unused_file_descriptors_(false) {
 }
 
 ProcessImpl::~ProcessImpl() {
@@ -84,6 +86,10 @@ void ProcessImpl::BindFd(int parent_fd, int child_fd) {
   pipe_map_[child_fd] = info;
 }
 
+void ProcessImpl::SetCloseUnusedFileDescriptors(bool close_unused_fds) {
+  close_unused_file_descriptors_ = close_unused_fds;
+}
+
 void ProcessImpl::SetUid(uid_t uid) {
   uid_ = uid;
 }
@@ -150,6 +156,39 @@ bool ProcessImpl::PopulatePipeMap() {
   return true;
 }
 
+bool ProcessImpl::IsFileDescriptorInPipeMap(int fd) const {
+  for (const auto& pipe : pipe_map_) {
+    if (fd == pipe.second.parent_fd_ ||
+        fd == pipe.second.child_fd_ ||
+        fd == pipe.first) {
+      return true;
+    }
+  }
+  return false;
+}
+
+void ProcessImpl::CloseUnusedFileDescriptors() {
+  size_t max_fds = base::GetMaxFds();
+  for (size_t i = 0; i < max_fds; i++) {
+    const int fd = static_cast<int>(i);
+
+    // Ignore STD file descriptors.
+    if (fd == STDIN_FILENO || fd == STDOUT_FILENO || fd == STDERR_FILENO) {
+      continue;
+    }
+
+    // Ignore file descriptors used by the PipeMap, they will be handled
+    // by this process later on.
+    if (IsFileDescriptorInPipeMap(fd)) {
+      continue;
+    }
+
+    // Since we're just trying to close anything we can find,
+    // ignore any error return values of close().
+    IGNORE_EINTR(close(fd));
+ }
+}
+
 bool ProcessImpl::Start() {
   // If no arguments are provided, fail.
   if (arguments_.empty()) {
@@ -177,6 +216,10 @@ bool ProcessImpl::Start() {
 
   if (pid == 0) {
     // Executing inside the child process.
+    // Close unused file descriptors.
+    if (close_unused_file_descriptors_) {
+      CloseUnusedFileDescriptors();
+    }
     // Close parent's side of the child pipes. dup2 ours into place and
     // then close our ends.
     for (PipeMap::iterator i = pipe_map_.begin(); i != pipe_map_.end(); ++i) {
diff --git a/brillo/process.h b/brillo/process.h
index d527d76..578879a 100644
--- a/brillo/process.h
+++ b/brillo/process.h
@@ -60,6 +60,12 @@ class BRILLO_EXPORT Process {
   // descriptor in the child.
   virtual void BindFd(int parent_fd, int child_fd) = 0;
 
+  // Set a flag |close_unused_fds| to indicate if the child process
+  // should close all unused file descriptors inherited from the
+  // parent process.  This will not close the file descriptors for
+  // the standard streams (stdin, stdout, and stderr).
+  virtual void SetCloseUnusedFileDescriptors(bool close_unused_fds) = 0;
+
   // Set the real/effective/saved user ID of the child process.
   virtual void SetUid(uid_t uid) = 0;
 
@@ -141,6 +147,7 @@ class BRILLO_EXPORT ProcessImpl : public Process {
   virtual void RedirectOutput(const std::string& output_file);
   virtual void RedirectUsingPipe(int child_fd, bool is_input);
   virtual void BindFd(int parent_fd, int child_fd);
+  virtual void SetCloseUnusedFileDescriptors(bool close_unused_fds);
   virtual void SetUid(uid_t uid);
   virtual void SetGid(gid_t gid);
   virtual void SetInheritParentSignalMask(bool inherit);
@@ -176,6 +183,9 @@ class BRILLO_EXPORT ProcessImpl : public Process {
  private:
   FRIEND_TEST(ProcessTest, ResetPidByFile);
 
+  bool IsFileDescriptorInPipeMap(int fd) const;
+  void CloseUnusedFileDescriptors();
+
   // Pid of currently managed process or 0 if no currently managed
   // process.  pid must not be modified except by calling
   // UpdatePid(new_pid).
@@ -193,6 +203,10 @@ class BRILLO_EXPORT ProcessImpl : public Process {
   // is set to false by default, which means by default the child process
   // will not inherit signal mask from the parent process.
   bool inherit_parent_signal_mask_;
+  // Flag indicating to close unused file descriptors inherited from the
+  // parent process when starting the child process, which avoids leaking
+  // unnecessary file descriptors to the child process.
+  bool close_unused_file_descriptors_;
 };
 
 }  // namespace brillo
diff --git a/brillo/process_mock.h b/brillo/process_mock.h
index c89fefe..d821cc4 100644
--- a/brillo/process_mock.h
+++ b/brillo/process_mock.h
@@ -36,6 +36,7 @@ class ProcessMock : public Process {
   MOCK_METHOD1(Reset, void(pid_t));
   MOCK_METHOD1(ResetPidByFile, bool(const std::string& pid_file));
   MOCK_METHOD0(Release, pid_t());
+  MOCK_METHOD1(SetCloseUnusedFileDescriptors, void(bool close_unused_fds));
 };
 
 }  // namespace brillo
diff --git a/brillo/process_unittest.cc b/brillo/process_unittest.cc
index ebf1d4e..16c1ecc 100644
--- a/brillo/process_unittest.cc
+++ b/brillo/process_unittest.cc
@@ -94,6 +94,7 @@ class ProcessTest : public ::testing::Test {
  protected:
   void CheckStderrCaptured();
   FilePath GetFdPath(int fd);
+  bool FileDescriptorExists(int pid, int fd);
 
   ProcessImpl process_;
   std::vector<const char*> args_;
@@ -167,6 +168,11 @@ FilePath ProcessTest::GetFdPath(int fd) {
   return FilePath(base::StringPrintf("/proc/self/fd/%d", fd));
 }
 
+bool ProcessTest::FileDescriptorExists(int pid, int fd) {
+  return base::PathExists(
+      FilePath(base::StringPrintf("/proc/%d/fd/%d", pid, fd)));
+}
+
 TEST_F(ProcessTest, RedirectStderrUsingPipe) {
   std::string contents;
   process_.RedirectOutput("");
@@ -334,4 +340,36 @@ TEST_F(ProcessTest, PreExecCallback) {
   ASSERT_NE(0, process_.Run());
 }
 
+TEST_F(ProcessTest, LeakUnusedFileDescriptors) {
+  int fds[2];
+  EXPECT_EQ(0, pipe(fds));
+  process_.AddArg(kBinSleep);
+  process_.AddArg("10000");
+  process_.SetCloseUnusedFileDescriptors(false);
+  ASSERT_TRUE(process_.Start());
+  // Give child process a bit time to come up.
+  usleep(10 * 1000);
+  // Verify file descriptors are leaking to the child process.
+  EXPECT_TRUE(FileDescriptorExists(process_.pid(), fds[0]));
+  EXPECT_TRUE(FileDescriptorExists(process_.pid(), fds[1]));
+
+  EXPECT_TRUE(process_.Kill(SIGTERM, 1));
+}
+
+TEST_F(ProcessTest, CloseUnusedFileDescriptors) {
+  int fds[2];
+  EXPECT_EQ(0, pipe(fds));
+  process_.AddArg(kBinSleep);
+  process_.AddArg("10000");
+  process_.SetCloseUnusedFileDescriptors(true);
+  ASSERT_TRUE(process_.Start());
+  // Give child process a bit time to come up.
+  usleep(10 * 1000);
+  // Verify file descriptors does not get leak to the child process.
+  EXPECT_FALSE(FileDescriptorExists(process_.pid(), fds[0]));
+  EXPECT_FALSE(FileDescriptorExists(process_.pid(), fds[1]));
+
+  EXPECT_TRUE(process_.Kill(SIGTERM, 1));
+}
+
 }  // namespace brillo