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

(cherry picked from commit qtbase/cff39fba10ffc10ee4dcfdc66ff6528eb26462d3)
Change-Id: I9793297be6cf3edfb75b65ba03b65f7a133ef194
Reviewed-by: Richard J. Moore <rich@kde.org>

1 files changed, 169 insertions, 5 deletions

diff --git a/tests/auto/qnetworkreply/tst_qnetworkreply.cpp b/tests/auto/qnetworkreply/tst_qnetworkreply.cpp
index 1362d0172a..6c4436f633 100644
--- a/tests/auto/qnetworkreply/tst_qnetworkreply.cpp
+++ b/tests/auto/qnetworkreply/tst_qnetworkreply.cpp
@@ -394,6 +394,10 @@ private Q_SLOTS:
     void closeDuringDownload_data();
     void closeDuringDownload();
 
+#ifndef QT_NO_SSL
+    void putWithServerClosingConnectionImmediately();
+#endif
+
     // NOTE: This test must be last!
     void parentingRepliesToTheApp();
 };
@@ -4411,12 +4415,16 @@ void tst_QNetworkReply::ioPostToHttpNoBufferFlag()
 class SslServer : public QTcpServer {
     Q_OBJECT
 public:
-    SslServer() : socket(0) {};
+    SslServer() : socket(0), m_ssl(true) {};
     void incomingConnection(int socketDescriptor) {
         QSslSocket *serverSocket = new QSslSocket;
         serverSocket->setParent(this);
 
         if (serverSocket->setSocketDescriptor(socketDescriptor)) {
+            if (!m_ssl) {
+                emit newPlainConnection(serverSocket);
+                return;
+            }
             connect(serverSocket, SIGNAL(encrypted()), this, SLOT(encryptedSlot()));
             serverSocket->setProtocol(QSsl::AnyProtocol);
             connect(serverSocket, SIGNAL(sslErrors(const QList<QSslError>&)), serverSocket, SLOT(ignoreSslErrors()));
@@ -4428,14 +4436,16 @@ public:
         }
     }
 signals:
-    void newEncryptedConnection();
+    void newEncryptedConnection(QSslSocket *s);
+    void newPlainConnection(QSslSocket *s);
 public slots:
     void encryptedSlot() {
         socket = (QSslSocket*) sender();
-        emit newEncryptedConnection();
+        emit newEncryptedConnection(socket);
     }
 public:
     QSslSocket *socket;
+    bool m_ssl;
 };
 
 // very similar to ioPostToHttpUploadProgress but for SSL
@@ -4454,7 +4464,7 @@ void tst_QNetworkReply::ioPostToHttpsUploadProgress()
     request.setRawHeader("Content-Type", "application/octet-stream");
     QNetworkReplyPtr reply = manager.post(request, &sourceFile);
     QSignalSpy spy(reply, SIGNAL(uploadProgress(qint64,qint64)));
-    connect(&server, SIGNAL(newEncryptedConnection()), &QTestEventLoop::instance(), SLOT(exitLoop()));
+    connect(&server, SIGNAL(newEncryptedConnection(QSslSocket*)), &QTestEventLoop::instance(), SLOT(exitLoop()));
     connect(reply, SIGNAL(sslErrors(const QList<QSslError>&)), reply, SLOT(ignoreSslErrors()));
 
     // get the request started and the incoming socket connected
@@ -4462,7 +4472,7 @@ void tst_QNetworkReply::ioPostToHttpsUploadProgress()
     QVERIFY(!QTestEventLoop::instance().timeout());
     QTcpSocket *incomingSocket = server.socket;
     QVERIFY(incomingSocket);
-    disconnect(&server, SIGNAL(newEncryptedConnection()), &QTestEventLoop::instance(), SLOT(exitLoop()));
+    disconnect(&server, SIGNAL(newEncryptedConnection(QSslSocket*)), &QTestEventLoop::instance(), SLOT(exitLoop()));
 
 
     incomingSocket->setReadBufferSize(1*1024);
@@ -6917,6 +6927,160 @@ void tst_QNetworkReply::closeDuringDownload()
     QTest::qWait(1000); //cancelling ftp takes some time, this avoids a warning caused by test's cleanup() destroying the connection cache before the abort is finished
 }
 
