Fix connection issue in case KNXnet/IP NAT was used

The local data endpoint did not receive usable data, because
its address and port number were not sent while establishing
the connection via NAT.
To overcome this problem, use a single socket for both the
control and data endpoint.

Task-number: QTBUG-67926
Change-Id: Ibcddbe111232b1eefae772561fa321311a6d8ab3
Reviewed-by: Karsten Heimrich <karsten.heimrich@qt.io>

2 files changed, 76 insertions, 131 deletions

diff --git a/src/knx/netip/qknxnetipendpointconnection.cpp b/src/knx/netip/qknxnetipendpointconnection.cpp
index 0afba7b..7d0a5a8 100644
--- a/src/knx/netip/qknxnetipendpointconnection.cpp
+++ b/src/knx/netip/qknxnetipendpointconnection.cpp
@@ -153,95 +153,64 @@ void QKnxNetIpEndpointConnectionPrivate::setup()
     m_errorString = QString();
     m_error = QKnxNetIpEndpointConnection::Error::None;
 
-    QObject::connect(m_dataEndpoint, &QUdpSocket::readyRead, [&]() {
-        while (m_dataEndpoint && m_dataEndpoint->state() == QUdpSocket::BoundState
-            && m_dataEndpoint->hasPendingDatagrams()) {
-            auto datagram = m_dataEndpoint->receiveDatagram();
-            QKnxByteArray data(datagram.data().constData(), datagram.data().size());
-                const auto header = QKnxNetIpFrameHeader::fromBytes(data, 0);
-                if (header.isValid() && header.totalSize() == data.size()) {
-                    // TODO: fix the version and validity checks
-                    // if (!m_supportedVersions.contains(header.protocolVersion())) {
-                    //     send E_VERSION_NOT_SUPPORTED confirmation frame
-                    //     send disconnect request
-                    // } else if (header.protocolVersion() != m_dataEndpointVersion {
-                    //     send disconnect request
-                    // } else {
-                    // TODO: set the m_dataEndpointVersion once we receive or send the first frame
-                        switch (header.serviceType()) {
-                        case QKnxNetIp::ServiceType::TunnelingRequest:
-                            processTunnelingRequest(QKnxNetIpFrame::fromBytes(data, 0));
-                            break;
-                        case QKnxNetIp::ServiceType::TunnelingAcknowledge:
-                            processTunnelingAcknowledge(QKnxNetIpFrame::fromBytes(data, 0));
-                            break;
-                        case QKnxNetIp::ServiceType::DeviceConfigurationRequest:
-                            processDeviceConfigurationRequest(QKnxNetIpFrame::fromBytes(data, 0));
-                            break;
-                        case QKnxNetIp::ServiceType::DeviceConfigurationAcknowledge:
-                            processDeviceConfigurationAcknowledge(QKnxNetIpFrame::fromBytes(data, 0));
-                            break;
-                        default:
-                            processDatagram(QKnxNetIpEndpointConnection::EndpointType::Data, datagram);
-                            break;
-                        }
-                    // }
-                }
-        }
-    });
+    QObject::connect(m_socket, &QUdpSocket::readyRead, [&]() {
+        while (m_socket && m_socket->state() == QUdpSocket::BoundState
+            && m_socket->hasPendingDatagrams()) {
+                auto datagram = m_socket->receiveDatagram();
+                auto data = QKnxByteArray::fromByteArray(datagram.data());
 
-    using overload = void (QUdpSocket::*)(QUdpSocket::SocketError);
-    QObject::connect(m_dataEndpoint,
-        static_cast<overload>(&QUdpSocket::error), [&](QUdpSocket::SocketError) {
-            setAndEmitErrorOccurred(QKnxNetIpEndpointConnection::Error::Network,
-                m_dataEndpoint->errorString());
-
-            Q_Q(QKnxNetIpEndpointConnection);
-            q->disconnectFromHost();
-    });
-
-    QObject::connect(m_controlEndpoint, &QUdpSocket::readyRead, [&]() {
-        while (m_controlEndpoint && m_controlEndpoint->state() == QUdpSocket::BoundState
-            && m_controlEndpoint->hasPendingDatagrams()) {
-                auto datagram = m_controlEndpoint->receiveDatagram();
-                QKnxByteArray data(datagram.data().constData(), datagram.data().size());
                 const auto header = QKnxNetIpFrameHeader::fromBytes(data, 0);
-                if (header.isValid() && header.totalSize() == data.size()) {
-                    // TODO: fix the version and validity checks
-                    // if (!m_supportedVersions.contains(header.protocolVersion())) {
-                    //     send E_VERSION_NOT_SUPPORTED confirmation frame
-                    //     send disconnect request
-                    // } else if (header.protocolVersion() != m_controlEndpointVersion) {
-                    //     send disconnect request
-                    // } else {
-                        switch (header.serviceType()) {
-                        case QKnxNetIp::ServiceType::ConnectResponse:
-                            processConnectResponse(QKnxNetIpFrame::fromBytes(data, 0), datagram);
-                            break;
-                        case QKnxNetIp::ServiceType::ConnectionStateResponse:
-                            processConnectionStateResponse(QKnxNetIpFrame::fromBytes(data, 0));
-                            break;
-                        case QKnxNetIp::ServiceType::DisconnectRequest:
-                            processDisconnectRequest(QKnxNetIpFrame::fromBytes(data, 0));
-                            break;
-                        case QKnxNetIp::ServiceType::DisconnectResponse:
-                            processDisconnectResponse(QKnxNetIpFrame::fromBytes(data, 0));
-                            break;
-                        default:
-                            processDatagram(QKnxNetIpEndpointConnection::EndpointType::Control,
-                                datagram);
-                            break;
-                        }
-                    // }
+                if (!header.isValid() || header.totalSize() != data.size())
+                    continue;
+
+                // TODO: fix the version and validity checks
+                // if (!m_supportedVersions.contains(header.protocolVersion())) {
+                //     send E_VERSION_NOT_SUPPORTED confirmation frame
+                //     send disconnect request
+                // } else if (header.protocolVersion() != m_controlEndpointVersion) {
+                //     send disconnect request
+                // } else {
+                // TODO: set the m_dataEndpointVersion once we receive or send the first frame
+                switch (header.serviceType()) {
+                case QKnxNetIp::ServiceType::ConnectResponse:
+                    processConnectResponse(QKnxNetIpFrame::fromBytes(data, 0), datagram);
+                    break;
+                case QKnxNetIp::ServiceType::ConnectionStateResponse:
+                    processConnectionStateResponse(QKnxNetIpFrame::fromBytes(data, 0));
+                    break;
+                case QKnxNetIp::ServiceType::DisconnectRequest:
+                    processDisconnectRequest(QKnxNetIpFrame::fromBytes(data, 0));
+                    break;
+                case QKnxNetIp::ServiceType::DisconnectResponse:
+                    processDisconnectResponse(QKnxNetIpFrame::fromBytes(data, 0));
+                    break;
+
+                case QKnxNetIp::ServiceType::TunnelingRequest:
+                    processTunnelingRequest(QKnxNetIpFrame::fromBytes(data, 0));
+                    break;
+                case QKnxNetIp::ServiceType::TunnelingAcknowledge:
+                    processTunnelingAcknowledge(QKnxNetIpFrame::fromBytes(data, 0));
+                    break;
+                case QKnxNetIp::ServiceType::DeviceConfigurationRequest:
+                    processDeviceConfigurationRequest(QKnxNetIpFrame::fromBytes(data, 0));
+                    break;
+                case QKnxNetIp::ServiceType::DeviceConfigurationAcknowledge:
+                    processDeviceConfigurationAcknowledge(QKnxNetIpFrame::fromBytes(data, 0));
+                    break;
+
+                default:
+                    processDatagram(datagram);
+                    break;
                 }
+                // }
         }
     });
 
     using overload = void (QUdpSocket::*)(QUdpSocket::SocketError);
-    QObject::connect(m_controlEndpoint,
+    QObject::connect(m_socket,
         static_cast<overload>(&QUdpSocket::error), [&](QUdpSocket::SocketError) {
             setAndEmitErrorOccurred(QKnxNetIpEndpointConnection::Error::Network,
-                m_controlEndpoint->errorString());
+                m_socket->errorString());
 
             Q_Q(QKnxNetIpEndpointConnection);
             q->disconnectFromHost();
@@ -256,11 +225,8 @@ void QKnxNetIpEndpointConnectionPrivate::cleanup()
     QKnxPrivate::clearTimer(&m_disconnectRequestTimer);
     QKnxPrivate::clearTimer(&m_acknowledgeTimer);
 
-    if (m_dataEndpoint) m_dataEndpoint->close();
-    if (m_controlEndpoint) m_controlEndpoint->close();
-
-    QKnxPrivate::clearSocket(&m_dataEndpoint);
-    QKnxPrivate::clearSocket(&m_controlEndpoint);
+    if (m_socket) m_socket->close();
+    QKnxPrivate::clearSocket(&m_socket);
 
     setAndEmitStateChanged(QKnxNetIpEndpointConnection::State::Disconnected);
 }
@@ -269,7 +235,7 @@ bool QKnxNetIpEndpointConnectionPrivate::sendCemiRequest()
 {
     if (!m_waitForAcknowledgement) {
         m_waitForAcknowledgement = true;
-        m_dataEndpoint->writeDatagram(m_lastSendCemiRequest.bytes().toByteArray(),
+        m_socket->writeDatagram(m_lastSendCemiRequest.bytes().toByteArray(),
             m_remoteDataEndpoint.address, m_remoteDataEndpoint.port);
         m_cemiRequests++;
         m_acknowledgeTimer->start(m_acknowledgeTimeout);
@@ -282,7 +248,7 @@ void QKnxNetIpEndpointConnectionPrivate::sendStateRequest()
     qDebug().noquote().nospace() << "Sending connection state request: 0x" << m_lastStateRequest
         .bytes().toHex();
 
-    m_controlEndpoint->writeDatagram(m_lastStateRequest.bytes().toByteArray(),
+    m_socket->writeDatagram(m_lastStateRequest.bytes().toByteArray(),
         m_remoteControlEndpoint.address, m_remoteControlEndpoint.port);
 
     m_stateRequests++;
@@ -311,7 +277,7 @@ void QKnxNetIpEndpointConnectionPrivate::processTunnelingRequest(const QKnxNetIp
                     .create();
 
                 qDebug() << "Sending tunneling acknowledge:" << ack;
-                m_dataEndpoint->writeDatagram(ack.bytes().toByteArray(),
+                m_socket->writeDatagram(ack.bytes().toByteArray(),
                     m_remoteDataEndpoint.address, m_remoteDataEndpoint.port);
 
                 if (!counterEquals)
@@ -383,7 +349,7 @@ void QKnxNetIpEndpointConnectionPrivate::processDeviceConfigurationRequest(const
         .create();
 
     qDebug() << "Sending device configuration acknowledge:" << ack;
-    m_dataEndpoint->writeDatagram(ack.bytes().toByteArray(),
+    m_socket->writeDatagram(ack.bytes().toByteArray(),
         m_remoteDataEndpoint.address, m_remoteDataEndpoint.port);
 
     m_receiveCount++;
@@ -459,7 +425,7 @@ void QKnxNetIpEndpointConnectionPrivate::processConnectResponse(const QKnxNetIpF
 
             m_lastStateRequest = QKnxNetIpConnectionStateRequestProxy::builder()
                 .setChannelId(m_channelId)
-                .setControlEndpoint(m_nat ? m_natEndpoint : m_localControlEndpoint)
+                .setControlEndpoint(m_nat ? m_natEndpoint : m_localEndpoint)
                 .create();
 
             QTimer::singleShot(0, [&]() { sendStateRequest(); });
@@ -509,7 +475,7 @@ void QKnxNetIpEndpointConnectionPrivate::processDisconnectRequest(const QKnxNetI
             .create();
         qDebug() << "Sending disconnect response:" << frame;
 
-        m_controlEndpoint->writeDatagram(frame.bytes().toByteArray(),
+        m_socket->writeDatagram(frame.bytes().toByteArray(),
             m_remoteControlEndpoint.address, m_remoteControlEndpoint.port);
 
         Q_Q(QKnxNetIpEndpointConnection);
@@ -533,8 +499,7 @@ void QKnxNetIpEndpointConnectionPrivate::processDisconnectResponse(const QKnxNet
     }
 }
 
-void QKnxNetIpEndpointConnectionPrivate::processDatagram(QKnxNetIpEndpointConnection::EndpointType,
-    const QNetworkDatagram &)
+void QKnxNetIpEndpointConnectionPrivate::processDatagram(const QNetworkDatagram &)
 {}
 
 void QKnxNetIpEndpointConnectionPrivate::setAndEmitStateChanged(
@@ -602,7 +567,7 @@ quint8 QKnxNetIpEndpointConnection::netIpHeaderVersion(EndpointType endpoint) co
 quint16 QKnxNetIpEndpointConnection::localPort() const
 {
     Q_D(const QKnxNetIpEndpointConnection);
-    return d->m_localControlEndpoint.port;
+    return d->m_localEndpoint.port;
 }
 
 void QKnxNetIpEndpointConnection::setLocalPort(quint16 port)
@@ -616,7 +581,7 @@ QHostAddress QKnxNetIpEndpointConnection::localAddress() const
     Q_D(const QKnxNetIpEndpointConnection);
     if (d->m_state == QKnxNetIpEndpointConnection::Disconnected)
         return d->m_user.address;
-    return d->m_localControlEndpoint.address;
+    return d->m_localEndpoint.address;
 }
 
 void QKnxNetIpEndpointConnection::setLocalAddress(const QHostAddress &address)
@@ -702,32 +667,18 @@ void QKnxNetIpEndpointConnection::connectToHost(const QHostAddress &address, qui
 
     d->setAndEmitStateChanged(QKnxNetIpEndpointConnection::State::Starting);
 
-    auto socket = new QUdpSocket(this);
-    QKnxPrivate::clearSocket(&(d->m_controlEndpoint));
-    if (!socket->bind(d->m_user.address, d->m_user.port)) {
-        d->setAndEmitErrorOccurred(QKnxNetIpEndpointConnection::Error::Network,
-            QKnxNetIpEndpointConnection::tr("Could not bind local control endpoint: %1")
-                .arg(socket->errorString()));
-        QKnxPrivate::clearSocket(&socket);
-        d->setAndEmitStateChanged(QKnxNetIpEndpointConnection::State::Disconnected);
-        return;
-    }
-    d->m_controlEndpoint = socket;
-    d->m_localControlEndpoint = Endpoint(socket->localAddress(), socket->localPort());
+    QKnxPrivate::clearSocket(&(d->m_socket));
 
-    socket = new QUdpSocket(this);
-    QKnxPrivate::clearSocket(&(d->m_dataEndpoint));
-    if (!socket->bind(d->m_localControlEndpoint.address, 0)) {
+    d->m_socket = new QUdpSocket(this);
+    if (!d->m_socket->bind(d->m_user.address, d->m_user.port)) {
         d->setAndEmitErrorOccurred(QKnxNetIpEndpointConnection::Error::Network,
-            QKnxNetIpEndpointConnection::tr("Could not bind local data endpoint: %1")
-                .arg(socket->errorString()));
-        QKnxPrivate::clearSocket(&socket);
-        QKnxPrivate::clearSocket(&(d->m_controlEndpoint));
+            QKnxNetIpEndpointConnection::tr("Could not bind endpoint: %1")
+                .arg(d->m_socket->errorString()));
+        QKnxPrivate::clearSocket(&d->m_socket);
         d->setAndEmitStateChanged(QKnxNetIpEndpointConnection::State::Disconnected);
         return;
     }
-    d->m_dataEndpoint = socket;
-    d->m_localDataEndpoint = Endpoint(socket->localAddress(), socket->localPort());
+    d->m_localEndpoint = Endpoint(d->m_socket->localAddress(), d->m_socket->localPort());
 
     d->setAndEmitStateChanged(QKnxNetIpEndpointConnection::State::Bound);
 
@@ -736,11 +687,8 @@ void QKnxNetIpEndpointConnection::connectToHost(const QHostAddress &address, qui
     d->setAndEmitStateChanged(QKnxNetIpEndpointConnection::State::Connecting);
 
     auto request = QKnxNetIpConnectRequestProxy::builder()
-        .setControlEndpoint(d->m_nat ? d->m_natEndpoint : d->m_localControlEndpoint)
-        // TODO: Fix this. It cannot work in NAT mode, because the sever does not know the data
-        // endpoint address if we pass a NAT endpoint. The most likely solution will be to use a
-        // single socket for control and data endpoint...
-        .setDataEndpoint(d->m_nat ? d->m_natEndpoint : d->m_localDataEndpoint)
+        .setControlEndpoint(d->m_nat ? d->m_natEndpoint : d->m_localEndpoint)
+        .setDataEndpoint(d->m_nat ? d->m_natEndpoint : d->m_localEndpoint)
         .setRequestInformation(d->m_cri)
         .create();
     d->m_controlEndpointVersion = request.header().protocolVersion();
@@ -749,7 +697,7 @@ void QKnxNetIpEndpointConnection::connectToHost(const QHostAddress &address, qui
 
     d->m_connectRequestTimer->start(QKnxNetIp::ConnectRequestTimeout);
 
-    d->m_controlEndpoint->writeDatagram(request.bytes().toByteArray(),
+    d->m_socket->writeDatagram(request.bytes().toByteArray(),
         d->m_remoteControlEndpoint.address, d->m_remoteControlEndpoint.port);
 }
 
@@ -768,11 +716,11 @@ void QKnxNetIpEndpointConnection::disconnectFromHost()
     } else {
         auto frame = QKnxNetIpDisconnectRequestProxy::builder()
             .setChannelId(d->m_channelId)
-            .setControlEndpoint(d->m_nat ? d->m_natEndpoint : d->m_localControlEndpoint)
+            .setControlEndpoint(d->m_nat ? d->m_natEndpoint : d->m_localEndpoint)
             .create();
 
         qDebug() << "Sending disconnect request:" << frame;
-        d->m_controlEndpoint->writeDatagram(frame.bytes().toByteArray(),
+        d->m_socket->writeDatagram(frame.bytes().toByteArray(),
             d->m_remoteControlEndpoint.address, d->m_remoteControlEndpoint.port);
 
         d->m_disconnectRequestTimer->start(QKnxNetIp::DisconnectRequestTimeout);
diff --git a/src/knx/netip/qknxnetipendpointconnection_p.h b/src/knx/netip/qknxnetipendpointconnection_p.h
index e68b883..d33a493 100644
--- a/src/knx/netip/qknxnetipendpointconnection_p.h
+++ b/src/knx/netip/qknxnetipendpointconnection_p.h
@@ -109,7 +109,7 @@ public:
     QKnxNetIpEndpointConnectionPrivate(const QHostAddress &address, quint16 port,
             const QKnxNetIpCri &cri, int sendAttempts, QKnxNetIp::Timeout ackTimeout)
         : m_cri(cri)
-        , m_localControlEndpoint { address, port }
+        , m_localEndpoint { address, port }
         , m_maxCemiRequest(sendAttempts)
         , m_acknowledgeTimeout(ackTimeout)
     {}
@@ -140,7 +140,7 @@ public:
     virtual void processDisconnectRequest(const QKnxNetIpFrame &frame);
     virtual void processDisconnectResponse(const QKnxNetIpFrame &frame);
 
-    virtual void processDatagram(QKnxNetIpEndpointConnection::EndpointType, const QNetworkDatagram &);
+    virtual void processDatagram(const QNetworkDatagram &);
 
     void setAndEmitStateChanged(QKnxNetIpEndpointConnection::State newState);
     void setAndEmitErrorOccurred(QKnxNetIpEndpointConnection::Error newError, const QString &message);
@@ -151,9 +151,7 @@ private:
     Endpoint m_remoteControlEndpoint;
 
     Endpoint m_natEndpoint { QHostAddress::AnyIPv4 };
-
-    Endpoint m_localDataEndpoint { QHostAddress::LocalHost };
-    Endpoint m_localControlEndpoint { QHostAddress::LocalHost };
+    Endpoint m_localEndpoint { QHostAddress::LocalHost };
 
     int m_channelId { -1 };
     quint8 m_sendCount { 0 };
@@ -188,8 +186,7 @@ private:
     QTimer *m_acknowledgeTimer { nullptr };
     bool m_waitForAcknowledgement { false };
 
-    QUdpSocket *m_dataEndpoint { nullptr };
-    QUdpSocket *m_controlEndpoint { nullptr };
+    QUdpSocket *m_socket { nullptr };
 
     UserProperties m_user;
 };