QCborValue: avoid allocating result if data is insufficient

Similar to the previous commit which applied to QCborStreamReader, don't
allocate too much data before checking that the stream actually has that
much.

Fixes: QTBUG-88256
Change-Id: I7b9b97ae9b32412abdc6fffd16454b7568a063ba
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
(cherry picked from commit 638171eb10cfb186a6c47ec052a3b0c5b6449386)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>

5 files changed, 82 insertions, 59 deletions

diff --git a/src/corelib/serialization/qcborstreamreader.cpp b/src/corelib/serialization/qcborstreamreader.cpp
index ddc93e469f..d097979bda 100644
--- a/src/corelib/serialization/qcborstreamreader.cpp
+++ b/src/corelib/serialization/qcborstreamreader.cpp
@@ -681,6 +681,7 @@ public:
         bool isPlainPointer() const { return maxlen_or_type >= 0; }
     };
 
+    static QCborStreamReader::StringResultCode appendStringChunk(QCborStreamReader &reader, QByteArray *data);
     QCborStreamReader::StringResult<qsizetype> readStringChunk(ReadStringChunk params);
     bool ensureStringIteration();
 };
@@ -1479,6 +1480,21 @@ QCborStreamReader::readStringChunk(char *ptr, qsizetype maxlen)
     return r;
 }
 
