fix(schema): stop EnumSchema.registry from retaining a lookup miss (#555)

CHANGELOG.md

@@ -22,6 +22,7 @@ The largest release since 1.0, and the first recorded here as it happened rather
 - **Changed** -- subclass registration is **opt-in** for `Engine`, `Reassembly`, `TraceFlow` and `dumpkit`'s `Dumper` (#514). Each registers if and only if its registry keyword is given -- `engine=` for `Engine`, `protocol=` for `Reassembly` and `TraceFlow`, `fmt=` for `Dumper`. Previously an absent keyword fell back to the class' own name, so *every* subclass of the public class was registered, and declining meant subclassing the parallel `*Base` class under an alias -- which is what every built-in does, and why the public classes had **0** subclasses between them against the `*Base` classes' 9, 5, 2 and 3. **This breaks out-of-tree code that subclasses one of the four and relies on the derived key**; pass the keyword, or call the matching `register_*` function. Nothing the library ships is affected, and the `*Base` classes remain importable. Two things that were silent are now loud: an unrecognised class keyword raises `UnsupportedCall` instead of being swallowed by `**kwargs` -- which used to register the class under its own name, so passing `name=` to a `Reassembly` subclass silently ignored the key it was given, `protocol=` being the real one -- and `Dumper`'s `ext=` without `fmt=` likewise. A class attribute is not an opt-in: `__engine_name__` and `__protocol_name__` still set the name a class reports, registered or not. Each metaclass also gained a class-level `registry` property mirroring `EnumSchema.registry`. As a side effect a `Dumper` subclass no longer touches the filesystem while its `class` statement runs: inferring `fmt` from the `kind` property meant instantiating the class against a `NamedTemporaryFile` mid-definition. `Engine`'s keyword is `engine=` rather than the `name=` this first shipped with, because `name` cannot be passed as a class keyword at all on Python 3.10: `mcls`, `name`, `bases` and `namespace` collide with `abc.ABCMeta.__new__`'s own parameters, which are positional-or-keyword before 3.11 and positional-only from 3.11, so a class statement naming any of the four raises `TypeError` from the metaclass before the hook is reached. Those four are the whole of the `ABCMeta.__new__` collision surface, measured on 3.10.21, 3.11.15 and 3.14.7; `engine=`, `protocol=` and `fmt=` are all outside it, so the documented registration path works on every supported version. There is no `name=` alias -- a keyword that worked on some interpreters and not others is the trap being removed, not a compatibility measure.
 - **Changed** -- extraction is around 46% faster on a 1,117-frame HTTP capture, with byte-identical output (#420). A reassembled datagram's payload is now analysed on first read rather than eagerly, which cuts IP reassembly's own cost by 90.7% and TCP's by 23.7% -- IP reassembly submits a datagram for every frame, fragmented or not (#424). Flow tracing over the same capture went from 1416.6 ms to 744.0 ms, because the flow dumper had been handing each record to a `Frame` constructor that re-dissected the whole protocol stack to return bytes it had just been given; options are no longer parsed twice either (#427). All output compared byte-for-byte across the sample captures in each case.
 - **Fixed** -- next-layer, option, chunk, block and parameter dispatch all read `defaultdict` registries, so a lookup miss inserted the key into class-level state shared by every later instance, after which a legitimate `register_*` call warned that the code was already registered. Every read now goes through a lookup that does not grow the table, and `IPv4.__option__` and `HIP.__parameter__` became inspectable class attributes rather than names assembled at call time (#426, #428, #429, #434). One break comes with it: a tuple-registered handler pair written to the documented `OptionParser`/`OptionConstructor` signature now works where it could previously never be called at all, and a pair written with an explicit leading `self` -- the only shape that used to work -- now does not.
+- **Fixed** -- the identical defect one layer up, in the schema layer's own `EnumSchema.registry`: `Option.registry[code]` for an unregistered `code` inserted the default schema under that code, so a single lookup made an unassigned TCP option number, e.g. `156`, read back as registered for the rest of the process. `EnumSchema.__enum__` is now built (or, when a subclass seeds it manually in its own class body -- `PCAPNG.Option`'s namespaced mapping, `TCP.MPTCP`'s plain one) as a retention-safe mapping that still returns the registered default on a miss, it just stops recording it; `.registry` keeps returning the same object it always did, so nothing that held a reference to it is affected (#555).
 - **Fixed** -- on Python 3.10 and older, no `Schema` subclass got its own `_abc_impl`: all of them fell through to `collections.abc.Mapping`'s, so a single `isinstance` or `issubclass` answer poisoned every later question about that class for the rest of the process. A terminating PCAP-NG `EndRecord` tested `True` as an `IPv4Record` (#439).
 - **Fixed** -- construction, which was broken in several places at once: the generated typed `__init__` was never installed, so `__post_init__` did not run and a schema built from a subset of its fields could not be packed at all -- `UDP(srcport=53, dstport=5353)` now packs (#430); IPv6 and Mobility Header option padding was wrong, leaving construction wholly broken (#398); `HTTP.make` called the versioned `make` unbound, so every real call raised `TypeError` (#452, #462); and `IPv4._make_data` returned the fragment offset in octets where the wire wants 8-octet units, and read `data.options` on a packet that has none (#494, #499).
 - **Fixed** -- a truncated or under-declared area no longer parses "successfully", and no longer wedges the process. The option and list loops could spin forever with no exception on a truncated area, reachable from untrusted input through HOPOPT, IPv6-Opts, MH, HIP and SCTP; each iteration must now advance the stream by at least one octet, and the error names the option, the offset and the octets remaining (#431, #432). Separately, wire-derived lengths in `ipv6_opts`, CALIPSO, MPL, REG_INFO and four HIP list callbacks underflowed below zero, which `ListField`'s own `while length > 0` then turned into a silent empty list; they are floored and raise instead (#449, #456, #460, #463).

docs/source/changelog/1.5.0.rst

@@ -192,6 +192,16 @@ pull requests between #326 and #509.
   ``OptionParser``/``OptionConstructor`` signature now works where it could
   previously never be called at all, and a pair written with an explicit leading
   ``self`` -- the only shape that used to work -- now does not.
+* **Fixed** -- the identical defect one layer up, in the schema layer's own
+  ``EnumSchema.registry``: ``Option.registry[code]`` for an unregistered
+  ``code`` inserted the default schema under that code, so a single lookup
+  made an unassigned TCP option number, e.g. ``156``, read back as registered
+  for the rest of the process. ``EnumSchema.__enum__`` is now built (or, when a
+  subclass seeds it manually in its own class body -- ``PCAPNG.Option``'s
+  namespaced mapping, ``TCP.MPTCP``'s plain one) as a retention-safe mapping
+  that still returns the registered default on a miss, it just stops recording
+  it; ``.registry`` keeps returning the same object it always did, so nothing
+  that held a reference to it is affected (#555).
 * **Fixed** -- on Python 3.10 and older, no ``Schema`` subclass got its own
   ``_abc_impl``: all of them fell through to ``collections.abc.Mapping``'s, so a
   single ``isinstance`` or ``issubclass`` answer poisoned every later question

docs/source/pcapkit/protocols/index.rst

@@ -223,6 +223,12 @@ Internal Definitions
    :no-members:
    :show-inheritance:
 
+.. autoclass:: pcapkit.protocols.schema.schema._EnumRegistry
+   :no-members:
+   :show-inheritance:
+
+   .. automethod:: __missing__
+
 Type Variables
 ~~~~~~~~~~~~~~
 

pcapkit/protocols/schema/schema.py

@@ -928,6 +928,37 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Schema':
         return self
 
 
+class _EnumRegistry(collections.defaultdict):
+    """A registry :class:`collections.defaultdict` that never inserts a miss.
+
+    :attr:`EnumSchema.registry` (and its class-level twin,
+    :attr:`EnumMeta.registry`) is read with a bare ``registry[code]`` at dozens
+    of call sites across the schema layer, e.g. ``Option.registry[type]``. A
+    plain :class:`collections.defaultdict` inserts whatever
+    :attr:`~collections.defaultdict.default_factory` returns the *first time*
+    an unregistered ``code`` is looked up -- and since the registry lives on
+    the *class*, that insertion is permanent and shared by every instance of
+    every subclass in the process. Parsing one packet carrying an unrecognised
+    code is therefore enough to grow the registry for the remainder of the
+    process, and to make a later, entirely legitimate
+    :meth:`EnumSchema.register` call report an overwrite that never happened.
+
+    This is the schema-layer instance of the defect :meth:`ProtocolBase.\
+    _lookup_registry <pcapkit.protocols.protocol.ProtocolBase._lookup_registry>`
+    fixed for the protocol-layer ``__proto__`` family in GitHub issues #421 and
+    #425/#428; see GitHub issue #555. The fallback itself is deliberate -- it
+    is how an unknown option, chunk or block falls back to its
+    ``Unknown*``/``Unassigned*`` schema -- so this subclass keeps returning it,
+    it just stops recording it.
+
+    """
+
+    def __missing__(self, key: 'Any') -> 'Any':
+        if self.default_factory is None:
+            raise KeyError(key)
+        return self.default_factory()
+
+
 class EnumMeta(SchemaMeta, Generic[_ET]):
     """Meta class to add dynamic support for :class:`EnumSchema`.
 
@@ -947,7 +978,16 @@ class EnumMeta(SchemaMeta, Generic[_ET]):
 
     @property
     def registry(cls) -> 'DefaultDict[_ET, Type[EnumSchema]]':
-        """Mapping of enumeration numbers to schemas."""
+        """Mapping of enumeration numbers to schemas.
+
+        Important:
+            The returned mapping is a :class:`_EnumRegistry`, not a plain
+            :class:`collections.defaultdict`: indexing it with an
+            unregistered ``code`` still returns :attr:`EnumSchema.__default__`'s
+            schema, but does **not** insert that code. See :class:`_EnumRegistry`
+            for why that distinction matters.
+
+        """
         return cls.__enum__
 
 
@@ -1012,6 +1052,12 @@ def registry(self) -> 'DefaultDict[_ET, Type[Self]]':
             This property is also available as a class
             attribute.
 
+        Important:
+            See :attr:`EnumMeta.registry`: the returned mapping is a
+            :class:`_EnumRegistry`, so looking up an unregistered ``code``
+            returns the default schema without recording ``code`` as if it
+            had been registered.
+
         """
         return self.__enum__
 
@@ -1030,16 +1076,32 @@ def __init_subclass__(cls, /, code: 'Optional[_ET | Iterable[_ET]]' = None, *arg
 
         Notes:
             If :attr:`__enum__` is not yet defined at function call,
-            it will automatically be defined as a :class:`collections.defaultdict`
+            it will automatically be defined as a :class:`_EnumRegistry`
             object, with the default value set to :attr:`__default__`.
 
             If intended to customise the :attr:`__enum__` mapping,
             it is possible to override the :meth:`__init_subclass__` method and
-            define :attr:`__enum__` manually.
+            define :attr:`__enum__` manually. Such a manual definition may use
+            a plain :class:`collections.defaultdict` -- e.g. to seed a
+            namespaced or nested mapping such as
+            :class:`pcapkit.protocols.schema.misc.pcapng.Option`'s -- so it is
+            swapped for the retention-safe :class:`_EnumRegistry` below, before
+            anything else can hold a reference to the original object.
 
         """
         if not hasattr(cls, '__enum__'):
-            cls.__enum__ = collections.defaultdict(cls.__default__)
+            cls.__enum__ = _EnumRegistry(cls.__default__)
+        elif '__enum__' in cls.__dict__ and not isinstance(cls.__dict__['__enum__'], _EnumRegistry):
+            # ``cls`` set its own ``__enum__`` in the class body, as a plain
+            # ``collections.defaultdict`` -- swap it for the retention-safe
+            # variant now, while ``cls`` is still being constructed and no
+            # external code has had a chance to capture a reference to the
+            # original dict. Every later access, through :attr:`registry` or
+            # otherwise, then sees the same safe object -- so identity across
+            # repeated ``.registry`` reads (see ``EnumMeta.registry``) is
+            # preserved, and nothing but the retention behaviour changes.
+            manual = cls.__dict__['__enum__']
+            cls.__enum__ = _EnumRegistry(getattr(manual, 'default_factory', None), manual)
 
         if code is not None:
             if isinstance(code, collections.abc.Iterable):

tests/protocols/schema/test_enum_schema_registry_unit.py

@@ -0,0 +1,158 @@
+"""Regression tests for GitHub issue #555.
+
+:class:`~pcapkit.protocols.schema.schema.EnumSchema`'s :attr:`registry` is a
+:class:`collections.defaultdict`-backed mapping of enumeration codes to schema
+classes. Reading it with a bare ``registry[code]`` for a code nobody registered
+used to *insert* that code -- with whatever the default factory produced -- the
+same defect fixed at the protocol layer's ``__proto__`` family by GitHub issues
+#421 and #425/#428. These tests cover both the auto-created
+:attr:`EnumSchema.__enum__` (the shape used by
+e.g. :class:`pcapkit.protocols.schema.transport.tcp.Option`) and a manually
+seeded one declared directly in a subclass's own class body (the shape used by
+e.g. :class:`pcapkit.protocols.schema.misc.pcapng.Option`'s outer, namespaced
+mapping).
+"""
+from __future__ import annotations
+
+import collections
+import enum
+import importlib.util
+import unittest
+
+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)
+
+
+@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed')
+class EnumSchemaRegistryRetentionTests(unittest.TestCase):
+    def setUp(self) -> None:
+        purge_modules(['pcapkit'])
+
+    def test_lookup_miss_on_auto_created_registry_does_not_grow_it(self) -> None:
+        """A miss on an auto-created ``__enum__`` must not be retained.
+
+        :class:`EnumSchema` subclasses that never assign :attr:`__enum__`
+        themselves get one built by :meth:`EnumSchema.__init_subclass__` --
+        the shape used by e.g.
+        :class:`pcapkit.protocols.schema.transport.tcp.Option`. A bare
+        ``registry[code]`` for an unregistered ``code`` must keep returning
+        the default schema without inserting ``code``.
+        """
+        from pcapkit.protocols.schema.schema import EnumSchema
+
+        class Code(enum.IntEnum):
+            registered = 1
+            unassigned = 2
+
+        class DefaultSchema:
+            """Stand-in for the fallback schema."""
+
+        class BaseSchema(EnumSchema[Code]):
+            __default__ = lambda: DefaultSchema  # noqa: E731
+
+        class RegisteredSchema(BaseSchema, code=Code.registered):
+            pass
+
+        self.assertNotIn(Code.unassigned, BaseSchema.registry)
+        before = len(BaseSchema.registry)
+
+        # the miss: a bare subscript, exactly as every schema-layer call site
+        # (e.g. ``Option.registry[type]``) performs it
+        result = BaseSchema.registry[Code.unassigned]
+
+        # the fallback is preserved -- this is not a narrowing to "raise on miss"
+        self.assertIs(result, DefaultSchema)
+
+        # the miss must not be retained
+        self.assertNotIn(Code.unassigned, BaseSchema.registry)
+        self.assertEqual(len(BaseSchema.registry), before)
+
+        # a second, independent miss confirms it is not a one-shot fluke
+        self.assertIs(BaseSchema.registry[Code.unassigned], DefaultSchema)
+        self.assertNotIn(Code.unassigned, BaseSchema.registry)
+
+        # the registered code is unaffected by any of the above
+        self.assertIs(BaseSchema.registry[Code.registered], RegisteredSchema)
+
+    def test_lookup_miss_on_manually_declared_registry_does_not_grow_it(self) -> None:
+        """A miss on a manually-declared ``__enum__`` must not be retained.
+
+        A subclass may assign :attr:`__enum__` itself, as a plain
+        :class:`collections.defaultdict`, in its own class body -- the shape
+        used by e.g. :class:`pcapkit.protocols.schema.misc.pcapng.Option`'s
+        outer, namespaced mapping and
+        :class:`pcapkit.protocols.schema.transport.tcp.MPTCP`. This must be
+        just as retention-safe as the auto-created case, even though the
+        object was never touched by :meth:`EnumSchema.__init_subclass__`'s
+        creation branch.
+        """
+        from pcapkit.protocols.schema.schema import EnumSchema
+
+        class Code(enum.IntEnum):
+            registered = 1
+            unassigned = 2
+
+        class DefaultSchema:
+            """Stand-in for the fallback schema."""
+
+        class ManualSchema(EnumSchema[Code]):
+            # declared directly, bypassing the auto-creation branch in
+            # ``EnumSchema.__init_subclass__`` -- pre-seeded with the
+            # registered entry, just like a real protocol's schema module
+            # would seed namespace defaults in its class body
+            __enum__ = collections.defaultdict(lambda: DefaultSchema)
+
+        class RegisteredSchema(ManualSchema, code=Code.registered):
+            pass
+
+        self.assertNotIn(Code.unassigned, ManualSchema.registry)
+        before = len(ManualSchema.registry)
+
+        result = ManualSchema.registry[Code.unassigned]
+
+        self.assertIs(result, DefaultSchema)
+        self.assertNotIn(Code.unassigned, ManualSchema.registry)
+        self.assertEqual(len(ManualSchema.registry), before)
+        self.assertIs(ManualSchema.registry[Code.registered], RegisteredSchema)
+
+    def test_registry_identity_is_stable_across_accesses(self) -> None:
+        """``.registry`` keeps returning the *same* object across accesses.
+
+        The retention fix must not swap in a fresh wrapper on every read --
+        :class:`_EnumRegistry` is the one object stored on :attr:`__enum__`,
+        exactly as a plain :class:`collections.defaultdict` would have been,
+        so ``is``-identity across the class-level accessor, the instance-level
+        accessor and a fresh instance all hold.
+        """
+        from pcapkit.protocols.schema.schema import EnumSchema
+
+        class Code(enum.IntEnum):
+            one = 1
+
+        class IdentitySchema(EnumSchema[Code]):
+            pass
+
+        self.assertIs(IdentitySchema.registry, IdentitySchema.registry)
+        self.assertIs(IdentitySchema().registry, IdentitySchema.registry)
+
+    def test_real_tcp_option_schema_registry_does_not_leak_on_miss(self) -> None:
+        """The exact reproduction from GitHub issue #555.
+
+        ``OptionNumber(156)`` is unassigned in
+        :class:`pcapkit.const.tcp.option.Option`, so nothing registers it
+        against :class:`pcapkit.protocols.schema.transport.tcp.Option`. A
+        single bare-subscript read must not make it appear registered
+        afterwards.
+        """
+        from pcapkit.const.tcp.option import Option as OptionNumber
+        from pcapkit.protocols.schema.transport.tcp import Option, UnassignedOption
+
+        probe = OptionNumber(156)
+        self.assertNotIn(probe, Option.registry)
+
+        schema = Option.registry[probe]
+
+        self.assertIs(schema, UnassignedOption)
+        self.assertNotIn(probe, Option.registry)