From 58bea7f3a2d0a88a56793c3878a7fcef74b4c646 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Sat, 7 Nov 2020 08:08:31 -0800 Subject: [PATCH] QCborStreamReader::next: don't allocate too much memory in a QBA Because CBOR strings are encoded in UTF-8, it's possible that the string that won't fit a QString in UTF-16 would still fit QByteArray in UTF-8 (e.g., anything US-ASCII and most Latin text). The previous solution was an improvement because we used to read into a QByteArray then convert the QByteArray to QString, thus using 3x the amount of memory (1x in QByteArray, 2x in QString). The previous commit skipped the middle allocation and made the regular readString() function do the decoding either directly on source memory or by reading in small chunks (16 kB). Future improvement for Qt 6.1: add readStringChunk(char16_t *, qsizetype) so we can do the validation / skipping at O(1) memory. Pick-to: 5.15 6.0 Change-Id: I7b9b97ae9b32412abdc6fffd1645458c655cc566 Reviewed-by: Allan Sandfeld Jensen --- .../serialization/qcborstreamreader.cpp | 25 +++++++++---------- .../serialization/cborlargedatavalidation.cpp | 19 +++++++------- .../tst_qcborstreamreader.cpp | 12 ++++++--- 3 files changed, 31 insertions(+), 25 deletions(-) diff --git a/src/corelib/serialization/qcborstreamreader.cpp b/src/corelib/serialization/qcborstreamreader.cpp index ef2ec08b884..f1bea6a1f17 100644 --- a/src/corelib/serialization/qcborstreamreader.cpp +++ b/src/corelib/serialization/qcborstreamreader.cpp @@ -1076,19 +1076,18 @@ bool QCborStreamReader::next(int maxRecursion) next(maxRecursion - 1); if (lastError() == QCborError::NoError) leaveContainer(); - } else if (isString() || isByteArray()) { - auto r = _readByteArray_helper(); - while (r.status == Ok) { - if (isString() && r.data.size() > MaxStringSize) { - d->handleError(CborErrorDataTooLarge); - break; - } - if (isString() && !QUtf8::isValidUtf8(r.data).isValidUtf8) { - d->handleError(CborErrorInvalidUtf8TextString); - break; - } - r = _readByteArray_helper(); - } + } else if (isByteArray()) { + char c; + StringResult r; + do { + r = readStringChunk(&c, 1); + } while (r.status == Ok); + } else if (isString()) { + // we need to use actual readString so we get UTF-8 validation + StringResult r; + do { + r = readString(); + } while (r.status == Ok); } else { // fixed types CborError err = cbor_value_advance_fixed(&d->currentElement); diff --git a/tests/auto/corelib/serialization/cborlargedatavalidation.cpp b/tests/auto/corelib/serialization/cborlargedatavalidation.cpp index c5cf1dc04e1..a9a31ddf572 100644 --- a/tests/auto/corelib/serialization/cborlargedatavalidation.cpp +++ b/tests/auto/corelib/serialization/cborlargedatavalidation.cpp @@ -117,6 +117,7 @@ void addValidationHugeDevice(qsizetype byteArrayInvalid, qsizetype stringInvalid qRegisterMetaType>(); QTest::addColumn>("device"); QTest::addColumn("expectedError"); + QTest::addColumn("expectedValidationError"); char buf[1 + sizeof(quint64)]; auto device = [&buf](QCborStreamReader::Type t, quint64 size) { @@ -130,21 +131,21 @@ void addValidationHugeDevice(qsizetype byteArrayInvalid, qsizetype stringInvalid // do the exact limits QTest::newRow("bytearray-just-too-big") - << device(QCborStreamReader::ByteArray, byteArrayInvalid) << CborErrorDataTooLarge; - - // TODO: Fix this to work for 64-bit. The test tries to allocate too much data and fails. - if (sizeof(size_t) == 4) { - QTest::newRow("string-just-too-big") - << device(QCborStreamReader::String, stringInvalid) << CborErrorDataTooLarge; - } + << device(QCborStreamReader::ByteArray, byteArrayInvalid) + << CborErrorDataTooLarge << CborNoError; + QTest::newRow("string-just-too-big") + << device(QCborStreamReader::String, stringInvalid) + << CborErrorDataTooLarge << CborErrorDataTooLarge; auto addSize = [=](const char *sizename, qint64 size) { if (byteArrayInvalid < size) QTest::addRow("bytearray-%s", sizename) - << device(QCborStreamReader::ByteArray, size) << CborErrorDataTooLarge; + << device(QCborStreamReader::ByteArray, size) + << CborErrorDataTooLarge << CborNoError; if (stringInvalid < size) QTest::addRow("string-%s", sizename) - << device(QCborStreamReader::String, size) << CborErrorDataTooLarge; + << device(QCborStreamReader::String, size) + << CborErrorDataTooLarge << CborErrorDataTooLarge; }; addSize("1GB", quint64(1) << 30); addSize("2GB", quint64(1) << 31); diff --git a/tests/auto/corelib/serialization/qcborstreamreader/tst_qcborstreamreader.cpp b/tests/auto/corelib/serialization/qcborstreamreader/tst_qcborstreamreader.cpp index 437d8c40ceb..f04f26b42fc 100644 --- a/tests/auto/corelib/serialization/qcborstreamreader/tst_qcborstreamreader.cpp +++ b/tests/auto/corelib/serialization/qcborstreamreader/tst_qcborstreamreader.cpp @@ -958,6 +958,7 @@ void tst_QCborStreamReader::hugeDeviceValidation() QFETCH(QSharedPointer, device); QFETCH(CborError, expectedError); + QFETCH(CborError, expectedValidationError); QCborError error = { QCborError::Code(expectedError) }; device->open(QIODevice::ReadOnly | QIODevice::Unbuffered); @@ -966,10 +967,15 @@ void tst_QCborStreamReader::hugeDeviceValidation() QVERIFY(parseOne(reader).isEmpty()); QCOMPARE(reader.lastError(), error); - // next() should fail reader.reset(); - QVERIFY(!reader.next()); - QCOMPARE(reader.lastError(), error); + error = { QCborError::Code(expectedValidationError) }; + if (error == QCborError{}) { + // this test actually succeeds, so don't do it to avoid large memory consumption + } else { + // next() should fail + QVERIFY(!reader.next()); + QCOMPARE(reader.lastError(), error); + } } static const int Recursions = 3;