QProcess/Unix: add failChildProcessModifier()

QProcess detects other types of failures from inside the modifier as
successful starts, because the childStartedPipe gets closed without an
error condition getting written. The new method allows a reporting as a
proper failure-to-start.

Added tests for both cases.

[ChangeLog][QtCore][QProcess] Added failChildProcessModifier().

Change-Id: Icfe44ecf285a480fafe4fffd174da2b10306d3c2
Reviewed-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

5 files changed, 217 insertions, 68 deletions

diff --git a/src/corelib/io/qprocess.cpp b/src/corelib/io/qprocess.cpp
index a710750732..ab7219a6cd 100644
--- a/src/corelib/io/qprocess.cpp
+++ b/src/corelib/io/qprocess.cpp
@@ -1646,8 +1646,10 @@ std::function<void(void)> QProcess::childProcessModifier() const
 
     \snippet code/src_corelib_io_qprocess.cpp 4
 
-    If the modifier function needs to exit the process, remember to use
-    \c{_exit()}, not \c{exit()}.
+    If the modifier function experiences a failure condition, it can use
+    failChildProcessModifier() to report the situation to the QProcess caller.
+    Alternatively, it may use other methods of stopping the process, like
+    \c{_exit()}, or \c{abort()}.
 
     Certain properties of the child process, such as closing all extraneous
     file descriptors or disconnecting from the controlling TTY, can be more
@@ -1674,7 +1676,7 @@ std::function<void(void)> QProcess::childProcessModifier() const
     only make use of low-level system calls, such as \c{read()},
     \c{write()}, \c{setsid()}, \c{nice()}, and similar.
 
