From ba05af82d3d8b7cbc6e22f93cbf1e3d1575afefe Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Sat, 18 Mar 2023 11:42:48 -0700 Subject: [PATCH] QProcess/Unix: protect against stack unwinding in the child process stub There are two types of stack unwinding that can happen on Unix systems: C++ exceptions and PThread cancellations (on some systems, like Linux, PThread cancellations can be caught in catch(...) statements). We call a variety of PThread cancellation functions from inside the child stub, like close(). To avoid problems, we disable PThread cancellations completely before fork() or vfork(). The C++ exception case is simpler, because we can be sure of catching them with the catch (...) statement and simply transform them into an error message. This is also testable, which the PThread cancellation isn't. The error message isn't ideal because we're string-frozen. I'll improve it for 6.6. Pick-to: 6.5 Change-Id: Icfe44ecf285a480fafe4fffd174d97a475c93ff1 Reviewed-by: Oswald Buddenhagen Reviewed-by: Volker Hilsheimer --- src/corelib/io/qprocess_p.h | 2 +- src/corelib/io/qprocess_unix.cpp | 60 ++++++++++++++++++- .../auto/corelib/io/qprocess/tst_qprocess.cpp | 29 +++++++++ 3 files changed, 87 insertions(+), 4 deletions(-) diff --git a/src/corelib/io/qprocess_p.h b/src/corelib/io/qprocess_p.h index c0273aeeaef..dacf41b77aa 100644 --- a/src/corelib/io/qprocess_p.h +++ b/src/corelib/io/qprocess_p.h @@ -303,7 +303,7 @@ public: void startProcess(); #if defined(Q_OS_UNIX) void commitChannels() const; - void execChild(int workingDirectory, char **argv, char **envp) const; + 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 5c72a39911a..d16fa7498e2 100644 --- a/src/corelib/io/qprocess_unix.cpp +++ b/src/corelib/io/qprocess_unix.cpp @@ -47,8 +47,40 @@ QT_BEGIN_NAMESPACE +// we need an errno number to use to indicate the child process modifier threw, +// something the regular operations shouldn't set. +static constexpr int FakeErrnoForThrow = +#ifdef ECANCELED + ECANCELED +#else + ESHUTDOWN +#endif + ; + using namespace Qt::StringLiterals; +namespace { +struct PThreadCancelGuard +{ +#if defined(PTHREAD_CANCEL_DISABLE) + int oldstate; + PThreadCancelGuard() noexcept(false) + { + pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &oldstate); + } + ~PThreadCancelGuard() noexcept(false) + { + reenable(); + } + void reenable() noexcept(false) + { + // this doesn't touch errno + pthread_setcancelstate(oldstate, nullptr); + } +#endif +}; +} + #if !defined(Q_OS_DARWIN) QT_BEGIN_INCLUDE_NAMESPACE @@ -472,6 +504,11 @@ void QProcessPrivate::startProcess() const CharPointerList argv(resolveExecutable(program), arguments); const CharPointerList envp(environment.d.constData()); + // Disable PThread cancellation from this point on: we mustn't have it + // enabled when the child starts running nor while our state could get + // corrupted if we abruptly exited this function. + PThreadCancelGuard cancelGuard; + // Start the child. auto execChild1 = [this, workingDirFd, &argv, &envp]() { execChild(workingDirFd, argv.pointers.get(), envp.pointers.get()); @@ -539,12 +576,24 @@ void QProcessPrivate::startProcess() ::fcntl(stderrChannel.pipe[0], F_SETFL, ::fcntl(stderrChannel.pipe[0], F_GETFL) | O_NONBLOCK); } +static bool callChildProcessModifier(const QProcessPrivate::UnixExtras *unixExtras) noexcept +{ + QT_TRY { + if (unixExtras->childProcessModifier) + unixExtras->childProcessModifier(); + } QT_CATCH (...) { + errno = FakeErrnoForThrow; + return false; + } + return true; +} + // IMPORTANT: // // This function is called in a vfork() context on some OSes (notably, Linux // with forkfd), so it MUST NOT modify any non-local variable because it's // still sharing memory with the parent process. -void QProcessPrivate::execChild(int workingDir, char **argv, char **envp) const +void QProcessPrivate::execChild(int workingDir, char **argv, char **envp) const noexcept { ::signal(SIGPIPE, SIG_DFL); // reset the signal that we ignored @@ -564,8 +613,10 @@ void QProcessPrivate::execChild(int workingDir, char **argv, char **envp) const } if (unixExtras) { - if (unixExtras->childProcessModifier) - unixExtras->childProcessModifier(); + if (!callChildProcessModifier(unixExtras.get())) { + std::strcpy(error.function, "throw"); + goto report_errno; + } } // execute the process @@ -965,6 +1016,9 @@ bool QProcessPrivate::startDetached(qint64 *pid) const CharPointerList argv(resolveExecutable(program), arguments); const CharPointerList envp(environment.d.constData()); + // see startProcess() for more information + PThreadCancelGuard cancelGuard; + pid_t childPid = fork(); if (childPid == 0) { ::signal(SIGPIPE, SIG_DFL); // reset the signal that we ignored diff --git a/tests/auto/corelib/io/qprocess/tst_qprocess.cpp b/tests/auto/corelib/io/qprocess/tst_qprocess.cpp index 4d54e8db27f..d10c9ea5662 100644 --- a/tests/auto/corelib/io/qprocess/tst_qprocess.cpp +++ b/tests/auto/corelib/io/qprocess/tst_qprocess.cpp @@ -112,6 +112,7 @@ private slots: #endif // Q_OS_WIN #if defined(Q_OS_UNIX) void setChildProcessModifier(); + void throwInChildProcessModifier(); #endif void exitCodeTest(); void systemEnvironment(); @@ -1469,6 +1470,34 @@ void tst_QProcess::setChildProcessModifier() QCOMPARE(process.exitStatus(), QProcess::NormalExit); QCOMPARE(process.exitCode(), 0); } + +void tst_QProcess::throwInChildProcessModifier() +{ +#ifndef __cpp_exceptions + Q_SKIP("Exceptions disabled."); +#else + QProcess process; + process.setChildProcessModifier([]() { + throw 42; + }); + process.setProgram("testProcessNormal/testProcessNormal"); + + process.start(); + QVERIFY(!process.waitForStarted(5000)); + QCOMPARE(process.state(), QProcess::NotRunning); + QCOMPARE(process.error(), QProcess::FailedToStart); + QVERIFY2(process.errorString().contains("throw"), + qPrintable(process.errorString())); + + // try again, to ensure QProcess internal state wasn't corrupted + process.start(); + QVERIFY(!process.waitForStarted(5000)); + QCOMPARE(process.state(), QProcess::NotRunning); + QCOMPARE(process.error(), QProcess::FailedToStart); + QVERIFY2(process.errorString().contains("throw"), + qPrintable(process.errorString())); +#endif +} #endif void tst_QProcess::exitCodeTest()