Verify returns from QXmlTestLogger's xmlQuote() and xmlCdata()

Using partially-converted text would lead to invalid XML, so don't use
the buffer contents if the return is zero. As a result,
QTestJUnitStreamer::formatEnd() needs to return some indication of
whether *it* succeeded, so change it to forward their int returns;
and, as it's in fact only used internally by the streamer, make it
private.

Make these functions [[nodiscard]] so that further uses of them will
be discouraged from ignoring the possibility of failure. Make the
public versions return bool so that they can correctly succeed on
empty input. Assert various conditions we can infer to save work we
don't need to do.

Change-Id: I899bad23d1dfbd05fc725de269def4ce213dbc5a
Reviewed-by: Tor Arne Vestbø <tor.arne.vestbo@qt.io>

4 files changed, 113 insertions, 95 deletions

diff --git a/src/testlib/qtestjunitstreamer.cpp b/src/testlib/qtestjunitstreamer.cpp
index 72ab7d1534..3e67e5c4bf 100644
--- a/src/testlib/qtestjunitstreamer.cpp
+++ b/src/testlib/qtestjunitstreamer.cpp
@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2016 The Qt Company Ltd.
+** Copyright (C) 2021 The Qt Company Ltd.
 ** Contact: https://www.qt.io/licensing/
 **
 ** This file is part of the QtTest module of the Qt Toolkit.
@@ -106,23 +106,26 @@ void QTestJUnitStreamer::formatEnd(const QTestElement *element, QTestCharBuffer
     QTest::qt_asprintf(formatted, "%s</%s>\n", indent, element->elementName());
 }
 
-void QTestJUnitStreamer::formatAttributes(const QTestElement* element, const QTestElementAttribute *attribute, QTestCharBuffer *formatted) const
+bool QTestJUnitStreamer::formatAttributes(const QTestElement* element,
+                                          const QTestElementAttribute *attribute,
+                                          QTestCharBuffer *formatted) const
 {
     if (!attribute || !formatted )
-        return;
+        return false;
 
     QTest::AttributeIndex attrindex = attribute->index();
 
     if (element && element->elementType() == QTest::LET_Text) {
         QTEST_ASSERT(attrindex == QTest::AI_Value);
-        QXmlTestLogger::xmlCdata(formatted, attribute->value());
-        return;
+        return QXmlTestLogger::xmlCdata(formatted, attribute->value());
     }
 
     QTestCharBuffer quotedValue;
-    QXmlTestLogger::xmlQuote(&quotedValue, attribute->value());
-    QTest::qt_asprintf(formatted, " %s=\"%s\"",
-        attribute->name(), quotedValue.constData());
+    if (QXmlTestLogger::xmlQuote(&quotedValue, attribute->value())) {
+        return QTest::qt_asprintf(formatted, " %s=\"%s\"",
+                                  attribute->name(), quotedValue.constData()) != 0;
+    }
+    return false;
 }
 
 void QTestJUnitStreamer::formatAfterAttributes(const QTestElement *element, QTestCharBuffer *formatted) const
@@ -176,8 +179,8 @@ void QTestJUnitStreamer::outputElementAttributes(const QTestElement* element, co
     QTestCharBuffer buf;
 
     for (auto *attribute : attributes) {
-        formatAttributes(element, attribute, &buf);
-        outputString(buf.data());
+        if (formatAttributes(element, attribute, &buf))
+            outputString(buf.data());
     }
 }
 