-    \sa childProcessModifier(), setUnixProcessParameters()
+    \sa childProcessModifier(), failChildProcessModifier(), setUnixProcessParameters()
 */
 void QProcess::setChildProcessModifier(const std::function<void(void)> &modifier)
 {
@@ -1685,6 +1687,46 @@ void QProcess::setChildProcessModifier(const std::function<void(void)> &modifier
 }
 
 /*!
+    \fn void QProcess::failChildProcessModifier(const char *description, int error) noexcept
+    \since 6.7
+
+    This functions can be used inside the modifier set with
+    setChildProcessModifier() to indicate an error condition was encountered.
+    When the modifier calls these functions, QProcess will emit errorOccurred()
+    with code QProcess::FailedToStart in the parent process. The \a description
+    can be used to include some information in errorString() to help diagnose
+    the problem, usually the name of the call that failed, similar to the C
+    Library function \c{perror()}. Additionally, the \a error parameter can be
+    an \c{<errno.h>} error code whose text form will also be included.
+
+    For example, a child modifier could prepare an extra file descriptor for
+    the child process this way:
+
+    \code
+        process.setChildProcessModifier([fd, &process]() {
+            if (dup2(fd, TargetFileDescriptor) < 0)
+                process.failChildProcessModifier(errno, "aux comm channel");
+        });
+        process.start();
+    \endcode
+
+    Where \c{fd} is a file descriptor currently open in the parent process. If
+    the \c{dup2()} system call resulted in an \c EBADF condition, the process
+    errorString() could be "Child process modifier reported error: aux comm
+    channel: Bad file descriptor".
+
+    This function does not return to the caller. Using it anywhere except in
+    the child modifier and with the correct QProcess object is undefined
+    behavior.
+
+    \note The implementation imposes a length limit to the \a description
+    parameter to about 500 characters. This does not include the text from the
+    \a error code.
+
+    \sa setChildProcessModifier(), setUnixProcessParameters()
+*/
+
+/*!
     \since 6.6
     Returns the \l UnixProcessParameters object describing extra flags and
     settings that will be applied to the child process on Unix systems. The
diff --git a/src/corelib/io/qprocess.h b/src/corelib/io/qprocess.h
index 0cb041f324..4839aa42b3 100644
--- a/src/corelib/io/qprocess.h
+++ b/src/corelib/io/qprocess.h
@@ -174,6 +174,7 @@ public:
 #if defined(Q_OS_UNIX) || defined(Q_QDOC)
     std::function<void(void)> childProcessModifier() const;
     void setChildProcessModifier(const std::function<void(void)> &modifier);
+    Q_NORETURN void failChildProcessModifier(const char *description, int error = 0) noexcept;
 
     enum UnixProcessFlag : quint32 {
         ResetSignalHandlers                 = 0x0001, // like POSIX_SPAWN_SETSIGDEF
diff --git a/src/corelib/io/qprocess_p.h b/src/corelib/io/qprocess_p.h
index 3b862bc5f3..0123af0463 100644
--- a/src/corelib/io/qprocess_p.h
+++ b/src/corelib/io/qprocess_p.h
@@ -304,7 +304,7 @@ public:
     void startProcess();
 #if defined(Q_OS_UNIX)
     void commitChannels() const;
-    void execChild(int workingDirectory, char **argv, char **envp) const noexcept;
+    Q_NORETURN void execChild(int workingDirectory, char **argv, char **envp) const noexcept;
 #endif
     bool processStarted(QString *errorMessage = nullptr);
     void processFinished();
diff --git a/src/corelib/io/qprocess_unix.cpp b/src/corelib/io/qprocess_unix.cpp
index ea39962b9d..6b6a0cba08 100644
--- a/src/corelib/io/qprocess_unix.cpp
+++ b/src/corelib/io/qprocess_unix.cpp
@@ -184,8 +184,12 @@ struct AutoPipe
 struct ChildError
 {
     int code;
-    char function[12];
+    char function[_POSIX_PIPE_BUF - sizeof(code)];
 };
+static_assert(std::is_trivial_v<ChildError>);
+#ifdef PIPE_BUF
+static_assert(PIPE_BUF >= sizeof(ChildError)); // PIPE_BUF may be bigger
+#endif
 
 // Used for argv and envp arguments to execve()
 struct CharPointerList
@@ -645,6 +649,42 @@ static constexpr int FakeErrnoForThrow =
 #endif
         ;
 
+static QString startFailureErrorMessage(ChildError &err, [[maybe_unused]] ssize_t bytesRead)
+{
+    // ChildError is less than the POSIX pipe buffer atomic size, so the read
+    // must not have been truncated
+    Q_ASSERT(bytesRead == sizeof(err));
+
+    qsizetype len = qstrnlen(err.function, sizeof(err.function));
+    QString complement = QString::fromUtf8(err.function, len);
+    if (err.code == FakeErrnoForThrow)
+        return QProcess::tr("Child process modifier threw an exception");
+    if (err.code == 0)
+        return QProcess::tr("Child process modifier reported error: %1")
+                .arg(std::move(complement));
+    if (err.code < 0)
+        return QProcess::tr("Child process modifier reported error: %1: %2")
+                .arg(std::move(complement), qt_error_string(-err.code));
+    return QProcess::tr("Child process set up failed: %1: %2")
+            .arg(std::move(complement), qt_error_string(err.code));
+}
+
+Q_NORETURN void
+failChildProcess(const QProcessPrivate *d, const char *description, int code) noexcept
+{
+    ChildError error = {};
+    error.code = code;
+    strncpy(error.function, description, sizeof(error.function));
+    qt_safe_write(d->childStartedPipe[1], &error, sizeof(error));
+    _exit(-1);
+}
+
+void QProcess::failChildProcessModifier(const char *description, int error) noexcept
+{
+    // We signal user errors with negative errnos
+    failChildProcess(d_func(), description, -error);
+}
+
 // See IMPORTANT notice below
 static void applyProcessParameters(const QProcess::UnixProcessParameters &params)
 {
@@ -704,22 +744,21 @@ static const char *callChildProcessModifier(const QProcessPrivate::UnixExtras *u
     return nullptr;
 }
 
-// this function doesn't return if the execution succeeds
-static const char *doExecChild(char **argv, char **envp, int workingDirFd,
-                               const QProcessPrivate::UnixExtras *unixExtras) noexcept
+Q_NORETURN static void
+doExecChild(char **argv, char **envp, int workingDirFd, const QProcessPrivate *d) noexcept
 {
     // enter the working directory
     if (workingDirFd != -1 && fchdir(workingDirFd) == -1)
-        return "fchdir";
+        failChildProcess(d, "fchdir", errno);
 
-    if (unixExtras) {
+    if (d->unixExtras) {
         // FIRST we call the user modifier function, before we dropping
         // privileges or closing non-standard file descriptors
-        if (const char *what = callChildProcessModifier(unixExtras))
-            return what;
+        if (const char *what = callChildProcessModifier(d->unixExtras.get()))
+            failChildProcess(d, what, FakeErrnoForThrow);
 
         // then we apply our other user-provided parameters
-        applyProcessParameters(unixExtras->processParameters);
+        applyProcessParameters(d->unixExtras->processParameters);
     }
 
     // execute the process
@@ -727,10 +766,9 @@ static const char *doExecChild(char **argv, char **envp, int workingDirFd,
         qt_safe_execv(argv[0], argv);
     else
         qt_safe_execve(argv[0], argv, envp);
-    return "execve";
+    failChildProcess(d, "execve", errno);
 }
 
-
 // IMPORTANT:
 //
 // This function is called in a vfork() context on some OSes (notably, Linux
@@ -740,22 +778,13 @@ void QProcessPrivate::execChild(int workingDir, char **argv, char **envp) const
 {
     QtVforkSafe::ignore_sigpipe();      // reset the signal that we ignored
 
-    ChildError error = { 0, {} };       // force zeroing of function[8]
-
     // Render channels configuration.
     commitChannels();
 
     // make sure this fd is closed if execv() succeeds
     qt_safe_close(childStartedPipe[0]);
 
-    const char *what = doExecChild(argv, envp, workingDir, unixExtras.get());
-    strcpy(error.function, what);
-
-    // notify failure
-    // don't use strerror or any other routines that may allocate memory, since
-    // some buggy libc versions can deadlock on locked mutexes.
-    error.code = errno;
-    qt_safe_write(childStartedPipe[1], &error, sizeof(error));
+    doExecChild(argv, envp, workingDir, this);
 }
 
 bool QProcessPrivate::processStarted(QString *errorMessage)
@@ -792,12 +821,8 @@ bool QProcessPrivate::processStarted(QString *errorMessage)
     }
 
     // did we read an error message?
-    if (errorMessage) {
-        if (buf.code == FakeErrnoForThrow)
-            *errorMessage = QProcess::tr("childProcessModifier() function threw an exception");
-        else
-            *errorMessage = QLatin1StringView(buf.function) + ": "_L1 + qt_error_string(buf.code);
-    }
+    if (errorMessage)
+        *errorMessage = startFailureErrorMessage(buf, ret);
 
     return false;
 }
@@ -1107,15 +1132,8 @@ void QProcessPrivate::waitForDeadChild()
 
 bool QProcessPrivate::startDetached(qint64 *pid)
 {
-
-#ifdef PIPE_BUF
-    static_assert(PIPE_BUF >= sizeof(ChildError));
-#else
-    static_assert(_POSIX_PIPE_BUF >= sizeof(ChildError));
-#endif
-    ChildError childStatus = { 0, {} };
-
     AutoPipe startedPipe, pidPipe;
+    childStartedPipe[1] = startedPipe[1];
     if (!startedPipe || !pidPipe) {
         setErrorAndEmit(QProcess::FailedToStart, "pipe: "_L1 + qt_error_string(errno));
         return false;
@@ -1155,22 +1173,14 @@ bool QProcessPrivate::startDetached(qint64 *pid)
         qt_safe_close(startedPipe[0]);
         qt_safe_close(pidPipe[0]);
 
-        auto reportFailed = [&](const char *function) {
-            childStatus.code = errno;
-            strcpy(childStatus.function, function);
-            qt_safe_write(startedPipe[1], &childStatus, sizeof(childStatus));
-            ::_exit(1);
-        };
-
         pid_t doubleForkPid = doFork();
         if (doubleForkPid == 0) {
             // Render channels configuration.
             commitChannels();
 
-            reportFailed(doExecChild(argv.pointers.get(), envp.pointers.get(), workingDirFd,
-                                     unixExtras.get()));
+            doExecChild(argv.pointers.get(), envp.pointers.get(), workingDirFd, this);
         } else if (doubleForkPid == -1) {
-            reportFailed("fork: ");
+            failChildProcess(this, "fork", errno);
         }
 
         // success
@@ -1198,6 +1208,7 @@ bool QProcessPrivate::startDetached(qint64 *pid)
     // successfully execve()'d the target process. If it returns any positive
     // result, it means one of the two children wrote an error result. Negative
     // values should not happen.
+    ChildError childStatus;
     ssize_t startResult = qt_safe_read(startedPipe[0], &childStatus, sizeof(childStatus));
 
     // reap the intermediate child
@@ -1213,10 +1224,8 @@ bool QProcessPrivate::startDetached(qint64 *pid)
     } else if (!success) {
         if (pid)
             *pid = -1;
-        QString msg;
-        if (startResult == sizeof(childStatus))
-            msg = QLatin1StringView(childStatus.function) + qt_error_string(childStatus.code);
-        setErrorAndEmit(QProcess::FailedToStart, msg);
+        setErrorAndEmit(QProcess::FailedToStart,
+                        startFailureErrorMessage(childStatus, startResult));
     }
     return success;
 }
diff --git a/tests/auto/corelib/io/qprocess/tst_qprocess.cpp b/tests/auto/corelib/io/qprocess/tst_qprocess.cpp
index cd55a72bf5..1e174e03b6 100644
--- a/tests/auto/corelib/io/qprocess/tst_qprocess.cpp
+++ b/tests/auto/corelib/io/qprocess/tst_qprocess.cpp
@@ -28,6 +28,10 @@
 
 #include <stdlib.h>
 
+#include "crasher.h"
+
+using namespace Qt::StringLiterals;
+
 typedef void (QProcess::*QProcessErrorSignal)(QProcess::ProcessError);
 
 class tst_QProcess : public QObject
@@ -114,7 +118,11 @@ private slots:
 #if defined(Q_OS_UNIX)
     void setChildProcessModifier_data();
     void setChildProcessModifier();
+    void failChildProcessModifier_data() { setChildProcessModifier_data(); }
+    void failChildProcessModifier();
     void throwInChildProcessModifier();
+    void terminateInChildProcessModifier_data();
+    void terminateInChildProcessModifier();
     void unixProcessParameters_data();
     void unixProcessParameters();
     void unixProcessParametersAndChildModifier();
@@ -1451,6 +1459,27 @@ void tst_QProcess::createProcessArgumentsModifier()
 #endif // Q_OS_WIN
 
 #ifdef Q_OS_UNIX
+static constexpr int sigs[] = { SIGABRT, SIGILL, SIGSEGV };
+struct DisableCrashLogger
+{
+    // disable core dumps too
+    tst_QProcessCrash::NoCoreDumps disableCoreDumps {};
+    std::array<struct sigaction, std::size(sigs)> oldhandlers;
+    DisableCrashLogger()
+    {
+        struct sigaction def = {};
+        def.sa_handler = SIG_DFL;
+        for (uint i = 0; i < std::size(sigs); ++i)
+            sigaction(sigs[i], &def, &oldhandlers[i]);
+    }
+    ~DisableCrashLogger()
+    {
+        // restore them
+        for (uint i = 0; i < std::size(sigs); ++i)
+            sigaction(sigs[i], &oldhandlers[i], nullptr);
+    }
+};
+
 static constexpr char messageFromChildProcess[] = "Message from the child process";
 static_assert(std::char_traits<char>::length(messageFromChildProcess) <= PIPE_BUF);
 static void childProcessModifier(int fd)
@@ -1496,6 +1525,36 @@ void tst_QProcess::setChildProcessModifier()
     qt_safe_close(pipes[0]);
 }
 
+void tst_QProcess::failChildProcessModifier()
+{
+    static const char failureMsg[] =
+            "Some error message from the child process would go here if this were a "
+            "real application";
+    static_assert(sizeof(failureMsg) < _POSIX_PIPE_BUF / 2,
+            "Implementation detail: the length of the message is limited");
+
+    QFETCH(bool, detached);
+    QProcess process;
+    process.setChildProcessModifier([&process]() {
+        process.failChildProcessModifier(failureMsg, EPERM);
+    });
+    process.setProgram("testProcessNormal/testProcessNormal");
+
+    if (detached) {
+        qint64 pid;
+        QVERIFY(!process.startDetached(&pid));
+        QCOMPARE(pid, -1);
+    } else {
+        process.start("testProcessNormal/testProcessNormal");
+        QVERIFY(!process.waitForStarted(5000));
+    }
+
+    QString errMsg = process.errorString();
+    QVERIFY2(errMsg.startsWith("Child process modifier reported error: "_L1 + failureMsg),
+             qPrintable(errMsg));
+    QVERIFY2(errMsg.endsWith(strerror(EPERM)), qPrintable(errMsg));
+}
+
 void tst_QProcess::throwInChildProcessModifier()
 {
 #ifndef __cpp_exceptions
@@ -1511,7 +1570,7 @@ void tst_QProcess::throwInChildProcessModifier()
     QVERIFY(!process.waitForStarted(5000));
     QCOMPARE(process.state(), QProcess::NotRunning);
     QCOMPARE(process.error(), QProcess::FailedToStart);
-    QVERIFY2(process.errorString().contains("childProcessModifier"),
+    QVERIFY2(process.errorString().contains("Child process modifier threw an exception"),
              qPrintable(process.errorString()));
 
     // try again, to ensure QProcess internal state wasn't corrupted
@@ -1519,11 +1578,59 @@ void tst_QProcess::throwInChildProcessModifier()
     QVERIFY(!process.waitForStarted(5000));
     QCOMPARE(process.state(), QProcess::NotRunning);
     QCOMPARE(process.error(), QProcess::FailedToStart);
-    QVERIFY2(process.errorString().contains("childProcessModifier"),
+    QVERIFY2(process.errorString().contains("Child process modifier threw an exception"),
              qPrintable(process.errorString()));
 #endif
 }
 
+void tst_QProcess::terminateInChildProcessModifier_data()
+{
+    using F = std::function<void(void)>;
+    QTest::addColumn<F>("function");
+    QTest::addColumn<QProcess::ExitStatus>("exitStatus");
+    QTest::addColumn<bool>("stderrIsEmpty");
+
+    QTest::newRow("_exit") << F([]() { _exit(0); }) << QProcess::NormalExit << true;
+    QTest::newRow("abort") << F(std::abort) << QProcess::CrashExit << true;
+    QTest::newRow("sigkill") << F([]() { raise(SIGKILL); }) << QProcess::CrashExit << true;
+    QTest::newRow("terminate") << F(std::terminate) << QProcess::CrashExit
+                               << (std::get_terminate() == std::abort);
+    QTest::newRow("crash") << F([]() { tst_QProcessCrash::crash(); }) << QProcess::CrashExit << true;
+}
+
+void tst_QProcess::terminateInChildProcessModifier()
+{
+    QFETCH(std::function<void(void)>, function);
+    QFETCH(QProcess::ExitStatus, exitStatus);
+    QFETCH(bool, stderrIsEmpty);
+
+    // temporarily disable QTest's crash logger
+    DisableCrashLogger disableCrashLogging;
+
+    QProcess process;
+    process.setChildProcessModifier(function);
+    process.setProgram("testProcessNormal/testProcessNormal");
+
+    // temporarily disable QTest's crash logger while starting the child process
+    {
+        DisableCrashLogger d;
+        process.start();
+    }
+
+    QVERIFY2(process.waitForStarted(5000), qPrintable(process.errorString()));
+    QVERIFY2(process.waitForFinished(5000), qPrintable(process.errorString()));
+    QCOMPARE(process.exitStatus(), exitStatus);
+
+    // some environments print extra stuff to stderr when we crash
+#ifndef Q_OS_QNX
+    if (!QTestPrivate::isRunningArmOnX86()) {
+        QByteArray standardError = process.readAllStandardError();
+        QVERIFY2(standardError.isEmpty() == stderrIsEmpty,
+                 "stderr was: " + standardError);
+    }
+#endif
+}
+
 void tst_QProcess::unixProcessParameters_data()
 {
     QTest::addColumn<QProcess::UnixProcessParameters>("params");
@@ -1644,16 +1751,12 @@ void tst_QProcess::unixProcessParametersAndChildModifier()
 void tst_QProcess::unixProcessParametersOtherFileDescriptors()
 {
     constexpr int TargetFileDescriptor = 3;
-    int pipes[2];
     int fd1 = open("/dev/null", O_RDONLY);
     int devnull = fcntl(fd1, F_DUPFD, 100); // instead of F_DUPFD_CLOEXEC
-    pipe2(pipes, O_CLOEXEC);
     close(fd1);
 
     auto closeFds = qScopeGuard([&] {
         close(devnull);
-        close(pipes[0]);
-        // we'll close pipe[1] before any QCOMPARE
     });
 
     QProcess process;
@@ -1662,20 +1765,14 @@ void tst_QProcess::unixProcessParametersOtherFileDescriptors()
             | QProcess::UnixProcessFlag::UseVFork;
     params.lowestFileDescriptorToClose = 4;
     process.setUnixProcessParameters(params);
-    process.setChildProcessModifier([devnull, pipes]() {
-        if (dup2(devnull, TargetFileDescriptor) == TargetFileDescriptor)
-            return;
-        write(pipes[1], &errno, sizeof(errno));
-        _exit(255);
+    process.setChildProcessModifier([devnull, &process]() {
+        if (dup2(devnull, TargetFileDescriptor) != TargetFileDescriptor)
+            process.failChildProcessModifier("dup2", errno);
     });
     process.setProgram("testUnixProcessParameters/testUnixProcessParameters");
     process.setArguments({ "file-descriptors2", QString::number(TargetFileDescriptor),
                            QString::number(devnull) });
     process.start();
-    close(pipes[1]);
-
-    if (int duperror; read(pipes[0], &duperror, sizeof(duperror)) == sizeof(duperror))
-        QFAIL(QByteArray("dup2 failed: ") + strerror(duperror));
 
     QVERIFY2(process.waitForStarted(5000), qPrintable(process.errorString()));
     QVERIFY(process.waitForFinished(5000));