Repository: XKNX/xknx
Pull request #1019 (2022-08-23 20:32:13)

Properly ignore repeated TunnellingRequests from UDP
<!--
  You are awesome! Thanks for contributing to our project!
  Please, DO NOT DELETE ANY TEXT from this template!.
-->
## Description
<!--
Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.
List any dependencies that are required for this change.
-->
From 3/8/4 Tunnelling §2.6 Frame confirmation

> If a KNXnet/IP Tunnelling Client receives a frame with a sequence number that equals the expected sequence number then it shall reply with a TUNNELLING_ACK (Status = E_NO_ERROR) frame and process the received frame.

> If a KNXnet/IP Tunnelling Client receives a frame with a sequence number that is one less than the expected sequence number then it shall reply with a TUNNELLING_ACK (Status = E_NO_ERROR) message and discard the received frame.

> If a KNXnet/IP Tunnelling Client receives a frame with a sequence number that is not equal to the expected sequence number and not equal to one less than the expected sequence number, the KNXnet/IP Tunnelling Client shall not reply and shall discard the received frame.

Fixes # (issue)

## Type of change
<!--
Please tick the applicable options.
NOTE: Ticking multiple options most likely indicates
that your change is to big and it is suggested to split it into several smaller PRs.
-->

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update

## Checklist:

- [ ] The documentation has been adjusted accordingly
- [x] Tests have been added that prove the fix is effective or that the feature works
- [x] The changes are documented in the changelog (docs/changelog.md)