diff --git a/src/testlib/qtestjunitstreamer_p.h b/src/testlib/qtestjunitstreamer_p.h
index 17c8a0383c..83b070d856 100644
--- a/src/testlib/qtestjunitstreamer_p.h
+++ b/src/testlib/qtestjunitstreamer_p.h
@@ -1,6 +1,6 @@
 /****************************************************************************
 **
-** Copyright (C) 2016 The Qt Company Ltd.
+** Copyright (C) 2021 The Qt Company Ltd.
 ** Contact: https://www.qt.io/licensing/
 **
 ** This file is part of the QtTest module of the Qt Toolkit.
@@ -71,7 +71,6 @@ class QTestJUnitStreamer
         void formatStart(const QTestElement *element, QTestCharBuffer *formatted) const;
         void formatEnd(const QTestElement *element, QTestCharBuffer *formatted) const;
         void formatAfterAttributes(const QTestElement *element, QTestCharBuffer *formatted) const;
-        void formatAttributes(const QTestElement *element, const QTestElementAttribute *attribute, QTestCharBuffer *formatted) const;
         void output(QTestElement *element) const;
         void outputElements(const std::vector<QTestElement*> &) const;
         void outputElementAttributes(const QTestElement *element, const std::vector<QTestElementAttribute*> &attributes) const;
@@ -79,6 +78,9 @@ class QTestJUnitStreamer
         void outputString(const char *msg) const;
 
     private:
+        [[nodiscard]] bool formatAttributes(const QTestElement *element,
+                                            const QTestElementAttribute *attribute,
+                                            QTestCharBuffer *formatted) const;
         static void indentForElement(const QTestElement* element, char* buf, int size);
 
         QJUnitTestLogger *testLogger;
diff --git a/src/testlib/qxmltestlogger.cpp b/src/testlib/qxmltestlogger.cpp
index 89c4cbb765..0b63b8b753 100644
--- a/src/testlib/qxmltestlogger.cpp
+++ b/src/testlib/qxmltestlogger.cpp
@@ -116,23 +116,28 @@ void QXmlTestLogger::startLogging()
 
     if (xmlmode == QXmlTestLogger::Complete) {
         QTestCharBuffer quotedTc;
-        xmlQuote(&quotedTc, QTestResult::currentTestObjectName());
-        QTest::qt_asprintf(&buf,
-                "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
-                "<TestCase name=\"%s\">\n", quotedTc.constData());
+        QTest::qt_asprintf(&buf, "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n");
         outputString(buf.constData());
+        if (xmlQuote(&quotedTc, QTestResult::currentTestObjectName())) {
+            QTest::qt_asprintf(&buf, "<TestCase name=\"%s\">\n", quotedTc.constData());
+            outputString(buf.constData());
+        } else {
+            // Unconditional end-tag => omitting the start tag is bad.
+            Q_ASSERT_X(false, "QXmlTestLogger::startLogging",
+                       "Insanely long test-case name or OOM issue");
+        }
     }
 
     QTestCharBuffer quotedBuild;
-    xmlQuote(&quotedBuild, QLibraryInfo::build());
-
-    QTest::qt_asprintf(&buf,
-                "<Environment>\n"
-                "    <QtVersion>%s</QtVersion>\n"
-                "    <QtBuild>%s</QtBuild>\n"
-                "    <QTestVersion>" QTEST_VERSION_STR "</QTestVersion>\n"
-                "</Environment>\n", qVersion(), quotedBuild.constData());
-    outputString(buf.constData());
+    if (!QLibraryInfo::build() || xmlQuote(&quotedBuild, QLibraryInfo::build())) {
+        QTest::qt_asprintf(&buf,
+                           "<Environment>\n"
+                           "    <QtVersion>%s</QtVersion>\n"
+                           "    <QtBuild>%s</QtBuild>\n"
+                           "    <QTestVersion>" QTEST_VERSION_STR "</QTestVersion>\n"
+                           "</Environment>\n", qVersion(), quotedBuild.constData());
+        outputString(buf.constData());
+    }
 }
 
 void QXmlTestLogger::stopLogging()
@@ -142,20 +147,24 @@ void QXmlTestLogger::stopLogging()
     QTest::qt_asprintf(&buf, "<Duration msecs=\"%s\"/>\n",
         QString::number(QTestLog::msecsTotalTime()).toUtf8().constData());
     outputString(buf.constData());
-    if (xmlmode == QXmlTestLogger::Complete) {
+    if (xmlmode == QXmlTestLogger::Complete)
         outputString("</TestCase>\n");
-    }
 
     QAbstractTestLogger::stopLogging();
 }
 
 void QXmlTestLogger::enterTestFunction(const char *function)
 {
-    QTestCharBuffer buf;
     QTestCharBuffer quotedFunction;
-    xmlQuote(&quotedFunction, function);
-    QTest::qt_asprintf(&buf, "<TestFunction name=\"%s\">\n", quotedFunction.constData());
-    outputString(buf.constData());
+    if (xmlQuote(&quotedFunction, function)) {
+        QTestCharBuffer buf;
+        QTest::qt_asprintf(&buf, "<TestFunction name=\"%s\">\n", quotedFunction.constData());
+        outputString(buf.constData());
+    } else {
+        // Unconditional end-tag => omitting the start tag is bad.
+        Q_ASSERT_X(false, "QXmlTestLogger::enterTestFunction",
+                   "Insanely long test-function name or OOM issue");
+    }
 }
 
 void QXmlTestLogger::leaveTestFunction()
@@ -239,41 +248,40 @@ void QXmlTestLogger::addIncident(IncidentTypes type, const char *description,
     QTestCharBuffer cdataTag;
     QTestCharBuffer cdataDescription;
 
-    xmlQuote(&quotedFile, file);
-    xmlCdata(&cdataGtag, gtag);
-    xmlCdata(&cdataTag, tag);
-    xmlCdata(&cdataDescription, description);
+    if (xmlQuote(&quotedFile, file)
+        && xmlCdata(&cdataGtag, gtag)
+        && xmlCdata(&cdataTag, tag)
+        && xmlCdata(&cdataDescription, description)) {
 
-    QTest::qt_asprintf(&buf,
-            QTest::incidentFormatString(QTest::isEmpty(description), notag),
-            QTest::xmlIncidentType2String(type),
-            quotedFile.constData(), line,
-            cdataGtag.constData(),
-            filler,
-            cdataTag.constData(),
-            cdataDescription.constData());
+        QTest::qt_asprintf(&buf,
+                           QTest::incidentFormatString(QTest::isEmpty(description), notag),
+                           QTest::xmlIncidentType2String(type),
+                           quotedFile.constData(), line,
+                           cdataGtag.constData(),
+                           filler,
+                           cdataTag.constData(),
+                           cdataDescription.constData());
 
-    outputString(buf.constData());
+        outputString(buf.constData());
+    }
 }
 
 void QXmlTestLogger::addBenchmarkResult(const QBenchmarkResult &result)
 {
-    QTestCharBuffer buf;
     QTestCharBuffer quotedMetric;
     QTestCharBuffer quotedTag;
 
-    xmlQuote(&quotedMetric,
-        benchmarkMetricName(result.metric));
-    xmlQuote(&quotedTag, result.context.tag.toUtf8().constData());
-
-    QTest::qt_asprintf(
-        &buf,
-        QTest::benchmarkResultFormatString(),
-        quotedMetric.constData(),
-        quotedTag.constData(),
-        result.value / double(result.iterations),
-        result.iterations);
-    outputString(buf.constData());
+    if (xmlQuote(&quotedMetric, benchmarkMetricName(result.metric))
+        && xmlQuote(&quotedTag, result.context.tag.toUtf8().constData())) {
+        QTestCharBuffer buf;
+        QTest::qt_asprintf(&buf,
+                           QTest::benchmarkResultFormatString(),
+                           quotedMetric.constData(),
+                           quotedTag.constData(),
+                           result.value / double(result.iterations),
+                           result.iterations);
+        outputString(buf.constData());
+    }
 }
 
 void QXmlTestLogger::addMessage(MessageTypes type, const QString &message,
@@ -290,30 +298,32 @@ void QXmlTestLogger::addMessage(MessageTypes type, const QString &message,
     QTestCharBuffer cdataTag;
     QTestCharBuffer cdataDescription;
 
-    xmlQuote(&quotedFile, file);
-    xmlCdata(&cdataGtag, gtag);
-    xmlCdata(&cdataTag, tag);
-    xmlCdata(&cdataDescription, message.toUtf8().constData());
-
-    QTest::qt_asprintf(&buf,
-            QTest::messageFormatString(message.isEmpty(), notag),
-            QTest::xmlMessageType2String(type),
-            quotedFile.constData(), line,
-            cdataGtag.constData(),
-            filler,
-            cdataTag.constData(),
-            cdataDescription.constData());
+    if (xmlQuote(&quotedFile, file)
+        && xmlCdata(&cdataGtag, gtag)
+        && xmlCdata(&cdataTag, tag)
+        && xmlCdata(&cdataDescription, message.toUtf8().constData())) {
+        QTest::qt_asprintf(&buf,
+                           QTest::messageFormatString(message.isEmpty(), notag),
+                           QTest::xmlMessageType2String(type),
+                           quotedFile.constData(), line,
+                           cdataGtag.constData(),
+                           filler,
+                           cdataTag.constData(),
+                           cdataDescription.constData());
 
-    outputString(buf.constData());
+        outputString(buf.constData());
+    }
 }
 
 int QXmlTestLogger::xmlQuote(QTestCharBuffer *destBuf, char const *src, qsizetype n)
 {
-    Q_ASSERT(n > 0 && destBuf->size() >= n);
-
+    // QTestCharBuffer initially has size 512, with '\0' at the start of its
+    // data; and we only grow it.
+    Q_ASSERT(n >= 512 && destBuf->size() == n);
     char *dest = destBuf->data();
-    if (!src || n == 1) {
-        *dest = '\0';
+
+    if (!src || !*src) {
+        Q_ASSERT(!dest[0]);
         return 0;
     }
 
@@ -359,19 +369,19 @@ int QXmlTestLogger::xmlQuote(QTestCharBuffer *destBuf, char const *src, qsizetyp
         }
     }
 
-    // If we get here, dest was completely filled (dest == end)
-    dest[-1] = '\0';
-    return dest - begin;
+    // If we get here, dest was completely filled:
+    Q_ASSERT(dest == end && end > begin);
+    dest[-1] = '\0'; // hygiene, but it'll be ignored
+    return n;
 }
 
 int QXmlTestLogger::xmlCdata(QTestCharBuffer *destBuf, char const *src, qsizetype n)
 {
-    Q_ASSERT(n > 0 && destBuf->size() >= n);
-
+    Q_ASSERT(n >= 512 && destBuf->size() == n);
     char *dest = destBuf->data();
 
-    if (!src || n == 1) {
-        *dest = '\0';
+    if (!src || !*src) {
+        Q_ASSERT(!dest[0]);
         return 0;
     }
 
@@ -404,10 +414,10 @@ int QXmlTestLogger::xmlCdata(QTestCharBuffer *destBuf, char const *src, qsizetyp
         ++dest;
     }
 
-    // If we get here, dest was completely filled:
+    // If we get here, dest was completely filled; caller shall grow and retry:
     Q_ASSERT(dest == end && end > begin);
-    dest[-1] = '\0';
-    return dest - begin;
+    dest[-1] = '\0'; // hygiene, but it'll be ignored
+    return n;
 }
 
 typedef int (*StringFormatFunction)(QTestCharBuffer *, char const *, qsizetype);
@@ -416,21 +426,24 @@ typedef int (*StringFormatFunction)(QTestCharBuffer *, char const *, qsizetype);
     A wrapper for string functions written to work with a fixed size buffer so they can be called
     with a dynamically allocated buffer.
 */
