commit e384bb2e403b0f0f666e89d5f6acfaec41120cc5
author Alex Deymo <deymo@google.com> Tue Mar 29 17:23:33 2016 -0700
committer Alex Deymo <deymo@google.com> Mon Apr 04 19:14:29 2016 -0700
tree c010aa4e478c1d6fc9b78abf486a4725341ed86f
parent b9d4a67734f52c76d1e7472e59276f82f9ec3fb5

Subprocess: Close all unused file descriptors.

This patch extends Subprocess::ExecFlags() method with a list of
file descriptors that should be kept open on the child process and
mapped to a pipe() in the parent. The remaining file descriptors
will be closed.

Bug: 27880754
TEST=Added unittests for this functionality.

Change-Id: Id96fb6a72f7805a0bfacfbc85422dc7d44750d93

common/subprocess.cc

diff --git a/common/subprocess.cc b/common/subprocess.cc
index 61f2d54..1bfb911 100644
--- a/common/subprocess.cc
+++ b/common/subprocess.cc
@@ -71,6 +71,7 @@
 // Process.
 bool LaunchProcess(const vector<string>& cmd,
                    uint32_t flags,
+                   const vector<int>& output_pipes,
                    brillo::Process* proc) {
   for (const string& arg : cmd)
     proc->AddArg(arg);
@@ -84,6 +85,10 @@
       env.emplace(key, value);
   }
 
+  for (const int fd : output_pipes) {
+    proc->RedirectUsingPipe(fd, false);
+  }
+  proc->SetCloseUnusedFileDescriptors(true);
   proc->RedirectUsingPipe(STDOUT_FILENO, false);
   proc->SetPreExecCallback(base::Bind(&SetupChild, env, flags));
 
@@ -143,10 +148,6 @@
   MessageLoop::current()->CancelTask(record->stdout_task_id);
   record->stdout_task_id = MessageLoop::kTaskIdNull;
 
-  // Release and close all the pipes now.
-  record->proc.Release();
-  record->proc.Reset(0);
-
   // Don't print any log if the subprocess exited with exit code 0.
   if (info.si_code != CLD_EXITED) {
     LOG(INFO) << "Subprocess terminated with si_code " << info.si_code;
@@ -160,20 +161,28 @@
   if (!record->callback.is_null()) {
     record->callback.Run(info.si_status, record->stdout);
   }
+  // Release and close all the pipes after calling the callback so our
+  // redirected pipes are still alive. Releasing the process first makes
+  // Reset(0) not attempt to kill the process, which is already a zombie at this
+  // point.
+  record->proc.Release();
+  record->proc.Reset(0);
+
   subprocess_records_.erase(pid_record);
 }
 
 pid_t Subprocess::Exec(const vector<string>& cmd,
                        const ExecCallback& callback) {
-  return ExecFlags(cmd, kRedirectStderrToStdout, callback);
+  return ExecFlags(cmd, kRedirectStderrToStdout, {}, callback);
 }
 
 pid_t Subprocess::ExecFlags(const vector<string>& cmd,
                             uint32_t flags,
+                            const vector<int>& output_pipes,
                             const ExecCallback& callback) {
   unique_ptr<SubprocessRecord> record(new SubprocessRecord(callback));
 
-  if (!LaunchProcess(cmd, flags, &record->proc)) {
+  if (!LaunchProcess(cmd, flags, output_pipes, &record->proc)) {
     LOG(ERROR) << "Failed to launch subprocess";
     return 0;
   }
@@ -215,6 +224,13 @@
   pid_record->second->proc.Release();
 }
 
+int Subprocess::GetPipeFd(pid_t pid, int fd) const {
+  auto pid_record = subprocess_records_.find(pid);
+  if (pid_record == subprocess_records_.end())
+    return -1;
+  return pid_record->second->proc.GetPipe(fd);
+}
+
 bool Subprocess::SynchronousExec(const vector<string>& cmd,
                                  int* return_code,
                                  string* stdout) {
@@ -232,7 +248,10 @@
                                       int* return_code,
                                       string* stdout) {
   brillo::ProcessImpl proc;
-  if (!LaunchProcess(cmd, flags, &proc)) {
+  // It doesn't make sense to redirect some pipes in the synchronous case
+  // because we won't be reading on our end, so we don't expose the output_pipes
+  // in this case.
+  if (!LaunchProcess(cmd, flags, {}, &proc)) {
     LOG(ERROR) << "Failed to launch subprocess";
     return false;
   }