Fix writing from sequential QIODevices to HTTP(S)/1.(0/1) clientsHEADdev

When handling HTTP(S)/1.1 clients use chunked transfer encoding
for sequential QIODevices, because sequential QIODevices don't
know their length. The failure to do so caused clients to hang.
When handling HTTP(S)/1.0 clients disconnect after write because
such clients don't support chunked transfer encoding.

Removed expect_fail from tests that now work.

Fixes: QTBUG-137330
Pick-to: 6.10 6.9 6.8
Change-Id: Ie7a0e106c5475b8298697cdee0ba080acab3ce97
Reviewed-by: Lena Biliaieva <[email protected]>
Reviewed-by: Matthias Rauter <[email protected]>

7 files changed, 76 insertions, 42 deletions

diff --git a/src/httpserver/qhttpserverhttp1protocolhandler.cpp b/src/httpserver/qhttpserverhttp1protocolhandler.cpp
index b858a60..35c2d28 100644
--- a/src/httpserver/qhttpserverhttp1protocolhandler.cpp
+++ b/src/httpserver/qhttpserverhttp1protocolhandler.cpp
@@ -111,16 +111,23 @@ struct IOChunkedTransfer
     const QPointer<QIODevice> sink;
     const QMetaObject::Connection bytesWrittenConnection;
     const QMetaObject::Connection readyReadConnection;
+    const QMetaObject::Connection readChannelFinished;
     bool inRead = false;
+    bool gotReadChannelFinished = false;
+    bool writingIsComplete = false;
+    bool useHttp1_1 = false;
 
