Merge pull request #2964 from PrismLauncher/backport-2958-to-release-9.x

[Backport release-9.x] skip parsing open QSaveFile temprary files as resources

launcher/Application.cpp

@@ -1883,3 +1883,31 @@ const QString Application::javaPath()
 {
     return m_settings->get("JavaDir").toString();
 }
+
+void Application::addQSavePath(QString path)
+{
+    QMutexLocker locker(&m_qsaveResourcesMutex);
+    m_qsaveResources[path] = m_qsaveResources.value(path, 0) + 1;
+}
+
+void Application::removeQSavePath(QString path)
+{
+    QMutexLocker locker(&m_qsaveResourcesMutex);
+    auto count = m_qsaveResources.value(path, 0) - 1;
+    if (count <= 0) {
+        m_qsaveResources.remove(path);
+    } else {
+        m_qsaveResources[path] = count;
+    }
+}
+
+bool Application::checkQSavePath(QString path)
+{
+    QMutexLocker locker(&m_qsaveResourcesMutex);
+    for (auto partialPath : m_qsaveResources.keys()) {
+        if (path.startsWith(partialPath) && m_qsaveResources.value(partialPath, 0) > 0) {
+            return true;
+        }
+    }
+    return false;
+}

launcher/Application.h

@@ -42,6 +42,7 @@
 #include <QDebug>
 #include <QFlag>
 #include <QIcon>
+#include <QMutex>
 #include <QUrl>
 #include <memory>
 
@@ -303,4 +304,13 @@ class Application : public QApplication {
     QList<QUrl> m_urlsToImport;
     QString m_instanceIdToShowWindowOf;
     std::unique_ptr<QFile> logFile;
+
+   public:
+    void addQSavePath(QString);
+    void removeQSavePath(QString);
+    bool checkQSavePath(QString);
+
+   private:
+    QHash<QString, int> m_qsaveResources;
+    mutable QMutex m_qsaveResourcesMutex;
 };

launcher/CMakeLists.txt

