From a18ebd2a70e83863bc9a8cc64a65791a6d879f02 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Wed, 13 May 2009 13:27:01 +0200 Subject: Improve the HTTP status line parsing and catch more errors. There's no need for using QByteArrayMatcher for one single character, so avoid the overhead. Also validate the message header a bit more: we require the status line to start with "HTTP/n.m " where n and m are positive integers less than 10. Task-number: 248838 Reviewed-by: Markus Goetz --- src/network/access/qhttpnetworkreply.cpp | 58 ++++++++++++++++---------- src/network/access/qhttpnetworkreply_p.h | 2 +- tests/auto/qnetworkreply/tst_qnetworkreply.cpp | 7 ++++ 3 files changed, 44 insertions(+), 23 deletions(-) diff --git a/src/network/access/qhttpnetworkreply.cpp b/src/network/access/qhttpnetworkreply.cpp index 1b41e1e..69e0a4c 100644 --- a/src/network/access/qhttpnetworkreply.cpp +++ b/src/network/access/qhttpnetworkreply.cpp @@ -409,12 +409,12 @@ qint64 QHttpNetworkReplyPrivate::readStatus(QAbstractSocket *socket) if (fragment.endsWith('\r')) { fragment.truncate(fragment.length()-1); } - if (!fragment.startsWith("HTTP/")) - return -1; - - parseStatus(fragment); + bool ok = parseStatus(fragment); state = ReadingHeaderState; fragment.clear(); // next fragment + + if (!ok) + return -1; break; } else { c = 0; @@ -431,26 +431,40 @@ qint64 QHttpNetworkReplyPrivate::readStatus(QAbstractSocket *socket) return bytes; } -void QHttpNetworkReplyPrivate::parseStatus(const QByteArray &status) +bool QHttpNetworkReplyPrivate::parseStatus(const QByteArray &status) { - const QByteArrayMatcher sp(" "); - int i = sp.indexIn(status); - const QByteArray version = status.mid(0, i); - int j = sp.indexIn(status, i + 1); + // from RFC 2616: + // Status-Line = HTTP-Version SP Status-Code SP Reason-Phrase CRLF + // HTTP-Version = "HTTP" "/" 1*DIGIT "." 1*DIGIT + // that makes: 'HTTP/n.n xxx Message' + // byte count: 0123456789012 + + static const int minLength = 11; + static const int dotPos = 6; + static const int spacePos = 8; + static const char httpMagic[] = "HTTP/"; + + if (status.length() < minLength + || !status.startsWith(httpMagic) + || status.at(dotPos) != '.' + || status.at(spacePos) != ' ') { + // I don't know how to parse this status line + return false; + } + + // optimize for the valid case: defer checking until the end + majorVersion = status.at(dotPos - 1) - '0'; + minorVersion = status.at(dotPos + 1) - '0'; + + int i = spacePos; + int j = status.indexOf(' ', i + 1); // j == -1 || at(j) == ' ' so j+1 == 0 && j+1 <= length() const QByteArray code = status.mid(i + 1, j - i - 1); - const QByteArray reason = status.mid(j + 1, status.count() - j); - - const QByteArrayMatcher slash("/"); - int k = slash.indexIn(version); - const QByteArrayMatcher dot("."); - int l = dot.indexIn(version, k); - const QByteArray major = version.mid(k + 1, l - k - 1); - const QByteArray minor = version.mid(l + 1, version.count() - l); - - majorVersion = QString::fromAscii(major.constData()).toInt(); - minorVersion = QString::fromAscii(minor.constData()).toInt(); - statusCode = QString::fromAscii(code.constData()).toInt(); - reasonPhrase = QString::fromAscii(reason.constData()); + + bool ok; + statusCode = code.toInt(&ok); + reasonPhrase = QString::fromLatin1(status.constData() + j + 1); + + return ok && uint(majorVersion) <= 9 && uint(minorVersion) <= 9; } qint64 QHttpNetworkReplyPrivate::readHeader(QAbstractSocket *socket) diff --git a/src/network/access/qhttpnetworkreply_p.h b/src/network/access/qhttpnetworkreply_p.h index c17c65c..cb4d34f 100644 --- a/src/network/access/qhttpnetworkreply_p.h +++ b/src/network/access/qhttpnetworkreply_p.h @@ -154,7 +154,7 @@ public: QHttpNetworkReplyPrivate(const QUrl &newUrl = QUrl()); ~QHttpNetworkReplyPrivate(); qint64 readStatus(QAbstractSocket *socket); - void parseStatus(const QByteArray &status); + bool parseStatus(const QByteArray &status); qint64 readHeader(QAbstractSocket *socket); void parseHeader(const QByteArray &header); qint64 readBody(QAbstractSocket *socket, QIODevice *out); diff --git a/tests/auto/qnetworkreply/tst_qnetworkreply.cpp b/tests/auto/qnetworkreply/tst_qnetworkreply.cpp index c97fcf3..4be0863 100644 --- a/tests/auto/qnetworkreply/tst_qnetworkreply.cpp +++ b/tests/auto/qnetworkreply/tst_qnetworkreply.cpp @@ -1900,6 +1900,9 @@ void tst_QNetworkReply::ioGetFromHttpBrokenServer_data() QTest::newRow("with-newlines") << QByteArray("Long first line\r\nLong second line") << false; QTest::newRow("with-newlines2") << QByteArray("\r\nSecond line") << false; QTest::newRow("with-newlines3") << QByteArray("ICY\r\nSecond line") << false; + QTest::newRow("invalid-version") << QByteArray("HTTP/123 200 \r\n") << false; + QTest::newRow("invalid-version2") << QByteArray("HTTP/a.\033 200 \r\n") << false; + QTest::newRow("invalid-reply-code") << QByteArray("HTTP/1.0 fuu \r\n") << false; QTest::newRow("empty+disconnect") << QByteArray() << true; @@ -1909,6 +1912,10 @@ void tst_QNetworkReply::ioGetFromHttpBrokenServer_data() QTest::newRow("with-newlines+disconnect") << QByteArray("Long first line\r\nLong second line") << true; QTest::newRow("with-newlines2+disconnect") << QByteArray("\r\nSecond line") << true; QTest::newRow("with-newlines3+disconnect") << QByteArray("ICY\r\nSecond line") << true; + + QTest::newRow("invalid-version+disconnect") << QByteArray("HTTP/123 200 ") << true; + QTest::newRow("invalid-version2+disconnect") << QByteArray("HTTP/a.\033 200 ") << true; + QTest::newRow("invalid-reply-code+disconnect") << QByteArray("HTTP/1.0 fuu ") << true; } void tst_QNetworkReply::ioGetFromHttpBrokenServer() -- cgit v0.12