diff options
author | Giuseppe D'Angelo <giuseppe.dangelo@kdab.com> | 2021-11-08 22:24:51 +0100 |
---|---|---|
committer | Giuseppe D'Angelo <giuseppe.dangelo@kdab.com> | 2021-11-26 02:14:52 +0100 |
commit | 8c9875893bbd7b1355e36d9501d8cb1f84cb6b4d (patch) | |
tree | c399cd37dbccd0abd9e781b7c410e263f0d8d622 /src/corelib | |
parent | 7039fb1e425016ed60383e3752695edbc28a4201 (diff) |
Q(Multi)Map: prevent dangling key/value after detach()
Q(Multi)Map mutating functions that take reference to a key and/or a
value (e.g. insert(), take(), etc.) must make sure that those references
are still valid -- that is, that the referred objects are still alive --
after the detach() call done inside those functions.
In fact, if the key/value are references into *this, one must take extra
steps in order to preserve them across the detach().
Consider the scenario where one has two shallow copies of QMap, each
accessed by a different thread, and each thread calls a mutating
function on its copy, using a reference into the map (e.g.
map.take(map.firstKey())). Let's call the shared payload of this QMap
SP, with its refcount of 2; it's important to note that the argument
(call it A) passed to the mutating function belongs to SP.
Each thread may then find the reference count to be different than 1 and
therefore do a detach() from inside the mutating function. Then this
could happen:
Thread 1: Thread 2:
detach() detach()
SP refcount != 1 => true SP refcount != 1 => true
deep copy from SP deep copy from SP
ref() the new copy ref() the new copy
SP.deref() => 1 => don't dealloc SP
set the new copy as payload
SP.deref() => 0 => dealloc SP
set the new copy as payload
use A to access the new copy use A to access the new copy
The order of ref()/deref() SP and the new copy in each thread doesn't
really matter here. What really matters is that SP has been destroyed
and that means A is a danging reference.
Fix this by keeping SP alive in the mutating functions before doing a
detach(). This can simply be realized by taking a local copy of the map
from within such functions.
remove() doesn't suffer from this because its implementation doesn't do
a bare detach() but something slightly smarter.
Change-Id: Iad974a1ad1bd5ee5d1e9378ae90947bef737b6bb
Pick-to: 6.2
Reviewed-by: Marc Mutz <marc.mutz@qt.io>
Reviewed-by: MÃ¥rten Nordheim <marten.nordheim@qt.io>
Diffstat (limited to 'src/corelib')
-rw-r--r-- | src/corelib/tools/qmap.h | 26 |
1 files changed, 23 insertions, 3 deletions
diff --git a/src/corelib/tools/qmap.h b/src/corelib/tools/qmap.h index 7fd6e4d705..f2f1bc96f4 100644 --- a/src/corelib/tools/qmap.h +++ b/src/corelib/tools/qmap.h @@ -359,6 +359,7 @@ public: if (!d) return T(); + const auto copy = d.isShared() ? *this : QMap(); // keep `key` alive across the detach // TODO: improve. There is no need of copying all the // elements (the one to be removed can be skipped). detach(); @@ -400,6 +401,7 @@ public: T &operator[](const Key &key) { + const auto copy = d.isShared() ? *this : QMap(); // keep `key` alive across the detach detach(); auto i = d->m.find(key); if (i == d->m.end()) @@ -669,6 +671,7 @@ public: iterator find(const Key &key) { + const auto copy = d.isShared() ? *this : QMap(); // keep `key` alive across the detach detach(); return iterator(d->m.find(key)); } @@ -687,6 +690,7 @@ public: iterator lowerBound(const Key &key) { + const auto copy = d.isShared() ? *this : QMap(); // keep `key` alive across the detach detach(); return iterator(d->m.lower_bound(key)); } @@ -700,6 +704,7 @@ public: iterator upperBound(const Key &key) { + const auto copy = d.isShared() ? *this : QMap(); // keep `key` alive across the detach detach(); return iterator(d->m.upper_bound(key)); } @@ -713,6 +718,7 @@ public: iterator insert(const Key &key, const T &value) { + const auto copy = d.isShared() ? *this : QMap(); // keep `key` alive across the detach // TODO: improve. In case of assignment, why copying first? detach(); return iterator(d->m.insert_or_assign(key, value).first); @@ -722,6 +728,7 @@ public: { // TODO: improve. In case of assignment, why copying first? typename Map::const_iterator dpos; + const auto copy = d.isShared() ? *this : QMap(); // keep `key`/`value` alive across the detach if (!d || d.isShared()) { auto posDistance = d ? std::distance(d->m.cbegin(), pos.i) : 0; detach(); @@ -789,6 +796,7 @@ public: QPair<iterator, iterator> equal_range(const Key &akey) { + const auto copy = d.isShared() ? *this : QMap(); // keep `key` alive across the detach detach(); auto result = d->m.equal_range(akey); return {iterator(result.first), iterator(result.second)}; @@ -986,15 +994,15 @@ public: if (!d) return 0; - // TODO: improve. Copy over only the elements not to be removed. - detach(); - // key and value may belong to this map. As such, we need to copy // them to ensure they stay valid throughout the iteration below // (which may destroy them) const Key keyCopy = key; const T valueCopy = value; + // TODO: improve. Copy over only the elements not to be removed. + detach(); + size_type result = 0; const auto &keyCompare = d->m.key_comp(); @@ -1024,6 +1032,8 @@ public: if (!d) return T(); + const auto copy = d.isShared() ? *this : QMultiMap(); // keep `key` alive across the detach + // TODO: improve. There is no need of copying all the // elements (the one to be removed can be skipped). detach(); @@ -1361,6 +1371,7 @@ public: iterator find(const Key &key) { + const auto copy = d.isShared() ? *this : QMultiMap(); // keep `key` alive across the detach detach(); return iterator(d->m.find(key)); } @@ -1379,6 +1390,8 @@ public: iterator find(const Key &key, const T &value) { + const auto copy = d.isShared() ? *this : QMultiMap(); // keep `key`/`value` alive across the detach + detach(); auto range = d->m.equal_range(key); @@ -1411,6 +1424,7 @@ public: iterator lowerBound(const Key &key) { + const auto copy = d.isShared() ? *this : QMultiMap(); // keep `key` alive across the detach detach(); return iterator(d->m.lower_bound(key)); } @@ -1424,6 +1438,7 @@ public: iterator upperBound(const Key &key) { + const auto copy = d.isShared() ? *this : QMultiMap(); // keep `key` alive across the detach detach(); return iterator(d->m.upper_bound(key)); } @@ -1437,6 +1452,7 @@ public: iterator insert(const Key &key, const T &value) { + const auto copy = d.isShared() ? *this : QMultiMap(); // keep `key`/`value` alive across the detach detach(); // note that std::multimap inserts at the end of an equal_range for a key, // QMultiMap at the beginning. @@ -1446,6 +1462,7 @@ public: iterator insert(const_iterator pos, const Key &key, const T &value) { + const auto copy = d.isShared() ? *this : QMultiMap(); // keep `key`/`value` alive across the detach typename Map::const_iterator dpos; if (!d || d.isShared()) { auto posDistance = d ? std::distance(d->m.cbegin(), pos.i) : 0; @@ -1484,6 +1501,8 @@ public: iterator replace(const Key &key, const T &value) { + const auto copy = d.isShared() ? *this : QMultiMap(); // keep `key`/`value` alive across the detach + // TODO: improve. No need of copying and then overwriting. detach(); @@ -1503,6 +1522,7 @@ public: QPair<iterator, iterator> equal_range(const Key &akey) { + const auto copy = d.isShared() ? *this : QMultiMap(); // keep `key` alive across the detach detach(); auto result = d->m.equal_range(akey); return {iterator(result.first), iterator(result.second)}; |