-    IOChunkedTransfer(QIODevice *input, QIODevice *output) :
+    IOChunkedTransfer(QIODevice *input, QIODevice *output, bool http1_1) :
           source(input),
           sink(output),
           bytesWrittenConnection(connectToBytesWritten(this, output)),
           readyReadConnection(QObject::connect(source.data(), &QIODevice::readyRead, source.data(),
-                                               [this]() { readFromInput(); }))
+                                               [this]() { readFromInput(); })),
+          readChannelFinished(QObject::connect(source.data(), &QIODevice::readChannelFinished,
+                                               source.data(),
+                                               [this]() { readChannelFinishedHandler(); })),
+          useHttp1_1(http1_1)
     {
-        Q_ASSERT(!source->atEnd());  // TODO error out
         QObject::connect(sink.data(), &QObject::destroyed, source.data(), &QObject::deleteLater);
         QObject::connect(source.data(), &QObject::destroyed, source.data(), [this]() {
             delete this;
@@ -132,6 +139,7 @@ struct IOChunkedTransfer
     {
         QObject::disconnect(bytesWrittenConnection);
         QObject::disconnect(readyReadConnection);
+        QObject::disconnect(readChannelFinished);
     }
 
     static QMetaObject::Connection connectToBytesWritten(IOChunkedTransfer *that, QIODevice *device)
@@ -152,6 +160,12 @@ struct IOChunkedTransfer
         return beginIndex == endIndex;
     }
 
+    void readChannelFinishedHandler()
+    {
+        gotReadChannelFinished = true;
+        readFromInput();
+    }
+
     void readFromInput()
     {
         if (inRead)
@@ -167,13 +181,19 @@ struct IOChunkedTransfer
             beginIndex = 0;
             endIndex = source->read(buffer, bufferSize);
             if (endIndex < 0) {
-                endIndex = beginIndex; // Mark the buffer as empty
-                qCWarning(lcHttpServerHttp1Handler, "Error reading chunk: %ls",
-                        qUtf16Printable(source->errorString()));
-                break;
+                endIndex = 0; // Mark the buffer as empty
+                if (!source->isSequential()) {
+                    qCWarning(lcHttpServerHttp1Handler, "Error reading chunk: %ls",
+                              qUtf16Printable(source->errorString()));
+                    return;
+                }
+            }
+            if (endIndex == 0) { // Nothing was read
+                if (!writingIsComplete
+                    && ((!source->isSequential() && source->atEnd()) || gotReadChannelFinished))
+                    completeWriting();
+                return;
             }
-            if (endIndex == 0)
-                break;
             memset(buffer + endIndex, 0, sizeof(buffer) - std::size_t(endIndex));
             writeToOutput();
         }
@@ -183,7 +203,6 @@ struct IOChunkedTransfer
     {
         if (sink.isNull() || source.isNull())
             return;
-
         if (isBufferEmpty())
             return;
 
@@ -200,19 +219,38 @@ struct IOChunkedTransfer
         }
 #endif
 
-        const auto writtenBytes = sink->write(buffer + beginIndex, endIndex);
+        if (useHttp1_1 && source->isSequential()) {
+            sink->write(QByteArray::number(endIndex - beginIndex, 16));
+            sink->write("\r\n");
+        }
+        const auto writtenBytes = sink->write(buffer + beginIndex, endIndex - beginIndex);
         if (writtenBytes < 0) {
             qCWarning(lcHttpServerHttp1Handler, "Error writing chunk: %ls",
                       qUtf16Printable(sink->errorString()));
             return;
         }
+        if (useHttp1_1 && source->isSequential())
+            sink->write("\r\n");
         beginIndex += writtenBytes;
-        if (isBufferEmpty()) {
-            if (source->bytesAvailable() && !inRead)
-                readFromInput();
-            else if (source->atEnd())  // Finishing
-                source->deleteLater();
+        if (isBufferEmpty() && !inRead)
+            readFromInput();
+    }
+
+    void completeWriting()
+    {
+        if (sink.isNull())
+            return;
+        Q_ASSERT(!source.isNull());
+        Q_ASSERT(isBufferEmpty());
+
+        if (source->isSequential()) {
+            if (useHttp1_1)
+                sink->write("0\r\n\r\n");
+            else
+                sink->close();
         }
+        source->deleteLater();
+        writingIsComplete = true;
     }
 };
 
@@ -325,6 +363,7 @@ void QHttpServerHttp1ProtocolHandler::handleReadyRead()
         return; // Partial read
 
     qCDebug(lcHttpServerHttp1Handler) << "Request:" << request;
+    useHttp1_1 = request.d->minorVersion == 1;
 
     QHttpServerResponder responder(this);
 
@@ -442,20 +481,19 @@ void QHttpServerHttp1ProtocolHandler::write(QIODevice *data, const QHttpHeaders
     }
 
     QHttpHeaders allHeaders(headers);
-    if (!input->isSequential()) { // Non-sequential QIODevice should know its data size
+    if (input->isSequential()) {
+        if (useHttp1_1)
+            allHeaders.append(QHttpHeaders::WellKnownHeader::TransferEncoding, "chunked");
+        else
+            allHeaders.append(QHttpHeaders::WellKnownHeader::Connection, "close");
+    } else { // Non-sequential QIODevice should know its data size
         allHeaders.append(QHttpHeaders::WellKnownHeader::ContentLength,
                           QByteArray::number(input->size()));
     }
-
     writeStatusAndHeaders(status, allHeaders);
 
-    if (input->atEnd()) {
-        qCDebug(lcHttpServerHttp1Handler, "No more data available.");
-        return;
-    }
-
     // input takes ownership of the IOChunkedTransfer pointer inside his constructor
-    new IOChunkedTransfer<>(input.release(), socket);
+    new IOChunkedTransfer<>(input.release(), socket, useHttp1_1);
     state = TransferState::Ready;
 }
 
diff --git a/src/httpserver/qhttpserverhttp1protocolhandler_p.h b/src/httpserver/qhttpserverhttp1protocolhandler_p.h
index a9b5ec1..bc3fd55 100644
--- a/src/httpserver/qhttpserverhttp1protocolhandler_p.h
+++ b/src/httpserver/qhttpserverhttp1protocolhandler_p.h
@@ -89,6 +89,7 @@ private:
     bool handlingRequest = false;
     bool protocolChanged = false;
     QElapsedTimer lastActiveTimer;
+    bool useHttp1_1 = false;
 };
 
 QT_END_NAMESPACE
diff --git a/src/httpserver/qhttpserverparser.cpp b/src/httpserver/qhttpserverparser.cpp
index 3be9385..6042814 100644
--- a/src/httpserver/qhttpserverparser.cpp
+++ b/src/httpserver/qhttpserverparser.cpp
@@ -76,6 +76,8 @@ bool QHttpServerParser::parseRequestLine(QByteArrayView line)
 
     headerParser.setMajorVersion(protocol[5] - '0');
     headerParser.setMinorVersion(protocol[7] - '0');
+    majorVersion = protocol[5] - '0';
+    minorVersion = protocol[7] - '0';
 
     method = parseRequestMethod(requestMethod);
     url = QUrl::fromEncoded(requestUrl.toByteArray());
