diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e41571ed..595c1a77e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,8 @@ This is the resolution of #548, which reported `TransType.L2TP` (115) as registe - **Fixed** -- `httpv1`'s `_RE_METHOD` was unanchored and `re.match` anchors only at the start, so it prefix-matched, and the request-line reader then passed the whole `para1` to `Method.get` rather than the captured `method` group. Together those meant `b'Get'` matched on the single character `G`, satisfied the guard that decides a start-line is a request, and handed the entire mixed-case token to a lookup that raised on it. Fixing either half alone still gives a wrong answer -- normalising the lookup would parse `b'Get'` as `GET` off a one-character match, and passing the group would parse it as a method named `G`. The pattern is now anchored at both ends and the captured group is what is looked up, so a token that is not a method is a malformed request line rather than a mis-parsed one. Method tokens are case-sensitive per [RFC 9110 Section 9.1](https://datatracker.ietf.org/doc/html/rfc9110#section-9.1), so no `re.I` was added: `GET` parses, `Get` and `get` are rejected (#583). - **Fixed** -- `_RE_STATUS` in the same reader carried the same unanchored prefix defect, found by auditing `_RE_METHOD`'s siblings, and it escaped as the wrong exception type. That pattern is only a guard -- the value is taken from `int(para2)` on the raw token -- so a prefix match let a malformed status past the guard and then out of `int()` uncaught, where `_read_http_header` documents `ProtocolError`. Measured: a status of `200x` raised `ValueError: invalid literal for int() with base 10: b'200x'`, and one of `2000` raised `ValueError: 2000 is not a valid StatusCode`; both are now `ProtocolError`. [RFC 9112 Section 4](https://datatracker.ietf.org/doc/html/rfc9112#section-4) gives `status-code = 3DIGIT`, exactly three, so the anchor is what the grammar already said -- the production lives in HTTP/1.1 because `status-code` is part of its `status-line`, while [RFC 9110 Section 15](https://datatracker.ietf.org/doc/html/rfc9110#section-15) covers the code semantics and the IANA registry rather than the syntax. `_RE_VERSION` was audited at the same time and is safe as it stands, because both of its call sites read the captured group rather than the raw token (#583). - **Fixed** -- `get()`'s documented `default` was ignored on the integer path throughout the generated `pcapkit.const` tree, because `get` delegated the lookup to the enum call and `_missing_` has no access to the caller's `default` -- so `Hardware.get(99999, 0)` raised `ValueError: 99999 is not a valid Hardware` instead of returning the fallback it was handed. The integer path now consults `default` before letting the lookup error escape. `-1`, the placeholder the generated signature already carried, is what separates "no default was supplied" from "a default was supplied and should be used", so a caller that asked for no fallback still gets the error rather than a silent substitution. The sweep #584 asked for puts the scope at 110 of the 118 integer registries, not the three the issue named; the two carrying a bespoke integer fallback of their own, `pcapng` `OptionType` and `reg` `AppType`, are deliberately left alone, since neither drops a default by raising. Not reachable from wire data -- every value a wire field can carry already resolves -- so this is a contract fix rather than a parse fix. Applied to the nine vendor templates as well as the 113 generated modules, and a new test renders the shared template and compares it against the module generated from it, so a regeneration cannot quietly undo it (#584). +- **Changed** -- `tests/protocols/transport/test_tcp_udp_unit.py` now reaches the MP_JOIN dispatchers through `TCP()` itself, instead of assigning a Python `set` to `_flags` on a bare `TCP.__new__(TCP)`. A `set` answers the membership tests `_make_mptcp_join` and `_read_mptcp_join` use, so every flag branch ran and both TCP modules read 100% statement and branch coverage -- while the attribute had neither the `aenum.IntFlag` type production assigns nor the ordering that governs when it exists at all, which is how #587 stayed invisible behind that number and how the `cast('Enum_Flags', 0)` no-op behind it went unnoticed too. Measured on the rewrite, against the 17 tests of that file: revert #587's hoist and two of them fail with `AttributeError: 'TCP' object has no attribute '_flags'` where all 17 passed before; restore the `cast` and two fail with `TypeError: argument of type 'int' is not a container or iterable`, again where all 17 passed. The library is unchanged and the file's tests still pass, so the coverage numbers do not move -- the point is what the same numbers are now worth (#603). +- **Fixed** -- documentation. `mptcp_dss_ack_selector`'s note said a corrected field-width lambda "would not have worked" and that fixing it belonged to `pcapkit.corekit.fields.numbers`, which is exactly where #598 then fixed it; the same paragraph sat in `test_tcp_mptcp_length_arithmetic_unit.py`'s module docstring, whose other stale claim was that MP_JOIN "cannot be built through the public `TCP()` constructor at all", true only until #587. A callable-length `NumberField` packs and unpacks both DSS widths now, and wire *absence* was never the obstacle either: `MPTCPDSS.ssn`, `dl_len` and `checksum` have always been `ConditionalField` on the sibling `M` flag, so the class already relied on that wrapper to keep a field off the wire. The `SwitchField` form is kept for the narrower reason the note now gives -- `ConditionalField`'s `length` forwards to the wrapped field without consulting the condition, so it is safe here only because `Schema.pack` and `Schema.unpack` special-case that wrapper by name, whereas a `SwitchField` always resolves to a concrete field. Replacing it would be a behaviour change and is not made (#603). Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) and `1.5.0b3` (2026-09-19), all published as prereleases and so resolved only by `pip install --pre`. `1.5.0b1` half-shipped: the tag, the GitHub release and the Conda deployments landed, but PyPI rejected the wheel because `twine check` found a Sphinx-only `:mod:` role in `README.rst`, which `pyproject.toml` declares as the dynamic long description. `1.5.0b2` is what reshipped it -- the release workflow is version-driven, so an existing version cannot republish -- and `1.5.0b3` followed the CI change that stops a TestPyPI outage from costing a release its wheels (#497, #498). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 3e039b554..efdc2ff06 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -814,6 +814,37 @@ pull requests between #326 and #509. well as the 113 generated modules, and a new test renders the shared template and compares it against the module generated from it, so a regeneration cannot quietly undo it (#584). +* **Changed** -- ``tests/protocols/transport/test_tcp_udp_unit.py`` now reaches the + MP_JOIN dispatchers through ``TCP()`` itself, instead of assigning a Python + ``set`` to ``_flags`` on a bare ``TCP.__new__(TCP)``. A ``set`` answers the + membership tests ``_make_mptcp_join`` and ``_read_mptcp_join`` use, so every flag + branch ran and both TCP modules read 100% statement and branch coverage -- while + the attribute had neither the ``aenum.IntFlag`` type production assigns nor the + ordering that governs when it exists at all, which is how #587 stayed invisible + behind that number and how the ``cast('Enum_Flags', 0)`` no-op behind it went + unnoticed too. Measured on the rewrite, against the 17 tests of that file: revert + #587's hoist and two of them fail with + ``AttributeError: 'TCP' object has no attribute '_flags'`` + where all 17 passed before; restore the ``cast`` and two fail with + ``TypeError: argument of type 'int' is not a container or iterable``, again + where all 17 passed. The library is unchanged and the file's tests still pass, so + the coverage numbers do not move -- the point is what the same numbers are now + worth (#603). +* **Fixed** -- documentation. ``mptcp_dss_ack_selector``'s note said a corrected + field-width lambda "would not have worked" and that fixing it belonged to + ``pcapkit.corekit.fields.numbers``, which is exactly where #598 then fixed it; the + same paragraph sat in ``test_tcp_mptcp_length_arithmetic_unit.py``'s module + docstring, whose other stale claim was that MP_JOIN "cannot be built through the + public ``TCP()`` constructor at all", true only until #587. A callable-length + ``NumberField`` packs and unpacks both DSS widths now, and wire *absence* was + never the obstacle either: ``MPTCPDSS.ssn``, ``dl_len`` and ``checksum`` have + always been ``ConditionalField`` on the sibling ``M`` flag, so the class already + relied on that wrapper to keep a field off the wire. The ``SwitchField`` form is + kept for the narrower reason the note now gives -- ``ConditionalField``'s + ``length`` forwards to the wrapped field without consulting the condition, so it + is safe here only because ``Schema.pack`` and ``Schema.unpack`` special-case that + wrapper by name, whereas a ``SwitchField`` always resolves to a concrete field. + Replacing it would be a behaviour change and is not made (#603). Preceded by ``1.5.0a1`` (2026-09-15), ``1.5.0b1`` and ``1.5.0b2`` (both 2026-09-18) and ``1.5.0b3`` (2026-09-19), all published as prereleases and so diff --git a/pcapkit/protocols/schema/transport/tcp.py b/pcapkit/protocols/schema/transport/tcp.py index 6164dd0c7..87d035e8f 100644 --- a/pcapkit/protocols/schema/transport/tcp.py +++ b/pcapkit/protocols/schema/transport/tcp.py @@ -256,7 +256,7 @@ def mptcp_dss_ack_selector(pkt: 'dict[str, Any]') -> 'Field': This is a :class:`~pcapkit.corekit.fields.misc.SwitchField` selector rather than a :class:`~pcapkit.corekit.fields.misc.ConditionalField` wrapping ``NumberField(length=lambda pkt: ...)``, which is what it was - until #576, for two independent reasons. + until #576. The width lambda read ``8 if pkt['flags']['a'] else 0`` -- **0**, not 4 -- so an unextended Data ACK packed no octets at all while the ``length`` @@ -265,20 +265,44 @@ def mptcp_dss_ack_selector(pkt: 'dict[str, Any]') -> 'Field': declared, and the ``ack`` value the caller supplied was simply not present. - Correcting the lambda to ``8 if ... else 4`` would not have worked, - because :class:`~pcapkit.corekit.fields.numbers.NumberField` cannot pack - a callable length at all: it calls ``build_template`` once at - ``__init__`` with the placeholder length ``-1``, which latches - ``_need_process = True``, and nothing clears that flag when - ``__call__`` later resolves the real length and rebuilds the template as - ``>I``/``>Q``. ``pre_process`` then hands :func:`struct.pack` bytes for - an integer template and it raises ``struct.error: required argument is - not an integer``. Measured on the 8-octet form, which the old lambda did - reach: ``_make_mptcp_dss(DSS, ack=1 << 40)`` raised exactly that. Fixing - that belongs to :mod:`pcapkit.corekit.fields.numbers`; selecting between - two fields that each fix ``__template__`` at class level sidesteps it - entirely and is the pattern :func:`mptcp_add_address_selector` already - uses here. + Correcting the lambda to ``8 if ... else 4`` would not have worked *at the + time*, because :class:`~pcapkit.corekit.fields.numbers.NumberField` could + not pack a callable length at all: it called ``build_template`` once at + ``__init__`` with the placeholder length ``-1``, which latched + ``_need_process = True``, and nothing cleared that flag when ``__call__`` + later resolved the real length and rebuilt the template as ``>I``/``>Q``. + ``pre_process`` then handed :func:`struct.pack` bytes for an integer + template and it raised ``struct.error: required argument is not an + integer``. Measured on the 8-octet form, which the old lambda did reach: + ``_make_mptcp_dss(DSS, ack=1 << 40)`` raised exactly that. + + That half is now history: **#598 fixed it**, in + :mod:`pcapkit.corekit.fields.numbers` where this note used to say the fix + belonged, by recomputing ``_need_process`` from the width actually in + force instead of once from the placeholder. A callable-length + ``NumberField`` packs and unpacks both the 4- and the 8-octet form today, + so ``ConditionalField(NumberField(length=...), lambda pkt: + pkt['flags']['A'])`` would express this field correctly. Nor was wire + *absence* ever the obstacle: :attr:`MPTCPDSS.ssn`, :attr:`MPTCPDSS.dl_len` + and :attr:`MPTCPDSS.checksum` are each a ``ConditionalField`` on the + sibling ``M`` flag, so this very class already leans on that wrapper to + keep a field off the wire. + + The ``SwitchField`` form is kept anyway, for a narrower reason about + composition rather than about absence. A + :class:`~pcapkit.corekit.fields.misc.ConditionalField`'s ``length`` + forwards to the wrapped field unconditionally, never consulting the + condition, so reading it while the condition is false -- the wrapped field + then still unresolved, at its ``-1`` placeholder -- raises + ``struct.error: bad char in struct format``. Nothing here meets that only + because :class:`Schema + `'s ``pack`` and ``unpack`` + special-case ``ConditionalField`` by name and skip the wrapped field + outright before any ``length`` is read. A ``SwitchField`` needs no such + special case: its selector always hands back an already-concrete field, + :class:`~pcapkit.corekit.fields.misc.NoValueField` included, so its + ``length`` is safe wherever it is read. Swapping the two would be a + behaviour change, not a tidy-up, and #603 does not make it. """ if not pkt['flags']['A']: @@ -308,8 +332,10 @@ def mptcp_dss_dsn_selector(pkt: 'dict[str, Any]') -> 'Field': Note: Identical in shape to :func:`mptcp_dss_ack_selector`, and it replaces the identical defect: ``NumberField(length=lambda pkt: 8 if pkt['flags']['m'] - else 0, ...)``. See that function's note for why the ``0`` was wrong and - why a corrected lambda would not have packed either. C.f. #576. + else 0, ...)``. See that function's note for why the ``0`` was wrong, why a + corrected lambda would not have packed either *at the time*, and why the + ``SwitchField`` form is kept now that #598 has made a callable length work. + C.f. #576, #598. """ if not pkt['flags']['M']: @@ -881,7 +907,7 @@ class MPTCPDSS(MPTCP, code=Enum_MPTCPOption.DSS): #: 4 octets when ``A`` is set, 8 when ``a`` is set as well, absent otherwise -- #: :rfc:`8684` section 3.3 figure 9. Both the presence test and the width live #: in :func:`mptcp_dss_ack_selector`, whose note records what this field - #: declared until #576 and why a narrower fix would not have packed. + #: declared until #576 and why the switch form is kept. ack: 'int' = SwitchField( selector=mptcp_dss_ack_selector, ) diff --git a/tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py b/tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py index 3b7e1b7a4..7b4fa8b87 100644 --- a/tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py +++ b/tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py @@ -79,19 +79,25 @@ the generator's own override arguments: 12 octets packed against a declared 20. A second defect sat behind the first: correcting the lambda to ``8 if ... else 4`` would still -not have packed, because :class:`~pcapkit.corekit.fields.numbers.NumberField` cannot pack a -callable length at all -- it calls ``build_template`` once at ``__init__`` with the placeholder -length ``-1``, latching ``_need_process = True``, and nothing clears that when ``__call__`` -later resolves the real length and rebuilds the template as ``>I``/``>Q``. Measured on the -8-octet form, which the old lambda did reach: ``_make_mptcp_dss(DSS, ack=1 << 40)`` raised -``struct.error: required argument is not an integer``. That belongs to -:mod:`pcapkit.corekit.fields.numbers` and is not fixed here; the schema instead selects between +not have packed *at the time*, because :class:`~pcapkit.corekit.fields.numbers.NumberField` +could not pack a callable length at all -- it called ``build_template`` once at ``__init__`` +with the placeholder length ``-1``, latching ``_need_process = True``, and nothing cleared that +when ``__call__`` later resolved the real length and rebuilt the template as ``>I``/``>Q``. +Measured on the 8-octet form, which the old lambda did reach: ``_make_mptcp_dss(DSS, ack=1 << +40)`` raised ``struct.error: required argument is not an integer``. #576 left that to +:mod:`pcapkit.corekit.fields.numbers`, and **#598 has since fixed it** by recomputing +``_need_process`` from the width in force rather than once from the placeholder, so a callable +length packs and unpacks both widths today. The schema keeps selecting between :class:`~pcapkit.corekit.fields.numbers.UInt32Field` and :class:`~pcapkit.corekit.fields.numbers.UInt64Field`, which each fix ``__template__`` at class level, through a :class:`~pcapkit.corekit.fields.misc.SwitchField` -- the pattern :func:`~pcapkit.protocols.schema.transport.tcp.mptcp_add_address_selector` already uses in that -module. :class:`TCPMPTCPDSSExtendedFieldsUnitTests` covers the 8-octet forms that could not be -packed before at all. +module -- for the narrower reason recorded in +:func:`~pcapkit.protocols.schema.transport.tcp.mptcp_dss_ack_selector`'s own note, which is +about :class:`~pcapkit.corekit.fields.misc.ConditionalField`'s condition-blind ``length`` and +not about wire absence. Swapping the two would be a behaviour change and is not made here. +:class:`TCPMPTCPDSSExtendedFieldsUnitTests` covers the 8-octet forms that could not be packed +before at all. MP_JOIN-SYN is not a defect either ----------------------------------- @@ -119,13 +125,21 @@ the blind spot :mod:`tests.protocols.test_option_roundtrip_unit`'s own docstring names ("a defect can leave the cycle closed"). -MP_JOIN cannot be built through the public ``TCP()`` constructor at all -- ``_make_mptcp_join`` -dispatches on ``self._flags``, which ``TCP._make`` assigns *after* it has already built the -options, so construction raises ``AttributeError: 'TCP' object has no attribute '_flags'``. -That is a separate, already-recorded gap (``tcp-mptcp/MP_JOIN`` in -:data:`tests.protocols.test_option_roundtrip_unit.EXPECTED_FAILURES`, with exactly that -diagnosis) and is not touched here, so the MP_JOIN classes drive the makers directly and reach -the readers by parsing bytes, where ``_flags`` *is* set. +When this module was written MP_JOIN could not be built through the public ``TCP()`` +constructor at all -- ``_make_mptcp_join`` dispatches on ``self._flags``, which ``TCP.make`` +assigned *after* it had already built the options, so construction raised ``AttributeError: +'TCP' object has no attribute '_flags'``. That was a separate, already-recorded gap +(``tcp-mptcp/MP_JOIN`` in :data:`tests.protocols.test_option_roundtrip_unit.EXPECTED_FAILURES`, +with exactly that diagnosis) and was left alone here, which is why the MP_JOIN classes drive +the makers directly and reach the readers by parsing bytes, where ``_flags`` *is* set. + +**#587 has since hoisted that assignment above the option build**, so the constructor route is +open now and its ``EXPECTED_FAILURES`` entry is gone; +:mod:`tests.protocols.transport.test_tcp_mptcp_join_flag_ordering_unit` covers all three +layouts through ``TCP()`` proper. The classes here are still written against the makers, which +is what keeps them a check on the *length arithmetic* of each form rather than on the dispatch, +so they are left as they are -- but the reason is now choice rather than impossibility. C.f. +#587, #603. """ from __future__ import annotations diff --git a/tests/protocols/transport/test_tcp_udp_unit.py b/tests/protocols/transport/test_tcp_udp_unit.py index 1efa219d7..ad5229d64 100644 --- a/tests/protocols/transport/test_tcp_udp_unit.py +++ b/tests/protocols/transport/test_tcp_udp_unit.py @@ -6,10 +6,14 @@ import importlib.util import types import unittest +from typing import TYPE_CHECKING from unittest import mock from tests._support import purge_modules +if TYPE_CHECKING: + from typing import Any + RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) @@ -18,6 +22,50 @@ class DummyData(dict): __getattr__ = dict.__getitem__ +def mptcp_option(opt: 'Any' = None, *, syn: 'bool' = False, ack: 'bool' = False, + **kwargs: 'Any') -> 'Any': + """Build one MPTCP option the way a caller builds it, and hand back its schema. + + The MP_JOIN makers dispatch on ``self._flags`` to pick between the three layouts of + :rfc:`8684` section 3.2, and :meth:`TCP.make + ` is what assigns that attribute -- as an + :class:`aenum.IntFlag` member, resolved *before* ``_make_tcp_options`` runs. + + Until #603 the MP_JOIN cases here wrote a plain :obj:`set` onto the attribute of a bare + ``object.__new__(TCP)`` instead. A :obj:`set` answers the ``in`` tests the dispatchers + use, so every branch ran and both TCP modules read 100% statement and branch coverage + -- while the attribute had neither the type nor the provenance production gives it. That + is how #587, an ordering defect that broke MP_JOIN construction for every caller, sat + behind that coverage number untouched, and how the ``cast('Enum_Flags', 0)`` no-op + behind it went unnoticed too. Going through ``TCP()`` means an ordering or type defect + fails a test here instead of passing one. + + Each call constructs a **fresh** instance, because the point is that ``_flags`` is + resolved from these very arguments rather than left over from an earlier call. + + Args: + opt: A ``pcapkit.protocols.data.transport.tcp`` option object, for the data-model + construction form. Passed as an + :class:`~pcapkit.corekit.multidict.OrderedMultiDict`, which is the shape + ``_make_tcp_options`` takes it in; ``subtype`` then comes from the object. + syn: Whether the carrying segment sets ``SYN``. + ack: Whether the carrying segment sets ``ACK``. + **kwargs: The keyword construction form, including ``subtype``. Ignored when + ``opt`` is given. + + Returns: + The constructed option schema, as ``_make_mode_mp`` returned it. + + """ + from pcapkit.const.tcp.option import Option + from pcapkit.corekit.multidict import OrderedMultiDict + from pcapkit.protocols.transport.tcp import TCP + + options = ([(Option.Multipath_TCP, kwargs)] if opt is None else + OrderedMultiDict([(Option.Multipath_TCP, opt)])) + return TCP(syn=syn, ack=ack, options=options).__header__.options[0] # type: ignore[arg-type] + + @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') class TCPUDPUnitTests(unittest.TestCase): def setUp(self) -> None: @@ -487,7 +535,12 @@ def test_tcp_option_constructors_cover_data_model_and_mapping_paths(self) -> Non self.assertEqual(mapped_options[0].mss, 1300) self.assertEqual(type(mapped_options[-1]).__name__, 'EndOfOptionList') - proto._flags = {Flags.SYN} + # NOTE: the MP_JOIN forms reach ``_make_mode_mp`` through :func:`mptcp_option`, + # i.e. through ``TCP()`` itself, rather than by writing ``proto._flags`` here. + # ``_flags`` is then the ``Enum_Flags`` member production assigns, resolved in + # production's order relative to the option build. C.f. #603. The options that do + # not read ``_flags`` keep calling ``proto._make_mode_mp`` directly, since going + # through the constructor would tell us nothing extra about them. join_syn = tcp_data.MPTCPJoinSYN( kind=Option.Multipath_TCP, length=12, @@ -498,9 +551,8 @@ def test_tcp_option_constructors_cover_data_model_and_mapping_paths(self) -> Non token=2, nonce=3, ) - self.assertEqual(proto._make_mode_mp(Option.Multipath_TCP, join_syn).addr_id, 1) + self.assertEqual(mptcp_option(join_syn, syn=True).addr_id, 1) - proto._flags = {Flags.SYN, Flags.ACK} join_synack = tcp_data.MPTCPJoinSYNACK( kind=Option.Multipath_TCP, length=20, @@ -511,9 +563,8 @@ def test_tcp_option_constructors_cover_data_model_and_mapping_paths(self) -> Non hmac=b'12345678', nonce=4, ) - self.assertEqual(proto._make_mode_mp(Option.Multipath_TCP, join_synack).addr_id, 2) + self.assertEqual(mptcp_option(join_synack, syn=True, ack=True).addr_id, 2) - proto._flags = {Flags.ACK} join_ack = tcp_data.MPTCPJoinACK( kind=Option.Multipath_TCP, length=24, @@ -521,9 +572,8 @@ def test_tcp_option_constructors_cover_data_model_and_mapping_paths(self) -> Non connection=Flags.ACK, hmac=b'1' * 20, ) - self.assertEqual(proto._make_mode_mp(Option.Multipath_TCP, join_ack).hmac, b'1' * 20) + self.assertEqual(mptcp_option(join_ack, ack=True).hmac, b'1' * 20) - proto._flags = set() capable = tcp_data.MPTCPCapable( kind=Option.Multipath_TCP, length=32, @@ -797,7 +847,6 @@ def assert_bad(reader, schema) -> None: )) def test_tcp_mptcp_constructors_cover_flag_branches(self) -> None: - from pcapkit.const.tcp.flags import Flags from pcapkit.const.tcp.mp_tcp_option import MPTCPOption from pcapkit.const.tcp.option import Option from pcapkit.protocols.transport.tcp import TCP @@ -825,24 +874,22 @@ def test_tcp_mptcp_constructors_cover_flag_branches(self) -> None: self.assertEqual(proto._make_mptcp_capable(MPTCPOption.MP_CAPABLE, rkey=2, skey=1).to_dict()['rkey'], 2) - proto._flags = {Flags.SYN} - join_syn = proto._make_mptcp_join(MPTCPOption.MP_JOIN, backup=True, - addr_id=1, token=2, nonce=3) + # NOTE: each MP_JOIN layout is selected by the flags a caller passes to ``TCP()``, + # not by a ``proto._flags`` written here -- see :func:`mptcp_option` and #603. + join_syn = mptcp_option(syn=True, subtype=MPTCPOption.MP_JOIN, backup=True, + addr_id=1, token=2, nonce=3) self.assertEqual(type(join_syn).__name__, 'MPTCPJoinSYN') self.assertTrue(join_syn.to_dict()['test']['backup']) - proto._flags = {Flags.SYN, Flags.ACK} - join_synack = proto._make_mptcp_join(MPTCPOption.MP_JOIN, addr_id=1, - hmac=b'12345678', nonce=3) + join_synack = mptcp_option(syn=True, ack=True, subtype=MPTCPOption.MP_JOIN, + addr_id=1, hmac=b'12345678', nonce=3) self.assertEqual(type(join_synack).__name__, 'MPTCPJoinSYNACK') self.assertEqual(join_synack.to_dict()['hmac'], b'12345678') - proto._flags = {Flags.ACK} - join_ack = proto._make_mptcp_join(MPTCPOption.MP_JOIN, hmac=b'1' * 20) + join_ack = mptcp_option(ack=True, subtype=MPTCPOption.MP_JOIN, hmac=b'1' * 20) self.assertEqual(type(join_ack).__name__, 'MPTCPJoinACK') self.assertEqual(join_ack.to_dict()['hmac'], b'1' * 20) - proto._flags = set() self.assertTrue(proto._make_mptcp_dss(MPTCPOption.DSS, data_fin=True, ack=1).to_dict()['flags']['A']) self.assertTrue(proto._make_mptcp_dss(MPTCPOption.DSS, dsn=1, ssn=2, @@ -858,13 +905,20 @@ def test_tcp_mptcp_constructors_cover_flag_branches(self) -> None: self.assertEqual(proto._make_mptcp_fastclose(MPTCPOption.MP_FASTCLOSE, key=123).to_dict()['key'], 123) + # NOTE: a segment with neither SYN nor ACK selects no MP_JOIN layout, and the + # library's own ``ProtocolError`` is what a caller must get for it. Reaching that + # through ``TCP()`` rather than through a hand-written ``proto._flags = set()`` + # makes this one assertion catch both defects #603 is about: revert #587's hoist + # and ``_flags`` does not exist yet, so this raises ``AttributeError``; restore the + # ``cast('Enum_Flags', 0)`` no-op and ``_flags`` is a plain ``int``, so + # ``Enum_Flags.SYN in self._flags`` raises ``TypeError``. A ``set`` gave the right + # answer for the wrong reason and hid both. with self.assertRaises(ProtocolError): - proto._make_mptcp_join(MPTCPOption.MP_JOIN) + mptcp_option(subtype=MPTCPOption.MP_JOIN) with self.assertRaises(ProtocolError): proto._make_mptcp_dss(MPTCPOption.DSS, dsn=1) def test_tcp_mptcp_readers_cover_subtype_and_error_branches(self) -> None: - from pcapkit.const.tcp.flags import Flags from pcapkit.const.tcp.mp_tcp_option import MPTCPOption from pcapkit.const.tcp.option import Option from pcapkit.corekit.multidict import OrderedMultiDict @@ -934,7 +988,19 @@ def mark(schema, length: int, subtype: MPTCPOption): MPTCPOption.MP_CAPABLE, ), options=options) - proto._flags = {Flags.SYN} + # NOTE: ``proto.make(...)`` rather than ``proto._flags = {Flags.SYN}``. Both + # 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 + # ``tests.protocols.transport.test_tcp_mptcp_join_flag_ordering_unit`` pins both + # halves against real segments. + proto.make(syn=True) join_syn = mark( MPTCPJoinSYN(test={'subtype': MPTCPOption.MP_JOIN.value, 'backup': 1}, addr_id=1, token=2, nonce=3), @@ -958,7 +1024,7 @@ def mark(schema, length: int, subtype: MPTCPOption): # defect rather than the RFC. The rejection case is now 20: the value the # guard used to *require*, which no MP_JOIN form produces, so it stays a # genuine rejection rather than an off-by-one near the correct length. - proto._flags = {Flags.SYN, Flags.ACK} + proto.make(syn=True, ack=True) join_synack = mark( MPTCPJoinSYNACK(test={'subtype': MPTCPOption.MP_JOIN.value, 'backup': 0}, addr_id=1, hmac=b'12345678', nonce=3), @@ -974,7 +1040,7 @@ def mark(schema, length: int, subtype: MPTCPOption): MPTCPOption.MP_JOIN, ), options=options) - proto._flags = {Flags.ACK} + proto.make(ack=True) join_ack = mark( MPTCPJoinACK(test={'subtype': MPTCPOption.MP_JOIN.value}, hmac=b'1' * 20), 24, @@ -987,7 +1053,7 @@ def mark(schema, length: int, subtype: MPTCPOption): 23, MPTCPOption.MP_JOIN, ), options=options) - proto._flags = set() + proto.make() with self.assertRaises(ProtocolError): proto._read_mode_mp(join_syn, options=options)