Refactor QHttpServerResponder::write(QIODevice, ...)

The current implementation has several problems.

1. The function takes an ownership the QIODevice and puts it into a smartpointer.
Also we conntected socket's destroyed signal to lambda which has captured the
smartpointer.
So if responder does not find the file, the smartpointer will
call deleteLater which then will call socket::destroyed and then lambda
will be called and try to access the smartpointer which does not exist
anymore.

2. The function takes an ownership the file(QIODevice)
and puts it into a smartpointer.
Also we conntected the QTemporaryFile's aboutToClose signal to lambda which has captured the
smartpointer.
So when the QTemporaryFile calls destructor it will emit aboutToClose
signal which will call the lambda and this lambda will try to access the smartpointer
which does not exist anymore.

3. If we send a file smaller than chunksize,
IOChunkedTransfer we will never be deleted.

4. Does not check a socket connection type (keep-alive).

5. Does not send anything if file is not found or opened in wrong
mode.

Change-Id: I699e7d5a462c4b8d195908747bf0386132b19973
Reviewed-by: Jesus Fernandez <Jesus.Fernandez@qt.io>

4 files changed, 79 insertions, 31 deletions

diff --git a/src/httpserver/qhttpserverresponder.cpp b/src/httpserver/qhttpserverresponder.cpp
index 1a0d8d3..e5ea834 100644
--- a/src/httpserver/qhttpserverresponder.cpp
+++ b/src/httpserver/qhttpserverresponder.cpp
@@ -145,7 +145,7 @@ struct IOChunkedTransfer
     char buffer[BUFFERSIZE];
     qint64 beginIndex = -1;
     qint64 endIndex = -1;
-    QScopedPointer<QIODevice, QScopedPointerDeleteLater> source;
+    QPointer<QIODevice> source;
     const QPointer<QIODevice> sink;
     const QMetaObject::Connection bytesWrittenConnection;
     const QMetaObject::Connection readyReadConnection;
@@ -155,11 +155,15 @@ struct IOChunkedTransfer
         bytesWrittenConnection(QObject::connect(sink, &QIODevice::bytesWritten, [this] () {
               writeToOutput();
         })),
