diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f3e779ec..ccc99bac9 100644 --- a/CHANGELOG.md +++ b/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). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 3c96119a4..4e230831a 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/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 diff --git a/docs/source/pcapkit/protocols/index.rst b/docs/source/pcapkit/protocols/index.rst index 1b212f057..a1534db98 100644 --- a/docs/source/pcapkit/protocols/index.rst +++ b/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 ~~~~~~~~~~~~~~ diff --git a/pcapkit/protocols/schema/schema.py b/pcapkit/protocols/schema/schema.py index 9a4dcb54a..7b3b63c56 100644 --- a/pcapkit/protocols/schema/schema.py +++ b/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 ` + 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): diff --git a/tests/protocols/schema/test_enum_schema_registry_unit.py b/tests/protocols/schema/test_enum_schema_registry_unit.py new file mode 100644 index 000000000..66b1d88f5 --- /dev/null +++ b/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)