Fix: Use after free collecting tasks from ResourceDownloadDialogs (#3670)

launcher/Application.cpp

@@ -96,6 +96,7 @@
 #include <QList>
 #include <QNetworkAccessManager>
 #include <QStringList>
+#include <QStringLiteral>
 #include <QStyleFactory>
 #include <QTranslator>
 #include <QWindow>
@@ -125,7 +126,6 @@
 #include <FileSystem.h>
 #include <LocalPeer.h>
 
-#include <QStringLiteral>
 #include <stdlib.h>
 #include <sys.h>
 #include "SysInfo.h"

launcher/qtlogging.ini

@@ -3,6 +3,9 @@
 # prevent log spam and strange bugs
 # qt.qpa.drawing in particular causes theme artifacts on MacOS
 qt.*.debug=false
+# supress image format noise
+kf.imageformats.plugins.hdr=false
+kf.imageformats.plugins.xcf=false
 # don't log credentials by default
 launcher.auth.credentials.debug=false
 # remove the debug lines, other log levels still get through

launcher/ui/pages/instance/ModFolderPage.cpp

@@ -48,6 +48,7 @@
 #include <QMessageBox>
 #include <QSortFilterProxyModel>
 #include <algorithm>
+#include <memory>
 
 #include "Application.h"
 
@@ -145,10 +146,17 @@ void ModFolderPage::downloadMods()
         QMessageBox::critical(this, tr("Error"), tr("Please install a mod loader first!"));
         return;
     }
-    auto mdownload = new ResourceDownload::ModDownloadDialog(this, m_model, m_instance);
-    mdownload->setAttribute(Qt::WA_DeleteOnClose);
-    connect(this, &QObject::destroyed, mdownload, &QDialog::close);
-    if (mdownload->exec()) {
+
+    m_downloadDialog = new ResourceDownload::ModDownloadDialog(this, m_model, m_instance);
+    connect(this, &QObject::destroyed, m_downloadDialog, &QDialog::close);
+    connect(m_downloadDialog, &QDialog::finished, this, &ModFolderPage::downloadDialogFinished);
+
+    m_downloadDialog->open();
+}
+
+void ModFolderPage::downloadDialogFinished(int result)
+{
+    if (result) {
         auto tasks = new ConcurrentTask(tr("Download Mods"), APPLICATION->settings()->get("NumberOfConcurrentDownloads").toInt());
         connect(tasks, &Task::failed, [this, tasks](QString reason) {
             CustomMessageBox::selectable(this, tr("Error"), reason, QMessageBox::Critical)->show();
@@ -166,8 +174,12 @@ void ModFolderPage::downloadMods()
             tasks->deleteLater();
         });
 
-        for (auto& task : mdownload->getTasks()) {
-            tasks->addTask(task);
+        if (m_downloadDialog) {
+            for (auto& task : m_downloadDialog->getTasks()) {
+                tasks->addTask(task);
+            }
+        } else {
+            qWarning() << "ResourceDownloadDialog vanished before we could collect tasks!";
         }
 
         ProgressDialog loadDialog(this);
@@ -176,6 +188,8 @@ void ModFolderPage::downloadMods()
 
         m_model->update();
     }
+    if (m_downloadDialog)
+        m_downloadDialog->deleteLater();
 }
 
 void ModFolderPage::updateMods(bool includeDeps)
@@ -301,38 +315,12 @@ void ModFolderPage::changeModVersion()
     if (mods_list.length() != 1 || mods_list[0]->metadata() == nullptr)
         return;
 
-    auto mdownload = new ResourceDownload::ModDownloadDialog(this, m_model, m_instance);
-    mdownload->setAttribute(Qt::WA_DeleteOnClose);
-    connect(this, &QObject::destroyed, mdownload, &QDialog::close);
-    mdownload->setResourceMetadata((*mods_list.begin())->metadata());
-    if (mdownload->exec()) {
-        auto tasks = new ConcurrentTask("Download Mods", APPLICATION->settings()->get("NumberOfConcurrentDownloads").toInt());
-        connect(tasks, &Task::failed, [this, tasks](QString reason) {
-            CustomMessageBox::selectable(this, tr("Error"), reason, QMessageBox::Critical)->show();
-            tasks->deleteLater();
-        });
-        connect(tasks, &Task::aborted, [this, tasks]() {
-            CustomMessageBox::selectable(this, tr("Aborted"), tr("Download stopped by user."), QMessageBox::Information)->show();
-            tasks->deleteLater();
-        });
-        connect(tasks, &Task::succeeded, [this, tasks]() {
-            QStringList warnings = tasks->warnings();
-            if (warnings.count())
-                CustomMessageBox::selectable(this, tr("Warnings"), warnings.join('\n'), QMessageBox::Warning)->show();
+    m_downloadDialog = new ResourceDownload::ModDownloadDialog(this, m_model, m_instance);
+    connect(this, &QObject::destroyed, m_downloadDialog, &QDialog::close);
+    connect(m_downloadDialog, &QDialog::finished, this, &ModFolderPage::downloadDialogFinished);
 
-            tasks->deleteLater();
-        });
-
-        for (auto& task : mdownload->getTasks()) {
-            tasks->addTask(task);
-        }
-
-        ProgressDialog loadDialog(this);
-        loadDialog.setSkipButton(true, tr("Abort"));
-        loadDialog.execWithTask(tasks);
-
-        m_model->update();
-    }
+    m_downloadDialog->setResourceMetadata((*mods_list.begin())->metadata());
+    m_downloadDialog->open();
 }
 
 void ModFolderPage::exportModMetadata()

