From 77800615b76a2a82984f652015fd496e2fdcef7b Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 6 Oct 2026 23:33:11 -0400 Subject: [PATCH] fix(l2tp): L2TPv2.from_data drops the L flag and the Length field `_make_data` returned the parsed Length under `length`, which `ProtocolBase.__init__` consumes as the parse length, so it never reached `make()` and the rebuild cleared `L` and omitted the Length octets. `make()` now takes it as `total_length`, as #1166 did for IPv4 and UDP, and `_make_data` passes it under that key. `test_link_unit` called `make(length=20)` directly and is updated to the new keyword. All 24 T/L/S/O(+pad) combinations now round-trip. Closes #1172 --- pcapkit/protocols/link/l2tpv2.py | 15 ++- .../link/test_l2tpv2_length_roundtrip_unit.py | 107 ++++++++++++++++++ tests/protocols/link/test_link_unit.py | 2 +- 3 files changed, 118 insertions(+), 6 deletions(-) create mode 100644 tests/protocols/link/test_l2tpv2_length_roundtrip_unit.py diff --git a/pcapkit/protocols/link/l2tpv2.py b/pcapkit/protocols/link/l2tpv2.py index c440699739..5ab40853e0 100644 --- a/pcapkit/protocols/link/l2tpv2.py +++ b/pcapkit/protocols/link/l2tpv2.py @@ -245,7 +245,7 @@ def make(self, type_namespace: 'Optional[dict[str, int] | dict[int, str] | Type[StdlibEnum] | Type[AenumEnum]]' = None, # pylint: disable=line-too-long type_reversed: 'bool' = False, priority: 'bool' = False, - length: 'Optional[int]' = None, + total_length: 'Optional[int]' = None, tunnel_id: 'int' = 0, session_id: 'int' = 0, ns: 'Optional[int]' = None, @@ -262,7 +262,8 @@ def make(self, type_namespace: Namespace of type. type_reversed: Reversed namespace of type. priority: Priority flag. - length: Length of packet data. + total_length: Length of the datagram, header included; the Length + field and its ``L`` flag are omitted when this is :data:`None`. tunnel_id: Tunnel ID. session_id: Session ID. ns: Sequence number. @@ -281,13 +282,13 @@ def make(self, return Schema_L2TP( flags={ 'type': type_, - 'len': length is not None, + 'len': total_length is not None, 'seq': ns is not None and nr is not None, 'offset': offset is not None, 'prio': priority, 'version': version, }, - length=length, + length=total_length, tunnel_id=tunnel_id, session_id=session_id, ns=ns, @@ -364,7 +365,11 @@ def _make_data(cls, data: 'Data_L2TP') -> 'dict[str, Any]': # type: ignore[over 'type': data.flags.type, 'priority': data.flags.prio, 'version': data.version, - 'length': data.length, + # NOTE: Passed as ``total_length`` since ``length`` is the parse + # length :class:`~pcapkit.protocols.protocol.ProtocolBase` takes, so + # under that key it never reached :meth:`make`, and the rebuild + # dropped both the Length field and the ``L`` flag (:issue:`1172`). + 'total_length': data.length, 'tunnel_id': data.tunnelid, 'session_id': data.sessionid, 'ns': data.ns, diff --git a/tests/protocols/link/test_l2tpv2_length_roundtrip_unit.py b/tests/protocols/link/test_l2tpv2_length_roundtrip_unit.py new file mode 100644 index 0000000000..2c981cb697 --- /dev/null +++ b/tests/protocols/link/test_l2tpv2_length_roundtrip_unit.py @@ -0,0 +1,107 @@ +# -*- coding: utf-8 -*- +"""L2TPv2 keeps the ``L`` flag and the Length field through ``from_data``. + +GitHub issue #1172: :meth:`L2TPv2._make_data +` returned the parsed Length +under ``length``, which is the parse length +:class:`~pcapkit.protocols.protocol.ProtocolBase` takes for itself, so it never +reached :meth:`~pcapkit.protocols.link.l2tpv2.L2TPv2.make`. The rebuild then +cleared ``L`` and omitted the two Length octets: ``c802000c0001000000010002`` +came back as ``88020001000000010002``. + +Every case builds its own octets in memory and reads no capture. + +:class:`L2TPv2` is imported inside each test, after +:func:`~tests._support.reimport_once_per_class`, not at module load, for the +reason :mod:`tests.protocols.link.test_ethernet_mac_roundtrip_unit` gives. + +""" + +import io +import itertools +import unittest + +from tests._support import reimport_once_per_class + +#: A PPP frame header, as the L2TPv2 payload. +PAYLOAD = bytes.fromhex('ff030021') + + +def build(type_: int, len_: int, seq: int, offset: int, pad: int) -> bytes: + """Build an L2TPv2 datagram with the given optional fields present.""" + flags = (type_ << 15) | (len_ << 14) | (seq << 11) | (offset << 9) | 2 + hdr_len = 6 + 2 * (len_ + 2 * seq + offset) + pad + body = b'' + if len_: + body += (hdr_len + len(PAYLOAD)).to_bytes(2, 'big') + body += (7).to_bytes(2, 'big') + (9).to_bytes(2, 'big') + if seq: + body += (3).to_bytes(2, 'big') + (4).to_bytes(2, 'big') + if offset: + body += pad.to_bytes(2, 'big') + bytes(pad) + return flags.to_bytes(2, 'big') + body + PAYLOAD + + +#: Every combination of ``T``, ``L``, ``S`` and ``O``; with ``O``, both an +#: empty and a three-octet (zero) offset pad. +COMBINATIONS = [ + (t, l, s, o, pad) + for t, l, s, o in itertools.product((0, 1), repeat=4) + for pad in ((0, 3) if o else (0,)) +] + + +class TestL2TPv2LengthRoundTrip(unittest.TestCase): + """Pin that the Length field survives a ``from_data`` rebuild.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + def test_issue_reproduction(self) -> None: + from pcapkit.protocols.link.l2tpv2 import L2TPv2 + + raw = bytes.fromhex('c802000c0001000000010002') # ZLB ACK: T, L and S set + parsed = L2TPv2(raw) + self.assertTrue(parsed.info.flags.len) + self.assertEqual(parsed.info.length, 12) + self.assertEqual(L2TPv2.from_data(parsed.info).data, raw) + + def test_every_flag_combination_round_trips(self) -> None: + from pcapkit.protocols.link.l2tpv2 import L2TPv2 + + self.assertEqual(len(COMBINATIONS), 24) + for t, l, s, o, pad in COMBINATIONS: + raw = build(t, l, s, o, pad) + with self.subTest(T=t, L=l, S=s, O=o, pad=pad, raw=raw.hex()): + parsed = L2TPv2(io.BytesIO(raw), len(raw)) + self.assertEqual(L2TPv2.from_data(parsed.info).data, raw) + + def test_make_data_passes_the_length_as_total_length(self) -> None: + from pcapkit.protocols.link.l2tpv2 import L2TPv2 + + values = L2TPv2._make_data(L2TPv2(build(1, 1, 1, 0, 0)).info) + self.assertNotIn('length', values) + self.assertEqual(values['total_length'], 16) + + values = L2TPv2._make_data(L2TPv2(build(0, 0, 0, 0, 0)).info) + self.assertIsNone(values['total_length']) + + def test_make_sets_l_flag_only_with_total_length(self) -> None: + from pcapkit.protocols.link.l2tpv2 import L2TPv2 + + # ``type`` is passed as the raw ``T`` bit so the octets do not depend on + # how :class:`~pcapkit.const.l2tp.type.Type` names it. + with_len = L2TPv2(type=0, tunnel_id=7, session_id=9, total_length=12, + payload=PAYLOAD) + self.assertEqual(with_len.data, build(0, 1, 0, 0, 0)) + self.assertTrue(with_len.info.flags.len) + self.assertEqual(with_len.info.length, 12) + + without = L2TPv2(type=0, tunnel_id=7, session_id=9, payload=PAYLOAD) + self.assertEqual(without.data, build(0, 0, 0, 0, 0)) + self.assertFalse(without.info.flags.len) + self.assertIsNone(without.info.length) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/link/test_link_unit.py b/tests/protocols/link/test_link_unit.py index 96e9fee6b7..d541a476c0 100644 --- a/tests/protocols/link/test_link_unit.py +++ b/tests/protocols/link/test_link_unit.py @@ -678,7 +678,7 @@ def test_l2tp_properties_read_and_make_variants(self) -> None: schema = maker.make( type=Type.Control, priority=True, - length=20, + total_length=20, tunnel_id=7, session_id=8, ns=9,