fix(corekit): recompute _need_process from the width in force, not once from the placeholder (#591)

CHANGELOG.md

@@ -51,6 +51,7 @@ The largest release since 1.0, and the first recorded here as it happened rather
 This is the resolution of #548, which reported `TransType.L2TP` (115) as registered nowhere and proposed binding `L2TPv2` there. That binding is wrong rather than merely awkward: [RFC 3931](https://datatracker.ietf.org/doc/html/rfc3931) §4.1.1 gives 115 to *L2TPv3 over IP*, whose session header is "free of any restrictions imposed by coexistence with L2TPv2 and L2F" and carries **no version nibble at all**, so a v2 parser cannot even detect that the datagram is not its own. Measured, it produced `version=4`, `tunnelid=0x5678` and `sessionid=0xff03` from the top half of a Session ID and two octets of the PPP frame behind it. 115 is a missing *class*, not a missing registration, and stays unbound until an `L2TPv3` class exists; no dissector was invented here to fill it. The reasoning is now recorded in `pcapkit.protocols.link.l2tp` rather than only in a test, and `register_protocol_code`'s worked example -- which named `L2TPv2` at 115 -- names `L2TPv3` instead (#548).
 - **Added** -- `tests/protocols/test_dispatch_reachability_unit.py`, the coverage #548 asked for: every `ProtocolBase` descendant whose `__index__` returns an enum member is checked to be reachable under that code in the registry its enum *type* designates, read from the same `_CODE_DESTINATIONS` table backing `code=` so the two cannot drift. Where `test_dispatch_registry_unit.py` walks the 38 entries that exist and checks each parses, this walks the classes and catches one nothing registered at all -- the shape in which `OSPF` once shipped reachable from no table. 23 claims verified, no gaps; a companion case injects a gap and confirms the audit reports it, so the guard cannot rot into a permanently green no-op (#548).
 - **Fixed** -- both halves of what the `Changelog drift` gate told an author, in `util/changelog_md.py`. `ResidualMarkupError` named the entry file but numbered its lines against the *converted* body, which rule 6 joins onto one line per block: measured on this entry, the body is 55 lines against the file's 580, so a reported number could not reach most of the file at all, and four roles written on two source lines were all reported as `line 46`. The conversion now carries a source map -- which line of the entry each stretch of output came from -- so every complaint cites a line of the file the message names, one complaint per construct rather than one per joined line, in the entry's own order (#588). Rule 2 separately accepted only the bare-number spelling of Sphinx's `:rfc:` role, so a citation of [RFC 6554 Section 3](https://datatracker.ietf.org/doc/html/rfc6554#section-3) fell past the rule that exists for it and was reported as a role the rules do not cover; both spellings now convert, with the anchor carried into the link target and the link text taken from Sphinx's own so that the Markdown and the rendered history say the same thing about the same page (#592). Regenerating `CHANGELOG.md` is byte-identical over all 37 committed entries, so the fix changes what the gate *says* and nothing about what it emits.
+- **Fixed** -- a `NumberField` whose `length` was a callable could not pack or parse at any width `struct` has a native integer code for. `length` is a placeholder of `-1` until the callable is resolved, `-1` has no native code, and the template builder raised `_need_process` for it and never put it back -- so the flag was a latch. Resolving the real width rebuilt the template and left the latch set, and `pre_process` then handed `bytes` to a template that had become `>Q`, raising `struct.error: required argument is not an integer`; parsing failed in the mirror direction, calling `int.from_bytes` on the integer that `struct.unpack` had already produced. The flag is now recomputed from the width actually in force rather than only ever raised, which is also what keeps a callable resolving to a width with no native code -- 3 octets, say -- byte-packed as it must be. **All four native widths were affected, not only the 8 that was reported**: the latch has nothing to do with the width it latches into, so 1, 2 and 4 failed identically, on `NumberField` and on `EnumField`, both of which leave `__template__` unset. This is what made every extended 8-octet MPTCP DSS form unbuildable, since those widths are chosen at runtime from the DSS flags and so must come from a callable; #585 worked around it in the TCP schema alone, leaving every other caller exposed (#591).
 - **Fixed** -- building any `MP_JOIN` option raised `AttributeError: 'TCP' object has no attribute '_flags'`. `TCP.make` constructed the options *before* it assigned the `self._flags` that the option makers read, and `_make_mptcp_join` branches on that attribute to choose between the three layouts [RFC 8684](https://datatracker.ietf.org/doc/html/rfc8684) section 3.2 gives for MP_JOIN -- figure 5 for SYN at 12 octets, figure 6 for SYN/ACK at 16, figure 7 for ACK at 24. The parse path was never affected: `read` assigns the flags before it parses the options, so the identical branches in `_read_mptcp_join` always had them. Fixed by hoisting the flag resolution above the `_make_tcp_options` call, leaving only the header's data-offset computation -- which genuinely needs the options' total length -- after it. **Not** fixed by giving `_flags` a zero default, which would have been worse: `Protocol.pack` is public and calls `make`, so an instance that had already parsed a segment always had the attribute set, and it silently built the option for the segment it had *read* rather than the one it was asked to write. Measured pre-fix, a parsed MP_JOIN-SYN instance asked to pack an MP_JOIN-ACK segment emitted an ACK header carrying figure 5's 12-octet SYN option, with the caller's 20-octet HMAC -- the whole authentication payload of figure 7's form -- replaced by an all-zero phantom token and nonce, and nothing raised. The crash was the benign symptom; a default value fixes only that and leaves the silent corruption. Hoisting also made one branch reachable for the first time, an MP_JOIN asked for on a segment with neither SYN nor ACK set: the accumulator was seeded with `cast('Enum_Flags', 0)`, and `typing.cast` being a runtime no-op, `self._flags` stayed a plain `int` on which the first membership test raised `TypeError` rather than the `ProtocolError` the method documents. It is now seeded with `Enum_Flags(0)`, a real flagless member that compares equal to `0` and ORs identically. The read path's own seed is deliberately unchanged -- a flagless MP_JOIN is rejected by `mptcp_data_selector` before `_read_mptcp_join` runs, so it cannot reach those branches (#587).
 - **Added** -- `tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py`, covering all three MP_JOIN layouts through the *public* constructor, the construct-pack-parse cycle for each, the stale-flags case that rules out a zero-valued default, the statement order itself, and controls that the parse path and the flag-independent options are unaffected. The gap it closes is why 100% statement and branch coverage of the two changed modules coexisted with a completely broken public path: the pre-existing cases reach `_make_mptcp_join` by assigning a Python `set` to `_flags` on a bare `TCP.__new__(TCP)`, which executes every branch while bypassing both the ordering and the accumulator's type. The now-stale `tcp-mptcp/MP_JOIN` entry is deleted from `EXPECTED_FAILURES`, and the MP_JOIN exclusion in `test_tcp_mptcp_subtype_unit.py` is lifted (#587).
 

docs/source/changelog/1.5.0.rst

@@ -584,6 +584,26 @@ pull requests between #326 and #509.
   rendered history say the same thing about the same page (#592). Regenerating
   ``CHANGELOG.md`` is byte-identical over all 37 committed entries, so the fix
   changes what the gate *says* and nothing about what it emits.
+* **Fixed** -- a ``NumberField`` whose ``length`` was a callable could not pack
+  or parse at any width ``struct`` has a native integer code for. ``length`` is
+  a placeholder of ``-1`` until the callable is resolved, ``-1`` has no native
+  code, and the template builder raised ``_need_process`` for it and never put
+  it back -- so the flag was a latch. Resolving the real width rebuilt the
+  template and left the latch set, and ``pre_process`` then handed ``bytes`` to
+  a template that had become ``>Q``, raising
+  ``struct.error: required argument is not an integer``; parsing failed in the
+  mirror direction, calling ``int.from_bytes`` on the integer that
+  ``struct.unpack`` had already produced. The flag is now recomputed from the
+  width actually in force rather than only ever raised, which is also what
+  keeps a callable resolving to a width with no native code -- 3 octets, say --
+  byte-packed as it must be. **All four native widths were affected, not only
+  the 8 that was reported**: the latch has nothing to do with the width it
+  latches into, so 1, 2 and 4 failed identically, on ``NumberField`` and on
+  ``EnumField``, both of which leave ``__template__`` unset. This is what made
+  every extended 8-octet MPTCP DSS form unbuildable, since those widths are
+  chosen at runtime from the DSS flags and so must come from a callable; #585
+  worked around it in the TCP schema alone, leaving every other caller exposed
+  (#591).
 * **Fixed** -- building any ``MP_JOIN`` option raised
   ``AttributeError: 'TCP' object has no attribute '_flags'``. ``TCP.make``
   constructed the options *before* it assigned the ``self._flags`` that the option

pcapkit/corekit/fields/numbers.py

@@ -131,6 +131,12 @@ def __call__(self, packet: 'dict[str, Any]') -> 'Self':
         This method will return a new instance of :class:`NumberField` instead of
         updating the current instance.
 
+        Notes:
+            Rebuilding the template here is what applies a callable ``length``,
+            and :meth:`build_template` recomputes ``self._need_process`` as it
+            goes, so the flag and the template always describe the same width.
+            They did not always: see GitHub issue #591.
+
         """
         new_self = super().__call__(packet)
 
@@ -154,18 +160,40 @@ def build_template(self, length: 'int', signed: 'bool') -> 'str':
         Returns:
             Template for field.
 
+        Notes:
+            ``self._need_process`` is **assigned** here rather than only ever
+            raised, so that it always describes the ``length`` this template
+            was built for. It used to be
+            set :data:`True` in the fall-through branch and never put back,
+            which made it a latch: a callable ``length`` is a placeholder of
+            ``-1`` at construction, ``-1`` takes the fall-through branch, and
+            the flag then survived the rebuild in :meth:`__call__` that
+            resolved the real width. :meth:`pre_process` consequently handed
+            :obj:`bytes` to a template that had become ``>Q`` -- or ``>I``,
+            ``>H``, ``>B`` -- and :func:`struct.pack` refused it. See GitHub
+            issue #591.
+
+            Assigning it is what tells a placeholder apart from a width that
+            genuinely needs byte packing, without having to remember that a
+            placeholder was ever in play: the answer for ``-1`` is
+            :data:`True`, the answer for ``8`` is :data:`False`, and whichever
+            width is in force now is the one that decides. A callable
+            resolving to, say, ``3`` still takes the fall-through branch and
+            still gets :data:`True`, because for ``3`` that is the correct
+            answer rather than a leftover one.
+
         """
         if length == 8:       # unpack to 8-byte integer (long long)
-            struct_fmt = 'q' if signed else 'Q'
+            struct_fmt, need_process = 'q' if signed else 'Q', False
         elif length == 4:     # unpack to 4-byte integer (int / long)
-            struct_fmt = 'i' if signed else 'I'
+            struct_fmt, need_process = 'i' if signed else 'I', False
         elif length == 2:     # unpack to 2-byte integer (short)
-            struct_fmt = 'h' if signed else 'H'
+            struct_fmt, need_process = 'h' if signed else 'H', False
         elif length == 1:     # unpack to 1-byte integer (char)
-            struct_fmt = 'b' if signed else 'B'
+            struct_fmt, need_process = 'b' if signed else 'B', False
         else:                 # do not unpack
-            struct_fmt = f'{length}s'
-            self._need_process = True
+            struct_fmt, need_process = f'{length}s', True
+        self._need_process = need_process
         return struct_fmt
 
     def pre_process(self, value: 'int', packet: 'dict[str, Any]') -> 'int | bytes':  # pylint: disable=unused-argument
@@ -187,21 +215,30 @@ def pre_process(self, value: 'int', packet: 'dict[str, Any]') -> 'int | bytes':
             its signed range afterwards, so that e.g. a PCAP-NG section length
             of ``-1`` (section length not specified) can be written out.
 
+            A field packed without having been resolved -- so with ``_length``
+            still negative -- has its width derived from the value instead, and
+            that rebuild can land on a width :func:`struct` has a native
+            integer code for. The flag is therefore consulted **after** the
+            rebuild rather than before it, since deciding first and rebuilding
+            second is how the template and the value being returned came to
+            disagree in the first place. C.f. #591.
+
         """
         value = value & self._bit_mask
         if self._signed and value > self._bit_mask >> 1:
             value -= self._bit_mask + 1
-        if not self._need_process:
-            return value
 
-        if self._length < 0:
+        if self._need_process and self._length < 0:
             self._length = math.ceil(value.bit_length() // 8)
 
             endian = '>' if self._byteorder == 'big' else '<'
             struct_fmt = self.build_template(self._length, self._signed)
 
             self._template = f'{endian}{struct_fmt}'
 
+        if not self._need_process:
+            return value
+
         return value.to_bytes(
             self._length, self._byteorder, signed=self._signed
         )