From 005a8bfbf0022f03dafafcf2b5c438ccf0675a49 Mon Sep 17 00:00:00 2001 From: James McDonnell Date: Fri, 18 Mar 2016 12:53:12 -0400 Subject: Remove QPROCESS_USE_SPAWN and all that it surrounds The spawn code was only used to make QProcess work on QNX 6.5.0. Fork works on QNX 6.6.0. The QNX spawn implementation has a flaw that causes a deadlock in certain situations. When a working directory is specified for the process, the QNX spawn implementation stops all threads except the one doing the spawn so that it can temporarily change the process' working directory. This can lead to a deadlock if the thread does anything that conficts with something being done in a stopped thread. QNX 6.5.0 is no longer supported in Qt 5.6.0 so we can just switch QNX to the fork implementation and get rid of the spawn implementation. Made a QNX specific adjustment to the hardExit test. There's a bug in the OS that the test can run into because it does something that normal applications wouldn't. Task-number: QTBUG-47250 Change-Id: Ib32567d2c15ce651815858000035ac5aa6f35224 Reviewed-by: Dan Cape Reviewed-by: Oswald Buddenhagen Reviewed-by: Rafael Roquetto --- src/corelib/io/forkfd_qt.cpp | 6 +- src/corelib/io/qprocess.cpp | 11 -- src/corelib/io/qprocess_p.h | 7 +- src/corelib/io/qprocess_unix.cpp | 185 ------------------------ tests/auto/corelib/io/qprocess/tst_qprocess.cpp | 38 ++--- 5 files changed, 12 insertions(+), 235 deletions(-) diff --git a/src/corelib/io/forkfd_qt.cpp b/src/corelib/io/forkfd_qt.cpp index b226332cc1..2ccc9ba837 100644 --- a/src/corelib/io/forkfd_qt.cpp +++ b/src/corelib/io/forkfd_qt.cpp @@ -35,11 +35,7 @@ #include #include "qprocess_p.h" -#ifdef QPROCESS_USE_SPAWN -# define FORKFD_NO_FORKFD -#else -# define FORKFD_NO_SPAWNFD -#endif +#define FORKFD_NO_SPAWNFD #if defined(QT_NO_DEBUG) && !defined(NDEBUG) # define NDEBUG diff --git a/src/corelib/io/qprocess.cpp b/src/corelib/io/qprocess.cpp index 952116b9db..a86e3cb438 100644 --- a/src/corelib/io/qprocess.cpp +++ b/src/corelib/io/qprocess.cpp @@ -1134,17 +1134,6 @@ bool QProcessPrivate::_q_processDied() if (crashed) { exitStatus = QProcess::CrashExit; setErrorAndEmit(QProcess::Crashed); - } else { -#ifdef QPROCESS_USE_SPAWN - // if we're using posix_spawn, waitForStarted always succeeds. - // POSIX documents that the sub-process launched by posix_spawn will exit with code - // 127 if anything prevents the target program from starting. - // http://pubs.opengroup.org/onlinepubs/009695399/functions/posix_spawn.html - if (exitStatus == QProcess::NormalExit && exitCode == 127) { - setError(QProcess::FailedToStart, - QProcess::tr("Process failed to start (spawned process exited with code 127)")); - } -#endif } bool wasRunning = (processState == QProcess::Running); diff --git a/src/corelib/io/qprocess_p.h b/src/corelib/io/qprocess_p.h index d3f251c399..436869462c 100644 --- a/src/corelib/io/qprocess_p.h +++ b/src/corelib/io/qprocess_p.h @@ -62,9 +62,6 @@ typedef HANDLE Q_PIPE; #else typedef int Q_PIPE; #define INVALID_Q_PIPE -1 -# ifdef Q_OS_QNX -# define QPROCESS_USE_SPAWN -# endif #endif #ifndef QT_NO_PROCESS @@ -347,10 +344,8 @@ public: void start(QIODevice::OpenMode mode); void startProcess(); -#if defined(Q_OS_UNIX) && !defined(QPROCESS_USE_SPAWN) +#if defined(Q_OS_UNIX) void execChild(const char *workingDirectory, char **path, char **argv, char **envp); -#elif defined(QPROCESS_USE_SPAWN) - pid_t spawnChild(pid_t *ppid, const char *workingDirectory, char **argv, char **envp); #endif bool processStarted(QString *errorMessage = Q_NULLPTR); void terminateProcess(); diff --git a/src/corelib/io/qprocess_unix.cpp b/src/corelib/io/qprocess_unix.cpp index 8eb5ac9564..18d203404d 100644 --- a/src/corelib/io/qprocess_unix.cpp +++ b/src/corelib/io/qprocess_unix.cpp @@ -423,14 +423,8 @@ void QProcessPrivate::startProcess() } // Start the process manager, and fork off the child process. -#if defined(QPROCESS_USE_SPAWN) - pid_t childPid; - forkfd = spawnChild(&childPid, workingDirPtr, argv, envp); - Q_ASSUME(forkfd != FFD_CHILD_PROCESS); -#else pid_t childPid; forkfd = ::forkfd(FFD_CLOEXEC, &childPid); -#endif int lastForkErrno = errno; if (forkfd != FFD_CHILD_PROCESS) { // Parent process. @@ -464,12 +458,10 @@ void QProcessPrivate::startProcess() } // Start the child. -#if !defined(QPROCESS_USE_SPAWN) if (forkfd == FFD_CHILD_PROCESS) { execChild(workingDirPtr, path, argv, envp); ::_exit(-1); } -#endif pid = Q_PID(childPid); @@ -508,147 +500,6 @@ void QProcessPrivate::startProcess() } } -#if defined(QPROCESS_USE_SPAWN) -static int doSpawn(pid_t *ppid, const posix_spawn_file_actions_t *file_actions, - char **argv, char **envp, const char *workingDir, bool spawn_detached) -{ - // A multi threaded QNX Process can't fork so we call spawnfd() instead. - posix_spawnattr_t attr; - posix_spawnattr_init(&attr); -# ifdef Q_OS_QNX - posix_spawnattr_setxflags(&attr, POSIX_SPAWN_SETSIGDEF | POSIX_SPAWN_SETPGROUP - | (spawn_detached * POSIX_SPAWN_NOZOMBIE)); -# else - posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETSIGDEF | POSIX_SPAWN_SETPGROUP); -# endif - posix_spawnattr_setpgroup(&attr, 0); - - sigset_t sigdefault; - sigemptyset(&sigdefault); - sigaddset(&sigdefault, SIGPIPE); // reset the signal that we ignored - posix_spawnattr_setsigdefault(&attr, &sigdefault); - - // enter the working directory - const char *oldWorkingDir = 0; - char buff[PATH_MAX + 1]; - - if (workingDir) { -# ifdef Q_OS_QNX - //we need to freeze everyone in order to avoid race conditions with //chdir(). - if (ThreadCtl(_NTO_TCTL_THREADS_HOLD, 0) == -1) - qWarning("ThreadCtl(): cannot hold threads: %s", qPrintable(qt_error_string(errno))); -# endif - - oldWorkingDir = QT_GETCWD(buff, PATH_MAX + 1); - if (QT_CHDIR(workingDir) == -1) - qWarning("ThreadCtl(): failed to chdir to %s", workingDir); - } - - int fd; - if (spawn_detached) { - fd = ::posix_spawn(ppid, argv[0], file_actions, &attr, argv, envp); - if (fd == -1) { - fd = ::posix_spawnp(ppid, argv[0], file_actions, &attr, argv, envp); - } - } else { - // use spawnfd - fd = ::spawnfd(FFD_CLOEXEC | FFD_NONBLOCK, ppid, argv[0], file_actions, &attr, argv, envp); - if (fd == -1) { - fd = ::spawnfd(FFD_CLOEXEC | FFD_NONBLOCK | FFD_SPAWN_SEARCH_PATH, ppid, argv[0], file_actions, - &attr, argv, envp); - } - } - - if (oldWorkingDir) { - if (QT_CHDIR(oldWorkingDir) == -1) - qWarning("ThreadCtl(): failed to chdir to %s", oldWorkingDir); - -# ifdef Q_OS_QNX - if (ThreadCtl(_NTO_TCTL_THREADS_CONT, 0) == -1) - qFatal("ThreadCtl(): cannot resume threads: %s", qPrintable(qt_error_string(errno))); -# endif - } - - posix_spawnattr_destroy(&attr); - return fd; -} - -pid_t QProcessPrivate::spawnChild(pid_t *ppid, const char *workingDir, char **argv, char **envp) -{ - // posix_spawn causes all file descriptors with FD_CLOEXEC to be closed automatically; - // we only need to add the actions for our own pipes - posix_spawn_file_actions_t file_actions; - posix_spawn_file_actions_init(&file_actions); - -# ifdef Q_OS_QNX - static const bool OS_QNX = true; -# else - static const bool OS_QNX = false; -#endif - - int fdmax = -1; - - if (processChannelMode == QProcess::MergedChannels) { - // managed stderr == stdout - posix_spawn_file_actions_adddup2(&file_actions, stdoutChannel.pipe[1], STDERR_FILENO); - - if (OS_QNX) - fdmax = qMax(fdmax, stdoutChannel.pipe[1]); - } else if (processChannelMode != QProcess::ForwardedChannels && processChannelMode != QProcess::ForwardedErrorChannel) { - // managed stderr - posix_spawn_file_actions_adddup2(&file_actions, stderrChannel.pipe[1], STDERR_FILENO); - - if (OS_QNX) - fdmax = qMax(fdmax, stderrChannel.pipe[1]); - else - posix_spawn_file_actions_addclose(&file_actions, stderrChannel.pipe[1]); - - } - - if (processChannelMode != QProcess::ForwardedChannels && processChannelMode != QProcess::ForwardedOutputChannel) { - // managed stdout - posix_spawn_file_actions_adddup2(&file_actions, stdoutChannel.pipe[1], STDOUT_FILENO); - - if (OS_QNX) - fdmax = qMax(fdmax, stdoutChannel.pipe[1]); - else - posix_spawn_file_actions_addclose(&file_actions, stdoutChannel.pipe[1]); - - } - - if (inputChannelMode == QProcess::ManagedInputChannel) { - posix_spawn_file_actions_adddup2(&file_actions, stdinChannel.pipe[0], STDIN_FILENO); - - if (OS_QNX) - fdmax = qMax(fdmax, stdinChannel.pipe[0]); - else - posix_spawn_file_actions_addclose(&file_actions, stdinChannel.pipe[0]); - } - - // Workaround: QNX's spawn implementation will actually dup all FD values - // LESS than fdmax - regardless of the FD_CLOEEXEC flag. So we need to add - // those to the list of files to close, otherwise dup will fail when some - // other thread closes the FD. - for (int i = 3; i <= fdmax; i++) { - if (::fcntl(i, F_GETFD) & FD_CLOEXEC) - posix_spawn_file_actions_addclose(&file_actions, i); - } - - int retval = doSpawn(ppid, &file_actions, argv, envp, workingDir, false); - - if (retval == -1) { - QString error = qt_error_string(errno); - qt_safe_write(childStartedPipe[1], error.data(), error.length() * sizeof(QChar)); - qt_safe_close(childStartedPipe[1]); - childStartedPipe[1] = -1; - } - - posix_spawn_file_actions_destroy(&file_actions); - return retval; -} - -#else - void QProcessPrivate::execChild(const char *workingDir, char **path, char **argv, char **envp) { ::signal(SIGPIPE, SIG_DFL); // reset the signal that we ignored @@ -716,7 +567,6 @@ report_errno: qt_safe_close(childStartedPipe[1]); childStartedPipe[1] = -1; } -#endif bool QProcessPrivate::processStarted(QString *errorMessage) { @@ -1100,40 +950,6 @@ bool QProcessPrivate::waitForDeadChild() return true; } -#if defined(QPROCESS_USE_SPAWN) -bool QProcessPrivate::startDetached(const QString &program, const QStringList &arguments, const QString &workingDirectory, qint64 *pid) -{ - QList enc_args; - enc_args.append(QFile::encodeName(program)); - for (int i = 0; i < arguments.size(); ++i) - enc_args.append(arguments.at(i).toLocal8Bit()); - - const int argc = enc_args.size(); - QScopedArrayPointer raw_argv(new char*[argc + 1]); - for (int i = 0; i < argc; ++i) - raw_argv[i] = const_cast(enc_args.at(i).data()); - raw_argv[argc] = 0; - - char **envp = 0; // inherit environment - - // Encode the working directory if it's non-empty, otherwise just pass 0. - const char *workingDirPtr = 0; - QByteArray encodedWorkingDirectory; - if (!workingDirectory.isEmpty()) { - encodedWorkingDirectory = QFile::encodeName(workingDirectory); - workingDirPtr = encodedWorkingDirectory.constData(); - } - - pid_t childPid; - int retval = doSpawn(&childPid, NULL, raw_argv.data(), envp, workingDirPtr, true); - if (pid && retval != -1) - *pid = childPid; - - return retval != -1; -} - -#else - bool QProcessPrivate::startDetached(const QString &program, const QStringList &arguments, const QString &workingDirectory, qint64 *pid) { QByteArray encodedWorkingDirectory = QFile::encodeName(workingDirectory); @@ -1248,7 +1064,6 @@ bool QProcessPrivate::startDetached(const QString &program, const QStringList &a qt_safe_close(pidPipe[0]); return success; } -#endif QT_END_NAMESPACE diff --git a/tests/auto/corelib/io/qprocess/tst_qprocess.cpp b/tests/auto/corelib/io/qprocess/tst_qprocess.cpp index 46bf1a19e8..7e1d5487ba 100644 --- a/tests/auto/corelib/io/qprocess/tst_qprocess.cpp +++ b/tests/auto/corelib/io/qprocess/tst_qprocess.cpp @@ -45,8 +45,6 @@ #include #include -# include // only so we get QPROCESS_USE_SPAWN - typedef void (QProcess::*QProcessFinishedSignal1)(int); typedef void (QProcess::*QProcessFinishedSignal2)(int, QProcess::ExitStatus); typedef void (QProcess::*QProcessErrorSignal)(QProcess::ProcessError); @@ -325,9 +323,6 @@ void tst_QProcess::startDetached() { QVERIFY(QProcess::startDetached("testProcessNormal/testProcessNormal", QStringList() << "arg1" << "arg2")); -#ifdef QPROCESS_USE_SPAWN - QEXPECT_FAIL("", "QProcess cannot detect failure to start when using posix_spawn()", Continue); -#endif QCOMPARE(QProcess::startDetached("nonexistingexe"), false); } @@ -707,9 +702,6 @@ void tst_QProcess::waitForFinished() QCOMPARE(output.count("\n"), 10*1024); process.start("blurdybloop"); -#if defined(QPROCESS_USE_SPAWN) && !defined(Q_OS_QNX) - QEXPECT_FAIL("", "QProcess cannot detect failure to start when using posix_spawn()", Abort); -#endif QVERIFY(!process.waitForFinished()); QCOMPARE(process.error(), QProcess::FailedToStart); } @@ -922,6 +914,16 @@ void tst_QProcess::hardExit() #endif QVERIFY2(proc.waitForStarted(), qPrintable(proc.errorString())); + +#if defined(Q_OS_QNX) + // QNX may lose the kill if it's delivered while the forked process + // is doing the exec that morphs it into testProcessEcho. It's very + // unlikely that a normal application would do such a thing. Make + // sure the test doesn't accidentally try to do it. + proc.write("A"); + QVERIFY(proc.waitForReadyRead(5000)); +#endif + proc.kill(); QVERIFY(proc.waitForFinished(5000)); @@ -1503,11 +1505,6 @@ void tst_QProcess::nativeArguments() void tst_QProcess::exitCodeTest() { for (int i = 0; i < 255; ++i) { -#ifdef QPROCESS_USE_SPAWN - // POSIX reserves exit code 127 when using posix_spawn - if (i == 127) - continue; -#endif QProcess process; process.start("testExitCodes/testExitCodes " + QString::number(i)); QVERIFY(process.waitForFinished(5000)); @@ -1518,9 +1515,6 @@ void tst_QProcess::exitCodeTest() void tst_QProcess::failToStart() { -#if defined(QPROCESS_USE_SPAWN) && !defined(Q_OS_QNX) - QSKIP("QProcess cannot detect failure to start when using posix_spawn()"); -#endif qRegisterMetaType("QProcess::ProcessError"); qRegisterMetaType("QProcess::ExitStatus"); qRegisterMetaType("QProcess::ProcessState"); @@ -1591,9 +1585,6 @@ void tst_QProcess::failToStart() void tst_QProcess::failToStartWithWait() { -#if defined(QPROCESS_USE_SPAWN) && !defined(Q_OS_QNX) - QSKIP("QProcess cannot detect failure to start when using posix_spawn()"); -#endif qRegisterMetaType("QProcess::ProcessError"); qRegisterMetaType("QProcess::ExitStatus"); @@ -1623,9 +1614,6 @@ void tst_QProcess::failToStartWithWait() void tst_QProcess::failToStartWithEventLoop() { -#if defined(QPROCESS_USE_SPAWN) && !defined(Q_OS_QNX) - QSKIP("QProcess cannot detect failure to start when using posix_spawn()"); -#endif qRegisterMetaType("QProcess::ProcessError"); qRegisterMetaType("QProcess::ExitStatus"); @@ -1912,9 +1900,6 @@ void tst_QProcess::waitForReadyReadForNonexistantProcess() QVERIFY(!process.waitForReadyRead()); // used to crash process.start("doesntexist"); QVERIFY(!process.waitForReadyRead()); -#if defined(QPROCESS_USE_SPAWN) && !defined(Q_OS_QNX) - QEXPECT_FAIL("", "QProcess cannot detect failure to start when using posix_spawn()", Abort); -#endif QCOMPARE(errorSpy.count(), 1); QCOMPARE(errorSpy.at(0).at(0).toInt(), 0); QCOMPARE(errorSpy2.count(), 1); @@ -2282,9 +2267,6 @@ void tst_QProcess::setNonExistentWorkingDirectory() // while on Unix with fork it's relative to the child's (with posix_spawn, it could be either). process.start(QFileInfo("testSetWorkingDirectory/testSetWorkingDirectory").absoluteFilePath()); QVERIFY(!process.waitForFinished()); -#ifdef QPROCESS_USE_SPAWN - QEXPECT_FAIL("", "QProcess cannot detect failure to start when using posix_spawn()", Continue); -#endif QCOMPARE(int(process.error()), int(QProcess::FailedToStart)); } #endif -- cgit v1.2.3