From ef98fb4587a31d53e03f36bae0f74e058d9ba944 Mon Sep 17 00:00:00 2001 From: Aleix Pol Date: Wed, 22 Jun 2016 17:49:45 +0200 Subject: [PATCH] Fix file transfer under SSL Introduces a big fat buffer :( Actually test the trasfers :) Takes QSslSocket causistic into account, for some reason QNAM refuses to mark as finished when the QSslSocket (through QIODevice) closes. It would be good to look into dropping the QBuffer, doing so with the test in place will help. --- core/backends/lan/downloadjob.cpp | 27 ++++++++++----- core/backends/lan/downloadjob.h | 2 ++ core/backends/lan/uploadjob.cpp | 40 +++++++++++++++++----- core/backends/lan/uploadjob.h | 8 +++-- core/filetransferjob.cpp | 2 +- tests/sendfiletest.cpp | 56 +++++++++++++++++++++++++++++++ 6 files changed, 115 insertions(+), 20 deletions(-) diff --git a/core/backends/lan/downloadjob.cpp b/core/backends/lan/downloadjob.cpp index 269e7bc01..3ddc08a16 100644 --- a/core/backends/lan/downloadjob.cpp +++ b/core/backends/lan/downloadjob.cpp @@ -37,9 +37,13 @@ DownloadJob::DownloadJob(const QHostAddress &address, const QVariantMap &transfe : KJob() , mAddress(address) , mPort(transferInfo["port"].toInt()) - , mSocket(new QSslSocket(this)) + , mSocket(new QSslSocket) + , mBuffer(new QBuffer) { LanLinkProvider::configureSslSocket(mSocket.data(), transferInfo.value("deviceId").toString(), true); + + connect(mSocket.data(), SIGNAL(error(QAbstractSocket::SocketError)), this, SLOT(socketFailed(QAbstractSocket::SocketError))); +// connect(mSocket.data(), &QAbstractSocket::stateChanged, [](QAbstractSocket::SocketState state){ qDebug() << "statechange" << state; }); } DownloadJob::~DownloadJob() @@ -50,23 +54,28 @@ DownloadJob::~DownloadJob() void DownloadJob::start() { //TODO: Timeout? - connect(mSocket.data(), &QAbstractSocket::disconnected, this, &DownloadJob::emitResult); - connect(mSocket.data(), SIGNAL(error(QAbstractSocket::SocketError)), this, SLOT(socketFailed(QAbstractSocket::SocketError))); -// connect(mSocket.data(), &QAbstractSocket::stateChanged, [](QAbstractSocket::SocketState state){ qDebug() << "statechange" << state; }); - // Cannot use read only, might be due to ssl handshake, getting QIODevice::ReadOnly error and no connection mSocket->connectToHostEncrypted(mAddress.toString(), mPort, QIODevice::ReadWrite); + + bool b = mBuffer->open(QBuffer::ReadWrite); + Q_ASSERT(b); } void DownloadJob::socketFailed(QAbstractSocket::SocketError error) { - qWarning(KDECONNECT_CORE) << "error..." << mSocket->errorString(); - setError(error + 1); - setErrorText(mSocket->errorString()); + if (error != QAbstractSocket::RemoteHostClosedError) { //remote host closes when finishes + qWarning(KDECONNECT_CORE) << "error..." << mSocket->errorString(); + setError(error + 1); + setErrorText(mSocket->errorString()); + } else { + auto ba = mSocket->readAll(); + mBuffer->write(ba); + mBuffer->seek(0); + } emitResult(); } QSharedPointer DownloadJob::getPayload() { - return mSocket.staticCast(); + return mBuffer.staticCast(); } diff --git a/core/backends/lan/downloadjob.h b/core/backends/lan/downloadjob.h index 6100e4176..76a74b72a 100644 --- a/core/backends/lan/downloadjob.h +++ b/core/backends/lan/downloadjob.h @@ -29,6 +29,7 @@ #include #include #include +#include #include @@ -46,6 +47,7 @@ private: QHostAddress mAddress; qint16 mPort; QSharedPointer mSocket; + QSharedPointer mBuffer; private Q_SLOTS: void socketFailed(QAbstractSocket::SocketError error); diff --git a/core/backends/lan/uploadjob.cpp b/core/backends/lan/uploadjob.cpp index 85e2035d8..09fe36fd3 100644 --- a/core/backends/lan/uploadjob.cpp +++ b/core/backends/lan/uploadjob.cpp @@ -27,7 +27,8 @@ #include "core_debug.h" -UploadJob::UploadJob(const QSharedPointer& source, const QString& deviceId): KJob() +UploadJob::UploadJob(const QSharedPointer& source, const QString& deviceId) + : KJob() { // TODO: initialize in constructor mInput = source; @@ -38,7 +39,7 @@ UploadJob::UploadJob(const QSharedPointer& source, const QString& dev // We will use this info if link is on ssl, to send encrypted payload this->mDeviceId = deviceId; - connect(mInput.data(), SIGNAL(readyRead()), this, SLOT(readyRead())); + connect(mInput.data(), SIGNAL(readyRead()), this, SLOT(startUploading())); connect(mInput.data(), SIGNAL(aboutToClose()), this, SLOT(aboutToClose())); } @@ -50,6 +51,7 @@ void UploadJob::start() if (mPort > 1764) { //No ports available? qCWarning(KDECONNECT_CORE) << "Error opening a port in range 1739-1764 for file transfer"; mPort = 0; + emitResult(); return; } } @@ -68,17 +70,19 @@ void UploadJob::newConnection() disconnect(mServer, SIGNAL(newConnection()), this, SLOT(newConnection())); mSocket = server->nextPendingConnection(); - connect(mSocket, SIGNAL(disconnected()), mSocket, SLOT(deleteLater())); + mSocket->setParent(this); + connect(mSocket, &QSslSocket::disconnected, this, &UploadJob::cleanup); + connect(mSocket, SIGNAL(error(QAbstractSocket::SocketError)), this, SLOT(socketFailed(QAbstractSocket::SocketError))); + connect(mSocket, SIGNAL(sslErrors(QList)), this, SLOT(sslErrors(QList))); + connect(mSocket, &QSslSocket::encrypted, this, &UploadJob::startUploading); +// connect(mSocket, &QAbstractSocket::stateChanged, [](QAbstractSocket::SocketState state){ qDebug() << "statechange" << state; }); LanLinkProvider::configureSslSocket(mSocket, mDeviceId, true); mSocket->startServerEncryption(); - mSocket->waitForEncrypted(); - - readyRead(); } -void UploadJob::readyRead() +void UploadJob::startUploading() { while ( mInput->bytesAvailable() > 0 ) { @@ -93,14 +97,19 @@ void UploadJob::readyRead() while ( mSocket->flush() ); } } - mInput->close(); } void UploadJob::aboutToClose() { +// qDebug() << "closing..."; mSocket->disconnectFromHost(); +} + +void UploadJob::cleanup() +{ mSocket->close(); +// qDebug() << "closed!"; emitResult(); } @@ -110,3 +119,18 @@ QVariantMap UploadJob::transferInfo() return {{"port", mPort}}; } +void UploadJob::socketFailed(QAbstractSocket::SocketError error) +{ + qWarning() << "error uploading" << error; + setError(2); + emitResult(); + mSocket->close(); +} + +void UploadJob::sslErrors(const QList& errors) +{ + qWarning() << "ssl errors" << errors; + setError(1); + emitResult(); + mSocket->close(); +} diff --git a/core/backends/lan/uploadjob.h b/core/backends/lan/uploadjob.h index 0a2218232..775fe4402 100644 --- a/core/backends/lan/uploadjob.h +++ b/core/backends/lan/uploadjob.h @@ -29,7 +29,7 @@ #include #include "server.h" -class UploadJob +class KDECONNECTCORE_EXPORT UploadJob : public KJob { Q_OBJECT @@ -48,9 +48,13 @@ private: QString mDeviceId; private Q_SLOTS: - void readyRead(); + void startUploading(); void newConnection(); void aboutToClose(); + void cleanup(); + + void socketFailed(QAbstractSocket::SocketError); + void sslErrors(const QList &errors); }; #endif // UPLOADJOB_H diff --git a/core/filetransferjob.cpp b/core/filetransferjob.cpp index 0c5dd84ac..4f63f76bd 100644 --- a/core/filetransferjob.cpp +++ b/core/filetransferjob.cpp @@ -72,7 +72,7 @@ void FileTransferJob::doStart() return; } - startTransfer(); + connect(mOrigin.data(), &QIODevice::readyRead, this, &FileTransferJob::startTransfer); } void FileTransferJob::startTransfer() diff --git a/tests/sendfiletest.cpp b/tests/sendfiletest.cpp index a5029cdfb..4652a7241 100644 --- a/tests/sendfiletest.cpp +++ b/tests/sendfiletest.cpp @@ -19,6 +19,10 @@ */ #include +#include +#include +#include +#include #include #include #include @@ -83,6 +87,58 @@ class TestSendFile : public QObject QCOMPARE(file.readAll(), content); } + void testSslJobs() + { + const QString aFile = QFINDTESTDATA("sendfiletest.cpp"); + const QString destFile = QDir::tempPath() + "/kdeconnect-test-sentfile"; + QFile(destFile).remove(); + + const QString deviceId = KdeConnectConfig::instance()->deviceId() + , deviceName = "testdevice" + , deviceType = KdeConnectConfig::instance()->deviceType(); + + KdeConnectConfig* kcc = KdeConnectConfig::instance(); + kcc->addTrustedDevice(deviceId, deviceName, deviceType); + kcc->setDeviceProperty(deviceId, QString("certificate"), QString::fromLatin1(kcc->certificate().toPem())); // Using same certificate from kcc, instead of generating + + QSharedPointer f(new QFile(aFile)); + UploadJob* uj = new UploadJob(f, deviceId); + QSignalSpy spyUpload(uj, &KJob::result); + uj->start(); + + auto info = uj->transferInfo(); + info.insert("deviceId", deviceId); + info.insert("size", aFile.size()); + + DownloadJob* dj = new DownloadJob(QHostAddress::LocalHost, info); + + QVERIFY(dj->getPayload()->open(QIODevice::ReadOnly)); + + FileTransferJob* ft = new FileTransferJob(dj->getPayload(), uj->transferInfo()["size"].toInt(), QUrl::fromLocalFile(destFile)); + + QSignalSpy spyDownload(dj, &KJob::result); + QSignalSpy spyTransfer(ft, &KJob::result); + + ft->start(); + dj->start(); + + QVERIFY(spyTransfer.count() || spyTransfer.wait(10000000000000000000)); + + if (ft->error()) qWarning() << "fterror" << ft->errorString(); + + QCOMPARE(ft->error(), 0); + QCOMPARE(spyDownload.count(), 1); + QCOMPARE(spyUpload.count(), 1); + + QFile resultFile(destFile), originFile(aFile); + QVERIFY(resultFile.open(QIODevice::ReadOnly)); + QVERIFY(originFile.open(QIODevice::ReadOnly)); + + const QByteArray resultContents = resultFile.readAll(), originContents = originFile.readAll(); + QCOMPARE(resultContents.size(), originContents.size()); + QCOMPARE(resultFile.readAll(), originFile.readAll()); + } + private: TestDaemon* mDaemon; };