diff --git a/src/libs/core/impl/ChildProcess.cpp b/src/libs/core/impl/ChildProcess.cpp index 2dc57207..dfcb532f 100644 --- a/src/libs/core/impl/ChildProcess.cpp +++ b/src/libs/core/impl/ChildProcess.cpp @@ -19,18 +19,17 @@ #include "ChildProcess.hpp" -#include -#include #include #include #include #include -#include #include #include -#include +#include +#include #include +#include #include #include @@ -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 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(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 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; }