JSON: When clearing duplicate object entries, also clear containers

Previously, if you had multiple entries with the same name in an object,
and some of them were again objects or arrays, parsing the JSON document
would leak memory.

Also, we use std::stable_sort instead of std::sort now, so that we don't
accidentally randomize the order of elements with equal keys.

[ChangeLog][QtCore][JSON] A memory leak in the JSON parser when reading
objects with duplicate keys was fixed.

Pick-to: 5.15 6.2 6.3
Fixes: QTBUG-99799
Change-Id: Ic2065f2e490c2d3506a356745542148ad9c24262
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

2 files changed, 63 insertions, 10 deletions

diff --git a/src/corelib/serialization/qjsonparser.cpp b/src/corelib/serialization/qjsonparser.cpp
index 168a2f6f7b..df66fc2772 100644
--- a/src/corelib/serialization/qjsonparser.cpp
+++ b/src/corelib/serialization/qjsonparser.cpp
@@ -379,10 +379,30 @@ error:
     return QCborValue();
 }
 
+// We need to retain the _last_ value for any duplicate keys and we need to deref containers.
+// Therefore the manual implementation of std::unique().
+template<typename Iterator, typename Compare, typename Assign>
+static Iterator customAssigningUniqueLast(Iterator first, Iterator last,
+                                          Compare compare, Assign assign)
+{
+    first = std::adjacent_find(first, last, compare);
+    if (first == last)
+        return last;
+
+    Iterator result = first;
+    while (++first != last) {
+        if (!compare(*result, *first))
+            ++result;
+        if (result != first)
+            assign(*result, *first);
+    }
+
+    return ++result;
+}
+
 static void sortContainer(QCborContainerPrivate *container)
 {
     using Forward = QJsonPrivate::KeyIterator;
-    using Reverse = std::reverse_iterator<Forward>;
     using Value = Forward::value_type;
 
     auto compare = [container](const Value &a, const Value &b)
@@ -417,17 +437,31 @@ static void sortContainer(QCborContainerPrivate *container)
         }
     };
 
-    std::sort(Forward(container->elements.begin()), Forward(container->elements.end()),
-              [&compare](const Value &a, const Value &b) { return compare(a, b) < 0; });
+    // The elements' containers are owned by the outer container, not by the elements themselves.
+    auto move = [](Forward::reference target, Forward::reference source)
+    {
+        QtCbor::Element &targetValue = target.value();
+
+        // If the target has a container, deref it before overwriting, so that we don't leak.
+        if (targetValue.flags & QtCbor::Element::IsContainer)
+            targetValue.container->deref();
+
+        // Do not move, so that we can clear the value afterwards.
+        target = source;
+
+        // Clear the source value, so that we don't store the same container twice.
+        source.value() = QtCbor::Element();
+    };
+
+    std::stable_sort(
+                Forward(container->elements.begin()), Forward(container->elements.end()),
+                [&compare](const Value &a, const Value &b) { return compare(a, b) < 0; });
 
-    // We need to retain the _last_ value for any duplicate keys. Therefore the reverse dance here.
-    auto it = std::unique(Reverse(container->elements.end()), Reverse(container->elements.begin()),
-                          [&compare](const Value &a, const Value &b) {
-        return compare(a, b) == 0;
-    }).base().elementsIterator();
+    Forward result = customAssigningUniqueLast(
+                Forward(container->elements.begin()),  Forward(container->elements.end()),
+                [&compare](const Value &a, const Value &b) { return compare(a, b) == 0; }, move);
 
-    // The erase from beginning is expensive but hopefully rare.
-    container->elements.erase(container->elements.begin(), it);
+    container->elements.erase(result.elementsIterator(), container->elements.end());
 }
 
 
diff --git a/tests/auto/corelib/serialization/json/tst_qtjson.cpp b/tests/auto/corelib/serialization/json/tst_qtjson.cpp
index 57a679dbc9..3a57d3b8a5 100644
--- a/tests/auto/corelib/serialization/json/tst_qtjson.cpp
+++ b/tests/auto/corelib/serialization/json/tst_qtjson.cpp
@@ -175,6 +175,7 @@ private Q_SLOTS:
     void fromToVariantConversions();
 
     void testIteratorComparison();
+    void noLeakOnNameClash();
 
 private:
     QString testDataDir;
@@ -3649,5 +3650,23 @@ void tst_QtJson::testIteratorComparison()
     QVERIFY(t.end() > t.begin());
 }
 
+void tst_QtJson::noLeakOnNameClash()
+{
+    QJsonDocument doc = QJsonDocument::fromJson("{\"\":{\"\":0},\"\":0}");
+    QVERIFY(!doc.isNull());
+    const QJsonObject obj = doc.object();
+
+    // Removed the duplicate key.
+    QCOMPARE(obj.length(), 1);
+
+    // Retained the last of the duplicates.
+    const QJsonValue val = obj.begin().value();
+    QVERIFY(val.isDouble());
+    QCOMPARE(val.toDouble(), 0.0);
+
+    // It should not leak.
+    // In particular it should not forget to deref the container for the inner object.
+}
+
 QTEST_MAIN(tst_QtJson)
 #include "tst_qtjson.moc"