QProcess/Unix: block all Unix signals between vfork() and exec()

This is similar to and extends the prevention of thread cancellation
introduced by commit ba05af82d3d8b7cbc6e22f93cbf1e3d1575afefe. This
prevents the situation in which a signal gets delivered (usually because
of a crash) and the parent process' handler is run, doing things it
shouldn't between vfork() and execve().

Most C libraries (all that I've investigated) unblock SIGABRT on
abort(), so this doesn't affect them. Likewise, on most OSes, crashes
ignore the signal block and terminate the application -- Darwin appears
to be an exception, but vfork() is not enabled there. Both situations
are tested by terminateInChildProcessModifier().

Task-number: QTBUG-113822
Change-Id: Ib5ce7a497e034ebabb2cfffd17628ca33969b7af
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>

src/corelib/io/qprocess_unix.cpp

@@ -233,6 +233,7 @@ struct QChildProcess
     const QProcessPrivate *d;
     CharPointerList argv;
     CharPointerList envp;
+    sigset_t oldsigset;
     int workingDirectory = -2;
 
     bool ok() const
@@ -253,6 +254,11 @@ struct QChildProcess
             }
         }
 
+        // Block Unix signals, to ensure the user's handlers aren't run in the
+        // child side and do something weird, especially if the handler and the
+        // user of QProcess are completely different codebases.
+        maybeBlockSignals();
+
         // Disable PThread cancellation until the child has successfully been
         // executed. We make a number of POSIX calls in the child that are thread
         // cancellation points and could cause an unexpected stack unwind. That
@@ -266,6 +272,25 @@ struct QChildProcess
             close(workingDirectory);
 
         restoreThreadCancellations();
+        restoreSignalMask();
+    }
+
+    void maybeBlockSignals() noexcept
+    {
+        // We only block Unix signals if we're using vfork(), to avoid a
+        // changing behavior to the user's modifier and because in some OSes
+        // this action would block crashing signals too.
+        if (usingVfork()) {
+            sigset_t emptyset;
+            sigfillset(&emptyset);
+            pthread_sigmask(SIG_SETMASK, &emptyset, &oldsigset);
+        }
+    }
+
+    void restoreSignalMask() const noexcept
+    {
+        if (usingVfork())
+            pthread_sigmask(SIG_SETMASK, &oldsigset, nullptr);
     }
 
     bool usingVfork() const noexcept;
@@ -581,7 +606,7 @@ inline QString QChildProcess::resolveExecutable(const QString &program)
     return program;
 }
 
-inline bool QChildProcess::usingVfork() const noexcept
+inline bool globalUsingVfork() noexcept
 {
 #if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
     // ASan writes to global memory, so we mustn't use vfork().
@@ -599,6 +624,14 @@ inline bool QChildProcess::usingVfork() const noexcept
     return false;
 #endif
 
+    return true;
+}
+
+inline bool QChildProcess::usingVfork() const noexcept
+{
+    if (!globalUsingVfork())
+        return false;
+
     if (!d->unixExtras || !d->unixExtras->childProcessModifier)
         return true;            // no modifier was supplied
 
@@ -608,6 +641,13 @@ inline bool QChildProcess::usingVfork() const noexcept
     return flags.testFlag(QProcess::UnixProcessFlag::UseVFork);
 }
 
+#ifdef QT_BUILD_INTERNAL
+Q_AUTOTEST_EXPORT bool _qprocessUsingVfork() noexcept
+{
+    return globalUsingVfork();
+}
+#endif
+
 void QProcessPrivate::startProcess()
 {
     Q_Q(QProcess);
@@ -810,6 +850,7 @@ void QChildProcess::startProcess() const noexcept
         failChildProcess(d, "fchdir", errno);
 
     bool sigpipeHandled = false;
+    bool sigmaskHandled = false;
     if (d->unixExtras) {
         // FIRST we call the user modifier function, before we dropping
         // privileges or closing non-standard file descriptors
@@ -820,11 +861,17 @@ void QChildProcess::startProcess() const noexcept
 
         auto flags = d->unixExtras->processParameters.flags;
         sigpipeHandled = flags.testAnyFlags(QProcess::ResetSignalHandlers | QProcess::IgnoreSigPipe);
+        sigmaskHandled = flags.testFlag(QProcess::ResetSignalHandlers);
     }
     if (!sigpipeHandled) {
         // reset the signal that we ignored
         QtVforkSafe::change_sigpipe(SIG_DFL);       // reset the signal that we ignored
     }
+    if (!sigmaskHandled) {
+        // restore the signal mask from the parent, if applyProcessParameters()
+        // hasn't completely reset it
+        restoreSignalMask();
+    }
 
     // execute the process
     if (!envp.pointers)

tests/auto/corelib/io/qprocess/tst_qprocess.cpp

@@ -123,6 +123,7 @@ private slots:
     void throwInChildProcessModifier();
     void terminateInChildProcessModifier_data();
     void terminateInChildProcessModifier();
+    void raiseInChildProcessModifier();
     void unixProcessParameters_data();
     void unixProcessParameters();
     void unixProcessParametersAndChildModifier();
@@ -1545,7 +1546,7 @@ void tst_QProcess::failChildProcessModifier()
         QVERIFY(!process.startDetached(&pid));
         QCOMPARE(pid, -1);
     } else {
-        process.start("testProcessNormal/testProcessNormal");
+        process.start();
         QVERIFY(!process.waitForStarted(5000));
     }
 
