* On POSIX, make sure we don't leak FDs when launching child Processes.

* Add a facility to LaunchProcess() to remap a given FD into a child process.

This change is needed for 2 reasons:
1)We want to use a socketpair() for IPC, the child process needs a known FD # for it's side of the socket.
2)The OS X Sandbox doesn't close FDs.

Review URL: http://codereview.chromium.org/14497

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@7156 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 200 insertions, 18 deletions

diff --git a/base/base.xcodeproj/project.pbxproj b/base/base.xcodeproj/project.pbxproj
index 152b78c..b25d1c8 100644
--- a/base/base.xcodeproj/project.pbxproj
+++ b/base/base.xcodeproj/project.pbxproj
@@ -171,6 +171,7 @@
 		ABFBD3E60DC793C600E164CB /* md5.cc in Sources */ = {isa = PBXBuildFile; fileRef = 825403290D92D2090006B936 /* md5.cc */; };
 		B52C916C0E9428F500208D01 /* clipboard_unittest.cc in Sources */ = {isa = PBXBuildFile; fileRef = B52C916B0E9428F500208D01 /* clipboard_unittest.cc */; };
 		B53C85280E9C298C000F70AB /* idle_timer.cc in Sources */ = {isa = PBXBuildFile; fileRef = 820EB4EB0E3A60FE009668FC /* idle_timer.cc */; };
+		B5637CB20EF2D79A004EF692 /* process_util_unittest.cc in Sources */ = {isa = PBXBuildFile; fileRef = 7BD8F4A00E65AA2400034DE9 /* process_util_unittest.cc */; };
 		B57E4D780E9C26340090055D /* idletimer_unittest.cc in Sources */ = {isa = PBXBuildFile; fileRef = B57E4D770E9C26340090055D /* idletimer_unittest.cc */; };
 		B5A8618E0EC1257900B332C2 /* clipboard.cc in Sources */ = {isa = PBXBuildFile; fileRef = B5A8618D0EC1257900B332C2 /* clipboard.cc */; };
 		B5D544AB0EAFB7E000272A1C /* sys_string_conversions_unittest.cc in Sources */ = {isa = PBXBuildFile; fileRef = B5D544AA0EAFB7E000272A1C /* sys_string_conversions_unittest.cc */; };
@@ -1465,6 +1466,7 @@
 				7B78D3960E54FE0100609465 /* path_service_unittest.cc in Sources */,
 				7B78D3970E54FE0100609465 /* pickle_unittest.cc in Sources */,
 				7B8505D50E5B441000730B43 /* png_codec_unittest.cc in Sources */,
+				B5637CB20EF2D79A004EF692 /* process_util_unittest.cc in Sources */,
 				7B78D3980E54FE0100609465 /* pr_time_unittest.cc in Sources */,
 				4D11B59C0E91730500EF7617 /* rand_util_unittest.cc in Sources */,
 				7B8505D30E5B43EE00730B43 /* rect_unittest.cc in Sources */,