@@ -306,6 +308,9 @@ bool QHttpServerParser::parse(QHttp2Stream *socket)
 {
     clear();
 
+    majorVersion = 2;
+    minorVersion = 0;
+
     for (const auto &pair : socket->receivedHeaders()) {
         if (pair.name == ":method") {
             method = parseRequestMethod(pair.value);
@@ -353,6 +358,8 @@ void QHttpServerParser::clear()
     currentChunkRead = 0;
     currentChunkSize = 0;
     upgrade = false;
+    majorVersion = 0;
+    minorVersion = 0;
 
     fragment.clear();
     bodyBuffer.clear();
diff --git a/src/httpserver/qhttpserverparser_p.h b/src/httpserver/qhttpserverparser_p.h
index ec1c0e4..cdc7580 100644
--- a/src/httpserver/qhttpserverparser_p.h
+++ b/src/httpserver/qhttpserverparser_p.h
@@ -80,6 +80,8 @@ public:
     qsizetype currentChunkRead;
     qsizetype currentChunkSize;
     bool upgrade;
+    int majorVersion;
+    int minorVersion;
 
     QByteArray fragment;
     QByteDataBuffer bodyBuffer;
diff --git a/src/httpserver/qhttpserverrequest.cpp b/src/httpserver/qhttpserverrequest.cpp
index e50dc1b..cecdbc4 100644
--- a/src/httpserver/qhttpserverrequest.cpp
+++ b/src/httpserver/qhttpserverrequest.cpp
@@ -306,6 +306,8 @@ QHttpServerRequest QHttpServerRequest::create(const QHttpServerParser &parser)
     request.d->method = parser.method;
     request.d->headers = parser.headers;
     request.d->body = parser.body;
+    request.d->majorVersion = parser.majorVersion;
+    request.d->minorVersion = parser.minorVersion;
     return request;
 }
 
diff --git a/src/httpserver/qhttpserverrequest_p.h b/src/httpserver/qhttpserverrequest_p.h
index 2c25559..044d66b 100644
--- a/src/httpserver/qhttpserverrequest_p.h
+++ b/src/httpserver/qhttpserverrequest_p.h
@@ -48,6 +48,8 @@ public:
     quint16 remotePort;
     QHostAddress localAddress;
     quint16 localPort;
+    int majorVersion;
+    int minorVersion;
 #if QT_CONFIG(ssl)
     QSslConfiguration sslConfiguration;
 #endif
diff --git a/tests/auto/qhttpserver/tst_qhttpserver.cpp b/tests/auto/qhttpserver/tst_qhttpserver.cpp
index dc090c3..62701f1 100644
--- a/tests/auto/qhttpserver/tst_qhttpserver.cpp
+++ b/tests/auto/qhttpserver/tst_qhttpserver.cpp
@@ -1653,12 +1653,6 @@ void tst_QHttpServer::writeSequentialDevice()
     QSignalSpy spy(reply.get(), &QNetworkReply::finished);
     spy.wait(2s);
 
-    if (!useHttp2) {
-        QEXPECT_FAIL(
-                "",
-                "QTBUG-137330: Writing from a Sequential QIODevice to HTTP/1.1 Hangs the Client",
-                Abort);
-    }
     QCOMPARE(spy.count(), 1);
     checkReply(reply.release(), "GoGoGo");
 }
@@ -1679,12 +1673,6 @@ void tst_QHttpServer::writeMuchToSequentialDevice()
     QSignalSpy spy(reply.get(), &QNetworkReply::finished);
     spy.wait(2s);
 
-    if (!useHttp2) {
-        QEXPECT_FAIL(
-                "",
-                "QTBUG-137330: Writing from a Sequential QIODevice to HTTP/1.1 Hangs the Client",
-                Abort);
-    }
     QCOMPARE(spy.count(), 1);
     checkReply(reply.release(), u"a"_s.repeated(bufferSize * 2));
 }
@@ -1704,12 +1692,6 @@ void tst_QHttpServer::writeFromEmptySequentialDevice()
     QSignalSpy spy(reply.get(), &QNetworkReply::finished);
     spy.wait(2s);
 
-    if (!useHttp2) {
-        QEXPECT_FAIL(
-                "",
-                "QTBUG-137330: Writing from a Sequential QIODevice to HTTP/1.1 Hangs the Client",
-                Abort);
-    }
     QCOMPARE(spy.count(), 1);
     checkReply(reply.release(), "");
 }