From b9f7add531ceab2590e2b24dec7b495950f9870a Mon Sep 17 00:00:00 2001 From: Edward Welbourne Date: Thu, 30 Sep 2021 12:50:05 +0200 Subject: [PATCH] Verify returns from QXmlTestLogger's xmlQuote() and xmlCdata() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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ø --- src/testlib/qtestjunitstreamer.cpp | 23 ++-- src/testlib/qtestjunitstreamer_p.h | 6 +- src/testlib/qxmltestlogger.cpp | 171 ++++++++++++++++------------- src/testlib/qxmltestlogger_p.h | 8 +- 4 files changed, 113 insertions(+), 95 deletions(-) diff --git a/src/testlib/qtestjunitstreamer.cpp b/src/testlib/qtestjunitstreamer.cpp index 72ab7d15341..3e67e5c4bfb 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\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("edValue, attribute->value()); - QTest::qt_asprintf(formatted, " %s=\"%s\"", - attribute->name(), quotedValue.constData()); + if (QXmlTestLogger::xmlQuote("edValue, 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 17c8a0383c8..83b070d856b 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 &) const; void outputElementAttributes(const QTestElement *element, const std::vector &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 89c4cbb765b..0b63b8b7530 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("edTc, QTestResult::currentTestObjectName()); - QTest::qt_asprintf(&buf, - "\n" - "\n", quotedTc.constData()); + QTest::qt_asprintf(&buf, "\n"); outputString(buf.constData()); + if (xmlQuote("edTc, QTestResult::currentTestObjectName())) { + QTest::qt_asprintf(&buf, "\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("edBuild, QLibraryInfo::build()); - - QTest::qt_asprintf(&buf, - "\n" - " %s\n" - " %s\n" - " " QTEST_VERSION_STR "\n" - "\n", qVersion(), quotedBuild.constData()); - outputString(buf.constData()); + if (!QLibraryInfo::build() || xmlQuote("edBuild, QLibraryInfo::build())) { + QTest::qt_asprintf(&buf, + "\n" + " %s\n" + " %s\n" + " " QTEST_VERSION_STR "\n" + "\n", qVersion(), quotedBuild.constData()); + outputString(buf.constData()); + } } void QXmlTestLogger::stopLogging() @@ -142,20 +147,24 @@ void QXmlTestLogger::stopLogging() QTest::qt_asprintf(&buf, "\n", QString::number(QTestLog::msecsTotalTime()).toUtf8().constData()); outputString(buf.constData()); - if (xmlmode == QXmlTestLogger::Complete) { + if (xmlmode == QXmlTestLogger::Complete) outputString("\n"); - } QAbstractTestLogger::stopLogging(); } void QXmlTestLogger::enterTestFunction(const char *function) { - QTestCharBuffer buf; QTestCharBuffer quotedFunction; - xmlQuote("edFunction, function); - QTest::qt_asprintf(&buf, "\n", quotedFunction.constData()); - outputString(buf.constData()); + if (xmlQuote("edFunction, function)) { + QTestCharBuffer buf; + QTest::qt_asprintf(&buf, "\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("edFile, file); - xmlCdata(&cdataGtag, gtag); - xmlCdata(&cdataTag, tag); - xmlCdata(&cdataDescription, description); + if (xmlQuote("edFile, 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("edMetric, - benchmarkMetricName(result.metric)); - xmlQuote("edTag, 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("edMetric, benchmarkMetricName(result.metric)) + && xmlQuote("edTag, 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("edFile, file); - xmlCdata(&cdataGtag, gtag); - xmlCdata(&cdataTag, tag); - xmlCdata(&cdataDescription, message.toUtf8().constData()); + if (xmlQuote("edFile, 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()); - 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 8091dada596..e72b6885459 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; };