codecov[bot]: # [Codecov](https://codecov.io/gh/XKNX/xknx/pull/1019?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) Report
> Merging [#1019](https://codecov.io/gh/XKNX/xknx/pull/1019?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) (c156d72) into [main](https://codecov.io/gh/XKNX/xknx/commit/ecd9e2201955dcbf318790ae4594fd68eb447add?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) (ecd9e22) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/XKNX/xknx/pull/1019/graphs/tree.svg?width=650&height=150&src=pr&token=irWbIygS84&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX)](https://codecov.io/gh/XKNX/xknx/pull/1019?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX)

```diff
@@           Coverage Diff           @@
##             main    #1019   +/-   ##
=======================================
  Coverage   96.29%   96.29%           
=======================================
  Files         133      133           
  Lines        8147     8155    +8     
=======================================
+ Hits         7845     7853    +8     
  Misses        302      302           
```


| [Impacted Files](https://codecov.io/gh/XKNX/xknx/pull/1019?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) | Coverage Δ | |
|---|---|---|
| [xknx/io/tunnel.py](https://codecov.io/gh/XKNX/xknx/pull/1019/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9pby90dW5uZWwucHk=) | `84.70% <100.00%> (+0.47%)` | :arrow_up: |


Patch:
diff --git a/docs/changelog.md b/docs/changelog.md
index c47ed56f..76889d8f 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -11,6 +11,7 @@ nav_order: 2
 ### Bugfixes
 
 - Correctly retry sending a TunnellingRequest if no TunnellingAck was received for the first time for UDP tunnelling connections.
+- Ignore repeated TunnellingRequests received from UDP tunnelling connections.
 - Properly log repeated heartbeat errors
 
 ## 1.0.0 Support for lukewarm temperatures 2022-08-13
diff --git a/xknx/io/tunnel.py b/xknx/io/tunnel.py
index fa1065ec..f3f67256 100644
--- a/xknx/io/tunnel.py
+++ b/xknx/io/tunnel.py
@@ -459,6 +459,7 @@ class UDPTunnel(_Tunnel):
             auto_reconnect=auto_reconnect,
             auto_reconnect_wait=auto_reconnect_wait,
         )
+        self.expected_sequence_number = 0
 
     def _init_transport(self) -> None:
         """Initialize transport transport."""
@@ -475,6 +476,7 @@ class UDPTunnel(_Tunnel):
             return
         (local_addr, local_port) = self.transport.getsockname()
         self.local_hpai = HPAI(ip_addr=local_addr, port=local_port)
+        self.expected_sequence_number = 0
 
     # OUTGOING REQUESTS
 
@@ -551,15 +553,33 @@ class UDPTunnel(_Tunnel):
     def _tunnelling_request_received(
         self, tunneling_request: TunnellingRequest
     ) -> None:
-        """Handle incoming tunnel request."""
-        # we should only ACK if the request matches the expected sequence number or one less
-        # we should not ACK and discard the request if the sequence number is higher than the expected sequence number
-        #   or if the sequence number lower thatn (expected -1)
-        self._send_tunnelling_ack(
-            tunneling_request.communication_channel_id,
-            tunneling_request.sequence_counter,
+        """Handle incoming tunnelling request."""
+        if tunneling_request.sequence_counter == self.expected_sequence_number:
+            self._send_tunnelling_ack(
+                tunneling_request.communication_channel_id,
+                tunneling_request.sequence_counter,
+            )
+            super()._tunnelling_request_received(tunneling_request)
+            self.expected_sequence_number = self.expected_sequence_number + 1 & 0xFF
+            return
+        if (
+            tunneling_request.sequence_counter
+            == self.expected_sequence_number - 1 & 0xFF
+        ):
+            self._send_tunnelling_ack(
+                tunneling_request.communication_channel_id,
+                tunneling_request.sequence_counter,
+            )
+            logger.debug(
+                "Received TunnellingRequest with sequence number one less than expected. "
+                "Discarding frame: %s",
+                tunneling_request,
+            )
+            return
+        logger.warning(
+            "Received TunnellingRequest with unexpected sequence number. Discarding frame: %s",
+            tunneling_request,
         )
-        super()._tunnelling_request_received(tunneling_request)
 
     def _send_tunnelling_ack(
         self, communication_channel_id: int, sequence_counter: int


Test patch:
diff --git a/test/io_tests/tunnel_test.py b/test/io_tests/tunnel_test.py
index 8b911345..3ee259fb 100644
--- a/test/io_tests/tunnel_test.py
+++ b/test/io_tests/tunnel_test.py
@@ -1,5 +1,6 @@
 """Test for KNX/IP Tunnelling connections."""
 import asyncio
+from copy import deepcopy
 from unittest.mock import AsyncMock, Mock, call, patch
 
 import pytest
@@ -58,8 +59,8 @@ class TestUDPTunnel:
             # communication_channel_id: 0x02   sequence_counter: 0x21
             bytes.fromhex("0610 0420 0017 04 02 21 00 2900bcd011162916030080 0c 3f"),
             # L_Data.ind T_Connect from 1.0.250 to 1.0.255 (xknx tunnel endpoint)
-            # communication_channel_id: 0x02   sequence_counter: 0x81
-            bytes.fromhex("0610 0420 0014 04 02 81 00 2900b06010fa10ff0080"),
+            # communication_channel_id: 0x02   sequence_counter: 0x21
+            bytes.fromhex("0610 0420 0014 04 02 21 00 2900b06010fa10ff0080"),
         ],
     )
     @patch("xknx.io.UDPTunnel._send_tunnelling_ack")
@@ -69,6 +70,7 @@ class TestUDPTunnel:
         _cemi.from_knx(raw[10:])
         telegram = _cemi.telegram
         telegram.direction = TelegramDirection.INCOMING
+        self.tunnel.expected_sequence_number = 0x21
 
         self.tunnel.transport.data_received_callback(raw, ("192.168.1.2", 3671))
         await asyncio.sleep(0)
@@ -91,6 +93,7 @@ class TestUDPTunnel:
         _cemi.from_knx(raw_req[10:])
         test_telegram = _cemi.telegram
         test_telegram.direction = TelegramDirection.INCOMING
+        self.tunnel.expected_sequence_number = 0x81
 
         response_telegram = Telegram(source_address=self.tunnel._src_address)
 
@@ -116,12 +119,55 @@ class TestUDPTunnel:
         ]
         send_ack_mock.assert_called_once_with(raw_req[7], raw_req[8])
 
+    async def test_repeated_tunnel_request(self, time_travel):
+        """Test Tunnel for receiving repeated TunnellingRequest frames."""
+        self.tunnel.transport.send = Mock()
+        self.tunnel.communication_channel = 1
+        self.tunnel.expected_sequence_number = 10
+
+        test_telegram = Telegram(payload=GroupValueWrite(DPTArray((1,))))
+        test_frame = KNXIPFrame.init_from_body(
+            TunnellingRequest(
+                communication_channel_id=1,
+                sequence_counter=10,
+                cemi=CEMIFrame.init_from_telegram(
+                    test_telegram, code=CEMIMessageCode.L_DATA_IND
+                ),
+            )
+        )
+        test_ack = KNXIPFrame.init_from_body(TunnellingAck(sequence_counter=10))
+        test_frame_9 = deepcopy(test_frame)
+        test_frame_9.body.sequence_counter = 9
+
+        # first frame - ACK and processed
+        self.tunnel._request_received(test_frame, None, None)
+        await time_travel(0)
+        assert self.tunnel.transport.send.call_args_list == [call(test_ack, addr=None)]
+        self.tunnel.transport.send.reset_mock()
+        assert self.tunnel.expected_sequence_number == 11
+        assert self.tg_received_mock.call_count == 1
+        # same sequence number as before - ACK, not processed
+        self.tunnel._request_received(test_frame, None, None)
+        await time_travel(0)
+        assert self.tunnel.transport.send.call_args_list == [call(test_ack, addr=None)]
+        self.tunnel.transport.send.reset_mock()
+        assert self.tunnel.expected_sequence_number == 11
+        assert self.tg_received_mock.call_count == 1
+        # wrong sequence number - no ACK, not processed
+        self.tunnel._request_received(test_frame_9, None, None)
+        await time_travel(0)
+        assert self.tunnel.transport.send.call_args_list == []
+        self.tunnel.transport.send.reset_mock()
+        assert self.tunnel.expected_sequence_number == 11
+        assert self.tg_received_mock.call_count == 1
+
     @patch("xknx.io.UDPTunnel._send_tunnelling_ack")
     def test_tunnel_request_received_cemi_too_small(self, send_ack_mock):
         """Test Tunnel sending ACK for unsupported frames."""
         # <UnsupportedCEMIMessage description="CEMI too small. Length: 9; CEMI: 2900b06010fa10ff00" />
         # communication_channel_id: 0x02   sequence_counter: 0x81
         raw = bytes.fromhex("0610 0420 0013 04 02 81 00 2900b06010fa10ff00")
+        self.tunnel.expected_sequence_number = 0x81
 
         self.tunnel.transport.data_received_callback(raw, ("192.168.1.2", 3671))
         self.tg_received_mock.assert_not_called()
@@ -134,6 +180,7 @@ class TestUDPTunnel:
         # <UnsupportedCEMIMessage description="APCI not supported: 0b1111111000 in CEMI: 2900b0d0000100000103f8" />
         # communication_channel_id: 0x02   sequence_counter: 0x4f
         raw = bytes.fromhex("0610 0420 0015 04 02 4f 00 2900b0d0000100000103f8")
+        self.tunnel.expected_sequence_number = 0x4F
 
         self.tunnel.transport.data_received_callback(raw, ("192.168.1.2", 3671))
         self.tg_received_mock.assert_not_called()
@@ -143,6 +190,7 @@ class TestUDPTunnel:
         """Test tunnel waits for L_DATA.con before sending another L_DATA.req."""
         self.tunnel.transport.send = Mock()
         self.tunnel.communication_channel = 1
+        self.tunnel.expected_sequence_number = 23
 
         test_telegram = Telegram(payload=GroupValueWrite(DPTArray((1,))))
         test_ack = KNXIPFrame.init_from_body(TunnellingAck(sequence_counter=23))
@@ -173,6 +221,7 @@ class TestUDPTunnel:
         self.tunnel.transport.send = Mock()
         self.tunnel.communication_channel = 1
         self.tunnel.sequence_number = 23
+        self.tunnel.expected_sequence_number = 15
 
         test_telegram = Telegram(payload=GroupValueWrite(DPTArray((1,))))
         request = KNXIPFrame.init_from_body(
@@ -192,15 +241,13 @@ class TestUDPTunnel:
         confirmation = KNXIPFrame.init_from_body(
             TunnellingRequest(
                 communication_channel_id=1,
-                sequence_counter=self.tunnel.sequence_number,
+                sequence_counter=15,
                 cemi=CEMIFrame.init_from_telegram(
                     test_telegram, code=CEMIMessageCode.L_DATA_CON
                 ),
             )
         )
-        confirmation_ack = KNXIPFrame.init_from_body(
-            TunnellingAck(sequence_counter=self.tunnel.sequence_number)
-        )
+        confirmation_ack = KNXIPFrame.init_from_body(TunnellingAck(sequence_counter=15))
 
         task = asyncio.create_task(self.tunnel.send_telegram(test_telegram))
         await time_travel(0)