[Detail Bug] KNX/IP TCP tunnelling: a malformed frame causes subsequent coalesced frames to be silently dropped

Detail Bug Report

https://app.detail.dev/org_62aa40f5-2c23-4914-a665-3bb2068af20e/bugs/bug_7c3cb8ce-1321-48ff-b6b9-0208e85afd11

Introduced in 00999b7091926a0f5eb5211b383aa7392dd5da17 by @kewde on Jul 27, 2026

Summary

• Context: _client_data_received in apps/knx-gui/src/knx_gui/knxip_tunnelling_gateway.py is the TCP framing layer that pulls KNX/IP frames out of the byte stream from the single connected tunnelling client and dispatches each one to its per-frame handler.
• Bug: The broad except Exception branch — present so that one malformed/unfamiliar frame does not tear down the TCP tunnel — silently discards the entire receive buffer on a parse error: the previously-buffered prefix, the rest of the offending segment, and any well-formed frames that followed the bad one in the same data_received callback. No length-based stream re-synchronisation is attempted, so a bad frame followed by a good frame in one TCP segment loses the good frame.
• Actual vs. expected: The inline comment's literal claim — "not tear down the whole connection over one bad/unfamiliar frame" — is satisfied (the TCP socket stays open). The bug is an un-commented latent side-effect of the simple return: the broad-except keeps the connection alive but, instead of skipping the single bad frame and continuing the while data: loop, it discards every frame after the bad one in the segment. There is no documented guarantee being violated; the comment promises only to keep the TCP transport up, and that is what the code does. The trailing-frame drop is a behaviour improvement opportunity — skip-and-resync over drop-and-return — not a contract breach.
• Impact: Bounded and rare in practice. The gateway acts on (has side effects for) exactly one body a tunnelling client sends unsolicited in steady state: TUNNELLING_REQUEST (_cemi_count += 1, _on_cemi/_forward_cemi). Every other dispatched body is request/response, which a strictly conformant client emits only after receiving the gateway's reply to the previous frame — i.e. by construction the bug-free split path. The broad-except fires before _handle_body, so the gateway sends no reply to the offending frame; a conformant client therefore waits for its response timeout before sending anything else, landing the bad frame and a following frame in separate data_received callbacks by construction. The trigger thus requires a non-conformant or bursting client that pipelines a frame without waiting for the (here-absent) reply, plus TCP coalescing collapsing the two frames into one data_received. The offending (unfamiliar) frame is lost by design; that is the broad-except's intended behaviour. The bug is the loss of the following good frame(s) in the same segment. Severity: low — the mechanism is real and the fix is small and correct, but the trigger requires a non-conformant pipelining client, the maximum loss per offending segment is bounded to the trailing frames in that one segment, and there is no persistent desync (§4).

Code with Bug

def _client_data_received(self, data: bytes) -> None:
    if self._logger:
        self._logger.debug("tcp data received", hex=data.hex(" "))
    if self._buffer:
        data = self._buffer + data
        self._buffer = b""                         # <-- prefix cleared up-front
    while data:
        try:
            frame, rest = KNXIPFrame.from_knx(data)
        except IncompleteKNXIPFrame:
            self._buffer = data                    # correct: re-buffer the partial tail
            return
        except Exception as e:
            # Catch broadly, not just the documented CouldNotParseKNXIP:
            # xknx's own frame parsing (e.g. an unrecognized SRP type
            # in SearchRequestExtended) can raise a plain ValueError
            # instead, which would otherwise tear down the whole
            # connection over one bad/unfamiliar frame.
            if self._logger:
                self._logger.warning(
                    "could not parse frame", error=str(e), hex=data.hex(" ")
                )
            return                                 # <-- BUG 🔴 drops bad frame *and* all trailing good frames in this TCP segment
        ...
        self._handle_body(frame.body)
        data = rest

Explanation

• When KNXIPFrame.from_knx(data) raises any exception other than IncompleteKNXIPFrame, the broad except Exception logs and immediately returns.
• At that point data still contains the unparsed bytes for the current segment (bad frame plus any subsequent frames coalesced by TCP), and _buffer has already been cleared (if it existed). The return therefore discards:
  • any previously buffered prefix,
  • the offending frame,
  • and any subsequent well-formed frames that arrived in the same data_received callback.
• This was reproduced with real handler behavior: after a CONNECT_REQUEST, sending a coalesced bad_frame + TUNNELLING_REQUEST results in _cemi_count staying at 0 (no _on_cemi call), while sending the same two frames split across separate data_received calls increments _cemi_count to 1.
• The issue is not persistent desync: subsequent clean frames received in later callbacks parse normally; the loss is bounded to frames in (or straddling) the segment where the parse error occurs.

Recommended Fix

The fix is a length-based re-sync that skips the single bad frame when the KNX/IP header was well-formed enough to yield a usable total_length.

except Exception as e:
    if self._logger:
        self._logger.warning("could not parse frame", error=str(e), hex=data.hex(" "))
    # Re-sync: skip just the bad frame when its KNX/IP header was
    # well-formed enough to give us a usable total_length. This
    # covers the forward-compat case (unknown KNXIPServiceType,
    # wrong protocol version, body ValueError on an unknown enum)
    # -- the "unfamiliar frame" group the broad-except exists for.
    # `KNXIPHeader.from_knx` raises CouldNotParseKNXIP *before*
    # setting total_length only when data[0] != 0x06 (genuine
    # stream corruption); that case keeps the existing drop-and-
    # return behaviour unchanged.
    if len(data) >= 6 and data[0] == 0x06:
        total_length = data[4] * 256 + data[5]
        if 6 <= total_length <= len(data):
            data = data[total_length:]
            continue
    # No usable total_length: either genuine stream corruption
    # (data[0] != 0x06) or a declared total_length that exceeds
    # the bytes actually received. Do not re-buffer -- the bytes
    # are not a known-good frame prefix and re-buffering them
    # would misalign the next segment. Drop the rest of this
    # segment (as today) and keep the transport open (as today).
    return

History

This bug was introduced in commit 00999b7. That commit rewrote the proxy from a UDP/multicast RoutingProxy (which had no TCP byte-stream framing) into a TCP-based KNXnet/IP tunnelling server, adding the new _client_data_received framing loop in its entirety; the broad except Exception: return was intended to keep the TCP transport up over one unfamiliar frame, but it slipped in by clearing self._buffer up-front and returning without any length-based re-sync, silently dropping any well-formed frames trailing the bad one in the same data_received callback. The two later commits that touched the file (fba940f splitting the proxy into its own plugin, and 97b7511 extracting the gateway into knxip_tunnelling_gateway.py) only moved or renamed the buggy block verbatim — they did not modify the framing logic.

Start in apps/knx-gui/src/knx_gui/knxip_tunnelling_gateway.py at _client_data_received and review the framing loop and its broad parse-error branch. Reproduce the coalesced malformed-frame plus TUNNELLING_REQUEST scenario described in the issue, then verify the malformed frame is discarded, the trailing valid frame is handled, and the TCP connection remains open.