launcher/ui/pages/instance/ModFolderPage.h

@@ -38,7 +38,9 @@
 
 #pragma once
 
+#include <QPointer>
 #include "ExternalResourcesPage.h"
+#include "ui/dialogs/ResourceDownloadDialog.h"
 
 class ModFolderPage : public ExternalResourcesPage {
     Q_OBJECT
@@ -63,6 +65,7 @@ class ModFolderPage : public ExternalResourcesPage {
     void removeItems(const QItemSelection& selection) override;
 
     void downloadMods();
+    void downloadDialogFinished(int result);
     void updateMods(bool includeDeps = false);
     void deleteModMetadata();
     void exportModMetadata();
@@ -70,6 +73,7 @@ class ModFolderPage : public ExternalResourcesPage {
 
    protected:
     std::shared_ptr<ModFolderModel> m_model;
+    QPointer<ResourceDownload::ModDownloadDialog> m_downloadDialog;
 };
 
 class CoreModFolderPage : public ModFolderPage {

launcher/ui/pages/instance/ResourcePackPage.cpp

@@ -84,10 +84,16 @@ void ResourcePackPage::downloadResourcePacks()
     if (m_instance->typeName() != "Minecraft")
         return;  // this is a null instance or a legacy instance
 
-    auto mdownload = new ResourceDownload::ResourcePackDownloadDialog(this, m_model, m_instance);
-    mdownload->setAttribute(Qt::WA_DeleteOnClose);
-    connect(this, &QObject::destroyed, mdownload, &QDialog::close);
-    if (mdownload->exec()) {
+    m_downloadDialog = new ResourceDownload::ResourcePackDownloadDialog(this, m_model, m_instance);
+    connect(this, &QObject::destroyed, m_downloadDialog, &QDialog::close);
+    connect(m_downloadDialog, &QDialog::finished, this, &ResourcePackPage::downloadDialogFinished);
+
+    m_downloadDialog->open();
+}
+
+void ResourcePackPage::downloadDialogFinished(int result)
+{
+    if (result) {
         auto tasks = new ConcurrentTask("Download Resource Pack", APPLICATION->settings()->get("NumberOfConcurrentDownloads").toInt());
         connect(tasks, &Task::failed, [this, tasks](QString reason) {
             CustomMessageBox::selectable(this, tr("Error"), reason, QMessageBox::Critical)->show();
@@ -105,8 +111,12 @@ void ResourcePackPage::downloadResourcePacks()
             tasks->deleteLater();
         });
 
-        for (auto& task : mdownload->getTasks()) {
-            tasks->addTask(task);
+        if (m_downloadDialog) {
+            for (auto& task : m_downloadDialog->getTasks()) {
+                tasks->addTask(task);
+            }
+        } else {
+            qWarning() << "ResourceDownloadDialog vanished before we could collect tasks!";
         }
 
         ProgressDialog loadDialog(this);
@@ -115,6 +125,8 @@ void ResourcePackPage::downloadResourcePacks()
 
         m_model->update();
     }
+    if (m_downloadDialog)
+        m_downloadDialog->deleteLater();
 }
 
 void ResourcePackPage::updateResourcePacks()
@@ -237,36 +249,10 @@ void ResourcePackPage::changeResourcePackVersion()
     if (resource.metadata() == nullptr)
         return;
 
-    auto mdownload = new ResourceDownload::ResourcePackDownloadDialog(this, m_model, m_instance);
-    mdownload->setAttribute(Qt::WA_DeleteOnClose);
-    connect(this, &QObject::destroyed, mdownload, &QDialog::close);
-    mdownload->setResourceMetadata(resource.metadata());
-    if (mdownload->exec()) {
-        auto tasks = new ConcurrentTask("Download Resource Packs", APPLICATION->settings()->get("NumberOfConcurrentDownloads").toInt());
-        connect(tasks, &Task::failed, [this, tasks](QString reason) {
-            CustomMessageBox::selectable(this, tr("Error"), reason, QMessageBox::Critical)->show();
-            tasks->deleteLater();
-        });
-        connect(tasks, &Task::aborted, [this, tasks]() {
-            CustomMessageBox::selectable(this, tr("Aborted"), tr("Download stopped by user."), QMessageBox::Information)->show();
-            tasks->deleteLater();
-        });
-        connect(tasks, &Task::succeeded, [this, tasks]() {
-            QStringList warnings = tasks->warnings();
-            if (warnings.count())
-                CustomMessageBox::selectable(this, tr("Warnings"), warnings.join('\n'), QMessageBox::Warning)->show();
+    m_downloadDialog = new ResourceDownload::ResourcePackDownloadDialog(this, m_model, m_instance);
+    connect(this, &QObject::destroyed, m_downloadDialog, &QDialog::close);
+    connect(m_downloadDialog, &QDialog::finished, this, &ResourcePackPage::downloadDialogFinished);
 
-            tasks->deleteLater();
-        });
-
-        for (auto& task : mdownload->getTasks()) {
-            tasks->addTask(task);
-        }
-
-        ProgressDialog loadDialog(this);
-        loadDialog.setSkipButton(true, tr("Abort"));
-        loadDialog.execWithTask(tasks);
-
-        m_model->update();
-    }
-}
+    m_downloadDialog->setResourceMetadata(resource.metadata());
+    m_downloadDialog->open();
+}

launcher/ui/pages/instance/ResourcePackPage.h

@@ -37,7 +37,10 @@
 
 #pragma once
 
+#include <QPointer>
+
 #include "ExternalResourcesPage.h"
+#include "ui/dialogs/ResourceDownloadDialog.h"
 #include "ui_ExternalResourcesPage.h"
 
 #include "minecraft/mod/ResourcePackFolderModel.h"
@@ -62,10 +65,12 @@ class ResourcePackPage : public ExternalResourcesPage {
 
    private slots:
     void downloadResourcePacks();
+    void downloadDialogFinished(int result);
     void updateResourcePacks();
     void deleteResourcePackMetadata();
     void changeResourcePackVersion();
 
    protected:
     std::shared_ptr<ResourcePackFolderModel> m_model;
+    QPointer<ResourceDownload::ResourceDownloadDialog> m_downloadDialog;
 };

launcher/ui/pages/instance/ShaderPackPage.cpp

@@ -81,10 +81,16 @@ void ShaderPackPage::downloadShaderPack()
     if (m_instance->typeName() != "Minecraft")
         return;  // this is a null instance or a legacy instance
 
-    auto mdownload = new ResourceDownload::ShaderPackDownloadDialog(this, m_model, m_instance);
-    mdownload->setAttribute(Qt::WA_DeleteOnClose);
-    connect(this, &QObject::destroyed, mdownload, &QDialog::close);
-    if (mdownload->exec()) {
+    m_downloadDialog = new ResourceDownload::ShaderPackDownloadDialog(this, m_model, m_instance);
+    connect(this, &QObject::destroyed, m_downloadDialog, &QDialog::close);
+    connect(m_downloadDialog, &QDialog::finished, this, &ShaderPackPage::downloadDialogFinished);
+
+    m_downloadDialog->open();
+}
+
+void ShaderPackPage::downloadDialogFinished(int result)
+{
+    if (result) {
         auto tasks = new ConcurrentTask("Download Shader Packs", APPLICATION->settings()->get("NumberOfConcurrentDownloads").toInt());
         connect(tasks, &Task::failed, [this, tasks](QString reason) {
             CustomMessageBox::selectable(this, tr("Error"), reason, QMessageBox::Critical)->show();
@@ -102,8 +108,12 @@ void ShaderPackPage::downloadShaderPack()
             tasks->deleteLater();
         });
 
-        for (auto& task : mdownload->getTasks()) {
-            tasks->addTask(task);
+        if (m_downloadDialog) {
+            for (auto& task : m_downloadDialog->getTasks()) {
+                tasks->addTask(task);
+            }
+        } else {
+            qWarning() << "ResourceDownloadDialog vanished before we could collect tasks!";
         }
 
         ProgressDialog loadDialog(this);
@@ -112,6 +122,8 @@ void ShaderPackPage::downloadShaderPack()
 
         m_model->update();
     }
+    if (m_downloadDialog)
+        m_downloadDialog->deleteLater();
 }
 
 void ShaderPackPage::updateShaderPacks()
@@ -234,36 +246,10 @@ void ShaderPackPage::changeShaderPackVersion()
     if (resource.metadata() == nullptr)
         return;
 
-    auto mdownload = new ResourceDownload::ShaderPackDownloadDialog(this, m_model, m_instance);
-    mdownload->setAttribute(Qt::WA_DeleteOnClose);
-    connect(this, &QObject::destroyed, mdownload, &QDialog::close);
-    mdownload->setResourceMetadata(resource.metadata());
-    if (mdownload->exec()) {
-        auto tasks = new ConcurrentTask("Download Shader Packs", APPLICATION->settings()->get("NumberOfConcurrentDownloads").toInt());
-        connect(tasks, &Task::failed, [this, tasks](QString reason) {
-            CustomMessageBox::selectable(this, tr("Error"), reason, QMessageBox::Critical)->show();
-            tasks->deleteLater();
-        });
-        connect(tasks, &Task::aborted, [this, tasks]() {
-            CustomMessageBox::selectable(this, tr("Aborted"), tr("Download stopped by user."), QMessageBox::Information)->show();
-            tasks->deleteLater();
-        });
-        connect(tasks, &Task::succeeded, [this, tasks]() {
-            QStringList warnings = tasks->warnings();
-            if (warnings.count())
-                CustomMessageBox::selectable(this, tr("Warnings"), warnings.join('\n'), QMessageBox::Warning)->show();
+    m_downloadDialog = new ResourceDownload::ShaderPackDownloadDialog(this, m_model, m_instance);
+    connect(this, &QObject::destroyed, m_downloadDialog, &QDialog::close);
+    connect(m_downloadDialog, &QDialog::finished, this, &ShaderPackPage::downloadDialogFinished);
 
-            tasks->deleteLater();
-        });
-
-        for (auto& task : mdownload->getTasks()) {
-            tasks->addTask(task);
-        }
-
-        ProgressDialog loadDialog(this);
-        loadDialog.setSkipButton(true, tr("Abort"));
-        loadDialog.execWithTask(tasks);
-
-        m_model->update();
-    }
+    m_downloadDialog->setResourceMetadata(resource.metadata());
+    m_downloadDialog->open();
 }

launcher/ui/pages/instance/ShaderPackPage.h

@@ -37,7 +37,9 @@
 
 #pragma once
 
+#include <QPointer>
 #include "ExternalResourcesPage.h"
+#include "ui/dialogs/ResourceDownloadDialog.h"
 
 class ShaderPackPage : public ExternalResourcesPage {
     Q_OBJECT
@@ -54,10 +56,12 @@ class ShaderPackPage : public ExternalResourcesPage {
 
    public slots:
     void downloadShaderPack();
+    void downloadDialogFinished(int result);
     void updateShaderPacks();
     void deleteShaderPackMetadata();
     void changeShaderPackVersion();
 
    private:
     std::shared_ptr<ShaderPackFolderModel> m_model;
+    QPointer<ResourceDownload::ShaderPackDownloadDialog> m_downloadDialog;
 };

launcher/ui/pages/instance/TexturePackPage.cpp

@@ -90,10 +90,15 @@ void TexturePackPage::downloadTexturePacks()
     if (m_instance->typeName() != "Minecraft")
         return;  // this is a null instance or a legacy instance
 
-    auto mdownload = new ResourceDownload::TexturePackDownloadDialog(this, m_model, m_instance);
-    mdownload->setAttribute(Qt::WA_DeleteOnClose);
-    connect(this, &QObject::destroyed, mdownload, &QDialog::close);
-    if (mdownload->exec()) {
+    m_downloadDialog = new ResourceDownload::TexturePackDownloadDialog(this, m_model, m_instance);
+    connect(this, &QObject::destroyed, m_downloadDialog, &QDialog::close);
+    connect(m_downloadDialog, &QDialog::finished, this, &TexturePackPage::downloadDialogFinished);
+    m_downloadDialog->open();
+}
+
+void TexturePackPage::downloadDialogFinished(int result)
+{
+    if (result) {
         auto tasks = new ConcurrentTask("Download Texture Packs", APPLICATION->settings()->get("NumberOfConcurrentDownloads").toInt());
         connect(tasks, &Task::failed, [this, tasks](QString reason) {
             CustomMessageBox::selectable(this, tr("Error"), reason, QMessageBox::Critical)->show();
@@ -111,8 +116,12 @@ void TexturePackPage::downloadTexturePacks()
             tasks->deleteLater();
         });
 
-        for (auto& task : mdownload->getTasks()) {
-            tasks->addTask(task);
+        if (m_downloadDialog) {
+            for (auto& task : m_downloadDialog->getTasks()) {
+                tasks->addTask(task);
+            }
+        } else {
+            qWarning() << "ResourceDownloadDialog vanished before we could collect tasks!";
         }
 
         ProgressDialog loadDialog(this);
@@ -121,6 +130,8 @@ void TexturePackPage::downloadTexturePacks()
 
         m_model->update();
     }
+    if (m_downloadDialog)
+        m_downloadDialog->deleteLater();
 }
 
 void TexturePackPage::updateTexturePacks()
@@ -243,36 +254,10 @@ void TexturePackPage::changeTexturePackVersion()
     if (resource.metadata() == nullptr)
         return;
 
-    auto mdownload = new ResourceDownload::TexturePackDownloadDialog(this, m_model, m_instance);
-    mdownload->setAttribute(Qt::WA_DeleteOnClose);
-    connect(this, &QObject::destroyed, mdownload, &QDialog::close);
-    mdownload->setResourceMetadata(resource.metadata());
-    if (mdownload->exec()) {
-        auto tasks = new ConcurrentTask("Download Texture Packs", APPLICATION->settings()->get("NumberOfConcurrentDownloads").toInt());
-        connect(tasks, &Task::failed, [this, tasks](QString reason) {
-            CustomMessageBox::selectable(this, tr("Error"), reason, QMessageBox::Critical)->show();
-            tasks->deleteLater();
-        });
-        connect(tasks, &Task::aborted, [this, tasks]() {
-            CustomMessageBox::selectable(this, tr("Aborted"), tr("Download stopped by user."), QMessageBox::Information)->show();
-            tasks->deleteLater();
-        });
-        connect(tasks, &Task::succeeded, [this, tasks]() {
-            QStringList warnings = tasks->warnings();
-            if (warnings.count())
-                CustomMessageBox::selectable(this, tr("Warnings"), warnings.join('\n'), QMessageBox::Warning)->show();
+    m_downloadDialog = new ResourceDownload::TexturePackDownloadDialog(this, m_model, m_instance);
+    connect(this, &QObject::destroyed, m_downloadDialog, &QDialog::close);
+    connect(m_downloadDialog, &QDialog::finished, this, &TexturePackPage::downloadDialogFinished);
 
-            tasks->deleteLater();
-        });
-
-        for (auto& task : mdownload->getTasks()) {
-            tasks->addTask(task);
-        }
-
-        ProgressDialog loadDialog(this);
-        loadDialog.setSkipButton(true, tr("Abort"));
-        loadDialog.execWithTask(tasks);
-
-        m_model->update();
-    }
+    m_downloadDialog->setResourceMetadata(resource.metadata());
+    m_downloadDialog->open();
 }

launcher/ui/pages/instance/TexturePackPage.h

@@ -37,7 +37,10 @@
 
 #pragma once
 
+#include <QPointer>
+
 #include "ExternalResourcesPage.h"
+#include "ui/dialogs/ResourceDownloadDialog.h"
 #include "ui_ExternalResourcesPage.h"
 
 #include "minecraft/mod/TexturePackFolderModel.h"
@@ -57,10 +60,12 @@ class TexturePackPage : public ExternalResourcesPage {
    public slots:
     void updateFrame(const QModelIndex& current, const QModelIndex& previous) override;
     void downloadTexturePacks();
+    void downloadDialogFinished(int result);
     void updateTexturePacks();
     void deleteTexturePackMetadata();
     void changeTexturePackVersion();
 
    private:
     std::shared_ptr<TexturePackFolderModel> m_model;
+    QPointer<ResourceDownload::TexturePackDownloadDialog> m_downloadDialog;
 };