diff --git a/src/corelib/tools/qmap.h b/src/corelib/tools/qmap.h index 7fd6e4d705f..f2f1bc96f40 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 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 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)}; diff --git a/tests/auto/corelib/tools/collections/tst_collections.cpp b/tests/auto/corelib/tools/collections/tst_collections.cpp index 6d21de9ae7d..fd857aa8755 100644 --- a/tests/auto/corelib/tools/collections/tst_collections.cpp +++ b/tests/auto/corelib/tools/collections/tst_collections.cpp @@ -63,6 +63,9 @@ void foo() #include #include +#include +#include +#include #include @@ -123,6 +126,15 @@ private slots: void foreach_2(); void insert_remove_loop(); + + void detachAssociativeContainerQMap() { detachAssociativeContainerImpl(); } + void detachAssociativeContainerQMultiMap() { detachAssociativeContainerImpl(); } + void detachAssociativeContainerQHash() { detachAssociativeContainerImpl(); } + void detachAssociativeContainerQMultiHash() { detachAssociativeContainerImpl(); } + +private: + template typename Container> + void detachAssociativeContainerImpl(); }; struct LargeStatic { @@ -3545,7 +3557,63 @@ void tst_Collections::insert_remove_loop() insert_remove_loop_impl>(); } +template typename Container> +void tst_Collections::detachAssociativeContainerImpl() +{ + constexpr int RUNS = 50; + for (int run = 0; run < RUNS; ++run) { + Container 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"