From 5d71639ce0a0bb2c5573d427e4ce1b7ab8585434 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Tue, 6 May 2025 16:30:27 +0200 Subject: [PATCH] wip Http2: remove any reference to HttpReply when deleted Prior to switching the protocol handler to use QHttp2Connection this particular issue (see bugreport) was not a problem because the handling of the IO-device being destroyed was simply to drop any pointer to it. QHttp2Stream, however, also has to keep track of the lifetime of the IO-device, because it needs to abort the stream if the data it's uploading is destroyed earlier than expected. Now, since QHttp2Stream might also have other errors come up, we have to connect to the generic 'errorOccurred' signal from it and handle whatever issues arise, notifying our users that the request for some reason cannot be fulfilled. It's thanks to this part that we were now, in certain cases, grabbing a stale pointer to the HttpNetworkReply and trying to call functions on it. We fix this somewhat indirectly. Because, after a HttpReply is destroyed, we shouldn't even have any references to it in the first place. And while it would usually be done as part of handling the deleted() signal, we actually disconnect from HttpNetworkReply's signals when we have processed one of the finished*() functions. But since we were still connected to the stream's signals we would still try to handle it. For the http1 protocol handler this was already handled in QHttpNetworkConnection::removeReply, which the HttpNetworkReply itself calls at start of destruction. The function will go through any place that the reply can be referenced and removes it. For http/2 it would remove it from the list of requests yet to be sent, but not from the in-progress list. So, we now add a new virtual function to the AbstractProtocolHandler and specialize it in Http2 to handle exactly this. Fixes: QTBUG-136549 Change-Id: Ie41863677a3b163f77d10bc3904ca515f6840be3 --- .../access/qabstractprotocolhandler_p.h | 9 ++ src/network/access/qhttp2protocolhandler.cpp | 31 ++++--- src/network/access/qhttp2protocolhandler_p.h | 2 +- src/network/access/qhttpnetworkconnection.cpp | 7 ++ tests/auto/network/access/http2/http2srv.cpp | 21 ++++- tests/auto/network/access/http2/http2srv.h | 2 + tests/auto/network/access/http2/tst_http2.cpp | 87 +++++++++++++++++++ 7 files changed, 144 insertions(+), 15 deletions(-) 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 b288e3a885f..6f5a59fd261 100644 --- a/src/network/access/qhttp2protocolhandler.cpp +++ b/src/network/access/qhttp2protocolhandler.cpp @@ -182,16 +182,6 @@ void QHttp2ProtocolHandler::_q_uploadDataReadyRead() } } -void QHttp2ProtocolHandler::_q_replyDestroyed(QObject *reply) -{ - const quint32 streamID = streamIDs.take(reply); - if (activeStreams.contains(streamID)) { - sendRST_STREAM(streamID, CANCEL); - markAsReset(streamID); - deleteActiveStream(streamID); - } -} - void QHttp2ProtocolHandler::_q_uploadDataDestroyed(QObject *uploadData) { streamIDs.remove(uploadData); @@ -364,7 +354,6 @@ bool QHttp2ProtocolHandler::sendRequest() return true; } - bool QHttp2ProtocolHandler::sendClientPreface() { // 3.5 HTTP/2 Connection Preface @@ -410,6 +399,23 @@ bool QHttp2ProtocolHandler::sendSETTINGS_ACK() return frameWriter.write(*m_socket); } +/*! + \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) +{ + auto it = streamIDs.find(reply); + if (it == streamIDs.end() || *it <= 0) + return false; + deleteActiveStream(quint32(*it)); + return true; +} + bool QHttp2ProtocolHandler::sendHEADERS(Stream &stream) { using namespace HPack; @@ -1357,10 +1363,9 @@ quint32 QHttp2ProtocolHandler::createNewStream(const HttpMessagePair &message, b const auto replyPrivate = reply->d_func(); replyPrivate->connection = m_connection; replyPrivate->connectionChannel = m_channel; + reply->setHttp2WasUsed(true); streamIDs.insert(reply, newStreamID); - connect(reply, SIGNAL(destroyed(QObject*)), - this, SLOT(_q_replyDestroyed(QObject*))); const Stream newStream(message, newStreamID, streamInitialSendWindowSize, diff --git a/src/network/access/qhttp2protocolhandler_p.h b/src/network/access/qhttp2protocolhandler_p.h index 80581ea1313..9c1db023444 100644 --- a/src/network/access/qhttp2protocolhandler_p.h +++ b/src/network/access/qhttp2protocolhandler_p.h @@ -61,7 +61,6 @@ public: private slots: void _q_uploadDataReadyRead(); - 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 sendClientPreface(); bool sendSETTINGS_ACK(); diff --git a/src/network/access/qhttpnetworkconnection.cpp b/src/network/access/qhttpnetworkconnection.cpp index 71d18a64124..4888a16e48c 100644 --- a/src/network/access/qhttpnetworkconnection.cpp +++ b/src/network/access/qhttpnetworkconnection.cpp @@ -1003,6 +1003,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 afb50dd5437..666099eb466 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(); @@ -684,6 +689,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: -- 2.47.0.windows.2