1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Q(Multi)Map: prevent dangling key/value after detach()

Browse Source 
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>
This commit is contained in:
Giuseppe D'Angelo 2021-11-08 22:24:51 +01:00
parent 7039fb1e42
commit 8c9875893b
2 changed files with 91 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
src/corelib/tools/qmap.h
View File

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

68
tests/auto/corelib/tools/collections/tst_collections.cpp
View File

@ -63,6 +63,9 @@ void foo()
#include <QTest>
#include <QVector>
#include <QScopedPointer>
#include <QThread>
#include <QSemaphore>
#include <algorithm>
@ -123,6 +126,15 @@ private slots:
void foreach_2();
void insert_remove_loop();
void detachAssociativeContainerQMap() { detachAssociativeContainerImpl<QMap>(); }
void detachAssociativeContainerQMultiMap() { detachAssociativeContainerImpl<QMultiMap>(); }
void detachAssociativeContainerQHash() { detachAssociativeContainerImpl<QHash>(); }
void detachAssociativeContainerQMultiHash() { detachAssociativeContainerImpl<QMultiHash>(); }
private:
template <template<typename, typename> typename Container>
void detachAssociativeContainerImpl();
};
struct LargeStatic {
@ -3545,7 +3557,63 @@ void tst_Collections::insert_remove_loop()
insert_remove_loop_impl<QVarLengthArray<std::string, 15>>();
}
template <template<typename, typename> typename Container>
void tst_Collections::detachAssociativeContainerImpl()
{
constexpr int RUNS = 50;
for (int run = 0; run < RUNS; ++run) {
Container<int, int> container;
for (int i = 0; i < 1'000; ++i) {
container.insert(i, i);
container.insert(i, i); // for multi-keyed containers
}
const auto it = container.constBegin();
const auto &key = it.key();
const auto &value = it.value();
const auto keyCopy = key;
const auto valueCopy = value;
QSemaphore sem1, sem2;
auto detachInAnotherThread = [&sem1, &sem2, copy = container]() mutable {
sem1.release();
sem2.acquire();
copy.clear(); // <==
};
QScopedPointer thread(QThread::create(std::move(detachInAnotherThread)));
thread->start();
sem2.release();
sem1.acquire();
// The following call may detach (because the container is
// shared), and then use key/value to search+insert.
//
// This means that key/value, as references, have to be valid
// throughout the insertion procedure. Note that they are
// references into the container *itself*; and that the
// insertion procedure is working on a new (detached) copy of
// the container's payload.
//
// There is now a possible scenario in which the clear() above
// finds the copy's refcount at 1, hence not perform a detach,
// and destroy its payload. But key/value were references into
// *that* payload (it's the payload that `container` itself
// used to share). If inside insert() we don't take extra
// measures to keep the payload alive, now they're dangling and
// the insertion will malfunction.
container.insert(key, value);
QVERIFY(container.contains(keyCopy));
QCOMPARE(container.value(keyCopy), valueCopy);
thread->wait();
}
}
QTEST_APPLESS_MAIN(tst_Collections)
#include "tst_collections.moc"