From 04e3cca92871d7d58a203b3e110c5d53f50bb322 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 17 Sep 2026 23:25:26 -0400 Subject: [PATCH] protocols: size MH's MN-ID option from its subtype, not identifier's Python type _make_opt_mn_id sized the identifier by inspecting isinstance(identifier, ...) rather than the resolved subtype_val that actually selects the wire format (mn_id_selector). For the IPv6_Address subtype -- the method's own default -- the schema always packs a fixed 16-octet address regardless of declared length, so a str or int identifier, including the no-argument default, declared a length that disagreed with what was packed. - pcapkit/protocols/internet/mh.py: size id_len from subtype_val (16 for IPv6_Address) and normalise identifier to an ipaddress.IPv6Address first, so the packed bytes and the declared length derive from one value. - examples/generators/options.py: drop the MN_ID_OPTION_TYPE override that worked around the defect by forcing an already-correct-type identifier; the generator now exercises the real (now-fixed) default. - tests/protocols/internet/test_mh_unit.py: add a dedicated test asserting len(packed) == length + 2 for every documented identifier type against IPv6_Address, including the default; fix an existing assertion that had encoded the buggy length (3) as expected. - docs/source/pep.rst: drop the deferred-defect note now that it is fixed. Re-measured against faf86d26b: PR #437 rewrote this module wholesale but did not fix this method, only worked around it in the generator. Full suite: 995 passed/17 skipped/1547 subtests before, 996 passed/17 skipped/1553 subtests after, no failures either side, baseline faf86d26b. Closes #448 --- docs/source/pep.rst | 3 -- examples/generators/options.py | 4 --- pcapkit/protocols/internet/mh.py | 11 +++++- tests/protocols/internet/test_mh_unit.py | 43 +++++++++++++++++++++++- 4 files changed, 52 insertions(+), 9 deletions(-) diff --git a/docs/source/pep.rst b/docs/source/pep.rst index a848eafbf0..aacc647070 100644 --- a/docs/source/pep.rst +++ b/docs/source/pep.rst @@ -223,9 +223,6 @@ What is left, and why: so putting it in that chain would make ``layer=`` and ``protocol=`` limits behave wrongly. Changing the parsed shape from :obj:`bytes` to ``Raw`` also changes what existing captures dump to, so it is its own change. -* **The MN-ID option's constructor mis-sizes a non-address identifier**, tracked - as `#448 `__. Pre-existing - and outside the registry-completion work, so it is filed rather than fixed here. Two wire-format traps are worth knowing before touching this code, since both look like ordinary fields and are not: diff --git a/examples/generators/options.py b/examples/generators/options.py index bd0b050641..b26c25a09f 100644 --- a/examples/generators/options.py +++ b/examples/generators/options.py @@ -810,10 +810,6 @@ def _mh_option_overrides() -> 'dict[Any, dict[str, Any]]': Enum_Option.Authorization_Data: {'data': b'\xa5' * 8}, Enum_Option.Mobility_Header_Link_Layer_Address_option: { 'address': b'\x00\x11\x22\x33\x44\x55'}, - # The default identifier is the *string* ``'::'``, whose ``len()`` is 2 - # rather than the 16 octets the field packs -- so the declared length - # is 3 going out and 17 coming back. An address object avoids it. - Enum_Option.MN_ID_OPTION_TYPE: {'identifier': ipaddress.IPv6Address('::1')}, # ``(len + 6) % 4`` has to be 0. Enum_Option.AUTH_OPTION_TYPE: {'data': b'\xa5\xa5'}, # Without this ``_make_opt_mesg_id`` reads the clock, and the capture diff --git a/pcapkit/protocols/internet/mh.py b/pcapkit/protocols/internet/mh.py index edfee69b2e..b81b0a93b1 100644 --- a/pcapkit/protocols/internet/mh.py +++ b/pcapkit/protocols/internet/mh.py @@ -7659,7 +7659,16 @@ def _make_opt_mn_id(self, type: 'Enum_Option', option: 'Optional[Data_MNIDOption subtype_val = self._make_index(subtype, subtype_default, namespace=subtype_namespace, # type: ignore[assignment] reversed=subtype_reversed, pack=False) - if isinstance(identifier, ipaddress.IPv6Address): + # NOTE: The wire format is chosen by ``subtype_val`` (c.f. ``mn_id_selector``), + # not by the Python type of ``identifier``, so the width has to be taken from + # the former. For the ``IPv6_Address`` subtype the schema always packs a fixed + # 16-octet address (:class:`~pcapkit.corekit.fields.ipaddress.IPv6AddressField` + # ignores any declared length), so ``identifier`` is normalised to that wire + # form here as well, keeping the packed bytes and the declared length derived + # from one value instead of two independent computations (c.f. #448). + if subtype_val == Enum_MNIDSubtype.IPv6_Address: + if not isinstance(identifier, ipaddress.IPv6Address): + identifier = ipaddress.IPv6Address(identifier) id_len = 16 elif isinstance(identifier, int): id_len = math.ceil(identifier.bit_length() / 8) diff --git a/tests/protocols/internet/test_mh_unit.py b/tests/protocols/internet/test_mh_unit.py index 4a52c56799..f6f563c9a1 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -498,8 +498,10 @@ def test_mh_option_constructors_cover_known_options_and_dispatch(self) -> None: self.assertEqual(proto._make_opt_mn_id(Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.NAI, identifier='node@example').length, 13) + # default subtype is IPv6_Address, which always packs a fixed 16-octet + # address regardless of the identifier's Python type -- see #448. self.assertEqual(proto._make_opt_mn_id(Option.MN_ID_OPTION_TYPE, - identifier=0x1234).length, 3) + identifier=0x1234).length, 17) self.assertEqual(proto._make_opt_auth(Option.AUTH_OPTION_TYPE, subtype=AuthSubtype.MN_HA, spi=7, data=b'ab').spi, 7) with self.assertRaises(ProtocolError): @@ -2289,6 +2291,45 @@ def test_mh_length_derived_addresses_pick_their_family(self) -> None: self.assertEqual(data.ipv4, ipv4) self.assertEqual(data.prefix, ipaddress.ip_address(prefix)) + def test_mh_mn_id_option_length_matches_packed_octets(self) -> None: + """The MN-ID option's declared length must count what actually gets packed. + + ``_make_opt_mn_id`` used to size the identifier from the *Python type* of + the ``identifier`` argument rather than from ``subtype_val``, which is what + actually selects the wire format (see ``mn_id_selector``). For the + ``IPv6_Address`` subtype -- the method's own default -- the schema always + packs a fixed 16-octet address + (:class:`~pcapkit.corekit.fields.ipaddress.IPv6AddressField` ignores any + declared length entirely), so a ``str`` or ``int`` identifier, including + the no-argument default, declared a ``length`` that disagreed with what was + actually packed: ``len(packed) != length + 2``. See #448. + """ + import ipaddress + + from pcapkit.const.mh.option import Option + from pcapkit.protocols.internet.mh import MH + + proto = object.__new__(MH) + + # every documented ``identifier`` type, against the subtype that used to + # mis-size three of the four -- including the method's own default, which + # is itself one of the broken types (``str``). + cases = ( + ('default (no identifier)', {}), + ("str '::'", {'identifier': '::'}), + ("str '2001:db8::1'", {'identifier': '2001:db8::1'}), + ('int 0x1234', {'identifier': 0x1234}), + ('bytes (16-octet packed form)', + {'identifier': ipaddress.IPv6Address('2001:db8::1').packed}), + ('IPv6Address', {'identifier': ipaddress.ip_address('2001:db8::1')}), + ) + for label, kwargs in cases: + with self.subTest(label): + schema = proto._make_opt_mn_id( # type: ignore[arg-type] + Option.MN_ID_OPTION_TYPE, **kwargs) + self.assertEqual(schema.length, 17) + self.assertEqual(len(schema.pack()), schema.length + 2) + def test_mh_redirect_option_rejects_contradictory_flags(self) -> None: """:rfc:`6463#section-4.2` allows exactly one of the ``K`` and ``N`` flags.