From 5d10f027218fe48b920f296c269162293bf55ff4 Mon Sep 17 00:00:00 2001 From: Robert Griebl Date: Tue, 12 Dec 2023 14:00:21 +0100 Subject: TSAN: fix data races in PackageManager These have been there for years and have never triggered, but now that TSAN found the races, it's time to fix them. Change-Id: I31fc3e9254d1a2145198621d55c812cd7226510b Pick-to: 6.6 6.5 Reviewed-by: Dominik Holland (cherry picked from commit 4cf77c5cc9dd569aee4a92a6b540d10cb928da82) Reviewed-by: Qt Cherry-pick Bot --- src/manager-lib/asynchronoustask.cpp | 23 ++++++++++++++--------- src/manager-lib/asynchronoustask.h | 3 +-- src/manager-lib/deinstallationtask.cpp | 11 +++++++---- src/manager-lib/deinstallationtask.h | 2 +- src/manager-lib/installationtask.cpp | 2 ++ src/manager-lib/packagemanager.cpp | 9 +++------ 6 files changed, 28 insertions(+), 22 deletions(-) diff --git a/src/manager-lib/asynchronoustask.cpp b/src/manager-lib/asynchronoustask.cpp index 17871da8..d750946f 100644 --- a/src/manager-lib/asynchronoustask.cpp +++ b/src/manager-lib/asynchronoustask.cpp @@ -25,29 +25,29 @@ QString AsynchronousTask::id() const AsynchronousTask::TaskState AsynchronousTask::state() const { + QMutexLocker locker(&m_mutex); return m_state; } void AsynchronousTask::setState(AsynchronousTask::TaskState state) { + QMutexLocker locker(&m_mutex); if (m_state != state) { m_state = state; - emit stateChanged(m_state); + locker.unlock(); + emit stateChanged(state); } } -bool AsynchronousTask::hasFailed() const -{ - return (m_state == Failed); -} - Error AsynchronousTask::errorCode() const { + QMutexLocker locker(&m_mutex); return m_errorCode; } QString AsynchronousTask::errorString() const { + QMutexLocker locker(&m_mutex); return m_errorString; } @@ -59,7 +59,7 @@ bool AsynchronousTask::cancel() bool AsynchronousTask::forceCancel() { - if (m_state == Queued) { + if (state() == Queued) { setError(Error::Canceled, qSL("canceled")); return true; } @@ -68,6 +68,7 @@ bool AsynchronousTask::forceCancel() QString AsynchronousTask::packageId() const { + QMutexLocker locker(&m_mutex); return m_packageId; } @@ -83,8 +84,12 @@ bool AsynchronousTask::postExecute() void AsynchronousTask::setError(Error errorCode, const QString &errorString) { - m_errorCode = errorCode; - m_errorString = errorString; + { + // setState() also locks + QMutexLocker locker(&m_mutex); + m_errorCode = errorCode; + m_errorString = errorString; + } setState(Failed); } diff --git a/src/manager-lib/asynchronoustask.h b/src/manager-lib/asynchronoustask.h index b3e3d678..b8b8f611 100644 --- a/src/manager-lib/asynchronoustask.h +++ b/src/manager-lib/asynchronoustask.h @@ -39,7 +39,6 @@ public: TaskState state() const; void setState(TaskState state); - bool hasFailed() const; Error errorCode() const; QString errorString() const; @@ -61,7 +60,7 @@ protected: void run() override final; protected: - QMutex m_mutex; + mutable QMutex m_mutex; QString m_id; QString m_packageId; diff --git a/src/manager-lib/deinstallationtask.cpp b/src/manager-lib/deinstallationtask.cpp index 7f0b778b..9b4f2d31 100644 --- a/src/manager-lib/deinstallationtask.cpp +++ b/src/manager-lib/deinstallationtask.cpp @@ -29,6 +29,7 @@ DeinstallationTask::DeinstallationTask(const QString &packageId, const QString & bool DeinstallationTask::cancel() { + QMutexLocker locker(&m_mutex); if (m_canBeCanceled) m_canceled = true; return m_canceled; @@ -70,10 +71,12 @@ void DeinstallationTask::execute() while (!m_canceled && !package->areAllApplicationsStoppedDueToBlock()) QThread::msleep(30); - // there's a small race condition here, but not doing a planned cancellation isn't harmful - m_canBeCanceled = false; - if (m_canceled) - throw Exception(Error::Canceled, "canceled"); + { + QMutexLocker locker(&m_mutex); + m_canBeCanceled = false; + if (m_canceled) + throw Exception(Error::Canceled, "canceled"); + } ScopedRenamer docDirRename; ScopedRenamer appDirRename; diff --git a/src/manager-lib/deinstallationtask.h b/src/manager-lib/deinstallationtask.h index 63e6613b..3159a3d1 100644 --- a/src/manager-lib/deinstallationtask.h +++ b/src/manager-lib/deinstallationtask.h @@ -30,7 +30,7 @@ private: QString m_documentPath; bool m_keepDocuments; bool m_canBeCanceled = true; - bool m_canceled = false; + QAtomicInteger m_canceled = false; // atomic for easy access in "busy" wait loop }; QT_END_NAMESPACE_AM diff --git a/src/manager-lib/installationtask.cpp b/src/manager-lib/installationtask.cpp index 1bf965a5..cebfb9d2 100644 --- a/src/manager-lib/installationtask.cpp +++ b/src/manager-lib/installationtask.cpp @@ -263,7 +263,9 @@ void InstallationTask::checkExtractedFile(const QString &file) Q_DECL_NOEXCEPT_E if (m_iconFileName.isEmpty()) throw Exception(Error::Package, "the 'icon' field in info.yaml cannot be empty or absent."); + m_mutex.lock(); m_packageId = m_package->id(); + m_mutex.unlock(); m_foundInfo = true; } else if (m_extractedFileCount == 2) { diff --git a/src/manager-lib/packagemanager.cpp b/src/manager-lib/packagemanager.cpp index 7b73f5b0..30731f83 100644 --- a/src/manager-lib/packagemanager.cpp +++ b/src/manager-lib/packagemanager.cpp @@ -1254,9 +1254,7 @@ void PackageManager::executeNextTask() AsynchronousTask *task = d->incomingTaskList.takeFirst(); - if (task->hasFailed()) { - task->setState(AsynchronousTask::Failed); - + if (task->state() == AsynchronousTask::Failed) { handleFailure(task); task->deleteLater(); @@ -1285,11 +1283,10 @@ void PackageManager::executeNextTask() }); connect(task, &AsynchronousTask::finished, this, [this, task]() { - task->setState(task->hasFailed() ? AsynchronousTask::Failed : AsynchronousTask::Finished); - - if (task->hasFailed()) { + if (task->state() == AsynchronousTask::Failed) { handleFailure(task); } else { + task->setState(AsynchronousTask::Finished); qCDebug(LogInstaller) << "emit finished" << task->id(); emit taskFinished(task->id()); } -- cgit v1.2.3