Implement QCborContainerPrivate::compact()

... and use it in QCborContainerPrivate::replaceAt_complex() to avoid
unconstrained memory growth in certain scenarios.

Remove the `reserved` parameter, because it was referring to the
elements array, not to the byte data, so it cannot really be used
in the implementation.

Fixes: QTBUG-109073
Pick-to: 6.7
Change-Id: I2e8fe7e4a4bf7a0ce06c87ca657f2bc01bae0341
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

2 files changed, 43 insertions, 14 deletions

diff --git a/src/corelib/serialization/qcborvalue.cpp b/src/corelib/serialization/qcborvalue.cpp
index f8758720a8..4fb7618017 100644
--- a/src/corelib/serialization/qcborvalue.cpp
+++ b/src/corelib/serialization/qcborvalue.cpp
@@ -922,14 +922,24 @@ QCborContainerPrivate::~QCborContainerPrivate()
     }
 }
 
-void QCborContainerPrivate::compact(qsizetype reserved)
+void QCborContainerPrivate::compact()
 {
     if (usedData > data.size() / 2)
         return;
 
     // 50% savings if we recreate the byte data
-    // ### TBD
-    Q_UNUSED(reserved);
+    QByteArray newData;
+    QByteArray::size_type newUsedData = 0;
+    // Compact only elements that have byte data.
+    // Nested containers will be compacted when their data changes.
+    for (auto &e : elements) {
+        if (e.flags & Element::HasByteData) {
+            if (const ByteData *b = byteData(e))
+                e.value = addByteDataImpl(newData, newUsedData, b->byte(), b->len);
+        }
+    }
+    data = newData;
+    usedData = newUsedData;
 }
 
 QCborContainerPrivate *QCborContainerPrivate::clone(QCborContainerPrivate *d, qsizetype reserved)
@@ -941,7 +951,7 @@ QCborContainerPrivate *QCborContainerPrivate::clone(QCborContainerPrivate *d, qs
         QExplicitlySharedDataPointer u(new QCborContainerPrivate(*d));
         if (reserved >= 0) {
             u->elements.reserve(reserved);
-            u->compact(reserved);
+            u->compact();
         }
 
         d = u.take();
@@ -1013,10 +1023,23 @@ void QCborContainerPrivate::replaceAt_complex(Element &e, const QCborValue &valu
 
         // Copy string data, if any
         if (const ByteData *b = value.container->byteData(value.n)) {
-            if (this == value.container)
-                e.value = addByteData(b->toByteArray(), b->len);
-            else
+            const auto flags = e.flags;
+            // The element e has an invalid e.value, because it is copied from
+            // value. It means that calling compact() will trigger an assertion
+            // or just silently corrupt the data.
+            // Temporarily unset the Element::HasByteData flag in order to skip
+            // the element e in the call to compact().
+            e.flags = e.flags & ~Element::HasByteData;
+            if (this == value.container) {
+                const QByteArray valueData = b->toByteArray();
+                compact();
+                e.value = addByteData(valueData, valueData.size());
+            } else {
+                compact();
                 e.value = addByteData(b->byte(), b->len);
+            }
+            // restore the flags
+            e.flags = flags;
         }
 
         if (disp == MoveContainer)
@@ -1053,7 +1076,7 @@ QCborValue QCborContainerPrivate::extractAt_complex(Element e)
         // make a shallow copy of the byte data
         container->appendByteData(b->byte(), b->len, e.type, e.flags);
         usedData -= b->len + qsizetype(sizeof(*b));
-        compact(elements.size());
+        compact();
     } else {
         // just share with the original byte data
         container->data = data;
diff --git a/src/corelib/serialization/qcborvalue_p.h b/src/corelib/serialization/qcborvalue_p.h
index c5cc6c6f0b..5930e330dc 100644
--- a/src/corelib/serialization/qcborvalue_p.h
+++ b/src/corelib/serialization/qcborvalue_p.h
@@ -103,18 +103,19 @@ public:
     QList<QtCbor::Element> elements;
 
     void deref() { if (!ref.deref()) delete this; }
-    void compact(qsizetype reserved);
+    void compact();
     static QCborContainerPrivate *clone(QCborContainerPrivate *d, qsizetype reserved = -1);
     static QCborContainerPrivate *detach(QCborContainerPrivate *d, qsizetype reserved);
     static QCborContainerPrivate *grow(QCborContainerPrivate *d, qsizetype index);
 
-    qptrdiff addByteData(const char *block, qsizetype len)
+    static qptrdiff addByteDataImpl(QByteArray &target, QByteArray::size_type &targetUsed,
+                                    const char *block, qsizetype len)
     {
         // This function does not do overflow checking, since the len parameter
         // is expected to be trusted. There's another version of this function
         // in decodeStringFromCbor(), which checks.
 
-        qptrdiff offset = data.size();
+        qptrdiff offset = target.size();
 
         // align offset
         offset += alignof(QtCbor::ByteData) - 1;
@@ -122,10 +123,10 @@ public:
 
         qptrdiff increment = qptrdiff(sizeof(QtCbor::ByteData)) + len;
 
-        usedData += increment;
-        data.resize(offset + increment);
+        targetUsed += increment;
+        target.resize(offset + increment);
 
-        char *ptr = data.begin() + offset;
+        char *ptr = target.begin() + offset;
         auto b = new (ptr) QtCbor::ByteData;
         b->len = len;
         if (block)
@@ -134,6 +135,11 @@ public:
         return offset;
     }
 
+    qptrdiff addByteData(const char *block, qsizetype len)
+    {
+        return addByteDataImpl(data, usedData, block, len);
+    }
+
     const QtCbor::ByteData *byteData(QtCbor::Element e) const
     {
         if ((e.flags & QtCbor::Element::HasByteData) == 0)