Tidy up QPixmapCache to prevent potential segfault

Following reports of segfaults in long-running programs leading to
QPixmapCache corruption, clean up some code smells:

* check pointers before dereferencing

We did this in some places, and a default constructed or moved-from key
could have a nullptr KeyData, so check everywhere.

* don't trunctate qsizetype to int

Still plenty of int APIs left, but no need for ints in internal code.

* don't underflow maxCost to -1 if totalCost was 0
* use ranged-for to iterate over list of keys
* guard any public function that might create the cache with the thread-
test

This avoids that the cache ends up living in the wrong thread.

* don't use reinterpret_cast when static_cast is enough (which is always
from void*)

Since the crash is not reproducible so far, and the reports indicate
that it can only be observed when the program has run for a long time,
there is no test case included. However, this removes some code smells
that might be responsible for data corruption.

Pick-to: 6.2
Task-number: QTBUG-97903
Task-number: QTBUG-91445
Task-number: QTCREATORBUG-26473
Change-Id: Ibdd8963d7dd44caab1468ecc6f81ace761719c69
Reviewed-by: Eirik Aavitsland <eirik.aavitsland@qt.io>

1 files changed, 36 insertions, 23 deletions

diff --git a/src/gui/image/qpixmapcache.cpp b/src/gui/image/qpixmapcache.cpp
index b3d22b48a1..d2619ec63f 100644
--- a/src/gui/image/qpixmapcache.cpp
+++ b/src/gui/image/qpixmapcache.cpp
@@ -93,14 +93,14 @@ QT_BEGIN_NAMESPACE
 
 static const int cache_limit_default = 10240; // 10 MB cache limit
 
-static inline int cost(const QPixmap &pixmap)
+static inline qsizetype cost(const QPixmap &pixmap)
 {
-    // make sure to do a 64bit calculation
-    const qint64 costKb = static_cast<qint64>(pixmap.width()) *
-            pixmap.height() * pixmap.depth() / (8 * 1024);
-    const qint64 costMax = std::numeric_limits<int>::max();
+    // make sure to do a 64bit calculation; qsizetype might be smaller
+    const qint64 costKb = static_cast<qint64>(pixmap.width())
+                        * pixmap.height() * pixmap.depth() / (8 * 1024);
+    const qint64 costMax = std::numeric_limits<qsizetype>::max();
     // a small pixmap should have at least a cost of 1(kb)
-    return static_cast<int>(qBound(1LL, costKb, costMax));
+    return static_cast<qsizetype>(qBound(1LL, costKb, costMax));
 }
 
 static inline bool qt_pixmapcache_thread_test()
@@ -255,7 +255,8 @@ QT_END_INCLUDE_NAMESPACE
 
 size_t qHash(const QPixmapCache::Key &k, size_t seed)
 {
-    return qHash(QPMCache::get(k)->key, seed);
+    const auto *keyData = QPMCache::get(k);
+    return qHash(keyData ? keyData->key : 0, seed);
 }
 
 QPMCache::QPMCache()
