From bd32c7d7055b436b8c33486a5b5ce1c29db77fd4 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Thu, 25 May 2023 18:17:23 -0700 Subject: [PATCH] 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 Reviewed-by: Volker Hilsheimer --- src/corelib/io/qprocess_unix.cpp | 49 +++++++++++- .../auto/corelib/io/qprocess/tst_qprocess.cpp | 80 ++++++++++++++++++- 2 files changed, 126 insertions(+), 3 deletions(-) diff --git a/src/corelib/io/qprocess_unix.cpp b/src/corelib/io/qprocess_unix.cpp index efada360058..0e081884e15 100644 --- a/src/corelib/io/qprocess_unix.cpp +++ b/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) diff --git a/tests/auto/corelib/io/qprocess/tst_qprocess.cpp b/tests/auto/corelib/io/qprocess/tst_qprocess.cpp index f70da6de5db..71239910dc6 100644 --- a/tests/auto/corelib/io/qprocess/tst_qprocess.cpp +++ b/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("params");