Merge branch 'srett-child-process-enhancement' into develop

src/libs/core/impl/ChildProcess.cpp

@@ -19,18 +19,17 @@
 
 #include "ChildProcess.hpp"
 
-#include <cerrno>
-#include <cstddef>
 #include <fcntl.h>
 #include <signal.h>
 #include <sys/types.h>
 #include <sys/wait.h>
-#include <system_error>
 #include <unistd.h>
 
 #include <algorithm>
-#include <iostream>
+#include <cerrno>
+#include <cstddef>
 #include <mutex>
+#include <system_error>
 
 #include <boost/asio/buffer.hpp>
 #include <boost/asio/read.hpp>
@@ -63,32 +62,23 @@ namespace lms::core
     {
         // make sure only one thread is executing this part of code
         static std::mutex mutex;
-        std::unique_lock<std::mutex> lock{ mutex };
+        const std::scoped_lock lock{ mutex };
 
         int pipefd[2];
 
         // Use 'pipe' instead of 'pipe2', more portable
-        if (pipe(pipefd) < 0)
+        if (pipe(pipefd) == -1)
             throw SystemException{ std::error_code{ errno, std::generic_category() }, "pipe failed!" };
 
-        // Manually set the O_NONBLOCK and O_CLOEXEC flags for both ends of the pipe
+        // Only set O_NONBLOCK on read end - usually programs don't expect stdout to be non-blocking
         if (fcntl(pipefd[0], F_SETFL, O_NONBLOCK) == -1)
             throw SystemException{ std::error_code{ errno, std::generic_category() }, "fcntl failed to set O_NONBLOCK!" };
 
-        if (fcntl(pipefd[1], F_SETFL, O_NONBLOCK) == -1)
-            throw SystemException{ std::error_code{ errno, std::generic_category() }, "fcntl failed to set O_NONBLOCK!" };
-
-        if (fcntl(pipefd[0], F_SETFD, FD_CLOEXEC) == -1)
-            throw SystemException{ std::error_code{ errno, std::generic_category() }, "fcntl failed to set FD_CLOEXEC!" };
-
-        if (fcntl(pipefd[1], F_SETFD, FD_CLOEXEC) == -1)
-            throw SystemException{ std::error_code{ errno, std::generic_category() }, "fcntl failed to set FD_CLOEXEC!" };
-
 #if defined(__linux__) && defined(F_SETPIPE_SZ)
         for (const int fd : { pipefd[0], pipefd[1] })
         {
-            constexpr std::size_t targetPipeSize{ static_cast<long>(65'536) * 4 };
-            std::size_t currentPipeSize{ 65'536 }; // common default value
+            constexpr int targetPipeSize{ 65'536 * 4 };
+            int currentPipeSize{ 65'536 }; // common default value
     #if defined(F_GETPIPE_SZ)
             const int pipeSizeRes{ fcntl(fd, F_GETPIPE_SZ) };
             if (pipeSizeRes == -1)
@@ -112,27 +102,36 @@ namespace lms::core
         }
 #endif
 
-        int res{ fork() };
+        const int res{ fork() };
         if (res == -1)
             throw SystemException{ std::error_code{ errno, std::generic_category() }, "fork failed!" };
 
         if (res == 0) // CHILD
         {
-            close(pipefd[0]);
-            close(STDIN_FILENO);
-            close(STDERR_FILENO);
+            // Never close stdin/out/err, most programs expect these to exist;
+            // rather connect them to /dev/null if unwanted
+            const int nullFd{ open("/dev/null", O_RDWR) };
+            // Ignore errors, worst thing is stderr writes to the same fd as lms
+            if (nullFd != -1)
+            {
+                dup2(nullFd, STDIN_FILENO);
+                dup2(nullFd, STDERR_FILENO);
+                close(nullFd);
+            }
 
             // Replace stdout with pipe write
             if (dup2(pipefd[1], STDOUT_FILENO) == -1)
                 exit(-1);
+            // Close pipe: read end not needed, write end was dup2ed
+            close(pipefd[0]);
+            close(pipefd[1]);
 
             std::vector<const char*> execArgs;
             std::transform(std::cbegin(args), std::cend(args), std::back_inserter(execArgs), [](const std::string& arg) { return arg.c_str(); });
             execArgs.push_back(nullptr);
 
-            res = execv(path.string().c_str(), (char* const*)&execArgs[0]);
-            if (res == -1)
-                exit(-1);
+            execv(path.string().c_str(), (char* const*)&execArgs[0]);
+            exit(-1);
         }
         else // PARENT
         {
@@ -141,7 +140,7 @@ namespace lms::core
                 boost::system::error_code assignError;
                 _childStdout.assign(pipefd[0], assignError);
                 if (assignError)
-                    throw SystemException{ assignError, "fork failed!" };
+                    throw SystemException{ assignError, "assigning read end of pipe to asio stream failed!" };
             }
             _childPID = res;
         }