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 <a.samirh78@gmail.com>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
(cherry picked from commit 4660a230d527a9cffda41999103aba6ff5c2ecd5)
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
(cherry picked from commit d4a6c81081db24812a043c44d0d3d683b53e7d80)
Reviewed-by: Daniel Smith <Daniel.Smith@qt.io>

3 files changed, 36 insertions, 1 deletions

diff --git a/src/corelib/text/qstring.cpp b/src/corelib/text/qstring.cpp
index 66838a2ec4..39ae476ff2 100644
--- a/src/corelib/text/qstring.cpp
+++ b/src/corelib/text/qstring.cpp
@@ -3131,7 +3131,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 084fb5c0e5..4c97640f69 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 assign();
@@ -911,6 +912,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 e6e3d6eefd..be59090b75 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)>(); }
@@ -3394,6 +3396,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)