From 87022c9a4ab00a9faaf6bcdaa8d5884962ccf305 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Wed, 13 May 2009 12:25:36 +0200 Subject: Fix handling of garbage data sent by the HTTP server instead of a proper HTTP reply. If the server's reply doesn't start with "HTTP/", then the reply is not valid. This could happen if we connected via HTTP to an HTTPS server. It could also happen with an Icecast server (reply ICY/x.y). Task-number: 248838 Reviewed-by: Markus Goetz --- src/network/access/qhttpnetworkconnection.cpp | 14 +++++++-- src/network/access/qhttpnetworkreply.cpp | 11 ++++++- tests/auto/qnetworkreply/tst_qnetworkreply.cpp | 42 ++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/src/network/access/qhttpnetworkconnection.cpp b/src/network/access/qhttpnetworkconnection.cpp index 5940fba..f558f2b 100644 --- a/src/network/access/qhttpnetworkconnection.cpp +++ b/src/network/access/qhttpnetworkconnection.cpp @@ -529,10 +529,20 @@ void QHttpNetworkConnectionPrivate::receiveReply(QAbstractSocket *socket, QHttpN QHttpNetworkReplyPrivate::ReplyState state = reply ? reply->d_func()->state : QHttpNetworkReplyPrivate::AllDoneState; switch (state) { case QHttpNetworkReplyPrivate::NothingDoneState: - case QHttpNetworkReplyPrivate::ReadingStatusState: - bytes += reply->d_func()->readStatus(socket); + case QHttpNetworkReplyPrivate::ReadingStatusState: { + qint64 statusBytes = reply->d_func()->readStatus(socket); + if (statusBytes == -1) { + // error reading the status, close the socket and emit error + socket->close(); + reply->d_func()->errorString = errorDetail(QNetworkReply::ProtocolFailure, socket); + emit reply->finishedWithError(QNetworkReply::ProtocolFailure, reply->d_func()->errorString); + QMetaObject::invokeMethod(q, "_q_startNextRequest", Qt::QueuedConnection); + break; + } + bytes += statusBytes; channels[i].lastStatus = reply->d_func()->statusCode; break; + } case QHttpNetworkReplyPrivate::ReadingHeaderState: bytes += reply->d_func()->readHeader(socket); if (reply->d_func()->state == QHttpNetworkReplyPrivate::ReadingDataState) { diff --git a/src/network/access/qhttpnetworkreply.cpp b/src/network/access/qhttpnetworkreply.cpp index fe3f6af..1b41e1e 100644 --- a/src/network/access/qhttpnetworkreply.cpp +++ b/src/network/access/qhttpnetworkreply.cpp @@ -409,6 +409,9 @@ qint64 QHttpNetworkReplyPrivate::readStatus(QAbstractSocket *socket) if (fragment.endsWith('\r')) { fragment.truncate(fragment.length()-1); } + if (!fragment.startsWith("HTTP/")) + return -1; + parseStatus(fragment); state = ReadingHeaderState; fragment.clear(); // next fragment @@ -418,7 +421,13 @@ qint64 QHttpNetworkReplyPrivate::readStatus(QAbstractSocket *socket) bytes += socket->read(&c, 1); fragment.append(c); } + + // is this a valid reply? + if (fragment.length() >= 5 && !fragment.startsWith("HTTP/")) + return -1; + } + return bytes; } @@ -660,4 +669,4 @@ void QHttpNetworkReply::ignoreSslErrors() QT_END_NAMESPACE -#endif // QT_NO_HTTP \ No newline at end of file +#endif // QT_NO_HTTP diff --git a/tests/auto/qnetworkreply/tst_qnetworkreply.cpp b/tests/auto/qnetworkreply/tst_qnetworkreply.cpp index 104b788..c97fcf3 100644 --- a/tests/auto/qnetworkreply/tst_qnetworkreply.cpp +++ b/tests/auto/qnetworkreply/tst_qnetworkreply.cpp @@ -172,6 +172,8 @@ private Q_SLOTS: void ioGetFromHttpsWithIgnoreSslErrors(); void ioGetFromHttpsWithSslHandshakeError(); #endif + void ioGetFromHttpBrokenServer_data(); + void ioGetFromHttpBrokenServer(); void ioGetWithManyProxies_data(); void ioGetWithManyProxies(); @@ -1887,6 +1889,46 @@ void tst_QNetworkReply::ioGetFromHttpsWithSslHandshakeError() } #endif +void tst_QNetworkReply::ioGetFromHttpBrokenServer_data() +{ + QTest::addColumn("dataToSend"); + QTest::addColumn("doDisconnect"); + + QTest::newRow("no-newline") << QByteArray("Hello World") << false; + QTest::newRow("just-newline") << QByteArray("\r\n") << false; + QTest::newRow("just-2newline") << QByteArray("\r\n\r\n") << false; + 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("empty+disconnect") << QByteArray() << true; + + QTest::newRow("no-newline+disconnect") << QByteArray("Hello World") << true; + QTest::newRow("just-newline+disconnect") << QByteArray("\r\n") << true; + QTest::newRow("just-2newline+disconnect") << QByteArray("\r\n\r\n") << true; + 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; +} + +void tst_QNetworkReply::ioGetFromHttpBrokenServer() +{ + QFETCH(QByteArray, dataToSend); + QFETCH(bool, doDisconnect); + MiniHttpServer server(dataToSend); + server.doClose = doDisconnect; + + QNetworkRequest request(QUrl("http://localhost:" + QString::number(server.serverPort()))); + QNetworkReplyPtr reply = manager.get(request); + + connect(reply, SIGNAL(finished()), &QTestEventLoop::instance(), SLOT(exitLoop())); + QTestEventLoop::instance().enterLoop(10); + QVERIFY(!QTestEventLoop::instance().timeout()); + + QCOMPARE(reply->url(), request.url()); + QVERIFY(reply->error() != QNetworkReply::NoError); +} + void tst_QNetworkReply::ioGetWithManyProxies_data() { QTest::addColumn >("proxyList"); -- cgit v0.12