1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
|
https://bugs.gentoo.org/935869
https://github.com/qt/qtbase/commit/2b1e36e183ce75c224305c7a94457b92f7a5cf58
From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= <marten.nordheim@qt.io>
Date: Tue, 25 Jun 2024 17:09:35 +0200
Subject: [PATCH] HTTP2: Delay any communication until encrypted() can be
responded to
We have the encrypted() signal that lets users do extra checks on the
established connection. It is emitted as BlockingQueued, so the HTTP
thread stalls until it is done emitting. Users can potentially call
abort() on the QNetworkReply at that point, which is passed as a Queued
call back to the HTTP thread. That means that any currently queued
signal emission will be processed before the abort() call is processed.
In the case of HTTP2 it is a little special since it is multiplexed and
the code is built to start requests as they are available. This means
that, while the code worked fine for HTTP1, since one connection only
has one request, it is not working for HTTP2, since we try to send more
requests in-between the encrypted() signal and the abort() call.
This patch changes the code to delay any communication until the
encrypted() signal has been emitted and processed, for HTTP2 only.
It's done by adding a few booleans, both to know that we have to return
early and so we can keep track of what events arose and what we need to
resume once enough time has passed that any abort() call must have been
processed.
Fixes: QTBUG-126610
--- a/src/network/access/qhttp2protocolhandler.cpp
+++ b/src/network/access/qhttp2protocolhandler.cpp
@@ -304,10 +304,10 @@
}
- if (!prefaceSent && !sendClientPreface())
- return false;
-
if (!requests.size())
return true;
+ if (!prefaceSent && !sendClientPreface())
+ return false;
+
m_channel->state = QHttpNetworkConnectionChannel::WritingState;
// Check what was promised/pushed, maybe we do not have to send a request
--- a/src/network/access/qhttpnetworkconnectionchannel.cpp
+++ b/src/network/access/qhttpnetworkconnectionchannel.cpp
@@ -210,4 +210,8 @@
{
Q_ASSERT(protocolHandler);
+ if (waitingForPotentialAbort) {
+ needInvokeSendRequest = true;
+ return false; // this return value is unused
+ }
return protocolHandler->sendRequest();
}
@@ -222,7 +226,6 @@
{
QMetaObject::invokeMethod(this, [this] {
- Q_ASSERT(protocolHandler);
if (reply)
- protocolHandler->sendRequest();
+ sendRequest();
}, Qt::ConnectionType::QueuedConnection);
}
@@ -231,4 +234,8 @@
{
Q_ASSERT(protocolHandler);
+ if (waitingForPotentialAbort) {
+ needInvokeReceiveReply = true;
+ return;
+ }
protocolHandler->_q_receiveReply();
}
@@ -237,4 +244,8 @@
{
Q_ASSERT(protocolHandler);
+ if (waitingForPotentialAbort) {
+ needInvokeReadyRead = true;
+ return;
+ }
protocolHandler->_q_readyRead();
}
@@ -1240,5 +1251,16 @@
// Similar to HTTP/1.1 counterpart below:
const auto &pair = std::as_const(h2RequestsToSend).first();
+ waitingForPotentialAbort = true;
emit pair.second->encrypted();
+
+ // We don't send or handle any received data until any effects from
+ // emitting encrypted() have been processed. This is necessary
+ // because the user may have called abort(). We may also abort the
+ // whole connection if the request has been aborted and there is
+ // no more requests to send.
+ QMetaObject::invokeMethod(this,
+ &QHttpNetworkConnectionChannel::checkAndResumeCommunication,
+ Qt::QueuedConnection);
+
// In case our peer has sent us its settings (window size, max concurrent streams etc.)
// let's give _q_receiveReply a chance to read them first ('invokeMethod', QueuedConnection).
@@ -1258,4 +1280,26 @@
}
+
+void QHttpNetworkConnectionChannel::checkAndResumeCommunication()
+{
+ Q_ASSERT(connection->connectionType() == QHttpNetworkConnection::ConnectionTypeHTTP2
+ || connection->connectionType() == QHttpNetworkConnection::ConnectionTypeHTTP2Direct);
+
+ // Because HTTP/2 requires that we send a SETTINGS frame as the first thing we do, and respond
+ // to a SETTINGS frame with an ACK, we need to delay any handling until we can ensure that any
+ // effects from emitting encrypted() have been processed.
+ // This function is called after encrypted() was emitted, so check for changes.
+
+ if (!reply && h2RequestsToSend.isEmpty())
+ abort();
+ waitingForPotentialAbort = false;
+ if (needInvokeReadyRead)
+ _q_readyRead();
+ if (needInvokeReceiveReply)
+ _q_receiveReply();
+ if (needInvokeSendRequest)
+ sendRequest();
+}
+
void QHttpNetworkConnectionChannel::requeueHttp2Requests()
{
--- a/src/network/access/qhttpnetworkconnectionchannel_p.h
+++ b/src/network/access/qhttpnetworkconnectionchannel_p.h
@@ -75,4 +75,8 @@
bool ssl;
bool isInitialized;
+ bool waitingForPotentialAbort = false;
+ bool needInvokeReceiveReply = false;
+ bool needInvokeReadyRead = false;
+ bool needInvokeSendRequest = false;
ChannelState state;
QHttpNetworkRequest request; // current request, only used for HTTP
@@ -147,4 +151,6 @@
void resendCurrentRequest();
+ void checkAndResumeCommunication();
+
bool isSocketBusy() const;
bool isSocketWriting() const;
--- a/tests/auto/network/access/http2/tst_http2.cpp
+++ b/tests/auto/network/access/http2/tst_http2.cpp
@@ -107,4 +107,6 @@
void duplicateRequestsWithAborts();
+ void abortOnEncrypted();
+
protected slots:
// Slots to listen to our in-process server:
@@ -1480,4 +1482,46 @@
}
+void tst_Http2::abortOnEncrypted()
+{
+#if !QT_CONFIG(ssl)
+ QSKIP("TLS support is needed for this test");
+#else
+ clearHTTP2State();
+ serverPort = 0;
+
+ ServerPtr targetServer(newServer(defaultServerSettings, H2Type::h2Direct));
+
+ QMetaObject::invokeMethod(targetServer.data(), "startServer", Qt::QueuedConnection);
+ runEventLoop();
+
+ nRequests = 1;
+ nSentRequests = 0;
+
+ const auto url = requestUrl(H2Type::h2Direct);
+ QNetworkRequest request(url);
+ request.setAttribute(QNetworkRequest::Http2DirectAttribute, true);
+
+ std::unique_ptr<QNetworkReply> reply{manager->get(request)};
+ reply->ignoreSslErrors();
+ connect(reply.get(), &QNetworkReply::encrypted, reply.get(), [reply = reply.get()](){
+ reply->abort();
+ });
+ connect(reply.get(), &QNetworkReply::errorOccurred, this, &tst_Http2::replyFinishedWithError);
+
+ runEventLoop();
+ STOP_ON_FAILURE
+
+ QCOMPARE(nRequests, 0);
+ QCOMPARE(reply->error(), QNetworkReply::OperationCanceledError);
+
+ const bool res = QTest::qWaitFor(
+ [this, server = targetServer.get()]() {
+ return serverGotSettingsACK || prefaceOK || nSentRequests > 0;
+ },
+ 500);
+ QVERIFY(!res);
+#endif // QT_CONFIG(ssl)
+}
+
void tst_Http2::serverStarted(quint16 port)
{
|
GitWeb