From c04b5e0c8c6669b2c4879911e096492df3387911 Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Thu, 4 Nov 2021 23:35:19 +0100 Subject: 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 --- src/gui/image/qpixmapcache.cpp | 59 ++++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 23 deletions(-) (limited to 'src/gui/image') 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(pixmap.width()) * - pixmap.height() * pixmap.depth() / (8 * 1024); - const qint64 costMax = std::numeric_limits::max(); + // make sure to do a 64bit calculation; qsizetype might be smaller + const qint64 costKb = static_cast(pixmap.width()) + * pixmap.height() * pixmap.depth() / (8 * 1024); + const qint64 costMax = std::numeric_limits::max(); // a small pixmap should have at least a cost of 1(kb) - return static_cast(qBound(1LL, costKb, costMax)); + return static_cast(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::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::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::remove(key); @@ -420,7 +424,7 @@ void QPMCache::resizeKeyArray(int size) { if (size <= keyArraySize || size == 0) return; - keyArray = q_check_ptr(reinterpret_cast(realloc(keyArray, + keyArray = q_check_ptr(static_cast(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 keys = QCache::keys(); - for (int i = 0; i < keys.size(); ++i) - keys.at(i).d->isValid = false; + const QList keys = QCache::keys(); + for (const auto &key : keys) { + if (key.d) + key.d->isValid = false; + } QCache::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; } -- cgit v1.2.3