diff --git a/CHANGELOG.md b/CHANGELOG.md index e528b210d..22b5fc2b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ The largest release since 1.0, and the first recorded here as it happened rather - **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). - **Fixed** -- two more defects #541 exposed rather than caused, both since it let construction reach code that had never run before. `MPTCP.subtype` was still `typing.TYPE_CHECKING`-only, an annotation rather than a field, so `TCP(options=[(Enum_Option.Multipath_TCP, ...)])` raised `AttributeError: ... has no attribute 'subtype'` for every subtype but `MP_JOIN`: the convenience constructor builds a schema in memory and reads it straight back through `_read_mptcp_*` with no byte round trip, so `_MPTCP.post_process` -- the only code that ever set `subtype` -- never ran. Fixed on the construction path (`TCP._make_mode_mp`) rather than by adding a third real field the way `kind`/`length` got in #541: unlike those two, `subtype` is already packed as 4 bits of each subtype's own `test` bitfield, and a second, independent field for the same bits would either double-encode them or need a "derive, don't pack" field kind this library does not have (#566). Separately, `_make_mptcp_capable` wrote `length=20 if rkey is None else 32` where [RFC 8684](https://datatracker.ietf.org/doc/html/rfc8684) section 3.1 gives 12 and 20, and `MPTCPCapable.rkey`'s own condition (`pkt['length'] != 32`) dropped the receiver's key for exactly the length the maker used to mean "key present" -- so a spec-correct, key-absent MP_CAPABLE could not be built at all, and a key-present one silently lost its key on the wire. Both, and the matching guard in `_read_mptcp_capable`, now agree on 12/20. **This changes MP_CAPABLE's packed output**: a 20-octet, key-present option built or parsed under the old code becomes 12 octets with no key, or 20 octets with the key actually present, depending on which the caller meant (#567). +- **Fixed** -- IPv6-Route's RPL routing data was five octets wide at the front where [RFC 6554](https://datatracker.ietf.org/doc/html/rfc6554) section 3 gives four, and three further defects sat stacked behind it. `CmprI`, `CmprE` and `Pad` are each a 4-bit field, sharing one 32-bit word with a 20-bit `Reserved`, but the schema declared `cmpr_i` and `cmpr_e` as whole octets -- so a constructed two-address header packed to 41 octets while the `Hdr Ext Len` of 5 derived from that inflated data area declared 48. Correcting the width is a **wire-format change**, and it reshapes the schema: `RPL(cmpr_i=..., cmpr_e=...)` is now `RPL(cmpr={'cmpr_i': ..., 'cmpr_e': ...})`, beside the `pad={'pad_len': ...}` that was already there. Behind it, the reader's `header.length % 16` guard read `Hdr Ext Len` as an octet count and assumed 16-octet addresses, which an SRH only carries when `CmprI` and `CmprE` are both 0 -- the unit confusion #487 fixed for Source Route and Type 2, flagged and deliberately left by #489 for want of a working RPL round trip to validate a replacement against. It is replaced by section 4.2's own address-count arithmetic, `n = (((Hdr Ext Len * 8) - Pad - (16 - CmprE)) / (16 - CmprI)) + 1`, which the reader now requires to close -- non-negative and whole. `RPL.post_process` subtracted `pad_len` a second time from a buffer whose own length callback had already taken it off, losing one address per `16 - CmprI` octets of padding, and set `ip` only when it had parsed octets -- so once the guard stopped rejecting every constructed header, `IPv6_Route(type=..., data={'ip': [...]})` raised a bare `AttributeError: 'RPL' object has no attribute 'ip'` from the reader. And `_make_data_type_rpl` computed `Pad` as `8 - length % 8` without the outer `% 8`, so an already-aligned address vector was handed a full 8 octets of padding where section 3 requires that when `CmprI` and `CmprE` are both 0, `Pad` MUST carry a value of 0. The narrower `cmpr_i` also removes a latent divide-by-zero: a whole octet could hold 16, making `16 - CmprI` zero, where a 4-bit field tops out at 15. `ipv6-route-type/RPL_Source_Route_Header` round-trips now and its `EXPECTED_FAILURES` entry is deleted; as with the guard it replaces, none of this has been checked against a real RPL capture (#564). - **Changed** -- `ModuleDescriptor.klass` reads an already-imported module out of `sys.modules` rather than re-entering `importlib.import_module`, which matters because next layer dispatch resolves a descriptor there on a per-frame path. A registry *hit* holding a `ModuleDescriptor` is resolved once and written back, but a *miss* deliberately is not -- recording a miss in a class-level `collections.defaultdict` is the defect #425/#428 fixed at this layer and #560 fixed at the schema layer -- so every unrecognised frame resolved the same fallback descriptor again: 48 of the 52 `ModuleDescriptor.klass` resolutions an extraction of `many_interfaces.pcapng` performs, and 4 of the 7 on `ipv4.pcap`. `import_module` keeps real per-call work for a module `sys.modules` already holds, so that resolution now costs ~117 ns rather than ~436 ns and the whole miss path ~526 ns rather than ~883 ns, on CPython 3.14.7. **The scale is worth stating plainly: this is not measurable in** `extract()` **wall clock.** 48 avoided calls is ~17 us against a ~37 ms extraction, two orders of magnitude inside this host's run-to-run variance, and the "~40% of cumulative time" reading that prompted the work was an artifact of `_import_next_layer` being a recursive-descent dispatcher -- its *self* time is 0.58%, while `aenum.extend_enum` is 16.7%. What the change is taken for is its shape rather than its speed: nothing memoises the resolved class, anywhere, so `sys.modules` stays the only module cache in play and its invalidation is the interpreter's. A memo of the class would serve the pre-reload class after an `importlib.reload` forever, and an instance of it fails `isinstance` against the live one. Proposed by `@Ts-Boom` in #563, whose profiling found the miss path; the implementation differs because that one added a second, never-invalidated cache of resolved classes (#574). Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) and `1.5.0b3` (2026-09-19), all published as prereleases and so resolved only by `pip install --pre`. `1.5.0b1` half-shipped: the tag, the GitHub release and the Conda deployments landed, but PyPI rejected the wheel because `twine check` found a Sphinx-only `:mod:` role in `README.rst`, which `pyproject.toml` declares as the dynamic long description. `1.5.0b2` is what reshipped it -- the release workflow is version-driven, so an existing version cannot republish -- and `1.5.0b3` followed the CI change that stops a TestPyPI outage from costing a release its wheels (#497, #498). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index deb8d7828..1927d5f65 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -420,6 +420,39 @@ pull requests between #326 and #509. packed output**: a 20-octet, key-present option built or parsed under the old code becomes 12 octets with no key, or 20 octets with the key actually present, depending on which the caller meant (#567). +* **Fixed** -- IPv6-Route's RPL routing data was five octets wide at the front + where :rfc:`6554` section 3 gives four, and three further defects sat + stacked behind it. ``CmprI``, ``CmprE`` and ``Pad`` are each a 4-bit field, + sharing one 32-bit word with a 20-bit ``Reserved``, but the schema declared + ``cmpr_i`` and ``cmpr_e`` as whole octets -- so a constructed two-address + header packed to 41 octets while the ``Hdr Ext Len`` of 5 derived from that + inflated data area declared 48. Correcting the width is a **wire-format + change**, and it reshapes the schema: ``RPL(cmpr_i=..., cmpr_e=...)`` is now + ``RPL(cmpr={'cmpr_i': ..., 'cmpr_e': ...})``, beside the + ``pad={'pad_len': ...}`` that was already there. Behind it, the reader's + ``header.length % 16`` guard read ``Hdr Ext Len`` as an octet count and + assumed 16-octet addresses, which an SRH only carries when ``CmprI`` and + ``CmprE`` are both 0 -- the unit confusion #487 fixed for Source Route and + Type 2, flagged and deliberately left by #489 for want of a working RPL + round trip to validate a replacement against. It is replaced by section + 4.2's own address-count arithmetic, + ``n = (((Hdr Ext Len * 8) - Pad - (16 - CmprE)) / (16 - CmprI)) + 1``, which + the reader now requires to close -- non-negative and whole. + ``RPL.post_process`` subtracted ``pad_len`` a second time from a buffer + whose own length callback had already taken it off, losing one address per + ``16 - CmprI`` octets of padding, and set ``ip`` only when it had parsed + octets -- so once the guard stopped rejecting every constructed header, + ``IPv6_Route(type=..., data={'ip': [...]})`` raised a bare + ``AttributeError: 'RPL' object has no attribute 'ip'`` from the reader. And + ``_make_data_type_rpl`` computed ``Pad`` as ``8 - length % 8`` without the + outer ``% 8``, so an already-aligned address vector was handed a full 8 + octets of padding where section 3 requires that when ``CmprI`` and ``CmprE`` + are both 0, ``Pad`` MUST carry a value of 0. The narrower ``cmpr_i`` also + removes a latent divide-by-zero: a whole octet could hold 16, making + ``16 - CmprI`` zero, where a 4-bit field tops out at 15. + ``ipv6-route-type/RPL_Source_Route_Header`` round-trips now and its + ``EXPECTED_FAILURES`` entry is deleted; as with the guard it replaces, none + of this has been checked against a real RPL capture (#564). * **Changed** -- ``ModuleDescriptor.klass`` reads an already-imported module out of ``sys.modules`` rather than re-entering ``importlib.import_module``, which matters because next layer dispatch resolves a descriptor there on a per-frame diff --git a/docs/source/pcapkit/protocols/internet/ipv6_route.rst b/docs/source/pcapkit/protocols/internet/ipv6_route.rst index 8b2f54b5e..70cfe0989 100644 --- a/docs/source/pcapkit/protocols/internet/ipv6_route.rst +++ b/docs/source/pcapkit/protocols/internet/ipv6_route.rst @@ -85,6 +85,10 @@ Header Schemas Type Stubs ~~~~~~~~~~ +.. autoclass:: pcapkit.protocols.schema.internet.ipv6_route.CmprInfo + :members: + :show-inheritance: + .. autoclass:: pcapkit.protocols.schema.internet.ipv6_route.PadInfo :members: :show-inheritance: diff --git a/pcapkit/protocols/internet/ipv6_route.py b/pcapkit/protocols/internet/ipv6_route.py index 7659befa3..70b18e55a 100644 --- a/pcapkit/protocols/internet/ipv6_route.py +++ b/pcapkit/protocols/internet/ipv6_route.py @@ -606,20 +606,37 @@ def _read_data_type_rpl(self, schema: 'Schema_RPL', *, header: 'Schema_IPv6_Rout Parsed route data. """ - # NOTE: this guard has the same surface shape as the Source Route and - # Type 2 unit confusion #487 fixed above -- ``header.length`` is - # ``Hdr Ext Len`` in 8-octet units, not octets, and ``% 16`` reads - # like a leftover assumption that it was already a total octet - # count. It is left as-is here: RPL addresses are variable-length - # (compressed by ``cmpr_i``/``cmpr_e``), so a fixed ``% 16`` bound is - # not obviously the right invariant even under correct units, and - # nothing here has been checked against a real RPL capture. Flagged - # for follow-up rather than guessed at. (The round trip through - # ``RPL.post_process`` this note used to say was broken -- it - # treated a ``make``-built ``list[bytes]`` as ``bytes`` and raised - # on pack -- was fixed by #556; that no longer blocks validating a - # replacement here, but the replacement itself is still unwritten.) - if header.length % 16 != 0: + # NOTE: the ``% 16`` bound that stood here had the same surface shape + # as the Source Route and Type 2 unit confusion #487 fixed above -- + # ``header.length`` is ``Hdr Ext Len``, in the 8-octet units + # :rfc:`6554#section-3` specifies, not octets -- and it additionally + # assumed 16-octet addresses, which an SRH only carries when ``CmprI`` + # and ``CmprE`` are both 0. #489 called it out but deliberately left + # it alone, because ``RPL.post_process`` raised on every pack back + # then so there was no round trip to validate a replacement against. + # #556 removed that blocker and #564 the mis-sized fixed area behind + # it, so the replacement is written here rather than guessed at. + # + # :rfc:`6554#section-4.2` derives the address count from the same + # fields this reader has to hand: + # + # n = (((Hdr Ext Len * 8) - Pad - (16 - CmprE)) / (16 - CmprI)) + 1 + # + # so the invariant actually available is that the division closes -- + # non-negative, and whole. ``16 - cmpr_i`` cannot be zero: ``CmprI`` + # is a *"4-bit unsigned integer"* per :rfc:`6554#section-3`, hence at + # most 15. Note this is a well-formedness check the library needs in + # order to walk ``Addresses[1..n]`` at all; :rfc:`6554#section-4.2` + # itself specifies no malformed-header drop condition, only + # ``Segments Left > n``, so nothing stricter is imposed here. Still + # not checked against a real RPL capture, which is the caveat the + # ``% 16`` bound carried too. + cmpr_i = schema.cmpr['cmpr_i'] + cmpr_e = schema.cmpr['cmpr_e'] + pad_len = schema.pad['pad_len'] + + remainder = header.length * 8 - pad_len - (16 - cmpr_e) + if remainder < 0 or remainder % (16 - cmpr_i) != 0: raise ProtocolError(f'{self.alias}: [TypeNo {header.type}] invalid format') ipv6_route = Data_RPL( @@ -627,9 +644,9 @@ def _read_data_type_rpl(self, schema: 'Schema_RPL', *, header: 'Schema_IPv6_Rout length=ipv6_route_header_length(header.length), type=header.type, seg_left=header.seg_left, - cmpr_i=schema.cmpr_i, - cmpr_e=schema.cmpr_e, - pad=schema.pad['pad_len'], + cmpr_i=cmpr_i, + cmpr_e=cmpr_e, + pad=pad_len, ip=tuple(schema.ip), ) return ipv6_route @@ -778,7 +795,16 @@ def _make_data_type_rpl(self, type: 'Enum_Routing', route: 'Optional[Data_RPL]' prefix_e = os_path.commonprefix(test_list) cmpr_e = len(prefix_e) - pad = 8 - ((len(ip) - 1) * (16 - cmpr_i) + (16 - cmpr_e)) % 8 + # NOTE: the outer ``% 8`` is what keeps a vector that is + # already 8-octet aligned from being handed a *full* 8 octets + # of padding -- ``8 - 0`` is 8, not 0. That is reachable + # whenever ``dst`` shares no prefix with the addresses, which + # makes ``cmpr_i`` and ``cmpr_e`` both 0 and the vector a + # multiple of 16, and it contradicts :rfc:`6554#section-3`: + # *"Note that when CmprI and CmprE are both 0, Pad MUST carry + # a value of 0."* ``_make_data_type_none`` above already + # spells the idiom this way; this branch did not. + pad = (8 - ((len(ip) - 1) * (16 - cmpr_i) + (16 - cmpr_e)) % 8) % 8 ip_val = [] for item in ip[:-1]: @@ -792,8 +818,10 @@ def _make_data_type_rpl(self, type: 'Enum_Routing', route: 'Optional[Data_RPL]' ip_val.append(cast('IPv6Address', parse_ip_address(ip[-1], descr, version=6)).packed[cmpr_e:]) return Schema_RPL( - cmpr_i=cmpr_i, - cmpr_e=cmpr_e, + cmpr={ + 'cmpr_i': cmpr_i, + 'cmpr_e': cmpr_e, + }, pad={ 'pad_len': pad, }, diff --git a/pcapkit/protocols/schema/internet/ipv6_route.py b/pcapkit/protocols/schema/internet/ipv6_route.py index 0ef07b776..f20b72962 100644 --- a/pcapkit/protocols/schema/internet/ipv6_route.py +++ b/pcapkit/protocols/schema/internet/ipv6_route.py @@ -31,6 +31,12 @@ if SPHINX_TYPE_CHECKING: # pragma: no cover from typing_extensions import TypedDict + class CmprInfo(TypedDict): + """Prefix-compression counts, two nibbles of a single octet.""" + + cmpr_i: int + cmpr_e: int + class PadInfo(TypedDict): """Padding length and reserved.""" @@ -180,10 +186,23 @@ def __init__(self, ip: 'IPv6Address | str | int | bytes') -> 'None': ... class RPL(RoutingType, code=Enum_Routing.RPL_Source_Route_Header): """Header schema for IPv6-Route RPL routing data.""" - #: CmprI. - cmpr_i: 'int' = UInt8Field() - #: CmprE. - cmpr_e: 'int' = UInt8Field() + #: CmprI and CmprE -- two 4-bit counts sharing one octet. + #: + #: NOTE: :rfc:`6554#section-3` gives ``CmprI`` and ``CmprE`` as *"4-bit + #: unsigned integer"*, i.e. the high and low nibble of a single octet, so + #: they cannot be two :class:`~pcapkit.corekit.fields.numbers.UInt8Field` + #: as they were before #564. Together with :attr:`pad` below -- ``Pad`` + #: (4 bits) plus ``Reserved`` (20 bits) -- this is the one 32-bit word the + #: diagram in :rfc:`6554#section-3` draws, and the same word + #: :meth:`IPv6_Route._read_data_type_rpl + #: `'s + #: own docstring already drew correctly. The split between the two fields + #: falls on the octet boundary between ``CmprE`` and ``Pad``, so neither + #: straddles an octet. + cmpr: 'CmprInfo' = BitField(length=1, namespace={ + 'cmpr_i': (0, 4), + 'cmpr_e': (4, 4), + }) #: Padding length and reserved. pad: 'PadInfo' = BitField(length=3, namespace={ 'pad_len': (0, 4), @@ -221,38 +240,80 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Schema': # here -- treating the list as ``bytes`` (as a bare ``cast`` # used to, without a runtime check) raised trying to slice and # re-join it. See #556. + # + # NOTE: ``ip`` still has to be *set*, though, rather than merely + # left alone. :meth:`Protocol.__post_init__ + # ` packs and + # then unpacks, and :meth:`IPv6_Route.read + # ` hands + # :meth:`~pcapkit.protocols.internet.ipv6_route.IPv6_Route._read_data_type_rpl` + # *this* schema on that path, not a re-parsed one -- so returning + # early without ``ip`` raised a bare ``AttributeError: 'RPL' + # object has no attribute 'ip'`` from the reader. That was masked + # for as long as the reader's ``% 16`` guard rejected every + # constructed header first; fixing the guard alongside #564 + # exposed it, so it is fixed in the same pass. Each item is one + # whole element of ``Addresses[1..n]``, so a full-width (16-octet) + # item is decoded the way the parse path below decodes an + # uncompressed one, and a compressed suffix is left as + # :obj:`bytes` -- which is exactly what that path does too. The + # :func:`isinstance` test keeps anything that is not :obj:`bytes` + # passing through untouched, as it did when this branch returned + # without setting ``ip`` at all, rather than failing here on a + # :func:`len` the item may not support. + self.ip = [ + cast('IPv6Address', ipaddress.ip_address(item)) + if isinstance(item, bytes) and len(item) == 16 else item + for item in buffer + ] return self dst_val = cast('Optional[IPv6Address]', packet.get('dst')) dst = dst_val.packed if dst_val is not None else None - ilen = 16 - self.cmpr_i - elen = 16 - self.cmpr_e + cmpr_i = self.cmpr['cmpr_i'] + cmpr_e = self.cmpr['cmpr_e'] + + ilen = 16 - cmpr_i + elen = 16 - cmpr_e addr = [] # type: list[IPv6Address | bytes] counter = 0 # Addresses[1..n-1] - for _ in range((len(buffer) - self.pad['pad_len'] - elen) // ilen): + # + # NOTE: ``buffer`` is ``self.addresses``, whose own ``length`` callback + # above already subtracted ``pad_len`` -- the trailing padding octets + # are read by :attr:`padding`, not by this field. Subtracting + # ``pad_len`` a *second* time here dropped one address for every + # ``ilen`` octets of padding, so a padded (i.e. compressed) header + # parsed one address short; measured with ``cmpr_i=cmpr_e=4`` and three + # addresses, which yields ``pad_len=4`` and walked one element instead + # of two. Only reachable once the ``% 16`` guard in + # :meth:`IPv6_Route._read_data_type_rpl + # ` + # stopped rejecting every such header, which is why it is fixed in the + # same pass as #564. + for _ in range((len(buffer) - elen) // ilen): buf = buffer[counter:counter + ilen] if dst is None: - if self.cmpr_i == 0: + if cmpr_i == 0: addr.append(cast('IPv6Address', ipaddress.ip_address(buf))) else: addr.append(buf) else: - buf = dst[:self.cmpr_i] + buf + buf = dst[:cmpr_i] + buf addr.append(cast('IPv6Address', ipaddress.ip_address(buf))) counter += ilen # Addresses[n] buf = buffer[counter:counter + elen] if dst is None: - if self.cmpr_e == 0: + if cmpr_e == 0: addr.append(cast('IPv6Address', ipaddress.ip_address(buf))) else: addr.append(buf) else: - buf = dst[:self.cmpr_e] + buf + buf = dst[:cmpr_e] + buf addr.append(cast('IPv6Address', ipaddress.ip_address(buf))) self.ip = addr @@ -262,5 +323,5 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Schema': #: Addresses (SRH prefix compression decoded). ip: 'list[IPv6Address | bytes]' - def __init__(self, cmpr_i: 'int', cmpr_e: 'int', pad: 'PadInfo', + def __init__(self, cmpr: 'CmprInfo', pad: 'PadInfo', addresses: 'list[bytes]') -> 'None': ... diff --git a/tests/protocols/internet/test_ipv6_extension_unit.py b/tests/protocols/internet/test_ipv6_extension_unit.py index 50bf85fb2..3d1fdd503 100644 --- a/tests/protocols/internet/test_ipv6_extension_unit.py +++ b/tests/protocols/internet/test_ipv6_extension_unit.py @@ -325,7 +325,8 @@ def test_ipv6_route_readers_and_constructors_cover_registered_types(self) -> Non with self.assertRaises(ProtocolError): proto._read_data_type_2(route_schema.Type2(ip='2001:db8::2'), header=header) - rpl_schema = route_schema.RPL(cmpr_i=0, cmpr_e=0, pad={'pad_len': 0}, addresses=[]) + rpl_schema = route_schema.RPL(cmpr={'cmpr_i': 0, 'cmpr_e': 0}, + pad={'pad_len': 0}, addresses=[]) object.__setattr__(rpl_schema, 'ip', (ip_address('2001:db8::3'),)) rpl_header = types.SimpleNamespace(next=TransType.TCP, length=16, type=Routing.RPL_Source_Route_Header, seg_left=1) @@ -362,20 +363,20 @@ def test_ipv6_route_readers_and_constructors_cover_registered_types(self) -> Non Routing.RPL_Source_Route_Header, ip=['2001:db8::8'], ) - self.assertEqual(rpl_plain.cmpr_i, 0) + self.assertEqual(rpl_plain.cmpr['cmpr_i'], 0) rpl_compressed = proto._make_data_type_rpl( Routing.RPL_Source_Route_Header, dst=ip_address('2001:db8::ffff'), ip=['2001:db8::1', '2001:db8::2'], ) - self.assertGreaterEqual(rpl_compressed.cmpr_i, 0) + self.assertGreaterEqual(rpl_compressed.cmpr['cmpr_i'], 0) rpl_from_data = proto._make_data_type_rpl( Routing.RPL_Source_Route_Header, route_data.RPL(next=TransType.TCP, length=16, type=Routing.RPL_Source_Route_Header, seg_left=1, cmpr_i=1, cmpr_e=2, pad=0, ip=(ip_address('2001:db8::9'),)), ) - self.assertEqual(rpl_from_data.cmpr_i, 1) + self.assertEqual(rpl_from_data.cmpr['cmpr_i'], 1) def test_ipv6_route_make_dst_rejects_a_bool(self) -> None: """A :obj:`bool` destination address must not be silently converted. See #540. @@ -454,8 +455,8 @@ def test_ipv6_route_rpl_source_addresses_reject_a_bool(self) -> None: dst=ip_address('2001:db8::ffff'), ip=['2001:db8::1', '2001:db8::2'], ) - self.assertGreaterEqual(rpl.cmpr_i, 0) - self.assertGreaterEqual(rpl.cmpr_e, 0) + self.assertGreaterEqual(rpl.cmpr['cmpr_i'], 0) + self.assertGreaterEqual(rpl.cmpr['cmpr_e'], 0) def test_ipv6_route_read_data_type_errors_report_real_routing_type(self) -> None: """Regression test for GH-442. @@ -487,7 +488,8 @@ def test_ipv6_route_read_data_type_errors_report_real_routing_type(self) -> None proto._read_data_type_2(route_schema.Type2(ip='2001:db8::2'), header=header) self.assertEqual(str(type2_ctx.exception), expected) - rpl_schema = route_schema.RPL(cmpr_i=0, cmpr_e=0, pad={'pad_len': 0}, addresses=[]) + rpl_schema = route_schema.RPL(cmpr={'cmpr_i': 0, 'cmpr_e': 0}, + pad={'pad_len': 0}, addresses=[]) with self.assertRaises(ProtocolError) as rpl_ctx: proto._read_data_type_rpl(rpl_schema, header=header) self.assertEqual(str(rpl_ctx.exception), expected) @@ -610,8 +612,8 @@ def make_route(type_: Routing, route: route_data.UnknownType | None = None, *, ip_address('2001:db8::2').packed, ], ) - self.assertGreaterEqual(rpl_bytes.cmpr_i, 0) - self.assertGreaterEqual(rpl_bytes.cmpr_e, 0) + self.assertGreaterEqual(rpl_bytes.cmpr['cmpr_i'], 0) + self.assertGreaterEqual(rpl_bytes.cmpr['cmpr_e'], 0) with mock.patch.object(Internet, '__post_init__', return_value=None) as post_init: post_proto = object.__new__(IPv6_Route) @@ -1519,17 +1521,19 @@ def test_ipv6_route_schema_selector_and_rpl_post_process_branches(self) -> None: first = ip_address('2001:db8::1') second = ip_address('2001:db8::2') - full = route_schema.RPL(cmpr_i=0, cmpr_e=0, pad={'pad_len': 0}, + full = route_schema.RPL(cmpr={'cmpr_i': 0, 'cmpr_e': 0}, pad={'pad_len': 0}, addresses=first.packed + second.packed) full.post_process({}) self.assertEqual([str(item) for item in full.ip], ['2001:db8::1', '2001:db8::2']) suffixes = first.packed[8:] + second.packed[8:] - compressed = route_schema.RPL(cmpr_i=8, cmpr_e=8, pad={'pad_len': 0}, addresses=suffixes) + compressed = route_schema.RPL(cmpr={'cmpr_i': 8, 'cmpr_e': 8}, + pad={'pad_len': 0}, addresses=suffixes) compressed.post_process({}) self.assertEqual(compressed.ip, [first.packed[8:], second.packed[8:]]) - with_dst = route_schema.RPL(cmpr_i=8, cmpr_e=8, pad={'pad_len': 0}, addresses=suffixes) + with_dst = route_schema.RPL(cmpr={'cmpr_i': 8, 'cmpr_e': 8}, + pad={'pad_len': 0}, addresses=suffixes) with_dst.post_process({'dst': ip_address('2001:db8::ffff')}) self.assertEqual([str(item) for item in with_dst.ip], ['2001:db8::1', '2001:db8::2']) @@ -1557,7 +1561,7 @@ def test_ipv6_route_rpl_packs_a_multi_address_list(self) -> None: # Directly at the schema level: ``addresses`` is a ``list[bytes]``, # exactly as ``_make_data_type_rpl`` hands it to the constructor. rpl_schema = route_schema.RPL( - cmpr_i=0, cmpr_e=0, pad={'pad_len': 0}, + cmpr={'cmpr_i': 0, 'cmpr_e': 0}, pad={'pad_len': 0}, addresses=[first.packed, second.packed], ) packed = bytes(rpl_schema) @@ -1575,6 +1579,244 @@ def test_ipv6_route_rpl_packs_a_multi_address_list(self) -> None: self.assertIn(first.packed, header_packed) self.assertIn(second.packed, header_packed) + def test_ipv6_route_rpl_fixed_area_is_four_octets(self) -> None: + """A built RPL header must be exactly as wide as its own ``Hdr Ext Len``. + + See #564. :rfc:`6554#section-3` ("Format of the RPL Routing Header") + draws the routing-data fixed area as a single 32-bit word -- + ``CmprI`` and ``CmprE`` are each a *"4-bit unsigned integer"*, ``Pad`` + likewise, and ``Reserved`` takes the remaining 20 bits -- so it is + **4** octets. The schema declared it as two ``UInt8Field`` plus a + 3-octet ``BitField``, which is **5**, and so built a header one octet + narrower than the ``Hdr Ext Len`` computed from that inflated data + area declared. Measured before the fix, for the two addresses the + round-trip table uses:: + + len(bytes(made)) == 41 # 4 + (5 + 32) + made.length == 5 # ceil((37 - 4) / 8) + 8 + 8 * 5 == 48 # what that Hdr Ext Len declares + + Note RFC 6554 carries no numbered figure captions at all, so the + citation is to Section 3 and never to a "Figure N". + + The assertions below are deliberately split. The ``make`` ones go + through the public entry point and are blind to how the schema spells + its fields, so they fail on the pre-fix tree with those exact numbers + rather than with a constructor error; the schema-level one pins the + nibble placement, which is the thing a reader of the RFC diagram would + check. + + """ + from pcapkit.const.ipv6.routing import Routing + from pcapkit.protocols.internet.ipv6_route import IPv6_Route + from pcapkit.protocols.schema.internet import ipv6_route as route_schema + + # The fixed area alone, with no addresses and no padding. + bare = route_schema.RPL(cmpr={'cmpr_i': 0, 'cmpr_e': 0}, + pad={'pad_len': 0}, addresses=[]) + self.assertEqual(len(bytes(bare)), 4) + + # CmprI is the high nibble of the first octet, CmprE the low one; Pad + # is the high nibble of the second, and Reserved is the 20 bits after + # it. The three trailing octets are the ``padding`` field that + # ``pad_len`` sizes, not part of the fixed area. + nibbles = route_schema.RPL(cmpr={'cmpr_i': 1, 'cmpr_e': 2}, + pad={'pad_len': 3}, addresses=[]) + self.assertEqual(bytes(nibbles)[:4], b'\x12\x30\x00\x00') + + proto = object.__new__(IPv6_Route) + addresses = [ip_address('2001:db8::1'), ip_address('2001:db8::2'), + ip_address('2001:db8::3')] + + # (address count, expected total octets, expected Hdr Ext Len) + for count, expected_octets, expected_hdr_ext_len in ( + (1, 24, 2), + (2, 40, 4), + (3, 56, 6), + ): + with self.subTest(addresses=count): + made = proto.make(type=Routing.RPL_Source_Route_Header, + data={'ip': addresses[:count]}, seg_left=count) + raw = bytes(made) + self.assertEqual(len(raw), expected_octets) + self.assertEqual(made.length, expected_hdr_ext_len) + # the invariant the issue is named after: the header is as wide + # as 'Hdr Ext Len' says, per :rfc:`6554#section-3` -- "Length of + # the Routing header in 8-octet units, not including the first 8 + # octets". + self.assertEqual(8 + 8 * made.length, len(raw)) + self.assertEqual(raw[1], expected_hdr_ext_len) + + def test_ipv6_route_rpl_length_guard_follows_rfc6554_address_arithmetic(self) -> None: + """The reader's length guard must judge ``Hdr Ext Len`` in 8-octet units. + + See #564; the guard is the defect #489 flagged and deliberately left, + for want of a working RPL round trip to validate a replacement + against. It read ``if header.length % 16 != 0``, which is wrong twice + over: ``header.length`` is ``Hdr Ext Len``, *"the length of the + Routing header in 8-octet units"* (:rfc:`6554#section-3`), not an + octet count; and a fixed multiple-of-16 bound assumes 16-octet + addresses, which an SRH only carries when ``CmprI`` and ``CmprE`` are + both 0. Together those rejected every realistic header -- the bound + admitted only ``Hdr Ext Len`` of 0, 16, 32 ..., i.e. nothing under + 136 octets. + + :rfc:`6554#section-4.2` gives the arithmetic that *is* available:: + + n = (((Hdr Ext Len * 8) - Pad - (16 - CmprE)) / (16 - CmprI)) + 1 + + so the invariant is that this division closes: non-negative, and + whole. + + Every case here is a **hand-built wire form**, parsed through the + public constructor. That matters for the same reason it did in #489: a + construct-then-parse round trip can pass on two mistakes cancelling + out, whereas octets written out by hand from the RFC diagram cannot + agree with a wrong reader. + + """ + from pcapkit.const.ipv6.routing import Routing + from pcapkit.protocols.internet.ipv6_route import IPv6_Route + from pcapkit.utilities.exceptions import ProtocolError + + first = ip_address('2001:db8::1') + second = ip_address('2001:db8::2') + + # Uncompressed: Hdr Ext Len 4, CmprI/CmprE/Pad all 0, two full + # addresses. 8 + 8*4 == 40 octets. The old '% 16' bound rejected this. + uncompressed = (bytes([0x11, 0x04, 0x03, 0x02]) + b'\x00\x00\x00\x00' + + first.packed + second.packed) + self.assertEqual(len(uncompressed), 40) + info = IPv6_Route(io.BytesIO(uncompressed), len(uncompressed), extension=True).info + self.assertEqual(info.length, 40) + self.assertEqual(info.cmpr_i, 0) + self.assertEqual(info.cmpr_e, 0) + self.assertEqual(info.pad, 0) + self.assertEqual([str(item) for item in info.ip], ['2001:db8::1', '2001:db8::2']) + + # Compressed: CmprI = CmprE = 4, so every element is 12 octets. Three + # of them is 36, plus Pad of 4 makes 40 == 8 * 5, so Hdr Ext Len is 5 + # and the header is 8 + 40 == 48 octets. RFC 6554 s4.2 closes: + # (5*8 - 4 - (16-4)) / (16-4) == 24 / 12 == 2, so n == 3. + suffixes = b''.join(addr[4:] for addr in ( + first.packed, second.packed, ip_address('2001:db8::3').packed)) + self.assertEqual(len(suffixes), 36) + compressed = (bytes([0x11, 0x05, 0x03, 0x03]) + bytes([0x44, 0x40, 0x00, 0x00]) + + suffixes + bytes(4)) + self.assertEqual(len(compressed), 48) + info = IPv6_Route(io.BytesIO(compressed), len(compressed), extension=True).info + self.assertEqual(info.length, 48) + self.assertEqual((info.cmpr_i, info.cmpr_e, info.pad), (4, 4, 4)) + # All THREE elements, not two. ``post_process`` used to subtract + # ``pad_len`` a second time from a buffer whose own length callback had + # already taken it off, losing one address per ``16 - CmprI`` octets of + # padding -- invisible for as long as the guard rejected every padded + # header before this point could be reached. + self.assertEqual(len(info.ip), 3) + self.assertEqual(list(info.ip), [first.packed[4:], second.packed[4:], + ip_address('2001:db8::3').packed[4:]]) + + # And a header whose arithmetic does not close is still rejected, with + # the in-library exception and the message the round-trip table matches + # on: Pad of 0 leaves 40 - 12 == 28 octets, which is not a whole number + # of 12-octet elements. + malformed = (bytes([0x11, 0x05, 0x03, 0x03]) + bytes([0x44, 0x00, 0x00, 0x00]) + + suffixes + bytes(4)) + with self.assertRaises(ProtocolError) as ctx: + IPv6_Route(io.BytesIO(malformed), len(malformed), extension=True) + self.assertEqual(str(ctx.exception), + f'IPv6-Route: [TypeNo {Routing.RPL_Source_Route_Header}] invalid format') + + # CmprI is 4 bits, so the divisor ``16 - CmprI`` bottoms out at 1 and + # can never be zero. Before #564 ``cmpr_i`` was a whole octet, so a + # hostile packet could set it to 16 and make ``post_process`` divide by + # zero; the field width now rules that out structurally. + max_cmpr = (bytes([0x11, 0x01, 0x03, 0x02]) + bytes([0xff, 0x00, 0x00, 0x00]) + + bytes(8)) + self.assertEqual(len(max_cmpr), 16) + info = IPv6_Route(io.BytesIO(max_cmpr), len(max_cmpr), extension=True).info + self.assertEqual((info.cmpr_i, info.cmpr_e), (15, 15)) + + def test_ipv6_route_rpl_pad_is_zero_when_nothing_is_elided(self) -> None: + """``Pad`` must be 0 when ``CmprI`` and ``CmprE`` are, not a full 8 octets. + + See #564. :rfc:`6554#section-3` is explicit: *"Note that when CmprI + and CmprE are both 0, Pad MUST carry a value of 0."* The compressing + branch of ``_make_data_type_rpl`` computed ``8 - length % 8`` without + the outer ``% 8``, so an address vector that was already 8-octet + aligned got a full 8 octets of padding rather than none -- ``8 - 0`` + is 8. That is reachable through the public API whenever ``dst`` shares + no prefix with the addresses, which is exactly the case that forces + ``CmprI`` and ``CmprE`` to 0. + + ``_make_data_type_none`` in the same module already spelled the idiom + with the outer modulo; this branch did not. + + """ + from pcapkit.const.ipv6.routing import Routing + from pcapkit.protocols.internet.ipv6_route import IPv6_Route + + proto = object.__new__(IPv6_Route) + + # 'fe80::1' shares no leading octet with either address, so the common + # prefix is empty and nothing can be elided. + schema = proto._make_data_type_rpl( + Routing.RPL_Source_Route_Header, + dst=ip_address('fe80::1'), + ip=[ip_address('2001:db8::1'), ip_address('2001:db8::2')], + ) + # ``pad`` is spelled the same before and after #564, so this assertion + # is the measurement rather than a consequence of the reshaped schema: + # it reads 8 on the pre-fix tree. + self.assertEqual(schema.pad['pad_len'], 0) + self.assertEqual(schema.cmpr['cmpr_i'], 0) + self.assertEqual(schema.cmpr['cmpr_e'], 0) + + # and the header that comes out is the same 40 octets the uncompressed + # build produces, rather than 8 longer. + made = proto.make(type=Routing.RPL_Source_Route_Header, + dst=ip_address('fe80::1'), + data={'ip': [ip_address('2001:db8::1'), ip_address('2001:db8::2')]}, + seg_left=2) + self.assertEqual(len(bytes(made)), 40) + self.assertEqual(8 + 8 * made.length, len(bytes(made))) + + def test_ipv6_route_rpl_construction_path_decodes_its_own_addresses(self) -> None: + """Constructing an RPL header must not raise ``AttributeError``. + + See #564. :meth:`Protocol.__post_init__` packs and then unpacks, and + ``IPv6_Route.read`` hands ``_read_data_type_rpl`` the schema ``make`` + just built rather than a freshly parsed one. On that path + ``RPL.post_process`` sees ``addresses`` as the ``list[bytes]`` the + constructor was handed (the #556 case) and returned early without + setting ``ip`` -- so the reader raised a bare, out-of-library + ``AttributeError: 'RPL' object has no attribute 'ip'``. + + It was masked for as long as the ``% 16`` guard rejected every + constructed header first, which is why it only became reachable once + that guard was corrected in the same pass. + + """ + from pcapkit.const.ipv6.routing import Routing + from pcapkit.protocols.internet.ipv6_route import IPv6_Route + + first = ip_address('2001:db8::1') + second = ip_address('2001:db8::2') + + built = IPv6_Route(type=Routing.RPL_Source_Route_Header, + data={'ip': [first, second]}, seg_left=2, payload=b'') + self.assertEqual([str(item) for item in built.info.ip], + ['2001:db8::1', '2001:db8::2']) + self.assertEqual(built.info.length, 40) + + # and the octets it built parse back to the same thing, then rebuild + # byte-for-byte -- the cycle the round-trip table drives. + raw = built.data + parsed = IPv6_Route(io.BytesIO(raw), len(raw), extension=True).info + again = bytes(IPv6_Route(type=parsed.type, data=parsed, next=parsed.next, + seg_left=parsed.seg_left, payload=b'')) + self.assertEqual(raw, again) + def _assert_padding_options_parse_from_the_wire(self, protocol_cls: type) -> None: """A ``Pad1`` option must consume exactly one octet, wherever it sits. diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index d1014876e..b91cf0d78 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -323,31 +323,36 @@ class Gap(NamedTuple): # offset. Both cases round-trip now; entries deleted rather than left # behind, per the note at the top of this table. - # RPL used to fail in ``post_process``, which assumed ``addresses`` was - # bytes -- true after unpacking, false while packing, where it is still the - # list the constructor was handed. That was fixed by #556, and fixing it - # exposed the defect immediately behind it: the reader's own length guard. - # ``header.length`` is ``Hdr Ext Len``, in 8-octet units rather than octets, - # so ``% 16`` cannot be the right invariant -- the same unit confusion #487 - # fixed for Source Route and Type 2. Behind *that* is a third defect (#564): - # the fixed area -- ``cmpr_i`` + ``cmpr_e`` + ``pad`` -- packs to 5 octets, - # one wider than the 4 RFC 6554 specifies and this method's own docstring - # diagram draws, so the header the guard is judging is not well-formed - # either way (measured: it constructs to 41 octets against the 48 its own - # ``Hdr Ext Len`` of 5 declares). None of the three is fixed here: RPL - # addresses are also variable-length under ``cmpr_i``/``cmpr_e``, so no - # fixed bound is obviously correct even once the units and the field - # widths are both right, and nothing has been checked against a real RPL - # capture. Unrelated to #487 (see #476/#480); still open. - 'ipv6-route-type/RPL_Source_Route_Header': Gap( - 'CONSTRUCT', 'IPv6-Route: [TypeNo 3] invalid format', - 'pcapkit/protocols/internet/ipv6_route.py:612 -- the guard rejects ' - 'the header, and the header is not well-formed to begin with: the ' - '5-octet cmpr_i/cmpr_e/pad fixed area is one octet wider than the 4 ' - 'RFC 6554 specifies (echoed in the docstring above), so Hdr Ext Len ' - 'is computed from a mis-sized data area (#564); % 16 additionally ' - 'treats Hdr Ext Len as octets rather than 8-octet units, the same ' - 'confusion #487 fixed for Source Route and Type 2'), + # ``RPL_Source_Route_Header`` used to be recorded here too, behind a stack + # of four defects that had to come off in order -- which is why it outlived + # #487 by several rounds: + # + # 1. ``post_process`` assumed ``addresses`` was bytes -- true after + # unpacking, false while packing, where it is still the list the + # constructor was handed. Fixed by #556, which unmasked the rest. + # 2. The reader's length guard read ``header.length`` (``Hdr Ext Len``, + # in 8-octet units) as an octet count and assumed 16-octet addresses, + # which an SRH only carries when ``CmprI`` and ``CmprE`` are both 0 -- + # the same unit confusion #487 fixed for Source Route and Type 2, + # flagged but deliberately left by #489 for want of a working round + # trip to validate a replacement against. + # 3. Behind that, the schema's fixed area -- ``cmpr_i`` + ``cmpr_e`` + + # ``pad`` -- packed to 5 octets where :rfc:`6554#section-3` gives 4, + # ``CmprI``/``CmprE``/``Pad`` being 4-bit fields sharing one 32-bit + # word with a 20-bit ``Reserved``. Measured before the fix: a + # constructed header of 41 octets against the 48 its own ``Hdr Ext + # Len`` of 5 declared. + # 4. And behind *that*, once the guard stopped rejecting every + # constructed header, ``_read_data_type_rpl`` raised a bare + # ``AttributeError`` on the construction path, because + # ``post_process`` set ``ip`` only when it had parsed octets. + # + # #564 took all four off together -- the guard could not be validated + # against a header whose width was still wrong -- replacing the ``% 16`` + # bound with :rfc:`6554#section-4.2`'s own address-count arithmetic. The + # case round-trips now; entry deleted rather than left behind, per the note + # at the top of this table. The caveat #489 recorded does survive: none of + # it has been checked against a real RPL capture. # -- Mobility Header ------------------------------------------------------ #