From 46fe59ad46d8cfac8909e7cf49818963ffbc21e7 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 17 Sep 2026 20:42:30 -0400 Subject: [PATCH 1/9] corekit: let a nested schema's field callbacks reach the enclosing schema - SchemaField.pack/unpack handed a nested schema a context of only {'__packet__': packet}, so a callback written the ordinary way -- length=lambda pkt: pkt['length'] -- raised KeyError as soon as a schema was nested. Measured casualty: CGA Parameters (mh.py CGAParameter.extensions), unparsable via the public API. - Fix: nested_packet_context() replaces that literal with a two-level collections.ChainMap, so a name absent locally falls through to the enclosing schema, while __packet__ still reaches it explicitly. ChainMap.__setitem__/__delitem__ always act on the nested map, so a write never reaches the parent and a shadowed name is read locally. - mh.py needed no change: CGAParameter.extensions's pkt['length'] now resolves via the fallback. CGA Parameters still does not parse -- it now reaches FieldValueError: Field parameters has invalid length (#446, ForwardMatchField vs. Schema.__len__), which is not fixed here. - pcapng.py's two __packet__ consumers are untouched: real callers hand them a plain {'__packet__': {...}} dict rather than going through SchemaField, so their hand-rolled fallback still has to handle that shape and is not redundant with this change. - Documented the __packet__ contract in nested_packet_context, and added it to Schema.unpack's reserved-key list alongside __length__ and __option_padding__. - The same latent defect affected six HTTP/2 frame schemas (reading the header's 'flags' the same way); updated tests/protocols/test_option_roundtrip_unit.py's EXPECTED_FAILURES: removed PUSH_PROMISE and PING (now round-trip cleanly), and re-attributed Multi_Prefix, DATA, HEADERS, CONTINUATION and SETTINGS to the distinct defects this fix newly exposes underneath. Ran PYTHONSAFEPATH=1 python -m pytest tests -q at baseline e2d8ed6d1 (960 passed, 17 skipped, 1264 subtests) and after (see PR description). --- pcapkit/corekit/fields/misc.py | 69 +++++- pcapkit/protocols/schema/schema.py | 10 + .../test_fields_misc_packet_context.py | 218 ++++++++++++++++++ tests/protocols/test_option_roundtrip_unit.py | 78 +++++-- 4 files changed, 342 insertions(+), 33 deletions(-) create mode 100644 tests/corekit/test_fields_misc_packet_context.py diff --git a/pcapkit/corekit/fields/misc.py b/pcapkit/corekit/fields/misc.py index 0170c4d9ea..476efeecb4 100644 --- a/pcapkit/corekit/fields/misc.py +++ b/pcapkit/corekit/fields/misc.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- """miscellaneous field class""" +import collections import copy import io from typing import TYPE_CHECKING, TypeVar, cast @@ -490,6 +491,51 @@ 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]') -> 'collections.ChainMap[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 two-level :class:`collections.ChainMap`: the nested schema's own + packet data (initially holding only the reserved ``__packet__`` key) + chained in front of ``packet``. + + 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 should fall through to + the enclosing schema rather than raise :exc:`KeyError`. That is what + :meth:`~collections.ChainMap.__getitem__`, ``in`` and :meth:`get + ` do here: they check the nested schema's + own data first and the enclosing schema's second. Iterating the + mapping (or calling :meth:`~collections.ChainMap.keys`) sees the + union of both, with the nested schema's own names taking precedence + where the two overlap. + + 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 helper. + + Nothing written through the returned mapping is written back to + ``packet``: :class:`~collections.ChainMap` always resolves + :meth:`~collections.ChainMap.__setitem__` and + :meth:`~collections.ChainMap.__delitem__` against its first map, so a + nested schema can set -- or shadow -- a name also declared by the + enclosing schema without either write ever reaching the enclosing + schema's own data, and without a write silently disappearing either. + + """ + return collections.ChainMap({'__packet__': packet}, packet) + + class SchemaField(FieldBase[_TS]): """Schema field for protocol schema. @@ -576,9 +622,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 +637,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 +650,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 +662,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 8a61b59a2c..65e9202f0c 100644 --- a/pcapkit/protocols/schema/schema.py +++ b/pcapkit/protocols/schema/schema.py @@ -680,6 +680,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..baf3ffa3e3 --- /dev/null +++ b/tests/corekit/test_fields_misc_packet_context.py @@ -0,0 +1,218 @@ +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 NestedPacketContextTests(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. + + """ + + 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()) + # 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. + self.assertEqual(captured['keys'], + {'__packet__', '__length__', 'tag', 'length', 'body'}) + + # 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_reaches_the_446_boundary_not_a_keyerror(self) -> None: + from pcapkit.protocols.internet.mh import MH + from pcapkit.utilities.exceptions import FieldValueError + + # 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__) is a separate, + # already-filed defect and is not fixed here: CGA Parameters still + # does not parse. What #445 fixes is *which* error that is -- no + # longer a KeyError out of CGAParameter.extensions. + with self.assertRaises(FieldValueError) as ctx: + MH(raw, len(raw), extension=True) + self.assertIn('parameters has invalid length', str(ctx.exception)) diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index f7830b7a65..ee4f8abf90 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -324,15 +324,25 @@ class Gap(NamedTuple): # -- 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. + # ``CGAParameter``'s nested length callback used to read ``pkt['length']`` + # from a context that held it only under ``__packet__`` while unpacking -- + # fixed by #445, which makes a name the nested schema does not itself + # declare fall through to the enclosing schema instead of raising. That + # unblocks ``CGAParameter.extensions`` and lets parsing reach the + # extension itself, exposing a second, unrelated defect #445's fix was + # never going to reach: ``_make_ext_multiprefix`` writes + # ``length=1 + len(prefixes) * 16`` -- 17 octets for one prefix -- where + # the wire format wants 4 (the flags) plus 8 per prefix, i.e. 12. That is + # already tracked and fixed on open PR #437 ("declared 1 + len(prefixes) + # * 16 data octets for a 4 + len(prefixes) * 8 payload"), which this + # branch does not carry yet -- not a new, untracked defect. 'mh-extension/Multi_Prefix': Gap( - 'PARSE', "KeyError: 'length'", - 'pcapkit/protocols/schema/internet/mh.py:516 -- needs ' - "pkt['__packet__']['length'] on the unpack path"), + 'PARSE', 'Field prefixes has invalid length', + 'pcapkit/protocols/internet/mh.py:3864 -- _make_ext_multiprefix ' + 'writes length=1 + len(prefixes) * 16 (17 for one prefix) instead of ' + '4 + len(prefixes) * 8 (12); fixed on open PR #437, not yet merged; ' + "unreachable before #445 fixed the KeyError: 'length' this hit " + 'first'), # -- HIP ------------------------------------------------------------------ @@ -389,23 +399,49 @@ 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. + # ``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 fixes that for all six. RST_STREAM, + # GOAWAY and WINDOW_UPDATE already passed, declaring no flag members at + # all; PUSH_PROMISE and PING now round-trip cleanly too, so their entries + # are gone. The other three get past ``flags`` and hit their own, + # unrelated defects. + # + # DATA, HEADERS and CONTINUATION carry no payload in the case this suite + # constructs, so the wire correctly encodes a 9-octet, header-only frame + # -- and reading it back asks ``SchemaField.unpack`` to unpack the frame + # body schema from 0 remaining octets. ``Schema.unpack``'s ``@prepare`` + # decorator treats *any* zero-length unpack as end-of-file and raises + # unconditionally, which is right for the outermost read and wrong here: + # an all-default, zero-octet frame body is a valid schema instance, not + # an empty stream. **{ 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') + 'PARSE', 'EOFError', + 'pcapkit/utilities/decorators.py:222 (prepare) -- raises ' + 'EOFError for any zero-length nested unpack, but a frame with no ' + 'payload legitimately unpacks its body from 0 octets; ' + "unreachable before #445 fixed the KeyError: 'flags' this hit " + 'first') + for name in ('DATA', 'HEADERS', 'CONTINUATION') }, + # ``SettingsFrame.settings`` declares ``item_type=SettingPair``, the raw + # schema class, rather than ``SchemaField(schema=SettingPair)``. So + # ``ListField.unpack``'s non-schema branch calls ``SettingPair(packet)``, + # binding the packet dict to ``SettingPair``'s first field (``id``) + # instead of constructing a field to read with, and then asks the result + # for a ``.length`` no ``Schema`` provides. + 'httpv2-frame/SETTINGS': Gap( + 'PARSE', "'SettingPair' object has no attribute 'length'", + 'pcapkit/protocols/schema/application/httpv2.py:285 -- settings ' + 'declares item_type=SettingPair instead of ' + 'SchemaField(schema=SettingPair); unreachable before #445 fixed the ' + "KeyError: 'flags' this hit first"), + # ``make`` writes ``length = payload + 9`` and a PRIORITY payload is five # octets, so the constructed header always says 14 -- while the reader # demands exactly 9. Its siblings all check payload+9 (RST_STREAM 13, From 86370d7d55ae7c11e52261b1beec08142a18d2cf Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 17 Sep 2026 22:24:54 -0400 Subject: [PATCH 2/9] corekit: stop nested_packet_context from disturbing the #439 abc cache - The previous NestedPacketContext used collections.ChainMap directly. ChainMap is itself a collections.abc.MutableMapping, and constructing one was enough to disturb the shared _abc_impl cache that every Schema subclass uses on CPython <= 3.10 (#439): a later, unrelated isinstance(some_dict, Schema) check in test_pcapng_remaining_constructor_branches_and_custom_dispatch flipped from False to True, raising AttributeError: 'dict' object has no attribute 'to_dict' from Schema.to_dict (schema.py:520). - Measured directly: with everything else unchanged, reverting only the ChainMap call back to a plain {'__packet__': packet} literal makes that test pass again on Python 3.10; restoring it reproduces the failure. Confirmed both in an isolated single-test run and via a local Python 3.10 venv (943 passed, 0 failed after this commit, against a failure before it). - Fix: NestedPacketContext is now a plain class implementing __getitem__/__setitem__/__delitem__/__contains__/__iter__/__len__/ get/update/copy/keys/values/items by hand, with none of it deriving from collections.abc. Same two-level fallback, same __packet__ reachability, same write isolation as before -- only the mechanism changes, not the contract. All existing tests pass unchanged. - Not a fix to #439 itself, which remains filed and untouched; this only stops this PR's own code from being the thing that trips it. Verified: PYTHONSAFEPATH=1 pytest tests -q on Python 3.14 (981 passed, 17 skipped, 1267 subtests, 0 failed) and on a local Python 3.10 venv (943 passed, 55 skipped, 1237 subtests, 0 failed -- the skip count differs only because that venv lacks some optional runtime deps). --- pcapkit/corekit/fields/misc.py | 166 +++++++++++++++++++++++++-------- 1 file changed, 128 insertions(+), 38 deletions(-) diff --git a/pcapkit/corekit/fields/misc.py b/pcapkit/corekit/fields/misc.py index 476efeecb4..653e3148b3 100644 --- a/pcapkit/corekit/fields/misc.py +++ b/pcapkit/corekit/fields/misc.py @@ -1,7 +1,6 @@ # -*- coding: utf-8 -*- """miscellaneous field class""" -import collections import copy import io from typing import TYPE_CHECKING, TypeVar, cast @@ -12,11 +11,11 @@ __all__ = [ 'ConditionalField', 'PayloadField', 'SwitchField', 'ForwardMatchField', - 'NoValueField', + 'NoValueField', 'NestedPacketContext', ] if TYPE_CHECKING: - from typing import IO, Any, Callable, Optional, Type + from typing import IO, Any, Callable, Iterator, Optional, Type from typing_extensions import Self @@ -491,49 +490,140 @@ 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]') -> 'collections.ChainMap[str, Any]': +class NestedPacketContext: + """Packet context handed to a nested schema's field callbacks. + + Args: + packet: The enclosing schema's own packet data. + + 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 should fall through to the enclosing + schema rather than raise :exc:`KeyError`. That is what + :meth:`__getitem__`, ``in`` and :meth:`get` do here: they check the + nested schema's own data first and the enclosing schema's second. + Iterating this mapping sees the union of both, with the nested schema's + own names taking precedence where the two overlap. + + 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 class. + + Nothing written through an instance is written back to ``packet``: + :meth:`__setitem__` and :meth:`__delitem__` always act on the nested + schema's own data, so a nested schema can set -- or shadow -- a name + also declared by the enclosing schema without either write ever + reaching the enclosing schema's own data, and without a write silently + disappearing either. + + Deliberately **not** a :class:`collections.abc.Mapping`. On CPython <= + 3.10, every :class:`~pcapkit.protocols.schema.schema.Schema` subclass + shares ``Schema``'s ``_abc_impl`` cache, so ``isinstance``/``issubclass`` + against any of them can return whichever answer was asked first (#439). + An earlier version of this class returned a bare + :class:`collections.ChainMap`, itself a + :class:`collections.abc.MutableMapping`, and using one here was enough to + disturb that shared cache and flip an unrelated, later + ``isinstance(some_dict, Schema)`` check elsewhere in the same process + from :data:`False` to :data:`True` -- + ``test_pcapng_remaining_constructor_branches_and_custom_dispatch`` broke + on Python 3.10 alone, confirmed by reverting *only* the ``ChainMap`` call + (nothing else) and watching it pass again. Implementing the same + two-level fallback over a plain object rather than an ABC avoids + touching that cache at all. + + """ + + __slots__ = ('_local', '_parent') + + def __init__(self, packet: 'dict[str, Any]') -> 'None': + self._local = {'__packet__': packet} # type: dict[str, Any] + self._parent = packet # type: dict[str, Any] + + def __repr__(self) -> 'str': + return f'{type(self).__name__}({self._local!r}, {self._parent!r})' + + def __getitem__(self, key: 'str') -> 'Any': + try: + return self._local[key] + except KeyError: + return self._parent[key] + + def __setitem__(self, key: 'str', value: 'Any') -> 'None': + self._local[key] = value + + def __delitem__(self, key: 'str') -> 'None': + del self._local[key] + + def __contains__(self, key: 'str') -> 'bool': + return key in self._local or key in self._parent + + def __iter__(self) -> 'Iterator[str]': + yield from self._local + for key in self._parent: + if key not in self._local: + yield key + + def __len__(self) -> 'int': + return len(set(self._local) | set(self._parent)) + + def keys(self) -> 'Iterator[str]': + """Iterate the union of names, same as :meth:`dict.keys`.""" + return iter(self) + + def values(self) -> 'Iterator[Any]': + """Iterate the values for :meth:`keys`, same as :meth:`dict.values`.""" + for key in self: + yield self[key] + + def items(self) -> 'Iterator[tuple[str, Any]]': + """Iterate ``(name, value)`` pairs, same as :meth:`dict.items`.""" + for key in self: + yield key, self[key] + + def get(self, key: 'str', default: 'Any' = None) -> 'Any': + """Get ``key``, falling through to the enclosing schema, or ``default``.""" + try: + return self[key] + except KeyError: + return default + + def update(self, other: 'Any' = (), **kwargs: 'Any') -> 'None': + """Update the nested schema's own data, same as :meth:`dict.update`.""" + if hasattr(other, 'keys'): + for key in other.keys(): + self[key] = other[key] + else: + for key, value in other: + self[key] = value + for key, value in kwargs.items(): + self[key] = value + + def copy(self) -> 'NestedPacketContext': + """Shallow-copy the nested schema's own data; the parent is shared.""" + new = NestedPacketContext(self._parent) + new._local = dict(self._local) + return new + + +def nested_packet_context(packet: 'dict[str, Any]') -> 'NestedPacketContext': """Build the packet context handed to a nested schema's field callbacks. Args: packet: The enclosing schema's own packet data. Returns: - A two-level :class:`collections.ChainMap`: the nested schema's own - packet data (initially holding only the reserved ``__packet__`` key) - chained in front of ``packet``. - - 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 should fall through to - the enclosing schema rather than raise :exc:`KeyError`. That is what - :meth:`~collections.ChainMap.__getitem__`, ``in`` and :meth:`get - ` do here: they check the nested schema's - own data first and the enclosing schema's second. Iterating the - mapping (or calling :meth:`~collections.ChainMap.keys`) sees the - union of both, with the nested schema's own names taking precedence - where the two overlap. - - 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 helper. - - Nothing written through the returned mapping is written back to - ``packet``: :class:`~collections.ChainMap` always resolves - :meth:`~collections.ChainMap.__setitem__` and - :meth:`~collections.ChainMap.__delitem__` against its first map, so a - nested schema can set -- or shadow -- a name also declared by the - enclosing schema without either write ever reaching the enclosing - schema's own data, and without a write silently disappearing either. + A :class:`NestedPacketContext` wrapping ``packet``. See that class + for the exact lookup, write and iteration semantics. """ - return collections.ChainMap({'__packet__': packet}, packet) + return NestedPacketContext(packet) class SchemaField(FieldBase[_TS]): From 6ac029a3636851af158a45003211fa442dbf60c1 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 17 Sep 2026 22:40:40 -0400 Subject: [PATCH 3/9] corekit: make NestedPacketContext a dict subclass; fix mypy and a citation - NestedPacketContext was a hand-written class implementing the mapping protocol from scratch, deriving from nothing -- correct, but it left Schema.pack/unpack's "packet: dict[str, Any]" annotation unsatisfied, which is what every other field class's pack/unpack still declares. Widening those annotations to admit the new type cascades through every field class that forwards packet along (ListField, ConditionalField, FieldBase.__call__, pre_process/post_process, ...), well outside this fix's file list, for seven new mypy errors net. - Fix: NestedPacketContext now subclasses dict directly. dict's own metaclass is plain "type", not ABCMeta, so subclassing or instantiating it never touches the shared _abc_impl cache that #439 is about -- confirmed again on the Python 3.10 venv (this test passes both alone and in the full pcapng module: 37 passed, 139 subtests). Being a real dict also nominally satisfies every existing "dict[str, Any]" annotation, so no other file needs to change. - __missing__ gives the enclosing-schema fallback for free, since dict.__getitem__ calls it automatically when a key is absent locally. - __contains__ and get are overridden because the dict built-ins for both bypass __missing__ entirely. - __iter__/__len__/keys/values/items are overridden for the union-of-both- levels semantics the design promises; dict's own versions would only see this instance's local keys. - copy is overridden because dict.copy() always returns a plain dict, even for a subclass, which would silently drop the fallback. - Plain assignment/deletion need no override: dict's own __setitem__/ __delitem__ already only touch this instance's own storage. - Removed the now-genuinely-unused "type: ignore[has-type]" this rewrite exposed on SchemaField.length -- confirmed present and already-unused on a clean origin/main (da2422728) checkout too, so this is a pre-existing, unrelated mypy hygiene gap this rewrite happened to touch, not something introduced by it. Net mypy count: 123 (down from main's own 124), with no new errors from this branch's own code. - Fixed a stale citation: the EOFError-on-zero-length Gap entries pointed at decorators.py:222; the actual "raise EOFError" is at :228 (prepare itself starts at :177, and #450 shifted lines since the citation was written). Verified: mypy pcapkit -> 123 errors/40 files (main: 124; the diff is the one pre-existing unused-ignore above, not a new error). pytest tests -q unaffected on Python 3.14. Local Python 3.10 venv: test_pcapng_remaining_constructor_branches_and_custom_dispatch passes alone and as part of the full misc/test_pcapng_unit.py module. --- pcapkit/corekit/fields/misc.py | 111 +++++++++--------- tests/protocols/test_option_roundtrip_unit.py | 2 +- 2 files changed, 54 insertions(+), 59 deletions(-) diff --git a/pcapkit/corekit/fields/misc.py b/pcapkit/corekit/fields/misc.py index 653e3148b3..e1aad69dc3 100644 --- a/pcapkit/corekit/fields/misc.py +++ b/pcapkit/corekit/fields/misc.py @@ -490,7 +490,7 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> '_TC' return self._field.unpack(buffer, packet) -class NestedPacketContext: +class NestedPacketContext(dict): """Packet context handed to a nested schema's field callbacks. Args: @@ -499,11 +499,17 @@ class NestedPacketContext: 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 should fall through to the enclosing - schema rather than raise :exc:`KeyError`. That is what - :meth:`__getitem__`, ``in`` and :meth:`get` do here: they check the - nested schema's own data first and the enclosing schema's second. - Iterating this mapping sees the union of both, with the nested schema's - own names taking precedence where the two overlap. + schema rather than raise :exc:`KeyError`. That is what :meth:`__missing__` + gives for free once this is a real :class:`dict` subclass: ``pkt[key]`` + checks this instance's own storage first (the nested schema's own + fields, plus the reserved ``__packet__`` entry) and only calls + :meth:`__missing__` -- falling through to the enclosing schema -- when + the key is not there. :meth:`__contains__` and :meth:`get` are + overridden to honour the same fallback, since the :class:`dict` + built-ins for both bypass :meth:`__missing__` entirely. Iterating this + mapping (or calling :meth:`keys`/:meth:`values`/:meth:`items`) sees the + union of both levels, with the nested schema's own names taking + precedence where the two overlap. The enclosing schema is also reachable *unconditionally* under the reserved ``__packet__`` key, for a callback that needs to name the outer @@ -516,73 +522,67 @@ class NestedPacketContext: through) this class. Nothing written through an instance is written back to ``packet``: - :meth:`__setitem__` and :meth:`__delitem__` always act on the nested - schema's own data, so a nested schema can set -- or shadow -- a name - also declared by the enclosing schema without either write ever - reaching the enclosing schema's own data, and without a write silently - disappearing either. - - Deliberately **not** a :class:`collections.abc.Mapping`. On CPython <= - 3.10, every :class:`~pcapkit.protocols.schema.schema.Schema` subclass - shares ``Schema``'s ``_abc_impl`` cache, so ``isinstance``/``issubclass`` + plain :class:`dict` assignment and deletion (``pkt[key] = value``, ``del + pkt[key]``) always act on this instance's own storage -- that is what a + :class:`dict` subclass gives for free, with no override needed -- so a + nested schema can set or shadow a name also declared by the enclosing + schema without either write ever reaching the enclosing schema's own + data, and without a write silently disappearing either. + + Deliberately a plain :class:`dict` subclass rather than a + :class:`collections.abc.Mapping`. On CPython <= 3.10, every + :class:`~pcapkit.protocols.schema.schema.Schema` subclass shares + ``Schema``'s ``_abc_impl`` cache, so ``isinstance``/``issubclass`` against any of them can return whichever answer was asked first (#439). An earlier version of this class returned a bare :class:`collections.ChainMap`, itself a - :class:`collections.abc.MutableMapping`, and using one here was enough to - disturb that shared cache and flip an unrelated, later + :class:`collections.abc.MutableMapping`, and using one here was enough + to disturb that shared cache and flip an unrelated, later ``isinstance(some_dict, Schema)`` check elsewhere in the same process from :data:`False` to :data:`True` -- ``test_pcapng_remaining_constructor_branches_and_custom_dispatch`` broke on Python 3.10 alone, confirmed by reverting *only* the ``ChainMap`` call - (nothing else) and watching it pass again. Implementing the same - two-level fallback over a plain object rather than an ABC avoids - touching that cache at all. + (nothing else) and watching it pass again. :class:`dict` itself is not + an :class:`~abc.ABCMeta`-based class -- constructing or using a + subclass of it never consults that cache -- so implementing the same + two-level fallback as a :class:`dict` subclass avoids touching it at + all, while also satisfying every existing ``packet: 'dict[str, Any]'`` + annotation on the rest of the field classes without having to widen any + of them. """ - __slots__ = ('_local', '_parent') + __slots__ = ('_parent',) def __init__(self, packet: 'dict[str, Any]') -> 'None': - self._local = {'__packet__': packet} # type: dict[str, Any] + super().__init__({'__packet__': packet}) self._parent = packet # type: dict[str, Any] - def __repr__(self) -> 'str': - return f'{type(self).__name__}({self._local!r}, {self._parent!r})' + def __missing__(self, key: 'str') -> 'Any': + return self._parent[key] - def __getitem__(self, key: 'str') -> 'Any': - try: - return self._local[key] - except KeyError: - return self._parent[key] - - def __setitem__(self, key: 'str', value: 'Any') -> 'None': - self._local[key] = value - - def __delitem__(self, key: 'str') -> 'None': - del self._local[key] - - def __contains__(self, key: 'str') -> 'bool': - return key in self._local or key in self._parent + def __contains__(self, key: 'object') -> 'bool': + return dict.__contains__(self, key) or key in self._parent def __iter__(self) -> 'Iterator[str]': - yield from self._local + yield from dict.__iter__(self) for key in self._parent: - if key not in self._local: + if not dict.__contains__(self, key): yield key def __len__(self) -> 'int': - return len(set(self._local) | set(self._parent)) + return len(set(dict.__iter__(self)) | set(self._parent)) - def keys(self) -> 'Iterator[str]': + def keys(self) -> 'Iterator[str]': # type: ignore[override] """Iterate the union of names, same as :meth:`dict.keys`.""" return iter(self) - def values(self) -> 'Iterator[Any]': + def values(self) -> 'Iterator[Any]': # type: ignore[override] """Iterate the values for :meth:`keys`, same as :meth:`dict.values`.""" for key in self: yield self[key] - def items(self) -> 'Iterator[tuple[str, Any]]': + def items(self) -> 'Iterator[tuple[str, Any]]': # type: ignore[override] """Iterate ``(name, value)`` pairs, same as :meth:`dict.items`.""" for key in self: yield key, self[key] @@ -594,21 +594,16 @@ def get(self, key: 'str', default: 'Any' = None) -> 'Any': except KeyError: return default - def update(self, other: 'Any' = (), **kwargs: 'Any') -> 'None': - """Update the nested schema's own data, same as :meth:`dict.update`.""" - if hasattr(other, 'keys'): - for key in other.keys(): - self[key] = other[key] - else: - for key, value in other: - self[key] = value - for key, value in kwargs.items(): - self[key] = value - def copy(self) -> 'NestedPacketContext': - """Shallow-copy the nested schema's own data; the parent is shared.""" + """Shallow-copy the nested schema's own data; the parent is shared. + + Overridden because :meth:`dict.copy` always returns a plain + :class:`dict`, even for a subclass instance, which would silently + drop the fallback to the enclosing schema. + + """ new = NestedPacketContext(self._parent) - new._local = dict(self._local) + dict.update(new, dict.items(self)) return new @@ -643,7 +638,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': diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index ca8ba5439f..bd0cc24a64 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -417,7 +417,7 @@ class Gap(NamedTuple): **{ f'httpv2-frame/{name}': Gap( 'PARSE', 'EOFError', - 'pcapkit/utilities/decorators.py:222 (prepare) -- raises ' + 'pcapkit/utilities/decorators.py:228 (prepare) -- raises ' 'EOFError for any zero-length nested unpack, but a frame with no ' 'payload legitimately unpacks its body from 0 octets; ' "unreachable before #445 fixed the KeyError: 'flags' this hit " From 023e2d7721f3aa0270dbea30c4570739b6eb676e Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 17 Sep 2026 23:11:43 -0400 Subject: [PATCH 4/9] tests: mh-extension and CGA Parameters now round-trip after #437/#456 - main merged #437 (MH registry completion, including the four mh-extension codes and the _make_ext_multiprefix arithmetic fix) and #456/#446 (the ForwardMatchField double-count in Schema.__len__) since this branch's last merge. Combined with this PR's own fix, all four mh-extension/{Multi_Prefix,Exp_FFFD,Exp_FFFE,Exp_FFFF} cases now round-trip cleanly -- verified directly against the round-trip harness (all four return 'OK'), not assumed from the PR descriptions. Deleted their EXPECTED_FAILURES entries; a stale PARSE/KeyError expectation would otherwise have failed this module outright, per its own two-way assertion. - The issue's own 40-octet CGA Parameters reproduction now parses completely end to end (confirmed directly: MH(raw, len(raw), extension=True) returns a populated CGAParametersOption, no exception). Rewrote test_cga_parameters_option_reaches_the_446_boundary _not_a_keyerror, which asserted the (now stale) FieldValueError boundary, as test_cga_parameters_option_now_parses_end_to_end, asserting the parsed fields directly. - #437 had pinned the pre-fix KeyError as test_mh_cga_parameters_option_is_unparsable_upstream, explicitly so that "whoever fixes it finds out here" -- and it did: this run turned that test red once the merge above landed. Replaced it with test_mh_cga_parameters_option_now_parses, asserting the option parses and its fields are what the wire says, and fixed the now-stale cross-reference and claim in test_mh_pmipv6_options_round_trip_byte_for_byte's docstring (CGA_Parameters is still excluded from that test's cases, but no longer because it cannot be parsed -- that is now a separate, deliberate scope decision for whoever adds its full round-trip identity). - Merged origin/main (0283a6d59) with one conflict, in this exact region of tests/protocols/test_option_roundtrip_unit.py, resolved by re-deriving the correct entries from the actual post-merge behaviour rather than picking either side. Verified: mypy pcapkit -> 123 errors/40 files (a fresh main, 0283a6d59, is 124 -- unchanged from before this merge). Round-trip harness: 7 passed, 363 subtests passed, 0 failed (up from 299 subtests before #437 grew the mh-extension family to four codes). tests/protocols/ internet/test_mh_unit.py: 35 passed, 266 subtests passed, 0 failed. Full local suite result to follow in the PR description. --- .../test_fields_misc_packet_context.py | 23 ++++---- tests/protocols/internet/test_mh_unit.py | 54 +++++++++++-------- tests/protocols/test_option_roundtrip_unit.py | 28 ++++------ 3 files changed, 54 insertions(+), 51 deletions(-) diff --git a/tests/corekit/test_fields_misc_packet_context.py b/tests/corekit/test_fields_misc_packet_context.py index baf3ffa3e3..e63d7ec676 100644 --- a/tests/corekit/test_fields_misc_packet_context.py +++ b/tests/corekit/test_fields_misc_packet_context.py @@ -195,9 +195,9 @@ class CGAParametersRegressionTests(unittest.TestCase): def setUp(self) -> None: purge_modules(['pcapkit']) - def test_cga_parameters_option_reaches_the_446_boundary_not_a_keyerror(self) -> None: + 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 - from pcapkit.utilities.exceptions import FieldValueError # The exact 40-octet reproduction from issue #445. raw = bytes.fromhex( @@ -209,10 +209,15 @@ def test_cga_parameters_option_reaches_the_446_boundary_not_a_keyerror(self) -> ) self.assertEqual(len(raw), 40) - # #446 (ForwardMatchField counted into Schema.__len__) is a separate, - # already-filed defect and is not fixed here: CGA Parameters still - # does not parse. What #445 fixes is *which* error that is -- no - # longer a KeyError out of CGAParameter.extensions. - with self.assertRaises(FieldValueError) as ctx: - MH(raw, len(raw), extension=True) - self.assertIn('parameters has invalid length', str(ctx.exception)) + # #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 4a52c56799..f3b2fd2d91 100644 --- a/tests/protocols/internet/test_mh_unit.py +++ b/tests/protocols/internet/test_mh_unit.py @@ -1883,10 +1883,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 @@ -2383,23 +2385,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 @@ -2411,9 +2418,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 98ca93ce0f..c694bb8775 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -319,25 +319,15 @@ class Gap(NamedTuple): 'assumes bytes; it runs on the pack path too, from schema.py:647'), # -- Mobility Header ------------------------------------------------------ - - # PLACEHOLDER -- re-verifying against the actual merged tree before - # settling this block; see the investigation in progress. - '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 ------------------------------------------------------------------ From 34b9663c9c21512048d3e969e452505333a4f072 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 00:25:28 -0400 Subject: [PATCH 5/9] tests: delete the stale httpv2-frame/SETTINGS EXPECTED_FAILURES entry PR #462 merged (bind HTTP.make to a real instance, and wrap SETTINGS' item schema, closing #459) since this branch's last merge, and pulled in via the fast-forward to 0e7abbe69. SettingsFrame.settings now wraps its item_type in SchemaField(schema=SettingPair) instead of passing the raw class, so the AttributeError this entry recorded no longer happens. Verified directly: tests/protocols/test_option_roundtrip_unit.py's httpv2-frame/SETTINGS case now returns 'OK'. This is what failed CI on Python 3.12 and Integration Python 3.15 at 0e7abbe69 -- not an interpreter-dependent or order-dependent failure, the same stale-entry mismatch on every interpreter; those two jobs simply reported first. Verified: mypy pcapkit -> 123 errors/40 files (unchanged). Round-trip harness, this file's own tests, and tests/protocols/internet/ test_mh_unit.py: 46 passed, 629 subtests passed, 0 failed. --- tests/protocols/test_option_roundtrip_unit.py | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index c694bb8775..669f6ad9b6 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -414,18 +414,11 @@ class Gap(NamedTuple): for name in ('DATA', 'HEADERS', 'CONTINUATION') }, - # ``SettingsFrame.settings`` declares ``item_type=SettingPair``, the raw - # schema class, rather than ``SchemaField(schema=SettingPair)``. So - # ``ListField.unpack``'s non-schema branch calls ``SettingPair(packet)``, - # binding the packet dict to ``SettingPair``'s first field (``id``) - # instead of constructing a field to read with, and then asks the result - # for a ``.length`` no ``Schema`` provides. - 'httpv2-frame/SETTINGS': Gap( - 'PARSE', "'SettingPair' object has no attribute 'length'", - 'pcapkit/protocols/schema/application/httpv2.py:285 -- settings ' - 'declares item_type=SettingPair instead of ' - 'SchemaField(schema=SettingPair); unreachable before #445 fixed the ' - "KeyError: 'flags' this hit first"), + # ``httpv2-frame/SETTINGS`` used to hit ``SettingsFrame.settings`` + # declaring ``item_type=SettingPair`` (the raw schema class) instead of + # ``SchemaField(schema=SettingPair)``, filed as #459. #462 wrapped it + # correctly and merged, so this now round-trips cleanly -- no entry + # needed. # ``make`` writes ``length = payload + 9`` and a PRIORITY payload is five # octets, so the constructed header always says 14 -- while the reader From aab958b1dd743259e71003a056455507d338c848 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 00:55:43 -0400 Subject: [PATCH 6/9] tests: delete all six now-stale httpv2-frame EXPECTED_FAILURES entries main gained #461 (closing #458: prepare's @prepare decorator now distinguishes a declared zero length from a derived one, raising StreamEOFError only for the latter) since this branch's previous merge, on top of #462 (closing #459, already handled). Between the two, all three remaining httpv2-frame entries this PR's own fix had exposed -- DATA, HEADERS, CONTINUATION -- now round-trip cleanly too. Verified directly against the round-trip harness: all three return 'OK'. Rewrote the HTTP/2 section's comment block to summarise all six frames' history (PUSH_PROMISE/PING via #445 itself, DATA/HEADERS/ CONTINUATION via #461, SETTINGS via #462) now that none of them need an entry. Also merged origin/main (fa128959e, #461) -- clean auto-merge, no conflicts, confirmed against #464's changes to tests/protocols/internet/test_mh_unit.py (different hunks, and its own test run clean: 62 passed, 272 subtests, 0 failed). Verified: mypy pcapkit -> 124 errors/40 files (main at fa128959e: 125, one more than its previous 124 -- unrelated to this branch, and this branch stays one fewer than whatever main's own count is, from the same pre-existing type: ignore cleanup as before). Round-trip harness: 7 passed, 363 subtests passed, 0 failed. mh-extension/* re-verified 'OK' again on this merge (all four). --- tests/protocols/test_option_roundtrip_unit.py | 41 ++++++------------- 1 file changed, 13 insertions(+), 28 deletions(-) diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index 669f6ad9b6..d14ef09b33 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -389,36 +389,21 @@ class Gap(NamedTuple): # 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 fixes that for all six. RST_STREAM, + # parent instead of raising, which fixed that for all six. RST_STREAM, # GOAWAY and WINDOW_UPDATE already passed, declaring no flag members at - # all; PUSH_PROMISE and PING now round-trip cleanly too, so their entries - # are gone. The other three get past ``flags`` and hit their own, - # unrelated defects. + # 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: # - # DATA, HEADERS and CONTINUATION carry no payload in the case this suite - # constructs, so the wire correctly encodes a 9-octet, header-only frame - # -- and reading it back asks ``SchemaField.unpack`` to unpack the frame - # body schema from 0 remaining octets. ``Schema.unpack``'s ``@prepare`` - # decorator treats *any* zero-length unpack as end-of-file and raises - # unconditionally, which is right for the outermost read and wrong here: - # an all-default, zero-octet frame body is a valid schema instance, not - # an empty stream. - **{ - f'httpv2-frame/{name}': Gap( - 'PARSE', 'EOFError', - 'pcapkit/utilities/decorators.py:228 (prepare) -- raises ' - 'EOFError for any zero-length nested unpack, but a frame with no ' - 'payload legitimately unpacks its body from 0 octets; ' - "unreachable before #445 fixed the KeyError: 'flags' this hit " - 'first') - for name in ('DATA', 'HEADERS', 'CONTINUATION') - }, - - # ``httpv2-frame/SETTINGS`` used to hit ``SettingsFrame.settings`` - # declaring ``item_type=SettingPair`` (the raw schema class) instead of - # ``SchemaField(schema=SettingPair)``, filed as #459. #462 wrapped it - # correctly and merged, so this now round-trips cleanly -- no entry - # needed. + # - 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 From 82dbf9416212e3160e386271e983fae1dd7fcb99 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 09:12:10 -0400 Subject: [PATCH 7/9] corekit: back to collections.ChainMap, no dedicated class The owner asked for the dict subclass gone in favour of ChainMap, which is where this design started before #439 was (wrongly) suspected of being disturbed by it. Two premises settled first: - The fallback container is still required: #471 (fixing #439 directly) does not touch pcapkit/corekit/fields/misc.py at all, and #445's own KeyError symptom persists on #471's tree without this fix. Different defects. - ChainMap never poisoned anything. The shared ABCMeta cache keys on the exact type queried: asking about a ChainMap instance caches ChainMap, not dict, and isinstance({}, Schema) stayed False afterwards. The actual poisoner was ordinary code asking isinstance about a plain dict (Info.__update__), unrelated to what this function returns either way. #471 has also now given every Schema subclass its own _abc_impl, removing the mechanism regardless. nested_packet_context() now returns a bare collections.ChainMap({'__packet__': packet}, packet) -- no custom class. This gets, with no code: setdefault honouring the fallback (the dict subclass's own setdefault bypassed __missing__ and would insert a name locally instead of returning the parent's value -- caught directly, see the test below), Mapping's __eq__ instead of dict's, and dict(**pkt)/.copy() working correctly out of the box. Only one thing needed a local fix rather than a class: ChainMap is not nominally a dict, so `value.pack(nested_packet_context(packet))` no longer satisfies Schema.pack's `dict[str, Any]` annotation. Widening that annotation cascades into every other field class's pack/unpack, which forward the same packet argument with their own dict[str, Any] annotations (measured: +7 new mypy errors). Used an explicit cast('dict[str, Any]', ...) at the one call site instead -- asserting that ChainMap satisfies every operation the annotation promises (subscript, in, .get(), iteration), which it does, rather than widening the annotation or reintroducing a fresh type: ignore. tests/corekit/test_fields_misc_packet_context.py's docstring already named collections.ChainMap (stale by coincidence during the dict detour, accurate again now) -- expanded it to say so deliberately, and added explicit setdefault/.copy() assertions to test_nested_schema_reads_enclosing_field_by_name_and_does_not_leak_writes, confirmed to fail under the retired dict subclass (setdefault returned 999, shadowing the parent's 3, instead of honouring it). Re-verified all ten EXPECTED_FAILURES entries deleted across the dict detour -- six httpv2-frame, four mh-extension -- individually via options.roundtrip(), not inferred from a green suite: all ten still return 'OK' against the ChainMap version. Merged origin/main (5182ad0ce, #471) first -- clean, no conflicts. Verified: mypy pcapkit -> 124 errors/40 files (a fresh main at 5182ad0ce: 125, one more from an unrelated pre-existing gap this branch already cleans up). Round-trip harness: 6 passed, 358 subtests, 0 failed. tests/protocols/misc/test_pcapng_unit.py + tests/protocols/internet/test_mh_unit.py + tests/protocols/schema/: 97 passed, 416 subtests, 0 failed. --- pcapkit/corekit/fields/misc.py | 198 +++++++----------- .../test_fields_misc_packet_context.py | 52 ++++- 2 files changed, 123 insertions(+), 127 deletions(-) diff --git a/pcapkit/corekit/fields/misc.py b/pcapkit/corekit/fields/misc.py index e1aad69dc3..48f72f63aa 100644 --- a/pcapkit/corekit/fields/misc.py +++ b/pcapkit/corekit/fields/misc.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- """miscellaneous field class""" +import collections import copy import io from typing import TYPE_CHECKING, TypeVar, cast @@ -11,11 +12,11 @@ __all__ = [ 'ConditionalField', 'PayloadField', 'SwitchField', 'ForwardMatchField', - 'NoValueField', 'NestedPacketContext', + 'NoValueField', ] if TYPE_CHECKING: - from typing import IO, Any, Callable, Iterator, Optional, Type + from typing import IO, Any, Callable, Optional, Type from typing_extensions import Self @@ -490,135 +491,75 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> '_TC' return self._field.unpack(buffer, packet) -class NestedPacketContext(dict): - """Packet context handed to a nested schema's field callbacks. - - Args: - packet: The enclosing schema's own packet data. - - 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 should fall through to the enclosing - schema rather than raise :exc:`KeyError`. That is what :meth:`__missing__` - gives for free once this is a real :class:`dict` subclass: ``pkt[key]`` - checks this instance's own storage first (the nested schema's own - fields, plus the reserved ``__packet__`` entry) and only calls - :meth:`__missing__` -- falling through to the enclosing schema -- when - the key is not there. :meth:`__contains__` and :meth:`get` are - overridden to honour the same fallback, since the :class:`dict` - built-ins for both bypass :meth:`__missing__` entirely. Iterating this - mapping (or calling :meth:`keys`/:meth:`values`/:meth:`items`) sees the - union of both levels, with the nested schema's own names taking - precedence where the two overlap. - - 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 class. - - Nothing written through an instance is written back to ``packet``: - plain :class:`dict` assignment and deletion (``pkt[key] = value``, ``del - pkt[key]``) always act on this instance's own storage -- that is what a - :class:`dict` subclass gives for free, with no override needed -- so a - nested schema can set or shadow a name also declared by the enclosing - schema without either write ever reaching the enclosing schema's own - data, and without a write silently disappearing either. - - Deliberately a plain :class:`dict` subclass rather than a - :class:`collections.abc.Mapping`. On CPython <= 3.10, every - :class:`~pcapkit.protocols.schema.schema.Schema` subclass shares - ``Schema``'s ``_abc_impl`` cache, so ``isinstance``/``issubclass`` - against any of them can return whichever answer was asked first (#439). - An earlier version of this class returned a bare - :class:`collections.ChainMap`, itself a - :class:`collections.abc.MutableMapping`, and using one here was enough - to disturb that shared cache and flip an unrelated, later - ``isinstance(some_dict, Schema)`` check elsewhere in the same process - from :data:`False` to :data:`True` -- - ``test_pcapng_remaining_constructor_branches_and_custom_dispatch`` broke - on Python 3.10 alone, confirmed by reverting *only* the ``ChainMap`` call - (nothing else) and watching it pass again. :class:`dict` itself is not - an :class:`~abc.ABCMeta`-based class -- constructing or using a - subclass of it never consults that cache -- so implementing the same - two-level fallback as a :class:`dict` subclass avoids touching it at - all, while also satisfying every existing ``packet: 'dict[str, Any]'`` - annotation on the rest of the field classes without having to widen any - of them. - - """ - - __slots__ = ('_parent',) - - def __init__(self, packet: 'dict[str, Any]') -> 'None': - super().__init__({'__packet__': packet}) - self._parent = packet # type: dict[str, Any] - - def __missing__(self, key: 'str') -> 'Any': - return self._parent[key] - - def __contains__(self, key: 'object') -> 'bool': - return dict.__contains__(self, key) or key in self._parent - - def __iter__(self) -> 'Iterator[str]': - yield from dict.__iter__(self) - for key in self._parent: - if not dict.__contains__(self, key): - yield key - - def __len__(self) -> 'int': - return len(set(dict.__iter__(self)) | set(self._parent)) - - def keys(self) -> 'Iterator[str]': # type: ignore[override] - """Iterate the union of names, same as :meth:`dict.keys`.""" - return iter(self) - - def values(self) -> 'Iterator[Any]': # type: ignore[override] - """Iterate the values for :meth:`keys`, same as :meth:`dict.values`.""" - for key in self: - yield self[key] - - def items(self) -> 'Iterator[tuple[str, Any]]': # type: ignore[override] - """Iterate ``(name, value)`` pairs, same as :meth:`dict.items`.""" - for key in self: - yield key, self[key] - - def get(self, key: 'str', default: 'Any' = None) -> 'Any': - """Get ``key``, falling through to the enclosing schema, or ``default``.""" - try: - return self[key] - except KeyError: - return default - - def copy(self) -> 'NestedPacketContext': - """Shallow-copy the nested schema's own data; the parent is shared. - - Overridden because :meth:`dict.copy` always returns a plain - :class:`dict`, even for a subclass instance, which would silently - drop the fallback to the enclosing schema. - - """ - new = NestedPacketContext(self._parent) - dict.update(new, dict.items(self)) - return new - - -def nested_packet_context(packet: 'dict[str, Any]') -> 'NestedPacketContext': +def nested_packet_context(packet: 'dict[str, Any]') -> 'collections.ChainMap[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 :class:`NestedPacketContext` wrapping ``packet``. See that class - for the exact lookup, write and iteration semantics. + A two-level :class:`collections.ChainMap`: an initially-empty map for + the nested schema's own data, chained in front of ``packet``, with + the reserved ``__packet__`` key seeded into the first map so it is + always reachable regardless of what either map otherwise holds. + + 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 should fall through to + the enclosing schema rather than raise :exc:`KeyError`. A + :class:`~collections.ChainMap` does this natively: :meth:`__getitem__`, + ``in`` and :meth:`~collections.ChainMap.get` all check the first map + (the nested schema's own data) before the second (``packet``), and so + does :meth:`~collections.ChainMap.setdefault` -- which a hand-written + :meth:`dict.setdefault` override would need to duplicate, since + :class:`dict`'s own bypasses :meth:`~object.__missing__` the same way + :meth:`~dict.get` and :meth:`~dict.__contains__` do. Iterating the + mapping (:meth:`~collections.ChainMap.keys`/:meth:`~collections.ChainMap.values`/ + :meth:`~collections.ChainMap.items`, or plain ``dict(**pkt)``) sees the + union of both maps, with the nested schema's own names taking + precedence where the two overlap. + + 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 is written back to + ``packet``: :class:`~collections.ChainMap` always resolves + :meth:`~collections.ChainMap.__setitem__` and + :meth:`~collections.ChainMap.__delitem__` against its first map, so a + nested schema can set -- or shadow -- a name also declared by the + enclosing schema without either write ever reaching the enclosing + schema's own data, and without a write silently disappearing either. + :meth:`~collections.ChainMap.copy` follows the same rule: it copies + only the first map, so the copy still sees the same parent and still + keeps its own writes local. + + No dedicated class: an earlier version of this function returned a + hand-written :class:`dict` subclass, adopted when a bare + :class:`collections.ChainMap` was (wrongly) suspected of corrupting + the shared :class:`~abc.ABCMeta` cache every :class:`Schema + ` subclass used to share on + CPython <= 3.10 (issue #439). Measured after the fact: the cache keys + on the exact type queried, so asking about a :class:`~collections.ChainMap` + instance caches lookups for :class:`~collections.ChainMap`, not for + :class:`dict` -- the actual poisoning came from ordinary code asking + :func:`isinstance` about a plain :class:`dict` + (:func:`~pcapkit.corekit.infoclass.Info.__update__`), which a + :class:`~collections.ChainMap`-based context never did either way. + #439 has since been fixed directly (every :class:`Schema` subclass + now gets its own ``_abc_impl``), which removes the mechanism + regardless of what this function returns. Nothing here needs to + reimplement the mapping protocol by hand. """ - return NestedPacketContext(packet) + return collections.ChainMap({'__packet__': packet}, packet) class SchemaField(FieldBase[_TS]): @@ -722,7 +663,16 @@ def pack(self, value: 'Optional[_TS | bytes]', packet: 'dict[str, Any]') -> 'byt return value packet.update(self._packet) - return value.pack(nested_packet_context(packet)) + # NOTE: ``nested_packet_context`` returns a ``collections.ChainMap``, + # not a ``dict``, so it does not nominally satisfy ``Schema.pack``'s + # ``dict[str, Any]`` annotation -- but it satisfies every operation + # that annotation promises (subscript, ``in``, ``.get()``, iteration), + # which is what the cast below asserts, explicitly, rather than + # widening ``Schema.pack``'s own signature. Widening it instead + # cascades into every other field class's ``pack``/``unpack``, which + # forward the same ``packet`` argument onward with their own + # ``dict[str, Any]`` annotations -- see :func:`nested_packet_context`. + return value.pack(cast('dict[str, Any]', nested_packet_context(packet))) def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> '_TS': """Unpack field value from :obj:`bytes`. diff --git a/tests/corekit/test_fields_misc_packet_context.py b/tests/corekit/test_fields_misc_packet_context.py index e63d7ec676..6ea932fea4 100644 --- a/tests/corekit/test_fields_misc_packet_context.py +++ b/tests/corekit/test_fields_misc_packet_context.py @@ -31,6 +31,22 @@ class NestedPacketContextTests(unittest.TestCase): 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 (wrongly) suspected of corrupting a shared + :class:`~abc.ABCMeta` cache on CPython <= 3.10 (issue #439). 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: @@ -72,6 +88,22 @@ def post_process(self, 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__'] @@ -110,9 +142,23 @@ def test_nested_schema_reads_enclosing_field_by_name_and_does_not_leak_writes(se self.assertEqual(captured['get_length'], 3) self.assertEqual(captured['get_missing'], 'sentinel') - # Iterating the mapping sees the union of both levels. - self.assertEqual(captured['keys'], - {'__packet__', '__length__', 'tag', 'length', 'body'}) + # 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 From b647e65e05c19559cf4c2d2f59657545ca2de539 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 10:29:04 -0400 Subject: [PATCH 8/9] corekit: stop asserting the ChainMap/#439 history as settled fact (#445) Both changes are docstring prose plus one test class name. No executable line changes; nothing about the fix itself moves. The review of 82dbf9416 was right that nested_packet_context's docstring overclaimed. It stated the ChainMap-poisoning reversal as measured fact, which does not reconcile with commit 86370d7d5's own directly-measured result: on a real CPython 3.10 venv at the time, toggling only the ChainMap call moved test_pcapng_remaining_constructor_branches_and_custom_dispatch between passing and failing, both ways. What is genuinely measured is narrower: the ABCMeta cache keys on the exact type queried, so probing a ChainMap caches ChainMap and not dict, and #439's poisoning came from ordinary code asking isinstance about a plain dict (Info.__update__). That does not by itself explain the 3.10 observation. The likeliest reconciliation -- the ChainMap was never causal but changed which concrete types flowed through unrelated isinstance calls, and so changed when the pre-existing corruption fired -- is plausible rather than demonstrated, and is now untestable: #439's direct fix removed the mechanism, so the original conditions are gone. The docstring now records that as two measurements that do not fully reconcile, and says plainly that it does not matter for correctness either way. This matters beyond tidiness: repeating an unverified conclusion as fact is a mistake this branch has already made twice, and a docstring is where it would have outlived the PR. Also, since the test file's own class docstring carried the same "(wrongly) suspected" claim, it is softened to match rather than left contradicting the module it documents. NestedPacketContextTests -> NestedPacketContextSemanticsTests: the old name referred to a class deleted earlier in this same branch, so it was a dangling reference; the new one names what the tests actually cover, the ChainMap-based fallback semantics. No other reference to the old name exists in pcapkit, tests or docs. Verified: tests/corekit/test_fields_misc_packet_context.py plus tests/protocols/internet/test_mh_unit.py -> 40 passed, 272 subtests, 0 failed. The diff touches no executable line in misc.py. --- pcapkit/corekit/fields/misc.py | 42 +++++++++++++------ .../test_fields_misc_packet_context.py | 15 ++++--- 2 files changed, 39 insertions(+), 18 deletions(-) diff --git a/pcapkit/corekit/fields/misc.py b/pcapkit/corekit/fields/misc.py index 48f72f63aa..ffdceb881c 100644 --- a/pcapkit/corekit/fields/misc.py +++ b/pcapkit/corekit/fields/misc.py @@ -543,20 +543,36 @@ def nested_packet_context(packet: 'dict[str, Any]') -> 'collections.ChainMap[str No dedicated class: an earlier version of this function returned a hand-written :class:`dict` subclass, adopted when a bare - :class:`collections.ChainMap` was (wrongly) suspected of corrupting - the shared :class:`~abc.ABCMeta` cache every :class:`Schema + :class:`collections.ChainMap` was suspected of corrupting the shared + :class:`~abc.ABCMeta` cache every :class:`Schema ` subclass used to share on - CPython <= 3.10 (issue #439). Measured after the fact: the cache keys - on the exact type queried, so asking about a :class:`~collections.ChainMap` - instance caches lookups for :class:`~collections.ChainMap`, not for - :class:`dict` -- the actual poisoning came from ordinary code asking - :func:`isinstance` about a plain :class:`dict` - (:func:`~pcapkit.corekit.infoclass.Info.__update__`), which a - :class:`~collections.ChainMap`-based context never did either way. - #439 has since been fixed directly (every :class:`Schema` subclass - now gets its own ``_abc_impl``), which removes the mechanism - regardless of what this function returns. Nothing here needs to - reimplement the mapping protocol by hand. + CPython <= 3.10 (issue #439). + + That suspicion is *probably* wrong, and the honest position is that it + is no longer decidable -- so it is recorded here as two measurements + that do not fully reconcile rather than as a settled reversal. What is + directly measured: the cache keys on the **exact type queried**, so + asking about a :class:`~collections.ChainMap` instance caches lookups + for :class:`~collections.ChainMap`, not for :class:`dict`, and 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 -- + which a :class:`~collections.ChainMap`-based context never did. + Against that: swapping this function's ``ChainMap`` for a plain + ``{'__packet__': packet}`` 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 is that the ``ChainMap`` was never causal on its own but + changed *which* concrete types flowed through unrelated + :func:`isinstance` calls in the same run, and so changed *when* the + pre-existing #439 corruption was triggered. That reconciliation is + plausible rather than demonstrated, and it cannot now be tested: #439 + has since been fixed directly (every :class:`Schema` subclass gets its + own ``_abc_impl``), which removes the mechanism outright, so the + original conditions no longer exist. It does not matter for + correctness either way -- with the mechanism gone, nothing here needs + to reimplement the mapping protocol by hand. """ return collections.ChainMap({'__packet__': packet}, packet) diff --git a/tests/corekit/test_fields_misc_packet_context.py b/tests/corekit/test_fields_misc_packet_context.py index 6ea932fea4..e2f3c8280d 100644 --- a/tests/corekit/test_fields_misc_packet_context.py +++ b/tests/corekit/test_fields_misc_packet_context.py @@ -11,7 +11,7 @@ HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) -class NestedPacketContextTests(unittest.TestCase): +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 @@ -33,10 +33,15 @@ class NestedPacketContextTests(unittest.TestCase): An intermediate version of this fix used a hand-written :class:`dict` subclass instead of :class:`collections.ChainMap`, adopted when a bare - ``ChainMap`` was (wrongly) suspected of corrupting a shared - :class:`~abc.ABCMeta` cache on CPython <= 3.10 (issue #439). Both the - suspicion and the workaround it produced have since been retired: #439 - was fixed directly, and the hand-written subclass's own + ``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 -- From c8a8d414157297e3033552b9baf434dd17e66c56 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 11:58:07 -0400 Subject: [PATCH 9/9] corekit: return a plain dict from nested_packet_context, not a ChainMap (#445) The owner picked the flat dict after being shown the comparison. This is the third container this function has returned -- ChainMap, then a hand-written dict subclass, then ChainMap again -- and a plain dict is what ends the question rather than continuing it. return {**packet, '__packet__': packet} What it buys, measured rather than argued: - The cast at SchemaField.pack's call site is gone. A real dict satisfies Schema.pack's own dict[str, Any] annotation natively, so there is nothing left to assert. That cast was the one wart of the ChainMap version and the thing its review probed hardest. - mypy is unchanged: 124 errors either way, and the only diff in the full error lists is the same four pre-existing misc.py errors shifted one line, because dropping the now-dead `import collections` removed a line. No error added, none removed. - Every mapping operation is dict's own, so setdefault, pop, ==, iteration and dict(**pkt) need no override and behave the way a reader of the code expects. The ChainMap version got setdefault and __eq__ right too, but by delegation rather than by being the thing itself. - It cannot interact with ABCMeta at all, dict not being an ABCMeta-based class, which retires the #439 question for this function permanently instead of leaving it resting on an argument about cache keying. Two consequences of a copy rather than a live view, both deliberate and neither reached by any current call site, now documented: deleting a name from the context removes it outright rather than reverting to the enclosing schema's value, and a mutation of the enclosing packet made after the context is built is not observed through it -- __packet__ stays bound to the live enclosing mapping for any callback that needs current values. The docstring is rewritten accordingly, and now records the CGAExtension case concretely -- it declares its own `length` while the option enclosing it declares `length` too -- so the reason the context must be write-local is a real collision in this codebase rather than a hypothetical. Verified on the merged tree (origin/main f7b5cc5cd merged in first, clean): tests/corekit/test_fields_misc_packet_context.py + tests/protocols/schema/ + tests/protocols/misc/test_pcapng_unit.py + tests/protocols/internet/test_mh_unit.py -> 101 passed, 416 subtests, 0 failed, identical to the ChainMap version. All ten EXPECTED_FAILURES labels deleted by this branch still return OK, enumerated individually through options.cases() rather than inferred from a green suite. --- pcapkit/corekit/fields/misc.py | 138 ++++++++++++++++----------------- 1 file changed, 65 insertions(+), 73 deletions(-) diff --git a/pcapkit/corekit/fields/misc.py b/pcapkit/corekit/fields/misc.py index ffdceb881c..7f1397ecf7 100644 --- a/pcapkit/corekit/fields/misc.py +++ b/pcapkit/corekit/fields/misc.py @@ -1,7 +1,6 @@ # -*- coding: utf-8 -*- """miscellaneous field class""" -import collections import copy import io from typing import TYPE_CHECKING, TypeVar, cast @@ -491,34 +490,27 @@ 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]') -> 'collections.ChainMap[str, Any]': +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 two-level :class:`collections.ChainMap`: an initially-empty map for - the nested schema's own data, chained in front of ``packet``, with - the reserved ``__packet__`` key seeded into the first map so it is - always reachable regardless of what either map otherwise holds. + 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 should fall through to - the enclosing schema rather than raise :exc:`KeyError`. A - :class:`~collections.ChainMap` does this natively: :meth:`__getitem__`, - ``in`` and :meth:`~collections.ChainMap.get` all check the first map - (the nested schema's own data) before the second (``packet``), and so - does :meth:`~collections.ChainMap.setdefault` -- which a hand-written - :meth:`dict.setdefault` override would need to duplicate, since - :class:`dict`'s own bypasses :meth:`~object.__missing__` the same way - :meth:`~dict.get` and :meth:`~dict.__contains__` do. Iterating the - mapping (:meth:`~collections.ChainMap.keys`/:meth:`~collections.ChainMap.values`/ - :meth:`~collections.ChainMap.items`, or plain ``dict(**pkt)``) sees the - union of both maps, with the nested schema's own names taking - precedence where the two overlap. + 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 @@ -530,52 +522,61 @@ def nested_packet_context(packet: 'dict[str, Any]') -> 'collections.ChainMap[str 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 is written back to - ``packet``: :class:`~collections.ChainMap` always resolves - :meth:`~collections.ChainMap.__setitem__` and - :meth:`~collections.ChainMap.__delitem__` against its first map, so a - nested schema can set -- or shadow -- a name also declared by the - enclosing schema without either write ever reaching the enclosing - schema's own data, and without a write silently disappearing either. - :meth:`~collections.ChainMap.copy` follows the same rule: it copies - only the first map, so the copy still sees the same parent and still - keeps its own writes local. - - No dedicated class: an earlier version of this function returned a - hand-written :class:`dict` subclass, adopted when a bare - :class:`collections.ChainMap` was suspected of corrupting the shared - :class:`~abc.ABCMeta` cache every :class:`Schema - ` subclass used to share on - CPython <= 3.10 (issue #439). - - That suspicion is *probably* wrong, and the honest position is that it - is no longer decidable -- so it is recorded here as two measurements - that do not fully reconcile rather than as a settled reversal. What is - directly measured: the cache keys on the **exact type queried**, so - asking about a :class:`~collections.ChainMap` instance caches lookups - for :class:`~collections.ChainMap`, not for :class:`dict`, and 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 -- - which a :class:`~collections.ChainMap`-based context never did. - Against that: swapping this function's ``ChainMap`` for a plain - ``{'__packet__': packet}`` literal was, at the time and on a real - CPython 3.10 venv, enough to move + 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 is that the ``ChainMap`` was never causal on its own but - changed *which* concrete types flowed through unrelated - :func:`isinstance` calls in the same run, and so changed *when* the - pre-existing #439 corruption was triggered. That reconciliation is - plausible rather than demonstrated, and it cannot now be tested: #439 - has since been fixed directly (every :class:`Schema` subclass gets its - own ``_abc_impl``), which removes the mechanism outright, so the - original conditions no longer exist. It does not matter for - correctness either way -- with the mechanism gone, nothing here needs - to reimplement the mapping protocol by hand. + 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 collections.ChainMap({'__packet__': packet}, packet) + return {**packet, '__packet__': packet} class SchemaField(FieldBase[_TS]): @@ -679,16 +680,7 @@ def pack(self, value: 'Optional[_TS | bytes]', packet: 'dict[str, Any]') -> 'byt return value packet.update(self._packet) - # NOTE: ``nested_packet_context`` returns a ``collections.ChainMap``, - # not a ``dict``, so it does not nominally satisfy ``Schema.pack``'s - # ``dict[str, Any]`` annotation -- but it satisfies every operation - # that annotation promises (subscript, ``in``, ``.get()``, iteration), - # which is what the cast below asserts, explicitly, rather than - # widening ``Schema.pack``'s own signature. Widening it instead - # cascades into every other field class's ``pack``/``unpack``, which - # forward the same ``packet`` argument onward with their own - # ``dict[str, Any]`` annotations -- see :func:`nested_packet_context`. - return value.pack(cast('dict[str, Any]', nested_packet_context(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`.