From 24d45c7768ac87b78ca4b45c247dfe4e7efe03d7 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Tue, 22 Sep 2026 00:54:59 -0400 Subject: [PATCH] fix(tcp): seed `TCP.read`'s flag accumulator with `Enum_Flags(0)`, not a no-op cast * `TCP.read` seeded `_flag` with `cast('Enum_Flags', 0)`. `typing.cast` is a runtime no-op, so a segment whose flags octet is all zero left `self._flags` a plain `int` -- the `|=` below is the only thing that promotes it, and it never runs. A membership test then raised `TypeError: argument of type 'int' is not a container or iterable`, and `TCP.connection` returned an `int` where it and `Data_TCP.connection` both annotate `Flags`. Seed `Enum_Flags(0)`, matching what #597 did to the sibling accumulator in `make`. * Correct the prose that recorded the read path as deliberately keeping its `cast`: the NOTE in `make`, one NOTE in `test_tcp_udp_unit.py`, and two docstrings in `test_tcp_mptcp_join_flag_ordering_unit.py`. * Add `test_a_flagless_segment_seeds_its_connection_flags_as_an_enum`, four subtests over the flags octet, asserting the *type* -- `0 == Enum_Flags(0)`, so equality cannot discriminate -- plus that `_read_mptcp_join` now reaches its own `ProtocolError` rather than a bare `TypeError`, plus the one observable difference: a flagless segment's `connection` dumps as `'Flags::None [0]'` where it dumped as `0`. That removes a type inconsistency (number when flagless, string otherwise); the literal `None` is a separate rendering defect in `pcapkit.dumpkit.common` and is left to its own change. Coverage cannot see the fix: the line already executed, and `tcp.py` reads 100%/100% either side. Subtests over `tests/protocols/transport/` go 99 -> 103. Verified against the unfixed snapshot: the new test fails `AssertionError: is not `, exit 1; with the fix, exit 0. No caller-visible parse change -- a flagless MP_JOIN still stops at `mptcp_data_selector`'s `FieldError`, measured identical both sides. Fixes #616 --- pcapkit/protocols/transport/tcp.py | 23 ++- .../test_tcp_mptcp_join_flag_ordering_unit.py | 26 ++-- .../protocols/transport/test_tcp_udp_unit.py | 132 +++++++++++++++++- 3 files changed, 160 insertions(+), 21 deletions(-) diff --git a/pcapkit/protocols/transport/tcp.py b/pcapkit/protocols/transport/tcp.py index 0a4497b76..2043b855d 100644 --- a/pcapkit/protocols/transport/tcp.py +++ b/pcapkit/protocols/transport/tcp.py @@ -478,7 +478,19 @@ def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_TCP': ) # connection control flags - _flag = cast('Enum_Flags', 0) + # + # NOTE: ``Enum_Flags(0)``, not ``cast('Enum_Flags', 0)``. :func:`typing.cast` is a + # runtime no-op -- it returns its second argument unchanged -- so the accumulator + # used to stay the plain :class:`int` ``0`` for a segment whose flags octet is all + # zero. Nothing then promoted it, because the ``|=`` below is the only promotion and + # it never runs; ``Enum_Flags.SYN in self._flags`` raised ``TypeError: argument of + # type 'int' is not a container or iterable`` instead of answering. A segment with + # any flag set masked the defect entirely. :class:`Enum_Flags` is an + # :class:`aenum.IntFlag` and declares no ``_missing_`` of its own, so + # ``Enum_Flags(0)`` is a valid flagless member that still compares equal to ``0`` + # and still ORs as before; only the type, and hence the ``repr``, differs. That is + # what :attr:`connection` already advertises it returns. C.f. #616. + _flag = Enum_Flags(0) for key, val in schema.flags.items(): if val == 1: _flag |= Enum_Flags.get(key.upper()) @@ -580,11 +592,12 @@ def make(self, # so this keeps the newly reachable no-SYN-no-ACK case raising the library's # documented error rather than a bare Python one. :class:`Enum_Flags` is an # :class:`aenum.IntFlag`, so ``Enum_Flags(0)`` is a valid flagless member that - # still compares equal to ``0`` and still ORs as before. The read path keeps its - # ``cast`` at the top of ``read``: it cannot reach these branches, because + # still compares equal to ``0`` and still ORs as before. :meth:`read` seeds itself + # the same way; it kept the ``cast`` until #616, on the grounds that # ``mptcp_data_selector`` rejects a flagless MP_JOIN before ``_read_mptcp_join`` - # runs, and changing it would alter the ``connection`` value reported for every - # flagless parsed segment. C.f. #587. + # runs, so no caller could reach the ``TypeError``. That made the read path latent + # rather than sound -- latent by virtue of a guard in another file -- which is a + # fragile reason for a ``TypeError`` not to happen. C.f. #587, #616. _flag = Enum_Flags(0) for key, val in flags.items(): if val == 1: diff --git a/tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py b/tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py index 0e424546e..de75d24c2 100644 --- a/tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py +++ b/tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py @@ -615,13 +615,13 @@ def test_the_flags_accumulator_is_a_real_enum_member(self) -> None: self._info = self.unpack(...) # -> read(), which sets _flags again so a fully constructed instance's ``_flags`` is whatever the **read** path left, - and the read path still seeds with ``cast('Enum_Flags', 0)`` -- measured: a - flagless ``TCP(...)`` reports ``_flags`` as the plain ``int`` ``0``. That read - path is deliberately unchanged (see - :meth:`TCPMPTCPJoinReadPathUnitTests.test_a_flagless_segment_is_rejected_by_the_schema_selector`), - and it cannot affect option construction, which has already finished by then. - Observing ``make`` alone is the only way to assert on the value the option makers - actually see. + not what ``make`` computed. When this test was written the read path also seeded + with ``cast('Enum_Flags', 0)``, so a flagless ``TCP(...)`` reported ``_flags`` as + the plain ``int`` ``0``; #616 changed that seed to ``Enum_Flags(0)``, so both paths + now leave an ``Enum_Flags`` member. The re-parse still overwrites what ``make`` + assigned either way, and it cannot affect option construction, which has already + finished by then. Observing ``make`` alone is the only way to assert on the value + the option makers actually see. """ from pcapkit.const.tcp.flags import Flags as Enum_Flags @@ -675,9 +675,15 @@ def test_a_flagless_segment_is_rejected_by_the_schema_selector(self) -> None: ``mptcp_data_selector`` cannot choose an MP_JOIN schema with neither flag set, so it raises :exc:`~pcapkit.utilities.exceptions.FieldError` there. That is why - ``_read_mptcp_join``'s closing ``ProtocolError`` remains unreachable and why its - ``cast('Enum_Flags', 0)`` was deliberately left as it was -- the make path needed - the ``Enum_Flags(0)`` seed; this one does not. + ``_read_mptcp_join``'s closing ``ProtocolError`` is unreachable from a caller, and + it is still unreachable: this guard is what #616 measured as identical either side + of changing the read path's seed. That unreachability was the original reason for + leaving the seed as ``cast('Enum_Flags', 0)``, and #616's reason for changing it + anyway -- a ``TypeError`` averted only by a guard in a different file is averted + fragilely. ``read`` seeds ``Enum_Flags(0)`` now, as ``make`` has since #597, so + calling the dispatcher directly on a flagless parsed segment reaches its + ``ProtocolError`` rather than a bare ``TypeError``; see + :meth:`tests.protocols.transport.test_tcp_udp_unit.TCPUDPUnitTests.test_a_flagless_segment_seeds_its_connection_flags_as_an_enum`. """ from pcapkit.protocols.transport.tcp import TCP diff --git a/tests/protocols/transport/test_tcp_udp_unit.py b/tests/protocols/transport/test_tcp_udp_unit.py index 191a1c848..e37458397 100644 --- a/tests/protocols/transport/test_tcp_udp_unit.py +++ b/tests/protocols/transport/test_tcp_udp_unit.py @@ -992,12 +992,16 @@ def mark(schema, length: int, subtype: MPTCPOption): # dispatchers read the same attribute with the same ``in`` tests, and ``make`` is # the public entry point that assigns it, so this installs the ``Enum_Flags`` # member production assigns instead of a ``set`` that merely answers ``in``. C.f. - # #603. It has to be ``make`` and not a parsed segment for the flagless case - # further down: ``read`` still accumulates into ``cast('Enum_Flags', 0)``, a runtime - # no-op, so a flagless *parsed* instance carries a plain ``int`` and - # ``_read_mptcp_join`` raises ``TypeError`` rather than its documented - # ``ProtocolError``. That is deliberate and unreachable from a caller -- - # ``mptcp_data_selector`` rejects a flagless MP_JOIN in the schema layer first, and + # #603. Until #616 it *had* to be ``make`` rather than a parsed segment for the + # flagless case further down: ``read`` seeded its accumulator with + # ``cast('Enum_Flags', 0)``, a runtime no-op, so a flagless *parsed* instance + # carried a plain ``int`` and ``_read_mptcp_join`` raised ``TypeError`` instead of + # its documented ``ProtocolError``. #616 made ``read`` seed ``Enum_Flags(0)`` too, + # so either entry point serves now; ``make`` stays because these rows are about the + # dispatchers rather than about parsing, and + # :meth:`test_a_flagless_segment_seeds_its_connection_flags_as_an_enum` below + # covers the parsed side. Either way ``mptcp_data_selector`` rejects a flagless + # MP_JOIN in the schema layer first, and # ``tests.protocols.transport.test_tcp_mptcp_join_flag_ordering_unit`` pins both # halves against real segments. proto.make(syn=True) @@ -1523,6 +1527,122 @@ def test_unregistered_mptcp_subtype_does_not_mutate_the_class_registry(self) -> finally: registry.pop(MPTCPOption(0xF), None) + def test_a_flagless_segment_seeds_its_connection_flags_as_an_enum(self) -> None: + """A segment whose flags octet is all zero still parses to an ``Enum_Flags``. C.f. #616. + + :meth:`TCP.read ` seeded its flag + accumulator with ``cast('Enum_Flags', 0)``. :func:`typing.cast` is a runtime + no-op -- it returns its second argument unchanged -- so the accumulator began + life as the plain :class:`int` ``0``. The ``|=`` in the loop below it is the + only thing that promotes it to an + :class:`~pcapkit.const.tcp.flags.Flags` member, and that never runs when no + flag bit is set, so a flagless segment left ``self._flags`` an :obj:`int`. + ``Enum_Flags.SYN in self._flags`` then raised ``TypeError: argument of type + 'int' is not a container or iterable`` rather than answering. Any flag at all + masked it, which is why it survived a file at 100% coverage. + + The type assertions are the load-bearing ones. ``0 == Enum_Flags(0)`` is + :data:`True`, so ``assertEqual(proto.connection, 0)`` passes on both sides of + the fix and cannot discriminate -- and neither can a membership test alone, on + the flagful rows. Hence ``assertIs(type(...), Enum_Flags)``, which is what the + :attr:`~pcapkit.protocols.transport.tcp.TCP.connection` property has always + advertised it returns. + + The last block pins the consequence rather than the type. ``_read_mptcp_join`` + chooses between :rfc:`8684` section 3.2's three MP_JOIN layouts by testing + ``self._flags`` for SYN and ACK, and falls through to the library's own + ``ProtocolError`` when neither is set. On an :obj:`int` it died on a bare + :exc:`TypeError` two lines earlier instead. That branch is not reachable from + a caller -- :func:`~pcapkit.protocols.schema.transport.tcp.mptcp_data_selector` + rejects a flagless MP_JOIN with a ``FieldError`` before the dispatcher runs -- + so this was latent, but latent only by virtue of a guard in a different file. + The instance is parsed through ``TCP(...)`` rather than having ``_flags`` + written onto a bare ``object.__new__(TCP)``, for the reason #603 and #612 give + in :func:`mptcp_option` above: a hand-written attribute proves nothing about + what production puts there. + + The closing block pins the one difference a consumer can see, so that it is a + recorded decision rather than a silent change: a flagless segment's + ``connection`` now dumps as the string ``'Flags::None [0]'`` where it dumped as + the number ``0``. That is not a regression so much as the removal of an + inconsistency -- the field was a number for a flagless segment and a string for + every other one -- but the literal ``None`` in it is a rendering defect of + :func:`~pcapkit.dumpkit.common.make_dumper`'s hook, which interpolates ``o.name`` + without accounting for a nameless composite member. It would do the same to any + zero-valued flag enumeration in the library, so it is left to its own change. + + """ + import struct + + import dictdumper + + from pcapkit.const.tcp.flags import Flags as Enum_Flags + from pcapkit.const.tcp.mp_tcp_option import MPTCPOption + from pcapkit.const.tcp.option import Option + from pcapkit.dumpkit.common import make_dumper + from pcapkit.protocols.transport.tcp import TCP + from pcapkit.utilities.exceptions import ProtocolError + + def segment(flags_octet: 'int') -> 'bytes': + """A bare 20-octet header -- data offset 5, no options -- and a flags octet.""" + return struct.pack('!HHIIBBHHH', 1, 2, 0, 0, 5 << 4, flags_octet, 0, 0, 0) + + # (flags octet, the Enum_Flags the octet's low bits mean) + cases = [ + (0x00, Enum_Flags(0)), + (0x02, Enum_Flags.SYN), + (0x10, Enum_Flags.ACK), + (0x12, Enum_Flags.SYN | Enum_Flags.ACK), + ] + for octet, expected in cases: + with self.subTest(flags_octet=octet): + raw = segment(octet) + proto = TCP(raw, len(raw)) + + # The accumulator, the property, and the data field are one value. + self.assertIs(type(proto._flags), Enum_Flags) + self.assertIs(type(proto.connection), Enum_Flags) + self.assertIs(type(proto.info.connection), Enum_Flags) + self.assertEqual(proto._flags, expected) + self.assertIs(proto.connection, proto._flags) + self.assertIs(proto.info.connection, proto._flags) + + # Membership answers instead of raising -- the whole point of #616. + self.assertEqual(Enum_Flags.SYN in proto.connection, bool(octet & 0x02)) + self.assertEqual(Enum_Flags.ACK in proto.connection, bool(octet & 0x10)) + + # The numeric value is unchanged by the fix, which is exactly why the + # type is what has to be asserted. + self.assertEqual(int(proto.connection), int(expected)) + self.assertEqual(proto.connection, int(expected)) + + # The flagless segment reaches the dispatcher's own fall-through error rather + # than a bare TypeError from the SYN membership test above it. + flagless = segment(0x00) + proto = TCP(flagless, len(flagless)) + self.assertIs(type(proto._flags), Enum_Flags) + schema = DummyData(kind=Option.Multipath_TCP, subtype=MPTCPOption.MP_JOIN) + with self.assertRaisesRegex(ProtocolError, 'invalid flags combination'): + proto._read_mptcp_join(schema, options=DummyData()) # type: ignore[arg-type] + + # The one place the change is observable to a consumer, pinned here so it cannot + # drift silently. ``make_dumper``'s hook renders any enum member as + # ``Type::name [value]``, and an ``aenum.IntFlag`` pseudo-member carrying no bits + # has ``name is None`` -- so a flagless segment dumps as the string + # ``'Flags::None [0]'`` where it used to dump as the number ``0``. Note what that + # replaced: ``connection`` was a JSON *number* for a flagless segment and a + # *string* for every other one, so the field is consistently typed now rather than + # switching type with the flags. The literal ``None`` is a rendering defect in + # ``pcapkit.dumpkit.common`` -- it reads ``o.name`` without accounting for a + # nameless composite member, and would do the same to any zero-valued flag enum in + # the library -- so it is left to its own change rather than fixed from here. + # ``self`` is reached only by the fallback ``super()`` call at the end of the hook, + # which an enum never gets to, so the unbound form needs no dumper instance. + self.assertIsNone(Enum_Flags(0).name) + hook = make_dumper(dictdumper.JSON).object_hook + self.assertEqual(hook(None, proto.info.connection), 'Flags::None [0]') + self.assertEqual(hook(None, Enum_Flags.ACK), 'Flags::ACK [2048]') + if __name__ == '__main__': unittest.main()