Merge pull request #2907 from Ryex/fix/icon_cache_stack_overflow

Prevent infinite recursion when mod icon load fails

launcher/MTPixmapCache.h

@@ -101,7 +101,7 @@ class PixmapCache final : public QObject {
      */
     bool _markCacheMissByEviciton()
     {
-        static constexpr uint maxInt = static_cast<uint>(std::numeric_limits<int>::max());
+        static constexpr uint maxCache = static_cast<uint>(std::numeric_limits<int>::max()) / 4;
         static constexpr uint step = 10240;
         static constexpr int oneSecond = 1000;
 
@@ -118,8 +118,8 @@ class PixmapCache final : public QObject {
         if (m_consecutive_fast_evicitons >= m_consecutive_fast_evicitons_threshold) {
             // increase the cache size
             uint newSize = _cacheLimit() + step;
-            if (newSize >= maxInt) {  // increase it until you overflow :D
+            if (newSize >= maxCache) {  // increase it until you overflow :D
-                newSize = maxInt;
+                newSize = maxCache;
                 qDebug() << m_consecutive_fast_evicitons
                          << tr("pixmap cache misses by eviction happened too fast, doing nothing as the cache size reached it's limit");
             } else {

launcher/minecraft/mod/Mod.cpp

@@ -36,6 +36,7 @@
  */
 
 #include "Mod.h"
+#include <qpixmap.h>
 
 #include <QDebug>
 #include <QDir>
@@ -241,7 +242,7 @@ void Mod::finishResolvingWithDetails(ModDetails&& details)
     if (metadata)
         setMetadata(std::move(metadata));
     if (!iconPath().isEmpty()) {
-        m_pack_image_cache_key.was_read_attempt = false;
+        m_packImageCacheKey.wasReadAttempt = false;
     }
 }
 
@@ -290,45 +291,53 @@ auto Mod::issueTracker() const -> QString
     return details().issue_tracker;
 }
 
-void Mod::setIcon(QImage new_image) const
+QPixmap Mod::setIcon(QImage new_image) const
 {
     QMutexLocker locker(&m_data_lock);
 
     Q_ASSERT(!new_image.isNull());
 
-    if (m_pack_image_cache_key.key.isValid())
+    if (m_packImageCacheKey.key.isValid())
-        PixmapCache::remove(m_pack_image_cache_key.key);
+        PixmapCache::remove(m_packImageCacheKey.key);
 
     // scale the image to avoid flooding the pixmapcache
     auto pixmap =
         QPixmap::fromImage(new_image.scaled({ 64, 64 }, Qt::AspectRatioMode::KeepAspectRatioByExpanding, Qt::SmoothTransformation));
 
-    m_pack_image_cache_key.key = PixmapCache::insert(pixmap);
+    m_packImageCacheKey.key = PixmapCache::insert(pixmap);
-    m_pack_image_cache_key.was_ever_used = true;
+    m_packImageCacheKey.wasEverUsed = true;
-    m_pack_image_cache_key.was_read_attempt = true;
+    m_packImageCacheKey.wasReadAttempt = true;
+    return pixmap;
 }
 
 QPixmap Mod::icon(QSize size, Qt::AspectRatioMode mode) const
 {
-    QPixmap cached_image;
+    auto pixmap_transform = [&size, &mode](QPixmap pixmap) {
-    if (PixmapCache::find(m_pack_image_cache_key.key, &cached_image)) {
         if (size.isNull())
-            return cached_image;
+            return pixmap;
-        return cached_image.scaled(size, mode, Qt::SmoothTransformation);
+        return pixmap.scaled(size, mode, Qt::SmoothTransformation);
+    };
+
+    QPixmap cached_image;
+    if (PixmapCache::find(m_packImageCacheKey.key, &cached_image)) {
+        return pixmap_transform(cached_image);
     }
 
     // No valid image we can get
-    if ((!m_pack_image_cache_key.was_ever_used && m_pack_image_cache_key.was_read_attempt) || iconPath().isEmpty())
+    if ((!m_packImageCacheKey.wasEverUsed && m_packImageCacheKey.wasReadAttempt) || iconPath().isEmpty())
         return {};
 
-    if (m_pack_image_cache_key.was_ever_used) {
+    if (m_packImageCacheKey.wasEverUsed) {
         qDebug() << "Mod" << name() << "Had it's icon evicted from the cache. reloading...";
         PixmapCache::markCacheMissByEviciton();
     }
     // Image got evicted from the cache or an attempt to load it has not been made. load it and retry.
-    m_pack_image_cache_key.was_read_attempt = true;
+    m_packImageCacheKey.wasReadAttempt = true;
-    ModUtils::loadIconFile(*this);
+    if (ModUtils::loadIconFile(*this, &cached_image)) {
-    return icon(size);
+        return pixmap_transform(cached_image);
+    }
+    // Image failed to load
+    return {};
 }
 
 bool Mod::valid() const

launcher/minecraft/mod/Mod.h

@@ -82,7 +82,7 @@ class Mod : public Resource {
     /** Gets the icon of the mod, converted to a QPixmap for drawing, and scaled to size. */
     [[nodiscard]] QPixmap icon(QSize size, Qt::AspectRatioMode mode = Qt::AspectRatioMode::IgnoreAspectRatio) const;
     /** Thread-safe. */
-    void setIcon(QImage new_image) const;
+    QPixmap setIcon(QImage new_image) const;
 
     auto metadata() -> std::shared_ptr<Metadata::ModStruct>;
     auto metadata() const -> const std::shared_ptr<Metadata::ModStruct>;
@@ -111,7 +111,7 @@ class Mod : public Resource {
 
     struct {
         QPixmapCache::Key key;
-        bool was_ever_used = false;
+        bool wasEverUsed = false;
-        bool was_read_attempt = false;
+        bool wasReadAttempt = false;
-    } mutable m_pack_image_cache_key;
+    } mutable m_packImageCacheKey;
 };

launcher/minecraft/mod/tasks/LocalModParseTask.cpp

@@ -647,11 +647,11 @@ bool validate(QFileInfo file)
     return ModUtils::process(mod, ProcessingLevel::BasicInfoOnly) && mod.valid();
 }
 
-bool processIconPNG(const Mod& mod, QByteArray&& raw_data)
+bool processIconPNG(const Mod& mod, QByteArray&& raw_data, QPixmap* pixmap)
 {
     auto img = QImage::fromData(raw_data);
     if (!img.isNull()) {
-        mod.setIcon(img);
+        *pixmap = mod.setIcon(img);
     } else {
         qWarning() << "Failed to parse mod logo:" << mod.iconPath() << "from" << mod.name();
         return false;
@@ -659,15 +659,15 @@ bool processIconPNG(const Mod& mod, QByteArray&& raw_data)
     return true;
 }
 
-bool loadIconFile(const Mod& mod)
+bool loadIconFile(const Mod& mod, QPixmap* pixmap)
 {
     if (mod.iconPath().isEmpty()) {
         qWarning() << "No Iconfile set, be sure to parse the mod first";
         return false;
     }
 
-    auto png_invalid = [&mod]() {
+    auto png_invalid = [&mod](const QString& reason) {
-        qWarning() << "Mod at" << mod.fileinfo().filePath() << "does not have a valid icon";
+        qWarning() << "Mod at" << mod.fileinfo().filePath() << "does not have a valid icon:" << reason;
         return false;
     };
 
@@ -676,24 +676,26 @@ bool loadIconFile(const Mod& mod)
             QFileInfo icon_info(FS::PathCombine(mod.fileinfo().filePath(), mod.iconPath()));
             if (icon_info.exists() && icon_info.isFile()) {
                 QFile icon(icon_info.filePath());
-                if (!icon.open(QIODevice::ReadOnly))
+                if (!icon.open(QIODevice::ReadOnly)) {
-                    return false;
+                    return png_invalid("failed  to open file " + icon_info.filePath());
+                }
                 auto data = icon.readAll();
 
-                bool icon_result = ModUtils::processIconPNG(mod, std::move(data));
+                bool icon_result = ModUtils::processIconPNG(mod, std::move(data), pixmap);
 
                 icon.close();
 
                 if (!icon_result) {
-                    return png_invalid();  // icon invalid
+                    return png_invalid("invalid png image");  // icon invalid
                 }
+                return true;
             }
-            return false;
+            return png_invalid("file '" + icon_info.filePath() + "' does not exists or is not a file");
         }
         case ResourceType::ZIPFILE: {
             QuaZip zip(mod.fileinfo().filePath());
             if (!zip.open(QuaZip::mdUnzip))
-                return false;
+                return png_invalid("failed to open '" + mod.fileinfo().filePath() + "' as a zip archive");
 
             QuaZipFile file(&zip);
 
@@ -701,28 +703,27 @@ bool loadIconFile(const Mod& mod)
                 if (!file.open(QIODevice::ReadOnly)) {
                     qCritical() << "Failed to open file in zip.";
                     zip.close();
-                    return png_invalid();
+                    return png_invalid("Failed to open '" + mod.iconPath() + "' in zip archive");
                 }
 
                 auto data = file.readAll();
 
-                bool icon_result = ModUtils::processIconPNG(mod, std::move(data));
+                bool icon_result = ModUtils::processIconPNG(mod, std::move(data), pixmap);
 
                 file.close();
                 if (!icon_result) {
-                    return png_invalid();  // icon png invalid
+                    return png_invalid("invalid png image");  // icon png invalid
                 }
-            } else {
+                return true;
-                return png_invalid();  // could not set icon as current file.
             }
-            return false;
+            return png_invalid("Failed to set '" + mod.iconPath() +
+                               "' as current file in zip archive");  // could not set icon as current file.
         }
         case ResourceType::LITEMOD: {
-            return false;  // can lightmods even have icons?
+            return png_invalid("litemods do not have icons");  // can lightmods even have icons?
         }
         default:
-            qWarning() << "Invalid type for mod, can not load icon.";
+            return png_invalid("Invalid type for mod, can not load icon.");
-            return false;
     }
 }
 

launcher/minecraft/mod/tasks/LocalModParseTask.h

@@ -26,8 +26,8 @@ bool processLitemod(Mod& mod, ProcessingLevel level = ProcessingLevel::Full);
 /** Checks whether a file is valid as a mod or not. */
 bool validate(QFileInfo file);
 
-bool processIconPNG(const Mod& mod, QByteArray&& raw_data);
+bool processIconPNG(const Mod& mod, QByteArray&& raw_data, QPixmap* pixmap);
-bool loadIconFile(const Mod& mod);
+bool loadIconFile(const Mod& mod, QPixmap* pixmap);
 }  // namespace ModUtils
 
 class LocalModParseTask : public Task {