Commit 1c24c1b4 authored by Shaun Ruffell's avatar Shaun Ruffell Committed by Facebook GitHub Bot

Only close currently open descriptors on Linux

Summary:
folly::Subprocess attempts to close (nearly) all file descriptors if
`closeOtherFds()` option is set. When the process limit for number of
file descriptors is large, but the actual number of open descriptors is low,
much time is spent calling `close()` on files that are not actually open.

On Linux, we can use the information in `/proc/self/fd` to close only those
files that are actually open to speed up the process in the normal case.

Reviewed By: yfeldblum

Differential Revision: D33128499

fbshipit-source-id: efa5a348ff657c8212d9329a3efc7dd5bc0bb4d9
parent 8ef03f99
...@@ -41,6 +41,7 @@ ...@@ -41,6 +41,7 @@
#include <folly/io/Cursor.h> #include <folly/io/Cursor.h>
#include <folly/lang/Assume.h> #include <folly/lang/Assume.h>
#include <folly/logging/xlog.h> #include <folly/logging/xlog.h>
#include <folly/portability/Dirent.h>
#include <folly/portability/Fcntl.h> #include <folly/portability/Fcntl.h>
#include <folly/portability/Sockets.h> #include <folly/portability/Sockets.h>
#include <folly/portability/Stdlib.h> #include <folly/portability/Stdlib.h>
...@@ -516,6 +517,67 @@ void Subprocess::spawnInternal( ...@@ -516,6 +517,67 @@ void Subprocess::spawnInternal(
} }
FOLLY_POP_WARNING FOLLY_POP_WARNING
// If requested, close all other file descriptors. Don't close
// any fds in options.fdActions_, and don't touch stdin, stdout, stderr.
// Ignore errors.
//
//
// This function is called in the child after fork but before exec so
// there is very little it can do. It cannot allocate memory and
// it cannot lock a mutex, just as if it were running in a signal
// handler.
void Subprocess::closeInheritedFds(const Options::FdMap& fdActions) {
#if defined(__linux__)
int dirfd = open("/proc/self/fd", O_RDONLY);
if (dirfd != -1) {
char buffer[32768];
int res;
while ((res = syscall(SYS_getdents64, dirfd, buffer, sizeof(buffer))) > 0) {
// linux_dirent64 is part of the kernel ABI for the getdents64 system
// call. It is currently the same as struct dirent64 in both glibc and
// musl, but those are library specific and could change. linux_dirent64
// is not defined in the standard set of Linux userspace headers
// (/usr/include/linux)
//
// We do not use the POSIX interfaces (opendir, readdir, etc..) for
// reading a directory since they may allocate memory / grab a lock, which
// is unsafe in this context.
struct linux_dirent64 {
uint64_t d_ino;
int64_t d_off;
uint16_t d_reclen;
unsigned char d_type;
char d_name[0];
} const* entry;
for (int offset = 0; offset < res; offset += entry->d_reclen) {
entry = reinterpret_cast<struct linux_dirent64*>(buffer + offset);
if (entry->d_type != DT_LNK) {
continue;
}
char* end_p = nullptr;
errno = 0;
int fd = static_cast<int>(::strtol(entry->d_name, &end_p, 10));
if (errno == ERANGE || fd < 3 || end_p == entry->d_name) {
continue;
}
if ((fd != dirfd) && (fdActions.count(fd) == 0)) {
::close(fd);
}
}
}
::close(dirfd);
return;
}
#endif
// If not running on Linux or if we failed to open /proc/self/fd, try to close
// all possible open file descriptors.
for (int fd = sysconf(_SC_OPEN_MAX) - 1; fd >= 3; --fd) {
if (fdActions.count(fd) == 0) {
::close(fd);
}
}
}
int Subprocess::prepareChild( int Subprocess::prepareChild(
const Options& options, const Options& options,
const sigset_t* sigmask, const sigset_t* sigmask,
...@@ -577,15 +639,8 @@ int Subprocess::prepareChild( ...@@ -577,15 +639,8 @@ int Subprocess::prepareChild(
} }
} }
// If requested, close all other file descriptors. Don't close
// any fds in options.fdActions_, and don't touch stdin, stdout, stderr.
// Ignore errors.
if (options.closeOtherFds_) { if (options.closeOtherFds_) {
for (int fd = sysconf(_SC_OPEN_MAX) - 1; fd >= 3; --fd) { closeInheritedFds(options.fdActions_);
if (options.fdActions_.count(fd) == 0) {
::close(fd);
}
}
} }
#if defined(__linux__) #if defined(__linux__)
......
...@@ -954,6 +954,9 @@ class Subprocess { ...@@ -954,6 +954,9 @@ class Subprocess {
char** env, char** env,
const Options& options) const; const Options& options) const;
// Closes fds inherited from parent in child process
static void closeInheritedFds(const Options::FdMap& fdActions);
/** /**
* Read from the error pipe, and throw SubprocessSpawnError if the child * Read from the error pipe, and throw SubprocessSpawnError if the child
* failed before calling exec(). * failed before calling exec().
......
...@@ -747,3 +747,17 @@ TEST(CommunicateSubprocessTest, RedirectStdioToDevNull) { ...@@ -747,3 +747,17 @@ TEST(CommunicateSubprocessTest, RedirectStdioToDevNull) {
EXPECT_EQ(0, proc.wait().exitStatus()); EXPECT_EQ(0, proc.wait().exitStatus());
} }
TEST(CloseOtherDescriptorsSubprocessTest, ClosesFileDescriptors) {
// Open another filedescriptor and check to make sure that it is not opened in
// child process
int fd = ::open("/", O_RDONLY);
auto guard = makeGuard([fd] { ::close(fd); });
auto options = Subprocess::Options().closeOtherFds().pipeStdout();
Subprocess proc(
std::vector<std::string>{"/bin/ls", "/proc/self/fd"}, options);
auto p = proc.communicate();
// stdin, stdout, stderr, and /proc/self/fd should be fds [0,3] in the child
EXPECT_EQ("0\n1\n2\n3\n", p.first);
proc.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