Minor fixes to ChildProcess

- Don't set FD_CLOEXEC, but close manually before fork:
  Since we don't use pipe2() to set the flag atomically, we're prone to
  races with concurrent fork+execs either way. As ChildProcess uses a
  mutex here anyways, as long as there is no other part in lms that
  would fork+exec, we're not any more or less safe now, but the code
  is shorter and we cannot fail the fcntl.
- Don't make the write-end of the pipe non-blocking. Usually programs
  do not expect this, i.e. they either never check the return code of
  a write to stdout (potential data loss), or if they do, they just
  bail out on any error, and don't handle EAGAIN. ffmpeg seemed
  to handle this fine though (or we were just lucky and always read
  the data faster than ffmpeg could produce it).
- Do not close stdin and stderr. Again ffmpeg seems to handle this,
  at least regarding stdin thanks to -nostdin, but in general if a
  process tries to write to stderr and fd 2 is not open, it might
  just bail out. Even worse, the process might have opened some
  file it wants to work with, and that file got assigned fd 2 (as
  that was the next free fd) - the program would corrupt whatever
  file it opened there whenever it tries to write to stderr.
  Try to open /dev/null instead for stdin and stderr, and if that
  fails, keep whatever lms inherited open, which should be safer
  than relying on the child process to handle this case properly.
- exec() does not return on success, no need to check return code,
  any return from it is a failure.
- Wrong error message in error path when assigning fd to boost stream.
- F_SETPIPE_SZ requires and int, not size_t. This worked on little
  endian systems since the value passed was < 2^32, but on big
  endian systems you'd effectively pass "0" if using a 64bit type.
- Address a few clang-tidy complaints (constness)

src/libs/core/impl/ChildProcess.cpp

@@ -63,32 +63,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::lock_guard<std::mutex> 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 +103,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 +141,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;
         }