+#ifndef QT_NO_SSL
+
+class PutWithServerClosingConnectionImmediatelyHandler: public QObject
+{
+    Q_OBJECT
+public:
+    bool m_parsedHeaders;
+    QByteArray m_receivedData;
+    QByteArray m_expectedData;
+    QSslSocket *m_socket;
+    PutWithServerClosingConnectionImmediatelyHandler(QSslSocket *s, QByteArray expected) :m_parsedHeaders(false),  m_expectedData(expected), m_socket(s)
+    {
+        m_socket->setParent(this);
+        connect(m_socket, SIGNAL(readyRead()), SLOT(readyReadSlot()));
+        connect(m_socket, SIGNAL(disconnected()), SLOT(disconnectedSlot()));
+    }
+signals:
+    void correctFileUploadReceived();
+    void corruptFileUploadReceived();
+
+public slots:
+    void closeDelayed() {
+        m_socket->close();
+    }
+
+    void readyReadSlot()
+    {
+        QByteArray data = m_socket->readAll();
+        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
+            // 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.
+        }
+
+    }
+    void disconnectedSlot()
+    {
+        if (m_parsedHeaders) {
+            //qDebug() << m_receivedData.left(m_receivedData.indexOf("\r\n\r\n"));
+            m_receivedData = m_receivedData.mid(m_receivedData.indexOf("\r\n\r\n")+4); // check only actual data
+        }
+        if (m_receivedData.length() > 0 && !m_expectedData.startsWith(m_receivedData)) {
+            // We had received some data but it is corrupt!
+            qDebug() << "CORRUPT" << m_receivedData.count();
+
+            // Use this to track down the pattern of the corruption and conclude the source
+            QFile a("/tmp/corrupt");
+            a.open(QIODevice::WriteOnly);
+            a.write(m_receivedData);
+            a.close();
+
+            QFile b("/tmp/correct");
+            b.open(QIODevice::WriteOnly);
+            b.write(m_expectedData);
+            b.close();
+            exit(1);
+            emit corruptFileUploadReceived();
+        } else {
+            emit correctFileUploadReceived();
+        }
+    }
+};
+
+class PutWithServerClosingConnectionImmediatelyServer: public SslServer
+{
+    Q_OBJECT
+public:
+    int m_correctUploads;
+    int m_corruptUploads;
+    int m_repliesFinished;
+    int m_expectedReplies;
+    QByteArray m_expectedData;
+    PutWithServerClosingConnectionImmediatelyServer() : SslServer(), m_correctUploads(0), m_corruptUploads(0), m_repliesFinished(0), m_expectedReplies(0)
+    {
+        QObject::connect(this, SIGNAL(newEncryptedConnection(QSslSocket*)), this, SLOT(createHandlerForConnection(QSslSocket*)));
+        QObject::connect(this, SIGNAL(newPlainConnection(QSslSocket*)), this, SLOT(createHandlerForConnection(QSslSocket*)));
+    }
+
+public slots:
+    void createHandlerForConnection(QSslSocket* s) {
+        PutWithServerClosingConnectionImmediatelyHandler *handler = new PutWithServerClosingConnectionImmediatelyHandler(s, m_expectedData);
+        handler->setParent(this);
+        QObject::connect(handler, SIGNAL(correctFileUploadReceived()), this, SLOT(increaseCorrect()));
+        QObject::connect(handler, SIGNAL(corruptFileUploadReceived()), this, SLOT(increaseCorrupt()));
+    }
+    void increaseCorrect() {
+        m_correctUploads++;
+    }
+    void increaseCorrupt() {
+        m_corruptUploads++;
+    }
+    void replyFinished() {
+        m_repliesFinished++;
+        if (m_repliesFinished == m_expectedReplies) {
+            QTestEventLoop::instance().exitLoop();
+        }
+     }
+};
+
+
+
+void tst_QNetworkReply::putWithServerClosingConnectionImmediately()
+{
+    const int numUploads = 40;
+    qint64 wantedSize = 512*1024; // 512 kB
+    QByteArray sourceFile;
+    for (int i = 0; i < wantedSize; ++i) {
+        sourceFile += (char)'a' +(i%26);
+    }
+    bool withSsl = false;
+
+    for (int s = 0; s <= 1; s++) {
+        withSsl = (s == 1);
+        // Test also needs to run several times because of 9c2ecf89
+        for (int j = 0; j < 20; j++) {
+            // emulate a minimal https server
+            PutWithServerClosingConnectionImmediatelyServer server;
+            server.m_ssl = withSsl;
+            server.m_expectedData = sourceFile;
+            server.m_expectedReplies = numUploads;
+            server.listen(QHostAddress(QHostAddress::LocalHost), 0);
+
+            for (int i = 0; i < numUploads; i++) {
+                // create the request
+                QUrl url = QUrl(QString("http%1://127.0.0.1:%2/file=%3").arg(withSsl ? "s" : "").arg(server.serverPort()).arg(i));
+                QNetworkRequest request(url);
+                QNetworkReply *reply = manager.put(request, sourceFile);
+                connect(reply, SIGNAL(sslErrors(QList<QSslError>)), reply, SLOT(ignoreSslErrors()));
+                connect(reply, SIGNAL(finished()), &server, SLOT(replyFinished()));
+                reply->setParent(&server);
+            }
+
+            // get the request started and the incoming socket connected
+            QTestEventLoop::instance().enterLoop(10);
+
+            //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);
+            // Because actually important is that we don't get any corruption:
+            QCOMPARE(server.m_corruptUploads, 0);
+
+            server.close();
+        }
+    }
+
+
+}
+
+#endif
+
 // NOTE: This test must be last testcase in tst_qnetworkreply!
 void tst_QNetworkReply::parentingRepliesToTheApp()
 {