-        readyReadConnection(QObject::connect(source.get(), &QIODevice::readyRead, [this] () {
+        readyReadConnection(QObject::connect(source, &QIODevice::readyRead, [this] () {
             readFromInput();
         }))
     {
         Q_ASSERT(!source->atEnd());  // TODO error out
+        QObject::connect(sink, &QObject::destroyed, source, &QObject::deleteLater);
+        QObject::connect(source, &QObject::destroyed, [this] () {
+            delete this;
+        });
         readFromInput();
     }
 
@@ -184,7 +188,6 @@ struct IOChunkedTransfer
         if (endIndex < 0) {
             endIndex = beginIndex; // Mark the buffer as empty
             qCWarning(lc, "Error reading chunk: %s", qPrintable(source->errorString()));
-            return;
         } else if (endIndex) {
             memset(buffer + endIndex, 0, sizeof(buffer) - std::size_t(endIndex));
             writeToOutput();
@@ -204,9 +207,9 @@ struct IOChunkedTransfer
         beginIndex += writtenBytes;
         if (isBufferEmpty()) {
             if (source->bytesAvailable())
-                QTimer::singleShot(0, source.get(), [this]() { readFromInput(); });
+                QTimer::singleShot(0, source, [this]() { readFromInput(); });
             else if (source->atEnd()) // Finishing
-                source.reset();
+                source->deleteLater();
         }
     }
 };
@@ -253,33 +256,24 @@ void QHttpServerResponder::write(QIODevice *data,
     Q_D(QHttpServerResponder);
     Q_ASSERT(d->socket);
     QScopedPointer<QIODevice, QScopedPointerDeleteLater> input(data);
-    auto socket = d->socket;
-    QObject::connect(input.get(), &QIODevice::aboutToClose, [&input](){ input.reset(); });
-    // TODO protect keep alive sockets
-    QObject::connect(input.get(), &QObject::destroyed, socket, &QObject::deleteLater);
-    QObject::connect(socket, &QObject::destroyed, [&input](){ input.reset(); });
 
     input->setParent(nullptr);
-    auto openMode = input->openMode();
-    if (!(openMode & QIODevice::ReadOnly)) {
-        if (openMode == QIODevice::NotOpen) {
-            if (!input->open(QIODevice::ReadOnly)) {
-                // TODO Add developer error handling
-                // TODO Send 500
-                qCDebug(lc, "500: Could not open device %s", qPrintable(input->errorString()));
-                return;
-            }
-        } else {
-            // TODO Handle that and send 500, the device is opened but not for reading.
-            // That doesn't make sense
-            qCDebug(lc) << "500: Device is opened in a wrong mode" << openMode
-                        << qPrintable(input->errorString());
+    if (!input->isOpen()) {
+        if (!input->open(QIODevice::ReadOnly)) {
+            // TODO Add developer error handling
+            qCDebug(lc, "500: Could not open device %s", qPrintable(input->errorString()));
+            write(StatusCode::InternalServerError);
             return;
         }
+    } else if (!(input->openMode() & QIODevice::ReadOnly)) {
+        // TODO Add developer error handling
+        qCDebug(lc) << "500: Device is opened in a wrong mode" << input->openMode();
+        write(StatusCode::InternalServerError);
+        return;
     }
-    if (!socket->isOpen()) {
+
+    if (!d->socket->isOpen()) {
         qCWarning(lc, "Cannot write to socket. It's disconnected");
-        delete socket;
         return;
     }
 
@@ -291,17 +285,15 @@ void QHttpServerResponder::write(QIODevice *data,
     d->addHeader(contentTypeString, mimeType);
 
     d->writeHeaders();
-    socket->write("\r\n");
+    d->socket->write("\r\n");
 
     if (input->atEnd()) {
         qCDebug(lc, "No more data available.");
         return;
     }
 
-    auto transfer = new IOChunkedTransfer<>(input.take(), socket);
-    QObject::connect(transfer->source.get(), &QObject::destroyed, [transfer]() {
-        delete transfer;
-    });
+    // input takes ownership of the IOChunkedTransfer pointer inside his constructor
+    new IOChunkedTransfer<>(input.take(), d->socket);
 }
 
 /*!
diff --git a/tests/auto/qhttpserverresponder/index.html b/tests/auto/qhttpserverresponder/index.html
new file mode 100644
index 0000000..90531a4
--- /dev/null
+++ b/tests/auto/qhttpserverresponder/index.html
@@ -0,0 +1,2 @@
+<html>
+</html>
diff --git a/tests/auto/qhttpserverresponder/qhttpserverresponder.pro b/tests/auto/qhttpserverresponder/qhttpserverresponder.pro
index 0459f40..76ca847 100644
--- a/tests/auto/qhttpserverresponder/qhttpserverresponder.pro
+++ b/tests/auto/qhttpserverresponder/qhttpserverresponder.pro
@@ -3,3 +3,5 @@ TARGET = tst_qhttpserverresponder
 SOURCES  += tst_qhttpserverresponder.cpp
 
 QT = httpserver testlib
+
+TESTDATA += *.html
diff --git a/tests/auto/qhttpserverresponder/tst_qhttpserverresponder.cpp b/tests/auto/qhttpserverresponder/tst_qhttpserverresponder.cpp
index 7a22b88..299b074 100644
--- a/tests/auto/qhttpserverresponder/tst_qhttpserverresponder.cpp
+++ b/tests/auto/qhttpserverresponder/tst_qhttpserverresponder.cpp
@@ -39,6 +39,8 @@
 #include <QtHttpServer/qabstracthttpserver.h>
 
 #include <QtCore/qjsondocument.h>
+#include <QtCore/qfile.h>
+#include <QtCore/qtemporaryfile.h>
 #include <QtTest/qsignalspy.h>
 #include <QtTest/qtest.h>
 #include <QtNetwork/qnetworkaccessmanager.h>
@@ -63,6 +65,8 @@ private slots:
     void writeStatusCode_data();
     void writeStatusCode();
     void writeJson();
+    void writeFile_data();
+    void writeFile();
 };
 
 #define qWaitForFinished(REPLY) QVERIFY(QSignalSpy(REPLY, &QNetworkReply::finished).wait())
@@ -152,6 +156,54 @@ void tst_QHttpServerResponder::writeJson()
     QCOMPARE(QJsonDocument::fromJson(reply->readAll()), json);
 }
 
+void tst_QHttpServerResponder::writeFile_data()
+{
+    QTest::addColumn<QIODevice *>("iodevice");
+    QTest::addColumn<int>("code");
+    QTest::addColumn<QString>("type");
+    QTest::addColumn<QString>("data");
+
+    QTest::addRow("index.html")
+        << qobject_cast<QIODevice *>(new QFile(QFINDTESTDATA("index.html"), this))
+        << 200
+        << "text/html"
+        << "<html>\n</html>\n";
+
+    QTest::addRow("index1.html - not found")
+        << qobject_cast<QIODevice *>(new QFile("./index1.html", this))
+        << 500
+        << "application/x-empty"
+        << "";
+
+    QTest::addRow("temporary file")
+        << qobject_cast<QIODevice *>(new QTemporaryFile(this))
+        << 200
+        << "text/html"
+        << "";
+}
+
+void tst_QHttpServerResponder::writeFile()
+{
+    QFETCH(QIODevice *, iodevice);
+    QFETCH(int, code);
+    QFETCH(QString, type);
+    QFETCH(QString, data);
+
+    QSignalSpy spyDestroyIoDevice(iodevice, &QObject::destroyed);
+
+    HttpServer server([&iodevice](QHttpServerResponder responder) {
+        responder.write(iodevice, "text/html");
+    });
+    auto reply = networkAccessManager->get(QNetworkRequest(server.url));
+    QTRY_VERIFY(reply->isFinished());
+
+    QCOMPARE(reply->header(QNetworkRequest::ContentTypeHeader), type);
+    QCOMPARE(reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(), code);
+    QCOMPARE(reply->readAll(), data);
+
+    QCOMPARE(spyDestroyIoDevice.count(), 1);
+}
+
 QT_END_NAMESPACE
 
 QTEST_MAIN(tst_QHttpServerResponder)