diff --git a/src/network/access/qabstractprotocolhandler_p.h b/src/network/access/qabstractprotocolhandler_p.h index da5eaeeb74c..42925a169d3 100644 --- a/src/network/access/qabstractprotocolhandler_p.h +++ b/src/network/access/qabstractprotocolhandler_p.h @@ -17,6 +17,8 @@ #include +#include + QT_REQUIRE_CONFIG(http); QT_BEGIN_NAMESPACE @@ -34,6 +36,13 @@ public: virtual void _q_receiveReply() = 0; virtual void _q_readyRead() = 0; virtual bool sendRequest() = 0; + // Called when the reply is being destroyed and removing itself from any other internals + virtual bool tryRemoveReply(QHttpNetworkReply *reply) + { + Q_UNUSED(reply); + // base implementation is a noop + return false; + } void setReply(QHttpNetworkReply *reply); protected: diff --git a/src/network/access/qhttp2protocolhandler.cpp b/src/network/access/qhttp2protocolhandler.cpp index 87dc504ee12..a99921f5288 100644 --- a/src/network/access/qhttp2protocolhandler.cpp +++ b/src/network/access/qhttp2protocolhandler.cpp @@ -151,15 +151,6 @@ void QHttp2ProtocolHandler::handleConnectionClosure() h2Connection->handleConnectionClosure(); } -void QHttp2ProtocolHandler::_q_replyDestroyed(QObject *reply) -{ - QPointer stream = streamIDs.take(reply); - requestReplyPairs.remove(stream); - QObject::disconnect(stream, nullptr, this, nullptr); - if (stream && stream->isActive()) - stream->sendRST_STREAM(CANCEL); -} - void QHttp2ProtocolHandler::_q_uploadDataDestroyed(QObject *uploadData) { QPointer stream = streamIDs.take(uploadData); @@ -262,6 +253,25 @@ bool QHttp2ProtocolHandler::sendRequest() return true; } +/*! + \internal + This gets called during destruction of \a reply, so do not call any functions + on \a reply. We check if there is a stream associated with the reply and, + if there is, we remove the request-reply pair associated with this stream, + delete the stream and return \c{true}. Otherwise nothing happens and we + return \c{false}. +*/ +bool QHttp2ProtocolHandler::tryRemoveReply(QHttpNetworkReply *reply) +{ + QHttp2Stream *stream = streamIDs.take(reply); + if (stream) { + requestReplyPairs.remove(stream); + stream->deleteLater(); + return true; + } + return false; +} + bool QHttp2ProtocolHandler::sendHEADERS(QHttp2Stream *stream, QHttpNetworkRequest &request) { using namespace HPack; @@ -623,8 +633,6 @@ void QHttp2ProtocolHandler::connectStream(const HttpMessagePair &message, QHttp2 auto *replyPrivate = reply->d_func(); replyPrivate->connection = m_connection; replyPrivate->connectionChannel = m_channel; - connect(reply, &QObject::destroyed, this, &QHttp2ProtocolHandler::_q_replyDestroyed, - Qt::UniqueConnection); reply->setHttp2WasUsed(true); QPointer &oldStream = streamIDs[reply]; diff --git a/src/network/access/qhttp2protocolhandler_p.h b/src/network/access/qhttp2protocolhandler_p.h index ecbc6823dcf..aca8a0b6f66 100644 --- a/src/network/access/qhttp2protocolhandler_p.h +++ b/src/network/access/qhttp2protocolhandler_p.h @@ -61,7 +61,6 @@ public: Q_INVOKABLE void handleConnectionClosure(); private slots: - void _q_replyDestroyed(QObject *reply); void _q_uploadDataDestroyed(QObject *uploadData); private: @@ -70,6 +69,7 @@ private: void _q_readyRead() override; Q_INVOKABLE void _q_receiveReply() override; Q_INVOKABLE bool sendRequest() override; + bool tryRemoveReply(QHttpNetworkReply *reply) override; bool sendSETTINGS_ACK(); bool sendHEADERS(QHttp2Stream *stream, QHttpNetworkRequest &request); diff --git a/src/network/access/qhttpnetworkconnection.cpp b/src/network/access/qhttpnetworkconnection.cpp index cf7aad1930d..4f105af084a 100644 --- a/src/network/access/qhttpnetworkconnection.cpp +++ b/src/network/access/qhttpnetworkconnection.cpp @@ -1002,6 +1002,13 @@ void QHttpNetworkConnectionPrivate::removeReply(QHttpNetworkReply *reply) QMetaObject::invokeMethod(q, "_q_startNextRequest", Qt::QueuedConnection); return; } + // Check if the h2 protocol handler already started processing it + if ((connectionType == QHttpNetworkConnection::ConnectionTypeHTTP2Direct + || channels[i].switchedToHttp2) + && channels[i].protocolHandler) { + if (channels[i].protocolHandler->tryRemoveReply(reply)) + return; + } } // remove from the high priority queue if (!highPriorityQueue.isEmpty()) { diff --git a/tests/auto/network/access/http2/http2srv.cpp b/tests/auto/network/access/http2/http2srv.cpp index 00c7669b38f..39b51a18c14 100644 --- a/tests/auto/network/access/http2/http2srv.cpp +++ b/tests/auto/network/access/http2/http2srv.cpp @@ -101,6 +101,11 @@ void Http2Server::setResponseBody(const QByteArray &body) responseBody = body; } +void Http2Server::enableSendEarlyError(bool enable) +{ + sendEarlyError = enable; +} + void Http2Server::setContentEncoding(const QByteArray &encoding) { contentEncoding = encoding; @@ -724,8 +729,12 @@ void Http2Server::handleDATA() const auto streamID = inboundFrame.streamID(); + // We need to allow this in the `sendEarlyError` case because it mirrors how + // we are required to allow some incoming frames in a grace-period after + // sending the peer a RST frame. We don't care about the grace period + // though. if (!is_valid_client_stream(streamID) || - closedStreams.find(streamID) != closedStreams.end()) { + (closedStreams.find(streamID) != closedStreams.end() && !sendEarlyError)) { emit invalidFrame(); connectionError = true; sendGOAWAY(connectionStreamID, PROTOCOL_ERROR, connectionStreamID); @@ -768,6 +777,14 @@ void Http2Server::handleDATA() sessionCurrRecvWindow += sessionRecvWindowSize / 2; } + if (sendEarlyError) { + if (activeRequests.find(streamID) != activeRequests.end()) { + responseBody = "not allowed"; + sendResponse(streamID, false); + } + return; + } + if (inboundFrame.flags().testFlag(FrameFlag::END_STREAM)) { if (responseBody.isEmpty()) { closedStreams.insert(streamID); // Enter "half-closed remote" state. @@ -904,6 +921,8 @@ void Http2Server::sendResponse(quint32 streamID, bool emptyBody) header.push_back(HPack::HeaderField("www-authenticate", authenticationHeader)); } else if (authenticationRequired) { header.push_back({ ":status", "401" }); + } else if (sendEarlyError) { + header.push_back({ ":status", "403" }); } else { header.push_back({":status", "200"}); } diff --git a/tests/auto/network/access/http2/http2srv.h b/tests/auto/network/access/http2/http2srv.h index c74a51bdb9d..3496233eb21 100644 --- a/tests/auto/network/access/http2/http2srv.h +++ b/tests/auto/network/access/http2/http2srv.h @@ -63,6 +63,7 @@ public: // To be called before server started: void enablePushPromise(bool enabled, const QByteArray &path = QByteArray()); void setResponseBody(const QByteArray &body); + void enableSendEarlyError(bool enable); // No content encoding is actually performed, call setResponseBody with already encoded data void setContentEncoding(const QByteArray &contentEncoding); // No authentication data is generated for the method, the full header value must be set @@ -147,6 +148,7 @@ private: RawSettings expectedClientSettings; bool connectionError = false; + bool sendEarlyError = false; Http2::FrameReader reader; Http2::Frame inboundFrame; diff --git a/tests/auto/network/access/http2/tst_http2.cpp b/tests/auto/network/access/http2/tst_http2.cpp index d7a36f61274..12f9cba6c79 100644 --- a/tests/auto/network/access/http2/tst_http2.cpp +++ b/tests/auto/network/access/http2/tst_http2.cpp @@ -11,6 +11,8 @@ #include "http2srv.h" +#include +#include #include #include #include @@ -21,6 +23,8 @@ #include #endif +#include +#include #include #include #include @@ -83,6 +87,7 @@ private slots: void goaway_data(); void goaway(); void earlyResponse(); + void earlyError(); void connectToHost_data(); void connectToHost(); void maxFrameSize(); @@ -687,6 +692,88 @@ void tst_Http2::earlyResponse() QVERIFY(serverGotSettingsACK); } +/* + Have the server return an error before we are done writing out POST data, + of course we should not crash if this happens. It's not guaranteed to + reproduce, so we run the request a few times to try to make it happen. + + This is a race-condition, so the test is written using QHttpNetworkConnection + to have more influence over the timing. +*/ +void tst_Http2::earlyError() +{ + clearHTTP2State(); + + serverPort = 0; + + const auto serverConnectionType = defaultConnectionType() == H2Type::h2c ? H2Type::h2Direct + : H2Type::h2Alpn; + ServerPtr server(newServer(defaultServerSettings, serverConnectionType)); + server->enableSendEarlyError(true); + QMetaObject::invokeMethod(server.data(), "startServer", Qt::QueuedConnection); + runEventLoop(); + QCOMPARE_NE(serverPort, 0); + + // SETUP create QHttpNetworkConnection primed for http2 usage + const auto connectionType = serverConnectionType == H2Type::h2Direct + ? QHttpNetworkConnection::ConnectionTypeHTTP2Direct + : QHttpNetworkConnection::ConnectionTypeHTTP2; + QHttpNetworkConnection connection(1, "127.0.0.1", serverPort, true, false, nullptr, + connectionType); + QSslConfiguration config = QSslConfiguration::defaultConfiguration(); + config.setAllowedNextProtocols({"h2"}); + connection.setSslConfiguration(config); + connection.ignoreSslErrors(); + + // SETUP manually setup the QHttpNetworkRequest + QHttpNetworkRequest req; + req.setSsl(true); + req.setHTTP2Allowed(true); + if (defaultConnectionType() == H2Type::h2c) + req.setH2cAllowed(true); + req.setOperation(QHttpNetworkRequest::Post); + req.setUrl(requestUrl(defaultConnectionType())); + + // ^ All the above is set-up, the real code starts below v + + // We need a sufficiently large payload so it can't be instantly transmitted + const QByteArray payload(1 * 1024 * 1024, 'a'); + auto byteDevice = std::unique_ptr( + QNonContiguousByteDeviceFactory::create(payload)); + req.setUploadByteDevice(byteDevice.get()); + + // Start sending the request. It needs to establish encryption so nothing + // happens right away (at time of writing...) + std::unique_ptr reply{connection.sendRequest(req)}; + QVERIFY(reply); + QSemaphore sem; + int statusCode = 0; + QObject::connect(reply.get(), &QHttpNetworkReply::finished, reply.get(), [&](){ + statusCode = reply->statusCode(); + // Here we forcibly replicate what happens when we get into the bad + // state: + // 1. The reply is aborted & deleted, but was not removed from internal + // container. + reply.reset(); + // 2. The byte-device is deleted afterwards, which would lead to + // use-after-free when we try to signal an error on the reply object. + byteDevice.reset(); + // Let the main thread continue + sem.release(); + }); + + using namespace std::chrono_literals; + QDeadlineTimer timer(5s); + while (!sem.tryAcquire() && !timer.hasExpired()) + QCoreApplication::processEvents(); + QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete); + QVERIFY(!reply); + QCOMPARE(statusCode, 403); + + QVERIFY(prefaceOK); + QTRY_VERIFY(serverGotSettingsACK); +} + void tst_Http2::connectToHost_data() { // The attribute to set on a new request: