From 2e7a82452673888d9beb3a8ef538acc2caf13a47 Mon Sep 17 00:00:00 2001 From: Mikhail Svetkin Date: Wed, 3 Oct 2018 10:14:42 +0200 Subject: [PATCH] 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 --- src/httpserver/qhttpserverresponder.cpp | 54 ++++++++----------- tests/auto/qhttpserverresponder/index.html | 2 + .../qhttpserverresponder.pro | 2 + .../tst_qhttpserverresponder.cpp | 52 ++++++++++++++++++ 4 files changed, 79 insertions(+), 31 deletions(-) create mode 100644 tests/auto/qhttpserverresponder/index.html 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 source; + QPointer source; const QPointer 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 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 @@ + + 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 #include +#include +#include #include #include #include @@ -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("iodevice"); + QTest::addColumn("code"); + QTest::addColumn("type"); + QTest::addColumn("data"); + + QTest::addRow("index.html") + << qobject_cast(new QFile(QFINDTESTDATA("index.html"), this)) + << 200 + << "text/html" + << "\n\n"; + + QTest::addRow("index1.html - not found") + << qobject_cast(new QFile("./index1.html", this)) + << 500 + << "application/x-empty" + << ""; + + QTest::addRow("temporary file") + << qobject_cast(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)