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>

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) {

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 {
+    // The use of size_t means none of the operations here can overflow because
-        // this duplicates a lot of addByteData, but with overflow checking
+    // all inputs are less than half SIZE_MAX.
-        QByteArray::size_type newSize;
+    auto addByteData_local = [this](size_t len) -> qint64 {
-        QByteArray::size_type increment = sizeof(QtCbor::ByteData);
+        constexpr size_t EstimatedOverhead = 16;
-        QByteArray::size_type alignment = alignof(QtCbor::ByteData);
+        constexpr size_t MaxMemoryIncrement = 16384;
-        QByteArray::size_type offset = data.size();
+        size_t offset = data.size();
 
-        // calculate the increment we want
+        // add space for aligned ByteData (this can't overflow)
-        if (add_overflow(increment, len, &increment))
+        offset += sizeof(QtCbor::ByteData) + alignof(QtCbor::ByteData);
+        offset &= ~(alignof(QtCbor::ByteData) - 1);
+        if (offset > size_t(MaxByteArraySize))
             return -1;
 
-        // align offset
+        // and calculate the size we want to have
-        if (add_overflow(offset, alignment - 1, &offset))
+        size_t newCapacity = offset + len;          // can't overflow
-            return -1;
+        if (len > MaxMemoryIncrement - EstimatedOverhead) {
-        offset &= ~(alignment - 1);
+            // there's a non-zero chance that we won't need this memory at all,
-
+            // so capa how much we allocate
-        // and calculate the final size
+            newCapacity = offset + MaxMemoryIncrement - EstimatedOverhead;
-        if (add_overflow(offset, increment, &newSize))
+        }
-            return -1;
+        if (newCapacity > size_t(MaxByteArraySize)) {
-        if (newSize > MaxByteArraySize)
+            // this may cause an allocation failure
-            return -1;
+            newCapacity = MaxByteArraySize;
-
+        }
-        data.resize(newSize);
+        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);
+    QCborStreamReader::StringResultCode status = qt_cbor_append_string_chunk(reader, &data);
-    while (r.status == QCborStreamReader::Ok) {
+    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();
+            status = qt_cbor_append_string_chunk(reader, &data);
-            auto newSize = oldSize;
+        } else {
-            if (!add_overflow(newSize, len, &newSize) && newSize < MaxByteArraySize) {
-                if (newSize != oldSize)
-                    data.resize(newSize);
-
-                // read the chunk
-                r = reader.readStringChunk(dataPtr() + oldSize, len);
-                continue;
-            }
-        }
-
             // error
-        r.status = QCborStreamReader::Error;
+            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) {
+    if (status == QCborStreamReader::Error) {
-        // There can only be errors if there was data to be read.
-        Q_ASSERT(e.flags & Element::HasByteData);
         data.truncate(e.value);
         return;
     }

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

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;
     }
 }
 

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);
     }
 }