From 226d06402b0fe8e95d6261c788df61f0af91b2f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= Date: Wed, 14 Jun 2023 18:48:19 +0200 Subject: [PATCH] Network-chat: Fix remote peer making multiple connections The system was just treating IP (and optionally port) as a unique identifier, so if a peer had multiple possible paths to a client they would connect multiple times. This fixes that by generating using QUuid in each client. We then use this during broadcast, replacing the username we sent before (which was not used), and as part of the greeting. The greeting now is more complex, since we need to send both username and the ID. Change-Id: I6c6c2ffd5198406aad48445a68dd6aab36de69c0 Reviewed-by: Konrad Kujawa Reviewed-by: Timur Pocheptsov --- examples/network/network-chat/client.cpp | 30 +++++------- examples/network/network-chat/client.h | 4 +- examples/network/network-chat/connection.cpp | 47 ++++++++++++++----- examples/network/network-chat/connection.h | 7 ++- examples/network/network-chat/peermanager.cpp | 26 +++++++--- examples/network/network-chat/peermanager.h | 2 + 6 files changed, 77 insertions(+), 39 deletions(-) diff --git a/examples/network/network-chat/client.cpp b/examples/network/network-chat/client.cpp index 54bde6cd037..495ae90d023 100644 --- a/examples/network/network-chat/client.cpp +++ b/examples/network/network-chat/client.cpp @@ -37,27 +37,14 @@ QString Client::nickName() const + ':' + QString::number(server.serverPort()); } -bool Client::hasConnection(const QHostAddress &senderIp, int senderPort) const +bool Client::hasConnection(const QByteArray &peerUniqueId) const { - auto [begin, end] = peers.equal_range(senderIp); - if (begin == peers.constEnd()) - return false; - - if (senderPort == -1) - return true; - - for (; begin != end; ++begin) { - Connection *connection = *begin; - if (connection->peerPort() == senderPort) - return true; - } - - return false; + return peers.contains(peerUniqueId); } void Client::newConnection(Connection *connection) { - connection->setGreetingMessage(peerManager->userName()); + connection->setGreetingMessage(peerManager->userName(), peerManager->uniqueId()); connect(connection, &Connection::errorOccurred, this, &Client::connectionError); connect(connection, &Connection::disconnected, this, &Client::disconnected); @@ -67,13 +54,18 @@ void Client::newConnection(Connection *connection) void Client::readyForUse() { Connection *connection = qobject_cast(sender()); - if (!connection || hasConnection(connection->peerAddress(), connection->peerPort())) + if (!connection || hasConnection(connection->uniqueId())) { + if (connection) { + connection->disconnectFromHost(); + connection->deleteLater(); + } return; + } connect(connection, &Connection::newMessage, this, &Client::newMessage); - peers.insert(connection->peerAddress(), connection); + peers.insert(connection->uniqueId(), connection); QString nick = connection->name(); if (!nick.isEmpty()) emit newParticipant(nick); @@ -93,7 +85,7 @@ void Client::connectionError(QAbstractSocket::SocketError /* socketError */) void Client::removeConnection(Connection *connection) { - if (peers.remove(connection->peerAddress(), connection) > 0) { + if (peers.remove(connection->uniqueId(), connection) > 0) { QString nick = connection->name(); if (!nick.isEmpty()) emit participantLeft(nick); diff --git a/examples/network/network-chat/client.h b/examples/network/network-chat/client.h index 55abd84c9e6..2ce6afe8269 100644 --- a/examples/network/network-chat/client.h +++ b/examples/network/network-chat/client.h @@ -21,7 +21,7 @@ public: void sendMessage(const QString &message); QString nickName() const; - bool hasConnection(const QHostAddress &senderIp, int senderPort = -1) const; + bool hasConnection(const QByteArray &peerUniqueId) const; signals: void newMessage(const QString &from, const QString &message); @@ -39,7 +39,7 @@ private: PeerManager *peerManager; Server server; - QMultiHash peers; + QMultiHash peers; }; #endif diff --git a/examples/network/network-chat/connection.cpp b/examples/network/network-chat/connection.cpp index f24e0e2a0b7..0f59cc91ede 100644 --- a/examples/network/network-chat/connection.cpp +++ b/examples/network/network-chat/connection.cpp @@ -21,7 +21,7 @@ static const int PingInterval = 5 * 1000; * plaintext = { 0 => text } * ping = { 1 => null } * pong = { 2 => null } - * greeting = { 3 => text } + * greeting = { 3 => { text, bytes } } */ Connection::Connection(QObject *parent) @@ -60,9 +60,15 @@ QString Connection::name() const return username; } -void Connection::setGreetingMessage(const QString &message) +void Connection::setGreetingMessage(const QString &message, const QByteArray &uniqueId) { greetingMessage = message; + localUniqueId = uniqueId; +} + +QByteArray Connection::uniqueId() const +{ + return peerUniqueId; } bool Connection::sendMessage(const QString &message) @@ -118,7 +124,29 @@ void Connection::processReadyRead() reader.next(); } else { // Current state: read command payload - if (reader.isString()) { + if (currentDataType == Greeting) { + if (state == ReadingGreeting) { + if (!reader.isContainer() || !reader.isLengthKnown() || reader.length() != 2) + break; // protocol error + state = ProcessingGreeting; + reader.enterContainer(); + } + if (state != ProcessingGreeting) + break; // protocol error + if (reader.isString()) { + auto r = reader.readString(); + buffer += r.data; + } else if (reader.isByteArray()) { + auto r = reader.readByteArray(); + peerUniqueId += r.data; + if (r.status == QCborStreamReader::EndOfString) { + reader.leaveContainer(); + processGreeting(); + } + } + if (state == ProcessingGreeting) + continue; + } else if (reader.isString()) { auto r = reader.readString(); buffer += r.data; if (r.status != QCborStreamReader::EndOfString) @@ -126,7 +154,7 @@ void Connection::processReadyRead() } else if (reader.isNull()) { reader.next(); } else { - break; // protocol error + break; // protocol error } // Next state: no command read @@ -136,13 +164,7 @@ void Connection::processReadyRead() transferTimerId = -1; } - if (state == ReadingGreeting) { - if (currentDataType != Greeting) - break; // protocol error - processGreeting(); - } else { - processData(); - } + processData(); } } @@ -172,7 +194,10 @@ void Connection::sendGreetingMessage() writer.startMap(1); writer.append(Greeting); + writer.startArray(2); writer.append(greetingMessage); + writer.append(localUniqueId); + writer.endArray(); writer.endMap(); isGreetingMessageSent = true; diff --git a/examples/network/network-chat/connection.h b/examples/network/network-chat/connection.h index d877b176279..4b063e7def8 100644 --- a/examples/network/network-chat/connection.h +++ b/examples/network/network-chat/connection.h @@ -20,6 +20,7 @@ public: enum ConnectionState { WaitingForGreeting, ReadingGreeting, + ProcessingGreeting, ReadyForUse }; enum DataType { @@ -35,9 +36,11 @@ public: ~Connection(); QString name() const; - void setGreetingMessage(const QString &message); + void setGreetingMessage(const QString &message, const QByteArray &uniqueId); bool sendMessage(const QString &message); + QByteArray uniqueId() const; + signals: void readyForUse(); void newMessage(const QString &from, const QString &message); @@ -62,6 +65,8 @@ private: QTimer pingTimer; QElapsedTimer pongTime; QString buffer; + QByteArray localUniqueId; + QByteArray peerUniqueId; ConnectionState state = WaitingForGreeting; DataType currentDataType = Undefined; int transferTimerId = -1; diff --git a/examples/network/network-chat/peermanager.cpp b/examples/network/network-chat/peermanager.cpp index da4210d85b8..a70ed7da73f 100644 --- a/examples/network/network-chat/peermanager.cpp +++ b/examples/network/network-chat/peermanager.cpp @@ -7,6 +7,7 @@ #include "peermanager.h" #include +#include static const qint32 BroadcastInterval = 2000; static const unsigned broadcastPort = 45000; @@ -27,6 +28,11 @@ PeerManager::PeerManager(Client *client) if (username.isEmpty()) username = "unknown"; + // We generate a unique per-process identifier so we can avoid multiple + // connections to/from the same remote peer as well as ignore our own + // broadcasts. + localUniqueId = QUuid::createUuid().toByteArray(); + updateAddresses(); broadcastSocket.bind(QHostAddress::Any, broadcastPort, QUdpSocket::ShareAddress @@ -49,6 +55,11 @@ QString PeerManager::userName() const return username; } +QByteArray PeerManager::uniqueId() const +{ + return localUniqueId; +} + void PeerManager::startBroadcasting() { broadcastTimer.start(); @@ -65,7 +76,7 @@ void PeerManager::sendBroadcastDatagram() { QCborStreamWriter writer(&datagram); writer.startArray(2); - writer.append(username); + writer.append(localUniqueId); writer.append(serverPort); writer.endArray(); } @@ -92,6 +103,7 @@ void PeerManager::readBroadcastDatagram() continue; int senderServerPort; + QByteArray peerUniqueId; { // decode the datagram QCborStreamReader reader(datagram); @@ -101,10 +113,12 @@ void PeerManager::readBroadcastDatagram() continue; reader.enterContainer(); - if (reader.lastError() != QCborError::NoError || !reader.isString()) + if (reader.lastError() != QCborError::NoError || !reader.isByteArray()) continue; - while (reader.readString().status == QCborStreamReader::Ok) { - // we don't actually need the username right now + auto r = reader.readByteArray(); + while (r.status == QCborStreamReader::Ok) { + peerUniqueId = r.data; + r = reader.readByteArray(); } if (reader.lastError() != QCborError::NoError || !reader.isUnsignedInteger()) @@ -112,10 +126,10 @@ void PeerManager::readBroadcastDatagram() senderServerPort = reader.toInteger(); } - if (isLocalHostAddress(senderIp) && senderServerPort == serverPort) + if (peerUniqueId == localUniqueId) continue; - if (!client->hasConnection(senderIp)) { + if (!client->hasConnection(peerUniqueId)) { Connection *connection = new Connection(this); emit newConnection(connection); connection->connectToHost(senderIp, senderServerPort); diff --git a/examples/network/network-chat/peermanager.h b/examples/network/network-chat/peermanager.h index dadabd88aa2..fff48540ea7 100644 --- a/examples/network/network-chat/peermanager.h +++ b/examples/network/network-chat/peermanager.h @@ -22,6 +22,7 @@ public: void setServerPort(int port); QString userName() const; + QByteArray uniqueId() const; void startBroadcasting(); bool isLocalHostAddress(const QHostAddress &address) const; @@ -41,6 +42,7 @@ private: QUdpSocket broadcastSocket; QTimer broadcastTimer; QString username; + QByteArray localUniqueId; int serverPort = 0; };