From 4e9eef339cfec37c72d165ec2d98b8ac456f9e27 Mon Sep 17 00:00:00 2001 From: Jarek Kobus Date: Tue, 8 Nov 2022 19:56:37 +0100 Subject: [PATCH] TarPackageCreationStep: Secure data access Don't read or modify private data from worker thread. Move Preparation of the deploy file list into the caller thread, as it is using e.g. pointer to Kit object, what doesn't look to be safe when used from non-main thread. Change-Id: I6523e8f46541f5f96d46fa98f0ba941c2ec46e74 Reviewed-by: Christian Kandeler --- .../remotelinux/tarpackagecreationstep.cpp | 121 ++++++++++-------- 1 file changed, 67 insertions(+), 54 deletions(-) diff --git a/src/plugins/remotelinux/tarpackagecreationstep.cpp b/src/plugins/remotelinux/tarpackagecreationstep.cpp index 6f33f70e665..67b3bb063e0 100644 --- a/src/plugins/remotelinux/tarpackagecreationstep.cpp +++ b/src/plugins/remotelinux/tarpackagecreationstep.cpp @@ -14,11 +14,13 @@ #include #include +#include #include #include #include #include +#include #include @@ -62,6 +64,7 @@ public: private: bool init() final; void doRun() final; + void doCancel() final; bool fromMap(const QVariantMap &map) final; QVariantMap toMap() const final; QVariant data(Id id) const final; @@ -71,11 +74,11 @@ private: bool isPackagingNeeded() const; void deployFinished(bool success); void addNeededDeploymentFiles(const DeployableFile &deployable, const Kit *kit); - bool runImpl(); - bool doPackage(); - bool appendFile(QFile &tarFile, const QFileInfo &fileInfo, const QString &remoteFilePath); + bool doPackage(const Utils::FilePath &tarFilePath, bool ignoreMissingFiles); + bool appendFile(QFile &tarFile, const QFileInfo &fileInfo, const QString &remoteFilePath, + const Utils::FilePath &tarFilePath, bool ignoreMissingFiles); - FilePath m_cachedPackageFilePath; + FilePath m_tarFilePath; bool m_deploymentDataModified = false; DeploymentTimeInfo m_deployTimes; BoolAspect *m_incrementalDeploymentAspect = nullptr; @@ -123,14 +126,53 @@ FilePath TarPackageCreationStep::packageFilePath() const bool TarPackageCreationStep::init() { - m_cachedPackageFilePath = packageFilePath(); + m_tarFilePath = packageFilePath(); m_packagingNeeded = isPackagingNeeded(); return true; } void TarPackageCreationStep::doRun() { - m_synchronizer.addFuture(runInThread([this] { return runImpl(); })); + const QList &files = target()->deploymentData().allFiles(); + + if (m_incrementalDeploymentAspect->value()) { + m_files.clear(); + for (const DeployableFile &file : files) + addNeededDeploymentFiles(file, kit()); + } else { + m_files = files; + } + + emit addOutput(Tr::tr("Creating tarball..."), OutputFormat::NormalMessage); + if (!m_packagingNeeded) { + emit addOutput(Tr::tr("Tarball up to date, skipping packaging."), OutputFormat::NormalMessage); + emit finished(true); + return; + } + + auto * const watcher = new QFutureWatcher(this); + connect(watcher, &QFutureWatcher::finished, this, [this, watcher] { + const bool success = watcher->result(); + if (success) { + m_deploymentDataModified = false; + emit addOutput(Tr::tr("Packaging finished successfully."), OutputFormat::NormalMessage); + } else { + emit addOutput(Tr::tr("Packaging failed."), OutputFormat::ErrorMessage); + } + emit finished(success); + watcher->deleteLater(); + connect(BuildManager::instance(), &BuildManager::buildQueueFinished, + this, &TarPackageCreationStep::deployFinished); + }); + auto future = Utils::runAsync(&TarPackageCreationStep::doPackage, this, + m_tarFilePath, m_ignoreMissingFilesAspect->value()); + watcher->setFuture(future); + m_synchronizer.addFuture(future); +} + +void TarPackageCreationStep::doCancel() +{ + m_synchronizer.waitForFinished(); } bool TarPackageCreationStep::fromMap(const QVariantMap &map) @@ -226,48 +268,14 @@ void TarPackageCreationStep::addNeededDeploymentFiles( } } -bool TarPackageCreationStep::runImpl() +bool TarPackageCreationStep::doPackage(const FilePath &tarFilePath, bool ignoreMissingFiles) { - const QList &files = target()->deploymentData().allFiles(); - - if (m_incrementalDeploymentAspect->value()) { - m_files.clear(); - for (const DeployableFile &file : files) - addNeededDeploymentFiles(file, kit()); - } else { - m_files = files; - } - - const bool success = doPackage(); - - if (success) { - m_deploymentDataModified = false; - emit addOutput(Tr::tr("Packaging finished successfully."), OutputFormat::NormalMessage); - } else { - emit addOutput(Tr::tr("Packaging failed."), OutputFormat::ErrorMessage); - } - - connect(BuildManager::instance(), &BuildManager::buildQueueFinished, - this, &TarPackageCreationStep::deployFinished); - - return success; -} - -bool TarPackageCreationStep::doPackage() -{ - emit addOutput(Tr::tr("Creating tarball..."), OutputFormat::NormalMessage); - if (!m_packagingNeeded) { - emit addOutput(Tr::tr("Tarball up to date, skipping packaging."), OutputFormat::NormalMessage); - return true; - } - // TODO: Optimization: Only package changed files - const FilePath tarFilePath = m_cachedPackageFilePath; - QFile tarFile(tarFilePath.toString()); + QFile tarFile(tarFilePath.toFSPathString()); if (!tarFile.open(QIODevice::WriteOnly | QIODevice::Truncate)) { raiseError(Tr::tr("Error: tar file %1 cannot be opened (%2).") - .arg(tarFilePath.toUserOutput(), tarFile.errorString())); + .arg(tarFilePath.toUserOutput(), tarFile.errorString())); return false; } @@ -278,8 +286,9 @@ bool TarPackageCreationStep::doPackage() continue; } QFileInfo fileInfo = d.localFilePath().toFileInfo(); - if (!appendFile(tarFile, fileInfo, d.remoteDirectory() + QLatin1Char('/') - + fileInfo.fileName())) { + if (!appendFile(tarFile, fileInfo, + d.remoteDirectory() + QLatin1Char('/') + fileInfo.fileName(), + tarFilePath, ignoreMissingFiles)) { return false; } } @@ -315,7 +324,7 @@ static bool setFilePath(TarFileHeader &header, const QByteArray &filePath) } static bool writeHeader(QFile &tarFile, const QFileInfo &fileInfo, const QString &remoteFilePath, - const QString &cachedPackageFilePath, QString *errorMessage) + const FilePath &tarFilePath, QString *errorMessage) { TarFileHeader header; std::memset(&header, '\0', sizeof header); @@ -367,18 +376,19 @@ static bool writeHeader(QFile &tarFile, const QFileInfo &fileInfo, const QString header.chksum[sizeof header.chksum-1] = 0; if (!tarFile.write(reinterpret_cast(&header), sizeof header)) { *errorMessage = Tr::tr("Error writing tar file \"%1\": %2") - .arg(cachedPackageFilePath, tarFile.errorString()); + .arg(tarFilePath.toUserOutput(), tarFile.errorString()); return false; } return true; } bool TarPackageCreationStep::appendFile(QFile &tarFile, const QFileInfo &fileInfo, - const QString &remoteFilePath) + const QString &remoteFilePath, + const FilePath &tarFilePath, + bool ignoreMissingFiles) { QString errorMessage; - if (!writeHeader(tarFile, fileInfo, remoteFilePath, m_cachedPackageFilePath.toUserOutput(), - &errorMessage)) { + if (!writeHeader(tarFile, fileInfo, remoteFilePath, tarFilePath, &errorMessage)) { raiseError(errorMessage); return false; } @@ -388,8 +398,10 @@ bool TarPackageCreationStep::appendFile(QFile &tarFile, const QFileInfo &fileInf for (const QString &fileName : files) { const QString thisLocalFilePath = dir.path() + QLatin1Char('/') + fileName; const QString thisRemoteFilePath = remoteFilePath + QLatin1Char('/') + fileName; - if (!appendFile(tarFile, QFileInfo(thisLocalFilePath), thisRemoteFilePath)) + if (!appendFile(tarFile, QFileInfo(thisLocalFilePath), thisRemoteFilePath, + tarFilePath, ignoreMissingFiles)) { return false; + } } return true; } @@ -399,7 +411,7 @@ bool TarPackageCreationStep::appendFile(QFile &tarFile, const QFileInfo &fileInf if (!file.open(QIODevice::ReadOnly)) { const QString message = Tr::tr("Error reading file \"%1\": %2.") .arg(nativePath, file.errorString()); - if (m_ignoreMissingFilesAspect->value()) { + if (ignoreMissingFiles) { raiseWarning(message); return true; } else { @@ -417,8 +429,9 @@ bool TarPackageCreationStep::appendFile(QFile &tarFile, const QFileInfo &fileInf while (!file.atEnd() && file.error() == QFile::NoError && tarFile.error() == QFile::NoError) { const QByteArray data = file.read(chunkSize); tarFile.write(data); - if (isCanceled()) - return false; + // TODO: replace with future interface +// if (isCanceled()) +// return false; } if (file.error() != QFile::NoError) { raiseError(Tr::tr("Error reading file \"%1\": %2.").arg(nativePath, file.errorString()));