@@ -1615,9 +1616,12 @@ void tst_QProcess::terminateInChildProcessModifier()
     // temporarily disable QTest's crash logger
     DisableCrashLogger disableCrashLogging;
 
+    // testForwardingHelper prints to both stdout and stderr, so if we fail to
+    // fail we should be able to tell too
     QProcess process;
     process.setChildProcessModifier(function);
-    process.setProgram("testProcessNormal/testProcessNormal");
+    process.setProgram("testForwardingHelper/testForwardingHelper");
+    process.setArguments({ "/dev/null" });
 
     // temporarily disable QTest's crash logger while starting the child process
     {
@@ -1628,6 +1632,7 @@ void tst_QProcess::terminateInChildProcessModifier()
     QVERIFY2(process.waitForStarted(5000), qPrintable(process.errorString()));
     QVERIFY2(process.waitForFinished(5000), qPrintable(process.errorString()));
     QCOMPARE(process.exitStatus(), exitStatus);
+    QCOMPARE(process.readAllStandardOutput(), QByteArray());
 
     // some environments print extra stuff to stderr when we crash
 #ifndef Q_OS_QNX
@@ -1639,6 +1644,77 @@ void tst_QProcess::terminateInChildProcessModifier()
 #endif
 }
 
+QT_BEGIN_NAMESPACE
+Q_AUTOTEST_EXPORT bool _qprocessUsingVfork() noexcept;
+QT_END_NAMESPACE
+void tst_QProcess::raiseInChildProcessModifier()
+{
+#ifdef QT_BUILD_INTERNAL
+    // This is similar to the above, but knowing that raise() doesn't unblock
+    // signals, unlike abort(), this implies that
+    //  1) the raise() in the child modifier will not run our handler
+    //  2) the write() to stdout after that will run
+    //  3) QProcess resets the signal handlers to the defaults, then unblocks
+    //  4) at that point, the signal will be delivered to the child, but our
+    //     handler is no longer active so there'll be no write() to stderr
+    //
+    // Note for maintenance: if in the future this test causes the parent
+    // process to die with SIGUSR1, it means the C library is buggy and is
+    // using a cached PID in the child process after vfork().
+    if (!QT_PREPEND_NAMESPACE(_qprocessUsingVfork()))
+        QSKIP("QProcess will only block Unix signals when using vfork()");
+
+    // we use SIGUSR1 because QtTest doesn't log it and because its default
+    // action is termination, not core dumping
+    struct SigUsr1Handler {
+        SigUsr1Handler()
+        {
+            struct sigaction sa = {};
+            sa.sa_flags = SA_RESETHAND;
+            sa.sa_handler = [](int) {
+                static const char msg[] = "SIGUSR1 handler was run";
+                write(STDERR_FILENO, msg, strlen(msg));
+                raise(SIGUSR1);     // re-raise
+            };
+            sigaction(SIGUSR1, &sa, nullptr);
+        }
+        ~SigUsr1Handler() { restore(); }
+        static void restore() { signal(SIGUSR1, SIG_DFL); }
+    } sigUsr1Handler;
+
+    QProcess process;
+
+    // QProcess will block signals with UseVFork
+    process.setUnixProcessParameters(QProcess::UnixProcessFlag::UseVFork |
+                                     QProcess::UnixProcessFlag::ResetSignalHandlers);
+    process.setChildProcessModifier([]() {
+        raise(SIGUSR1);
+        ::childProcessModifier(STDOUT_FILENO);
+    });
+
+    // testForwardingHelper prints to both stdout and stderr, so if we fail to
+    // fail we should be able to tell too
+    process.setProgram("testForwardingHelper/testForwardingHelper");
+    process.setArguments({ "/dev/null" });
+
+    process.start();
+    QVERIFY2(process.waitForStarted(5000), qPrintable(process.errorString()));
+    QVERIFY2(process.waitForFinished(5000), qPrintable(process.errorString()));
+    QCOMPARE(process.error(), QProcess::Crashed);
+
+    // ensure the write() from the child modifier DID get run
+    QCOMPARE(process.readAllStandardOutput(), messageFromChildProcess);
+
+    // some environments print extra stuff to stderr when we crash
+    if (!QTestPrivate::isRunningArmOnX86()) {
+        // and write() from the SIGUSR1 handler did not
+        QCOMPARE(process.readAllStandardError(), QByteArray());
+    }
+#else
+    QSKIP("Requires QT_BUILD_INTERNAL symbols");
+#endif
+}
+
 void tst_QProcess::unixProcessParameters_data()
 {
     QTest::addColumn<QProcess::UnixProcessParameters>("params");