Process: allow closed target fd.
The comment suggest that the fstat() check is to simplify the logic to
dup2, this patch switch to use base::ShuffleFileDescriptors() to handle
that logic so that we can allow closed fd.
Bug: 121177337
Test: unit tests
Change-Id: I9fdb0cd00fb371ebcd90a3d6e91a353159ab8413
diff --git a/brillo/process.cc b/brillo/process.cc
index 8773244..ead6f20 100644
--- a/brillo/process.cc
+++ b/brillo/process.cc
@@ -20,6 +20,7 @@
#include <base/logging.h>
#include <base/memory/ptr_util.h>
#include <base/posix/eintr_wrapper.h>
+#include <base/posix/file_descriptor_shuffle.h>
#include <base/process/process_metrics.h>
#include <base/strings/string_number_conversions.h>
#include <base/strings/string_util.h>
@@ -141,20 +142,6 @@
}
bool ProcessImpl::PopulatePipeMap() {
- // Verify all target fds are already open. With this assumption we
- // can be sure that the pipe fds created below do not overlap with
- // any of the target fds which simplifies how we dup2 to them. Note
- // that multi-threaded code could close i->first between this loop
- // and the next.
- for (PipeMap::iterator i = pipe_map_.begin(); i != pipe_map_.end(); ++i) {
- struct stat stat_buffer;
- if (fstat(i->first, &stat_buffer) < 0) {
- int saved_errno = errno;
- LOG(ERROR) << "Unable to fstat fd " << i->first << ": " << saved_errno;
- return false;
- }
- }
-
for (PipeMap::iterator i = pipe_map_.begin(); i != pipe_map_.end(); ++i) {
if (i->second.is_bound_) {
// already have a parent fd, and the child fd gets dup()ed later.
@@ -243,24 +230,19 @@
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) {
- if (i->second.parent_fd_ != -1)
- IGNORE_EINTR(close(i->second.parent_fd_));
- // If we want to bind a fd to the same fd in the child, we don't need to
- // close and dup2 it.
- if (i->second.child_fd_ == i->first)
- continue;
- HANDLE_EINTR(dup2(i->second.child_fd_, i->first));
+
+ base::InjectiveMultimap fd_shuffle;
+ for (const auto& it : pipe_map_) {
+ // Close parent's side of the child pipes.
+ if (it.second.parent_fd_ != -1)
+ IGNORE_EINTR(close(it.second.parent_fd_));
+
+ fd_shuffle.emplace_back(it.second.child_fd_, it.first, true);
}
- // Defer the actual close() of the child fd until afterward; this lets the
- // same child fd be bound to multiple fds using BindFd. Don't close the fd
- // if it was bound to itself.
- for (PipeMap::iterator i = pipe_map_.begin(); i != pipe_map_.end(); ++i) {
- if (i->second.child_fd_ == i->first)
- continue;
- IGNORE_EINTR(close(i->second.child_fd_));
+
+ if (!base::ShuffleFileDescriptors(&fd_shuffle)) {
+ PLOG(ERROR) << "Could not shuffle file descriptors";
+ _exit(kErrorExitStatus);
}
if (!input_file_.empty()) {
diff --git a/brillo/process_unittest.cc b/brillo/process_unittest.cc
index d980e2b..f65cf34 100644
--- a/brillo/process_unittest.cc
+++ b/brillo/process_unittest.cc
@@ -28,7 +28,6 @@
static const char kBinSh[] = SYSTEM_PREFIX "/bin/sh";
static const char kBinCat[] = SYSTEM_PREFIX "/bin/cat";
-static const char kBinCp[] = SYSTEM_PREFIX "/bin/cp";
static const char kBinEcho[] = SYSTEM_PREFIX "/bin/echo";
static const char kBinFalse[] = SYSTEM_PREFIX "/bin/false";
static const char kBinSleep[] = SYSTEM_PREFIX "/bin/sleep";
@@ -181,11 +180,11 @@
}
void ProcessTest::CheckStderrCaptured() {
- std::string contents;
process_.AddArg(kBinSh);
process_.AddArg("-c");
process_.AddArg("echo errormessage 1>&2 && exit 1");
EXPECT_EQ(1, process_.Run());
+ std::string contents;
EXPECT_TRUE(base::ReadFileToString(FilePath(output_file_), &contents));
EXPECT_NE(std::string::npos, contents.find("errormessage"));
EXPECT_EQ("", GetLog());
@@ -207,7 +206,6 @@
}
TEST_F(ProcessTest, RedirectStderrUsingPipe) {
- std::string contents;
process_.RedirectOutput("");
process_.AddArg(kBinSh);
process_.AddArg("-c");
@@ -219,6 +217,7 @@
EXPECT_GE(pipe_fd, 0);
EXPECT_EQ(-1, process_.GetPipe(STDOUT_FILENO));
EXPECT_EQ(-1, process_.GetPipe(STDIN_FILENO));
+ std::string contents;
EXPECT_TRUE(base::ReadFileToString(GetFdPath(pipe_fd), &contents));
EXPECT_NE(std::string::npos, contents.find("errormessage"));
EXPECT_EQ("", GetLog());
@@ -228,15 +227,23 @@
int saved_stderr = dup(STDERR_FILENO);
close(STDERR_FILENO);
process_.RedirectOutput("");
- process_.AddArg(kBinCp);
+ process_.AddArg(kBinSh);
+ process_.AddArg("-c");
+ process_.AddArg("echo errormessage >&2 && exit 1");
process_.RedirectUsingPipe(STDERR_FILENO, false);
- EXPECT_FALSE(process_.Start());
- EXPECT_TRUE(FindLog("Unable to fstat fd 2:"));
+ EXPECT_EQ(1, process_.Run());
+ int pipe_fd = process_.GetPipe(STDERR_FILENO);
+ EXPECT_GE(pipe_fd, 0);
+ EXPECT_EQ(-1, process_.GetPipe(STDOUT_FILENO));
+ EXPECT_EQ(-1, process_.GetPipe(STDIN_FILENO));
+ std::string contents;
+ EXPECT_TRUE(base::ReadFileToString(GetFdPath(pipe_fd), &contents));
+ EXPECT_NE(std::string::npos, contents.find("errormessage"));
+ EXPECT_EQ("", GetLog());
dup2(saved_stderr, STDERR_FILENO);
}
TEST_F(ProcessTest, RedirectStdoutUsingPipe) {
- std::string contents;
process_.RedirectOutput("");
process_.AddArg(kBinEcho);
process_.AddArg("hello world\n");
@@ -247,13 +254,13 @@
EXPECT_GE(pipe_fd, 0);
EXPECT_EQ(-1, process_.GetPipe(STDERR_FILENO));
EXPECT_EQ(-1, process_.GetPipe(STDIN_FILENO));
+ std::string contents;
EXPECT_TRUE(base::ReadFileToString(GetFdPath(pipe_fd), &contents));
EXPECT_NE(std::string::npos, contents.find("hello world\n"));
EXPECT_EQ("", GetLog());
}
TEST_F(ProcessTest, RedirectStdinUsingPipe) {
- std::string contents;
const char kMessage[] = "made it!\n";
process_.AddArg(kBinCat);
process_.RedirectUsingPipe(STDIN_FILENO, true);
