diff --git a/examples/generators/options.py b/examples/generators/options.py index 50aab897d2..67b2a3ad2f 100644 --- a/examples/generators/options.py +++ b/examples/generators/options.py @@ -974,32 +974,37 @@ def _mh_extension_build(code: 'Any', kwargs: 'dict[str, Any]') -> 'Any': #: ``Length`` had been counted in 4-octet units where the RFC counts bytes -- #: which is what moved the one-copy figure from 44 to 46, level with two copies. #: -#: Three remain, none of them helped by a second copy since each fails at two -#: copies as well as at one: +#: Two remain, neither helped by a second copy since each fails at two copies +#: as well as at one: #: #: * ``HOST_ID`` declares ``len=8`` and packs 18. Recorded as #: ``hip-parameter/HOST_ID``. #: * ``HIP_TRANSFORM`` is HIPv1-only -- ``_read_param_hip_transform`` raises for #: any other version -- while this table builds it at version 2. Recorded as #: ``hip-parameter/HIP_TRANSFORM``. -#: * ``R1_Counter`` (128) parses as an ``UnassignedParameter``: ``_read_param_*`` -#: and ``_make_param_*`` are found by enumeration member name so both exist for -#: code 128, but the *schema* registry is keyed on the ``code=`` of the class -#: statement and ``R1CounterParameter`` declares only 129. Recorded as -#: ``hip-parameter/R1_Counter``, filed as #690. #: -#: So the pair was routing around nothing by the time #672 and #679 landed, and -#: dropping to one copy does not change what round-trips. Measured over this -#: table's 49 HIP codes on ``5f0a1aa90`` (after #696, which #689 itself waited -#: on -- see the issue): +#: A third gap here, ``R1_Counter`` (128), was closed separately by #690 and was +#: never one of the pair's cancellations above: it parsed as an +#: ``UnassignedParameter`` because the *schema* registry is keyed on the +#: ``code=`` of the class statement and ``R1CounterParameter`` declared only +#: 129 -- even though ``__parameter__``'s ``_read_param_*``/``_make_param_*`` +#: entries already existed for both codes, as two hand-written dict entries, +#: not because of any name-normalisation rule. No longer recorded as of #690, +#: which registered ``R1CounterParameter`` for 128 too. +#: +#: So the pair was routing around nothing by the time #672, #679 and #690 had +#: all landed, and dropping to one copy does not change what round-trips. +#: Measured over this table's 49 HIP codes on ``5f0a1aa90`` (after #696, which +#: #689 itself waited on -- see the issue) and again after #690: #: #: ====================== ======== ========== #: tree one copy two copies #: ====================== ======== ========== #: ``5f0a1aa90`` 46 OK 46 OK +#: after #690 47 OK 47 OK #: ====================== ======== ========== #: -#: The three gaps above are the only cases either setting fails, and neither +#: The two gaps above are the only cases either setting fails, and neither #: their ``status`` nor their ``defect`` moves between settings, so #: :data:`tests.protocols.test_option_roundtrip_unit.EXPECTED_FAILURES` needed no #: change to keep recording them accurately at one copy. diff --git a/pcapkit/protocols/schema/internet/hip.py b/pcapkit/protocols/schema/internet/hip.py index 31679edd44..34c5d05e86 100644 --- a/pcapkit/protocols/schema/internet/hip.py +++ b/pcapkit/protocols/schema/internet/hip.py @@ -563,8 +563,28 @@ def __init__(self, type: 'Enum_Parameter', len: 'int', index: 'int', @schema_final -class R1CounterParameter(Parameter, code=Enum_Parameter.R1_COUNTER): - """Header schema for HIP ``R1_COUNTER`` parameters.""" +class R1CounterParameter(Parameter, code=[Enum_Parameter.R1_Counter, + Enum_Parameter.R1_COUNTER]): + """Header schema for HIP ``R1_COUNTER`` parameters. + + Registered under both codes, not 129 alone. :rfc:`5201#section-5.2.3` + (HIPv1) and :rfc:`7401#section-5.2.3` (HIPv2) give the identical 4 + 8 + layout to code 128 (``R1_Counter``) and code 129 (``R1_COUNTER``) -- + one parameter under two numbers, the difference being HIP's own C-bit + rather than an unrelated code -- and the field list below is that layout + exactly, since #696 widened :attr:`counter` to eight octets. + :attr:`~pcapkit.protocols.internet.hip.HIP.__parameter__` already carries + two hand-written entries -- not a name-normalisation rule; ``R1_Counter`` + and ``R1_COUNTER`` differ only in case, and each needed its own line -- + mapping both codes to the same ``_read_param_r1_counter``, and + ``_make_param_r1_counter`` already built this class for 128; it was only + the schema lookup this class's own ``code=`` feeds -- consulted on the + parse path by :class:`~pcapkit.corekit.fields.collections.OptionField` + -- that fell back to :class:`UnassignedParameter` for 128, since a + single-code ``code=`` registered 129 alone and left 128 unclaimed. + See #690. + + """ #: Reserved. reserved: 'bytes' = PaddingField(length=4) diff --git a/tests/protocols/internet/test_hip_r1_counter_width_unit.py b/tests/protocols/internet/test_hip_r1_counter_width_unit.py index 33057b9c80..a18882b332 100644 --- a/tests/protocols/internet/test_hip_r1_counter_width_unit.py +++ b/tests/protocols/internet/test_hip_r1_counter_width_unit.py @@ -175,12 +175,12 @@ def test_a_lone_r1_counter_parameter_survives_a_hip_packet(self) -> None: both codes; the two-copy pairing in :mod:`examples.generators.options` is what concealed that. - Code 128 is deliberately not exercised here. It fails for an unrelated - reason that #672 does not touch -- the schema registry is keyed on the - ``code=`` of the class statement and ``R1CounterParameter`` declares only - 129, so 128 parses as an ``UnassignedParameter`` -- which is #690, and - which ``hip-parameter/R1_Counter`` in - :mod:`tests.protocols.test_option_roundtrip_unit` records. + Code 128 is deliberately not exercised here. #690 registered + ``R1CounterParameter`` for 128 as well as 129, so it now reaches this + same class rather than parsing as an ``UnassignedParameter`` -- + that coverage lives with the fix, in + :meth:`HIPUnitTests.test_hip_r1_counter_code_128_resolves_to_its_own_schema`, + not here; this module stays scoped to the width #672 fixed. """ from pcapkit.const.hip.parameter import Parameter from pcapkit.protocols.internet.hip import HIP diff --git a/tests/protocols/internet/test_hip_unit.py b/tests/protocols/internet/test_hip_unit.py index 63a6652fc5..ad26eb3cbd 100644 --- a/tests/protocols/internet/test_hip_unit.py +++ b/tests/protocols/internet/test_hip_unit.py @@ -2303,6 +2303,108 @@ def test_hip_nat_traversal_mode_and_esp_transform_survive_the_full_parser(self) for copy in copies: self.assertEqual(getattr(copy, attr), (1,)) + def test_hip_r1_counter_code_128_resolves_to_its_own_schema(self) -> None: + """#690: HIPv1's ``R1_Counter`` (code 128) must not fall through to + ``UnassignedParameter`` on the parse path. + + ``R1CounterParameter`` used to register itself with ``code= + Enum_Parameter.R1_COUNTER`` alone -- 129, the HIPv2 spelling. + :attr:`~pcapkit.protocols.internet.hip.HIP.__parameter__` already + carries two hand-written entries -- not a name-normalisation rule -- + mapping both 128 and 129 to ``_read_param_r1_counter``, and + ``_make_param_r1_counter`` already built a real ``R1CounterParameter`` + for 128 -- so construction worked and only parsing was broken. The + *schema* used to unpack a parameter off the wire is chosen by a + different mapping, :attr:`~pcapkit.protocols.schema.schema.EnumSchema. + registry`, wired into :class:`HIP`'s ``param`` field as + ``OptionField(..., registry=Parameter.registry)``, and that one is + keyed solely by the ``code=`` a schema class declares. So code 128 + fell to ``Parameter.__default__`` -- ``UnassignedParameter``, whose + schema has ``value: bytes`` and no ``counter`` -- and + ``_read_param_r1_counter`` failed reading ``schema.counter`` off it + with ``AttributeError: 'UnassignedParameter' object has no attribute + 'counter'``. Measured verbatim on the tree before this fix, with the + exact bytes this test builds. + + The expected wire bytes below are derived from + ``R1CounterParameter.__fields__`` rather than a hex literal, on + purpose: a literal is exactly what made the first version of this + test fail the moment #696 widened ``counter`` from four octets to + eight (12-octet records became 16). Deriving from the field widths + keeps this test about the registry dispatch it names, rather than + about a wire width #696's own ``test_hip_r1_counter_width_unit.py`` + already pins. + + Two copies, matching + ``test_hip_nat_traversal_mode_and_esp_transform_survive_the_full_parser``'s + own reason: proving the reader's stride across repeated occurrences + of the same code, not working around any alignment defect. With #696 + landed a lone ``R1_COUNTER`` record already packs to a self-aligned + 16 octets and parses cleanly on its own -- measured directly, one + copy raises nothing and reads back the same counter -- so two copies + here are a stride check, not a workaround. + + """ + from pcapkit.const.hip.parameter import Parameter + from pcapkit.protocols.data.internet import hip as hip_data + from pcapkit.protocols.internet.hip import HIP + from pcapkit.protocols.schema.internet import hip as hip_schema + + # The schema registry itself: both codes now resolve to the same class. + self.assertIs(hip_schema.Parameter.registry[Parameter.R1_Counter], + hip_schema.R1CounterParameter) + self.assertIs(hip_schema.Parameter.registry[Parameter.R1_COUNTER], + hip_schema.R1CounterParameter) + + proto = object.__new__(HIP) + + r1_schema = proto._make_param_r1_counter(Parameter.R1_Counter, version=1, + counter=0xaabbccdd) + r1_one = bytes(r1_schema) + + # Derived from the schema's own declared field widths -- see the + # docstring above for why this is not a hex literal. This only agrees + # with the actual packed record because _make_param_r1_counter hard- + # codes len=12 (below) rather than deriving it the same way; the two + # would diverge the moment reserved + counter stopped summing to 12. + reserved_len = hip_schema.R1CounterParameter.__fields__['reserved'].length + counter_len = hip_schema.R1CounterParameter.__fields__['counter'].length + content_len = reserved_len + counter_len + self.assertEqual(content_len, 12) # matches _make_param_r1_counter's len=12 + total_len = hip_schema.parameter_total_len(content_len) + padding_len = total_len - 4 - content_len + expected = ( + int(Parameter.R1_Counter).to_bytes(2, 'big') + + content_len.to_bytes(2, 'big') + + bytes(reserved_len) + + (0xaabbccdd).to_bytes(counter_len, 'big') + + bytes(padding_len) + ) + self.assertEqual(r1_one, expected) + self.assertEqual(len(r1_one), total_len) + self.assertEqual(len(r1_one) % 8, 0) + + param_area = r1_one * 2 + self.assertEqual(len(param_area) % 8, 0) + hdr_len_units = 4 + len(param_area) // 8 + + # next(1)=0x3b len(1) pkt(1)=0x00 ver(1)=0x11 (version nibble 1, plus + # the reserved bit that must be 1) checksum(2) control(2) shit(16) + # rhit(16) -- the fixed 40-octet header, declaring HIPv1 and the + # parameter area to follow. + fixed = (bytes([0x3b, hdr_len_units, 0x00, 0x11]) + bytes(2) + + bytes(2) + bytes(16) + bytes(16)) + self.assertEqual(len(fixed), 40) + raw = fixed + param_area + + parsed = HIP(raw, len(raw), extension=True) + self.assertEqual(parsed.info.version, 1) + copies = parsed.info.parameters.getlist(Parameter.R1_Counter) + self.assertEqual(len(copies), 2) + for copy in copies: + self.assertIsInstance(copy, hip_data.R1CounterParameter) + self.assertEqual(copy.counter, 0xaabbccdd) + def test_hip_schema_selectors_and_encrypted_parameter_branches(self) -> None: from pcapkit.const.hip.cipher import Cipher from pcapkit.const.hip.hi_algorithm import HIAlgorithm diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index bc6f8f39f9..aa36845c30 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -388,14 +388,14 @@ class Gap(NamedTuple): # -- HIP ------------------------------------------------------------------ - # ``_read_param_*`` and ``_make_param_*`` are found by enumeration member - # name, so both exist for code 128; but the *schema* registry is keyed by - # the ``code=`` of the class statement, and ``R1CounterParameter`` declares - # only 129. So code 128 parses as an ``UnassignedParameter``. - 'hip-parameter/R1_Counter': Gap( - 'PARSE', "no attribute 'counter'", - 'pcapkit/protocols/internet/hip.py:822 -- Parameter.registry[128] is ' - 'UnassignedParameter, because R1CounterParameter declares code=129 only'), + # ``hip-parameter/R1_Counter`` used to be here: ``HIP.__parameter__``'s + # hand-written dict entries already mapped both 128 and 129 to + # ``_read_param_r1_counter``/``_make_param_r1_counter`` (not a + # name-normalisation rule -- each code has its own literal entry), so both + # existed for code 128. But the *schema* registry was keyed by the + # ``code=`` of the class statement, and ``R1CounterParameter`` declared + # only 129 -- so code 128 parsed as an ``UnassignedParameter``. Fixed by + # #690, registering ``R1CounterParameter`` under both codes. # ``ENCRYPTED`` used to have an entry here, for two defects at once: that # ``_make_param_encrypted`` passed ``cipher=``, a keyword