From 4660a230d527a9cffda41999103aba6ff5c2ecd5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= <marten.nordheim@qt.io>
Date: Mon, 14 Aug 2023 18:48:31 +0200
Subject: [PATCH] QString/QByteArray: fix append() wrt. raw data

When appending to an empty string or byte array, we optimize and
copy the internal pointer. But if the other string/byte array was
created with fromRawData this might be temporary data on the stack/heap
and might be de-allocated or overwritten before the string/byte array
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
Pick-to: 6.6 6.5 6.2
Change-Id: Ia9aa5f463121c2ce2e0e8eee8a6c8612b7297f2b
Reviewed-by: Ahmad Samir <a.samirh78@gmail.com>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
---
 src/corelib/text/qbytearray.cpp                 |  5 ++++-
 src/corelib/text/qstring.cpp                    |  5 ++++-
 .../corelib/text/qbytearray/tst_qbytearray.cpp  | 15 +++++++++++++++
 tests/auto/corelib/text/qstring/tst_qstring.cpp | 17 +++++++++++++++++
 4 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/src/corelib/text/qbytearray.cpp b/src/corelib/text/qbytearray.cpp
index f53a5407031..794472680b4 100644
--- a/src/corelib/text/qbytearray.cpp
+++ b/src/corelib/text/qbytearray.cpp
@@ -2039,7 +2039,10 @@ QByteArray &QByteArray::append(const QByteArray &ba)
 {
     if (!ba.isNull()) {
         if (isNull()) {
-            operator=(ba);
+            if (Q_UNLIKELY(!ba.d.isMutable()))
+                assign(ba); // fromRawData, so we do a deep copy
+            else
+                operator=(ba);
         } else if (ba.size()) {
             append(QByteArrayView(ba));
         }
diff --git a/src/corelib/text/qstring.cpp b/src/corelib/text/qstring.cpp
index f20eee89fc5..b319b706406 100644
--- a/src/corelib/text/qstring.cpp
+++ b/src/corelib/text/qstring.cpp
@@ -3144,7 +3144,10 @@ QString &QString::append(const QString &str)
 {
     if (!str.isNull()) {
         if (isNull()) {
-            operator=(str);
+            if (Q_UNLIKELY(!str.d.isMutable()))
+                assign(str); // fromRawData, so we do a deep copy
+            else
+                operator=(str);
         } 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 f5e149836ab..ec418edcdcb 100644
--- a/tests/auto/corelib/text/qbytearray/tst_qbytearray.cpp
+++ b/tests/auto/corelib/text/qbytearray/tst_qbytearray.cpp
@@ -51,6 +51,7 @@ private slots:
     void prependExtended_data();
     void prependExtended();
     void append();
+    void appendFromRawData();
     void appendExtended_data();
     void appendExtended();
     void appendEmptyNull();
@@ -914,6 +915,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.
+    QCOMPARE_NE((void *)copy.constData(), (void *)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 46734a63cc2..f2fc6ceb6f2 100644
--- a/tests/auto/corelib/text/qstring/tst_qstring.cpp
+++ b/tests/auto/corelib/text/qstring/tst_qstring.cpp
@@ -469,6 +469,8 @@ private slots:
     void append_bytearray_special_cases();
 #endif
 
+    void appendFromRawData();
+
     void operator_pluseq_qstring()            { operator_pluseq_impl<QString>(); }
     void operator_pluseq_qstring_data()       { operator_pluseq_data(); }
     void operator_pluseq_qstringview()        { operator_pluseq_impl<QStringView, QString &(QString::*)(QStringView)>(); }
@@ -3404,6 +3406,21 @@ void tst_QString::append_bytearray_special_cases()
 }
 #endif // !defined(QT_RESTRICTED_CAST_FROM_ASCII) && !defined(QT_NO_CAST_FROM_ASCII)
 
+void tst_QString::appendFromRawData()
+{
+    const char16_t utf[] = u"Hello World!";
+    auto *rawData = reinterpret_cast<const QChar *>(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.
+    QCOMPARE_NE((void *)copy.constData(), (void *)str.constData());
+}
+
 void tst_QString::assign()
 {
     // QString &assign(QAnyStringView)