-int allocateStringFn(QTestCharBuffer *str, char const *src, StringFormatFunction func)
+static bool allocateStringFn(QTestCharBuffer *str, char const *src, StringFormatFunction func)
 {
     constexpr int MAXSIZE = 1024 * 1024 * 2;
     int size = str->size();
+    Q_ASSERT(size >= 512 && !str->data()[0]);
 
     do {
         const int res = func(str, src, size);
-        if (res < size) // Success or fatal failure
-            return res;
+        if (res < size) { // Success
+            Q_ASSERT(res > 0 || (!res && (!src || !src[0])));
+            return true;
+        }
 
         // Buffer wasn't big enough, try again, if not too big:
         size *= 2;
     } while (size <= MAXSIZE && str->reset(size));
 
-    return 0;
+    return false;
 }
 
 /*
@@ -442,7 +455,7 @@ int allocateStringFn(QTestCharBuffer *str, char const *src, StringFormatFunction
 
     Returns 0 on invalid or empty input, the actual length written on success.
 */
-int QXmlTestLogger::xmlQuote(QTestCharBuffer *str, char const *src)
+bool QXmlTestLogger::xmlQuote(QTestCharBuffer *str, char const *src)
 {
     return allocateStringFn(str, src, QXmlTestLogger::xmlQuote);
 }
