Commit cc9032a0 authored by Rahul Arunapuram Gokul's avatar Rahul Arunapuram Gokul Committed by Facebook GitHub Bot

remove folly::Subprocess::CLOSE

Summary:
As mentioned in D30901400 (https://github.com/facebook/folly/commit/b258ee31860b47b9196872264f6f3a331d450b1f) / OSS commit b258ee31, `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
parent ba00aa3c
...@@ -553,14 +553,10 @@ int Subprocess::prepareChild( ...@@ -553,14 +553,10 @@ int Subprocess::prepareChild(
// as they all have the FD_CLOEXEC flag set and will be closed at // as they all have the FD_CLOEXEC flag set and will be closed at
// exec time. // 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_) { for (auto& p : options.fdActions_) {
if (p.second == CLOSE) { if (p.second == DEV_NULL) {
if (::close(p.first) == -1) {
return errno;
}
} else if (p.second == DEV_NULL) {
// folly/portability/Fcntl provides an impl of open that will // folly/portability/Fcntl provides an impl of open that will
// map this to NUL on Windows. // map this to NUL on Windows.
auto devNull = ::open("/dev/null", O_RDWR | O_CLOEXEC); auto devNull = ::open("/dev/null", O_RDWR | O_CLOEXEC);
......
...@@ -253,8 +253,7 @@ class FOLLY_EXPORT SubprocessSpawnError : public SubprocessError { ...@@ -253,8 +253,7 @@ class FOLLY_EXPORT SubprocessSpawnError : public SubprocessError {
*/ */
class Subprocess { class Subprocess {
public: public:
// CLOSE is deprecated and will be removed, consider using DEV_NULL instead // removed CLOSE = -1
static const int CLOSE = -1;
static const int PIPE = -2; static const int PIPE = -2;
static const int PIPE_IN = -3; static const int PIPE_IN = -3;
static const int PIPE_OUT = -4; static const int PIPE_OUT = -4;
......
...@@ -762,8 +762,8 @@ TEST(Singleton, DoubleRegistrationLogging) { ...@@ -762,8 +762,8 @@ TEST(Singleton, DoubleRegistrationLogging) {
auto p = Subprocess( auto p = Subprocess(
std::vector<std::string>{sub.string()}, std::vector<std::string>{sub.string()},
Subprocess::Options() Subprocess::Options()
.stdinFd(Subprocess::CLOSE) .stdinFd(Subprocess::DEV_NULL)
.stdoutFd(Subprocess::CLOSE) .stdoutFd(Subprocess::DEV_NULL)
.pipeStderr() .pipeStderr()
.closeOtherFds()); .closeOtherFds());
auto err = p.communicate("").second; auto err = p.communicate("").second;
......
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