parent
5741f0f03e
commit
5383317c95
@ -1,284 +0,0 @@
|
|||||||
From 6326bec46a618c72feba4a2bb994c4d475050aed Mon Sep 17 00:00:00 2001
|
|
||||||
From: Ahmad Samir <a.samirh78@gmail.com>
|
|
||||||
Date: Thu, 22 Jun 2023 15:56:07 +0300
|
|
||||||
Subject: [PATCH] QXmlStreamReader: make fastScanName() indicate parsing status to callers
|
|
||||||
|
|
||||||
This fixes a crash while parsing an XML file with garbage data, the file
|
|
||||||
starts with '<' then garbage data:
|
|
||||||
- The loop in the parse() keeps iterating until it hits "case 262:",
|
|
||||||
which calls fastScanName()
|
|
||||||
- fastScanName() iterates over the text buffer scanning for the
|
|
||||||
attribute name (e.g. "xml:lang"), until it finds ':'
|
|
||||||
- Consider a Value val, fastScanName() is called on it, it would set
|
|
||||||
val.prefix to a number > val.len, then it would hit the 4096 condition
|
|
||||||
and return (returned 0, now it returns the equivalent of
|
|
||||||
std::null_opt), which means that val.len doesn't get modified, making
|
|
||||||
it smaller than val.prefix
|
|
||||||
- The code would try constructing an XmlStringRef with negative length,
|
|
||||||
which would hit an assert in one of QStringView's constructors
|
|
||||||
|
|
||||||
Add an assert to the XmlStringRef constructor.
|
|
||||||
|
|
||||||
Add unittest based on the file from the bug report.
|
|
||||||
|
|
||||||
Later on I will replace FastScanNameResult with std::optional<qsizetype>
|
|
||||||
(std::optional is C++17, which isn't required by Qt 5.15, and we want to
|
|
||||||
backport this fix).
|
|
||||||
|
|
||||||
Credit to OSS-Fuzz.
|
|
||||||
|
|
||||||
Fixes: QTBUG-109781
|
|
||||||
Fixes: QTBUG-114829
|
|
||||||
Pick-to: 6.6 6.5 6.2 5.15
|
|
||||||
Change-Id: I455a5eeb47870c2ac9ffd0cbcdcd99c1ae2dd374
|
|
||||||
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
|
|
||||||
---
|
|
||||||
|
|
||||||
diff --git a/src/corelib/serialization/qxmlstream.cpp b/src/corelib/serialization/qxmlstream.cpp
|
|
||||||
index b76243c176..adb57a0239 100644
|
|
||||||
--- a/src/corelib/serialization/qxmlstream.cpp
|
|
||||||
+++ b/src/corelib/serialization/qxmlstream.cpp
|
|
||||||
@@ -1243,7 +1243,10 @@ inline qsizetype QXmlStreamReaderPrivate::fastScanContentCharList()
|
|
||||||
return n;
|
|
||||||
}
|
|
||||||
|
|
||||||
-inline qsizetype QXmlStreamReaderPrivate::fastScanName(qint16 *prefix)
|
|
||||||
+// Fast scan an XML attribute name (e.g. "xml:lang").
|
|
||||||
+inline QXmlStreamReaderPrivate::FastScanNameResult
|
|
||||||
+QXmlStreamReaderPrivate::fastScanName(Value *val)
|
|
||||||
+
|
|
||||||
{
|
|
||||||
qsizetype n = 0;
|
|
||||||
uint c;
|
|
||||||
@@ -1251,7 +1254,8 @@ inline qsizetype QXmlStreamReaderPrivate::fastScanName(qint16 *prefix)
|
|
||||||
if (n >= 4096) {
|
|
||||||
// This is too long to be a sensible name, and
|
|
||||||
// can exhaust memory, or the range of decltype(*prefix)
|
|
||||||
- return 0;
|
|
||||||
+ raiseNamePrefixTooLongError();
|
|
||||||
+ return {};
|
|
||||||
}
|
|
||||||
switch (c) {
|
|
||||||
case '\n':
|
|
||||||
@@ -1280,23 +1284,24 @@ inline qsizetype QXmlStreamReaderPrivate::fastScanName(qint16 *prefix)
|
|
||||||
case '+':
|
|
||||||
case '*':
|
|
||||||
putChar(c);
|
|
||||||
- if (prefix && *prefix == n+1) {
|
|
||||||
- *prefix = 0;
|
|
||||||
+ if (val && val->prefix == n + 1) {
|
|
||||||
+ val->prefix = 0;
|
|
||||||
+
|
|
||||||
putChar(':');
|
|
||||||
--n;
|
|
||||||
}
|
|
||||||
- return n;
|
|
||||||
+ return FastScanNameResult(n);
|
|
||||||
case ':':
|
|
||||||
- if (prefix) {
|
|
||||||
- if (*prefix == 0) {
|
|
||||||
- *prefix = qint16(n + 2);
|
|
||||||
+ if (val) {
|
|
||||||
+ if (val->prefix == 0) {
|
|
||||||
+ val->prefix = n + 2;
|
|
||||||
} else { // only one colon allowed according to the namespace spec.
|
|
||||||
putChar(c);
|
|
||||||
- return n;
|
|
||||||
+ return FastScanNameResult(n);
|
|
||||||
}
|
|
||||||
} else {
|
|
||||||
putChar(c);
|
|
||||||
- return n;
|
|
||||||
+ return FastScanNameResult(n);
|
|
||||||
}
|
|
||||||
Q_FALLTHROUGH();
|
|
||||||
default:
|
|
||||||
@@ -1305,12 +1310,13 @@ inline qsizetype QXmlStreamReaderPrivate::fastScanName(qint16 *prefix)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
- if (prefix)
|
|
||||||
- *prefix = 0;
|
|
||||||
+ if (val)
|
|
||||||
+ val->prefix = 0;
|
|
||||||
+
|
|
||||||
qsizetype pos = textBuffer.size() - n;
|
|
||||||
putString(textBuffer, pos);
|
|
||||||
textBuffer.resize(pos);
|
|
||||||
- return 0;
|
|
||||||
+ return FastScanNameResult(0);
|
|
||||||
}
|
|
||||||
|
|
||||||
enum NameChar { NameBeginning, NameNotBeginning, NotName };
|
|
||||||
@@ -1791,6 +1797,14 @@ void QXmlStreamReaderPrivate::raiseWellFormedError(const QString &message)
|
|
||||||
raiseError(QXmlStreamReader::NotWellFormedError, message);
|
|
||||||
}
|
|
||||||
|
|
||||||
+void QXmlStreamReaderPrivate::raiseNamePrefixTooLongError()
|
|
||||||
+{
|
|
||||||
+ // TODO: add a ImplementationLimitsExceededError and use it instead
|
|
||||||
+ raiseError(QXmlStreamReader::NotWellFormedError,
|
|
||||||
+ QXmlStream::tr("Length of XML attribute name exceeds implemnetation limits (4KiB "
|
|
||||||
+ "characters)."));
|
|
||||||
+}
|
|
||||||
+
|
|
||||||
void QXmlStreamReaderPrivate::parseError()
|
|
||||||
{
|
|
||||||
|
|
||||||
diff --git a/src/corelib/serialization/qxmlstream.g b/src/corelib/serialization/qxmlstream.g
|
|
||||||
index d06c371eb8..fc122e6681 100644
|
|
||||||
--- a/src/corelib/serialization/qxmlstream.g
|
|
||||||
+++ b/src/corelib/serialization/qxmlstream.g
|
|
||||||
@@ -1419,7 +1419,12 @@ space_opt ::= space;
|
|
||||||
qname ::= LETTER;
|
|
||||||
/.
|
|
||||||
case $rule_number: {
|
|
||||||
- sym(1).len += fastScanName(&sym(1).prefix);
|
|
||||||
+ Value &val = sym(1);
|
|
||||||
+ if (auto res = fastScanName(&val))
|
|
||||||
+ val.len += *res;
|
|
||||||
+ else
|
|
||||||
+ return false;
|
|
||||||
+
|
|
||||||
if (atEnd) {
|
|
||||||
resume($rule_number);
|
|
||||||
return false;
|
|
||||||
@@ -1430,7 +1435,11 @@ qname ::= LETTER;
|
|
||||||
name ::= LETTER;
|
|
||||||
/.
|
|
||||||
case $rule_number:
|
|
||||||
- sym(1).len += fastScanName();
|
|
||||||
+ if (auto res = fastScanName())
|
|
||||||
+ sym(1).len += *res;
|
|
||||||
+ else
|
|
||||||
+ return false;
|
|
||||||
+
|
|
||||||
if (atEnd) {
|
|
||||||
resume($rule_number);
|
|
||||||
return false;
|
|
||||||
diff --git a/src/corelib/serialization/qxmlstream_p.h b/src/corelib/serialization/qxmlstream_p.h
|
|
||||||
index 1fd69a2c1f..fba6aeb496 100644
|
|
||||||
--- a/src/corelib/serialization/qxmlstream_p.h
|
|
||||||
+++ b/src/corelib/serialization/qxmlstream_p.h
|
|
||||||
@@ -38,7 +38,7 @@ public:
|
|
||||||
|
|
||||||
constexpr XmlStringRef() = default;
|
|
||||||
constexpr inline XmlStringRef(const QString *string, qsizetype pos, qsizetype length)
|
|
||||||
- : m_string(string), m_pos(pos), m_size(length)
|
|
||||||
+ : m_string(string), m_pos(pos), m_size((Q_ASSERT(length >= 0), length))
|
|
||||||
{
|
|
||||||
}
|
|
||||||
XmlStringRef(const QString *string)
|
|
||||||
@@ -471,7 +471,16 @@ public:
|
|
||||||
qsizetype fastScanLiteralContent();
|
|
||||||
qsizetype fastScanSpace();
|
|
||||||
qsizetype fastScanContentCharList();
|
|
||||||
- qsizetype fastScanName(qint16 *prefix = nullptr);
|
|
||||||
+
|
|
||||||
+ struct FastScanNameResult {
|
|
||||||
+ FastScanNameResult() : ok(false) {}
|
|
||||||
+ explicit FastScanNameResult(qsizetype len) : addToLen(len), ok(true) { }
|
|
||||||
+ operator bool() { return ok; }
|
|
||||||
+ qsizetype operator*() { Q_ASSERT(ok); return addToLen; }
|
|
||||||
+ qsizetype addToLen;
|
|
||||||
+ bool ok;
|
|
||||||
+ };
|
|
||||||
+ FastScanNameResult fastScanName(Value *val = nullptr);
|
|
||||||
inline qsizetype fastScanNMTOKEN();
|
|
||||||
|
|
||||||
|
|
||||||
@@ -480,6 +489,7 @@ public:
|
|
||||||
|
|
||||||
void raiseError(QXmlStreamReader::Error error, const QString& message = QString());
|
|
||||||
void raiseWellFormedError(const QString &message);
|
|
||||||
+ void raiseNamePrefixTooLongError();
|
|
||||||
|
|
||||||
QXmlStreamEntityResolver *entityResolver;
|
|
||||||
|
|
||||||
diff --git a/src/corelib/serialization/qxmlstreamparser_p.h b/src/corelib/serialization/qxmlstreamparser_p.h
|
|
||||||
index e3ae6faa44..afd83381b3 100644
|
|
||||||
--- a/src/corelib/serialization/qxmlstreamparser_p.h
|
|
||||||
+++ b/src/corelib/serialization/qxmlstreamparser_p.h
|
|
||||||
@@ -947,7 +947,12 @@ bool QXmlStreamReaderPrivate::parse()
|
|
||||||
break;
|
|
||||||
|
|
||||||
case 262: {
|
|
||||||
- sym(1).len += fastScanName(&sym(1).prefix);
|
|
||||||
+ Value &val = sym(1);
|
|
||||||
+ if (auto res = fastScanName(&val))
|
|
||||||
+ val.len += *res;
|
|
||||||
+ else
|
|
||||||
+ return false;
|
|
||||||
+
|
|
||||||
if (atEnd) {
|
|
||||||
resume(262);
|
|
||||||
return false;
|
|
||||||
@@ -955,7 +960,11 @@ bool QXmlStreamReaderPrivate::parse()
|
|
||||||
} break;
|
|
||||||
|
|
||||||
case 263:
|
|
||||||
- sym(1).len += fastScanName();
|
|
||||||
+ if (auto res = fastScanName())
|
|
||||||
+ sym(1).len += *res;
|
|
||||||
+ else
|
|
||||||
+ return false;
|
|
||||||
+
|
|
||||||
if (atEnd) {
|
|
||||||
resume(263);
|
|
||||||
return false;
|
|
||||||
diff --git a/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp b/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp
|
|
||||||
index 2799e7a999..7eb0aac5cc 100644
|
|
||||||
--- a/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp
|
|
||||||
+++ b/tests/auto/corelib/serialization/qxmlstream/tst_qxmlstream.cpp
|
|
||||||
@@ -581,6 +581,8 @@ private slots:
|
|
||||||
void readBack() const;
|
|
||||||
void roundTrip() const;
|
|
||||||
void roundTrip_data() const;
|
|
||||||
+ void test_fastScanName_data() const;
|
|
||||||
+ void test_fastScanName() const;
|
|
||||||
|
|
||||||
void entityExpansionLimit() const;
|
|
||||||
|
|
||||||
@@ -1753,5 +1755,42 @@ void tst_QXmlStream::roundTrip() const
|
|
||||||
QCOMPARE(out, in);
|
|
||||||
}
|
|
||||||
|
|
||||||
+void tst_QXmlStream::test_fastScanName_data() const
|
|
||||||
+{
|
|
||||||
+ QTest::addColumn<QByteArray>("data");
|
|
||||||
+ QTest::addColumn<QXmlStreamReader::Error>("errorType");
|
|
||||||
+
|
|
||||||
+ // 4096 is the limit in QXmlStreamReaderPrivate::fastScanName()
|
|
||||||
+
|
|
||||||
+ QByteArray arr = "<a"_ba + ":" + QByteArray("b").repeated(4096 - 1);
|
|
||||||
+ QTest::newRow("data1") << arr << QXmlStreamReader::PrematureEndOfDocumentError;
|
|
||||||
+
|
|
||||||
+ arr = "<a"_ba + ":" + QByteArray("b").repeated(4096);
|
|
||||||
+ QTest::newRow("data2") << arr << QXmlStreamReader::NotWellFormedError;
|
|
||||||
+
|
|
||||||
+ arr = "<"_ba + QByteArray("a").repeated(4000) + ":" + QByteArray("b").repeated(96);
|
|
||||||
+ QTest::newRow("data3") << arr << QXmlStreamReader::PrematureEndOfDocumentError;
|
|
||||||
+
|
|
||||||
+ arr = "<"_ba + QByteArray("a").repeated(4000) + ":" + QByteArray("b").repeated(96 + 1);
|
|
||||||
+ QTest::newRow("data4") << arr << QXmlStreamReader::NotWellFormedError;
|
|
||||||
+
|
|
||||||
+ arr = "<"_ba + QByteArray("a").repeated(4000 + 1) + ":" + QByteArray("b").repeated(96);
|
|
||||||
+ QTest::newRow("data5") << arr << QXmlStreamReader::NotWellFormedError;
|
|
||||||
+}
|
|
||||||
+
|
|
||||||
+void tst_QXmlStream::test_fastScanName() const
|
|
||||||
+{
|
|
||||||
+ QFETCH(QByteArray, data);
|
|
||||||
+ QFETCH(QXmlStreamReader::Error, errorType);
|
|
||||||
+
|
|
||||||
+ QXmlStreamReader reader(data);
|
|
||||||
+ QXmlStreamReader::TokenType tokenType;
|
|
||||||
+ while (!reader.atEnd())
|
|
||||||
+ tokenType = reader.readNext();
|
|
||||||
+
|
|
||||||
+ QCOMPARE(tokenType, QXmlStreamReader::Invalid);
|
|
||||||
+ QCOMPARE(reader.error(), errorType);
|
|
||||||
+}
|
|
||||||
+
|
|
||||||
#include "tst_qxmlstream.moc"
|
|
||||||
// vim: et:ts=4:sw=4:sts=4
|
|
Loading…
Reference in new issue