From cff39fba10ffc10ee4dcfdc66ff6528eb26462d3 Mon Sep 17 00:00:00 2001 From: Markus Goetz Date: Fri, 10 Apr 2015 14:09:53 +0200 Subject: QNAM: Fix upload corruptions when server closes connection This patch fixes several upload corruptions if the server closes the connection while/before we send data into it. They happen inside multiple places in the HTTP layer and are explained in the comments. Corruptions are: * The upload byte device has an in-flight signal with pending upload data, if it gets reset (because server closes the connection) then the re-send of the request was sometimes taking this stale in-flight pending upload data. * Because some signals were DirectConnection and some were QueuedConnection, there was a chance that a direct signal overtakes a queued signal. The state machine then sent data down the socket which was buffered there (and sent later) although it did not match the current state of the state machine when it was actually sent. * A socket was seen as being able to have requests sent even though it was not encrypted yet. This relates to the previous corruption where data is stored inside the socket's buffer and then sent later. The included auto test produces all fixed corruptions, I detected no regressions via the other tests. This code also adds a bit of sanity checking to protect from possible further problems. [ChangeLog][QtNetwork] Fix HTTP(s) upload corruption when server closes connection Change-Id: I54c883925ec897050941498f139c4b523030432e Reviewed-by: Peter Hartmann --- .../access/qhttpnetworkconnectionchannel.cpp | 35 ++++++++++++++++----- .../access/qhttpnetworkconnectionchannel_p.h | 2 ++ src/network/access/qhttpprotocolhandler.cpp | 7 +++++ src/network/access/qhttpthreaddelegate_p.h | 36 +++++++++++++++++----- src/network/access/qnetworkreplyhttpimpl.cpp | 25 ++++++++++----- src/network/access/qnetworkreplyhttpimpl_p.h | 7 +++-- 6 files changed, 88 insertions(+), 24 deletions(-) (limited to 'src/network') diff --git a/src/network/access/qhttpnetworkconnectionchannel.cpp b/src/network/access/qhttpnetworkconnectionchannel.cpp index 9f63280bf8..49c6793b1f 100644 --- a/src/network/access/qhttpnetworkconnectionchannel.cpp +++ b/src/network/access/qhttpnetworkconnectionchannel.cpp @@ -106,15 +106,19 @@ void QHttpNetworkConnectionChannel::init() socket->setProxy(QNetworkProxy::NoProxy); #endif + // We want all signals (except the interactive ones) be connected as QueuedConnection + // because else we're falling into cases where we recurse back into the socket code + // and mess up the state. Always going to the event loop (and expecting that when reading/writing) + // is safer. QObject::connect(socket, SIGNAL(bytesWritten(qint64)), this, SLOT(_q_bytesWritten(qint64)), - Qt::DirectConnection); + Qt::QueuedConnection); QObject::connect(socket, SIGNAL(connected()), this, SLOT(_q_connected()), - Qt::DirectConnection); + Qt::QueuedConnection); QObject::connect(socket, SIGNAL(readyRead()), this, SLOT(_q_readyRead()), - Qt::DirectConnection); + Qt::QueuedConnection); // The disconnected() and error() signals may already come // while calling connectToHost(). @@ -143,13 +147,13 @@ void QHttpNetworkConnectionChannel::init() // won't be a sslSocket if encrypt is false QObject::connect(sslSocket, SIGNAL(encrypted()), this, SLOT(_q_encrypted()), - Qt::DirectConnection); + Qt::QueuedConnection); QObject::connect(sslSocket, SIGNAL(sslErrors(QList)), this, SLOT(_q_sslErrors(QList)), Qt::DirectConnection); QObject::connect(sslSocket, SIGNAL(encryptedBytesWritten(qint64)), this, SLOT(_q_encryptedBytesWritten(qint64)), - Qt::DirectConnection); + Qt::QueuedConnection); if (ignoreAllSslErrors) sslSocket->ignoreSslErrors(); @@ -186,8 +190,11 @@ void QHttpNetworkConnectionChannel::close() // pendingEncrypt must only be true in between connected and encrypted states pendingEncrypt = false; - if (socket) + if (socket) { + // socket can be 0 since the host lookup is done from qhttpnetworkconnection.cpp while + // there is no socket yet. socket->close(); + } } @@ -353,6 +360,14 @@ bool QHttpNetworkConnectionChannel::ensureConnection() } return false; } + + // This code path for ConnectedState + if (pendingEncrypt) { + // Let's only be really connected when we have received the encrypted() signal. Else the state machine seems to mess up + // and corrupt the things sent to the server. + return false; + } + return true; } @@ -659,6 +674,12 @@ bool QHttpNetworkConnectionChannel::isSocketReading() const void QHttpNetworkConnectionChannel::_q_bytesWritten(qint64 bytes) { Q_UNUSED(bytes); + if (ssl) { + // In the SSL case we want to send data from encryptedBytesWritten signal since that one + // is the one going down to the actual network, not only into some SSL buffer. + return; + } + // bytes have been written to the socket. write even more of them :) if (isSocketWriting()) sendRequest(); @@ -734,7 +755,7 @@ void QHttpNetworkConnectionChannel::_q_connected() // ### FIXME: if the server closes the connection unexpectedly, we shouldn't send the same broken request again! //channels[i].reconnectAttempts = 2; - if (pendingEncrypt) { + if (ssl || pendingEncrypt) { // FIXME: Didn't work properly with pendingEncrypt only, we should refactor this into an EncrypingState #ifndef QT_NO_SSL if (connection->sslContext().isNull()) { // this socket is making the 1st handshake for this connection, diff --git a/src/network/access/qhttpnetworkconnectionchannel_p.h b/src/network/access/qhttpnetworkconnectionchannel_p.h index 692c0e6a94..231fe11135 100644 --- a/src/network/access/qhttpnetworkconnectionchannel_p.h +++ b/src/network/access/qhttpnetworkconnectionchannel_p.h @@ -83,6 +83,8 @@ typedef QPair HttpMessagePair; class QHttpNetworkConnectionChannel : public QObject { Q_OBJECT public: + // TODO: Refactor this to add an EncryptingState (and remove pendingEncrypt). + // Also add an Unconnected state so IdleState does not have double meaning. enum ChannelState { IdleState = 0, // ready to send request ConnectingState = 1, // connecting to host diff --git a/src/network/access/qhttpprotocolhandler.cpp b/src/network/access/qhttpprotocolhandler.cpp index 28e10f751e..3357948519 100644 --- a/src/network/access/qhttpprotocolhandler.cpp +++ b/src/network/access/qhttpprotocolhandler.cpp @@ -368,6 +368,13 @@ bool QHttpProtocolHandler::sendRequest() // nothing to read currently, break the loop break; } else { + if (m_channel->written != uploadByteDevice->pos()) { + // Sanity check. This was useful in tracking down an upload corruption. + qWarning() << "QHttpProtocolHandler: Internal error in sendRequest. Expected to write at position" << m_channel->written << "but read device is at" << uploadByteDevice->pos(); + Q_ASSERT(m_channel->written == uploadByteDevice->pos()); + m_connection->d_func()->emitReplyError(m_socket, m_reply, QNetworkReply::ProtocolFailure); + return false; + } qint64 currentWriteSize = m_socket->write(readPointer, currentReadSize); if (currentWriteSize == -1 || currentWriteSize != currentReadSize) { // socket broke down diff --git a/src/network/access/qhttpthreaddelegate_p.h b/src/network/access/qhttpthreaddelegate_p.h index 16610828cb..b553409391 100644 --- a/src/network/access/qhttpthreaddelegate_p.h +++ b/src/network/access/qhttpthreaddelegate_p.h @@ -187,6 +187,7 @@ protected: QByteArray m_dataArray; bool m_atEnd; qint64 m_size; + qint64 m_pos; // to match calls of haveDataSlot with the expected position public: QNonContiguousByteDeviceThreadForwardImpl(bool aE, qint64 s) : QNonContiguousByteDevice(), @@ -194,7 +195,8 @@ public: m_amount(0), m_data(0), m_atEnd(aE), - m_size(s) + m_size(s), + m_pos(0) { } @@ -202,6 +204,11 @@ public: { } + qint64 pos() Q_DECL_OVERRIDE + { + return m_pos; + } + const char* readPointer(qint64 maximumLength, qint64 &len) { if (m_amount > 0) { @@ -229,11 +236,10 @@ public: m_amount -= a; m_data += a; + m_pos += a; - // To main thread to inform about our state - emit processedData(a); - - // FIXME possible optimization, already ask user thread for some data + // To main thread to inform about our state. The m_pos will be sent as a sanity check. + emit processedData(m_pos, a); return true; } @@ -250,10 +256,21 @@ public: { m_amount = 0; m_data = 0; + m_dataArray.clear(); + + if (wantDataPending) { + // had requested the user thread to send some data (only 1 in-flight at any moment) + wantDataPending = false; + } // Communicate as BlockingQueuedConnection bool b = false; emit resetData(&b); + if (b) { + // the reset succeeded, we're at pos 0 again + m_pos = 0; + // the HTTP code will anyway abort the request if !b. + } return b; } @@ -264,8 +281,13 @@ public: public slots: // From user thread: - void haveDataSlot(QByteArray dataArray, bool dataAtEnd, qint64 dataSize) + void haveDataSlot(qint64 pos, QByteArray dataArray, bool dataAtEnd, qint64 dataSize) { + if (pos != m_pos) { + // Sometimes when re-sending a request in the qhttpnetwork* layer there is a pending haveData from the + // user thread on the way to us. We need to ignore it since it is the data for the wrong(later) chunk. + return; + } wantDataPending = false; m_dataArray = dataArray; @@ -285,7 +307,7 @@ signals: // to main thread: void wantData(qint64); - void processedData(qint64); + void processedData(qint64 pos, qint64 amount); void resetData(bool *b); }; diff --git a/src/network/access/qnetworkreplyhttpimpl.cpp b/src/network/access/qnetworkreplyhttpimpl.cpp index 4ce7303dbb..974a101e9c 100644 --- a/src/network/access/qnetworkreplyhttpimpl.cpp +++ b/src/network/access/qnetworkreplyhttpimpl.cpp @@ -424,6 +424,7 @@ QNetworkReplyHttpImplPrivate::QNetworkReplyHttpImplPrivate() , synchronous(false) , state(Idle) , statusCode(0) + , uploadByteDevicePosition(false) , uploadDeviceChoking(false) , outgoingData(0) , bytesUploaded(-1) @@ -863,9 +864,9 @@ void QNetworkReplyHttpImplPrivate::postRequest() q, SLOT(uploadByteDeviceReadyReadSlot()), Qt::QueuedConnection); - // From main thread to user thread: - QObject::connect(q, SIGNAL(haveUploadData(QByteArray,bool,qint64)), - forwardUploadDevice, SLOT(haveDataSlot(QByteArray,bool,qint64)), Qt::QueuedConnection); + // From user thread to http thread: + QObject::connect(q, SIGNAL(haveUploadData(qint64,QByteArray,bool,qint64)), + forwardUploadDevice, SLOT(haveDataSlot(qint64,QByteArray,bool,qint64)), Qt::QueuedConnection); QObject::connect(uploadByteDevice.data(), SIGNAL(readyRead()), forwardUploadDevice, SIGNAL(readyRead()), Qt::QueuedConnection); @@ -873,8 +874,8 @@ void QNetworkReplyHttpImplPrivate::postRequest() // From http thread to user thread: QObject::connect(forwardUploadDevice, SIGNAL(wantData(qint64)), q, SLOT(wantUploadDataSlot(qint64))); - QObject::connect(forwardUploadDevice, SIGNAL(processedData(qint64)), - q, SLOT(sentUploadDataSlot(qint64))); + QObject::connect(forwardUploadDevice,SIGNAL(processedData(qint64, qint64)), + q, SLOT(sentUploadDataSlot(qint64,qint64))); QObject::connect(forwardUploadDevice, SIGNAL(resetData(bool*)), q, SLOT(resetUploadDataSlot(bool*)), Qt::BlockingQueuedConnection); // this is the only one with BlockingQueued! @@ -1268,12 +1269,22 @@ void QNetworkReplyHttpImplPrivate::replySslConfigurationChanged(const QSslConfig void QNetworkReplyHttpImplPrivate::resetUploadDataSlot(bool *r) { *r = uploadByteDevice->reset(); + if (*r) { + // reset our own position which is used for the inter-thread communication + uploadByteDevicePosition = 0; + } } // Coming from QNonContiguousByteDeviceThreadForwardImpl in HTTP thread -void QNetworkReplyHttpImplPrivate::sentUploadDataSlot(qint64 amount) +void QNetworkReplyHttpImplPrivate::sentUploadDataSlot(qint64 pos, qint64 amount) { + if (uploadByteDevicePosition + amount != pos) { + // Sanity check, should not happen. + error(QNetworkReply::UnknownNetworkError, ""); + return; + } uploadByteDevice->advanceReadPointer(amount); + uploadByteDevicePosition += amount; } // Coming from QNonContiguousByteDeviceThreadForwardImpl in HTTP thread @@ -1298,7 +1309,7 @@ void QNetworkReplyHttpImplPrivate::wantUploadDataSlot(qint64 maxSize) QByteArray dataArray(data, currentUploadDataLength); // Communicate back to HTTP thread - emit q->haveUploadData(dataArray, uploadByteDevice->atEnd(), uploadByteDevice->size()); + emit q->haveUploadData(uploadByteDevicePosition, dataArray, uploadByteDevice->atEnd(), uploadByteDevice->size()); } void QNetworkReplyHttpImplPrivate::uploadByteDeviceReadyReadSlot() diff --git a/src/network/access/qnetworkreplyhttpimpl_p.h b/src/network/access/qnetworkreplyhttpimpl_p.h index 77d9c5a368..1940922f6e 100644 --- a/src/network/access/qnetworkreplyhttpimpl_p.h +++ b/src/network/access/qnetworkreplyhttpimpl_p.h @@ -120,7 +120,7 @@ public: Q_PRIVATE_SLOT(d_func(), void resetUploadDataSlot(bool *r)) Q_PRIVATE_SLOT(d_func(), void wantUploadDataSlot(qint64)) - Q_PRIVATE_SLOT(d_func(), void sentUploadDataSlot(qint64)) + Q_PRIVATE_SLOT(d_func(), void sentUploadDataSlot(qint64,qint64)) Q_PRIVATE_SLOT(d_func(), void uploadByteDeviceReadyReadSlot()) Q_PRIVATE_SLOT(d_func(), void emitReplyUploadProgress(qint64, qint64)) Q_PRIVATE_SLOT(d_func(), void _q_cacheSaveDeviceAboutToClose()) @@ -144,7 +144,7 @@ signals: void startHttpRequestSynchronously(); - void haveUploadData(QByteArray dataArray, bool dataAtEnd, qint64 dataSize); + void haveUploadData(const qint64 pos, QByteArray dataArray, bool dataAtEnd, qint64 dataSize); }; class QNetworkReplyHttpImplPrivate: public QNetworkReplyPrivate @@ -195,6 +195,7 @@ public: // upload QNonContiguousByteDevice* createUploadByteDevice(); QSharedPointer uploadByteDevice; + qint64 uploadByteDevicePosition; bool uploadDeviceChoking; // if we couldn't readPointer() any data at the moment QIODevice *outgoingData; QSharedPointer outgoingDataBuffer; @@ -283,7 +284,7 @@ public: // From QNonContiguousByteDeviceThreadForwardImpl in HTTP thread: void resetUploadDataSlot(bool *r); void wantUploadDataSlot(qint64); - void sentUploadDataSlot(qint64); + void sentUploadDataSlot(qint64, qint64); // From user's QNonContiguousByteDevice void uploadByteDeviceReadyReadSlot(); -- cgit v1.2.3 From 00fe833189e79be3f8f00d1972d1f54b7f384dde Mon Sep 17 00:00:00 2001 From: Markus Goetz Date: Wed, 6 May 2015 12:57:58 +0200 Subject: QNAM: Fix compiler warning Change-Id: I2ae6493e13c9b168c64c458e42ea90d4ec2d8628 Reviewed-by: Friedemann Kleint Reviewed-by: Olivier Goffart (Woboq GmbH) --- src/network/access/qnetworkreplyhttpimpl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/network') diff --git a/src/network/access/qnetworkreplyhttpimpl.cpp b/src/network/access/qnetworkreplyhttpimpl.cpp index 974a101e9c..a4f677efab 100644 --- a/src/network/access/qnetworkreplyhttpimpl.cpp +++ b/src/network/access/qnetworkreplyhttpimpl.cpp @@ -1280,7 +1280,7 @@ void QNetworkReplyHttpImplPrivate::sentUploadDataSlot(qint64 pos, qint64 amount) { if (uploadByteDevicePosition + amount != pos) { // Sanity check, should not happen. - error(QNetworkReply::UnknownNetworkError, ""); + error(QNetworkReply::UnknownNetworkError, QString()); return; } uploadByteDevice->advanceReadPointer(amount); -- cgit v1.2.3