From f25300cadf26c068d6cfb54d0900294f21a4c74e Mon Sep 17 00:00:00 2001 From: Jarek Kobus Date: Thu, 16 Dec 2021 10:43:58 +0100 Subject: [PATCH] Fix a possible crash in process launcher It may apparently happen that when calling QProcess::start() we may receive a synchronous signal QProcess::errorOccurred() from the process we are trying to start. In this case the handler of the error signal might have removed the process from m_processes hash, which invalidated the "Process *& process" reference inside LauncherSocketHandler::handleStartPacket(). So, using process reference after calling start() may be dangerous. Refactor ProcessStartHandler so that it stores the pointer to the process it handles. The pointer to the handler should still be valid after calling start(), since the process itself is being deleted with a delay. Make ProcessStartHandler a member of ProcessHelper. In this way it's being reused in QProcessImpl and ProcessLauncher. Fixes: QTCREATORBUG-26726 Change-Id: I8e3f39953035d76c83bbbb13bd78e3042ba2a14e Reviewed-by: Orgad Shaneh Reviewed-by: Qt CI Bot Reviewed-by: --- src/libs/utils/processutils.cpp | 22 ++++++++----------- src/libs/utils/processutils.h | 16 +++++++++----- src/libs/utils/qtcprocess.cpp | 16 +++++++------- .../processlauncher/launchersockethandler.cpp | 10 ++++----- 4 files changed, 32 insertions(+), 32 deletions(-) diff --git a/src/libs/utils/processutils.cpp b/src/libs/utils/processutils.cpp index f949c05f72c..1f57fc15d1f 100644 --- a/src/libs/utils/processutils.cpp +++ b/src/libs/utils/processutils.cpp @@ -44,44 +44,40 @@ QIODevice::OpenMode ProcessStartHandler::openMode() const return QIODevice::ReadWrite; // initial write and then reading (close the write channel) } -void ProcessStartHandler::handleProcessStart(QProcess *process) +void ProcessStartHandler::handleProcessStart() { if (m_processMode == ProcessMode::Writer) return; if (m_writeData.isEmpty()) - process->closeWriteChannel(); + m_process->closeWriteChannel(); } -void ProcessStartHandler::handleProcessStarted(QProcess *process) +void ProcessStartHandler::handleProcessStarted() { if (!m_writeData.isEmpty()) { - process->write(m_writeData); + m_process->write(m_writeData); m_writeData = {}; if (m_processMode == ProcessMode::Reader) - process->closeWriteChannel(); + m_process->closeWriteChannel(); } } - -void ProcessStartHandler::setBelowNormalPriority(QProcess *process) +void ProcessStartHandler::setBelowNormalPriority() { #ifdef Q_OS_WIN - process->setCreateProcessArgumentsModifier( + m_process->setCreateProcessArgumentsModifier( [](QProcess::CreateProcessArguments *args) { args->flags |= BELOW_NORMAL_PRIORITY_CLASS; }); -#else - Q_UNUSED(process) #endif // Q_OS_WIN } -void ProcessStartHandler::setNativeArguments(QProcess *process, const QString &arguments) +void ProcessStartHandler::setNativeArguments(const QString &arguments) { #ifdef Q_OS_WIN if (!arguments.isEmpty()) - process->setNativeArguments(arguments); + m_process->setNativeArguments(arguments); #else - Q_UNUSED(process) Q_UNUSED(arguments) #endif // Q_OS_WIN } diff --git a/src/libs/utils/processutils.h b/src/libs/utils/processutils.h index db3d18a7a37..74f3c1c8e20 100644 --- a/src/libs/utils/processutils.h +++ b/src/libs/utils/processutils.h @@ -38,23 +38,26 @@ enum class ProcessMode { class ProcessStartHandler { public: + ProcessStartHandler(QProcess *process) : m_process(process) {} + void setProcessMode(ProcessMode mode) { m_processMode = mode; } void setWriteData(const QByteArray &writeData) { m_writeData = writeData; } QIODevice::OpenMode openMode() const; - void handleProcessStart(QProcess *process); - void handleProcessStarted(QProcess *process); - void setBelowNormalPriority(QProcess *process); - void setNativeArguments(QProcess *process, const QString &arguments); + void handleProcessStart(); + void handleProcessStarted(); + void setBelowNormalPriority(); + void setNativeArguments(const QString &arguments); private: ProcessMode m_processMode = ProcessMode::Reader; QByteArray m_writeData; + QProcess *m_process; }; class ProcessHelper : public QProcess { public: - ProcessHelper(QObject *parent) : QProcess(parent) + ProcessHelper(QObject *parent) : QProcess(parent), m_processStartHandler(this) { #if QT_VERSION >= QT_VERSION_CHECK(6, 0, 0) && defined(Q_OS_UNIX) setChildProcessModifier([this] { setupChildProcess_impl(); }); @@ -65,6 +68,8 @@ public: void setupChildProcess() override { setupChildProcess_impl(); } #endif + ProcessStartHandler *processStartHandler() { return &m_processStartHandler; } + using QProcess::setErrorString; void setLowPriority() { m_lowPriority = true; } @@ -74,6 +79,7 @@ private: void setupChildProcess_impl(); bool m_lowPriority = false; bool m_unixTerminalDisabled = false; + ProcessStartHandler m_processStartHandler; }; } // namespace Utils diff --git a/src/libs/utils/qtcprocess.cpp b/src/libs/utils/qtcprocess.cpp index 810f770e0e3..ca7f9f9bbff 100644 --- a/src/libs/utils/qtcprocess.cpp +++ b/src/libs/utils/qtcprocess.cpp @@ -314,17 +314,18 @@ public: { m_process->setWorkingDirectory(dir); } void start(const QString &program, const QStringList &arguments, const QByteArray &writeData) override { - m_processStartHandler.setProcessMode(processMode()); - m_processStartHandler.setWriteData(writeData); + ProcessStartHandler *handler = m_process->processStartHandler(); + handler->setProcessMode(processMode()); + handler->setWriteData(writeData); if (isBelowNormalPriority()) - m_processStartHandler.setBelowNormalPriority(m_process); - m_processStartHandler.setNativeArguments(m_process, nativeArguments()); + handler->setBelowNormalPriority(); + handler->setNativeArguments(nativeArguments()); if (isLowPriority()) m_process->setLowPriority(); if (isUnixTerminalDisabled()) m_process->setUnixTerminalDisabled(); - m_process->start(program, arguments, m_processStartHandler.openMode()); - m_processStartHandler.handleProcessStart(m_process); + m_process->start(program, arguments, handler->openMode()); + handler->handleProcessStart(); } void terminate() override { m_process->terminate(); } @@ -367,11 +368,10 @@ public: private: void handleStarted() { - m_processStartHandler.handleProcessStarted(m_process); + m_process->processStartHandler()->handleProcessStarted(); emit started(); } ProcessHelper *m_process; - ProcessStartHandler m_processStartHandler; }; static uint uniqueToken() diff --git a/src/tools/processlauncher/launchersockethandler.cpp b/src/tools/processlauncher/launchersockethandler.cpp index e803d182b60..21e7e7624e0 100644 --- a/src/tools/processlauncher/launchersockethandler.cpp +++ b/src/tools/processlauncher/launchersockethandler.cpp @@ -44,11 +44,9 @@ public: ProcessHelper(parent), m_token(token) { } quintptr token() const { return m_token; } - ProcessStartHandler *processStartHandler() { return &m_processStartHandler; } private: const quintptr m_token; - ProcessStartHandler m_processStartHandler; }; LauncherSocketHandler::LauncherSocketHandler(QString serverPath, QObject *parent) @@ -156,7 +154,7 @@ void LauncherSocketHandler::handleProcessStarted() Process *proc = senderProcess(); ProcessStartedPacket packet(proc->token()); packet.processId = proc->processId(); - proc->processStartHandler()->handleProcessStarted(proc); + proc->processStartHandler()->handleProcessStarted(); sendPacket(packet); } @@ -212,14 +210,14 @@ void LauncherSocketHandler::handleStartPacket() handler->setProcessMode(packet.processMode); handler->setWriteData(packet.writeData); if (packet.belowNormalPriority) - handler->setBelowNormalPriority(process); - handler->setNativeArguments(process, packet.nativeArguments); + handler->setBelowNormalPriority(); + handler->setNativeArguments(packet.nativeArguments); if (packet.lowPriority) process->setLowPriority(); if (packet.unixTerminalDisabled) process->setUnixTerminalDisabled(); process->start(packet.command, packet.arguments, handler->openMode()); - handler->handleProcessStart(process); + handler->handleProcessStart(); } void LauncherSocketHandler::handleWritePacket()