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>

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();
 
-    // 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();
+        // 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();
 
-    // The erase from beginning is expensive but hopefully rare.
-    container->elements.erase(container->elements.begin(), it);
+        // 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; });
+
+    Forward result = customAssigningUniqueLast(
+                Forward(container->elements.begin()),  Forward(container->elements.end()),
+                [&compare](const Value &a, const Value &b) { return compare(a, b) == 0; }, move);
+
+    container->elements.erase(result.elementsIterator(), container->elements.end());
 }
 
 

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"