+// used by qcborvalue.cpp
+QCborStreamReader::StringResultCode qt_cbor_append_string_chunk(QCborStreamReader &reader, QByteArray *data)
+{
+    return QCborStreamReaderPrivate::appendStringChunk(reader, data);
+}
+
+inline QCborStreamReader::StringResultCode
+QCborStreamReaderPrivate::appendStringChunk(QCborStreamReader &reader, QByteArray *data)
+{
+    auto status = reader.d->readStringChunk(data).status;
+    if (status == QCborStreamReader::EndOfString && reader.lastError() == QCborError::NoError)
+        reader.preparse();
+    return status;
+}
+
 Q_NEVER_INLINE QCborStreamReader::StringResult<qsizetype>
 QCborStreamReaderPrivate::readStringChunk(ReadStringChunk params)
 {
@@ -1547,17 +1563,23 @@ QCborStreamReaderPrivate::readStringChunk(ReadStringChunk params)
         ptr = params.ptr;
     } else if (params.isByteArray()) {
         // See note above on having ensured there is enough incoming data.
+        auto oldSize = params.array->size();
+        auto newSize = oldSize;
+        if (add_overflow<decltype(newSize)>(oldSize, toRead, &newSize)) {
+            handleError(CborErrorDataTooLarge);
+            return result;
+        }
         try {
-            params.array->resize(toRead);
+            params.array->resize(newSize);
         } catch (const std::bad_alloc &) {
             // the distinction between DataTooLarge and OOM is mostly for
             // compatibility with Qt 5; in Qt 6, we could consider everything
             // to be OOM.
-            handleError(toRead > MaxByteArraySize ? CborErrorDataTooLarge: CborErrorOutOfMemory);
+            handleError(newSize > MaxByteArraySize ? CborErrorDataTooLarge: CborErrorOutOfMemory);
             return result;
         }
 
-        ptr = const_cast<char *>(params.array->constData());
+        ptr = const_cast<char *>(params.array->constData()) + oldSize;
     }
 
     if (device) {
diff --git a/src/corelib/serialization/qcborvalue.cpp b/src/corelib/serialization/qcborvalue.cpp
index c7a2198644..f27ef94b4a 100644
--- a/src/corelib/serialization/qcborvalue.cpp
+++ b/src/corelib/serialization/qcborvalue.cpp
@@ -1556,31 +1556,37 @@ inline void QCborContainerPrivate::setErrorInReader(QCborStreamReader &reader, Q
     qt_cbor_stream_set_error(reader.d.data(), error);
 }
 
+extern QCborStreamReader::StringResultCode qt_cbor_append_string_chunk(QCborStreamReader &reader, QByteArray *data);
+
 void QCborContainerPrivate::decodeStringFromCbor(QCborStreamReader &reader)
 {
-    auto addByteData_local = [this](QByteArray::size_type len) -> qint64 {
-        // this duplicates a lot of addByteData, but with overflow checking
-        QByteArray::size_type newSize;
-        QByteArray::size_type increment = sizeof(QtCbor::ByteData);
-        QByteArray::size_type alignment = alignof(QtCbor::ByteData);
-        QByteArray::size_type offset = data.size();
-
-        // calculate the increment we want
-        if (add_overflow(increment, len, &increment))
-            return -1;
-
-        // align offset
-        if (add_overflow(offset, alignment - 1, &offset))
-            return -1;
-        offset &= ~(alignment - 1);
-
-        // and calculate the final size
-        if (add_overflow(offset, increment, &newSize))
-            return -1;
-        if (newSize > MaxByteArraySize)
+    // The use of size_t means none of the operations here can overflow because
+    // all inputs are less than half SIZE_MAX.
+    auto addByteData_local = [this](size_t len) -> qint64 {
+        constexpr size_t EstimatedOverhead = 16;
+        constexpr size_t MaxMemoryIncrement = 16384;
+        size_t offset = data.size();
+
+        // add space for aligned ByteData (this can't overflow)
+        offset += sizeof(QtCbor::ByteData) + alignof(QtCbor::ByteData);
+        offset &= ~(alignof(QtCbor::ByteData) - 1);
+        if (offset > size_t(MaxByteArraySize))
             return -1;
 
-        data.resize(newSize);
+        // and calculate the size we want to have
+        size_t newCapacity = offset + len;          // can't overflow
+        if (len > MaxMemoryIncrement - EstimatedOverhead) {
+            // there's a non-zero chance that we won't need this memory at all,
+            // so capa how much we allocate
+            newCapacity = offset + MaxMemoryIncrement - EstimatedOverhead;
+        }
+        if (newCapacity > size_t(MaxByteArraySize)) {
+            // this may cause an allocation failure
+            newCapacity = MaxByteArraySize;
+        }
+        if (newCapacity > size_t(data.capacity()))
+            data.reserve(newCapacity);
+        data.resize(offset + sizeof(QtCbor::ByteData));
         return offset;
     };
     auto dataPtr = [this]() {
@@ -1617,42 +1623,32 @@ void QCborContainerPrivate::decodeStringFromCbor(QCborStreamReader &reader)
 
     // read chunks
     bool isAscii = (e.type == QCborValue::String);
-    auto r = reader.readStringChunk(dataPtr() + e.value + sizeof(ByteData), len);
-    while (r.status == QCborStreamReader::Ok) {
+    QCborStreamReader::StringResultCode status = qt_cbor_append_string_chunk(reader, &data);
+    while (status == QCborStreamReader::Ok) {
         if (e.type == QCborValue::String && len) {
             // verify UTF-8 string validity
             auto utf8result = QUtf8::isValidUtf8(QByteArrayView(dataPtr(), data.size()).last(len));
             if (!utf8result.isValidUtf8) {
-                r.status = QCborStreamReader::Error;
+                status = QCborStreamReader::Error;
                 setErrorInReader(reader, { QCborError::InvalidUtf8String });
                 break;
             }
             isAscii = isAscii && utf8result.isValidAscii;
         }
 
-        // allocate space for the next chunk
         rawlen = reader.currentStringChunkSize();
         len = rawlen;
         if (len == rawlen) {
-            auto oldSize = data.size();
-            auto newSize = oldSize;
-            if (!add_overflow(newSize, len, &newSize) && newSize < MaxByteArraySize) {
-                if (newSize != oldSize)
-                    data.resize(newSize);
-
-                // read the chunk
-                r = reader.readStringChunk(dataPtr() + oldSize, len);
-                continue;
-            }
+            status = qt_cbor_append_string_chunk(reader, &data);
+        } else {
+            // error
+            status = QCborStreamReader::Error;
+            setErrorInReader(reader, { QCborError::DataTooLarge });
         }
-
-        // error
-        r.status = QCborStreamReader::Error;
-        setErrorInReader(reader, { QCborError::DataTooLarge });
     }
 
     // update size
-    if (r.status == QCborStreamReader::EndOfString && e.flags & Element::HasByteData) {
+    if (status == QCborStreamReader::EndOfString) {
         auto b = new (dataPtr() + e.value) ByteData;
         b->len = data.size() - e.value - int(sizeof(*b));
         usedData += b->len;
@@ -1667,14 +1663,12 @@ void QCborContainerPrivate::decodeStringFromCbor(QCborStreamReader &reader)
         if (e.type == QCborValue::String) {
             if (Q_UNLIKELY(b->len > MaxStringSize)) {
                 setErrorInReader(reader, { QCborError::DataTooLarge });
-                r.status = QCborStreamReader::Error;
+                status = QCborStreamReader::Error;
             }
         }
     }
 
-    if (r.status == QCborStreamReader::Error) {
-        // There can only be errors if there was data to be read.
-        Q_ASSERT(e.flags & Element::HasByteData);
+    if (status == QCborStreamReader::Error) {
         data.truncate(e.value);
         return;
     }
diff --git a/src/corelib/serialization/qcborvalue_p.h b/src/corelib/serialization/qcborvalue_p.h
index d68d44e7be..5c8f244ad8 100644
--- a/src/corelib/serialization/qcborvalue_p.h
+++ b/src/corelib/serialization/qcborvalue_p.h
@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2018 Intel Corporation.
+** Copyright (C) 2020 Intel Corporation.
 ** Contact: https://www.qt.io/licensing/
 **
 ** This file is part of the QtCore module of the Qt Toolkit.
@@ -53,6 +53,10 @@
 
 #include "qcborvalue.h"
 
+#if QT_CONFIG(cborstreamreader)
+#  include "qcborstreamreader.h"
+#endif
+
 #include <private/qglobal_p.h>
 #include <private/qstringconverter_p.h>
 
@@ -414,9 +418,11 @@ public:
         elements.remove(idx);
     }
 
-    void decodeValueFromCbor(QCborStreamReader &reader, int remainiingStackDepth);
+#if QT_CONFIG(cborstreamreader)
+    void decodeValueFromCbor(QCborStreamReader &reader, int remainingStackDepth);
     void decodeStringFromCbor(QCborStreamReader &reader);
     static inline void setErrorInReader(QCborStreamReader &reader, QCborError error);
+#endif
 };
 
 QT_END_NAMESPACE
diff --git a/tests/auto/corelib/serialization/cborlargedatavalidation.cpp b/tests/auto/corelib/serialization/cborlargedatavalidation.cpp
index 64191db1ef..c5cf1dc04e 100644
--- a/tests/auto/corelib/serialization/cborlargedatavalidation.cpp
+++ b/tests/auto/corelib/serialization/cborlargedatavalidation.cpp
@@ -87,25 +87,28 @@ void addValidationLargeData(qsizetype minInvalid, qsizetype maxInvalid)
         toolong[0] = sizeof(v) > 4 ? 0x5b : 0x5a;
         qToBigEndian(v, toolong + 1);
 
+        bool overflows = v > std::numeric_limits<qsizetype>::max() - 1 - qsizetype(sizeof(v));
+        CborError err = overflows ? CborErrorDataTooLarge : CborErrorUnexpectedEOF;
+
         QTest::addRow("bytearray-too-big-for-qbytearray-%llx", v)
-                << QByteArray(toolong, sizeof(toolong)) << 0 << CborErrorUnexpectedEOF;
+                << QByteArray(toolong, sizeof(toolong)) << 0 << err;
         QTest::addRow("bytearray-chunked-too-big-for-qbytearray-%llx", v)
                 << ('\x5f' + QByteArray(toolong, sizeof(toolong)) + '\xff')
-                << 0 << CborErrorUnexpectedEOF;
+                << 0 << err;
         QTest::addRow("bytearray-2chunked-too-big-for-qbytearray-%llx", v)
                 << ("\x5f\x40" + QByteArray(toolong, sizeof(toolong)) + '\xff')
-                << 0 << CborErrorUnexpectedEOF;
+                << 0 << err;
         toolong[0] |= 0x20;
 
         // QCborStreamReader::readString copies to a QByteArray first
         QTest::addRow("string-too-big-for-qbytearray-%llx", v)
-                << QByteArray(toolong, sizeof(toolong)) << 0 << CborErrorUnexpectedEOF;
+                << QByteArray(toolong, sizeof(toolong)) << 0 << err;
         QTest::addRow("string-chunked-too-big-for-qbytearray-%llx", v)
                 << ('\x7f' + QByteArray(toolong, sizeof(toolong)) + '\xff')
-                << 0 << CborErrorUnexpectedEOF;
+                << 0 << err;
         QTest::addRow("string-2chunked-too-big-for-qbytearray-%llx", v)
                 << ("\x7f\x60" + QByteArray(toolong, sizeof(toolong)) + '\xff')
-                << 0 << CborErrorUnexpectedEOF;
+                << 0 << err;
     }
 }
 
diff --git a/tests/auto/corelib/serialization/qcborvalue/tst_qcborvalue.cpp b/tests/auto/corelib/serialization/qcborvalue/tst_qcborvalue.cpp
index 64d4451f08..4ec5095f1c 100644
--- a/tests/auto/corelib/serialization/qcborvalue/tst_qcborvalue.cpp
+++ b/tests/auto/corelib/serialization/qcborvalue/tst_qcborvalue.cpp
@@ -2007,11 +2007,11 @@ void tst_QCborValue::validation_data()
     qToBigEndian(MinInvalid - 1, toolong + 1);
     QTest::addRow("bytearray-2chunked+1-too-big-for-qbytearray-%llx", MinInvalid)
             << ("\x5f\x41z" + QByteArray(toolong, sizeof(toolong)) + '\xff')
-            << 0 << CborErrorDataTooLarge;
+            << 0 << CborErrorUnexpectedEOF;
     toolong[0] |= 0x20;
     QTest::addRow("string-2chunked+1-too-big-for-qbytearray-%llx", MinInvalid)
             << ("\x7f\x61z" + QByteArray(toolong, sizeof(toolong)) + '\xff')
-            << 0 << CborErrorDataTooLarge;
+            << 0 << CborErrorUnexpectedEOF;
 
     // These tests say we have arrays and maps with very large item counts.
     // They are meant to ensure we don't pre-allocate a lot of memory
@@ -2042,7 +2042,6 @@ void tst_QCborValue::validation()
 
     QCborParserError parserError;
     QCborValue decoded = QCborValue::fromCbor(data, &parserError);
-    if (parserError.error != QCborError::DataTooLarge)      // ### temporary!!
     QCOMPARE(parserError.error, error);
 
     if (data.startsWith('\x81')) {
@@ -2050,7 +2049,6 @@ void tst_QCborValue::validation()
         char *ptr = const_cast<char *>(data.constData());
         QByteArray mid = QByteArray::fromRawData(ptr + 1, data.size() - 1);
         decoded = QCborValue::fromCbor(mid, &parserError);
-        if (parserError.error != QCborError::DataTooLarge)  // ### temporary!!
         QCOMPARE(parserError.error, error);
     }
 }