@@ -455,7 +468,7 @@ int QXmlTestLogger::xmlQuote(QTestCharBuffer *str, char const *src)
 
     Returns 0 on invalid or empty input, the actual length written on success.
 */
-int QXmlTestLogger::xmlCdata(QTestCharBuffer *str, char const *src)
+bool QXmlTestLogger::xmlCdata(QTestCharBuffer *str, char const *src)
 {
     return allocateStringFn(str, src, QXmlTestLogger::xmlCdata);
 }
diff --git a/src/testlib/qxmltestlogger_p.h b/src/testlib/qxmltestlogger_p.h
index 8091dada59..e72b688545 100644
--- a/src/testlib/qxmltestlogger_p.h
+++ b/src/testlib/qxmltestlogger_p.h
@@ -77,11 +77,11 @@ public:
     void addMessage(MessageTypes type, const QString &message,
                     const char *file = nullptr, int line = 0) override;
 
-    static int xmlCdata(QTestCharBuffer *dest, char const *src);
-    static int xmlQuote(QTestCharBuffer *dest, char const *src);
+    [[nodiscard]] static bool xmlCdata(QTestCharBuffer *dest, char const *src);
+    [[nodiscard]] static bool xmlQuote(QTestCharBuffer *dest, char const *src);
 private:
-    static int xmlCdata(QTestCharBuffer *dest, char const *src, qsizetype n);
-    static int xmlQuote(QTestCharBuffer *dest, char const *src, qsizetype n);
+    [[nodiscard]] static int xmlCdata(QTestCharBuffer *dest, char const *src, qsizetype n);
+    [[nodiscard]] static int xmlQuote(QTestCharBuffer *dest, char const *src, qsizetype n);
 
     XmlMode xmlmode;
 };