@@ -286,16 +287,19 @@ QPMCache::~QPMCache()
 */
 bool QPMCache::flushDetachedPixmaps(bool nt)
 {
-    int mc = maxCost();
-    setMaxCost(nt ? totalCost() * 3 / 4 : totalCost() -1);
+    auto mc = maxCost();
+    const qsizetype currentTotal = totalCost();
+    if (currentTotal)
+        setMaxCost(nt ? currentTotal * 3 / 4 : currentTotal - 1);
     setMaxCost(mc);
     ps = totalCost();
 
     bool any = false;
     QHash<QString, QPixmapCache::Key>::iterator it = cacheKeys.begin();
     while (it != cacheKeys.end()) {
-        if (!contains(it.value())) {
-            releaseKey(it.value());
+        const auto value = it.value();
+        if (value.isValid() && !contains(value)) {
+            releaseKey(value);
             it = cacheKeys.erase(it);
             any = true;
         } else {
@@ -337,7 +341,7 @@ QPixmap *QPMCache::object(const QString &key) const
 
 QPixmap *QPMCache::object(const QPixmapCache::Key &key) const
 {
-    Q_ASSERT(key.d->isValid);
+    Q_ASSERT(key.isValid());
     QPixmap *ptr = QCache<QPixmapCache::Key, QPixmapCacheEntry>::object(key);
     //We didn't find the pixmap in the cache, the key is not valid anymore
     if (!ptr)
@@ -383,7 +387,7 @@ QPixmapCache::Key QPMCache::insert(const QPixmap &pixmap, int cost)
 
 bool QPMCache::replace(const QPixmapCache::Key &key, const QPixmap &pixmap, int cost)
 {
-    Q_ASSERT(key.d->isValid);
+    Q_ASSERT(key.isValid());
     //If for the same key we had already an entry so we should delete the pixmap and use the new one
     QCache<QPixmapCache::Key, QPixmapCacheEntry>::remove(key);
 
@@ -420,7 +424,7 @@ void QPMCache::resizeKeyArray(int size)
 {
     if (size <= keyArraySize || size == 0)
         return;
-    keyArray = q_check_ptr(reinterpret_cast<int *>(realloc(keyArray,
+    keyArray = q_check_ptr(static_cast<int *>(realloc(keyArray,
                     size * sizeof(int))));
     for (int i = keyArraySize; i != size; ++i)
         keyArray[i] = i + 1;
@@ -441,13 +445,14 @@ QPixmapCache::Key QPMCache::createKey()
 
 void QPMCache::releaseKey(const QPixmapCache::Key &key)
 {
-    if (key.d->key > keyArraySize || key.d->key <= 0)
+    QPixmapCache::KeyData *keyData = key.d;
+    if (!keyData || keyData->key > keyArraySize || keyData->key <= 0)
         return;
-    key.d->key--;
-    keyArray[key.d->key] = freeKey;
-    freeKey = key.d->key;
-    key.d->isValid = false;
-    key.d->key = 0;
+    keyData->key--;
+    keyArray[keyData->key] = freeKey;
+    freeKey = keyData->key;
+    keyData->isValid = false;
+    keyData->key = 0;
 }
 
 void QPMCache::clear()
@@ -457,9 +462,11 @@ void QPMCache::clear()
     freeKey = 0;
     keyArraySize = 0;
     //Mark all keys as invalid
-    QList<QPixmapCache::Key> keys = QCache<QPixmapCache::Key, QPixmapCacheEntry>::keys();
-    for (int i = 0; i < keys.size(); ++i)
-        keys.at(i).d->isValid = false;
+    const QList<QPixmapCache::Key> keys = QCache<QPixmapCache::Key, QPixmapCacheEntry>::keys();
+    for (const auto &key : keys) {
+        if (key.d)
+            key.d->isValid = false;
+    }
     QCache<QPixmapCache::Key, QPixmapCacheEntry>::clear();
     // Nothing left to flush; stop the timer
     if (theid) {
@@ -608,6 +615,8 @@ bool QPixmapCache::replace(const Key &key, const QPixmap &pixmap)
 
 int QPixmapCache::cacheLimit()
 {
+    if (!qt_pixmapcache_thread_test())
+        return 0;
     return pm_cache()->maxCost();
 }
 
@@ -671,11 +680,15 @@ void QPixmapCache::clear()
 
 void QPixmapCache::flushDetachedPixmaps()
 {
+    if (!qt_pixmapcache_thread_test())
+        return;
     pm_cache()->flushDetachedPixmaps(true);
 }
 
 int QPixmapCache::totalUsed()
 {
+    if (!qt_pixmapcache_thread_test())
+        return 0;
     return (pm_cache()->totalCost()+1023) / 1024;
 }