diff --git a/base/multiprocess_test.h b/base/multiprocess_test.h
index a9c67d5..4cd5785 100644
--- a/base/multiprocess_test.h
+++ b/base/multiprocess_test.h
@@ -61,11 +61,20 @@ class MultiProcessTest : public PlatformTest {
   // TODO(darin): re-enable this once we have base/debug_util.h
   // ProcessDebugFlags(&cl, DebugUtil::UNKNOWN, false);
   base::ProcessHandle SpawnChild(const std::wstring& procname) {
-    return SpawnChild(procname, false);
+    base::file_handle_mapping_vector empty_file_list;
+    return SpawnChild(procname, empty_file_list, false);
   }
 
   base::ProcessHandle SpawnChild(const std::wstring& procname,
                                  bool debug_on_start) {
+    base::file_handle_mapping_vector empty_file_list;
+    return SpawnChild(procname, empty_file_list, false);
+  }
+
+  base::ProcessHandle SpawnChild(
+      const std::wstring& procname,
+      const base::file_handle_mapping_vector& fds_to_map,
+      bool debug_on_start) {
     CommandLine cl;
     base::ProcessHandle handle = static_cast<base::ProcessHandle>(NULL);
 
@@ -89,7 +98,7 @@ class MultiProcessTest : public PlatformTest {
 
     std::string switchstr = WideToUTF8(wswitchstr);
     clvec.push_back(switchstr.c_str());
-    base::LaunchApp(clvec, false, &handle);
+    base::LaunchApp(clvec, fds_to_map, false, &handle);
 #endif
 
     return handle;
diff --git a/base/process_util.h b/base/process_util.h
index 5146c12..c88d341 100644
--- a/base/process_util.h
+++ b/base/process_util.h
@@ -66,6 +66,12 @@ ProcessHandle GetCurrentProcessHandle();
 // Win XP SP1 as well.
 int GetProcId(ProcessHandle process);
 
+#if defined(OS_POSIX)
+// Returns the maximum number of files that a process can have open.
+// Returns 0 on error.
+int GetMaxFilesOpenInProcess();
+#endif
+
 #if defined(OS_WIN)
 // Runs the given application name with the given command line. Normally, the
 // first command line argument should be the path to the process, and don't
@@ -87,13 +93,18 @@ bool LaunchApp(const std::wstring& cmdline,
 // Runs the application specified in argv[0] with the command line argv.
 // Both the elements of argv and argv itself must be terminated with a null
 // byte.
+// Before launching all FDs open in the parent process will be marked as
+// close-on-exec.  |fds_to_remap| defines a mapping of src fd->dest fd to
+// propagate FDs into the child process.
 //
 // As above, if wait is true, execute synchronously. The pid will be stored
 // in process_handle if that pointer is non-null.
 //
 // Note that the first argument in argv must point to the filename,
 // and must be fully specified.
+typedef std::vector<std::pair<int, int> > file_handle_mapping_vector;
 bool LaunchApp(const std::vector<std::string>& argv,
+               const file_handle_mapping_vector& fds_to_remap,
                bool wait, ProcessHandle* process_handle);
 #endif
 
diff --git a/base/process_util_linux.cc b/base/process_util_linux.cc
index 8109032..ba22a40 100644
--- a/base/process_util_linux.cc
+++ b/base/process_util_linux.cc
@@ -6,6 +6,8 @@
 
 #include <ctype.h>
 #include <dirent.h>
+#include <fcntl.h>
+#include <unistd.h>
 #include <string>
 #include <sys/types.h>
 #include <sys/wait.h>
@@ -29,6 +31,7 @@ enum ParsingState {
 namespace base {
 
 bool LaunchApp(const std::vector<std::string>& argv,
+               const file_handle_mapping_vector& fds_to_remap,
                bool wait, ProcessHandle* process_handle) {
   bool retval = true;
 
@@ -39,8 +42,33 @@ bool LaunchApp(const std::vector<std::string>& argv,
   }
   argv_copy[argv.size()] = NULL;
 
+  // Make sure we don't leak any FDs to the child process by marking all FDs
+  // as close-on-exec.
+  int max_files = GetMaxFilesOpenInProcess();
+  for (int i = STDERR_FILENO + 1; i < max_files; i++) {
+    int flags = fcntl(i, F_GETFD);
+    if (flags != -1) {
+      fcntl(i, F_SETFD, flags | FD_CLOEXEC);
+    }
+  }
+
   int pid = fork();
   if (pid == 0) {
+    for (file_handle_mapping_vector::const_iterator it = fds_to_remap.begin();
+         it != fds_to_remap.end();
+         ++it) {
+      int src_fd = it->first;
+      int dest_fd = it->second;
+      if (src_fd == dest_fd) {
+        int flags = fcntl(src_fd, F_GETFD);
+        if (flags != -1) {
+          fcntl(src_fd, F_SETFD, flags & ~FD_CLOEXEC);
+        }
+      } else {
+        dup2(src_fd, dest_fd);
+      }
+    }
+
     execvp(argv_copy[0], argv_copy);
   } else if (pid < 0) {
     retval = false;
@@ -60,7 +88,8 @@ bool LaunchApp(const std::vector<std::string>& argv,
 
 bool LaunchApp(const CommandLine& cl,
                bool wait, bool start_hidden, ProcessHandle* process_handle) {
-  return LaunchApp(cl.argv(), wait, process_handle);
+  file_handle_mapping_vector no_files;
+  return LaunchApp(cl.argv(), no_files, wait, process_handle);
 }
 
 // Attempts to kill the process identified by the given process
@@ -108,7 +137,7 @@ bool DidProcessCrash(ProcessHandle handle) {
 }
 
 NamedProcessIterator::NamedProcessIterator(const std::wstring& executable_name,
-                                           const ProcessFilter* filter) 
+                                           const ProcessFilter* filter)
     :
        executable_name_(executable_name),
        filter_(filter) {
diff --git a/base/process_util_mac.mm b/base/process_util_mac.mm
index c29b488..330e8b2 100644
--- a/base/process_util_mac.mm
+++ b/base/process_util_mac.mm
@@ -19,41 +19,81 @@ extern char** environ;
 namespace base {
 
 bool LaunchApp(const std::vector<std::string>& argv,
+               const file_handle_mapping_vector& fds_to_remap,
                bool wait, ProcessHandle* process_handle) {
   bool retval = true;
-  
+
   char* argv_copy[argv.size() + 1];
   for (size_t i = 0; i < argv.size(); i++) {
     argv_copy[i] = const_cast<char*>(argv[i].c_str());
   }
   argv_copy[argv.size()] = NULL;
-  
+
+  // Make sure we don't leak any FDs to the child process by marking all FDs
+  // as close-on-exec.
+  int max_files = GetMaxFilesOpenInProcess();
+  for (int i = STDERR_FILENO + 1; i < max_files; i++) {
+    int flags = fcntl(i, F_GETFD);
+    if (flags != -1) {
+      fcntl(i, F_SETFD, flags | FD_CLOEXEC);
+    }
+  }
+
+  posix_spawn_file_actions_t file_actions;
+  if (posix_spawn_file_actions_init(&file_actions) != 0) {
+    return false;
+  }
+
+  // Turn fds_to_remap array into a set of dup2 calls.
+  for (file_handle_mapping_vector::const_iterator it = fds_to_remap.begin();
+       it != fds_to_remap.end();
+       ++it) {
+    int src_fd = it->first;
+    int dest_fd = it->second;
+
+    if (src_fd == dest_fd) {
+      int flags = fcntl(src_fd, F_GETFD);
+      if (flags != -1) {
+        fcntl(src_fd, F_SETFD, flags & ~FD_CLOEXEC);
+      }
+    } else {
+      if (posix_spawn_file_actions_adddup2(&file_actions, src_fd, dest_fd) != 0)
+          {
+        posix_spawn_file_actions_destroy(&file_actions);
+        return false;
+      }
+    }
+  }
+
   int pid = 0;
-  int spawn_succeeded = (posix_spawnp(&pid, 
-                                      argv_copy[0], 
-                                      NULL, 
-                                      NULL, 
-                                      argv_copy, 
+  int spawn_succeeded = (posix_spawnp(&pid,
+                                      argv_copy[0],
+                                      &file_actions,
+                                      NULL,
+                                      argv_copy,
                                       environ) == 0);
-  
-  bool process_handle_valid = pid > 0;  
+
+  posix_spawn_file_actions_destroy(&file_actions);
+
+  bool process_handle_valid = pid > 0;
   if (!spawn_succeeded || !process_handle_valid) {
     retval = false;
   } else {
     if (wait)
       waitpid(pid, 0, 0);
-    
+
     if(process_handle)
       *process_handle = pid;
   }
-  
+
   return retval;
 }
 
 bool LaunchApp(const CommandLine& cl,
                bool wait, bool start_hidden, ProcessHandle* process_handle) {
   // TODO(playmobil): Do we need to respect the start_hidden flag?
-  return LaunchApp(cl.argv(), wait, process_handle);
+  file_handle_mapping_vector no_files;
+  return LaunchApp(cl.argv(), no_files, wait, process_handle);
 }
 
 bool ProcessMetrics::GetIOCounters(IoCounters* io_counters) {
diff --git a/base/process_util_posix.cc b/base/process_util_posix.cc
index 3153a59..2cfa6f9 100644
--- a/base/process_util_posix.cc
+++ b/base/process_util_posix.cc
@@ -9,6 +9,7 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <unistd.h>
+#include <limits>
 
 #include "base/basictypes.h"
 #include "base/logging.h"
@@ -31,6 +32,23 @@ int GetProcId(ProcessHandle process) {
   return process;
 }
 
+int GetMaxFilesOpenInProcess() {
+  struct rlimit rlimit;
+  if (getrlimit(RLIMIT_NOFILE, &rlimit) != 0) {
+    return 0;
+  }
+
+  // rlim_t is a uint64 - clip to maxint.
+  // We do this since we use the value of this function to close FD #s in a loop
+  // if we didn't clamp the value, doing this would be too time consuming.
+  rlim_t max_int = static_cast<rlim_t>(std::numeric_limits<int32>::max());
+  if (rlimit.rlim_cur > max_int) {
+    return max_int;
+  }
+
+  return rlimit.rlim_cur;
+}
+
 ProcessMetrics::ProcessMetrics(ProcessHandle process) : process_(process),
                                                         last_time_(0),
                                                         last_system_time_(0) {
diff --git a/base/process_util_unittest.cc b/base/process_util_unittest.cc
index 7e8b9a2..67c9500 100644
--- a/base/process_util_unittest.cc
+++ b/base/process_util_unittest.cc
@@ -9,10 +9,15 @@
 #include "base/process_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#if defined(OS_LINUX)
+#include <dlfcn.h>
+#endif
+#if defined(OS_POSIX)
+#include <fcntl.h>
+#include <sys/socket.h>
+#endif
 #if defined(OS_WIN)
 #include <windows.h>
-#elif defined(OS_LINUX)
-#include <dlfcn.h>
 #endif
 
 namespace base {
@@ -137,4 +142,72 @@ TEST_F(ProcessUtilTest, CalcFreeMemory) {
 }
 #endif  // defined(OS_WIN)
 
+#if defined(OS_POSIX)
+const int kChildPipe = 20;  // FD # for write end of pipe in child process.
+MULTIPROCESS_TEST_MAIN(ProcessUtilsLeakFDChildProcess) {
+  // This child process counts the number of open FDs, it then writes that
+  // number out to a pipe connected to the parent.
+  int num_open_files = 0;
+  int write_pipe = kChildPipe;
+  int max_files = GetMaxFilesOpenInProcess();
+  for (int i = STDERR_FILENO + 1; i < max_files; i++) {
+    if (i != kChildPipe) {
+      if (close(i) != -1) {
+        LOG(WARNING) << "Leaked FD " << i;
+        num_open_files += 1;
+      }
+    }
+  }
+
+#if defined(OS_LINUX)
+  // On Linux, '/etc/localtime' is opened before the test's main() enters.
+  const int expected_num_open_fds = 1;
+  num_open_files -= expected_num_open_fds;
+#endif  // defined(OS_LINUX)
+
+  write(write_pipe, &num_open_files, sizeof(num_open_files));
+  close(write_pipe);
+
+  return 0;
+}
+
+TEST_F(ProcessUtilTest, FDRemapping) {
+  // Open some files to check they don't get leaked to the child process.
+  int fds[2];
+  pipe(fds);
+  int pipe_read_fd = fds[0];
+  int pipe_write_fd = fds[1];
+
+  // open some dummy fds to make sure they don't propogate over to the
+  // child process.
+  int dev_null = open("/dev/null", O_RDONLY);
+  int sockets[2];
+  socketpair(AF_UNIX, SOCK_STREAM, 0, sockets);
+
+  file_handle_mapping_vector fd_mapping_vec;
+  fd_mapping_vec.push_back(std::pair<int,int>(pipe_write_fd, kChildPipe));
+  ProcessHandle handle = this->SpawnChild(L"ProcessUtilsLeakFDChildProcess",
+                                          fd_mapping_vec,
+                                          false);
+  ASSERT_NE(static_cast<ProcessHandle>(NULL), handle);
+  close(pipe_write_fd);
+
+  // Read number of open files in client process from pipe;
+  int num_open_files = -1;
+  ssize_t bytes_read = read(pipe_read_fd, &num_open_files,
+                            sizeof(num_open_files));
+  ASSERT_EQ(bytes_read, static_cast<ssize_t>(sizeof(num_open_files)));
+
+  // Make sure 0 fds are leaked to the client.
+  ASSERT_EQ(0, num_open_files);
+
+  EXPECT_TRUE(WaitForSingleProcess(handle, 1000));
+  close(fds[0]);
+  close(sockets[0]);
+  close(sockets[1]);
+  close(dev_null);
+}
+
+#endif  // defined(OS_POSIX)
+
 }  // namespace base