X-Git-Url: http://plrg.eecs.uci.edu/git/?a=blobdiff_plain;f=folly%2FSubprocess.cpp;h=7ecc8de486634aa315187438808b9a358eb5393d;hb=186bd8ade9668acd931201b4913d3c2af1f9535f;hp=8de5d36979b8e28b1d3cf260cd66083cc9d8b6b3;hpb=8a5fcb4f368ca65e0e55ee6efa530403a0903adf;p=folly.git diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp index 8de5d369..7ecc8de4 100644 --- a/folly/Subprocess.cpp +++ b/folly/Subprocess.cpp @@ -1,5 +1,5 @@ /* - * Copyright 2014 Facebook, Inc. + * Copyright 2015 Facebook, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -50,6 +50,18 @@ constexpr int kChildFailure = 126; namespace folly { +ProcessReturnCode::ProcessReturnCode(ProcessReturnCode&& p) noexcept + : rawStatus_(p.rawStatus_) { + p.rawStatus_ = ProcessReturnCode::RV_NOT_STARTED; +} + +ProcessReturnCode& ProcessReturnCode::operator=(ProcessReturnCode&& p) + noexcept { + rawStatus_ = p.rawStatus_; + p.rawStatus_ = ProcessReturnCode::RV_NOT_STARTED; + return *this; +} + ProcessReturnCode::State ProcessReturnCode::state() const { if (rawStatus_ == RV_NOT_STARTED) return NOT_STARTED; if (rawStatus_ == RV_RUNNING) return RUNNING; @@ -96,6 +108,7 @@ std::string ProcessReturnCode::str() const { (coreDumped() ? " (core dumped)" : "")); } CHECK(false); // unreached + return ""; // silence GCC warning } CalledProcessError::CalledProcessError(ProcessReturnCode rc) @@ -189,13 +202,9 @@ Subprocess::Subprocess( Subprocess::~Subprocess() { CHECK_NE(returnCode_.state(), ProcessReturnCode::RUNNING) << "Subprocess destroyed without reaping child"; - closeAll(); } namespace { -void closeChecked(int fd) { - checkUnixError(::close(fd), "close"); -} struct ChildErrorInfo { int errCode; @@ -214,16 +223,9 @@ void childError(int errFd, int errCode, int errnoValue) { } // namespace -void Subprocess::closeAll() { - for (auto& p : pipes_) { - closeChecked(p.parentFd); - } - pipes_.clear(); -} - void Subprocess::setAllNonBlocking() { for (auto& p : pipes_) { - int fd = p.parentFd; + int fd = p.pipe.fd(); int flags = ::fcntl(fd, F_GETFL); checkUnixError(flags, "fcntl"); int r = ::fcntl(fd, F_SETFL, flags | O_NONBLOCK); @@ -244,12 +246,8 @@ void Subprocess::spawn( // Make a copy, we'll mutate options Options options(optionsIn); - // On error, close all of the pipes_ - auto pipesGuard = makeGuard([&] { - for (auto& p : this->pipes_) { - CHECK_ERR(::close(p.parentFd)); - } - }); + // On error, close all pipes_ (ignoring errors, but that seems fine here). + auto pipesGuard = makeGuard([this] { pipes_.clear(); }); // Create a pipe to use to receive error information from the child, // in case it fails before calling exec() @@ -289,14 +287,6 @@ void Subprocess::spawn( // child has exited and can be immediately waited for. In all other cases, // we have no way of cleaning up the child. - if (options.processGroupLeader_) { - // This is done both in the parent and the child to avoid the race where - // the parent assumes that the child is a leader, but the child has not - // yet run setprp(). Not checking error codes since we're deliberately - // racing the child, which may already have run execve(), and expect to - // lose frequently. - setpgid(pid_, pid_); - } // Close writable side of the errFd pipe in the parent process CHECK_ERR(::close(errFds[1])); errFds[1] = -1; @@ -333,6 +323,9 @@ void Subprocess::spawnInternal( // doesn't need to reset the flag on its end, as we always dup2() the fd, // and dup2() fds don't share the close-on-exec flag. #if FOLLY_HAVE_PIPE2 + // If possible, set close-on-exec atomically. Otherwise, a concurrent + // Subprocess invocation can fork() between "pipe" and "fnctl", + // causing FDs to leak. r = ::pipe2(fds, O_CLOEXEC); checkUnixError(r, "pipe2"); #else @@ -343,21 +336,21 @@ void Subprocess::spawnInternal( r = fcntl(fds[1], F_SETFD, FD_CLOEXEC); checkUnixError(r, "set FD_CLOEXEC"); #endif - PipeInfo pinfo; - pinfo.direction = p.second; + pipes_.emplace_back(); + Pipe& pipe = pipes_.back(); + pipe.direction = p.second; int cfd; if (p.second == PIPE_IN) { // Child gets reading end - pinfo.parentFd = fds[1]; + pipe.pipe = folly::File(fds[1], /*owns_fd=*/ true); cfd = fds[0]; } else { - pinfo.parentFd = fds[0]; + pipe.pipe = folly::File(fds[0], /*owns_fd=*/ true); cfd = fds[1]; } p.second = cfd; // ensure it gets dup2()ed - pinfo.childFd = p.first; + pipe.childFd = p.first; childFds.push_back(cfd); - pipes_.push_back(pinfo); } } @@ -551,6 +544,8 @@ ProcessReturnCode Subprocess::poll() { pid_t found = ::waitpid(pid_, &status, WNOHANG); checkUnixError(found, "waitpid"); if (found != 0) { + // Though the child process had quit, this call does not close the pipes + // since its descendants may still be using them. returnCode_ = ProcessReturnCode(status); pid_ = -1; } @@ -574,6 +569,8 @@ ProcessReturnCode Subprocess::wait() { found = ::waitpid(pid_, &status, 0); } while (found == -1 && errno == EINTR); checkUnixError(found, "waitpid"); + // Though the child process had quit, this call does not close the pipes + // since its descendants may still be using them. DCHECK_EQ(found, pid_); returnCode_ = ProcessReturnCode(status); pid_ = -1; @@ -717,12 +714,14 @@ std::pair Subprocess::communicateIOBuf( void Subprocess::communicate(FdCallback readCallback, FdCallback writeCallback) { + // This serves to prevent wait() followed by communicate(), but if you + // legitimately need that, send a patch to delete this line. returnCode_.enforce(ProcessReturnCode::RUNNING); setAllNonBlocking(); std::vector fds; fds.reserve(pipes_.size()); - std::vector toClose; + std::vector toClose; // indexes into pipes_ toClose.reserve(pipes_.size()); while (!pipes_.empty()) { @@ -731,7 +730,7 @@ void Subprocess::communicate(FdCallback readCallback, for (auto& p : pipes_) { pollfd pfd; - pfd.fd = p.parentFd; + pfd.fd = p.pipe.fd(); // Yes, backwards, PIPE_IN / PIPE_OUT are defined from the // child's point of view. if (!p.enabled) { @@ -754,13 +753,14 @@ void Subprocess::communicate(FdCallback readCallback, for (size_t i = 0; i < pipes_.size(); ++i) { auto& p = pipes_[i]; - DCHECK_EQ(fds[i].fd, p.parentFd); + auto parentFd = p.pipe.fd(); + DCHECK_EQ(fds[i].fd, parentFd); short events = fds[i].revents; bool closed = false; if (events & POLLOUT) { DCHECK(!(events & POLLIN)); - if (writeCallback(p.parentFd, p.childFd)) { + if (writeCallback(parentFd, p.childFd)) { toClose.push_back(i); closed = true; } @@ -770,7 +770,7 @@ void Subprocess::communicate(FdCallback readCallback, // on) end of file if (events & (POLLIN | POLLHUP)) { DCHECK(!(events & POLLOUT)); - if (readCallback(p.parentFd, p.childFd)) { + if (readCallback(parentFd, p.childFd)) { toClose.push_back(i); closed = true; } @@ -785,7 +785,7 @@ void Subprocess::communicate(FdCallback readCallback, // Close the fds in reverse order so the indexes hold after erase() for (int idx : boost::adaptors::reverse(toClose)) { auto pos = pipes_.begin() + idx; - closeChecked(pos->parentFd); + pos->pipe.close(); // Throws on error pipes_.erase(pos); } } @@ -799,10 +799,10 @@ bool Subprocess::notificationsEnabled(int childFd) const { return pipes_[findByChildFd(childFd)].enabled; } -int Subprocess::findByChildFd(int childFd) const { +size_t Subprocess::findByChildFd(int childFd) const { auto pos = std::lower_bound( pipes_.begin(), pipes_.end(), childFd, - [] (const PipeInfo& info, int fd) { return info.childFd < fd; }); + [] (const Pipe& pipe, int fd) { return pipe.childFd < fd; }); if (pos == pipes_.end() || pos->childFd != childFd) { throw std::invalid_argument(folly::to( "child fd not found ", childFd)); @@ -812,10 +812,19 @@ int Subprocess::findByChildFd(int childFd) const { void Subprocess::closeParentFd(int childFd) { int idx = findByChildFd(childFd); - closeChecked(pipes_[idx].parentFd); + pipes_[idx].pipe.close(); // May throw pipes_.erase(pipes_.begin() + idx); } +std::vector Subprocess::takeOwnershipOfPipes() { + std::vector pipes; + for (auto& p : pipes_) { + pipes.emplace_back(p.childFd, std::move(p.pipe)); + } + pipes_.clear(); + return pipes; +} + namespace { class Initializer {