fix(tcp): give MPTCP real kind/length fields, repairing pack and parse

CHANGELOG.md

@@ -34,6 +34,7 @@ The largest release since 1.0, and the first recorded here as it happened rather
 - **Fixed** -- constant lookups that rejected a value the registry defines. `RouterAlert(0)` is the only value [RFC 2113](https://datatracker.ietf.org/doc/html/rfc2113) defines and the one IGMP, RSVP and MLD actually send, and it was discarded because the vendor crawler skipped a header row IANA's CSV does not have; IPX `Socket(0)` is that protocol's own default, so `bytes(IPX(...))` crashed on its own defaults; and two FTP `_missing_` overrides were plain methods rather than classmethods, so every unregistered value raised `TypeError` instead of extending the enumeration (#492, #503).
 - **Fixed** -- `format='text'` raised `AttributeError` before writing anything, naming a `dictdumper.Text` that has never existed. It now points at `Tree`, as the `'txt'` alias beside it already did.
 - **Fixed** -- 45 places where a documentation page contradicted the code (#413), ambiguous cross-references and five autodoc signature failures (#416), and `Extractor`'s documented exception plus 40 phantom or stale `Args:` labels (#501).
+- **Fixed** -- `TCP._make_mptcp_addaddr` could not build an `ADD_ADDR` option end to end: its `kind=`/`length=` arguments were rejected with `UnknownFieldWarning` and silently dropped, and `.pack()` then raised `KeyError: 'length'` from `port`'s own condition, `pkt['length'] in (10, 22)`. The cause was one layer up -- `MPTCP`, the base class every Multipath TCP subtype schema inherits, declared `kind` and `length` only under `typing.TYPE_CHECKING` rather than as real fields, unlike `Option`, which every non-Multipath TCP option schema inherits instead. That silently dropped `kind=`/`length=` for every `_make_mptcp_*` constructor, not only `ADD_ADDR`'s, so `MPTCP` now declares both for real, the same way `Option` already did (#541). The same missing fields broke parsing too: with no `kind`/`length` fields ahead of it, a Multipath TCP subtype schema's own leading field read the `kind` octet itself rather than the octet meant for it, an off-by-two in field alignment rather than a wire-format change -- a correct sender's octets were always right, only this library's reading of them was shifted. Spec-correct `ADD_ADDR` and `MP_PRIO` options failed to parse with `FieldError: TCP: [OptNo 30] 3 invalid IP version` and `KeyError: 'length'` respectively; both parse correctly now.
 - **Fixed** -- which exception a malformed TCP SACK option raised depended on unrelated process state: a clean interpreter raised `ProtocolError` as documented, but a process that had already popped `pcapkit.corekit.fields.misc` from `sys.modules` -- which the `#439` ABC-cache regression tests do in every case's `setUp`/`tearDown` -- raised `FieldValueError` instead, from a different layer entirely, before the documented check was even reached (#525). The cause was `ListField.unpack` resolving `SchemaField` through a function-local import re-run on every call; a module popped and reimported mid-process comes back as a second, distinct class, so `isinstance` against it silently misclassified the field and billed each item by its declared length instead of by what it actually consumed. **Any caller relying on the previously-observed** `FieldValueError` **for this case now gets** `ProtocolError` **instead, deterministically**, matching the method's own docstring. Fixed by importing at module level instead.
 - **Fixed** -- two dropped-keyword/wrong-cast defects flagged in review during this release and never filed until now: HIP's `_make_param_encrypted` passed `cipher=` to a schema with no such field, so the value was silently dropped and an AES-cipher `ENCRYPTED` parameter built through `make` packed without its IV; and IPv6-Route's `RPL.post_process`, which runs on every `Schema.pack` and not only after a parse, assumed `self.addresses` was still the concatenated `bytes` a parse leaves it as, and raised slicing the `list[bytes]` a `make`-built multi-address header actually holds there (#556).
 

docs/source/changelog/1.5.0.rst

@@ -282,6 +282,25 @@ pull requests between #326 and #509.
   (#413), ambiguous cross-references and five autodoc signature failures (#416),
   and ``Extractor``'s documented exception plus 40 phantom or stale ``Args:``
   labels (#501).
+* **Fixed** -- ``TCP._make_mptcp_addaddr`` could not build an ``ADD_ADDR``
+  option end to end: its ``kind=``/``length=`` arguments were rejected with
+  ``UnknownFieldWarning`` and silently dropped, and ``.pack()`` then raised
+  ``KeyError: 'length'`` from ``port``'s own condition,
+  ``pkt['length'] in (10, 22)``. The cause was one layer up -- ``MPTCP``, the
+  base class every Multipath TCP subtype schema inherits, declared ``kind`` and
+  ``length`` only under ``typing.TYPE_CHECKING`` rather than as real fields,
+  unlike ``Option``, which every non-Multipath TCP option schema inherits
+  instead. That silently dropped ``kind=``/``length=`` for every
+  ``_make_mptcp_*`` constructor, not only ``ADD_ADDR``'s, so ``MPTCP`` now
+  declares both for real, the same way ``Option`` already did (#541). The same
+  missing fields broke parsing too: with no ``kind``/``length`` fields ahead of
+  it, a Multipath TCP subtype schema's own leading field read the ``kind``
+  octet itself rather than the octet meant for it, an off-by-two in field
+  alignment rather than a wire-format change -- a correct sender's octets were
+  always right, only this library's reading of them was shifted. Spec-correct
+  ``ADD_ADDR`` and ``MP_PRIO`` options failed to parse with
+  ``FieldError: TCP: [OptNo 30] 3 invalid IP version`` and
+  ``KeyError: 'length'`` respectively; both parse correctly now.
 * **Fixed** -- which exception a malformed TCP SACK option raised depended on
   unrelated process state: a clean interpreter raised ``ProtocolError`` as
   documented, but a process that had already popped

pcapkit/protocols/schema/transport/tcp.py

@@ -621,11 +621,31 @@ class MPTCP(EnumSchema[Enum_MPTCPOption]):
 
     __enum__: 'DefaultDict[Enum_MPTCPOption, Type[MPTCP]]' = collections.defaultdict(lambda: MPTCPUnknown)
 
+    # NOTE: ``_MPTCP.data`` (a :class:`~pcapkit.corekit.fields.misc.SwitchField`,
+    # via :func:`mptcp_data_selector`) hands each subtype schema below the
+    # *whole* option -- ``kind``, ``length`` and all -- starting at the same
+    # first octet ``_MPTCP.test`` peeked and rewound past, rather than the
+    # bytes left over after some outer field already consumed a header. So
+    # unlike :class:`MPTCP`'s siblings that inherit :class:`Option`, which
+    # declares ``kind``/``length`` for exactly this reason, this base class
+    # has to declare its own -- unconditionally, since every Multipath TCP
+    # subtype carries an explicit length octet (no EOOL/NOP-style exception
+    # applies here). Every subclass's own leading field starts at the third
+    # octet as a result, which is where its ``test`` (subtype/flags) field
+    # expects to read from.
+    #
+    # Before this, both directions were broken: packing rejected ``kind=``/
+    # ``length=`` from the ``_make_mptcp_*`` makers with ``UnknownFieldWarning``
+    # and then ``KeyError: 'length'`` the moment a sibling field's condition
+    # (e.g. ``MPTCPAddAddress.port``, ``MPTCPCapable.rkey``) read
+    # ``pkt['length']``; unpacking silently misread the ``kind`` octet as the
+    # subtype/flags octet, since nothing had consumed it first. C.f. #541.
+    #: Option kind.
+    kind: 'Enum_Option' = EnumField(length=1, namespace=Enum_Option)
+    #: MPTCP length.
+    length: 'int' = UInt8Field()
+
     if TYPE_CHECKING:
-        #: Option kind.
-        kind: 'Enum_Option'
-        #: MPTCP length.
-        length: 'int'
         #: MPTCP subtype.
         subtype: 'Enum_MPTCPOption'
 

pcapkit/protocols/transport/tcp.py

@@ -2900,10 +2900,16 @@ def _make_mptcp_addaddr(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_M
             # a ``bool`` into an ``IPv4Address`` that the schema's own guard can no
             # longer tell from a real address -- ``addr=True`` reached
             # ``mptcp_add_address_selector`` as ``0.0.0.1`` with ``version=4``
-            # (c.f. #508). This option cannot be constructed end to end at all for
-            # an unrelated reason, ``KeyError: 'length'`` from
-            # pcapkit/protocols/schema/transport/tcp.py:790, which is why the
-            # corruption here was only ever visible on the schema the maker returns.
+            # (c.f. #508). Until #541, this option could not be constructed end to
+            # end at all for an unrelated reason -- ``KeyError: 'length'`` from the
+            # ``port`` field's condition at
+            # pcapkit/protocols/schema/transport/tcp.py:819, since
+            # ``Schema_MPTCPAddAddress`` (like every ``MPTCP`` subtype schema)
+            # declared no ``kind``/``length`` fields of its own for ``kind=``/
+            # ``length=`` below to land in -- which is why the corruption here was
+            # only ever visible on the schema the maker returns. #541 gave
+            # ``MPTCP`` real ``kind``/``length`` fields, so both now land and this
+            # constructs and packs correctly.
             addr_val = parse_ip_address(
                 addr, f'{self.alias}: [OptNo {Enum_Option.Multipath_TCP}] invalid address')
         version = addr_val.version

tests/protocols/test_option_roundtrip_unit.py

@@ -146,34 +146,53 @@ class Gap(NamedTuple):
         'negative below 40 kbps, and the option it emits at rate=0 does not '
         'parse back'),
 
-    # Four lambdas read ``pkt['length']`` while packing, but the enclosing
-    # ``_make_mptcp_*`` never puts a ``length`` in the packet, so the key is
-    # simply absent on the construction path.
+    # #541 declared real ``kind``/``length`` fields on ``MPTCP``, which is what
+    # gets these seven far enough to construct and pack -- they used to fail
+    # here with ``KeyError: 'length'`` (four of them) or
+    # ``AttributeError: ... no attribute 'kind'`` (three), and #541's fix closed
+    # both. The next field the construction path never sets is ``subtype``:
+    # ``MPTCP.subtype`` is still ``TYPE_CHECKING``-only, an annotation rather
+    # than a field, and the only thing that ever sets it is
+    # ``_MPTCP.post_process``, which runs on a real byte-level unpack -- not on
+    # the schema a ``_make_mptcp_*`` maker returns in memory, which is what
+    # ``TCP``'s convenience constructor (``TCP(options=[(code, kwargs)])``)
+    # reads straight back through ``_read_mptcp_*`` with no round trip in
+    # between. Filed as #566, which also has the fix for the pattern itself
+    # (whether ``subtype`` should become a real field the way ``kind``/
+    # ``length`` did, or be set some other way) rather than a per-case patch
+    # here.
     'tcp-mptcp/MP_CAPABLE': Gap(
-        'CONSTRUCT', "KeyError: 'length'",
-        'pcapkit/protocols/schema/transport/tcp.py:658'),
+        'CONSTRUCT', "'MPTCPCapable' object has no attribute 'subtype'",
+        'pcapkit/protocols/schema/transport/tcp.py:650 -- MPTCP.subtype is '
+        'TYPE_CHECKING-only, set only by _MPTCP.post_process (#566); and '
+        "separately, pcapkit/protocols/transport/tcp.py:2669's "
+        '`length=20 if rkey is None else 32` is RFC 8684 section 3.1\'s 12/20 '
+        'swapped, so once #566 is fixed this case would still pack the wrong '
+        'length rather than round-trip (#567)'),
     'tcp-mptcp/ADD_ADDR': Gap(
-        'CONSTRUCT', "KeyError: 'length'",
-        'pcapkit/protocols/schema/transport/tcp.py:790'),
+        'CONSTRUCT', "'MPTCPAddAddress' object has no attribute 'subtype'",
+        'pcapkit/protocols/schema/transport/tcp.py:650 -- MPTCP.subtype is '
+        'TYPE_CHECKING-only, set only by _MPTCP.post_process (#566)'),
     'tcp-mptcp/REMOVE_ADDR': Gap(
-        'CONSTRUCT', "KeyError: 'length'",
-        'pcapkit/protocols/schema/transport/tcp.py:807'),
+        'CONSTRUCT', "'MPTCPRemoveAddress' object has no attribute 'subtype'",
+        'pcapkit/protocols/schema/transport/tcp.py:650 -- MPTCP.subtype is '
+        'TYPE_CHECKING-only, set only by _MPTCP.post_process (#566)'),
     'tcp-mptcp/MP_PRIO': Gap(
-        'CONSTRUCT', "KeyError: 'length'",
-        'pcapkit/protocols/schema/transport/tcp.py:827'),
-
-    # ``_read_tcp_options`` reads ``schema.kind`` off every option schema, but
-    # the nested Multipath TCP subtype schemas do not declare one -- the field
-    # belongs to the enclosing option.
+        'CONSTRUCT', "'MPTCPPriority' object has no attribute 'subtype'",
+        'pcapkit/protocols/schema/transport/tcp.py:650 -- MPTCP.subtype is '
+        'TYPE_CHECKING-only, set only by _MPTCP.post_process (#566)'),
     'tcp-mptcp/DSS': Gap(
-        'CONSTRUCT', "no attribute 'kind'",
-        'pcapkit/protocols/transport/tcp.py:668 -- MPTCPDSS declares no kind'),
+        'CONSTRUCT', "'MPTCPDSS' object has no attribute 'subtype'",
+        'pcapkit/protocols/schema/transport/tcp.py:650 -- MPTCP.subtype is '
+        'TYPE_CHECKING-only, set only by _MPTCP.post_process (#566)'),
     'tcp-mptcp/MP_FAIL': Gap(
-        'CONSTRUCT', "no attribute 'kind'",
-        'pcapkit/protocols/transport/tcp.py:668 -- MPTCPFallback declares no kind'),
+        'CONSTRUCT', "'MPTCPFallback' object has no attribute 'subtype'",
+        'pcapkit/protocols/schema/transport/tcp.py:650 -- MPTCP.subtype is '
+        'TYPE_CHECKING-only, set only by _MPTCP.post_process (#566)'),
     'tcp-mptcp/MP_FASTCLOSE': Gap(
-        'CONSTRUCT', "no attribute 'kind'",
-        'pcapkit/protocols/transport/tcp.py:668 -- MPTCPFastclose declares no kind'),
+        'CONSTRUCT', "'MPTCPFastclose' object has no attribute 'subtype'",
+        'pcapkit/protocols/schema/transport/tcp.py:650 -- MPTCP.subtype is '
+        'TYPE_CHECKING-only, set only by _MPTCP.post_process (#566)'),
 
     # ``_make_mptcp_join`` branches on ``self._flags``, which only the parse
     # path ever sets, so the constructor cannot be called at all.