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 354b88f5cfe35fba517e11d20487b0ac17720990..28483046db48e9067b415e06b875b8de550221c3 100644 (file)
--- 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 e057fb7ad1a4e2129b7094a791e21a0386a4c1ff..fbd92b3223774b7fdca9bc989c6216f2e17586d5 100644 (file)
--- 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 827686600eea2d38beca2327f0fed3bf1fdc0e33..5c9c3f37b95a6270cf981e2cb653e1db717633f2 100644 (file)
--- 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;