diff options
author | Sen Jiang <senj@google.com> | 2019-01-08 18:34:17 -0800 |
---|---|---|
committer | android-build-merger <android-build-merger@google.com> | 2019-01-08 18:34:17 -0800 |
commit | e6218de3c64bb25bc74e42ce546d84f312134579 (patch) | |
tree | 20d5e8dedfac5e0e036c71f1747d4cadea596f8d | |
parent | 2b2d62f76e02b50be325afdffce6a93219435b2b (diff) | |
parent | 1193286ebe0b443972e68ff62d0b4fd960bb3412 (diff) | |
download | platform_external_libbrillo-e6218de3c64bb25bc74e42ce546d84f312134579.tar.gz platform_external_libbrillo-e6218de3c64bb25bc74e42ce546d84f312134579.tar.bz2 platform_external_libbrillo-e6218de3c64bb25bc74e42ce546d84f312134579.zip |
Process: allow closed target fd.
am: 1193286ebe
Change-Id: I95e7b2d991e6d0d149ea635fb004f8378082acda
-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); |