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 <christian.kandeler@qt.io>

src/plugins/remotelinux/tarpackagecreationstep.cpp

@@ -14,11 +14,13 @@
 #include <projectexplorer/target.h>
 
 #include <utils/futuresynchronizer.h>
+#include <utils/runextensions.h>
 
 #include <QDateTime>
 #include <QDir>
 #include <QFile>
 #include <QFileInfo>
+#include <QFutureWatcher>
 
 #include <cstring>
 
@@ -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(const Utils::FilePath &tarFilePath, bool ignoreMissingFiles);
-    bool doPackage();
+    bool appendFile(QFile &tarFile, const QFileInfo &fileInfo, const QString &remoteFilePath,
-    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<DeployableFile> &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<bool>(this);
+    connect(watcher, &QFutureWatcher<bool>::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<DeployableFile> &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.toFSPathString());
-    QFile tarFile(tarFilePath.toString());
 
     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('/')
+        if (!appendFile(tarFile, fileInfo,
-                + fileInfo.fileName())) {
+                        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<char *>(&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(),
+    if (!writeHeader(tarFile, fileInfo, remoteFilePath, tarFilePath, &errorMessage)) {
-                     &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())
+        // TODO: replace with future interface
-            return false;
+//        if (isCanceled())
+//            return false;
     }
     if (file.error() != QFile::NoError) {
         raiseError(Tr::tr("Error reading file \"%1\": %2.").arg(nativePath, file.errorString()));