Add zero-termination checks to QString and QByteArray tests

This uses an alternative approach to the testing formerly introduced
in 4ef5a626. Zero-termination tests are injected into all QCOMPARE/QTEST
invocations. This makes such testing more thorough and widespread, and
gets seamlessly extended by future tests.

It also fixes an issue uncovered by the test where using a past-the-end
position with QString::insert(pos, char), could move uninitialized data
and clobber the null-terminator.

Change-Id: I7392580245b419ee65c3ae6f261b6e851d66dd4f
Reviewed-by: Jędrzej Nowacki <jedrzej.nowacki@nokia.com>

diff --git a/src/corelib/tools/qstring.cpp b/src/corelib/tools/qstring.cpp
index 710aec9..f551e32 100644 (file)
--- a/src/corelib/tools/qstring.cpp
+++ b/src/corelib/tools/qstring.cpp
@@ -1501,7 +1501,7 @@ QString& QString::insert(int i, QChar ch)
     if (i < 0)
         return *this;
     expand(qMax(i, d->size));
-    ::memmove(d->data() + i + 1, d->data() + i, (d->size - i) * sizeof(QChar));
+    ::memmove(d->data() + i + 1, d->data() + i, (d->size - i - 1) * sizeof(QChar));
     d->data()[i] = ch.unicode();
     return *this;
 }

diff --git a/tests/auto/corelib/tools/qbytearray/tst_qbytearray.cpp b/tests/auto/corelib/tools/qbytearray/tst_qbytearray.cpp
index ea6f745..a30ecb7 100644 (file)
--- a/tests/auto/corelib/tools/qbytearray/tst_qbytearray.cpp
+++ b/tests/auto/corelib/tools/qbytearray/tst_qbytearray.cpp
@@ -180,6 +180,65 @@ static const QByteArrayDataPtr staticNotNullTerminated = { const_cast<QByteArray
 static const QByteArrayDataPtr staticShifted = { const_cast<QByteArrayData *>(&statics.shifted.data) };
 static const QByteArrayDataPtr staticShiftedNotNullTerminated = { const_cast<QByteArrayData *>(&statics.shiftedNotNullTerminated.data) };
 
+template <class T> const T &verifyZeroTermination(const T &t) { return t; }
+
+QByteArray verifyZeroTermination(const QByteArray &ba)
+{
+    // This test does some evil stuff, it's all supposed to work.
+
+    QByteArray::DataPtr baDataPtr = const_cast<QByteArray &>(ba).data_ptr();
+
+    // Skip if isStatic() or fromRawData(), as those offer no guarantees
+    if (baDataPtr->ref.isStatic()
+            || baDataPtr->offset != QByteArray().data_ptr()->offset)
+        return ba;
+
+    int baSize = ba.size();
+    char baTerminator = ba.constData()[baSize];
+    if ('\0' != baTerminator)
+        return QString::fromAscii(
+            "*** Result ('%1') not null-terminated: 0x%2 ***").arg(QString::fromAscii(ba))
+                .arg(baTerminator, 2, 16, QChar('0')).toAscii();
+
+    // Skip mutating checks on shared strings
+    if (baDataPtr->ref.isShared())
+        return ba;
+
+    const char *baData = ba.constData();
+    const QByteArray baCopy(baData, baSize); // Deep copy
+
+    const_cast<char *>(baData)[baSize] = 'x';
+    if ('x' != ba.constData()[baSize]) {
+        return QString::fromAscii("*** Failed to replace null-terminator in "
+                "result ('%1') ***").arg(QString::fromAscii(ba)).toAscii();
+    }
+    if (ba != baCopy) {
+        return QString::fromAscii( "*** Result ('%1') differs from its copy "
+                "after null-terminator was replaced ***").arg(QString::fromAscii(ba)).toAscii();
+    }
+    const_cast<char *>(baData)[baSize] = '\0'; // Restore sanity
+
+    return ba;
+}
+
+// Overriding QTest's QCOMPARE, to check QByteArray for null termination
+#undef QCOMPARE
+#define QCOMPARE(actual, expected)                                      \
+    do {                                                                \
+        if (!QTest::qCompare(verifyZeroTermination(actual), expected,   \
+                #actual, #expected, __FILE__, __LINE__))                \
+            return;                                                     \
+    } while (0)                                                         \
+    /**/
+#undef QTEST
+#define QTEST(actual, testElement)                                      \
+    do {                                                                \
+        if (!QTest::qTest(verifyZeroTermination(actual), testElement,   \
+                #actual, #testElement, __FILE__, __LINE__))             \
+            return;                                                     \
+    } while (0)                                                         \
+    /**/
+
 tst_QByteArray::tst_QByteArray()
 {
     qRegisterMetaType<qulonglong>("qulonglong");

diff --git a/tests/auto/corelib/tools/qstring/tst_qstring.cpp b/tests/auto/corelib/tools/qstring/tst_qstring.cpp
index 7d4a9b5..9739448 100644 (file)
--- a/tests/auto/corelib/tools/qstring/tst_qstring.cpp
+++ b/tests/auto/corelib/tools/qstring/tst_qstring.cpp
@@ -229,6 +229,65 @@ private slots:
     void assignQLatin1String();
 };
 
+template <class T> const T &verifyZeroTermination(const T &t) { return t; }
+
+QString verifyZeroTermination(const QString &str)
+{
+    // This test does some evil stuff, it's all supposed to work.
+
+    QString::DataPtr strDataPtr = const_cast<QString &>(str).data_ptr();
+
+    // Skip if isStatic() or fromRawData(), as those offer no guarantees
+    if (strDataPtr->ref.isStatic()
+            || strDataPtr->offset != QString().data_ptr()->offset)
+        return str;
+
+    int strSize = str.size();
+    QChar strTerminator = str.constData()[strSize];
+    if (QChar('\0') != strTerminator)
+        return QString::fromAscii(
+            "*** Result ('%1') not null-terminated: 0x%2 ***").arg(str)
+                .arg(strTerminator.unicode(), 4, 16, QChar('0'));
+
+    // Skip mutating checks on shared strings
+    if (strDataPtr->ref.isShared())
+        return str;
+
+    const QChar *strData = str.constData();
+    const QString strCopy(strData, strSize); // Deep copy
+
+    const_cast<QChar *>(strData)[strSize] = QChar('x');
+    if (QChar('x') != str.constData()[strSize]) {
+        return QString::fromAscii("*** Failed to replace null-terminator in "
+                "result ('%1') ***").arg(str);
+    }
+    if (str != strCopy) {
+        return QString::fromAscii( "*** Result ('%1') differs from its copy "
+                "after null-terminator was replaced ***").arg(str);
+    }
+    const_cast<QChar *>(strData)[strSize] = QChar('\0'); // Restore sanity
+
+    return str;
+}
+
+// Overriding QTest's QCOMPARE, to check QString for null termination
+#undef QCOMPARE
+#define QCOMPARE(actual, expected)                                      \
+    do {                                                                \
+        if (!QTest::qCompare(verifyZeroTermination(actual), expected,   \
+                #actual, #expected, __FILE__, __LINE__))                \
+            return;                                                     \
+    } while (0)                                                         \
+    /**/
+#undef QTEST
+#define QTEST(actual, testElement)                                      \
+    do {                                                                \
+        if (!QTest::qTest(verifyZeroTermination(actual), testElement,   \
+                #actual, #testElement, __FILE__, __LINE__))             \
+            return;                                                     \
+    } while (0)                                                         \
+    /**/
+
 typedef QList<int> IntList;
 
 Q_DECLARE_METATYPE(QList<QVariant>)