Commit 870912b8 authored by Rocky Liu's avatar Rocky Liu Committed by Dave Watson

Set O_CLOEXEC by default when creating pipes to avoid race conditions...

Set O_CLOEXEC by default when creating pipes to avoid race conditions resulting from concurrent Subprocess creations

Summary:
[folly::Subprocess] Set O_CLOEXEC by default when creating pipes to avoid race conditions resulting from concurrent Subprocess creations

If multiple threads are creating Subprocess objects concurrently, the
write side file descriptor of the pipe created in the parent process
might be inherited into other child processes unintentionally and never
closed, causing the parent process to hang while reading from the read
side of its pipe, thinking the other side must have been closed.

The fix to the problem is to create the pipes and set O_CLOEXEC in
a single pipe2 call. Then the child could clear the O_CLOEXEC flag
selectively before calling exec().

Test Plan:
Existing unit tests of Subprocess
Added a new unit test which will hang in Subprocess constructor without
this fix.

Reviewed By: tudorb@fb.com

FB internal diff: D1267396
parent db37af84
...@@ -248,24 +248,24 @@ void Subprocess::spawn( ...@@ -248,24 +248,24 @@ void Subprocess::spawn(
}); });
// Create a pipe to use to receive error information from the child, // Create a pipe to use to receive error information from the child,
// in case it fails before calling exec() // in case it fails before calling exec(), setting the close-on-exec flag
// on both sides of the pipe.
// This way the pipe will be closed automatically in the child if execve()
// succeeds. If the exec fails the child can write error information to the
// pipe.
// Note that O_CLOEXEC must be set in a single call while we are creating
// the pipe instead of doing pipe()/fcntl separately, which might race if a
// another thread calls fork()/exec() concurrently and both sides of the pipe
// may be inherited by the corresponding child process without being closed.
int errFds[2]; int errFds[2];
int r = ::pipe(errFds); int r = ::pipe2(errFds, O_CLOEXEC);
checkUnixError(r, "pipe"); checkUnixError(r, "pipe2");
SCOPE_EXIT { SCOPE_EXIT {
CHECK_ERR(::close(errFds[0])); CHECK_ERR(::close(errFds[0]));
if (errFds[1] >= 0) { if (errFds[1] >= 0) {
CHECK_ERR(::close(errFds[1])); CHECK_ERR(::close(errFds[1]));
} }
}; };
// Ask the child to close the read end of the error pipe.
options.fdActions_[errFds[0]] = CLOSE;
// Set the close-on-exec flag on the write side of the pipe.
// This way the pipe will be closed automatically in the child if execve()
// succeeds. If the exec fails the child can write error information to the
// pipe.
r = fcntl(errFds[1], F_SETFD, FD_CLOEXEC);
checkUnixError(r, "set FD_CLOEXEC");
// Perform the actual work of setting up pipes then forking and // Perform the actual work of setting up pipes then forking and
// executing the child. // executing the child.
...@@ -312,8 +312,14 @@ void Subprocess::spawnInternal( ...@@ -312,8 +312,14 @@ void Subprocess::spawnInternal(
for (auto& p : options.fdActions_) { for (auto& p : options.fdActions_) {
if (p.second == PIPE_IN || p.second == PIPE_OUT) { if (p.second == PIPE_IN || p.second == PIPE_OUT) {
int fds[2]; int fds[2];
r = ::pipe(fds); // Set O_CLOEXEC on both ends of the pipe atomically while creating
checkUnixError(r, "pipe"); // the pipe. The child will clear O_CLOEXEC on its side of the pipe
// before calling exec() so that stays open afterwards.
// This way even if a concurrently constructed Subprocess inherits
// both ends of this pipe, they will be automatically closed
// after the corresponding exec().
r = ::pipe2(fds, O_CLOEXEC);
checkUnixError(r, "pipe2");
PipeInfo pinfo; PipeInfo pinfo;
pinfo.direction = p.second; pinfo.direction = p.second;
int cfd; int cfd;
...@@ -422,9 +428,13 @@ int Subprocess::prepareChild(const Options& options, ...@@ -422,9 +428,13 @@ int Subprocess::prepareChild(const Options& options,
} }
} }
// Close parent's ends of all pipes
for (auto& p : pipes_) { for (auto& p : pipes_) {
r = ::close(p.parentFd); // Clear FD_CLOEXEC on the child side of the pipe so
// it stays open after exec() (so that the child could
// access it).
// See spawnInternal() for why FD_CLOEXEC must be set
// by default on pipes.
r = fcntl(p.childFd, F_SETFD, 0);
if (r == -1) { if (r == -1) {
return errno; return errno;
} }
......
...@@ -19,6 +19,7 @@ ...@@ -19,6 +19,7 @@
#include <unistd.h> #include <unistd.h>
#include <sys/types.h> #include <sys/types.h>
#include <dirent.h> #include <dirent.h>
#include <thread>
#include <boost/container/flat_set.hpp> #include <boost/container/flat_set.hpp>
#include <glog/logging.h> #include <glog/logging.h>
...@@ -32,6 +33,7 @@ ...@@ -32,6 +33,7 @@
#include "folly/gen/File.h" #include "folly/gen/File.h"
#include "folly/gen/String.h" #include "folly/gen/String.h"
#include "folly/experimental/io/FsUtil.h" #include "folly/experimental/io/FsUtil.h"
#include "folly/Memory.h"
using namespace folly; using namespace folly;
...@@ -436,3 +438,21 @@ TEST(CommunicateSubprocessTest, Chatty) { ...@@ -436,3 +438,21 @@ TEST(CommunicateSubprocessTest, Chatty) {
EXPECT_EQ(0, proc.wait().exitStatus()); EXPECT_EQ(0, proc.wait().exitStatus());
}); });
} }
TEST(ConcurrentSubprocessTest, construction) {
std::vector<std::unique_ptr<Subprocess>> ps(100);
auto action = [](std::unique_ptr<Subprocess>& p) {
p = make_unique<Subprocess>("read", Subprocess::pipeStdout());
};
std::vector<std::thread> threads;
for (auto& p : ps) {
threads.emplace_back(action, ref(p));
}
for (auto& t : threads) {
t.join();
}
for (auto& p : ps) {
p->terminate();
p->wait();
}
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment