mirrors/nextcloud-desktop
1
0
Fork 0
mirror of https://github.com/nextcloud/desktop.git synced 2024-11-29 12:19:03 +03:00
Code Issues Projects Releases Packages Wiki Activity
nextcloud-desktop/admin/qt/patches/0007-X-Network-Fix-up-previous-corruption-patch.patch
Daniel Molkentin 2d604cee25 Qt: Add Windows-specific patches OBS, also a QNAM fix
2015-11-25 22:36:37 +01:00

434 lines
21 KiB
Diff
Raw Blame History

From eae0cb09f1310e755c2aff7c1112f7a6c09d7a53 Mon Sep 17 00:00:00 2001
From: Markus Goetz <markus@woboq.com>
Date: Fri, 19 Jun 2015 15:35:34 +0200
Subject: [PATCH] Network: Fix up previous corruption patch
This is a fix-up for cff39fba10ffc10ee4dcfdc66ff6528eb26462d3.
That patch lead to some internal state issues that lead to the QTBUG-47048
or to QNetworkReply objects erroring with "Connection Closed" when
the server closed the Keep-Alive connection.
This patch changes the QNAM socket slot connections to be DirectConnection.
We don't close the socket anymore in slots where it is anyway in a closed state
afterwards. This prevents event/stack recursions.
We also flush QSslSocket/QTcpSocket receive buffers when receiving a disconnect
so that the developer always gets the full decrypted data from the buffers.
[ChangeLog][QtNetwork] Fix HTTP issues with "Unknown Error" and "Connection Closed"
[ChangeLog][QtNetwork][Sockets] Read OS/encrypted read buffers when connection
closed by server.
Change-Id: Ib4d6a2d0d988317e3a5356f36e8dbcee4590beed
Task-number: QTBUG-47048
Reviewed-by: Kai Koehne <kai.koehne@theqtcompany.com>
Reviewed-by: Richard J. Moore <rich@kde.org>
---
src/network/access/qhttpnetworkconnection.cpp | 1 -
.../access/qhttpnetworkconnectionchannel.cpp | 108 +++++++++++++--------
.../access/qhttpnetworkconnectionchannel_p.h | 1 +
src/network/access/qhttpnetworkreply.cpp | 2 +-
src/network/access/qhttpprotocolhandler.cpp | 1 -
src/network/socket/qabstractsocket.cpp | 7 +-
src/network/ssl/qsslsocket.cpp | 8 ++
src/network/ssl/qsslsocket_openssl.cpp | 7 ++
.../access/qnetworkreply/tst_qnetworkreply.cpp | 9 +-
9 files changed, 94 insertions(+), 50 deletions(-)
diff --git a/src/network/access/qhttpnetworkconnection.cpp b/src/network/access/qhttpnetworkconnection.cpp
index 365ce55..543c70e 100644
--- a/src/network/access/qhttpnetworkconnection.cpp
+++ b/src/network/access/qhttpnetworkconnection.cpp
@@ -917,7 +917,6 @@ void QHttpNetworkConnectionPrivate::_q_startNextRequest()
for (int i = 0; i < channelCount; ++i) {
if (channels[i].resendCurrent && (channels[i].state != QHttpNetworkConnectionChannel::ClosingState)) {
channels[i].resendCurrent = false;
- channels[i].state = QHttpNetworkConnectionChannel::IdleState;
// if this is not possible, error will be emitted and connection terminated
if (!channels[i].resetUploadData())
diff --git a/src/network/access/qhttpnetworkconnectionchannel.cpp b/src/network/access/qhttpnetworkconnectionchannel.cpp
index 49c6793..e2f6307 100644
--- a/src/network/access/qhttpnetworkconnectionchannel.cpp
+++ b/src/network/access/qhttpnetworkconnectionchannel.cpp
@@ -58,6 +58,11 @@ QT_BEGIN_NAMESPACE
// TODO: Put channel specific stuff here so it does not polute qhttpnetworkconnection.cpp
+// Because in-flight when sending a request, the server might close our connection (because the persistent HTTP
+// connection times out)
+// We use 3 because we can get a _q_error 3 times depending on the timing:
+static const int reconnectAttemptsDefault = 3;
+
QHttpNetworkConnectionChannel::QHttpNetworkConnectionChannel()
: socket(0)
, ssl(false)
@@ -69,7 +74,7 @@ QHttpNetworkConnectionChannel::QHttpNetworkConnectionChannel()
, resendCurrent(false)
, lastStatus(0)
, pendingEncrypt(false)
- , reconnectAttempts(2)
+ , reconnectAttempts(reconnectAttemptsDefault)
, authMethod(QAuthenticatorPrivate::None)
, proxyAuthMethod(QAuthenticatorPrivate::None)
, authenticationCredentialsSent(false)
@@ -106,19 +111,18 @@ 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.
+ // After some back and forth in all the last years, this is now a DirectConnection because otherwise
+ // the state inside the *Socket classes gets messed up, also in conjunction with the socket notifiers
+ // which behave slightly differently on Windows vs Linux
QObject::connect(socket, SIGNAL(bytesWritten(qint64)),
this, SLOT(_q_bytesWritten(qint64)),
- Qt::QueuedConnection);
+ Qt::DirectConnection);
QObject::connect(socket, SIGNAL(connected()),
this, SLOT(_q_connected()),
- Qt::QueuedConnection);
+ Qt::DirectConnection);
QObject::connect(socket, SIGNAL(readyRead()),
this, SLOT(_q_readyRead()),
- Qt::QueuedConnection);
+ Qt::DirectConnection);
// The disconnected() and error() signals may already come
// while calling connectToHost().
@@ -129,10 +133,10 @@ void QHttpNetworkConnectionChannel::init()
qRegisterMetaType<QAbstractSocket::SocketError>();
QObject::connect(socket, SIGNAL(disconnected()),
this, SLOT(_q_disconnected()),
- Qt::QueuedConnection);
+ Qt::DirectConnection);
QObject::connect(socket, SIGNAL(error(QAbstractSocket::SocketError)),
this, SLOT(_q_error(QAbstractSocket::SocketError)),
- Qt::QueuedConnection);
+ Qt::DirectConnection);
#ifndef QT_NO_NETWORKPROXY
@@ -147,13 +151,13 @@ void QHttpNetworkConnectionChannel::init()
// won't be a sslSocket if encrypt is false
QObject::connect(sslSocket, SIGNAL(encrypted()),
this, SLOT(_q_encrypted()),
- Qt::QueuedConnection);
+ Qt::DirectConnection);
QObject::connect(sslSocket, SIGNAL(sslErrors(QList<QSslError>)),
this, SLOT(_q_sslErrors(QList<QSslError>)),
Qt::DirectConnection);
QObject::connect(sslSocket, SIGNAL(encryptedBytesWritten(qint64)),
this, SLOT(_q_encryptedBytesWritten(qint64)),
- Qt::QueuedConnection);
+ Qt::DirectConnection);
if (ignoreAllSslErrors)
sslSocket->ignoreSslErrors();
@@ -397,7 +401,7 @@ void QHttpNetworkConnectionChannel::allDone()
// reset the reconnection attempts after we receive a complete reply.
// in case of failures, each channel will attempt two reconnects before emitting error.
- reconnectAttempts = 2;
+ reconnectAttempts = reconnectAttemptsDefault;
// now the channel can be seen as free/idle again, all signal emissions for the reply have been done
if (state != QHttpNetworkConnectionChannel::ClosingState)
@@ -651,6 +655,15 @@ void QHttpNetworkConnectionChannel::closeAndResendCurrentRequest()
QMetaObject::invokeMethod(connection, "_q_startNextRequest", Qt::QueuedConnection);
}
+void QHttpNetworkConnectionChannel::resendCurrentRequest()
+{
+ requeueCurrentlyPipelinedRequests();
+ if (reply)
+ resendCurrent = true;
+ if (qobject_cast<QHttpNetworkConnection*>(connection))
+ QMetaObject::invokeMethod(connection, "_q_startNextRequest", Qt::QueuedConnection);
+}
+
bool QHttpNetworkConnectionChannel::isSocketBusy() const
{
return (state & QHttpNetworkConnectionChannel::BusyState);
@@ -694,8 +707,8 @@ void QHttpNetworkConnectionChannel::_q_disconnected()
return;
}
- // read the available data before closing
- if (isSocketWaiting() || isSocketReading()) {
+ // read the available data before closing (also done in _q_error for other codepaths)
+ if ((isSocketWaiting() || isSocketReading()) && socket->bytesAvailable()) {
if (reply) {
state = QHttpNetworkConnectionChannel::ReadingState;
_q_receiveReply();
@@ -707,7 +720,8 @@ void QHttpNetworkConnectionChannel::_q_disconnected()
state = QHttpNetworkConnectionChannel::IdleState;
requeueCurrentlyPipelinedRequests();
- close();
+
+ pendingEncrypt = false;
}
@@ -789,11 +803,19 @@ void QHttpNetworkConnectionChannel::_q_error(QAbstractSocket::SocketError socket
errorCode = QNetworkReply::ConnectionRefusedError;
break;
case QAbstractSocket::RemoteHostClosedError:
- // try to reconnect/resend before sending an error.
- // while "Reading" the _q_disconnected() will handle this.
- if (state != QHttpNetworkConnectionChannel::IdleState && state != QHttpNetworkConnectionChannel::ReadingState) {
+ // This error for SSL comes twice in a row, first from SSL layer ("The TLS/SSL connection has been closed") then from TCP layer.
+ // Depending on timing it can also come three times in a row (first time when we try to write into a closing QSslSocket).
+ // The reconnectAttempts handling catches the cases where we can re-send the request.
+ if (!reply && state == QHttpNetworkConnectionChannel::IdleState) {
+ // Not actually an error, it is normal for Keep-Alive connections to close after some time if no request
+ // is sent on them. No need to error the other replies below. Just bail out here.
+ // The _q_disconnected will handle the possibly pipelined replies
+ return;
+ } else if (state != QHttpNetworkConnectionChannel::IdleState && state != QHttpNetworkConnectionChannel::ReadingState) {
+ // Try to reconnect/resend before sending an error.
+ // While "Reading" the _q_disconnected() will handle this.
if (reconnectAttempts-- > 0) {
- closeAndResendCurrentRequest();
+ resendCurrentRequest();
return;
} else {
errorCode = QNetworkReply::RemoteHostClosedError;
@@ -818,24 +840,15 @@ void QHttpNetworkConnectionChannel::_q_error(QAbstractSocket::SocketError socket
// we can ignore the readbuffersize as the data is already
// in memory and we will not receive more data on the socket.
reply->setReadBufferSize(0);
+ reply->setDownstreamLimited(false);
_q_receiveReply();
-#ifndef QT_NO_SSL
- if (ssl) {
- // QT_NO_OPENSSL. The QSslSocket can still have encrypted bytes in the plainsocket.
- // So we need to check this if the socket is a QSslSocket. When the socket is flushed
- // it will force a decrypt of the encrypted data in the plainsocket.
- QSslSocket *sslSocket = static_cast<QSslSocket*>(socket);
- qint64 beforeFlush = sslSocket->encryptedBytesAvailable();
- while (sslSocket->encryptedBytesAvailable()) {
- sslSocket->flush();
- _q_receiveReply();
- qint64 afterFlush = sslSocket->encryptedBytesAvailable();
- if (afterFlush == beforeFlush)
- break;
- beforeFlush = afterFlush;
- }
+ if (!reply) {
+ // No more reply assigned after the previous call? Then it had been finished successfully.
+ requeueCurrentlyPipelinedRequests();
+ state = QHttpNetworkConnectionChannel::IdleState;
+ QMetaObject::invokeMethod(connection, "_q_startNextRequest", Qt::QueuedConnection);
+ return;
}
-#endif
}
errorCode = QNetworkReply::RemoteHostClosedError;
@@ -846,7 +859,7 @@ void QHttpNetworkConnectionChannel::_q_error(QAbstractSocket::SocketError socket
case QAbstractSocket::SocketTimeoutError:
// try to reconnect/resend before sending an error.
if (state == QHttpNetworkConnectionChannel::WritingState && (reconnectAttempts-- > 0)) {
- closeAndResendCurrentRequest();
+ resendCurrentRequest();
return;
}
errorCode = QNetworkReply::TimeoutError;
@@ -860,7 +873,7 @@ void QHttpNetworkConnectionChannel::_q_error(QAbstractSocket::SocketError socket
case QAbstractSocket::ProxyConnectionClosedError:
// try to reconnect/resend before sending an error.
if (reconnectAttempts-- > 0) {
- closeAndResendCurrentRequest();
+ resendCurrentRequest();
return;
}
errorCode = QNetworkReply::ProxyConnectionClosedError;
@@ -868,7 +881,7 @@ void QHttpNetworkConnectionChannel::_q_error(QAbstractSocket::SocketError socket
case QAbstractSocket::ProxyConnectionTimeoutError:
// try to reconnect/resend before sending an error.
if (reconnectAttempts-- > 0) {
- closeAndResendCurrentRequest();
+ resendCurrentRequest();
return;
}
errorCode = QNetworkReply::ProxyTimeoutError;
@@ -916,8 +929,18 @@ void QHttpNetworkConnectionChannel::_q_error(QAbstractSocket::SocketError socket
// send the next request
QMetaObject::invokeMethod(that, "_q_startNextRequest", Qt::QueuedConnection);
- if (that) //signal emission triggered event loop
- close();
+ if (that) {
+ //signal emission triggered event loop
+ if (!socket)
+ state = QHttpNetworkConnectionChannel::IdleState;
+ else if (socket->state() == QAbstractSocket::UnconnectedState)
+ state = QHttpNetworkConnectionChannel::IdleState;
+ else
+ state = QHttpNetworkConnectionChannel::ClosingState;
+
+ // pendingEncrypt must only be true in between connected and encrypted states
+ pendingEncrypt = false;
+ }
}
#ifndef QT_NO_NETWORKPROXY
@@ -941,7 +964,8 @@ void QHttpNetworkConnectionChannel::_q_proxyAuthenticationRequired(const QNetwor
void QHttpNetworkConnectionChannel::_q_uploadDataReadyRead()
{
- sendRequest();
+ if (reply)
+ sendRequest();
}
#ifndef QT_NO_SSL
diff --git a/src/network/access/qhttpnetworkconnectionchannel_p.h b/src/network/access/qhttpnetworkconnectionchannel_p.h
index 231fe11..a834b7d 100644
--- a/src/network/access/qhttpnetworkconnectionchannel_p.h
+++ b/src/network/access/qhttpnetworkconnectionchannel_p.h
@@ -169,6 +169,7 @@ public:
void handleUnexpectedEOF();
void closeAndResendCurrentRequest();
+ void resendCurrentRequest();
bool isSocketBusy() const;
bool isSocketWriting() const;
diff --git a/src/network/access/qhttpnetworkreply.cpp b/src/network/access/qhttpnetworkreply.cpp
index 55863a3..8b71bd8 100644
--- a/src/network/access/qhttpnetworkreply.cpp
+++ b/src/network/access/qhttpnetworkreply.cpp
@@ -191,7 +191,7 @@ QByteArray QHttpNetworkReply::readAny()
return QByteArray();
// we'll take the last buffer, so schedule another read from http
- if (d->downstreamLimited && d->responseData.bufferCount() == 1)
+ if (d->downstreamLimited && d->responseData.bufferCount() == 1 && !isFinished())
d->connection->d_func()->readMoreLater(this);
return d->responseData.read();
}
diff --git a/src/network/access/qhttpprotocolhandler.cpp b/src/network/access/qhttpprotocolhandler.cpp
index 3357948..380aaac 100644
--- a/src/network/access/qhttpprotocolhandler.cpp
+++ b/src/network/access/qhttpprotocolhandler.cpp
@@ -250,7 +250,6 @@ bool QHttpProtocolHandler::sendRequest()
if (!m_reply) {
// heh, how should that happen!
qWarning() << "QAbstractProtocolHandler::sendRequest() called without QHttpNetworkReply";
- m_channel->state = QHttpNetworkConnectionChannel::IdleState;
return false;
}
diff --git a/src/network/socket/qabstractsocket.cpp b/src/network/socket/qabstractsocket.cpp
index 2666771..0e82d4a 100644
--- a/src/network/socket/qabstractsocket.cpp
+++ b/src/network/socket/qabstractsocket.cpp
@@ -768,6 +768,7 @@ bool QAbstractSocketPrivate::canReadNotification()
void QAbstractSocketPrivate::canCloseNotification()
{
Q_Q(QAbstractSocket);
+ // Note that this method is only called on Windows. Other platforms close in the canReadNotification()
#if defined (QABSTRACTSOCKET_DEBUG)
qDebug("QAbstractSocketPrivate::canCloseNotification()");
@@ -777,7 +778,11 @@ void QAbstractSocketPrivate::canCloseNotification()
if (isBuffered) {
// Try to read to the buffer, if the read fail we can close the socket.
newBytes = buffer.size();
- if (!readFromSocket()) {
+ qint64 oldReadBufferMaxSize = readBufferMaxSize;
+ readBufferMaxSize = 0; // temporarily disable max read buffer, we want to empty the OS buffer
+ bool hadReadFromSocket = readFromSocket();
+ readBufferMaxSize = oldReadBufferMaxSize;
+ if (!hadReadFromSocket) {
q->disconnectFromHost();
return;
}
diff --git a/src/network/ssl/qsslsocket.cpp b/src/network/ssl/qsslsocket.cpp
index c1fab94..2b9e923 100644
--- a/src/network/ssl/qsslsocket.cpp
+++ b/src/network/ssl/qsslsocket.cpp
@@ -2294,6 +2294,14 @@ void QSslSocketPrivate::_q_errorSlot(QAbstractSocket::SocketError error)
qCDebug(lcSsl) << "\tstate =" << q->state();
qCDebug(lcSsl) << "\terrorString =" << q->errorString();
#endif
+ // this moves encrypted bytes from plain socket into our buffer
+ if (plainSocket->bytesAvailable()) {
+ qint64 tmpReadBufferMaxSize = readBufferMaxSize;
+ readBufferMaxSize = 0; // reset temporarily so the plain sockets completely drained drained
+ transmit();
+ readBufferMaxSize = tmpReadBufferMaxSize;
+ }
+
q->setSocketError(plainSocket->error());
q->setErrorString(plainSocket->errorString());
emit q->error(error);
diff --git a/src/network/ssl/qsslsocket_openssl.cpp b/src/network/ssl/qsslsocket_openssl.cpp
index ac4336a..94655fe 100644
--- a/src/network/ssl/qsslsocket_openssl.cpp
+++ b/src/network/ssl/qsslsocket_openssl.cpp
@@ -1419,6 +1419,13 @@ void QSslSocketBackendPrivate::disconnected()
{
if (plainSocket->bytesAvailable() <= 0)
destroySslContext();
+ else {
+ // Move all bytes into the plain buffer
+ qint64 tmpReadBufferMaxSize = readBufferMaxSize;
+ readBufferMaxSize = 0; // reset temporarily so the plain socket buffer is completely drained
+ transmit();
+ readBufferMaxSize = tmpReadBufferMaxSize;
+ }
//if there is still buffered data in the plain socket, don't destroy the ssl context yet.
//it will be destroyed when the socket is deleted.
}
diff --git a/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp b/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp
index d2edf67..138f528 100644
--- a/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp
+++ b/tests/auto/network/access/qnetworkreply/tst_qnetworkreply.cpp
@@ -1051,7 +1051,7 @@ protected:
// clean up QAbstractSocket's residue:
while (client->bytesToWrite() > 0) {
qDebug() << "Still having" << client->bytesToWrite() << "bytes to write, doing that now";
- if (!client->waitForBytesWritten(2000)) {
+ if (!client->waitForBytesWritten(10000)) {
qDebug() << "ERROR: FastSender:" << client->error() << "cleaning up residue";
return;
}
@@ -1071,7 +1071,7 @@ protected:
measuredSentBytes += writeNextData(client, bytesToWrite);
while (client->bytesToWrite() > 0) {
- if (!client->waitForBytesWritten(2000)) {
+ if (!client->waitForBytesWritten(10000)) {
qDebug() << "ERROR: FastSender:" << client->error() << "during blocking write";
return;
}
@@ -7946,7 +7946,7 @@ public slots:
m_receivedData += data;
if (!m_parsedHeaders && m_receivedData.contains("\r\n\r\n")) {
m_parsedHeaders = true;
- QTimer::singleShot(qrand()%10, this, SLOT(closeDelayed())); // simulate random network latency
+ QTimer::singleShot(qrand()%60, this, SLOT(closeDelayed())); // simulate random network latency
// This server simulates a web server connection closing, e.g. because of Apaches MaxKeepAliveRequests or KeepAliveTimeout
// In this case QNAM needs to re-send the upload data but it had a bug which then corrupts the upload
// This test catches that.
@@ -8052,11 +8052,12 @@ void tst_QNetworkReply::putWithServerClosingConnectionImmediately()
// get the request started and the incoming socket connected
QTestEventLoop::instance().enterLoop(10);
+ QVERIFY(!QTestEventLoop::instance().timeout());
//qDebug() << "correct=" << server.m_correctUploads << "corrupt=" << server.m_corruptUploads << "expected=" <<numUploads;
// Sanity check because ecause of 9c2ecf89 most replies will error out but we want to make sure at least some of them worked
- QVERIFY(server.m_correctUploads > 5);
+ QVERIFY(server.m_correctUploads > 2);
// Because actually important is that we don't get any corruption:
QCOMPARE(server.m_corruptUploads, 0);
--
1.9.1
Reference in a new issue View git blame Copy permalink