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>

1 files changed, 68 insertions, 0 deletions

diff --git a/tests/auto/corelib/tools/collections/tst_collections.cpp b/tests/auto/corelib/tools/collections/tst_collections.cpp
index 6d21de9ae7..fd857aa875 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 <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"