diff options
author | Sen Jiang <senj@google.com> | 2019-01-08 18:46:21 -0800 |
---|---|---|
committer | android-build-merger <android-build-merger@google.com> | 2019-01-08 18:46:21 -0800 |
commit | 493cfa063e2a7f1647f05c79a78454f03d6a72db (patch) | |
tree | 20d5e8dedfac5e0e036c71f1747d4cadea596f8d | |
parent | 2a757f43dd4c6346a9cefdaedc154d84e0976b16 (diff) | |
parent | ad083ea5b7530c163b376ea1bf96182abde70320 (diff) | |
download | platform_external_libbrillo-493cfa063e2a7f1647f05c79a78454f03d6a72db.tar.gz platform_external_libbrillo-493cfa063e2a7f1647f05c79a78454f03d6a72db.tar.bz2 platform_external_libbrillo-493cfa063e2a7f1647f05c79a78454f03d6a72db.zip |
Process: allow closed target fd. am: 1193286ebe am: e6218de3c6
am: ad083ea5b7
Change-Id: Id1fbfe6f69c08017d5c6f959ccd337a25ef8fe65
-rw-r--r-- | brillo/process.cc | 44 | ||||
-rw-r--r-- | brillo/process_unittest.cc | 23 |
2 files changed, 28 insertions, 39 deletions
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 @@ int ProcessImpl::GetPipe(int child_fd) { } 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 @@ bool ProcessImpl::Start() { 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 kBinStat[] = "/usr/bin/stat"; 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 @@ TEST_F(ProcessTest, BadExecutable) { } 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 @@ FilePath ProcessTest::GetFdPath(int fd) { } TEST_F(ProcessTest, RedirectStderrUsingPipe) { - std::string contents; process_.RedirectOutput(""); process_.AddArg(kBinSh); process_.AddArg("-c"); @@ -219,6 +217,7 @@ TEST_F(ProcessTest, RedirectStderrUsingPipe) { 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 @@ TEST_F(ProcessTest, RedirectStderrUsingPipeWhenPreviouslyClosed) { 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 @@ TEST_F(ProcessTest, RedirectStdoutUsingPipe) { 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); |