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 <oswald.buddenhagen@gmx.de>
Reviewed-by:  Alexey Edelev <alexey.edelev@qt.io>
(cherry picked from commit 418dcf88f827effb2981dcd1699b395e2aeaac2f)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
This commit is contained in:
Thiago Macieira 2024-03-11 11:42:20 -07:00 committed by Qt Cherry-pick Bot
parent 58991ca2ae
commit 2b13207c2e

View File

@ -256,15 +256,6 @@ struct QChildProcess
: d(d), argv(resolveExecutable(d->program), d->arguments), : d(d), argv(resolveExecutable(d->program), d->arguments),
envp(d->environmentPrivate()) 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 // 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 // child side and do something weird, especially if the handler and the
// user of QProcess are completely different codebases. // 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 // would be bad enough with regular fork(), but it's likely fatal with
// vfork(). // vfork().
disableThreadCancellations(); 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) ~QChildProcess() noexcept(false)
{ {