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
Reviewed-by: Mate Barany <mate.barany@qt.io>
(cherry picked from commit b0b9adf06675c5caa37105ceee157245e3dcca21)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
(cherry picked from commit 6b4e11e63ead46dde5c1002c123ca964bb6aa342)

src/network/access/qabstractprotocolhandler_p.h

@@ -17,6 +17,8 @@
 
 #include <QtNetwork/private/qtnetworkglobal_p.h>
 
+#include <QtCore/qtpreprocessorsupport.h>
+
 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:

src/network/access/qhttp2protocolhandler.cpp

@@ -151,15 +151,6 @@ void QHttp2ProtocolHandler::handleConnectionClosure()
     h2Connection->handleConnectionClosure();
 }
 
-void QHttp2ProtocolHandler::_q_replyDestroyed(QObject *reply)
-{
-    QPointer<QHttp2Stream> 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<QHttp2Stream> 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<QHttp2Stream> &oldStream = streamIDs[reply];

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

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

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"});
     }

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;

tests/auto/network/access/http2/tst_http2.cpp

@@ -11,6 +11,8 @@
 
 #include "http2srv.h"
 
+#include <QtNetwork/private/qhttpnetworkconnection_p.h>
+#include <QtNetwork/private/qhttpnetworkreply_p.h>
 #include <QtNetwork/private/http2protocol_p.h>
 #include <QtNetwork/qnetworkaccessmanager.h>
 #include <QtNetwork/qhttp2configuration.h>
@@ -21,6 +23,8 @@
 #include <QtNetwork/qsslsocket.h>
 #endif
 
+#include <QtCore/private/qnoncontiguousbytedevice_p.h>
+#include <QtCore/qsemaphore.h>
 #include <QtCore/qglobal.h>
 #include <QtCore/qobject.h>
 #include <QtCore/qthread.h>
@@ -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<QNonContiguousByteDevice>(
+            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<QHttpNetworkReply> 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: