SERVER-26910 lock around fork() in shell launcher

The shell process management functions fork()/exec() in a multi-threaded
program without sufficient guards to avoid leaking fds into children.
This causes deadlocks around the program registry (as we rely on file
descriptor closure to break us out of dedicated child threads which read
child output).

The fix is to use a global lock to guard us from before the call to
pipe() until after we close write side of the pipe in the parent.

1 files changed, 83 insertions, 67 deletions

diff --git a/src/mongo/shell/shell_utils_launcher.cpp b/src/mongo/shell/shell_utils_launcher.cpp
index 629e48e5106..55ee501c10a 100644
--- a/src/mongo/shell/shell_utils_launcher.cpp
+++ b/src/mongo/shell/shell_utils_launcher.cpp
@@ -134,9 +134,7 @@ void safeClose(int fd) {
     }
 }
 
-#ifdef _WIN32
 stdx::mutex _createProcessMtx;
-#endif
 }  // namespace
 
 ProgramOutputMultiplexer programOutputLogger;
@@ -374,37 +372,58 @@ ProgramRunner::ProgramRunner(const BSONObj& args, const BSONObj& env) {
 
 void ProgramRunner::start() {
     int pipeEnds[2];
-    int status = pipe(pipeEnds);
-    if (status != 0) {
-        const auto ewd = errnoWithDescription();
-        error() << "failed to create pipe: " << ewd;
-        fassertFailed(16701);
-    }
+
+    {
+        // NOTE(JCAREY):
+        //
+        // We take this lock from before our call to pipe until after we close the write side (in
+        // the parent) to avoid leaking fds from threads racing around fork().  I.e.
+        //
+        // Thread A: calls pipe()
+        // Thread B: calls fork()
+        // A: sets cloexec on read and write sides
+        // B: has a forked child with open fds
+        // A: spawns a child thread to read it's child process's stdout
+        // A: A's child process exits
+        // A: wait's on A's reader thread in de-register
+        // A: deadlocks forever (because the child reader thread stays in read() because of the open
+        //    fd in B)
+        //
+        // Holding the lock for the duration of those events prevents the leaks and thus the
+        // associated deadlocks.
+        stdx::lock_guard<stdx::mutex> lk(_createProcessMtx);
+        int status = pipe(pipeEnds);
+        if (status != 0) {
+            const auto ewd = errnoWithDescription();
+            error() << "failed to create pipe: " << ewd;
+            fassertFailed(16701);
+        }
 #ifndef _WIN32
-    // The calls to fcntl to set CLOEXEC ensure that processes started by the process we are about
-    // to fork do *not* inherit the file descriptors for the pipe. If grandchild processes could
-    // inherit the FD for the pipe, than the pipe wouldn't close on child process exit. On windows,
-    // instead the handle inherit flag is turned off after the call to CreateProcess.
-    status = fcntl(pipeEnds[0], F_SETFD, FD_CLOEXEC);
-    if (status != 0) {
-        const auto ewd = errnoWithDescription();
-        error() << "failed to set FD_CLOEXEC on pipe end 0: " << ewd;
-        fassertFailed(40308);
-    }
-    status = fcntl(pipeEnds[1], F_SETFD, FD_CLOEXEC);
-    if (status != 0) {
-        const auto ewd = errnoWithDescription();
-        error() << "failed to set FD_CLOEXEC on pipe end 1: " << ewd;
-        fassertFailed(40317);
-    }
+        // The calls to fcntl to set CLOEXEC ensure that processes started by the process we are
+        // about to fork do *not* inherit the file descriptors for the pipe. If grandchild processes
+        // could inherit the FD for the pipe, than the pipe wouldn't close on child process exit. On
+        // windows, instead the handle inherit flag is turned off after the call to CreateProcess.
+        status = fcntl(pipeEnds[0], F_SETFD, FD_CLOEXEC);
+        if (status != 0) {
+            const auto ewd = errnoWithDescription();
+            error() << "failed to set FD_CLOEXEC on pipe end 0: " << ewd;
+            fassertFailed(40308);
+        }
+        status = fcntl(pipeEnds[1], F_SETFD, FD_CLOEXEC);
+        if (status != 0) {
+            const auto ewd = errnoWithDescription();
+            error() << "failed to set FD_CLOEXEC on pipe end 1: " << ewd;
+            fassertFailed(40317);
+        }
 #endif
 
-    fflush(0);
+        fflush(0);
 
-    launchProcess(pipeEnds[1]);  // sets _pid
+        launchProcess(pipeEnds[1]);  // sets _pid
 
-    // Close the write end of the pipe.
-    safeClose(pipeEnds[1]);
+        // Close the write end of the pipe.
+        safeClose(pipeEnds[1]);
+    }
 
     if (_port >= 0) {
         registry.registerProgram(_pid, _port);
@@ -551,47 +570,44 @@ void ProgramRunner::launchProcess(int child_stdout) {
     }
     std::memset(lpEnvironment.get() + environmentOffset, 0, sizeof(wchar_t));
 
-    {
-        stdx::lock_guard<stdx::mutex> lk(_createProcessMtx);
-        HANDLE h = reinterpret_cast<HANDLE>(_get_osfhandle(child_stdout));
-        invariant(h != INVALID_HANDLE_VALUE);
-        invariant(SetHandleInformation(h, HANDLE_FLAG_INHERIT, 1));
-
-        STARTUPINFO si;
-        ZeroMemory(&si, sizeof(si));
-        si.cb = sizeof(si);
-        si.hStdError = h;
-        si.hStdOutput = h;
-        si.dwFlags |= STARTF_USESTDHANDLES;
-
-        PROCESS_INFORMATION pi;
-        ZeroMemory(&pi, sizeof(pi));
-
-        DWORD dwCreationFlags = 0;
-        dwCreationFlags |= CREATE_UNICODE_ENVIRONMENT;
-
-        bool success = CreateProcessW(nullptr,
-                                      const_cast<LPWSTR>(args.c_str()),
-                                      nullptr,
-                                      nullptr,
-                                      true,
-                                      dwCreationFlags,
-                                      lpEnvironment.get(),
-                                      nullptr,
-                                      &si,
-                                      &pi) != 0;
-        if (!success) {
-            const auto ewd = errnoWithDescription();
-            ss << "couldn't start process " << _argv[0] << "; " << ewd;
-            uasserted(14042, ss.str());
-        }
+    HANDLE h = reinterpret_cast<HANDLE>(_get_osfhandle(child_stdout));
+    invariant(h != INVALID_HANDLE_VALUE);
+    invariant(SetHandleInformation(h, HANDLE_FLAG_INHERIT, 1));
+
+    STARTUPINFO si;
+    ZeroMemory(&si, sizeof(si));
+    si.cb = sizeof(si);
+    si.hStdError = h;
+    si.hStdOutput = h;
+    si.dwFlags |= STARTF_USESTDHANDLES;
+
+    PROCESS_INFORMATION pi;
+    ZeroMemory(&pi, sizeof(pi));
+
+    DWORD dwCreationFlags = 0;
+    dwCreationFlags |= CREATE_UNICODE_ENVIRONMENT;
+
+    bool success = CreateProcessW(nullptr,
+                                  const_cast<LPWSTR>(args.c_str()),
+                                  nullptr,
+                                  nullptr,
+                                  true,
+                                  dwCreationFlags,
+                                  lpEnvironment.get(),
+                                  nullptr,
+                                  &si,
+                                  &pi) != 0;
+    if (!success) {
+        const auto ewd = errnoWithDescription();
+        ss << "couldn't start process " << _argv[0] << "; " << ewd;
+        uasserted(14042, ss.str());
+    }
 
-        CloseHandle(pi.hThread);
-        invariant(SetHandleInformation(h, HANDLE_FLAG_INHERIT, 0));
+    CloseHandle(pi.hThread);
+    invariant(SetHandleInformation(h, HANDLE_FLAG_INHERIT, 0));
 
-        _pid = ProcessId::fromNative(pi.dwProcessId);
-        registry.insertHandleForPid(_pid, pi.hProcess);
-    }
+    _pid = ProcessId::fromNative(pi.dwProcessId);
+    registry.insertHandleForPid(_pid, pi.hProcess);
 #else
 
     std::string execErrMsg = str::stream() << "Unable to start program " << _argv[0];