From: Christian Kamm <mail@ckamm.de>
Date: Wed, 7 Aug 2019 09:14:30 +0000 (+0200)
Subject: Checksums: Fix crash due to threading issue
X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~12^2~21^2~468^2~210
X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=c9dbe465424307ebd4de87429c825eb0567faacc;p=nextcloud-desktop.git

Checksums: Fix crash due to threading issue

The checksum computation thread was potentially using a QFile that was
deleted in the gui thread.

For #7368
---

diff --git a/src/common/checksums.cpp b/src/common/checksums.cpp
index 354b88f5c..28483046d 100644
--- a/src/common/checksums.cpp
+++ b/src/common/checksums.cpp
@@ -225,24 +225,25 @@ QByteArray ComputeChecksum::checksumType() const
 void ComputeChecksum::start(const QString &filePath)
 {
     qCInfo(lcChecksums) << "Computing" << checksumType() << "checksum of" << filePath << "in a thread";
-    _file.reset(new QFile(filePath));
-    if (!_file->open(QIODevice::ReadOnly)) {
-        qCWarning(lcChecksums) << "Could not open file" << filePath << "for reading to compute a checksum" << _file->errorString();
+    // make_unique() would be more appropriate, but QtConcurrent requires
+    // copyable types.
+    auto file = std::make_shared<QFile>(filePath);
+    if (!file->open(QIODevice::ReadOnly)) {
+        qCWarning(lcChecksums) << "Could not open file" << filePath << "for reading to compute a checksum" << file->errorString();
         emit done(QByteArray(), QByteArray());
         return;
     }
-    start(_file.get());
-}
-
-void ComputeChecksum::start(QIODevice *device)
-{
-    qCInfo(lcChecksums) << "Computing" << checksumType() << "checksum of iodevice in a thread";
 
-    // Calculate the checksum in a different thread first.
     connect(&_watcher, &QFutureWatcherBase::finished,
         this, &ComputeChecksum::slotCalculationDone,
         Qt::UniqueConnection);
-    _watcher.setFuture(QtConcurrent::run(ComputeChecksum::computeNow, device, checksumType()));
+
+    // Capturing "file" extends its lifetime to the lifetime of the new thread.
+    // Bug: The thread will keep running even if ComputeChecksum is deleted.
+    auto type = checksumType();
+    _watcher.setFuture(QtConcurrent::run([file, type]() {
+        return ComputeChecksum::computeNow(file.get(), type);
+    }));
 }
 
 QByteArray ComputeChecksum::computeNowOnFile(const QString &filePath, const QByteArray &checksumType)
@@ -289,9 +290,6 @@ QByteArray ComputeChecksum::computeNow(QIODevice *device, const QByteArray &chec
 
 void ComputeChecksum::slotCalculationDone()
 {
-    // Close the file and delete the instance
-    _file.reset(nullptr);
-
     QByteArray checksum = _watcher.future().result();
     if (!checksum.isNull()) {
         emit done(_checksumType, checksum);
@@ -333,12 +331,6 @@ void ValidateChecksumHeader::start(const QString &filePath, const QByteArray &ch
         calculator->start(filePath);
 }
 
-void ValidateChecksumHeader::start(QIODevice *device, const QByteArray &checksumHeader)
-{
-    if (auto calculator = prepareStart(checksumHeader))
-        calculator->start(device);
-}
-
 void ValidateChecksumHeader::slotChecksumCalculated(const QByteArray &checksumType,
     const QByteArray &checksum)
 {
diff --git a/src/common/checksums.h b/src/common/checksums.h
index e057fb7ad..fbd92b322 100644
--- a/src/common/checksums.h
+++ b/src/common/checksums.h
@@ -93,22 +93,10 @@ public:
 
     QByteArray checksumType() const;
 
-    /**
-     * Computes the checksum for given device.
-     *
-     * done() is emitted when the calculation finishes.
-     *
-     * Does not take ownership of the device.
-     * Does not call open() on the device.
-     */
-    void start(QIODevice *device);
-
     /**
      * Computes the checksum for the given file path.
      *
      * done() is emitted when the calculation finishes.
-     *
-     * Convenience wrapper for start(QIODevice*) above.
      */
     void start(const QString &filePath);
 
@@ -131,9 +119,6 @@ private slots:
 private:
     QByteArray _checksumType;
 
-    // The convenience wrapper may open a file and must close it too
-    std::unique_ptr<QFile> _file;
-
     // watcher for the checksum calculation thread
     QFutureWatcher<QByteArray> _watcher;
 };
@@ -154,16 +139,6 @@ public:
      * If no checksum is there, or if a correct checksum is there, the signal validated()
      * will be emitted. In case of any kind of error, the signal validationFailed() will
      * be emitted.
-     *
-     * Does not take ownership of the device.
-     * Does not call open() on the device.
-     */
-    void start(QIODevice *device, const QByteArray &checksumHeader);
-
-    /**
-     * Same as above but opening a file by path.
-     *
-     * Convenience function for start(QIODevice*) above
      */
     void start(const QString &filePath, const QByteArray &checksumHeader);
 
diff --git a/test/testchecksumvalidator.cpp b/test/testchecksumvalidator.cpp
index 827686600..5c9c3f37b 100644
--- a/test/testchecksumvalidator.cpp
+++ b/test/testchecksumvalidator.cpp
@@ -192,20 +192,20 @@ using namespace OCC::Utility;
 
         file->seek(0);
         _successDown = false;
-        vali->start(file, adler);
+        vali->start(_testfile, adler);
 
         QTRY_VERIFY(_successDown);
 
         _expectedError = QLatin1String("The downloaded file does not match the checksum, it will be resumed.");
         _errorSeen = false;
         file->seek(0);
-        vali->start(file, "Adler32:543345");
+        vali->start(_testfile, "Adler32:543345");
         QTRY_VERIFY(_errorSeen);
 
         _expectedError = QLatin1String("The checksum header contained an unknown checksum type 'Klaas32'");
         _errorSeen = false;
         file->seek(0);
-        vali->start(file, "Klaas32:543345");
+        vali->start(_testfile, "Klaas32:543345");
         QTRY_VERIFY(_errorSeen);
 
         delete vali;