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
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;
}
diff --git a/common/subprocess.h b/common/subprocess.h
index 6b952dc..b655fb7 100644
--- a/common/subprocess.h
+++ b/common/subprocess.h
@@ -64,15 +64,26 @@
void Init(brillo::AsynchronousSignalHandlerInterface* async_signal_handler);
// Launches a process in the background and calls the passed |callback| when
- // the process exits.
+ // the process exits. The file descriptors specified in |output_pipes| will
+ // be available in the child as the writer end of a pipe. Use GetPipeFd() to
+ // know the reader end in the parent. Only stdin, stdout, stderr and the file
+ // descriptors in |output_pipes| will be open in the child.
// Returns the process id of the new launched process or 0 in case of failure.
pid_t Exec(const std::vector<std::string>& cmd, const ExecCallback& callback);
pid_t ExecFlags(const std::vector<std::string>& cmd,
uint32_t flags,
+ const std::vector<int>& output_pipes,
const ExecCallback& callback);
// Kills the running process with SIGTERM and ignores the callback.
- void KillExec(pid_t tag);
+ void KillExec(pid_t pid);
+
+ // Return the parent end of the pipe mapped onto |fd| in the child |pid|. This
+ // file descriptor is available until the callback for the child |pid|
+ // returns. After that the file descriptor will be closed. The passed |fd|
+ // must be one of the file descriptors passed to ExecFlags() in
+ // |output_pipes|, otherwise returns -1.
+ int GetPipeFd(pid_t pid, int fd) const;
// Executes a command synchronously. Returns true on success. If |stdout| is
// non-null, the process output is stored in it, otherwise the output is
diff --git a/common/subprocess_unittest.cc b/common/subprocess_unittest.cc
index c6cbab0..5ca44e8 100644
--- a/common/subprocess_unittest.cc
+++ b/common/subprocess_unittest.cc
@@ -37,6 +37,7 @@
#include <brillo/message_loops/message_loop.h>
#include <brillo/message_loops/message_loop_utils.h>
#include <brillo/strings/string_utils.h>
+#include <brillo/unittest_utils.h>
#include <gtest/gtest.h>
#include "update_engine/common/test_utils.h"
@@ -95,6 +96,27 @@
MessageLoop::current()->BreakLoop();
}
+void ExpectedDataOnPipe(const Subprocess* subprocess,
+ pid_t* pid,
+ int child_fd,
+ const string& child_fd_data,
+ int expected_return_code,
+ int return_code,
+ const string& /* output */) {
+ EXPECT_EQ(expected_return_code, return_code);
+
+ // Verify that we can read the data from our end of |child_fd|.
+ int fd = subprocess->GetPipeFd(*pid, child_fd);
+ EXPECT_NE(-1, fd);
+ vector<char> buf(child_fd_data.size() + 1);
+ EXPECT_EQ(static_cast<ssize_t>(child_fd_data.size()),
+ HANDLE_EINTR(read(fd, buf.data(), buf.size())));
+ EXPECT_EQ(child_fd_data,
+ string(buf.begin(), buf.begin() + child_fd_data.size()));
+
+ MessageLoop::current()->BreakLoop();
+}
+
} // namespace
TEST_F(SubprocessTest, IsASingleton) {
@@ -125,10 +147,40 @@
EXPECT_TRUE(subprocess_.ExecFlags(
{kBinPath "/sh", "-c", "echo on stdout; echo on stderr >&2"},
0,
+ {},
base::Bind(&ExpectedResults, 0, "on stdout\n")));
loop_.Run();
}
+TEST_F(SubprocessTest, PipeRedirectFdTest) {
+ pid_t pid;
+ pid = subprocess_.ExecFlags(
+ {kBinPath "/sh", "-c", "echo on pipe >&3"},
+ 0,
+ {3},
+ base::Bind(&ExpectedDataOnPipe, &subprocess_, &pid, 3, "on pipe\n", 0));
+ EXPECT_NE(0, pid);
+
+ // Wrong file descriptor values should return -1.
+ EXPECT_EQ(-1, subprocess_.GetPipeFd(pid, 123));
+ loop_.Run();
+ // Calling GetPipeFd() after the callback runs is invalid.
+ EXPECT_EQ(-1, subprocess_.GetPipeFd(pid, 3));
+}
+
+// Test that a pipe file descriptor open in the parent is not open in the child.
+TEST_F(SubprocessTest, PipeClosedWhenNotRedirectedTest) {
+ brillo::ScopedPipe pipe;
+ const vector<string> cmd = {kBinPath "/sh", "-c",
+ base::StringPrintf("echo on pipe >/proc/self/fd/%d", pipe.writer)};
+ EXPECT_TRUE(subprocess_.ExecFlags(
+ cmd,
+ 0,
+ {},
+ base::Bind(&ExpectedResults, 1, "")));
+ loop_.Run();
+}
+
TEST_F(SubprocessTest, EnvVarsAreFiltered) {
EXPECT_TRUE(
subprocess_.Exec({kUsrBinPath "/env"}, base::Bind(&ExpectedEnvVars)));
diff --git a/p2p_manager.cc b/p2p_manager.cc
index 734918d..127e5ff 100644
--- a/p2p_manager.cc
+++ b/p2p_manager.cc
@@ -396,7 +396,7 @@
// We expect to run just "p2p-client" and find it in the path.
child_pid_ = Subprocess::Get().ExecFlags(
- cmd, Subprocess::kSearchPath,
+ cmd, Subprocess::kSearchPath, {},
Bind(&LookupData::OnLookupDone, base::Unretained(this)));
if (!child_pid_) {