diff --git a/pcapkit/corekit/fields/misc.py b/pcapkit/corekit/fields/misc.py index 0170c4d9ea..7f1397ecf7 100644 --- a/pcapkit/corekit/fields/misc.py +++ b/pcapkit/corekit/fields/misc.py @@ -490,6 +490,95 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> '_TC' return self._field.unpack(buffer, packet) +def nested_packet_context(packet: 'dict[str, Any]') -> 'dict[str, Any]': + """Build the packet context handed to a nested schema's field callbacks. + + Args: + packet: The enclosing schema's own packet data. + + Returns: + A plain :class:`dict` holding a shallow copy of ``packet``'s own names, + plus the reserved ``__packet__`` key bound to ``packet`` itself. + + Notes: + A nested schema's field callbacks are written exactly like a top-level + schema's -- ``length=lambda pkt: pkt['length']`` -- so a name the + nested schema does not itself declare has to resolve to the enclosing + schema's value rather than raise :exc:`KeyError`. Copying the + enclosing names in is what gives that, with no lookup protocol to + implement: every mapping operation is :class:`dict`'s own, so + ``pkt[key]``, ``key in pkt``, :meth:`~dict.get`, + :meth:`~dict.setdefault`, :meth:`~dict.pop`, ``==``, iteration and + ``dict(**pkt)`` all behave exactly as a caller reading the code would + expect, and none of them needs an override. + + The enclosing schema is also reachable *unconditionally* under the + reserved ``__packet__`` key, for a callback that needs to name the + outer schema specifically rather than whichever schema happens to + declare a given field -- see + :func:`pcapkit.protocols.schema.misc.pcapng.packet_byteorder` and + :meth:`~pcapkit.protocols.schema.misc.pcapng.BlockType.post_process` + for why that distinction matters, and note that both already + hand-roll this exact fallback and so are unaffected by (and do not + need to route through) this function. + + Nothing written through the returned mapping reaches ``packet``, + because the returned mapping *is* a copy: a nested schema can set -- + or shadow -- a name also declared by the enclosing schema without the + write ever touching the enclosing schema's own data, and without the + write silently disappearing either. That matters concretely rather + than hypothetically: + :class:`~pcapkit.protocols.schema.internet.mh.CGAExtension` declares + its own ``length`` while the option enclosing it declares ``length`` + too, so handing a nested schema the enclosing mapping itself would let + the inner ``length`` overwrite the outer one mid-pack. + + Two consequences of it being a copy rather than a live view, both + deliberate and neither reached by any current call site. A name + deleted from the returned mapping is simply gone, rather than + reverting to the enclosing schema's value. And the copy is taken when + this function is called, so a later mutation of ``packet`` is not + observed through it -- ``__packet__`` remains bound to the live + enclosing mapping for any callback that needs the current value. + + No dedicated class and no :class:`~collections.ChainMap`. Earlier + versions of this function returned each in turn: a + :class:`~collections.ChainMap` first, then a hand-written + :class:`dict` subclass adopted when the ``ChainMap`` was suspected of + corrupting the shared :class:`~abc.ABCMeta` cache every + :class:`Schema ` subclass used + to share on CPython <= 3.10 (issue #439), and then a + :class:`~collections.ChainMap` again once that suspicion was doubted. + A plain :class:`dict` ends the question: it satisfies every + ``packet: 'dict[str, Any]'`` annotation on the rest of the field + classes natively, so no :func:`~typing.cast` is needed at the call + site, and it cannot interact with :class:`~abc.ABCMeta` at all because + :class:`dict` is not an :class:`~abc.ABCMeta`-based class. + + On the #439 suspicion itself, for the record, since it drove two + rewrites: it is *probably* wrong and no longer decidable. What is + directly measured is that the cache keys on the **exact type + queried**, so asking about a :class:`~collections.ChainMap` instance + caches lookups for :class:`~collections.ChainMap` and not for + :class:`dict`, and that the poisoning observed in #439 came from + ordinary code asking :func:`isinstance` about a plain :class:`dict` -- + :func:`~pcapkit.corekit.infoclass.Info.__update__` does exactly that. + Against that, swapping the ``ChainMap`` for a plain literal was, at + the time and on a real CPython 3.10 venv, enough to move + ``test_pcapng_remaining_constructor_branches_and_custom_dispatch`` + between passing and failing, toggled both ways. The likeliest + reconciliation -- that the ``ChainMap`` was never causal but changed + which concrete types flowed through unrelated :func:`isinstance` calls + in the same run, and so changed *when* the pre-existing corruption + fired -- is plausible rather than demonstrated, and cannot now be + tested: #439 has been fixed directly, every :class:`Schema` subclass + gets its own ``_abc_impl``, and the original conditions no longer + exist. It does not affect correctness either way. + + """ + return {**packet, '__packet__': packet} + + class SchemaField(FieldBase[_TS]): """Schema field for protocol schema. @@ -507,7 +596,7 @@ class SchemaField(FieldBase[_TS]): @property def length(self) -> 'int': """Field size.""" - return self._length # type: ignore[has-type] + return self._length @property def optional(self) -> 'bool': @@ -576,9 +665,10 @@ def pack(self, value: 'Optional[_TS | bytes]', packet: 'dict[str, Any]') -> 'byt Packed field value. Notes: - We will use ``packet`` as a ``__packet__`` key in the packet context - passed to the underlying :class:`~pcapkit.protocols.schema.schema.Schema` - for packing purposes. + ``packet`` is reachable from the nested schema's own field + callbacks both under a ``__packet__`` key and, for a name the + nested schema does not itself declare, directly -- see + :func:`~pcapkit.corekit.fields.misc.nested_packet_context`. """ if value is None: @@ -590,9 +680,7 @@ def pack(self, value: 'Optional[_TS | bytes]', packet: 'dict[str, Any]') -> 'byt return value packet.update(self._packet) - return value.pack({ - '__packet__': packet, - }) + return value.pack(nested_packet_context(packet)) def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> '_TS': """Unpack field value from :obj:`bytes`. @@ -605,9 +693,10 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> '_TS' Unpacked field value. Notes: - We will use ``packet`` as a ``__packet__`` key in the packet context - passed to the underlying :class:`~pcapkit.protocols.schema.schema.Schema` - for unpacking purposes. + ``packet`` is reachable from the nested schema's own field + callbacks both under a ``__packet__`` key and, for a name the + nested schema does not itself declare, directly -- see + :func:`~pcapkit.corekit.fields.misc.nested_packet_context`. """ if isinstance(buffer, bytes): @@ -616,9 +705,8 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> '_TS' file = buffer packet.update(self._packet) - return cast('_TS', self._schema.unpack(file, self.length, { # type: ignore[call-arg,misc] - '__packet__': packet, - })) + return cast('_TS', self._schema.unpack(file, self.length, # type: ignore[call-arg,misc] + nested_packet_context(packet))) class ForwardMatchField(FieldBase[_TC]): diff --git a/pcapkit/protocols/schema/schema.py b/pcapkit/protocols/schema/schema.py index 1cab73401e..e0b5cbeeb8 100644 --- a/pcapkit/protocols/schema/schema.py +++ b/pcapkit/protocols/schema/schema.py @@ -753,6 +753,16 @@ def unpack(cls, data: 'bytes | IO[bytes]', of the remaining data, which is used to determine the length of the payload field. + When this schema is nested -- unpacked through a + :class:`~pcapkit.corekit.fields.misc.SchemaField` rather than + directly -- ``packet`` is not the enclosing schema's own data, but + a context built by :func:`~pcapkit.corekit.fields.misc. + nested_packet_context`: a name this schema does not itself + declare falls through to the enclosing schema, and the enclosing + schema is also reachable unconditionally under a ``__packet__`` + key. See that function for the exact lookup, write and iteration + semantics. + And an ``__option_padding__`` key in the ``packet`` to record how much of an :class:`~pcapkit.corekit.fields.collections.OptionField`'s declared diff --git a/tests/corekit/test_fields_misc_packet_context.py b/tests/corekit/test_fields_misc_packet_context.py new file mode 100644 index 0000000000..e2f3c8280d --- /dev/null +++ b/tests/corekit/test_fields_misc_packet_context.py @@ -0,0 +1,274 @@ +from __future__ import annotations + +import importlib.util +import sys +import unittest +from unittest import mock + +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) + + +class NestedPacketContextSemanticsTests(unittest.TestCase): + """The packet context a nested schema's field callbacks see -- issue #445. + + ``SchemaField.pack``/``unpack`` used to hand a nested schema a nested + context whose *only* content was ``{'__packet__': packet}``: a name the + nested schema declares itself resolves normally, but a name it does not + declare -- which every top-level schema writes as ``pkt['length']`` and + which a nested one inherited unmodified -- raised :exc:`KeyError` instead + of reaching the enclosing schema. ``pcapkit.corekit.fields.misc. + nested_packet_context`` replaces that literal with a two-level + :class:`collections.ChainMap`, so a name absent locally falls through to + the enclosing schema, while ``__packet__`` keeps naming it explicitly. + + :meth:`test_nested_schema_reads_enclosing_field_by_name_and_does_not_leak_writes` + is the load-bearing case: it fails with the recorded ``KeyError`` before + the fix and passes after, and in the same pass checks that the mapping + does not confuse a name the nested schema shadows with the enclosing + schema's own, and that nothing the nested schema writes through the + mapping is ever written back to the enclosing schema's own data. + + An intermediate version of this fix used a hand-written :class:`dict` + subclass instead of :class:`collections.ChainMap`, adopted when a bare + ``ChainMap`` was suspected of corrupting a shared + :class:`~abc.ABCMeta` cache on CPython <= 3.10 (issue #439) -- a suspicion + that is probably wrong but is no longer decidable, since #439's direct fix + removed the mechanism; see + :func:`pcapkit.corekit.fields.misc.nested_packet_context` for why it is + recorded as two measurements that do not fully reconcile rather than as a + settled reversal. Both the suspicion and the workaround it produced have + since been retired: #439 was fixed directly, and the hand-written + subclass's own + ``.setdefault()`` bypassed the fallback the same way :class:`dict`'s + built-in one does, silently inserting a name locally instead of + honouring what the enclosing schema already had for it -- + :meth:`test_nested_schema_reads_enclosing_field_by_name_and_does_not_leak_writes` + pins the corrected behaviour (``setdefault`` on a name absent locally but + present in the parent returns the parent's value rather than inserting a + new one) precisely because that was the gap. See + :func:`pcapkit.corekit.fields.misc.nested_packet_context` for the full + history. + + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def _make_schema_classes(self): + """Outer/Inner pair mirroring ``CGAParametersOption``/``CGAParameter``. + + ``Inner`` declares its own ``tag`` (shadowing ``Outer.tag``, with a + different value) and a ``body`` sized from ``pkt['length']`` -- a name + only ``Outer`` declares. ``captured`` is filled in by ``Inner. + post_process``, which runs after every one of ``Inner``'s own fields + has been parsed and set, so it can inspect the fully populated packet + context: the shadowed name, the name that had to fall through, an + explicit ``__packet__`` lookup, ``in``, ``.get()`` and the mapping's + own keys. + + """ + from pcapkit.corekit.fields.misc import SchemaField + from pcapkit.corekit.fields.numbers import UInt8Field + from pcapkit.corekit.fields.strings import BytesField + from pcapkit.protocols.schema.schema import Schema, schema_final + + captured = {} + + @schema_final + class Inner(Schema): + """No 'length' of its own; 'tag' shadows the enclosing schema's.""" + + tag: int = UInt8Field() + body: bytes = BytesField(length=lambda pkt: pkt['length'] - 1) + + def post_process(self, packet): + captured['own_tag'] = packet['tag'] + captured['fallback_length'] = packet['length'] + captured['explicit_outer_tag'] = packet['__packet__']['tag'] + captured['has_length'] = 'length' in packet + captured['has_missing'] = 'no_such_name' in packet + captured['get_length'] = packet.get('length', 'sentinel') + captured['get_missing'] = packet.get('no_such_name', 'sentinel') + captured['keys'] = set(packet.keys()) + captured['dict_star'] = dict(**packet) + # setdefault on a name absent locally but present in the + # parent must honour the fallback (return 42, not overwrite + # it with 999) -- a plain dict subclass's own setdefault + # bypasses __missing__ the same way get/__contains__ do, and + # would insert 999 locally instead. ChainMap.setdefault + # delegates through __contains__/__getitem__, so it does not. + captured['setdefault_existing'] = packet.setdefault('length', 999) + captured['setdefault_new'] = packet.setdefault('brand_new', 7) + # A copy must keep seeing the parent, and writes to the copy + # must stay local to the copy (never touch the original, and + # never touch the shared parent either). + snapshot = packet.copy() + snapshot['tag'] = 0xFF + captured['copy_sees_parent_length'] = snapshot['length'] + captured['copy_write_did_not_leak_to_original'] = packet['tag'] + # A live reference, not a copy: read again after ``Outer`` + # finishes, to prove this schema's writes never landed in it. + captured['parent_ref'] = packet['__packet__'] + return self + + @schema_final + class Outer(Schema): + tag: int = UInt8Field() + length: int = UInt8Field() + inner: Inner = SchemaField(length=lambda pkt: pkt['length'], schema=Inner) + + return Inner, Outer, captured + + def test_nested_schema_reads_enclosing_field_by_name_and_does_not_leak_writes(self) -> None: + _Inner, Outer, captured = self._make_schema_classes() + + # tag=0xAA, length=3 (Inner's own total size), then Inner's own + # tag=0x22 (one octet) and a 2-octet body. + raw = b'\xaa\x03\x22\xbb\xcc' + outer = Outer.unpack(raw, len(raw), None) + + # The bug, fixed: 'length' is not Inner's own field, and resolves to + # Outer's by falling through rather than raising KeyError. + self.assertEqual(outer.inner.body, b'\xbb\xcc') + self.assertEqual(captured['fallback_length'], 3) + + # Shadowing: Inner's own 'tag' (0x22) is not confused with Outer's + # (0xAA), and the latter is still reachable explicitly. + self.assertEqual(captured['own_tag'], 0x22) + self.assertEqual(outer.tag, 0xAA) + self.assertEqual(captured['explicit_outer_tag'], 0xAA) + + # ``in`` and ``.get()`` honour the same fallback as ``__getitem__``. + self.assertTrue(captured['has_length']) + self.assertFalse(captured['has_missing']) + self.assertEqual(captured['get_length'], 3) + self.assertEqual(captured['get_missing'], 'sentinel') + + # Iterating the mapping sees the union of both levels, however it is + # asked for: .keys(), or plain dict(**pkt). + expected_keys = {'__packet__', '__length__', 'tag', 'length', 'body'} + self.assertEqual(captured['keys'], expected_keys) + self.assertEqual(set(captured['dict_star']), expected_keys) + + # setdefault falls through to the parent for a name absent locally + # (returning its existing value, 3, not inserting a new local 999), + # and behaves like a normal dict.setdefault for a name absent + # everywhere. + self.assertEqual(captured['setdefault_existing'], 3) + self.assertEqual(captured['setdefault_new'], 7) + + # A copy still sees the parent, and a write to the copy never + # reaches the original it was copied from. + self.assertEqual(captured['copy_sees_parent_length'], 3) + self.assertEqual(captured['copy_write_did_not_leak_to_original'], 0x22) + + # The write path: nothing Inner set (its own 'tag', 'body', ...) is + # visible on Outer's own packet data once Outer is done. Outer's own + # 'tag' is exactly what it was, not clobbered by Inner's shadowing + # write of the same name. + parent = captured['parent_ref'] + self.assertEqual(parent['tag'], 0xAA) + self.assertNotIn('body', parent) + + def test_pcapng_byteorder_consumer_still_works_with_both_shapes(self) -> None: + """The one existing, hand-rolled ``__packet__`` fallback is unaffected. + + ``packet_byteorder`` (``pcapkit/protocols/schema/misc/pcapng.py:168``) + is called both with a plain dict built by hand -- as several tests and + :func:`~pcapkit.foundation.engines.pcapng` construct -- and with what + ``SchemaField`` now actually builds. Both must keep working, and + neither is expected to change: the site already implements the + fallback itself and does not route through + :func:`~pcapkit.corekit.fields.misc.nested_packet_context`. + + """ + from pcapkit.corekit.fields.misc import nested_packet_context + from pcapkit.protocols.schema.misc.pcapng import packet_byteorder + + outer = {'byteorder': 'little'} + + # Hand-built, the shape every existing caller uses. + self.assertEqual(packet_byteorder({'__packet__': outer}), 'little') + # What SchemaField hands a real nested schema now. + self.assertEqual(packet_byteorder(nested_packet_context(outer)), 'little') + # No enclosing packet at all -- the top-level case. + self.assertEqual(packet_byteorder({}), sys.byteorder) + # The nested schema's own 'byteorder' takes precedence either way. + self.assertEqual(packet_byteorder({'byteorder': 'big', '__packet__': outer}), 'big') + local = nested_packet_context(outer) + local['byteorder'] = 'big' + self.assertEqual(packet_byteorder(local), 'big') + + @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') + def test_pcapng_block_type_mismatch_consumer_still_works_with_both_shapes(self) -> None: + """The other hand-rolled fallback, ``BlockType.post_process``. + + Mirrors ``tests/protocols/misc/test_pcapng_unit.py``'s own + ``UnknownBlock(...).post_process({'__packet__': {...}})`` call, and + additionally checks it still resolves the enclosing block's type when + handed what ``SchemaField`` builds today. + + """ + from pcapkit.corekit.fields.misc import nested_packet_context + from pcapkit.const.pcapng.block_type import BlockType + import pcapkit.protocols.schema.misc.pcapng as schema_pcapng + + mismatch = schema_pcapng.UnknownBlock(length=16, body=b'abcd', length2=20) + outer = {'type': BlockType.Reserved_0x00000000} + + # A found outer 'type' renders its code into the message; the + # fallback default, 'N/A', is what a lookup miss would show instead. + with mock.patch('pcapkit.protocols.schema.misc.pcapng.warn') as warn: + mismatch.post_process({'__packet__': outer}) + warn.assert_called_once() + self.assertNotIn('N/A', warn.call_args.args[0]) + + with mock.patch('pcapkit.protocols.schema.misc.pcapng.warn') as warn: + mismatch.post_process(nested_packet_context(outer)) + warn.assert_called_once() + self.assertNotIn('N/A', warn.call_args.args[0]) + + +class CGAParametersRegressionTests(unittest.TestCase): + """The measured casualty from issue #445, end to end. + + A CGA Parameters option could not be parsed at all: sizing + ``CGAParameter.extensions`` (``pcapkit/protocols/schema/internet/mh.py``, + ``CGAParameter.extensions``) reads ``pkt['length']``, a name only the + enclosing ``CGAParametersOption`` declares. No change to ``mh.py`` itself + was needed -- the fallback in ``nested_packet_context`` resolves it. + + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_cga_parameters_option_now_parses_end_to_end(self) -> None: + from pcapkit.const.mh.option import Option as Enum_Option + from pcapkit.protocols.internet.mh import MH + + # The exact 40-octet reproduction from issue #445. + raw = bytes.fromhex( + '11040000123400000c1e' + '0000000000000000000000000000086f' + '0000000020010db8' + '00' + '3003010203' + ) + self.assertEqual(len(raw), 40) + + # #446 (ForwardMatchField counted into Schema.__len__, fixed on + # #456/main) was a second, separate defect blocking this exact + # option: with #445 alone, this reached FieldValueError: Field + # parameters has invalid length rather than parsing. With both + # fixes applied, CGA Parameters parses end to end. + m = MH(raw, len(raw), extension=True) + option = m.info.options[Enum_Option.CGA_Parameters] + parameter = option.parameters[0] + self.assertEqual(parameter.prefix, 0x20010db8) + self.assertEqual(parameter.collision_count, 0) + self.assertEqual(parameter.public_key, b'\x30\x03\x01\x02\x03') + self.assertEqual(len(parameter.extensions), 0) diff --git a/tests/protocols/internet/test_mh_unit.py b/tests/protocols/internet/test_mh_unit.py index 24044c926a..42ffe9bbe3 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -1885,10 +1885,12 @@ def test_mh_pmipv6_options_round_trip_byte_for_byte(self) -> None: raise. Note: - :attr:`~pcapkit.const.mh.option.Option.CGA_Parameters` is absent, and - deliberately so: it cannot be parsed at all, on this branch or on - ``main``. See - :meth:`test_mh_cga_parameters_option_is_unparsable_upstream`. + :attr:`~pcapkit.const.mh.option.Option.CGA_Parameters` is absent, + deliberately: it now parses (see + :meth:`test_mh_cga_parameters_option_now_parses`), but nobody has + verified this test's stricter round-trip identity for it yet -- + that is a separate, deliberate scope decision left for whoever + takes it on next, not implied by parsing alone. """ import ipaddress @@ -2574,23 +2576,28 @@ def test_mh_nested_suboptions_build_from_raw_kwargs(self) -> None: {'vendor': 32473, 'subtype': 3, 'data': payload})]) self.assertIn(payload, option.pack()) - def test_mh_cga_parameters_option_is_unparsable_upstream(self) -> None: - """The CGA Parameters option cannot be parsed, and this is not new. - - :attr:`~pcapkit.protocols.schema.internet.mh.CGAParameter.extensions` sizes - itself from ``pkt['length']``, but :class:`CGAParameter - ` has no ``length`` - field of its own and - :class:`~pcapkit.corekit.fields.misc.SchemaField` hands a nested schema a - fresh packet dict rather than the enclosing option's, so the lookup fails. - A well-formed option therefore raises :exc:`KeyError` on parse. - - This is recorded rather than fixed: the remaining half of the fault is in - how a :class:`~pcapkit.corekit.fields.misc.ForwardMatchField` is counted - towards the nested schema's length, which is shared field machinery well - outside the mobility header. The test pins the *current* behaviour so that - whoever fixes it finds out here. + def test_mh_cga_parameters_option_now_parses(self) -> None: + """The CGA Parameters option parses -- this used to be pinned as broken. + + This test used to be + ``test_mh_cga_parameters_option_is_unparsable_upstream``, pinning a + :exc:`KeyError` on the exact reproduction below: sizing + :attr:`~pcapkit.protocols.schema.internet.mh.CGAParameter.extensions` + read ``pkt['length']``, a name only the enclosing + :class:`~pcapkit.protocols.schema.internet.mh.CGAParametersOption` + declared, and :class:`~pcapkit.corekit.fields.misc.SchemaField` handed + a nested schema a fresh packet dict rather than the enclosing + option's. Fixed by #445 (the nested lookup now falls through to the + enclosing schema). The second half of the fault this test's own + docstring named -- a :class:`~pcapkit.corekit.fields.misc. + ForwardMatchField` miscounted into the nested schema's length -- was + fixed separately, by #446/#456. Both landed, so the option this test + exists for now parses end to end; see also + :meth:`tests.corekit.test_fields_misc_packet_context. + CGAParametersRegressionTests.test_cga_parameters_option_now_parses_end_to_end` + for the same reproduction asserting the parsed fields. """ + from pcapkit.const.mh.option import Option from pcapkit.protocols.internet.mh import MH # type 12, 30 octets: a 16-octet modifier, 8-octet subnet prefix, one @@ -2602,9 +2609,10 @@ def test_mh_cga_parameters_option_is_unparsable_upstream(self) -> None: '3003010203') self.assertEqual(len(raw), 40) - with self.assertRaises(KeyError) as caught: - MH(io.BytesIO(raw), len(raw), extension=True) - self.assertEqual(caught.exception.args[0], 'length') + mh = MH(io.BytesIO(raw), len(raw), extension=True) + option = mh.info.options[Option.CGA_Parameters] + self.assertEqual(len(option.parameters), 1) + self.assertEqual(option.parameters[0].prefix, 0x20010db8) if __name__ == '__main__': diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index 152e23d7b9..ab8b7977b1 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -300,31 +300,15 @@ class Gap(NamedTuple): 'assumes bytes; it runs on the pack path too, from schema.py:647'), # -- Mobility Header ------------------------------------------------------ - - # ``CGAParameter``'s nested length callback reads ``pkt['length']``, which - # is present while packing and absent while unpacking: ``SchemaField.unpack`` - # starts the nested schema with a fresh context whose parent is under - # ``__packet__``. A CGA extension has no other carrier, so the whole - # ``MH.__extension__`` registry is unreachable through the public API -- - # every code in it fails here, identically, before its own schema is ever - # unpacked. That is #445, and it is why all four entries below name one site - # in ``CGAParameter`` rather than anything in the extensions themselves. - 'mh-extension/Multi_Prefix': Gap( - 'PARSE', "KeyError: 'length'", - 'pcapkit/protocols/schema/internet/mh.py:873 -- #445; needs ' - "pkt['__packet__']['length'] on the unpack path"), - 'mh-extension/Exp_FFFD': Gap( - 'PARSE', "KeyError: 'length'", - 'pcapkit/protocols/schema/internet/mh.py:873 -- #445; needs ' - "pkt['__packet__']['length'] on the unpack path"), - 'mh-extension/Exp_FFFE': Gap( - 'PARSE', "KeyError: 'length'", - 'pcapkit/protocols/schema/internet/mh.py:873 -- #445; needs ' - "pkt['__packet__']['length'] on the unpack path"), - 'mh-extension/Exp_FFFF': Gap( - 'PARSE', "KeyError: 'length'", - 'pcapkit/protocols/schema/internet/mh.py:873 -- #445; needs ' - "pkt['__packet__']['length'] on the unpack path"), + # + # ``mh-extension/{Multi_Prefix,Exp_FFFD,Exp_FFFE,Exp_FFFF}`` all round-trip + # cleanly now that #445, #437 and #446 are all applied together: #445 let + # ``CGAParameter.extensions`` size itself instead of raising + # ``KeyError: 'length'``, #437 (merged as registry completion) both + # registered the three experimental codes and fixed + # ``_make_ext_multiprefix``'s bogus length arithmetic, and #446/#456 fixed + # the ``ForwardMatchField`` double-count that stopped ``CGAParametersOption + # .parameters`` from sizing correctly. No entries needed here any more. # -- HIP ------------------------------------------------------------------ @@ -396,22 +380,26 @@ class Gap(NamedTuple): # -- HTTP/2 --------------------------------------------------------------- - # ``SchemaField.pack`` gives a nested frame schema a fresh packet context - # whose only link to the parent is ``__packet__``, but six frame schemas - # reach for the HTTP/2 header's ``flags`` bitfield directly -- either from a - # ConditionalField test or from ``FrameType.post_process``. The three frames - # that pass are exactly the three declaring no flag members at all: - # RST_STREAM, GOAWAY and WINDOW_UPDATE. - **{ - f'httpv2-frame/{name}': Gap( - 'CONSTRUCT', "KeyError: 'flags'", - 'pcapkit/protocols/schema/application/httpv2.py:144 ' - '(FrameType.post_process) and the pad_len ConditionalField tests at ' - ':175, :204, :305 -- the nested context reaches for the parent ' - "header's flags") - for name in ('DATA', 'HEADERS', 'SETTINGS', 'PUSH_PROMISE', 'PING', - 'CONTINUATION') - }, + # ``SchemaField.pack`` used to give a nested frame schema a fresh packet + # context whose only link to the parent was ``__packet__``, and six frame + # schemas reach for the HTTP/2 header's ``flags`` bitfield directly -- + # either from a ConditionalField test or from ``FrameType.post_process``. + # #445 makes a name absent from the nested schema fall through to the + # parent instead of raising, which fixed that for all six. RST_STREAM, + # GOAWAY and WINDOW_UPDATE already passed, declaring no flag members at + # all; the other five each got past ``flags`` and hit their own, + # unrelated defect in turn -- and every one of those has since been + # fixed and merged too, so none of the six needs an entry any more: + # + # - PUSH_PROMISE, PING: round-tripped cleanly as soon as #445 landed. + # - DATA, HEADERS, CONTINUATION: hit ``decorators.py``'s ``@prepare`` + # treating a zero-length nested unpack (a frame with no payload) as + # end-of-file. Filed as #458, fixed and merged as #461 (``prepare`` now + # distinguishes a *declared* zero length from a *derived* one). + # - SETTINGS: hit ``SettingsFrame.settings`` declaring + # ``item_type=SettingPair`` (the raw schema class) instead of + # ``SchemaField(schema=SettingPair)``. Filed as #459, fixed and merged + # as #462. # ``make`` writes ``length = payload + 9`` and a PRIORITY payload is five # octets, so the constructed header always says 14 -- while the reader