Skip to content

Commit

Permalink
remove folly::Subprocess::CLOSE
Browse files Browse the repository at this point in the history
Summary:
As mentioned in D30901400 (b258ee3) / OSS commit b258ee3, `folly::Subprocess::CLOSE` has implications that are rather suboptimal in real-world use.

To recap, opening a socket or file will use the lowest unused fd number, meaning that if we close stdin/stdout/stderr they can be replaced by a random file or socket. This may result in unexpected behaviour.

Reviewed By: luciang

Differential Revision: D30991108

fbshipit-source-id: 9055af84423500a1896015b5fe866ceb2cc64186
  • Loading branch information
rahulg authored and facebook-github-bot committed Sep 22, 2021
1 parent ba00aa3 commit cc9032a
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 11 deletions.
10 changes: 3 additions & 7 deletions folly/Subprocess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -553,14 +553,10 @@ int Subprocess::prepareChild(
// as they all have the FD_CLOEXEC flag set and will be closed at
// exec time.

// Close all fds that we're supposed to close.
// Redirect requested FDs to /dev/null or NUL.
// Redirect requested FDs to /dev/null or NUL
// dup2 any explicitly specified FDs
for (auto& p : options.fdActions_) {
if (p.second == CLOSE) {
if (::close(p.first) == -1) {
return errno;
}
} else if (p.second == DEV_NULL) {
if (p.second == DEV_NULL) {
// folly/portability/Fcntl provides an impl of open that will
// map this to NUL on Windows.
auto devNull = ::open("/dev/null", O_RDWR | O_CLOEXEC);
Expand Down
3 changes: 1 addition & 2 deletions folly/Subprocess.h
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,7 @@ class FOLLY_EXPORT SubprocessSpawnError : public SubprocessError {
*/
class Subprocess {
public:
// CLOSE is deprecated and will be removed, consider using DEV_NULL instead
static const int CLOSE = -1;
// removed CLOSE = -1
static const int PIPE = -2;
static const int PIPE_IN = -3;
static const int PIPE_OUT = -4;
Expand Down
4 changes: 2 additions & 2 deletions folly/test/SingletonTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -762,8 +762,8 @@ TEST(Singleton, DoubleRegistrationLogging) {
auto p = Subprocess(
std::vector<std::string>{sub.string()},
Subprocess::Options()
.stdinFd(Subprocess::CLOSE)
.stdoutFd(Subprocess::CLOSE)
.stdinFd(Subprocess::DEV_NULL)
.stdoutFd(Subprocess::DEV_NULL)
.pipeStderr()
.closeOtherFds());
auto err = p.communicate("").second;
Expand Down

0 comments on commit cc9032a

Please sign in to comment.