From 81a0e5974084b1b6fbfb6d6440a83e741cea3be4 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Tue, 10 Mar 2015 15:17:15 +0100 Subject: [PATCH] [master] Fixed races in ProcessSpawn util (#3733) --- src/lib/util/process_spawn.cc | 88 ++++++++++++++------ src/lib/util/process_spawn.h | 14 +++- src/lib/util/tests/process_spawn_unittest.cc | 4 +- 3 files changed, 76 insertions(+), 30 deletions(-) diff --git a/src/lib/util/process_spawn.cc b/src/lib/util/process_spawn.cc index e6eb5c1129..34e8dba193 100644 --- a/src/lib/util/process_spawn.cc +++ b/src/lib/util/process_spawn.cc @@ -19,11 +19,28 @@ #include #include #include +#include #include namespace isc { namespace util { +/// @brief Type for process state +struct ProcessState { + + /// @brief Constructor + ProcessState() : running_(true), status_(0) { + } + + /// @brief true until the exit status is collected + bool running_; + + /// @brief 0 or the exit status + int status_; +}; + +typedef std::map ProcessStates; + /// @brief Implementation of the @c ProcessSpawn class. /// /// This pimpl idiom is used by the @c ProcessSpawn in this case to @@ -95,7 +112,7 @@ public: /// exception is thrown. /// /// @param pid A process pid. - void clearStatus(const pid_t pid); + void clearState(const pid_t pid); private: @@ -125,7 +142,7 @@ private: SignalSetPtr signals_; /// @brief A map holding the status codes of executed processes. - std::map process_status_; + ProcessStates process_state_; /// @brief Path to an executable. std::string executable_; @@ -136,7 +153,7 @@ private: ProcessSpawnImpl::ProcessSpawnImpl(const std::string& executable, const ProcessArgs& args) - : signals_(new SignalSet(SIGCHLD)), process_status_(), + : signals_(new SignalSet(SIGCHLD)), process_state_(), executable_(executable), args_(new char*[args.size() + 2]) { // Set the handler which is invoked immediately when the signal // is received. @@ -154,7 +171,6 @@ ProcessSpawnImpl::ProcessSpawnImpl(const std::string& executable, } } - ProcessSpawnImpl::~ProcessSpawnImpl() { int i = 0; // Deallocate strings in the array of arguments. @@ -183,12 +199,20 @@ ProcessSpawnImpl::getCommandLine() const { pid_t ProcessSpawnImpl::spawn() { + // Protect us against SIGCHLD signals + sigset_t sset; + sigemptyset(&sset); + sigaddset(&sset, SIGCHLD); + sigprocmask(SIG_BLOCK, &sset, 0); + // Create the child pid_t pid = fork(); if (pid < 0) { isc_throw(ProcessSpawnError, "unable to fork current process"); } else if (pid == 0) { - // We're in the child process. Run the executable. + // We're in the child process. + sigprocmask(SIG_UNBLOCK, &sset, 0); + // Run the executable. if (execvp(executable_.c_str(), args_) != 0) { // We may end up here if the execvp failed, e.g. as a result // of issue with permissions or invalid executable name. @@ -198,25 +222,28 @@ ProcessSpawnImpl::spawn() { exit(EXIT_SUCCESS); } - process_status_[pid] = 0; + // We're in the parent process. + process_state_.insert(std::pair(pid, ProcessState())); + sigprocmask(SIG_UNBLOCK, &sset, 0); return (pid); } bool ProcessSpawnImpl::isRunning(const pid_t pid) const { - if (process_status_.find(pid) == process_status_.end()) { + ProcessStates::const_iterator proc = process_state_.find(pid); + if (proc == process_state_.end()) { isc_throw(BadValue, "the process with the pid '" << pid << "' hasn't been spawned and it status cannot be" " returned"); } - return ((pid != 0) && (kill(pid, 0) == 0)); + return (proc->second.running_); } bool ProcessSpawnImpl::isAnyRunning() const { - for (std::map::const_iterator proc = process_status_.begin(); - proc != process_status_.end(); ++proc) { - if (isRunning(proc->first)) { + for (ProcessStates::const_iterator proc = process_state_.begin(); + proc != process_state_.end(); ++proc) { + if (proc->second.running_) { return (true); } } @@ -225,13 +252,13 @@ ProcessSpawnImpl::isAnyRunning() const { int ProcessSpawnImpl::getExitStatus(const pid_t pid) const { - std::map::const_iterator status = process_status_.find(pid); - if (status == process_status_.end()) { + ProcessStates::const_iterator proc = process_state_.find(pid); + if (proc == process_state_.end()) { isc_throw(InvalidOperation, "the process with the pid '" << pid << "' hasn't been spawned and it status cannot be" " returned"); } - return (WEXITSTATUS(status->second)); + return (WEXITSTATUS(proc->second.status_)); } char* @@ -249,27 +276,34 @@ ProcessSpawnImpl::allocateArg(const std::string& src) const { bool ProcessSpawnImpl::waitForProcess(int signum) { // We're only interested in SIGCHLD. - if (signum == SIGCHLD) { + if (signum != SIGCHLD) { + return (false); + } + for (;;) { int status = 0; - pid_t pid = waitpid(-1, &status, 0); - if (pid > 0) { - /// @todo Check that the terminating process was started - /// by our instance of ProcessSpawn and only handle it - /// if it was. - process_status_[pid] = status; + pid_t pid = waitpid(-1, &status, WNOHANG); + if (pid <= 0) { + break; + } + ProcessStates::iterator proc = process_state_.find(pid); + /// Check that the terminating process was started + /// by our instance of ProcessSpawn + if (proc != process_state_.end()) { + // In this order please + proc->second.status_ = status; + proc->second.running_ = false; } - return (true); } - return (false); + return (true); } void -ProcessSpawnImpl::clearStatus(const pid_t pid) { +ProcessSpawnImpl::clearState(const pid_t pid) { if (isRunning(pid)) { isc_throw(InvalidOperation, "unable to remove the status for the" "process (pid: " << pid << ") which is still running"); } - process_status_.erase(pid); + process_state_.erase(pid); } ProcessSpawn::ProcessSpawn(const std::string& executable, @@ -307,8 +341,8 @@ ProcessSpawn::getExitStatus(const pid_t pid) const { } void -ProcessSpawn::clearStatus(const pid_t pid) { - return (impl_->clearStatus(pid)); +ProcessSpawn::clearState(const pid_t pid) { + return (impl_->clearState(pid)); } } diff --git a/src/lib/util/process_spawn.h b/src/lib/util/process_spawn.h index aa43e56362..e9647db905 100644 --- a/src/lib/util/process_spawn.h +++ b/src/lib/util/process_spawn.h @@ -96,6 +96,10 @@ public: /// @brief Checks if the process is still running. /// + /// Note that only a negative (false) result is reliable as the child + /// process can exit between the time its state is checked and this + /// function returns. + /// /// @param pid ID of the child processes for which state should be checked. /// /// @return true if the child process is running, false otherwise. @@ -111,6 +115,9 @@ public: /// If the process is still running, the previous status is returned /// or 0, if the process is being ran for the first time. /// + /// @note @c ProcessSpawn::isRunning should be called and have returned + /// false before using @c ProcessSpawn::getExitStatus. + /// /// @param pid ID of the child process for which exit status should be /// returned. /// @@ -123,8 +130,13 @@ public: /// If the process is still running, the status is not removed and the /// exception is thrown. /// + /// Note @c ProcessSpawn::isRunning must be called and have returned + /// false before using clearState(). And of course + /// @c ProcessSpawn::getExitStatus should be called first, if there is + /// some interest in the status. + /// /// @param pid A process pid. - void clearStatus(const pid_t pid); + void clearState(const pid_t pid); private: diff --git a/src/lib/util/tests/process_spawn_unittest.cc b/src/lib/util/tests/process_spawn_unittest.cc index a4650c3f6c..50e217d888 100644 --- a/src/lib/util/tests/process_spawn_unittest.cc +++ b/src/lib/util/tests/process_spawn_unittest.cc @@ -90,11 +90,11 @@ TEST(ProcessSpawn, spawnTwoProcesses) { // Clear the status of the first process. An attempt to get the status // for the cleared process should result in exception. But, there should // be no exception for the second process. - process.clearStatus(pid1); + process.clearState(pid1); EXPECT_THROW(process.getExitStatus(pid1), InvalidOperation); EXPECT_NO_THROW(process.getExitStatus(pid2)); - process.clearStatus(pid2); + process.clearState(pid2); EXPECT_THROW(process.getExitStatus(pid2), InvalidOperation); } -- 2.47.2