@@ -30,6 +30,7 @@ set(CORE_SOURCES
     StringUtils.cpp
     QVariantUtils.h
     RuntimeContext.h
+    PSaveFile.h
 
     # Basic instance manipulation tasks (derived from InstanceTask)
     InstanceCreationTask.h

launcher/FileSystem.cpp

@@ -45,7 +45,6 @@
 #include <QDirIterator>
 #include <QFile>
 #include <QFileInfo>
-#include <QSaveFile>
 #include <QStandardPaths>
 #include <QStorageInfo>
 #include <QTextStream>
@@ -54,6 +53,7 @@
 #include <system_error>
 
 #include "DesktopServices.h"
+#include "PSaveFile.h"
 #include "StringUtils.h"
 
 #if defined Q_OS_WIN32
@@ -191,8 +191,8 @@ void ensureExists(const QDir& dir)
 void write(const QString& filename, const QByteArray& data)
 {
     ensureExists(QFileInfo(filename).dir());
-    QSaveFile file(filename);
+    PSaveFile file(filename);
-    if (!file.open(QSaveFile::WriteOnly)) {
+    if (!file.open(PSaveFile::WriteOnly)) {
         throw FileSystemException("Couldn't open " + filename + " for writing: " + file.errorString());
     }
     if (data.size() != file.write(data)) {
@@ -213,8 +213,8 @@ void appendSafe(const QString& filename, const QByteArray& data)
         buffer = QByteArray();
     }
     buffer.append(data);
-    QSaveFile file(filename);
+    PSaveFile file(filename);
-    if (!file.open(QSaveFile::WriteOnly)) {
+    if (!file.open(PSaveFile::WriteOnly)) {
         throw FileSystemException("Couldn't open " + filename + " for writing: " + file.errorString());
     }
     if (buffer.size() != file.write(buffer)) {
@@ -971,8 +971,7 @@ bool createShortcut(QString destination, QString target, QStringList args, QStri
     if (!args.empty())
         argstring = " \"" + args.join("\" \"") + "\"";
 
-    stream << "#!/bin/bash"
+    stream << "#!/bin/bash" << "\n";
-           << "\n";
     stream << "\"" << target << "\" " << argstring << "\n";
 
     stream.flush();
@@ -1016,12 +1015,9 @@ bool createShortcut(QString destination, QString target, QStringList args, QStri
     if (!args.empty())
         argstring = " '" + args.join("' '") + "'";
 
-    stream << "[Desktop Entry]"
+    stream << "[Desktop Entry]" << "\n";
-           << "\n";
+    stream << "Type=Application" << "\n";
-    stream << "Type=Application"
+    stream << "Categories=Game;ActionGame;AdventureGame;Simulation" << "\n";
-           << "\n";
-    stream << "Categories=Game;ActionGame;AdventureGame;Simulation"
-           << "\n";
     stream << "Exec=\"" << target.toLocal8Bit() << "\"" << argstring.toLocal8Bit() << "\n";
     stream << "Name=" << name.toLocal8Bit() << "\n";
     if (!icon.isEmpty()) {

launcher/PSaveFile.h

@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-3.0-only
+/*
+ *  Prism Launcher - Minecraft Launcher
+ *  Copyright (c) 2023-2024 Trial97 <alexandru.tripon97@gmail.com>
+ *
+ *  This program is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, version 3.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+#pragma once
+
+#include <QFileInfo>
+#include <QSaveFile>
+#include "Application.h"
+
+#if defined(LAUNCHER_APPLICATION)
+
+/* PSaveFile
+ * A class that mimics QSaveFile for Windows.
+ *
+ * When reading resources, we need to avoid accessing temporary files
+ * generated by QSaveFile. If we start reading such a file, we may
+ * inadvertently keep it open while QSaveFile is trying to remove it,
+ * or we might detect the file just before it is removed, leading to
+ * race conditions and errors.
+ *
+ * Unfortunately, QSaveFile doesn't provide a way to retrieve the
+ * temporary file name or to set a specific template for the temporary
+ * file name it uses. By default, QSaveFile appends a `.XXXXXX` suffix
+ * to the original file name, where the `XXXXXX` part is dynamically
+ * generated to ensure uniqueness.
+ *
+ * This class acts like a lock by adding and removing the target file
+ * name into/from a global string set, helping to manage access to
+ * files during critical operations.
+ *
+ * Note: Please do not use the `setFileName` function directly, as it
+ * is not virtual and cannot be overridden.
+ */
+class PSaveFile : public QSaveFile {
+   public:
+    PSaveFile(const QString& name) : QSaveFile(name) { addPath(name); }
+    PSaveFile(const QString& name, QObject* parent) : QSaveFile(name, parent) { addPath(name); }
+    virtual ~PSaveFile()
+    {
+        if (auto app = APPLICATION_DYN) {
+            app->removeQSavePath(m_absoluteFilePath);
+        }
+    }
+
+   private:
+    void addPath(const QString& path)
+    {
+        m_absoluteFilePath = QFileInfo(path).absoluteFilePath() + ".";  // add dot for tmp files only
+        if (auto app = APPLICATION_DYN) {
+            app->addQSavePath(m_absoluteFilePath);
+        }
+    }
+    QString m_absoluteFilePath;
+};
+#else
+#define PSaveFile QSaveFile
+#endif

launcher/minecraft/World.cpp

@@ -38,7 +38,6 @@
 #include <QDebug>
 #include <QDir>
 #include <QDirIterator>
-#include <QSaveFile>
 #include <QString>
 
 #include <FileSystem.h>
@@ -57,6 +56,7 @@
 #include <optional>
 
 #include "FileSystem.h"
+#include "PSaveFile.h"
 
 using std::nullopt;
 using std::optional;
@@ -183,7 +183,7 @@ bool putLevelDatDataToFS(const QFileInfo& file, QByteArray& data)
     if (fullFilePath.isNull()) {
         return false;
     }
-    QSaveFile f(fullFilePath);
+    PSaveFile f(fullFilePath);
     if (!f.open(QIODevice::WriteOnly)) {
         return false;
     }

launcher/minecraft/mod/tasks/BasicFolderLoadTask.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "Application.h"
 #include "FileSystem.h"
 #include "minecraft/mod/Resource.h"
 
@@ -52,6 +53,9 @@ class BasicFolderLoadTask : public Task {
         m_dir.refresh();
         for (auto entry : m_dir.entryInfoList()) {
             auto filePath = entry.absoluteFilePath();
+            if (auto app = APPLICATION_DYN; app && app->checkQSavePath(filePath)) {
+                continue;
+            }
             auto newFilePath = FS::getUniqueResourceName(filePath);
             if (newFilePath != filePath) {
                 FS::move(filePath, newFilePath);

launcher/minecraft/mod/tasks/ModFolderLoadTask.cpp

@@ -36,6 +36,7 @@
 
 #include "ModFolderLoadTask.h"
 
+#include "Application.h"
 #include "FileSystem.h"
 #include "minecraft/mod/MetadataHandler.h"
 
@@ -65,6 +66,9 @@ void ModFolderLoadTask::executeTask()
     m_mods_dir.refresh();
     for (auto entry : m_mods_dir.entryInfoList()) {
         auto filePath = entry.absoluteFilePath();
+        if (auto app = APPLICATION_DYN; app && app->checkQSavePath(filePath)) {
+            continue;
+        }
         auto newFilePath = FS::getUniqueResourceName(filePath);
         if (newFilePath != filePath) {
             FS::move(filePath, newFilePath);

launcher/modplatform/packwiz/Packwiz.cpp

@@ -72,7 +72,7 @@ auto stringEntry(toml::table table, QString entry_name) -> QString
 {
     auto node = table[StringUtils::toStdString(entry_name)];
     if (!node) {
-        qCritical() << "Failed to read str property '" + entry_name + "' in mod metadata.";
+        qWarning() << "Failed to read str property '" + entry_name + "' in mod metadata.";
         return {};
     }
 
@@ -83,7 +83,7 @@ auto intEntry(toml::table table, QString entry_name) -> int
 {
     auto node = table[StringUtils::toStdString(entry_name)];
     if (!node) {
-        qCritical() << "Failed to read int property '" + entry_name + "' in mod metadata.";
+        qWarning() << "Failed to read int property '" + entry_name + "' in mod metadata.";
         return {};
     }
 

launcher/net/FileSink.cpp

@@ -55,7 +55,7 @@ Task::State FileSink::init(QNetworkRequest& request)
     }
 
     wroteAnyData = false;
-    m_output_file.reset(new QSaveFile(m_filename));
+    m_output_file.reset(new PSaveFile(m_filename));
     if (!m_output_file->open(QIODevice::WriteOnly)) {
         qCCritical(taskNetLogC) << "Could not open " + m_filename + " for writing";
         return Task::State::Failed;

launcher/net/FileSink.h

@@ -35,8 +35,7 @@
 
 #pragma once
 
-#include <QSaveFile>
+#include "PSaveFile.h"
-
 #include "Sink.h"
 
 namespace Net {
@@ -60,6 +59,6 @@ class FileSink : public Sink {
    protected:
     QString m_filename;
     bool wroteAnyData = false;
-    std::unique_ptr<QSaveFile> m_output_file;
+    std::unique_ptr<PSaveFile> m_output_file;
 };
 }  // namespace Net

launcher/settings/INIFile.cpp

@@ -39,7 +39,6 @@
 
 #include <QDebug>
 #include <QFile>
-#include <QSaveFile>
 #include <QStringList>
 #include <QTemporaryFile>
 #include <QTextStream>

launcher/ui/dialogs/ExportInstanceDialog.cpp

@@ -51,7 +51,6 @@
 #include <icons/IconList.h>
 #include <QDebug>
 #include <QFileInfo>
-#include <QSaveFile>
 #include <QSortFilterProxyModel>
 #include <QStack>
 #include <functional>

launcher/ui/pages/instance/ModFolderPage.cpp

@@ -124,7 +124,7 @@ ModFolderPage::ModFolderPage(BaseInstance* inst, std::shared_ptr<ModFolderModel>
         ui->actionsToolbar->addAction(ui->actionVisitItemPage);
         connect(ui->actionVisitItemPage, &QAction::triggered, this, &ModFolderPage::visitModPages);
 
-        auto changeVersion = new QAction(tr("Change Version"));
+        auto changeVersion = new QAction(tr("Change Version"), this);
         changeVersion->setToolTip(tr("Change mod version"));
         changeVersion->setEnabled(false);
         ui->actionsToolbar->insertActionAfter(ui->actionUpdateItem, changeVersion);