From 29b2fe40dc778ec73da7e5643fcfd8979d8ecebc Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Fri, 17 Mar 2023 08:27:26 -0700 Subject: [PATCH] Revert commit "don't ever force fork() instead of forkfd()" This reverts commit d6bf71123d3ef073f25610345cb5dc920e4fb783 and the docs from commit 82b75570f099911076ad0e144927862e8e359fbd ("QProcess/Linux: fix file descriptor leak in case of failed child start"). Despite the title of the commit being reverted, the actual consequence is slightly different: we always use the forkfd() function, but we change whether we force the use of the fork() library function by use of the FFD_USE_FORK flag. Commit 97645478de3ceffce11f58eab140c4c775e48be5 (5.15) first added the FFD_USE_FORK flag with a hack to detect whether the setupChild() virtual might have been overwritten. A configure-time feature to force the flag was added in commit 2ed99ff5ca338ac02f71c347b1449d4662e6c221 (6.0). Before the 6.0 release, commit d6bf71123d3ef073f25610345cb5dc920e4fb783 removed the conditional use of FFD_USE_FORK, with the changelog message saying "pthread_atfork() callbacks are consistently not invoked". We've also since added vfork()-like behavior. We tried it for Qt 5.15 and reverted shortly afterwards because we had got the memory semantics wrong. Commit e1a787a76ed462e4ed49db78a40c6d7e272182d7 (6.5) finally got it right, for Linux, which revealed another set of problems with functions used in the child process modifier. Therefore, we're going to make vfork() and clone() usage opt-in if the child process modifier is active. This commit is the first part: disabling their use by default. The flag to opt in will come in Qt 6.6. [ChangeLog][QtCore][QProcess] Reverted a change from Qt 6.0 that made the childProcessModifier() callback be run in a child created by means other than a real fork() library call, a situation in which certain other library functions would be unusable, unreliable, or cause deadlocks. A flag to opt in to the solution with better performance will be added to Qt 6.6. Task-number: QTBUG-104493 Fixes: QTBUG-111243 Fixes: QTBUG-111964 Pick-to: 6.5 Change-Id: Icfe44ecf285a480fafe4fffd174d3e66843e5a29 Reviewed-by: Fabian Kosmale --- src/corelib/io/qprocess.cpp | 9 --------- src/corelib/io/qprocess_unix.cpp | 2 ++ 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/corelib/io/qprocess.cpp b/src/corelib/io/qprocess.cpp index 41a7fffd8b..7f3cc7bd89 100644 --- a/src/corelib/io/qprocess.cpp +++ b/src/corelib/io/qprocess.cpp @@ -1588,15 +1588,6 @@ std::function QProcess::childProcessModifier() const "async-signal-safe" is advised). Most of the Qt API is unsafe inside this callback, including qDebug(), and may lead to deadlocks. - \note On some systems (notably, Linux), QProcess will use \c{vfork()} - semantics to start the child process, so this function must obey even - stricter constraints. First, because it is still sharing memory with the - parent process, it must not write to any non-local variable and must obey - proper ordering semantics when reading from them, to avoid data races. - Second, even more library functions may misbehave; therefore, this function - should only make use of low-level system calls, such as \c{read()}, - \c{write()}, \c{setsid()}, \c{nice()}, and similar. - \sa childProcessModifier() */ void QProcess::setChildProcessModifier(const std::function &modifier) diff --git a/src/corelib/io/qprocess_unix.cpp b/src/corelib/io/qprocess_unix.cpp index 741cabe6fe..a5e52fb793 100644 --- a/src/corelib/io/qprocess_unix.cpp +++ b/src/corelib/io/qprocess_unix.cpp @@ -471,6 +471,8 @@ void QProcessPrivate::startProcess() #if defined(Q_OS_LINUX) && !QT_CONFIG(forkfd_pidfd) ffdflags |= FFD_USE_FORK; #endif + if (unixExtras && unixExtras->childProcessModifier) + ffdflags |= FFD_USE_FORK; pid_t childPid; forkfd = ::vforkfd(ffdflags , &childPid, execChild2, &execChild1);