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>

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 QXmlTestLogger::xmlCdata(formatted, attribute->value());
-        return;
     }
 
     QTestCharBuffer quotedValue;
-    QXmlTestLogger::xmlQuote(&quotedValue, attribute->value());
+    if (QXmlTestLogger::xmlQuote(&quotedValue, attribute->value())) {
-    QTest::qt_asprintf(formatted, " %s=\"%s\"",
+        return QTest::qt_asprintf(formatted, " %s=\"%s\"",
-        attribute->name(), quotedValue.constData());
+                                  attribute->name(), quotedValue.constData()) != 0;
+    }
+    return false;
 }
 
 void QTestJUnitStreamer::formatAfterAttributes(const QTestElement *element, QTestCharBuffer *formatted) const
@@ -176,7 +179,7 @@ void QTestJUnitStreamer::outputElementAttributes(const QTestElement* element, co
     QTestCharBuffer buf;
 
     for (auto *attribute : attributes) {
-        formatAttributes(element, attribute, &buf);
+        if (formatAttributes(element, attribute, &buf))
             outputString(buf.data());
     }
 }

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;

src/testlib/qxmltestlogger.cpp

@@ -116,16 +116,20 @@ 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");
-        QTest::qt_asprintf(&buf,
-                "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
-                "<TestCase name=\"%s\">\n", quotedTc.constData());
         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());
+    if (!QLibraryInfo::build() || xmlQuote(&quotedBuild, QLibraryInfo::build())) {
-
         QTest::qt_asprintf(&buf,
                            "<Environment>\n"
                            "    <QtVersion>%s</QtVersion>\n"
@@ -134,6 +138,7 @@ void QXmlTestLogger::startLogging()
                            "</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);
+    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,10 +248,10 @@ void QXmlTestLogger::addIncident(IncidentTypes type, const char *description,
     QTestCharBuffer cdataTag;
     QTestCharBuffer cdataDescription;
 
-    xmlQuote(&quotedFile, file);
+    if (xmlQuote(&quotedFile, file)
-    xmlCdata(&cdataGtag, gtag);
+        && xmlCdata(&cdataGtag, gtag)
-    xmlCdata(&cdataTag, tag);
+        && xmlCdata(&cdataTag, tag)
-    xmlCdata(&cdataDescription, description);
+        && xmlCdata(&cdataDescription, description)) {
 
         QTest::qt_asprintf(&buf,
                            QTest::incidentFormatString(QTest::isEmpty(description), notag),
@@ -255,19 +264,17 @@ void QXmlTestLogger::addIncident(IncidentTypes type, const char *description,
 
         outputString(buf.constData());
     }
+}
 
 void QXmlTestLogger::addBenchmarkResult(const QBenchmarkResult &result)
 {
-    QTestCharBuffer buf;
     QTestCharBuffer quotedMetric;
     QTestCharBuffer quotedTag;
 
-    xmlQuote(&quotedMetric,
+    if (xmlQuote(&quotedMetric, benchmarkMetricName(result.metric))
-        benchmarkMetricName(result.metric));
+        && xmlQuote(&quotedTag, result.context.tag.toUtf8().constData())) {
-    xmlQuote(&quotedTag, result.context.tag.toUtf8().constData());
+        QTestCharBuffer buf;
-
+        QTest::qt_asprintf(&buf,
-    QTest::qt_asprintf(
-        &buf,
                            QTest::benchmarkResultFormatString(),
                            quotedMetric.constData(),
                            quotedTag.constData(),
@@ -275,6 +282,7 @@ void QXmlTestLogger::addBenchmarkResult(const QBenchmarkResult &result)
                            result.iterations);
         outputString(buf.constData());
     }
+}
 
 void QXmlTestLogger::addMessage(MessageTypes type, const QString &message,
                                 const char *file, int line)
@@ -290,11 +298,10 @@ void QXmlTestLogger::addMessage(MessageTypes type, const QString &message,
     QTestCharBuffer cdataTag;
     QTestCharBuffer cdataDescription;
 
-    xmlQuote(&quotedFile, file);
+    if (xmlQuote(&quotedFile, file)
-    xmlCdata(&cdataGtag, gtag);
+        && xmlCdata(&cdataGtag, gtag)
-    xmlCdata(&cdataTag, tag);
+        && xmlCdata(&cdataTag, tag)
-    xmlCdata(&cdataDescription, message.toUtf8().constData());
+        && xmlCdata(&cdataDescription, message.toUtf8().constData())) {
-
         QTest::qt_asprintf(&buf,
                            QTest::messageFormatString(message.isEmpty(), notag),
                            QTest::xmlMessageType2String(type),
@@ -306,14 +313,17 @@ void QXmlTestLogger::addMessage(MessageTypes type, const QString &message,
 
         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)
+    // If we get here, dest was completely filled:
-    dest[-1] = '\0';
+    Q_ASSERT(dest == end && end > begin);
-    return dest - 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) {
+    if (!src || !*src) {
-        *dest = '\0';
+        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';
+    dest[-1] = '\0'; // hygiene, but it'll be ignored
-    return dest - begin;
+    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
+        if (res < size) { // Success
-            return res;
+            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);
 }

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);
+    [[nodiscard]] static bool xmlCdata(QTestCharBuffer *dest, char const *src);
-    static int xmlQuote(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);
+    [[nodiscard]] static int xmlCdata(QTestCharBuffer *dest, char const *src, qsizetype n);
-    static int xmlQuote(QTestCharBuffer *dest, char const *src, qsizetype n);
+    [[nodiscard]] static int xmlQuote(QTestCharBuffer *dest, char const *src, qsizetype n);
 
     XmlMode xmlmode;
 };