From ba33685ad0e88e99aab0fe9eff6c7a4591d9f525 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Mon, 14 Aug 2023 18:48:31 +0200 Subject: [PATCH] QString: fix append() wrt. raw data When appending to an empty string, we optimize and copy the internal pointer. But if the other string was created with fromRawData this might be temporary data on the stack/heap and might be de-allocated or overwritten before the string is used or is forced to make a deep-copy. This would lead to incorrect data being used. This is easy to overlook if you plan to append multiple strings together, potentially supplied through an argument. Upon appending a second string it would make a full copy, but there might not be a guarantee for that. So, it's hard for users to avoid this pitfall! Fixes: QTBUG-115752 Change-Id: Ia9aa5f463121c2ce2e0e8eee8a6c8612b7297f2b Reviewed-by: Ahmad Samir Reviewed-by: Thiago Macieira (cherry picked from commit 4660a230d527a9cffda41999103aba6ff5c2ecd5) Reviewed-by: Qt CI Bot Reviewed-by: Marc Mutz --- src/corelib/text/qstring.cpp | 4 ++++ .../corelib/text/qbytearray/tst_qbytearray.cpp | 15 +++++++++++++++ tests/auto/corelib/text/qstring/tst_qstring.cpp | 17 +++++++++++++++++ 3 files changed, 36 insertions(+) diff --git a/src/corelib/text/qstring.cpp b/src/corelib/text/qstring.cpp index 9cecd7b8269..d06ac392a89 100644 --- a/src/corelib/text/qstring.cpp +++ b/src/corelib/text/qstring.cpp @@ -2882,6 +2882,10 @@ QString &QString::append(const QString &str) if (!str.isNull()) { if (isNull()) { operator=(str); + if (Q_UNLIKELY(!d.isMutable() && d.size > 0)) { + d.detach(); // fromRawData, so we do a deep copy + d.data()[d.size] = u'\0'; + } } else if (str.size()) { append(str.constData(), str.size()); } diff --git a/tests/auto/corelib/text/qbytearray/tst_qbytearray.cpp b/tests/auto/corelib/text/qbytearray/tst_qbytearray.cpp index cd750aa268b..f38c8ee681e 100644 --- a/tests/auto/corelib/text/qbytearray/tst_qbytearray.cpp +++ b/tests/auto/corelib/text/qbytearray/tst_qbytearray.cpp @@ -81,6 +81,7 @@ private slots: void prependExtended_data(); void prependExtended(); void append(); + void appendFromRawData(); void appendExtended_data(); void appendExtended(); void insert(); @@ -1142,6 +1143,20 @@ void tst_QByteArray::append() } } +void tst_QByteArray::appendFromRawData() +{ + char rawData[] = "Hello World!"; + QByteArray ba = QByteArray::fromRawData(rawData, std::size(rawData) - 1); + + QByteArray copy; + copy.append(ba); + QCOMPARE(copy, ba); + // We make an _actual_ copy, because appending a byte array + // created with fromRawData() might be optimized to copy the DataPointer, + // which means we may point to temporary stack data. + QVERIFY(copy.constData() != ba.constData()); +} + void tst_QByteArray::appendExtended_data() { prependExtended_data(); diff --git a/tests/auto/corelib/text/qstring/tst_qstring.cpp b/tests/auto/corelib/text/qstring/tst_qstring.cpp index 861f2b77e21..7ea7c4ba0aa 100644 --- a/tests/auto/corelib/text/qstring/tst_qstring.cpp +++ b/tests/auto/corelib/text/qstring/tst_qstring.cpp @@ -434,6 +434,8 @@ private slots: void append_bytearray_special_cases_data(); void append_bytearray_special_cases(); + void appendFromRawData(); + void operator_pluseq_qstring() { operator_pluseq_impl(); } void operator_pluseq_qstring_data() { operator_pluseq_data(); } void operator_pluseq_qstringview() { operator_pluseq_impl(); } @@ -3015,6 +3017,21 @@ void tst_QString::append_bytearray_special_cases() } } +void tst_QString::appendFromRawData() +{ + const char16_t utf[] = u"Hello World!"; + auto *rawData = reinterpret_cast(utf); + QString str = QString::fromRawData(rawData, std::size(utf) - 1); + + QString copy; + copy.append(str); + QCOMPARE(copy, str); + // We make an _actual_ copy, because appending a byte array + // created with fromRawData() might be optimized to copy the DataPointer, + // which means we may point to temporary stack data. + QVERIFY(copy.constData() != str.constData()); +} + void tst_QString::operator_pluseq_special_cases() { {