Line 0
Link Here
|
|
|
1 |
--- src/corelib/kernel/qeventdispatcher_glib.cpp.sav 2014-03-28 15:26:37.000000000 +0100 |
Line 0
Link Here
|
|
|
1 |
From fa81aa6d027049e855b76f5408586a288f160575 Mon Sep 17 00:00:00 2001 |
2 |
From: Markus Goetz <markus@woboq.com> |
3 |
Date: Tue, 28 Apr 2015 11:57:36 +0200 |
4 |
Subject: QNAM: Fix upload corruptions when server closes connection |
5 |
|
6 |
This patch fixes several upload corruptions if the server closes the connection |
7 |
while/before we send data into it. They happen inside multiple places in the HTTP |
8 |
layer and are explained in the comments. |
9 |
Corruptions are: |
10 |
* The upload byte device has an in-flight signal with pending upload data, if |
11 |
it gets reset (because server closes the connection) then the re-send of the |
12 |
request was sometimes taking this stale in-flight pending upload data. |
13 |
* Because some signals were DirectConnection and some were QueuedConnection, there |
14 |
was a chance that a direct signal overtakes a queued signal. The state machine |
15 |
then sent data down the socket which was buffered there (and sent later) although |
16 |
it did not match the current state of the state machine when it was actually sent. |
17 |
* A socket was seen as being able to have requests sent even though it was not |
18 |
encrypted yet. This relates to the previous corruption where data is stored inside |
19 |
the socket's buffer and then sent later. |
20 |
|
21 |
The included auto test produces all fixed corruptions, I detected no regressions |
22 |
via the other tests. |
23 |
This code also adds a bit of sanity checking to protect from possible further |
24 |
problems. |
25 |
|
26 |
[ChangeLog][QtNetwork] Fix HTTP(s) upload corruption when server closes connection |
27 |
|
28 |
(cherry picked from commit qtbase/cff39fba10ffc10ee4dcfdc66ff6528eb26462d3) |
29 |
Change-Id: I9793297be6cf3edfb75b65ba03b65f7a133ef194 |
30 |
Reviewed-by: Richard J. Moore <rich@kde.org> |
31 |
--- |
32 |
src/corelib/io/qnoncontiguousbytedevice.cpp | 19 +++ |
33 |
src/corelib/io/qnoncontiguousbytedevice_p.h | 4 + |
34 |
.../access/qhttpnetworkconnectionchannel.cpp | 47 +++++- |
35 |
src/network/access/qhttpthreaddelegate_p.h | 36 ++++- |
36 |
src/network/access/qnetworkaccesshttpbackend.cpp | 24 ++- |
37 |
src/network/access/qnetworkaccesshttpbackend_p.h | 5 +- |
38 |
tests/auto/qnetworkreply/tst_qnetworkreply.cpp | 174 ++++++++++++++++++++- |
39 |
7 files changed, 280 insertions(+), 29 deletions(-) |
40 |
|
41 |
diff --git a/src/corelib/io/qnoncontiguousbytedevice.cpp b/src/corelib/io/qnoncontiguousbytedevice.cpp |
42 |
index bf58eee..1a0591e 100644 |
43 |
--- a/src/corelib/io/qnoncontiguousbytedevice.cpp |
44 |
+++ b/src/corelib/io/qnoncontiguousbytedevice.cpp |
45 |
@@ -245,6 +245,12 @@ qint64 QNonContiguousByteDeviceByteArrayImpl::size() |
46 |
return byteArray->size(); |
47 |
} |
48 |
|
49 |
+qint64 QNonContiguousByteDeviceByteArrayImpl::pos() |
50 |
+{ |
51 |
+ return currentPosition; |
52 |
+} |
53 |
+ |
54 |
+ |
55 |
QNonContiguousByteDeviceRingBufferImpl::QNonContiguousByteDeviceRingBufferImpl(QSharedPointer<QRingBuffer> rb) |
56 |
: QNonContiguousByteDevice(), currentPosition(0) |
57 |
{ |
58 |
@@ -296,6 +302,11 @@ qint64 QNonContiguousByteDeviceRingBufferImpl::size() |
59 |
return ringBuffer->size(); |
60 |
} |
61 |
|
62 |
+qint64 QNonContiguousByteDeviceRingBufferImpl::pos() |
63 |
+{ |
64 |
+ return currentPosition; |
65 |
+} |
66 |
+ |
67 |
QNonContiguousByteDeviceIoDeviceImpl::QNonContiguousByteDeviceIoDeviceImpl(QIODevice *d) |
68 |
: QNonContiguousByteDevice(), |
69 |
currentReadBuffer(0), currentReadBufferSize(16*1024), |
70 |
@@ -415,6 +426,14 @@ qint64 QNonContiguousByteDeviceIoDeviceImpl::size() |
71 |
return device->size() - initialPosition; |
72 |
} |
73 |
|
74 |
+qint64 QNonContiguousByteDeviceIoDeviceImpl::pos() |
75 |
+{ |
76 |
+ if (device->isSequential()) |
77 |
+ return -1; |
78 |
+ |
79 |
+ return device->pos(); |
80 |
+} |
81 |
+ |
82 |
QByteDeviceWrappingIoDevice::QByteDeviceWrappingIoDevice(QNonContiguousByteDevice *bd) : QIODevice((QObject*)0) |
83 |
{ |
84 |
byteDevice = bd; |
85 |
diff --git a/src/corelib/io/qnoncontiguousbytedevice_p.h b/src/corelib/io/qnoncontiguousbytedevice_p.h |
86 |
index b6966eb..d1a99a1 100644 |
87 |
--- a/src/corelib/io/qnoncontiguousbytedevice_p.h |
88 |
+++ b/src/corelib/io/qnoncontiguousbytedevice_p.h |
89 |
@@ -69,6 +69,7 @@ public: |
90 |
virtual const char* readPointer(qint64 maximumLength, qint64 &len) = 0; |
91 |
virtual bool advanceReadPointer(qint64 amount) = 0; |
92 |
virtual bool atEnd() = 0; |
93 |
+ virtual qint64 pos() { return -1; } |
94 |
virtual bool reset() = 0; |
95 |
void disableReset(); |
96 |
bool isResetDisabled() { return resetDisabled; } |
97 |
@@ -108,6 +109,7 @@ public: |
98 |
bool atEnd(); |
99 |
bool reset(); |
100 |
qint64 size(); |
101 |
+ qint64 pos(); |
102 |
protected: |
103 |
QByteArray* byteArray; |
104 |
qint64 currentPosition; |
105 |
@@ -123,6 +125,7 @@ public: |
106 |
bool atEnd(); |
107 |
bool reset(); |
108 |
qint64 size(); |
109 |
+ qint64 pos(); |
110 |
protected: |
111 |
QSharedPointer<QRingBuffer> ringBuffer; |
112 |
qint64 currentPosition; |
113 |
@@ -140,6 +143,7 @@ public: |
114 |
bool atEnd(); |
115 |
bool reset(); |
116 |
qint64 size(); |
117 |
+ qint64 pos(); |
118 |
protected: |
119 |
QIODevice* device; |
120 |
QByteArray* currentReadBuffer; |
121 |
diff --git a/src/network/access/qhttpnetworkconnectionchannel.cpp b/src/network/access/qhttpnetworkconnectionchannel.cpp |
122 |
index 550e090..db2f712 100644 |
123 |
--- a/src/network/access/qhttpnetworkconnectionchannel.cpp |
124 |
+++ b/src/network/access/qhttpnetworkconnectionchannel.cpp |
125 |
@@ -107,15 +107,19 @@ void QHttpNetworkConnectionChannel::init() |
126 |
socket->setProxy(QNetworkProxy::NoProxy); |
127 |
#endif |
128 |
|
129 |
+ // We want all signals (except the interactive ones) be connected as QueuedConnection |
130 |
+ // because else we're falling into cases where we recurse back into the socket code |
131 |
+ // and mess up the state. Always going to the event loop (and expecting that when reading/writing) |
132 |
+ // is safer. |
133 |
QObject::connect(socket, SIGNAL(bytesWritten(qint64)), |
134 |
this, SLOT(_q_bytesWritten(qint64)), |
135 |
- Qt::DirectConnection); |
136 |
+ Qt::QueuedConnection); |
137 |
QObject::connect(socket, SIGNAL(connected()), |
138 |
this, SLOT(_q_connected()), |
139 |
- Qt::DirectConnection); |
140 |
+ Qt::QueuedConnection); |
141 |
QObject::connect(socket, SIGNAL(readyRead()), |
142 |
this, SLOT(_q_readyRead()), |
143 |
- Qt::DirectConnection); |
144 |
+ Qt::QueuedConnection); |
145 |
|
146 |
// The disconnected() and error() signals may already come |
147 |
// while calling connectToHost(). |
148 |
@@ -144,13 +148,13 @@ void QHttpNetworkConnectionChannel::init() |
149 |
// won't be a sslSocket if encrypt is false |
150 |
QObject::connect(sslSocket, SIGNAL(encrypted()), |
151 |
this, SLOT(_q_encrypted()), |
152 |
- Qt::DirectConnection); |
153 |
+ Qt::QueuedConnection); |
154 |
QObject::connect(sslSocket, SIGNAL(sslErrors(QList<QSslError>)), |
155 |
this, SLOT(_q_sslErrors(QList<QSslError>)), |
156 |
Qt::DirectConnection); |
157 |
QObject::connect(sslSocket, SIGNAL(encryptedBytesWritten(qint64)), |
158 |
this, SLOT(_q_encryptedBytesWritten(qint64)), |
159 |
- Qt::DirectConnection); |
160 |
+ Qt::QueuedConnection); |
161 |
} |
162 |
#endif |
163 |
} |
164 |
@@ -163,7 +167,8 @@ void QHttpNetworkConnectionChannel::close() |
165 |
else |
166 |
state = QHttpNetworkConnectionChannel::ClosingState; |
167 |
|
168 |
- socket->close(); |
169 |
+ if (socket) |
170 |
+ socket->close(); |
171 |
} |
172 |
|
173 |
|
174 |
@@ -280,6 +285,14 @@ bool QHttpNetworkConnectionChannel::sendRequest() |
175 |
// nothing to read currently, break the loop |
176 |
break; |
177 |
} else { |
178 |
+ if (written != uploadByteDevice->pos()) { |
179 |
+ // Sanity check. This was useful in tracking down an upload corruption. |
180 |
+ qWarning() << "QHttpProtocolHandler: Internal error in sendRequest. Expected to write at position" << written << "but read device is at" << uploadByteDevice->pos(); |
181 |
+ Q_ASSERT(written == uploadByteDevice->pos()); |
182 |
+ connection->d_func()->emitReplyError(socket, reply, QNetworkReply::ProtocolFailure); |
183 |
+ return false; |
184 |
+ } |
185 |
+ |
186 |
qint64 currentWriteSize = socket->write(readPointer, currentReadSize); |
187 |
if (currentWriteSize == -1 || currentWriteSize != currentReadSize) { |
188 |
// socket broke down |
189 |
@@ -639,6 +652,14 @@ bool QHttpNetworkConnectionChannel::ensureConnection() |
190 |
} |
191 |
return false; |
192 |
} |
193 |
+ |
194 |
+ // This code path for ConnectedState |
195 |
+ if (pendingEncrypt) { |
196 |
+ // Let's only be really connected when we have received the encrypted() signal. Else the state machine seems to mess up |
197 |
+ // and corrupt the things sent to the server. |
198 |
+ return false; |
199 |
+ } |
200 |
+ |
201 |
return true; |
202 |
} |
203 |
|
204 |
@@ -980,6 +1001,13 @@ void QHttpNetworkConnectionChannel::_q_readyRead() |
205 |
void QHttpNetworkConnectionChannel::_q_bytesWritten(qint64 bytes) |
206 |
{ |
207 |
Q_UNUSED(bytes); |
208 |
+ |
209 |
+ if (ssl) { |
210 |
+ // In the SSL case we want to send data from encryptedBytesWritten signal since that one |
211 |
+ // is the one going down to the actual network, not only into some SSL buffer. |
212 |
+ return; |
213 |
+ } |
214 |
+ |
215 |
// bytes have been written to the socket. write even more of them :) |
216 |
if (isSocketWriting()) |
217 |
sendRequest(); |
218 |
@@ -1029,7 +1057,7 @@ void QHttpNetworkConnectionChannel::_q_connected() |
219 |
|
220 |
// ### FIXME: if the server closes the connection unexpectedly, we shouldn't send the same broken request again! |
221 |
//channels[i].reconnectAttempts = 2; |
222 |
- if (!pendingEncrypt) { |
223 |
+ if (!pendingEncrypt && !ssl) { // FIXME: Didn't work properly with pendingEncrypt only, we should refactor this into an EncrypingState |
224 |
state = QHttpNetworkConnectionChannel::IdleState; |
225 |
if (!reply) |
226 |
connection->d_func()->dequeueRequest(socket); |
227 |
@@ -1157,7 +1185,10 @@ void QHttpNetworkConnectionChannel::_q_proxyAuthenticationRequired(const QNetwor |
228 |
|
229 |
void QHttpNetworkConnectionChannel::_q_uploadDataReadyRead() |
230 |
{ |
231 |
- sendRequest(); |
232 |
+ if (reply && state == QHttpNetworkConnectionChannel::WritingState) { |
233 |
+ // There might be timing issues, make sure to only send upload data if really in that state |
234 |
+ sendRequest(); |
235 |
+ } |
236 |
} |
237 |
|
238 |
#ifndef QT_NO_OPENSSL |
239 |
diff --git a/src/network/access/qhttpthreaddelegate_p.h b/src/network/access/qhttpthreaddelegate_p.h |
240 |
index 7648325..9dd0deb 100644 |
241 |
--- a/src/network/access/qhttpthreaddelegate_p.h |
242 |
+++ b/src/network/access/qhttpthreaddelegate_p.h |
243 |
@@ -190,6 +190,7 @@ protected: |
244 |
QByteArray m_dataArray; |
245 |
bool m_atEnd; |
246 |
qint64 m_size; |
247 |
+ qint64 m_pos; // to match calls of haveDataSlot with the expected position |
248 |
public: |
249 |
QNonContiguousByteDeviceThreadForwardImpl(bool aE, qint64 s) |
250 |
: QNonContiguousByteDevice(), |
251 |
@@ -197,7 +198,8 @@ public: |
252 |
m_amount(0), |
253 |
m_data(0), |
254 |
m_atEnd(aE), |
255 |
- m_size(s) |
256 |
+ m_size(s), |
257 |
+ m_pos(0) |
258 |
{ |
259 |
} |
260 |
|
261 |
@@ -205,6 +207,11 @@ public: |
262 |
{ |
263 |
} |
264 |
|
265 |
+ qint64 pos() |
266 |
+ { |
267 |
+ return m_pos; |
268 |
+ } |
269 |
+ |
270 |
const char* readPointer(qint64 maximumLength, qint64 &len) |
271 |
{ |
272 |
if (m_amount > 0) { |
273 |
@@ -232,11 +239,10 @@ public: |
274 |
|
275 |
m_amount -= a; |
276 |
m_data += a; |
277 |
+ m_pos += a; |
278 |
|
279 |
- // To main thread to inform about our state |
280 |
- emit processedData(a); |
281 |
- |
282 |
- // FIXME possible optimization, already ask user thread for some data |
283 |
+ // To main thread to inform about our state. The m_pos will be sent as a sanity check. |
284 |
+ emit processedData(m_pos, a); |
285 |
|
286 |
return true; |
287 |
} |
288 |
@@ -253,10 +259,21 @@ public: |
289 |
{ |
290 |
m_amount = 0; |
291 |
m_data = 0; |
292 |
+ m_dataArray.clear(); |
293 |
+ |
294 |
+ if (wantDataPending) { |
295 |
+ // had requested the user thread to send some data (only 1 in-flight at any moment) |
296 |
+ wantDataPending = false; |
297 |
+ } |
298 |
|
299 |
// Communicate as BlockingQueuedConnection |
300 |
bool b = false; |
301 |
emit resetData(&b); |
302 |
+ if (b) { |
303 |
+ // the reset succeeded, we're at pos 0 again |
304 |
+ m_pos = 0; |
305 |
+ // the HTTP code will anyway abort the request if !b. |
306 |
+ } |
307 |
return b; |
308 |
} |
309 |
|
310 |
@@ -267,8 +284,13 @@ public: |
311 |
|
312 |
public slots: |
313 |
// From user thread: |
314 |
- void haveDataSlot(QByteArray dataArray, bool dataAtEnd, qint64 dataSize) |
315 |
+ void haveDataSlot(qint64 pos, QByteArray dataArray, bool dataAtEnd, qint64 dataSize) |
316 |
{ |
317 |
+ if (pos != m_pos) { |
318 |
+ // Sometimes when re-sending a request in the qhttpnetwork* layer there is a pending haveData from the |
319 |
+ // user thread on the way to us. We need to ignore it since it is the data for the wrong(later) chunk. |
320 |
+ return; |
321 |
+ } |
322 |
wantDataPending = false; |
323 |
|
324 |
m_dataArray = dataArray; |
325 |
@@ -288,7 +310,7 @@ signals: |
326 |
|
327 |
// to main thread: |
328 |
void wantData(qint64); |
329 |
- void processedData(qint64); |
330 |
+ void processedData(qint64 pos, qint64 amount); |
331 |
void resetData(bool *b); |
332 |
}; |
333 |
|
334 |
diff --git a/src/network/access/qnetworkaccesshttpbackend.cpp b/src/network/access/qnetworkaccesshttpbackend.cpp |
335 |
index cc67258..fe2f627 100644 |
336 |
--- a/src/network/access/qnetworkaccesshttpbackend.cpp |
337 |
+++ b/src/network/access/qnetworkaccesshttpbackend.cpp |
338 |
@@ -193,6 +193,7 @@ QNetworkAccessHttpBackendFactory::create(QNetworkAccessManager::Operation op, |
339 |
QNetworkAccessHttpBackend::QNetworkAccessHttpBackend() |
340 |
: QNetworkAccessBackend() |
341 |
, statusCode(0) |
342 |
+ , uploadByteDevicePosition(false) |
343 |
, pendingDownloadDataEmissions(new QAtomicInt()) |
344 |
, pendingDownloadProgressEmissions(new QAtomicInt()) |
345 |
, loadingFromCache(false) |
346 |
@@ -610,9 +611,9 @@ void QNetworkAccessHttpBackend::postRequest() |
347 |
forwardUploadDevice->setParent(delegate); // needed to make sure it is moved on moveToThread() |
348 |
delegate->httpRequest.setUploadByteDevice(forwardUploadDevice); |
349 |
|
350 |
- // From main thread to user thread: |
351 |
- QObject::connect(this, SIGNAL(haveUploadData(QByteArray, bool, qint64)), |
352 |
- forwardUploadDevice, SLOT(haveDataSlot(QByteArray, bool, qint64)), Qt::QueuedConnection); |
353 |
+ // From user thread to http thread: |
354 |
+ QObject::connect(this, SIGNAL(haveUploadData(qint64,QByteArray,bool,qint64)), |
355 |
+ forwardUploadDevice, SLOT(haveDataSlot(qint64,QByteArray,bool,qint64)), Qt::QueuedConnection); |
356 |
QObject::connect(uploadByteDevice.data(), SIGNAL(readyRead()), |
357 |
forwardUploadDevice, SIGNAL(readyRead()), |
358 |
Qt::QueuedConnection); |
359 |
@@ -620,8 +621,8 @@ void QNetworkAccessHttpBackend::postRequest() |
360 |
// From http thread to user thread: |
361 |
QObject::connect(forwardUploadDevice, SIGNAL(wantData(qint64)), |
362 |
this, SLOT(wantUploadDataSlot(qint64))); |
363 |
- QObject::connect(forwardUploadDevice, SIGNAL(processedData(qint64)), |
364 |
- this, SLOT(sentUploadDataSlot(qint64))); |
365 |
+ QObject::connect(forwardUploadDevice,SIGNAL(processedData(qint64, qint64)), |
366 |
+ this, SLOT(sentUploadDataSlot(qint64,qint64))); |
367 |
connect(forwardUploadDevice, SIGNAL(resetData(bool*)), |
368 |
this, SLOT(resetUploadDataSlot(bool*)), |
369 |
Qt::BlockingQueuedConnection); // this is the only one with BlockingQueued! |
370 |
@@ -915,12 +916,21 @@ void QNetworkAccessHttpBackend::replySslConfigurationChanged(const QSslConfigura |
371 |
void QNetworkAccessHttpBackend::resetUploadDataSlot(bool *r) |
372 |
{ |
373 |
*r = uploadByteDevice->reset(); |
374 |
+ if (*r) { |
375 |
+ // reset our own position which is used for the inter-thread communication |
376 |
+ uploadByteDevicePosition = 0; |
377 |
+ } |
378 |
} |
379 |
|
380 |
// Coming from QNonContiguousByteDeviceThreadForwardImpl in HTTP thread |
381 |
-void QNetworkAccessHttpBackend::sentUploadDataSlot(qint64 amount) |
382 |
+void QNetworkAccessHttpBackend::sentUploadDataSlot(qint64 pos, qint64 amount) |
383 |
{ |
384 |
+ if (uploadByteDevicePosition + amount != pos) { |
385 |
+ // Sanity check, should not happen. |
386 |
+ error(QNetworkReply::UnknownNetworkError, ""); |
387 |
+ } |
388 |
uploadByteDevice->advanceReadPointer(amount); |
389 |
+ uploadByteDevicePosition += amount; |
390 |
} |
391 |
|
392 |
// Coming from QNonContiguousByteDeviceThreadForwardImpl in HTTP thread |
393 |
@@ -933,7 +943,7 @@ void QNetworkAccessHttpBackend::wantUploadDataSlot(qint64 maxSize) |
394 |
QByteArray dataArray(data, currentUploadDataLength); |
395 |
|
396 |
// Communicate back to HTTP thread |
397 |
- emit haveUploadData(dataArray, uploadByteDevice->atEnd(), uploadByteDevice->size()); |
398 |
+ emit haveUploadData(uploadByteDevicePosition, dataArray, uploadByteDevice->atEnd(), uploadByteDevice->size()); |
399 |
} |
400 |
|
401 |
/* |
402 |
diff --git a/src/network/access/qnetworkaccesshttpbackend_p.h b/src/network/access/qnetworkaccesshttpbackend_p.h |
403 |
index 13519c6..b4ed67c 100644 |
404 |
--- a/src/network/access/qnetworkaccesshttpbackend_p.h |
405 |
+++ b/src/network/access/qnetworkaccesshttpbackend_p.h |
406 |
@@ -112,7 +112,7 @@ signals: |
407 |
|
408 |
void startHttpRequestSynchronously(); |
409 |
|
410 |
- void haveUploadData(QByteArray dataArray, bool dataAtEnd, qint64 dataSize); |
411 |
+ void haveUploadData(const qint64 pos, QByteArray dataArray, bool dataAtEnd, qint64 dataSize); |
412 |
private slots: |
413 |
// From HTTP thread: |
414 |
void replyDownloadData(QByteArray); |
415 |
@@ -129,13 +129,14 @@ private slots: |
416 |
// From QNonContiguousByteDeviceThreadForwardImpl in HTTP thread: |
417 |
void resetUploadDataSlot(bool *r); |
418 |
void wantUploadDataSlot(qint64); |
419 |
- void sentUploadDataSlot(qint64); |
420 |
+ void sentUploadDataSlot(qint64, qint64); |
421 |
|
422 |
bool sendCacheContents(const QNetworkCacheMetaData &metaData); |
423 |
|
424 |
private: |
425 |
QHttpNetworkRequest httpRequest; // There is also a copy in the HTTP thread |
426 |
int statusCode; |
427 |
+ qint64 uploadByteDevicePosition; |
428 |
QString reasonPhrase; |
429 |
// Will be increased by HTTP thread: |
430 |
QSharedPointer<QAtomicInt> pendingDownloadDataEmissions; |
431 |
|