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

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()