Detect when calls to execv fail (#2751)

* Detect when calls to execv fail

* Use pipe+fcntl instead of pipe2

pipe2 is not available on macOS

* remove outdated comment

* Use _exit in child if execv fails

* Prevent Process::create leaks on some failures, and more robust in setting std streams for child

---------

Co-authored-by: Yong He <yonghe@outlook.com>

1 files changed, 149 insertions, 33 deletions

diff --git a/source/core/unix/slang-unix-process.cpp b/source/core/unix/slang-unix-process.cpp
index 2275238d9..e440bbb38 100644
--- a/source/core/unix/slang-unix-process.cpp
+++ b/source/core/unix/slang-unix-process.cpp
@@ -8,6 +8,7 @@
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 
 //#include <dirent.h>
 #include <errno.h>
@@ -370,8 +371,32 @@ SlangResult UnixPipeStream::write(const void* buffer, size_t length)
 
 static const int kCannotExecute = 126;
 
+static int pipeCLOEXEC(int pipefd[2])
+{
+#if SLANG_APPLE_FAMILY
+    // without pipe2 on macOS, there's an unavoidable race here where
+    // another process could fork and execv with execWatchPipe before we
+    // can set CLOEXEC on it...
+    if(pipe(pipefd) == -1 ||
+       fcntl(pipefd[1], F_SETFD, FD_CLOEXEC) == -1 ||
+       fcntl(pipefd[0], F_SETFD, FD_CLOEXEC) == -1)
+    {
+        return -1;
+    }
+    return 0;
+#else
+    return pipe2(pipefd, O_CLOEXEC);
+#endif
+}
+
 /* static */SlangResult Process::create(const CommandLine& commandLine, Process::Flags flags, RefPtr<Process>& outProcess)
 {
+    const char* whatFailed = nullptr;
+    pid_t childPid;
+
+    //
+    // Set up command line
+    //
     List<char const*> argPtrs;
 
     const auto& exe = commandLine.m_executableLocation;
@@ -389,50 +414,92 @@ static const int kCannotExecute = 126;
     // Terminate with a null
     argPtrs.add(nullptr);
 
-    int stdoutPipe[2];
-    int stderrPipe[2];
-    int stdinPipe[2];
+    //
+    // Set up pipes
+    //
+    int stdinPipe[2] = {-1, -1};
+    int stdoutPipe[2] = {-1, -1};
+    int stderrPipe[2] = {-1, -1};
+
+    // We will create this pipe with O_CLOEXEC, so that it gets closed
+    // automatically if the child's exec succeeds
+    int execWatchPipe[2] = {-1, -1};
+
+    if (pipe(stdinPipe) == -1 ||
+        pipe(stdoutPipe) == -1 ||
+        pipe(stderrPipe) == -1 ||
+        pipeCLOEXEC(execWatchPipe) == -1)
+    {
+        whatFailed = "pipe";
+        goto reportErr;
+    }
 
-    if (pipe(stdoutPipe) == -1 || pipe(stderrPipe) == -1 || pipe(stdinPipe) == -1)
+    // Make sure that none of our pipes are going to be clobbered by dup2 to
+    // 0,1,2 in the child.
+    whatFailed = "fcntl";
+    int next;
+    if(stdinPipe[0] < 3)
     {
-        fprintf(stderr, "error: `pipe` failed\n");
-        return SLANG_FAIL;
+        if(-1 == (next = fcntl(stdinPipe[0], F_DUPFD, 3)))
+        {
+            goto reportErr;
+        }
+        close(stdinPipe[0]);
+        stdinPipe[0] = next;
+    }
+    if(stdoutPipe[1] < 3)
+    {
+        if(-1 == (next = fcntl(stdoutPipe[1], F_DUPFD, 3)))
+        {
+            goto reportErr;
+        }
+        close(stdoutPipe[1]);
+        stdoutPipe[1] = next;
+    }
+    if(stderrPipe[1] < 3)
+    {
+        if(-1 == (next = fcntl(stderrPipe[1], F_DUPFD, 3)))
+        {
+            goto reportErr;
+        }
+        close(stderrPipe[1]);
+        stderrPipe[1] = next;
+    }
+    if(execWatchPipe[1] < 3)
+    {
+        if(-1 == (next = fcntl(execWatchPipe[1], F_DUPFD_CLOEXEC, 3)))
+        {
+            goto reportErr;
+        }
+        close(execWatchPipe[1]);
+        execWatchPipe[1] = next;
     }
+    whatFailed = nullptr;
 
-    // TODO(JS): 
-    // Ideally we'd have a mechanism to test if execvp can be successfully executed (at least in principal) before
-    // doing fork. Once we have forked we then have the problem of communicating to the parent process somehow.
-    //
-    // We could do a search down PATH and test files etc, but this seems to repeat a lot of the functionality of exec.
-    //    
-    pid_t childPid = fork();
+    childPid = fork();
     if (childPid == -1)
     {
-        fprintf(stderr, "error: `fork` failed\n");
-        return SLANG_FAIL;
+        whatFailed = "fork";
+        goto reportErr;
     }
 
     if (childPid == 0)
     {
         // We are the child process.
 
-        // Duplicate into standard handles
-        dup2(stdoutPipe[1], STDOUT_FILENO);
-        dup2(stderrPipe[1], STDERR_FILENO);
-        dup2(stdinPipe[0], STDIN_FILENO);
+        // Close unused fds and duplicate into standard handles
 
-        // Close all of the handles
+        ::close(execWatchPipe[0]);
+        ::close(stdinPipe[1]);
         ::close(stdoutPipe[0]);
-        ::close(stdoutPipe[1]);
-
         ::close(stderrPipe[0]);
-        ::close(stderrPipe[1]);
 
+        dup2(stdinPipe[0], STDIN_FILENO);
         ::close(stdinPipe[0]);
-        ::close(stdinPipe[1]);
-
-        // TODO(JS): Strictly speaking if m_executableType is 'Path' then we shouldn't be searching. Ie which
-        // exec we use here should be dependent on the executable type.
+        dup2(stdoutPipe[1], STDOUT_FILENO);
+        ::close(stdoutPipe[1]);
+        dup2(stderrPipe[1], STDERR_FILENO);
+        ::close(stderrPipe[1]);
 
         if (exe.m_type == ExecutableLocation::Type::Path)
         {
@@ -447,32 +514,81 @@ static const int kCannotExecute = 126;
 
         // If we get here, then `exec` failed
 
+        // Signal the failure to our parent
+        int execErr = errno;
+        ::write(execWatchPipe[1], &execErr, sizeof(execErr));
+
         // NOTE! Because we have dup2 into STDERR_FILENO, this error will *not* generally appear on
         // the terminal but in the stderrPipe. 
         fprintf(stderr, "error: `exec` failed\n");
 
         // Terminate with failure. 
-        exit(kCannotExecute);
-
-        return SLANG_FAIL;
+        // Call _exit() rather than exit() so we don't run anything registered with atexit()
+        ::_exit(kCannotExecute);
     }
     else
     {
         // We are the parent process
+        ::close(execWatchPipe[1]);
+        ::close(stdinPipe[0]);
         ::close(stdoutPipe[1]);
         ::close(stderrPipe[1]);
-        ::close(stdinPipe[0]);
 
         RefPtr<Stream> streams[Index(StdStreamType::CountOf)];
 
-        // Previously code didn't need to close, so we'll make stream not own the handles
+        // Previously code didn't need to close, so we'll make stream now own the handles
         streams[Index(StdStreamType::Out)] = new UnixPipeStream(stdoutPipe[0], FileAccess::Read, true);
+        stdoutPipe[0] = -1;
         streams[Index(StdStreamType::ErrorOut)] = new UnixPipeStream(stderrPipe[0], FileAccess::Read, true);
+        stderrPipe[0] = -1;
         streams[Index(StdStreamType::In)] = new UnixPipeStream(stdinPipe[1], FileAccess::Write, true);
+        stdinPipe[1] = -1;
+
+        // Check that the exec actually succeeded
+        int execErrCode;
+        // Our success is if we read zero bytes, indicating that the pipe was
+        // closed by the child's exec and O_CLOEXEC. (and us just above)
+        const int readRes = ::read(execWatchPipe[0], &execErrCode, sizeof(execErrCode));
+        if(readRes < 0)
+        {
+            whatFailed = "read from forked process";
+            goto reportErr;
+        }
+        else if (readRes > 0)
+        {
+            // exec failed, and the child reported back to us
+            // don't print messages by default, as we do some speculative
+            // execution of processes to see if they exist and it gets noisy
+            const bool verbose = false;
+            if (verbose)
+            {
+                fprintf(stderr, "error: exec for \"%s\" failed: %s\n", argPtrs[0],
+                        ::strerror(execErrCode));
+            }
+            whatFailed = "exec";
+            // Don't report the exec as we expect some of them to fail 
+            goto closePipes;
+        }
 
         outProcess = new UnixProcess(childPid, streams[0].readRef());
-        return SLANG_OK;
     }
+
+    goto closePipes;
+
+    // Report any error and then cleanup
+reportErr:
+    fprintf(stderr, "error: `%s` failed (%s)\n", whatFailed, strerror(errno));
+closePipes:
+    ::close(execWatchPipe[0]);
+    ::close(execWatchPipe[1]);
+    ::close(stdinPipe[0]);
+    ::close(stdinPipe[1]);
+    ::close(stderrPipe[0]);
+    ::close(stderrPipe[1]);
+    ::close(stdoutPipe[0]);
+    ::close(stdoutPipe[1]);
+
+    return whatFailed ? SLANG_FAIL : SLANG_OK;
 }
 
 /* static */uint64_t Process::getClockFrequency()