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 <konrad.kujawa@qt.io>
Reviewed-by: Timur Pocheptsov <timur.pocheptsov@qt.io>

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);
+    return peers.contains(peerUniqueId);
-    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;
 }
 
 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<Connection *>(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);

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<QHostAddress, Connection *> peers;
+    QMultiHash<QByteArray, Connection *> peers;
 };
 
 #endif

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,9 +124,31 @@ void Connection::processReadyRead()
             reader.next();
         } else {
             // Current state: read command payload
+            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)
                     continue;
             } else if (reader.isNull()) {
@@ -136,15 +164,9 @@ void Connection::processReadyRead()
                 transferTimerId = -1;
             }
 
-            if (state == ReadingGreeting) {
-                if (currentDataType != Greeting)
-                    break;              // protocol error
-                processGreeting();
-            } else {
             processData();
         }
     }
-    }
 
     if (reader.lastError() != QCborError::EndOfFile)
         abort();       // parse error
@@ -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;
 

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;

examples/network/network-chat/peermanager.cpp

@@ -7,6 +7,7 @@
 #include "peermanager.h"
 
 #include <QNetworkInterface>
+#include <QUuid>
 
 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) {
+            auto r = reader.readByteArray();
-                // we don't actually need the username right now
+            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);

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;
 };