From 2b13207c2e408a6552b2486a1c90ad1d29e4c4b9 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Mon, 11 Mar 2024 11:42:20 -0700 Subject: [PATCH] QProcess/Unix: fix improper restoration of signal mask and cancel state By just moving the handling of the child process' desired target directory below the initialization of either the signal mask and PThread cancel state, without that "return". Commit 52ed6af5277100ed5b9a4f4231b94013ce539a2c ("QProcess/Unix: merge some code from startProcess() and startDetached()") introduced QChildProcess and merged the functionality of PThreadCancelGuard into it. But it added that "return;" to the code path failing to opendirfd() the target directory, meaning that the QChildProcess constructor could exit without calling disableThreadCancellations(), but the destructor would still run restoreThreadCancellations() every time the opening failed. And we have tests for that: setNonExistentWorkingDirectory and detachedSetNonExistentWorkingDirectory. For the cancel state, the uninitialized variable we ended up passing to pthread_setcancelstate() was probably harmless, because the cancellation state is almost always active and the variable would have been non-zero. And we don't test pthread cancellation, so we would never notice the problem. But commit bd32c7d7055b436b8c33486a5b5ce1c29db77fd4 ("QProcess/Unix: block all Unix signals between vfork() and exec()") introduced a block of the Unix signals with the same uninitialized variable problem. Unlike the PThread cancellation state, the original signal mask would usually be empty, so the "restoration" would actually mask signals we wanted. And one such important signal is SIGCHLD, used by QProcess/forkfd when *not* using vfork semantics. This meant that tests that had a child process modifier (meaning, they wouldn't use vfork semantics) would end up timing out because we'd never get the SIGCHLD that told us the child had exited. Fixes: QTBUG-123083 Pick-to: 6.7.0 Change-Id: I1362eb554b97dc012d02eab2dbca90b06728460e Reviewed-by: Oswald Buddenhagen Reviewed-by: Alexey Edelev (cherry picked from commit 418dcf88f827effb2981dcd1699b395e2aeaac2f) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/io/qprocess_unix.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/corelib/io/qprocess_unix.cpp b/src/corelib/io/qprocess_unix.cpp index 1179373cf39..ca65a3c776c 100644 --- a/src/corelib/io/qprocess_unix.cpp +++ b/src/corelib/io/qprocess_unix.cpp @@ -256,15 +256,6 @@ struct QChildProcess : d(d), argv(resolveExecutable(d->program), d->arguments), envp(d->environmentPrivate()) { - if (!d->workingDirectory.isEmpty()) { - workingDirectory = opendirfd(QFile::encodeName(d->workingDirectory)); - if (workingDirectory < 0) { - d->setErrorAndEmit(QProcess::FailedToStart, "chdir: "_L1 + qt_error_string()); - d->cleanup(); - return; - } - } - // 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. @@ -276,6 +267,15 @@ struct QChildProcess // would be bad enough with regular fork(), but it's likely fatal with // vfork(). disableThreadCancellations(); + + if (!d->workingDirectory.isEmpty()) { + workingDirectory = opendirfd(QFile::encodeName(d->workingDirectory)); + if (workingDirectory < 0) { + d->setErrorAndEmit(QProcess::FailedToStart, "chdir: "_L1 + qt_error_string()); + d->cleanup(); + } + } + } ~QChildProcess() noexcept(false) {