whytofly
/
qt6-bb10
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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>
bb10
Volker Hilsheimer 2021-11-04 23:35:19 +01:00
parent faa854ffeb
commit c04b5e0c8c
1 changed files with 36 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

59
src/gui/image/qpixmapcache.cpp
View File

@ -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;
}