diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c013e393..69299f3ab 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 1a98515f1..d8ddf44e6 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/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 diff --git a/pcapkit/corekit/fields/numbers.py b/pcapkit/corekit/fields/numbers.py index c921a7c48..2fff3b0a3 100644 --- a/pcapkit/corekit/fields/numbers.py +++ b/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,14 +215,20 @@ 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 '<' @@ -202,6 +236,9 @@ def pre_process(self, value: 'int', packet: 'dict[str, Any]') -> 'int | bytes': self._template = f'{endian}{struct_fmt}' + if not self._need_process: + return value + return value.to_bytes( self._length, self._byteorder, signed=self._signed ) diff --git a/tests/corekit/test_fields_numbers_callable_length.py b/tests/corekit/test_fields_numbers_callable_length.py new file mode 100644 index 000000000..66f588c86 --- /dev/null +++ b/tests/corekit/test_fields_numbers_callable_length.py @@ -0,0 +1,283 @@ +"""A :class:`~pcapkit.corekit.fields.numbers.NumberField` whose ``length`` is a callable. + +GitHub issue #591. ``_need_process`` used to be a *latch*: ``build_template`` +raised it :data:`True` in its fall-through branch and nothing ever put it back. +A callable ``length`` is a placeholder of ``-1`` until +:meth:`~pcapkit.corekit.fields.numbers.NumberField.__call__` resolves it, ``-1`` +takes that fall-through branch, and the flag then outlived the very rebuild that +installed the real width. ``pre_process`` consequently handed :obj:`bytes` to a +template that had become ``>Q``, and :func:`struct.pack` refused it with +``required argument is not an integer``. + +Every test here is written as a **comparison against the same width supplied +statically**, because that is what makes the defect legible: the resolved length +and the struct template are identical either way, so the only difference left is +how the length arrived. A test that merely asserted ``pack`` succeeds would pass +on a fix that quietly changed the encoding. +""" + +from __future__ import annotations + +import importlib.util +import struct +import unittest + +from tests._support import purge_modules + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: Widths :func:`struct` has a native integer code for, paired with the +#: unsigned and signed format characters they build. These are the widths the +#: defect hit, and -- established by :meth:`CallableLengthTests +#: .test_every_native_width_was_affected_not_only_the_reported_eight` rather +#: than assumed -- *all four* were hit, not only the 8 that #591 reproduces. +NATIVE_WIDTHS = {1: ('B', 'b'), 2: ('H', 'h'), 4: ('I', 'i'), 8: ('Q', 'q')} + +#: Widths with no native :func:`struct` integer code, which therefore +#: legitimately still need byte packing. ``build_template`` takes the same +#: fall-through branch for these as it does for the ``-1`` placeholder, which is +#: why clearing the flag unconditionally would have been the wrong fix. +BYTE_PACKED_WIDTHS = (3, 5, 6, 7, 9, 16) + + +def _high_bit_value(width: 'int') -> 'int': + """A value whose top bit is set, so silent truncation cannot hide.""" + return (1 << (width * 8 - 1)) | 1 + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class CallableLengthTests(unittest.TestCase): + """The reported defect, and the widths either side of it.""" + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_the_reported_case_a_callable_resolving_to_eight_packs(self) -> None: + """``NumberField(length=lambda pkt: 8)`` could not pack at all. + + The reproduction in #591 verbatim: ``_make_mptcp_dss(DSS, ack=1 << 40)`` + raised ``struct.error: required argument is not an integer`` because + this field did. + + """ + from pcapkit.corekit.fields.numbers import NumberField + + field = NumberField(length=lambda pkt: 8)(dict()) + + self.assertEqual(field._length, 8) + self.assertEqual(field.template, '>Q') + self.assertIs(field._need_process, False, + 'the -1 placeholder latched _need_process and #591 is back') + self.assertEqual(field.pack(1 << 40, dict()).hex(), '0000010000000000') + + def test_a_callable_length_packs_exactly_as_the_same_static_length_does(self) -> None: + """The comparison that is the whole proof. + + Identical resolved length, identical template; the *only* difference is + whether ``length`` arrived as an :obj:`int` or as a callable returning + that same :obj:`int`. Before the fix the static column packed and the + callable column raised, for all four native widths. + + """ + from pcapkit.corekit.fields.numbers import NumberField + + for width in (*NATIVE_WIDTHS, *BYTE_PACKED_WIDTHS): + value = _high_bit_value(width) + with self.subTest(width=width): + dynamic = NumberField(length=lambda pkt, w=width: w)(dict()) + static = NumberField(length=width)(dict()) + + self.assertEqual(dynamic._length, static._length) + self.assertEqual(dynamic.template, static.template) + self.assertIs(dynamic._need_process, static._need_process) + self.assertEqual(dynamic.pack(value, dict()), static.pack(value, dict())) + + def test_every_native_width_was_affected_not_only_the_reported_eight(self) -> None: + """#591 left this open; it is established here rather than assumed. + + The issue reproduces the 8-octet case only and says explicitly that + whether ``1``/``2``/``4`` are equally affected was not checked. They + are: the latch has nothing to do with the width it latches *into*, so a + callable resolving to 1, 2 or 4 failed with the same + ``required argument is not an integer`` as one resolving to 8. Pinned + per width so a fix that only repaired the reported one fails here. + + """ + from pcapkit.corekit.fields.numbers import NumberField + + for width, (unsigned_fmt, signed_fmt) in NATIVE_WIDTHS.items(): + for signed, fmt in ((False, unsigned_fmt), (True, signed_fmt)): + with self.subTest(width=width, signed=signed): + field = NumberField(length=lambda pkt, w=width: w, signed=signed)(dict()) + + self.assertEqual(field.template, f'>{fmt}') + self.assertIs(field._need_process, False) + + value = -1 if signed else _high_bit_value(width) + self.assertEqual(field.pack(value, dict()), + struct.pack(f'>{fmt}', value)) + + def test_a_callable_resolving_to_a_byte_packed_width_still_needs_processing(self) -> None: + """The subtlety that makes this more than a one-line fix. + + ``build_template`` takes the same fall-through branch for any width + outside ``{1, 2, 4, 8}`` as it does for the ``-1`` placeholder. So + clearing the flag whenever a callable is resolved -- rather than + recomputing it from the width now in force -- would have broken every + one of these, which genuinely do need :meth:`int.to_bytes`. + + """ + from pcapkit.corekit.fields.numbers import NumberField + + for width in BYTE_PACKED_WIDTHS: + value = _high_bit_value(width) + with self.subTest(width=width): + field = NumberField(length=lambda pkt, w=width: w)(dict()) + + self.assertEqual(field.template, f'>{width}s') + self.assertIs(field._need_process, True, + f'width {width} has no native struct code and must be ' + f'byte-packed') + self.assertEqual(field.pack(value, dict()), + value.to_bytes(width, 'big')) + + def test_the_flag_always_agrees_with_the_template(self) -> None: + """The invariant the fix establishes, stated directly. + + ``_need_process`` says "this template wants :obj:`bytes`", which is true + exactly when the template is an ``s`` format. Holding that for every + width, reached both ways, is what stops the two drifting apart again -- + by any route, not just the one #591 came in through. + + """ + from pcapkit.corekit.fields.numbers import NumberField + + for width in range(1, 17): + for supply in ('static', 'callable'): + with self.subTest(width=width, supply=supply): + length = width if supply == 'static' else (lambda pkt, w=width: w) + field = NumberField(length=length)(dict()) + + self.assertIs(field._need_process, field.template.endswith('s'), + f'template {field.template} and ' + f'_need_process={field._need_process} disagree') + + def test_the_placeholder_itself_still_needs_processing(self) -> None: + """The flag is recomputed, not blanket-cleared. + + While ``length`` is still the ``-1`` placeholder there is no width to + pack to, so :data:`True` is the correct answer for it -- and it is the + answer the unresolved field keeps. What changed is that the answer is + no longer *sticky*. + + """ + from pcapkit.corekit.fields.numbers import NumberField + + field = NumberField(length=lambda pkt: 8) # deliberately not resolved + + self.assertEqual(field._length, -1) + self.assertIs(field._need_process, True) + + def test_an_unresolved_field_repairs_its_length_and_honours_the_new_template(self) -> None: + """``pre_process``'s ``_length < 0`` safety net, kept consistent. + + A field packed without having been resolved falls into the repair at + ``numbers.py``, which recomputes the length from the value and rebuilds + the template. That rebuild can land on a native width, and before the + fix the method went on to return :obj:`bytes` anyway -- the same + mismatch as #591, one branch further down. Widths with no native code + still come back as :obj:`bytes`. + + """ + from pcapkit.corekit.fields.numbers import NumberField + + for value, width, fmt in ((0xFF, 1, '>B'), (0xFFFF, 2, '>H'), + (0xFFFFFFFF, 4, '>I'), (0xFFFFFFFFFFFFFFFF, 8, '>Q'), + (0x800001, 3, '>3s')): + with self.subTest(value=hex(value)): + field = NumberField(length=lambda pkt: 8) # deliberately not resolved + + self.assertEqual(field.pack(value, dict()), value.to_bytes(width, 'big')) + self.assertEqual(field._length, width) + self.assertEqual(field.template, fmt) + self.assertIs(field._need_process, fmt.endswith('s')) + + def test_a_callable_length_roundtrips_through_pack_and_unpack(self) -> None: + """Parsing was broken by the same latch, in the mirror direction. + + With the flag stuck :data:`True`, ``post_process`` called + :meth:`int.from_bytes` on the :obj:`int` that ``struct.unpack`` had + already produced from a ``>Q`` template. So the 8-octet callable field + could not read what it could not write. + + """ + from pcapkit.corekit.fields.numbers import NumberField + + for width in (*NATIVE_WIDTHS, *BYTE_PACKED_WIDTHS): + value = _high_bit_value(width) + with self.subTest(width=width): + field = NumberField(length=lambda pkt, w=width: w)(dict()) + + buffer = field.pack(value, dict()) + self.assertEqual(len(buffer), width) + self.assertEqual(field.unpack(buffer, dict()), value) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class SubclassCallableLengthTests(unittest.TestCase): + """The same field classes as they are actually used in the schemas.""" + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_an_enum_field_with_a_callable_length_packs(self) -> None: + """:class:`~pcapkit.corekit.fields.numbers.EnumField` leaves ``__template__`` unset. + + Which is what exposes it: ``__init__`` only calls ``build_template`` -- + and so only latches the flag -- when no ``__template__`` fixes the + format. :class:`EnumField` inherits :data:`None` for both, so it was + affected exactly as the bare base class was. + + """ + from pcapkit.corekit.fields.numbers import EnumField + + for width in (1, 2, 4, 8): + with self.subTest(width=width): + field = EnumField(length=lambda pkt, w=width: w)(dict()) + + self.assertIs(field._need_process, False) + self.assertEqual(field.pack(1 << 4, dict()), + (1 << 4).to_bytes(width, 'big')) + + def test_a_subclass_fixing_a_template_keeps_working_either_way(self) -> None: + """The control on the other side: these were never broken, and still are not. + + ``UInt32Field`` and friends fix ``__template__``, so ``__init__`` skips + ``build_template`` entirely and the placeholder never latched anything. + A callable resolving to a *different* width than the class fixes is + still honoured, and still gets the right flag for that width -- which + is the case #585's ``SwitchField`` workaround was built out of. + + """ + from pcapkit.corekit.fields.numbers import UInt32Field, UInt64Field + + wide = UInt64Field(length=lambda pkt: 8)(dict()) + self.assertEqual(wide.template, '>Q') + self.assertIs(wide._need_process, False) + self.assertEqual(wide.pack(1 << 40, dict()).hex(), '0000010000000000') + + # a callable overriding the class's own fixed width, in both directions + widened = UInt32Field(length=lambda pkt: 8)(dict()) + self.assertEqual(widened.template, '>Q') + self.assertIs(widened._need_process, False) + self.assertEqual(widened.pack(1 << 40, dict()).hex(), '0000010000000000') + + odd = UInt32Field(length=lambda pkt: 3)(dict()) + self.assertEqual(odd.template, '>3s') + self.assertIs(odd._need_process, True) + self.assertEqual(odd.pack(0x800001, dict()).hex(), '800001') + + +if __name__ == '__main__': + unittest.main()