From c7579eb17017b8c6e1e422781af7940accd266a4 Mon Sep 17 00:00:00 2001 From: Simon Redman Date: Wed, 5 Jun 2019 15:14:50 +0000 Subject: [PATCH] Fix LanLinkProviderTest on Windows ## Summary LanLinkProviderTest fails on Windows. This patch fixes that. I believe the root cause is that we are using a shared UDP socket to listen for identity broadcasts both in the LanLinkProvider and in the test. Apparently this works on Linux, but on Windows the LanLinkProvider picks up its own identity packet and pairs with itself. This patch gives a parameter to LanLinkProvider to allow it to listen and broadcast on different ports, then uses that ability in the test to make the test pass on Windows. ## Test Plan ### Before: lanlinkprovider test fails, first because it can't bind its UDP listener socket, and then because Windows seems to handle shared sockets differently than Linux, so the UDP broadcasts were not reaching the test's listener. ### After: lanlinkprovider test seems to pass reliably both in my Windows VM and in the CI --- core/backends/lan/lanlinkprovider.cpp | 21 +++-- core/backends/lan/lanlinkprovider.h | 17 +++- tests/lanlinkprovidertest.cpp | 121 +++++++++++++++++++++++--- 3 files changed, 140 insertions(+), 19 deletions(-) diff --git a/core/backends/lan/lanlinkprovider.cpp b/core/backends/lan/lanlinkprovider.cpp index d587b225f..572ac3f8a 100644 --- a/core/backends/lan/lanlinkprovider.cpp +++ b/core/backends/lan/lanlinkprovider.cpp @@ -45,10 +45,16 @@ #define MIN_VERSION_WITH_SSL_SUPPORT 6 -LanLinkProvider::LanLinkProvider(bool testMode) +LanLinkProvider::LanLinkProvider( + bool testMode, + quint16 udpBroadcastPort, + quint16 udpListenPort + ) : m_server(new Server(this)) , m_udpSocket(this) , m_tcpPort(0) + , m_udpBroadcastPort(udpBroadcastPort) + , m_udpListenPort(udpListenPort) , m_testMode(testMode) , m_combineBroadcastsTimer(this) { @@ -86,14 +92,14 @@ void LanLinkProvider::onStart() { const QHostAddress bindAddress = m_testMode? QHostAddress::LocalHost : QHostAddress::Any; - bool success = m_udpSocket.bind(bindAddress, UDP_PORT, QUdpSocket::ShareAddress); + bool success = m_udpSocket.bind(bindAddress, m_udpListenPort, QUdpSocket::ShareAddress); if (!success) { QAbstractSocket::SocketError sockErr = m_udpSocket.error(); // Refer to https://doc.qt.io/qt-5/qabstractsocket.html#SocketError-enum to decode socket error number QString errorMessage = QMetaEnum::fromType().valueToKey(sockErr); qCritical(KDECONNECT_CORE) << QLatin1String("Failed to bind UDP socket on port") - << UDP_PORT + << m_udpListenPort << QLatin1String("with error") << errorMessage; } @@ -160,15 +166,14 @@ void LanLinkProvider::broadcastToNetwork() QHostAddress sourceAddress = ifaceAddress.ip(); if (sourceAddress.protocol() == QAbstractSocket::IPv4Protocol && sourceAddress != QHostAddress::LocalHost) { qCDebug(KDECONNECT_CORE()) << "Broadcasting as" << sourceAddress; - sendSocket.bind(sourceAddress, UDP_PORT); - sendSocket.writeDatagram(np.serialize(), destAddress, UDP_PORT); + sendSocket.writeDatagram(np.serialize(), destAddress, m_udpBroadcastPort); sendSocket.close(); } } } } #else - m_udpSocket.writeDatagram(np.serialize(), destAddress, UDP_PORT); + m_udpSocket.writeDatagram(np.serialize(), destAddress, m_udpBroadcastPort); #endif } @@ -237,7 +242,7 @@ void LanLinkProvider::connectError(QAbstractSocket::SocketError socketError) NetworkPacket np(QLatin1String("")); NetworkPacket::createIdentityPacket(&np); np.set(QStringLiteral("tcpPort"), m_tcpPort); - m_udpSocket.writeDatagram(np.serialize(), m_receivedIdentityPackets[socket].sender, UDP_PORT); + m_udpSocket.writeDatagram(np.serialize(), m_receivedIdentityPackets[socket].sender, m_udpBroadcastPort); //The socket we created didn't work, and we didn't manage //to create a LanDeviceLink from it, deleting everything. @@ -299,7 +304,7 @@ void LanLinkProvider::tcpSocketConnected() //I think this will never happen, but if it happens the deviceLink //(or the socket that is now inside it) might not be valid. Delete them. qCDebug(KDECONNECT_CORE) << "Fallback (2), try reverse connection (send udp packet)"; - m_udpSocket.writeDatagram(np2.serialize(), m_receivedIdentityPackets[socket].sender, UDP_PORT); + m_udpSocket.writeDatagram(np2.serialize(), m_receivedIdentityPackets[socket].sender, m_udpBroadcastPort); } delete m_receivedIdentityPackets.take(socket).np; diff --git a/core/backends/lan/lanlinkprovider.h b/core/backends/lan/lanlinkprovider.h index 6304fceec..ee014f054 100644 --- a/core/backends/lan/lanlinkprovider.h +++ b/core/backends/lan/lanlinkprovider.h @@ -40,7 +40,16 @@ class KDECONNECTCORE_EXPORT LanLinkProvider Q_OBJECT public: - LanLinkProvider(bool testMode = false); + /** + * @param testMode Some special overrides needed while testing + * @param udpBroadcastPort Port which should be used for *sending* identity packets + * @param udpListenPort Port which should be used for *receiving* identity packets + */ + LanLinkProvider( + bool testMode = false, + quint16 udpBroadcastPort = UDP_PORT, + quint16 udpListenPort = UDP_PORT + ); ~LanLinkProvider() override; QString name() override { return QStringLiteral("LanLinkProvider"); } @@ -53,6 +62,9 @@ public: static void configureSslSocket(QSslSocket* socket, const QString& deviceId, bool isDeviceTrusted); static void configureSocket(QSslSocket* socket); + /** + * This is the default UDP port both for broadcasting and receiving identity packets + */ const static quint16 UDP_PORT = 1716; const static quint16 MIN_TCP_PORT = 1716; const static quint16 MAX_TCP_PORT = 1764; @@ -83,6 +95,9 @@ private: QUdpSocket m_udpSocket; quint16 m_tcpPort; + quint16 m_udpBroadcastPort; + quint16 m_udpListenPort; + QMap m_links; QMap m_pairingHandlers; diff --git a/tests/lanlinkprovidertest.cpp b/tests/lanlinkprovidertest.cpp index 7288bade7..3589404a6 100644 --- a/tests/lanlinkprovidertest.cpp +++ b/tests/lanlinkprovidertest.cpp @@ -32,6 +32,7 @@ #include #include #include +#include /* * This class tests the working of LanLinkProvider under different conditions that when identity packet is received over TCP, over UDP and same when the device is paired. @@ -42,14 +43,28 @@ class LanLinkProviderTest : public QObject Q_OBJECT public: explicit LanLinkProviderTest() - : m_lanLinkProvider(true) { + : m_lanLinkProvider(true) + , m_server(nullptr) + , m_reader(nullptr) + , m_udpSocket(nullptr) + { QStandardPaths::setTestModeEnabled(true); } public Q_SLOTS: void initTestCase(); + void init(); + void cleanup(); private Q_SLOTS: + /** + * Test that the LanLinkProvider will send an identity packet to a non-default port + */ + void testChangedUDPBroadcastPort(); + /** + * Test that the LanLinkProvider will receive an identity packet on a non-default port + */ + void testChangedUDPListenPort(); void pairedDeviceTcpPacketReceived(); void pairedDeviceUdpPacketReceived(); @@ -78,7 +93,7 @@ private: void removeTrustedDevice(); void setSocketAttributes(QSslSocket* socket); void testIdentityPacket(QByteArray& identityPacket); - + void socketBindErrorFail(const QUdpSocket& socket); }; void LanLinkProviderTest::initTestCase() @@ -90,22 +105,90 @@ void LanLinkProviderTest::initTestCase() m_privateKey = QCA::KeyGenerator().createRSA(2048); m_certificate = generateCertificate(m_deviceId, m_privateKey); - m_lanLinkProvider.onStart(); - m_identityPacket = QStringLiteral("{\"id\":1439365924847,\"type\":\"kdeconnect.identity\",\"body\":{\"deviceId\":\"testdevice\",\"deviceName\":\"Test Device\",\"protocolVersion\":6,\"deviceType\":\"phone\",\"tcpPort\":") + QString::number(TEST_PORT) + QStringLiteral("}}"); } +void LanLinkProviderTest::init() +{ + m_lanLinkProvider.onStart(); +} + +void LanLinkProviderTest::cleanup() +{ + m_lanLinkProvider.onStop(); +} + +void LanLinkProviderTest::testChangedUDPBroadcastPort() +{ + quint16 udpListenPort = LanLinkProvider::UDP_PORT; + quint16 udpBroadcastPort = LanLinkProvider::UDP_PORT + 1; + + m_lanLinkProvider.onStop(); + LanLinkProvider testlanLinkProvider(true, udpBroadcastPort, udpListenPort); + testlanLinkProvider.onStart(); + + QUdpSocket mUdpServer; + bool bindSuccessful = mUdpServer.bind(QHostAddress::LocalHost, udpBroadcastPort, QUdpSocket::ShareAddress | QUdpSocket::ReuseAddressHint); + if (!bindSuccessful) { + socketBindErrorFail(mUdpServer); + } + + QSignalSpy spy(&mUdpServer, SIGNAL(readyRead())); + testlanLinkProvider.onNetworkChange(); + QVERIFY2(!spy.isEmpty() || spy.wait(), "Did not receive UDP packet"); +} + +void LanLinkProviderTest::testChangedUDPListenPort() +{ + quint16 udpListenPort = LanLinkProvider::UDP_PORT + 1; + quint16 udpBroadcastPort = LanLinkProvider::UDP_PORT; + + m_lanLinkProvider.onStop(); + LanLinkProvider testlanLinkProvider(true, udpBroadcastPort, udpListenPort); + testlanLinkProvider.onStart(); + + m_server = new Server(this); + QUdpSocket testUdpSocket; + + m_server->listen(QHostAddress::LocalHost, TEST_PORT); + + QSignalSpy spy(m_server, SIGNAL(newConnection())); + + // Write an identity packet to udp socket here. We do not broadcast it here. + qint64 bytesWritten = testUdpSocket.writeDatagram(m_identityPacket.toLatin1(), QHostAddress::LocalHost, udpListenPort); + QCOMPARE(bytesWritten, m_identityPacket.size()); + + // In response to receiving an identity packet, the LanLinkProvider should try to open a TCP connection to us + QVERIFY(!spy.isEmpty() || spy.wait()); + + QSslSocket* serverSocket = m_server->nextPendingConnection(); + + QVERIFY2(serverSocket != 0, "Server socket is null"); + QVERIFY2(serverSocket->isOpen(), "Server socket already closed"); + + delete m_server; +} + void LanLinkProviderTest::pairedDeviceTcpPacketReceived() { + quint16 udpListenPort = LanLinkProvider::UDP_PORT; + quint16 udpBroadcastPort = LanLinkProvider::UDP_PORT + 1; + + m_lanLinkProvider.onStop(); + LanLinkProvider testlanLinkProvider(true, udpBroadcastPort, udpListenPort); + testlanLinkProvider.onStart(); + KdeConnectConfig* kcc = KdeConnectConfig::instance(); addTrustedDevice(); QUdpSocket* mUdpServer = new QUdpSocket; - bool b = mUdpServer->bind(QHostAddress::LocalHost, LanLinkProvider::UDP_PORT, QUdpSocket::ShareAddress); - QVERIFY(b); + bool bindSuccessful = mUdpServer->bind(QHostAddress::LocalHost, udpBroadcastPort, QUdpSocket::ShareAddress); + if (!bindSuccessful) { + socketBindErrorFail(*mUdpServer); + } QSignalSpy spy(mUdpServer, SIGNAL(readyRead())); - m_lanLinkProvider.onNetworkChange(); + testlanLinkProvider.onNetworkChange(); QVERIFY(!spy.isEmpty() || spy.wait()); QByteArray datagram; @@ -208,12 +291,21 @@ void LanLinkProviderTest::pairedDeviceUdpPacketReceived() void LanLinkProviderTest::unpairedDeviceTcpPacketReceived() { + quint16 udpListenPort = LanLinkProvider::UDP_PORT; + quint16 udpBroadcastPort = LanLinkProvider::UDP_PORT + 1; + + m_lanLinkProvider.onStop(); + LanLinkProvider testlanLinkProvider(true, udpBroadcastPort, udpListenPort); + testlanLinkProvider.onStart(); + QUdpSocket* mUdpServer = new QUdpSocket; - bool b = mUdpServer->bind(QHostAddress::LocalHost, LanLinkProvider::UDP_PORT, QUdpSocket::ShareAddress); - QVERIFY(b); + bool bindSuccessful = mUdpServer->bind(QHostAddress::LocalHost, udpBroadcastPort, QUdpSocket::ShareAddress); + if (!bindSuccessful) { + socketBindErrorFail(*mUdpServer); + } QSignalSpy spy(mUdpServer, SIGNAL(readyRead())); - m_lanLinkProvider.onNetworkChange(); + testlanLinkProvider.onNetworkChange(); QVERIFY(!spy.isEmpty() || spy.wait()); QByteArray datagram; @@ -351,6 +443,15 @@ void LanLinkProviderTest::removeTrustedDevice() kcc->removeTrustedDevice(m_deviceId); } +void LanLinkProviderTest::socketBindErrorFail(const QUdpSocket& socket) +{ + QAbstractSocket::SocketError sockErr = socket.error(); + // Refer to https://doc.qt.io/qt-5/qabstractsocket.html#SocketError-enum to decode socket error number + QString errorMessage = QLatin1String("Failed to bind UDP socket with error "); + errorMessage = errorMessage + QMetaEnum::fromType().valueToKey(sockErr); + QFAIL(errorMessage.toLocal8Bit().data()); +} + QTEST_GUILESS_MAIN(LanLinkProviderTest)