From f42e93d312cee38b17eb28ce0fecd10717be7c74 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 25 Sep 2026 01:36:14 -0400 Subject: [PATCH] fix(fields): fall through to an unregistered member instead of minting (#575) - TCP/UDP/SCTP's PortEnumField and PCAP-NG's OptionEnumField called their registry's .get() directly on every parsed value, which mints a fresh member via aenum.extend_enum for any value no row or documented span covers. On http.pcap that is 111 calls and ~14% of extraction self time, entirely from unassigned ephemeral ports. - All four now peek at what .get() would consult -- the registry's own rows, then AppType's declared IANA spans via _missing_, or OptionType's per-namespace membership -- and only call .get() once one already holds. A genuine miss gets EnumField._unregistered_member: an instance built by calling the registry's own storage base's __new__ (str or int, picked per namespace) directly, skipping the registry's own __new__ and the registration inside it, so isinstance holds and nothing is ever added to any lookup table. Per the owner's ruling, tracked more broadly as #775 and applied here only to these four call sites. - The three port fields check the value against the field's own declared byte width (self.length) before consulting the registry at all, so a port outside that width still raises #764's deliberate rejection instead of being absorbed alongside a genuine in-range miss. A pcapkit.utilities.exceptions rejection still propagates unchanged. AppType.get() and every _missing_ are unmodified, so direct get() callers still mint. - The extra attributes are passed in the order the registry's own __new__ sets them -- svc, port, proto for AppType and opt_name, opt_value for OptionType -- because make_dumper renders a member's addon keys straight out of its __dict__ in insertion order. Any other order dumps an unassigned value's keys the opposite way round from every declared one's, in all four formats, while leaving every value correct: 1117 rendered port blocks of each kind on http.pcap alone. - Enum.__reduce_ex__ reduces a member to (cls, (value,)), the one lookup an unregistered member is absent from, so _unregistered_member installs a __reduce_ex__ of its own that rebuilds an equivalent unregistered member. Without it, removing the mint would have taken away a pickle round-trip that worked before -- and taken it away on read-back, since dumps succeeds either way. copy/deepcopy reduce through it too on 3.10, where Enum.__copy__/__deepcopy__ do not exist. - dictdumper.plist.PLIST (which 'xml' also maps to) writes / content with no entity escaping at all (JarryShaw/DictDumper#125, tracked as #772), so the unregistered member's rendering broke plist/xml reports. make_dumper's object_hook now escapes &, < and > for PLIST-rooted output only, leaving json/tree/text untouched. extend_enum calls on http.pcap: 111 (13.6-14.1%) -> 0. many_interfaces.pcapng: 15 -> 2, both from the untouched _missing_ mechanism. Wall-clock extract(nofile=True), 7 fresh processes per tree at host load 1.29 -> 1.27: http.pcap 0.952 -> 0.679 s, many_interfaces.pcapng 0.130 -> 0.103 s. New unit tests cover no-mint, isinstance, out-of-width, equality-without-identity, value-lookup still raising, in-library-rejection, round-trip, escaping, addon key order, _unregistered_member's int and TypeError branches, and the pickle/copy round-trip; each fails without its fix. coverage run -m pytest and python -m unittest agree. Fixes #575. --- pcapkit/corekit/fields/numbers.py | 217 +++++++ pcapkit/dumpkit/common.py | 70 ++- pcapkit/protocols/schema/misc/pcapng.py | 42 +- pcapkit/protocols/schema/transport/sctp.py | 27 +- pcapkit/protocols/schema/transport/tcp.py | 81 ++- pcapkit/protocols/schema/transport/udp.py | 27 +- ...fields_numbers_port_option_no_mint_unit.py | 541 ++++++++++++++++++ tests/dumpkit/test_common_unit.py | 114 ++++ 8 files changed, 1094 insertions(+), 25 deletions(-) create mode 100644 tests/corekit/test_fields_numbers_port_option_no_mint_unit.py diff --git a/pcapkit/corekit/fields/numbers.py b/pcapkit/corekit/fields/numbers.py index 644f9d89d..7ed5f2884 100644 --- a/pcapkit/corekit/fields/numbers.py +++ b/pcapkit/corekit/fields/numbers.py @@ -2,6 +2,7 @@ """numerical field class""" import enum +import functools import math from typing import TYPE_CHECKING, Generic, TypeVar, Union, cast @@ -584,7 +585,223 @@ def post_process(self, value: 'int | bytes', packet: 'dict[str, Any]') -> 'Stdli # no member carries it, so it is not this layer's to absorb. if isinstance(error, BaseError): raise + return self._pseudo_member(value) + + def _pseudo_member(self, value: 'int') -> 'StdlibEnum | AenumEnum': + """Build the bounded nameless pseudo-member this method falls back to + when ``value`` is a foreign miss rather than an in-library rejection. + + Returns: + A single-member, throwaway :class:`enum.IntEnum` instance, built + fresh per call rather than :func:`~aenum.extend_enum`-ed onto + ``self._namespace``, per this method's own docstring above. + + """ unknown = enum.IntEnum('', { '': value, }, module='pcapkit.const', qualname='pcapkit.const.') return getattr(unknown, '') + + @staticmethod + def _unregistered_member(namespace: 'Type[StdlibEnum] | Type[AenumEnum]', + value: 'Any', name: 'str' = '', + **attrs: 'Any') -> 'Any': + """Build a member of ``namespace``, absent from every one of its own + lookup tables, for a value a *parse* -- rather than a direct call to + the registry's own ``get()`` -- resolved without anyone asking for a + name. + + GitHub issue #575: the owner's ruling is that an unassigned wire value + should resolve to a real member of the registry the field names -- + ``isinstance`` against it and every ancestor holds, and it renders and + dispatches exactly like a declared one -- provided building it never + grows the registry, which is the whole reason the field stopped + calling ``get()`` unconditionally in the first place. This is what + gets there: it calls ``namespace``'s own storage base's ``__new__`` + directly -- :class:`str` or :class:`int`, whichever ``namespace`` + derives from -- which skips ``namespace``'s *own* ``__new__`` + entirely, and with it the ``cls.__registry__.add(...)`` / + ``cls.__members_ns__[...] = ...`` line every registry in this package + uses to record a member it mints. No entry is added to + ``_member_map_`` or ``_value2member_map_`` either, since those are + only ever touched by the metaclass machinery :func:`aenum.extend_enum` + drives, which this bypasses completely. + + Note: + Building a member this way, rather than as some other type + altogether, is why :meth:`~pcapkit.protocols.schema.transport.tcp.PortEnumField.post_process` + and its siblings need this rather than :meth:`_pseudo_member`: + the result answers ``isinstance(result, AppType)`` truthfully, + which matters to at least seven ``isinstance`` sites elsewhere in + :mod:`pcapkit.protocols` (see ``test_a_member_is_still_an_apptype`` + in ``tests/const/test_const_apptype_split_unit.py``), and a value + that fails all of them would be a second defect standing in for + the one this fix removes. + + The member this returns is absent from ``_value2member_map_``, so + a *value*-keyed lookup on it -- ``self._namespace(value)`` -- + still raises exactly as it did before this existed. Nothing on the + parse or reconstruction path does that to a value it just resolved + this way, which is what keeps this safe to return from + ``post_process``. A direct call to + :meth:`~pcapkit.const.reg.apptype.AppType.get` for the same port + is a different matter and deliberately unchanged: asking the + registry for a name is an explicit request for a named member, so + it still mints one -- measured on this tree, + ``AppType.get(54321, proto=tcp)`` returns ``PORT_54321_tcp`` and + takes ``TCP.__members__`` from 6147 to 6148, and a second call + with the same port returns that member rather than raising. Two + unregistered members for the same value also compare equal + without being identical, since + :class:`~pcapkit.const.reg.apptype.AppType` and + :class:`~pcapkit.const.pcapng.option_type.OptionType` both define + ``__eq__``/``__hash__`` off an attribute (``.port`` / + ``.opt_value``) rather than object identity -- harmless for every + reader in this package, since none compares one with ``is`` or + keys a mapping on it expecting identity, but worth knowing before + reusing this elsewhere. + + :mod:`pickle` and :func:`copy.copy`/:func:`copy.deepcopy` all + reduce an :class:`~enum.Enum` member through + ``Enum.__reduce_ex__``, which returns ``(cls, (value,))`` -- the + one lookup this member is deliberately absent from. Left alone + that is a genuine regression rather than a pre-existing + limitation, because the call sites used to *mint*, so the member + was registered and a round-trip worked. Measured on CPython + 3.14.7, resolving port 53406 through + :class:`~pcapkit.protocols.schema.transport.tcp.PortEnumField`: + on ``83b58ebda`` ``pickle.loads(pickle.dumps(member))`` returned + the member, and with the mint removed and nothing in its place it + raised ``ValueError: 'unknown [53406 - tcp]' is not a valid TCP`` + -- while ``pickle.dumps`` still succeeded, so the failure + surfaced only on read-back rather than where it was caused. + + So ``__reduce_ex__`` is set on the member itself, reducing it to + ``_rebuild_unregistered_member`` instead of to a value lookup. + Both :mod:`pickle` and :mod:`copy` fetch that attribute with + :func:`getattr` on the object rather than on its type, so a + per-instance override is honoured: verified against the C + :mod:`pickle` accelerator on every protocol from 0 to 5, against + the pure-Python ``pickle._Pickler``, and against + :func:`copy.copy`/:func:`copy.deepcopy` both as they are on 3.11+ + and with CPython's ``Enum.__copy__``/``__deepcopy__`` deleted to + emulate 3.10, where those two do not exist. Rebuilding re-enters + this method rather than ``namespace.__new__``, so an unpickled + member is unregistered exactly as the original was and the + registry does not grow -- ``TCP.__members__`` measured at 6147 + before and after. On 3.11+ ``copy``/``deepcopy`` still return the + member itself, since ``Enum.__copy__`` short-circuits ahead of + any reduction; on 3.10 they return an equal rebuilt one, which is + the same answer for an immutable value. + + Args: + namespace: The concrete registry class to build the member as an + instance of. ``isinstance`` holds against it and every + ancestor; it never gains an entry in any of its own tables. + value: The value ``namespace``'s own constructor would have + wrapped -- e.g. the crafted string + :meth:`~pcapkit.const.reg.apptype.AppType.__new__` builds from + a name, a port and a transport, or + :meth:`~pcapkit.const.pcapng.option_type.OptionType.__new__`'s + equivalent -- kept the same shape here so a rendered or + re-keyed member reads the same either way. + name: The member's own ``.name``; ``''`` matches every + other nameless value this package produces. + **attrs: Extra attributes to set on the returned member, matching + the shape the caller's registry gives its real members -- + :class:`~pcapkit.const.reg.apptype.AppType`'s ``.port``, + ``.svc`` and ``.proto``, or + :class:`~pcapkit.const.pcapng.option_type.OptionType`'s + ``.opt_name`` and ``.opt_value``. + + Returns: + The unregistered member. + + Raises: + TypeError: If ``namespace`` derives from neither :class:`str` nor + :class:`int` -- every registry this package builds does one or + the other, and guessing wrong for some future one would ship + a member silently missing whatever its storage base provides, + rather than saying plainly that this needs extending first. + + """ + obj: 'Any' + if issubclass(namespace, str): + obj = str.__new__(namespace, value) + elif issubclass(namespace, int): + obj = int.__new__(namespace, value) + else: + raise TypeError( + f'{namespace!r} derives from neither str nor int; ' + '_unregistered_member does not know how to build one of its members') + # NOTE: setting the enum protocol's own name/value attributes + # directly, rather than through namespace's own __new__, is what + # skips the registration that __new__ would otherwise have done. + obj._name_ = name # pylint: disable=protected-access + obj._value_ = value # pylint: disable=protected-access + for attr_name, attr_value in attrs.items(): + setattr(obj, attr_name, attr_value) + # NOTE: ``Enum.__reduce_ex__`` reduces a member to ``(cls, (value,))``, + # i.e. to the one lookup this member is deliberately absent from, so + # pickle and copy would both raise on it without this. Overriding it + # per instance -- which pickle and copy both honour, since both fetch + # it with getattr on the object rather than on its type -- rebuilds an + # equivalent unregistered member instead. functools.partial rather + # than a closure keeps this off obj itself, so the member does not + # become part of a reference cycle merely by being reducible. + obj.__reduce_ex__ = functools.partial( + _reduce_unregistered_member, namespace, value, name, attrs) + return obj + + +def _rebuild_unregistered_member(namespace: 'Type[StdlibEnum] | Type[AenumEnum]', + value: 'Any', name: 'str', + attrs: 'dict[str, Any]') -> 'Any': + """Rebuild the member :meth:`EnumField._unregistered_member` returned. + + This is what :mod:`pickle` and :mod:`copy` reconstruct through, in place of + the value lookup ``Enum.__reduce_ex__`` would otherwise have reduced the + member to. It is a module-level function rather than a method so that every + :mod:`pickle` protocol can name it: protocols below 4 cannot reference a + callable nested inside a class. + + Args: + namespace: The registry class to rebuild the member as an instance of. + value: The member's ``_value_``. + name: The member's ``_name_``. + attrs: The extra attributes the member carried. + + Returns: + A member equal to the original and, like it, absent from every one of + ``namespace``'s lookup tables -- rebuilding goes back through + :meth:`EnumField._unregistered_member` and never through + ``namespace.__new__``, so it cannot register anything either. + + """ + return EnumField._unregistered_member( # pylint: disable=protected-access + namespace, value, name, **attrs) + + +def _reduce_unregistered_member( # pylint: disable=unused-argument + namespace: 'Type[StdlibEnum] | Type[AenumEnum]', + value: 'Any', name: 'str', attrs: 'dict[str, Any]', + protocol: 'int') -> 'tuple[Callable[..., Any], tuple[Any, ...]]': + """The ``__reduce_ex__`` :meth:`EnumField._unregistered_member` installs. + + Bound to its first four arguments with :func:`functools.partial`, so that + the reducer holds the ingredients of the member rather than the member + itself. + + Args: + namespace: The registry class the member is an instance of. + value: The member's ``_value_``. + name: The member's ``_name_``. + attrs: The extra attributes the member carries. + protocol: The :mod:`pickle` protocol version, ignored -- the reduction + is the same for all of them, and :mod:`copy` passes 4 here. + + Returns: + A two-tuple of ``_rebuild_unregistered_member`` and its arguments. + + """ + return (_rebuild_unregistered_member, (namespace, value, name, attrs)) diff --git a/pcapkit/dumpkit/common.py b/pcapkit/dumpkit/common.py index f07b70f41..cd83f1a53 100644 --- a/pcapkit/dumpkit/common.py +++ b/pcapkit/dumpkit/common.py @@ -15,10 +15,12 @@ import decimal import enum import ipaddress +import xml.sax.saxutils from typing import TYPE_CHECKING import aenum import dictdumper.dumper +import dictdumper.plist from pcapkit.corekit.infoclass import Info from pcapkit.corekit.multidict import MultiDict, OrderedMultiDict @@ -229,6 +231,22 @@ def make_dumper(output: 'Type[ABCDumper]') -> 'Type[ABCDumper]': Customised :class:`~dictdumper.dumper.Dumper` object. """ + # NOTE: :class:`~dictdumper.plist.PLIST` -- which is also what + # :attr:`Extractor.__output__ ` + # maps the ``'xml'`` format to, so the two are the same writer under two + # names here -- interpolates every ````/```` value into its + # XML-shaped markup with no entity escaping at all: neither ``&``, ``<`` + # nor ``>`` is escaped anywhere in :mod:`dictdumper`. Filed upstream as + # JarryShaw/DictDumper#125 and tracked here as GitHub issue #772. ``json``, + # ``tree`` and ``text`` have no such defect and all three characters are + # legal in their output, so escaping is conditioned on the output class + # rather than done unconditionally in :func:`render_enum`, which would + # double-escape those three. :class:`~dictdumper.xml.XML` itself defines + # no ``_append_string`` of its own -- its own module docstring says not to + # use it directly -- so :class:`~dictdumper.plist.PLIST` is the only + # concrete writer this applies to today. + escape_strings = issubclass(output, dictdumper.plist.PLIST) + class DictDumper(output): """Customised :class:`~dictdumper.dumper.Dumper` object.""" @@ -240,35 +258,55 @@ def object_hook(self, o: 'Any') -> 'Any': o: object to convert Returns: - Converted object. + Converted object, escaped for XML-shaped output where needed. + + Notes: + :meth:`~dictdumper.dumper.Dumper._encode_value` -- and so this + method -- is called once per node the dumper writes, however + deeply nested, so escaping the :class:`str` result here on the + way out reaches every string the writer will ever interpolate + raw, not merely the ones built directly in this method. The + one exception is a :class:`~pcapkit.corekit.multidict.MultiDict` + key: :meth:`~dictdumper.dumper.Dumper._append_dict` writes a + dict's keys straight from the mapping without ever calling + this method on them, so a key built from :func:`render_enum` + is escaped inline, right where it is built, instead. """ if isinstance(o, decimal.Decimal): - return str(o) - if isinstance(o, datetime.timedelta): - return o.total_seconds() - if isinstance(o, (Info, Schema)): - return o.to_dict() - if isinstance(o, (ipaddress.IPv4Address, ipaddress.IPv6Address)): - return str(o) - if isinstance(o, (MultiDict, OrderedMultiDict)): + result = str(o) # type: Any + elif isinstance(o, datetime.timedelta): + result = o.total_seconds() + elif isinstance(o, (Info, Schema)): + result = o.to_dict() + elif isinstance(o, (ipaddress.IPv4Address, ipaddress.IPv6Address)): + result = str(o) + elif isinstance(o, (MultiDict, OrderedMultiDict)): temp = collections.defaultdict(list) # type: DefaultDict[str, list[Any]] for key, val in o.items(multi=True): if isinstance(key, (enum.Enum, aenum.Enum)): key = render_enum(key) + if escape_strings: + key = xml.sax.saxutils.escape(key) temp[key].append(val) - return temp - if isinstance(o, dict): - return o - if isinstance(o, (enum.Enum, aenum.Enum)): + result = temp + elif isinstance(o, dict): + result = o + elif isinstance(o, (enum.Enum, aenum.Enum)): addon = {key: val for key, val in o.__dict__.items() if not key.startswith('_')} if addon: - return { + result = { 'enum': render_enum(o), **addon, } - return render_enum(o) - return super(type(self), self).object_hook(o) # type: ignore[unreachable] + else: + result = render_enum(o) + else: + result = super(type(self), self).object_hook(o) + + if escape_strings and isinstance(result, str): + return xml.sax.saxutils.escape(result) + return result def default(self, o: 'Any') -> 'Literal["fallback"]': # pylint: disable=unused-argument """Check content type for function call. diff --git a/pcapkit/protocols/schema/misc/pcapng.py b/pcapkit/protocols/schema/misc/pcapng.py index d4b644e6d..ccceb1384 100644 --- a/pcapkit/protocols/schema/misc/pcapng.py +++ b/pcapkit/protocols/schema/misc/pcapng.py @@ -566,11 +566,49 @@ def post_process(self, value: 'int | bytes', packet: 'dict[str, Any]') -> 'Enum_ packet: Packet data. Returns: - Processed field value. + Processed field value -- the registry member declared for the + option code in this namespace (or in the shared ``opt`` + namespace), or an unregistered member of the same registry, + carrying the code itself, when neither declares one. See GitHub + issue #575. + + Notes: + :meth:`~pcapkit.const.pcapng.option_type.OptionType.get` mints a + fresh member -- via :func:`aenum.extend_enum` -- for any code + neither namespace's row covers, and does so unconditionally on a + miss: unlike :class:`~pcapkit.const.reg.apptype.AppType`, its + ``_missing_`` never declines, so it cannot be consulted the way + :meth:`pcapkit.protocols.schema.transport.tcp.PortEnumField.post_process` + consults :class:`~pcapkit.const.reg.apptype.AppType`'s. This + instead replicates the read-only membership test + :meth:`~pcapkit.const.pcapng.option_type.OptionType.get` itself + runs first, and only calls it once that test finds the code + already declared, so an undeclared option type gets + :meth:`EnumField._unregistered_member` instead of a fresh + registry row. """ value = super(EnumField, self).post_process(value, packet) - return self._namespace.get(value, namespace=self._opt_ns) + namespace = self._opt_ns + members_ns = self._namespace.__members_ns__ + if value not in members_ns.get('opt', {}) and value not in members_ns.get(namespace, {}): + # NOTE: an unregistered member of self._namespace itself, per + # GitHub issue #575's owner ruling. ``.opt_name`` and + # ``.opt_value`` are what a real OptionType member carries -- + # read unconditionally by e.g. pcapng's own ``_option_key`` -- + # and the ``_unknown`` name matches what the mint this + # replaces used to call it, so a rendered or re-keyed member + # reads the same either way. They are passed in the order + # OptionType.__new__ sets them, because DictDumper.object_hook + # renders a member's addon keys straight out of its ``__dict__`` + # in insertion order, so any other order here would dump an + # undeclared option code's keys the other way round from every + # declared one's. + opt_name = f'{namespace}_unknown' + return self._unregistered_member( + self._namespace, f'{opt_name} [{value:d}]', + opt_name=opt_name, opt_value=value) + return self._namespace.get(value, namespace=namespace) @schema_final diff --git a/pcapkit/protocols/schema/transport/sctp.py b/pcapkit/protocols/schema/transport/sctp.py index b97b53dff..31736e429 100644 --- a/pcapkit/protocols/schema/transport/sctp.py +++ b/pcapkit/protocols/schema/transport/sctp.py @@ -17,6 +17,7 @@ from pcapkit.corekit.fields.numbers import EnumField, UInt16Field, UInt32Field from pcapkit.corekit.fields.strings import BitField, BytesField, PaddingField from pcapkit.protocols.schema.schema import EnumSchema, Schema, schema_final +from pcapkit.utilities.exceptions import BaseError from pcapkit.utilities.logging import SPHINX_TYPE_CHECKING __all__ = [ @@ -216,11 +217,33 @@ def post_process(self, value: 'int | bytes', packet: 'dict[str, Any]') -> 'Enum_ packet: Packet data. Returns: - Processed field value. + Processed field value -- the registry member declared for the + port, or an unregistered member of the same registry, carrying + the port itself, when the registry declares none. See GitHub + issue #575. + + Notes: + See :meth:`pcapkit.protocols.schema.transport.tcp.PortEnumField.post_process`, + whose notes this mirrors verbatim aside from the transport. """ value = super(EnumField, self).post_process(value, packet) - return self._namespace.get(value, proto=Enum_TransportProtocol.sctp) + proto = Enum_TransportProtocol.sctp + if not (isinstance(value, int) and 0 <= value < (1 << (8 * self.length))): + return self._namespace.get(value, proto=proto) + owner = self._namespace._dispatch(value, proto) # pylint: disable=protected-access + if not owner.__registry__.getlist(value): # type: ignore[union-attr] + try: + declared = owner._missing_(value) # pylint: disable=protected-access + except ValueError as error: + if isinstance(error, BaseError): + raise + declared = None + if declared is None: + return self._unregistered_member( + owner, f'unknown [{value:d} - {proto.name}]', + svc='unknown', port=value, proto=proto) + return self._namespace.get(value, proto=proto) class ErrorCause(EnumSchema[Enum_CauseCode]): diff --git a/pcapkit/protocols/schema/transport/tcp.py b/pcapkit/protocols/schema/transport/tcp.py index 1fdbb04a6..07fbef556 100644 --- a/pcapkit/protocols/schema/transport/tcp.py +++ b/pcapkit/protocols/schema/transport/tcp.py @@ -18,7 +18,7 @@ UInt64Field) from pcapkit.corekit.fields.strings import BitField, BytesField, PaddingField from pcapkit.protocols.schema.schema import EnumSchema, Schema, schema_final -from pcapkit.utilities.exceptions import FieldError +from pcapkit.utilities.exceptions import BaseError, FieldError from pcapkit.utilities.logging import SPHINX_TYPE_CHECKING __all__ = [ @@ -387,11 +387,86 @@ def post_process(self, value: 'int | bytes', packet: 'dict[str, Any]') -> 'Enum_ packet: Packet data. Returns: - Processed field value. + Processed field value -- the registry member declared for the + port, or an unregistered member of the same registry, carrying + the port itself, when the registry declares none. See GitHub + issue #575. + + Notes: + :meth:`~pcapkit.const.reg.apptype.AppType.get` mints a fresh + member -- via :func:`aenum.extend_enum` -- for any port neither an + existing row nor one of :meth:`_missing_`'s documented IANA spans + accounts for, which in practice means the ephemeral/dynamic range. + Calling it unconditionally on every parsed port therefore grew the + registry without bound. This peeks at the registry + :meth:`~pcapkit.const.reg.apptype.AppType.get` itself would + consult -- its per-port rows via ``__registry__.getlist``, then + its documented spans via ``_missing_`` -- and only calls + :meth:`~pcapkit.const.reg.apptype.AppType.get` once one of those is + already known to hold, so a genuine miss gets + :meth:`EnumField._unregistered_member` instead of a mint. + + A port outside this field's own width is rejected *before* any of + that, rather than being let through to :meth:`_missing_` and + caught alongside a genuine miss. Both are a bare :exc:`ValueError` + with nothing to tell them apart by type, and GitHub issue #764 + gave the out-of-range case a deliberate, ``breaking``-tagged + rejection specifically so it would stop being minted over -- a + catch keyed on exception type alone cannot see the difference + between that and :mod:`aenum`'s own "no member has this value", + so it would absorb both and quietly revert #764 for these four + fields. Checking the width first needs no exception-based + distinction at all: it asks the same question :meth:`_missing_` + would eventually ask, and asks it in a way that never manufactures + the bare :exc:`ValueError` this method would otherwise have to + tell apart from a foreign one. ``self.length`` is the field's own + declared byte width (``2`` for every caller of this class, hence + ``0``-``65535``) rather than a hard-coded ``65535`` borrowed from + :meth:`~pcapkit.const.reg.apptype.AppType._missing_`'s own guard, + so the two stay in lockstep by construction: whatever width this + field is ever given, the range checked here is exactly the range a + value of that width can hold, no more permissive and no more + restrictive. """ value = super(EnumField, self).post_process(value, packet) - return self._namespace.get(value, proto=Enum_TransportProtocol.tcp) + proto = Enum_TransportProtocol.tcp + if not (isinstance(value, int) and 0 <= value < (1 << (8 * self.length))): + # NOTE: lets AppType.get() -- unmodified -- raise #764's rejection + # for a port this field's own width cannot represent, rather than + # risking it being absorbed below as a foreign miss. + return self._namespace.get(value, proto=proto) + owner = self._namespace._dispatch(value, proto) # pylint: disable=protected-access + if not owner.__registry__.getlist(value): # type: ignore[union-attr] + try: + declared = owner._missing_(value) # pylint: disable=protected-access + except ValueError as error: + # NOTE: value is already known to be in-width here, so this + # ValueError is aenum's own "no member has this value" for an + # in-range but unassigned port -- a foreign miss, absorbed -- + # never #764's out-of-range rejection, which never reaches + # this branch. A pcapkit.utilities.exceptions error is still a + # deliberate registry decision and propagates unchanged. + if isinstance(error, BaseError): + raise + declared = None + if declared is None: + # NOTE: an unregistered member of ``owner`` itself, per GitHub + # issue #575's owner ruling -- see EnumField._unregistered_member + # -- rather than a foreign pseudo-enum. ``.port``, ``.svc`` and + # ``.proto`` are what a real AppType member carries -- read + # unconditionally by e.g. Transport._decode_next_layer's + # ``srcport.port`` -- and ``svc='unknown'`` matches what the + # mint this replaces used to name it. They are passed in the + # order AppType.__new__ sets them, because + # DictDumper.object_hook renders a member's addon keys straight + # out of its ``__dict__`` in insertion order -- so any other + # order here would make an unassigned port dump ``port`` before + # ``svc`` while every declared one dumps ``svc`` first. + return self._unregistered_member( + owner, f'unknown [{value:d} - {proto.name}]', + svc='unknown', port=value, proto=proto) + return self._namespace.get(value, proto=proto) class Option(EnumSchema[Enum_Option]): diff --git a/pcapkit/protocols/schema/transport/udp.py b/pcapkit/protocols/schema/transport/udp.py index 2f5f4c593..21d84cb55 100644 --- a/pcapkit/protocols/schema/transport/udp.py +++ b/pcapkit/protocols/schema/transport/udp.py @@ -10,6 +10,7 @@ from pcapkit.corekit.fields.numbers import EnumField, UInt16Field from pcapkit.corekit.fields.strings import BytesField from pcapkit.protocols.schema.schema import Schema, schema_final +from pcapkit.utilities.exceptions import BaseError __all__ = ['UDP'] @@ -63,11 +64,33 @@ def post_process(self, value: 'int | bytes', packet: 'dict[str, Any]') -> 'Enum_ packet: Packet data. Returns: - Processed field value. + Processed field value -- the registry member declared for the + port, or an unregistered member of the same registry, carrying + the port itself, when the registry declares none. See GitHub + issue #575. + + Notes: + See :meth:`pcapkit.protocols.schema.transport.tcp.PortEnumField.post_process`, + whose notes this mirrors verbatim aside from the transport. """ value = super(EnumField, self).post_process(value, packet) - return self._namespace.get(value, proto=Enum_TransportProtocol.udp) + proto = Enum_TransportProtocol.udp + if not (isinstance(value, int) and 0 <= value < (1 << (8 * self.length))): + return self._namespace.get(value, proto=proto) + owner = self._namespace._dispatch(value, proto) # pylint: disable=protected-access + if not owner.__registry__.getlist(value): # type: ignore[union-attr] + try: + declared = owner._missing_(value) # pylint: disable=protected-access + except ValueError as error: + if isinstance(error, BaseError): + raise + declared = None + if declared is None: + return self._unregistered_member( + owner, f'unknown [{value:d} - {proto.name}]', + svc='unknown', port=value, proto=proto) + return self._namespace.get(value, proto=proto) @schema_final diff --git a/tests/corekit/test_fields_numbers_port_option_no_mint_unit.py b/tests/corekit/test_fields_numbers_port_option_no_mint_unit.py new file mode 100644 index 000000000..8ecb88d93 --- /dev/null +++ b/tests/corekit/test_fields_numbers_port_option_no_mint_unit.py @@ -0,0 +1,541 @@ +# -*- coding: utf-8 -*- +"""A registry-backed ``EnumField`` subclass minted a member per unassigned value. + +GitHub issue #575. :meth:`PortEnumField.post_process +` (TCP, UDP, +SCTP) and :meth:`OptionEnumField.post_process +` (PCAP-NG) +each resolved a wire value by calling their registry's ``.get()`` directly:: + + return self._namespace.get(value, proto=...) + +which mints a fresh member -- via :func:`aenum.extend_enum` -- for any value +neither an existing row nor one of the registry's own documented spans +accounts for. On ``examples/captures/http.pcap`` that is 111 calls, all from +ephemeral TCP ports with no IANA assignment, and measured at 13-17% of +extraction self time. + +Every one of those four subclasses now falls through to +:meth:`EnumField._unregistered_member ` +instead of minting: an actual instance of the registry the field names -- +``isinstance`` holds, and it renders and dispatches exactly like a declared +member -- built by calling the registry's own storage base's ``__new__`` +directly and skipping the registry's own ``__new__`` (and the +``cls.__registry__.add(...)`` / ``cls.__members_ns__[...]`` line inside it) +entirely, so nothing is ever added to any of its lookup tables. That is the +owner's ruling on this issue's return type: *"we should even apply to all +other Enum's legit but unbounded values -- so that we dont create registered +enums out of unrecognised/unregistered values, unless user/caller explicitly +created them"* -- tracked more broadly as #775, and applied here only to +these four call sites. + +This mirrors :mod:`tests.corekit.test_fields_numbers_unassigned_enum`'s shape: +the field is tested directly, in isolation from extraction, and the "still +raises" and "still resolves to the real, named member" paths are pinned +alongside the fallback so the fix cannot be mistaken for a blanket +``except ValueError: pass``. + +""" + +from __future__ import annotations + +import copy +import enum +import pickle +import unittest +from typing import TYPE_CHECKING + +import aenum + +from tests._support import purge_modules + +if TYPE_CHECKING: + from typing import Any + + +class IntCode(aenum.IntEnum): + """An :class:`int`-backed registry stand-in. + + Module level rather than local to a test method because :mod:`pickle` has + to be able to find it by name, which a class defined inside a method cannot + be. + + """ + + first = 1 + second = 2 + + +class StdlibIntCode(enum.IntEnum): + """The same shape, from the standard library's :mod:`enum`. + + ``_unregistered_member`` is annotated for both libraries, so both are + exercised rather than assuming they behave alike here. + + """ + + first = 1 + second = 2 + + +class OpaqueCode(aenum.Enum): + """A registry deriving from neither :class:`str` nor :class:`int`.""" + + alpha = 'a' + beta = 'b' + + +class PortEnumFieldBoundedFallbackTests(unittest.TestCase): + """TCP, UDP and SCTP's port fields.""" + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def _fields(self) -> 'list[tuple[str, Any, Any]]': + from pcapkit.const.reg.apptype import AppType, TransportProtocol + from pcapkit.protocols.schema.transport import sctp as sctp_schema + from pcapkit.protocols.schema.transport import tcp as tcp_schema + from pcapkit.protocols.schema.transport import udp as udp_schema + + return [ + ('tcp', tcp_schema.PortEnumField(length=2, namespace=AppType), TransportProtocol.tcp), + ('udp', udp_schema.PortEnumField(length=2, namespace=AppType), TransportProtocol.udp), + ('sctp', sctp_schema.PortEnumField(length=2, namespace=AppType), TransportProtocol.sctp), + ] + + def test_an_ephemeral_port_resolves_without_growing_the_registry(self) -> None: + """The issue's own finding: 111 ephemeral ports, 111 ``extend_enum`` calls. + + 54321 carries no IANA assignment on any of the three transports and + sits in none of ``AppType._missing_``'s documented spans, so this is + exactly the value that used to mint ``PORT_54321_``. The + resolved member still carries ``.port``, ``.svc`` and ``.proto`` -- + read unconditionally by e.g. ``Transport._decode_next_layer``'s + ``srcport.port`` -- because losing them would trade one crash for + another. Per the owner's ruling, it is now a real member of the + registry -- ``isinstance`` holds against both the per-transport + registry and ``AppType`` itself -- rather than some foreign stand-in, + and two such members for the same value are distinct objects, which + is what proves neither was cached into any lookup table. + + """ + from pcapkit.const.reg.apptype import AppType + + for label, field, proto in self._fields(): + with self.subTest(transport=label): + owner = AppType.__registries__[proto] + before = len(owner.__members__) + + resolved = field.unpack(b'\xd4\x31', {}) # 54321 + resolved_again = field.unpack(b'\xd4\x31', {}) + + self.assertEqual(len(owner.__members__), before) + self.assertIsInstance(resolved, owner) + self.assertIsInstance(resolved, AppType) + self.assertEqual(resolved.port, 54321) + self.assertEqual(resolved.svc, 'unknown') + self.assertEqual(resolved.proto, proto) + self.assertEqual(resolved.name, '') + # NOTE: a real member of a registered value would be the same + # object every time (aenum caches by value); two distinct + # objects here is what proves this one was never registered. + self.assertIsNot(resolved, resolved_again) + + def test_two_unregistered_members_for_the_same_port_compare_equal(self) -> None: + """A caveat of returning a real member: equality is by ``.port``, not + identity, and that holds even across two never-registered instances. + + ``AppType`` defines ``__eq__``/``__hash__`` off ``.port`` for every + member, declared or not, so this is not new to the unregistered case + -- but it is worth pinning explicitly, since nothing else in this + module would notice if it silently changed. No reader in this + package compares a resolved port with ``is`` or keys a mapping on it + expecting identity (checked by inspection, not asserted here). + + """ + from pcapkit.const.reg.apptype import AppType + + for label, field, proto in self._fields(): + with self.subTest(transport=label): + first = field.unpack(b'\xd4\x31', {}) # 54321 + second = field.unpack(b'\xd4\x31', {}) # same port again + + self.assertIsNot(first, second) + self.assertEqual(first, second) + self.assertEqual(hash(first), hash(second)) + self.assertIsInstance(first, AppType) + self.assertIsInstance(second, AppType) + + def test_a_value_lookup_on_the_resolved_port_still_raises(self) -> None: + """The member is absent from ``_value2member_map_``, so looking the + same port back up *by value*, on the same per-transport class the + field resolved it against -- rather than reading the attributes off + the object already in hand -- still raises, exactly as it did before + this member existed. Nothing on the parse or reconstruction path + does this to a value it just resolved; this pins that the registry + itself has not changed underneath that assumption. + + A direct call to ``.get()`` is different on purpose and stays that + way: the owner's ruling and the docstring above are both explicit + that ``get()`` is the public API and minting for a caller who asked + for this exact value directly is defensible, which is also what + ``tests/const/test_const_apptype_split_unit.py`` pins for + ``PORT_59001_tcp``/``PORT_51_tcp``. So this only asserts the + value-lookup side, never the ``.get()`` side. + + """ + from pcapkit.const.reg.apptype import AppType + + for label, field, proto in self._fields(): + with self.subTest(transport=label): + owner = AppType.__registries__[proto] + resolved = field.unpack(b'\xd4\x31', {}) # 54321 + del resolved # the object itself is not what is re-looked-up + + with self.assertRaises(ValueError): + owner(54321) + + def test_an_assigned_port_still_resolves_to_its_declared_member(self) -> None: + """Port 80 is still ``http``, on every transport that carries it.""" + from pcapkit.const.reg.apptype import AppType + + for label, field, proto in self._fields(): + with self.subTest(transport=label): + owner = AppType.__registries__[proto] + before = len(owner.__members__) + + resolved = field.unpack(b'\x00\x50', {}) # 80 + + self.assertEqual(len(owner.__members__), before) + self.assertIsInstance(resolved, AppType) + self.assertEqual(resolved.svc, 'http') + self.assertIs(resolved, AppType.get(80, proto=proto)) + + def test_a_documented_span_still_mints_its_own_bounded_member(self) -> None: + """``_missing_``'s own declared-range mechanism is untouched. + + 230 falls in the ``225-241`` "Reserved" span every registry's + ``_missing_`` answers regardless of transport, so it has always + resolved to a real, named member -- the mechanism the issue + explicitly leaves alone, as distinct from ``get()``'s unbounded + fall-through this fix removes. + + """ + from pcapkit.const.reg.apptype import AppType + + for label, field, proto in self._fields(): + with self.subTest(transport=label): + resolved = field.unpack(b'\x00\xe6', {}) # 230 + + self.assertIsInstance(resolved, AppType) + self.assertEqual(resolved.svc, 'reserved') + self.assertEqual(resolved.port, 230) + + def test_an_out_of_width_port_still_raises_764s_rejection(self) -> None: + """The bounded fallback must not revert GitHub issue #764. + + ``post_process`` is reachable directly -- not only through ``unpack``, + which cannot even construct an out-of-range value from a 2-octet + field -- and a catch keyed on exception type alone cannot tell + #764's deliberate out-of-range rejection apart from :mod:`aenum`'s own + "no member has this value" for an in-range but unassigned port: both + are a bare :exc:`ValueError`. So ``-1`` and ``70000`` (neither + representable by a 2-octet port) must still raise, exactly as + ``AppType.get()`` itself does, while ``54321`` (in-range, merely + unassigned) still resolves to the unregistered member. + + """ + from pcapkit.const.reg.apptype import AppType + + for label, field, proto in self._fields(): + with self.subTest(transport=label): + for out_of_width in (-1, 70000): + with self.subTest(transport=label, value=out_of_width): + with self.assertRaises(ValueError): + field.post_process(out_of_width, {}) + + resolved = field.post_process(54321, {}) + self.assertIsInstance(resolved, AppType) + self.assertEqual(resolved.port, 54321) + + def test_an_in_library_rejection_is_not_absorbed(self) -> None: + """A registry's own deliberate decision still propagates. + + Patches the TCP registry's ``_missing_`` to reject with one of + :mod:`pcapkit.utilities.exceptions`, which a blanket + ``except ValueError: pass`` would have swallowed. Nothing under + :mod:`pcapkit.const` actually raises one here today -- this is the + stand-in the fallback still has to respect regardless. + + """ + from pcapkit.const.reg.apptype import AppType, TransportProtocol + from pcapkit.protocols.schema.transport import tcp as tcp_schema + from pcapkit.utilities.exceptions import FieldValueError + + owner = AppType.__registries__[TransportProtocol.tcp] + had_own = '_missing_' in owner.__dict__ + original = owner.__dict__.get('_missing_') + + def _reject(cls: object, value: int) -> None: + raise FieldValueError('%r is deliberately rejected' % value) + + owner._missing_ = classmethod(_reject) + try: + field = tcp_schema.PortEnumField(length=2, namespace=AppType) + with self.assertRaises(FieldValueError): + field.unpack(b'\xd4\x31', {}) + finally: + if had_own: + owner._missing_ = original + else: + del owner._missing_ + + def test_the_resolved_pseudo_member_repacks_to_the_octets_it_came_from(self) -> None: + """The fallback is round-trip safe, same as #701's.""" + from pcapkit.const.reg.apptype import AppType + from pcapkit.protocols.schema.transport import tcp as tcp_schema + + field = tcp_schema.PortEnumField(length=2, namespace=AppType) + resolved = field.unpack(b'\xd4\x31', {}) + + self.assertEqual(field.pack(resolved, {}), b'\xd4\x31') + + +class OptionEnumFieldBoundedFallbackTests(unittest.TestCase): + """PCAP-NG's option-type field.""" + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_an_undeclared_option_code_resolves_without_growing_the_registry(self) -> None: + """An interface-block option code no ``if`` or ``opt`` row covers. + + ``OptionType._missing_`` never declines -- unlike ``AppType``'s, it + mints unconditionally on any miss -- so this fallback cannot consult + it the way the port fields consult ``AppType``'s, and instead + replicates the read-only membership test ``OptionType.get`` itself + runs first. ``.opt_name`` and ``.opt_value`` are read unconditionally + by pcapng's own ``_option_key``. + + """ + from pcapkit.const.pcapng.option_type import OptionType + from pcapkit.protocols.schema.misc import pcapng as pcapng_schema + + field = pcapng_schema.OptionEnumField(length=2, namespace='if') + before = len(OptionType.__members__) + + resolved = field.unpack(b'\x27\x0f', {}) # 9999 + resolved_again = field.unpack(b'\x27\x0f', {}) + + self.assertEqual(len(OptionType.__members__), before) + self.assertIsInstance(resolved, OptionType) + self.assertEqual(resolved.opt_name, 'if_unknown') + self.assertEqual(resolved.opt_value, 9999) + # NOTE: as for AppType -- a real, registered member would be the same + # cached object every time; two distinct ones proves it never was. + self.assertIsNot(resolved, resolved_again) + + def test_a_declared_option_code_still_resolves_to_its_member(self) -> None: + """``if_tsresol`` (9) is still ``if_tsresol``.""" + from pcapkit.const.pcapng.option_type import OptionType + from pcapkit.protocols.schema.misc import pcapng as pcapng_schema + + field = pcapng_schema.OptionEnumField(length=2, namespace='if') + before = len(OptionType.__members__) + + resolved = field.unpack(b'\x00\x09', {}) # if_tsresol == 9 + + self.assertEqual(len(OptionType.__members__), before) + self.assertIsInstance(resolved, OptionType) + self.assertEqual(resolved.opt_name, 'if_tsresol') + + def test_the_resolved_pseudo_member_repacks_to_the_octets_it_came_from(self) -> None: + """The fallback is round-trip safe here too.""" + from pcapkit.protocols.schema.misc import pcapng as pcapng_schema + + field = pcapng_schema.OptionEnumField(length=2, namespace='if') + resolved = field.unpack(b'\x27\x0f', {}) + + self.assertEqual(field.pack(resolved, {}), b'\x27\x0f') + + +class UnregisteredMemberStorageBaseTests(unittest.TestCase): + """``_unregistered_member``'s ``int`` branch and its ``TypeError`` guard. + + Neither is reachable from the four call sites this issue touches: + :class:`~pcapkit.const.reg.apptype.AppType` and + :class:`~pcapkit.const.pcapng.option_type.OptionType` are both + ``StrEnum``, so all four take the :class:`str` branch. The method is + written for registries in general, though, and an untested branch in a + fallback is the kind that turns out to be wrong the first time something + needs it -- so both are driven directly here, against registries standing + in for the ones a future caller would hand it. + + """ + + def test_an_int_backed_namespace_gets_an_int_backed_member(self) -> None: + """``int.__new__`` is used where the registry derives from :class:`int`. + + The resulting member has to be usable *as* an :class:`int` -- that is + the whole reason the branch picks the storage base rather than always + using :class:`str` -- and, exactly as on the :class:`str` path, must + leave the registry's own tables untouched. Both ``__members__`` and + ``_value2member_map_`` are compared as mappings rather than by length, + so a substitution of equal size would still be caught. + + """ + from pcapkit.corekit.fields.numbers import EnumField + + for namespace in (IntCode, StdlibIntCode): + with self.subTest(namespace=namespace.__name__): + members = dict(namespace.__members__) + by_value = dict(namespace._value2member_map_) # pylint: disable=protected-access + + member = EnumField._unregistered_member( # pylint: disable=protected-access + namespace, 4242, 'unassigned', label='none') + + self.assertIsInstance(member, namespace) + self.assertIsInstance(member, int) + self.assertEqual(int(member), 4242) + self.assertEqual(member.value, 4242) + self.assertEqual(member.name, 'unassigned') + self.assertEqual(member.label, 'none') + + self.assertEqual(dict(namespace.__members__), members) + self.assertEqual( + dict(namespace._value2member_map_), by_value) # pylint: disable=protected-access + # NOTE: the same value-keyed lookup the str path is pinned on + # above -- absent from _value2member_map_, so it still raises. + with self.assertRaises(ValueError): + namespace(4242) + + def test_a_namespace_deriving_from_neither_str_nor_int_is_refused(self) -> None: + """The guard says so plainly instead of guessing a storage base. + + Building such a member with :class:`str` or :class:`int` regardless + would ship one silently missing whatever its real base provides, which + is a worse failure than refusing: it would surface somewhere else + entirely, as a missing method rather than as an unsupported registry. + + """ + from pcapkit.corekit.fields.numbers import EnumField + + before = dict(OpaqueCode.__members__) + + with self.assertRaises(TypeError) as caught: + EnumField._unregistered_member( # pylint: disable=protected-access + OpaqueCode, 'gamma', 'unassigned') + + self.assertIn('derives from neither str nor int', str(caught.exception)) + self.assertEqual(dict(OpaqueCode.__members__), before) + + +class UnregisteredMemberRoundTripTests(unittest.TestCase): + """:mod:`pickle` and :mod:`copy` of a resolved unassigned port. + + ``Enum.__reduce_ex__`` reduces a member to ``(cls, (value,))``, i.e. to the + one lookup an unregistered member is deliberately absent from. Before this + issue's fix the port fields minted, so the member *was* registered and a + round-trip worked; without the ``__reduce_ex__`` + :meth:`~pcapkit.corekit.fields.numbers.EnumField._unregistered_member` + installs, removing the mint would have taken that away -- and taken it away + on read-back, since ``pickle.dumps`` succeeds either way. These pin the + mitigation, and that it does not put the mint back. + + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def _resolved(self) -> 'Any': + """An unassigned TCP port, resolved through the real field.""" + from pcapkit.const.reg.apptype import AppType + from pcapkit.protocols.schema.transport import tcp as tcp_schema + + field = tcp_schema.PortEnumField(length=2, namespace=AppType) + return field.unpack(b'\xd0\x9e', {}) # 53406 + + def test_a_pickle_round_trip_rebuilds_an_equivalent_unregistered_member(self) -> None: + """Every protocol, since the reduction has to name a callable each can + reference -- a module-level function, not one nested in a class, which + protocols below 4 cannot address. + + """ + from pcapkit.const.reg.apptype import AppType + + member = self._resolved() + owner = type(member) + before = len(owner.__members__) + + for protocol in range(pickle.HIGHEST_PROTOCOL + 1): + with self.subTest(protocol=protocol): + restored = pickle.loads(pickle.dumps(member, protocol)) + + self.assertIsInstance(restored, owner) + self.assertIsInstance(restored, AppType) + self.assertEqual(restored, member) + self.assertEqual(restored.port, 53406) + self.assertEqual(restored.svc, 'unknown') + self.assertEqual(restored.proto, member.proto) + self.assertEqual(restored.name, '') + # NOTE: rebuilding goes back through _unregistered_member and + # never through owner.__new__, so it cannot register anything. + self.assertNotIn(restored.name, owner.__members__) + self.assertEqual(len(owner.__members__), before) + + def test_copy_and_deepcopy_survive_without_the_3_11_enum_shim(self) -> None: + """CPython's :mod:`enum` grew ``Enum.__copy__``/``__deepcopy__``, both + returning ``self`` without any lookup, in 3.11; on 3.10 -- still a + supported version here -- neither exists and both fall through to + ``__reduce_ex__``. :class:`aenum.Enum` subclasses stdlib + :class:`enum.Enum` and defines only ``__reduce_ex__`` of its own, so + deleting those two off :class:`enum.Enum` is what 3.10 looks like from + here; there is no 3.10 interpreter on this host to run instead. + + """ + member = self._resolved() + owner = type(member) + before = len(owner.__members__) + + shim = [name for name in ('__copy__', '__deepcopy__') + if name in enum.Enum.__dict__] + if shim: + # 3.11+: both short-circuit ahead of any reduction at all. + self.assertIs(copy.copy(member), member) + self.assertIs(copy.deepcopy(member), member) + + originals = [(name, enum.Enum.__dict__[name]) for name in shim] + for name, _ in originals: + delattr(enum.Enum, name) + try: + for label, copier in (('copy', copy.copy), ('deepcopy', copy.deepcopy)): + with self.subTest(copier=label): + restored = copier(member) + + self.assertIsInstance(restored, owner) + self.assertEqual(restored, member) + self.assertEqual(restored.port, 53406) + self.assertEqual(restored.svc, 'unknown') + self.assertEqual(restored.name, '') + self.assertEqual(len(owner.__members__), before) + finally: + for name, original in originals: + setattr(enum.Enum, name, original) + + def test_the_reducer_is_not_rendered_as_one_of_the_members_attributes(self) -> None: + """``__reduce_ex__`` lives in the member's own ``__dict__``, which is + what :meth:`pcapkit.dumpkit.common.make_dumper`'s ``object_hook`` reads + a member's addon keys out of. It is filtered there by its leading + underscore -- pinned here so the mitigation cannot start leaking a + :class:`functools.partial` into every dumped capture. + + """ + member = self._resolved() + + self.assertIn('__reduce_ex__', member.__dict__) + self.assertEqual([key for key in member.__dict__ if not key.startswith('_')], + ['svc', 'port', 'proto']) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/dumpkit/test_common_unit.py b/tests/dumpkit/test_common_unit.py index 0a79a33a3..0af1f9578 100644 --- a/tests/dumpkit/test_common_unit.py +++ b/tests/dumpkit/test_common_unit.py @@ -173,6 +173,120 @@ def test_make_dumper_object_hook_conversions_and_fallbacks(self) -> None: dumper._append_fallback(123, file) self.assertEqual(file.getvalue(), "'123'") + def test_make_dumper_escapes_strings_only_for_plist_like_output(self) -> None: + """GitHub issue #772: no entity escaping anywhere in :mod:`dictdumper`. + + ``dictdumper.plist.PLIST`` (which is also what ``'xml'`` maps to -- + see ``Extractor.__output__``) interpolates every ````/```` + value straight into its markup, so a value containing ``&``, ``<`` or + ``>`` breaks the document it is embedded in. Filed upstream as + JarryShaw/DictDumper#125. ``json``/``tree``/``text`` have no such + defect and all three characters are legal there, so the escaping in + :func:`~pcapkit.dumpkit.common.make_dumper` must fire for a + PLIST-rooted ``output`` and stay off for anything else -- a plain + :class:`str`, an enum rendered by :func:`~pcapkit.dumpkit.common.render_enum`, + and a :class:`~pcapkit.corekit.multidict.MultiDict` key built from one, + since that key is written by the PLIST writer directly and never + passed back through this hook the way a value is. + + """ + import dictdumper.plist + + from pcapkit.corekit.multidict import MultiDict + from pcapkit.dumpkit.common import make_dumper + + class PlistLikeDumper(dictdumper.plist.PLIST): + """Shares PLIST's class identity without its file I/O.""" + + def __init__(self, *args: 'object', **kwargs: 'object') -> None: # pylint: disable=super-init-not-called + pass + + plist_dumper = make_dumper(PlistLikeDumper)('unused') + self.assertEqual(plist_dumper.object_hook('a & b '), 'a & b <c>') + + unknown = enum.IntEnum('', {'': 1}) + member = getattr(unknown, '') + self.assertEqual(plist_dumper.object_hook(member), + '<unknown>::<unassigned> [1]') + + multidict = MultiDict() + multidict.add(member, 'value') + converted = plist_dumper.object_hook(multidict) + self.assertEqual(converted['<unknown>::<unassigned> [1]'], ['value']) + + # NOTE: unlike BaseDumper above (used elsewhere in this class to prove + # the *fallback* is reached at all), this stub's object_hook is the + # identity for anything it does not itself convert -- exactly what + # dictdumper.dumper.Dumper.object_hook does -- so it is what a + # non-PLIST format such as json/tree/text actually sees. + class IdentityDumper: + def object_hook(self, value: 'object') -> 'object': + return value + + def _encode_func(self, value: 'object') -> 'object': + raise NotImplementedError + + plain_dumper = make_dumper(IdentityDumper)() + self.assertEqual(plain_dumper.object_hook('a & b '), 'a & b ') + self.assertEqual(plain_dumper.object_hook(member), ':: [1]') + + def test_an_unassigned_port_dumps_its_addon_keys_in_the_declared_order(self) -> None: + """GitHub issue #575's fallback must not reorder what it renders. + + :meth:`object_hook` builds a member's addon mapping straight out of its + :attr:`~object.__dict__`, which preserves insertion order, so the order + the attributes were *set* in is the order they are dumped in. + ``AppType.__new__`` sets ``svc``, then ``port``, then ``proto``, while + :meth:`~pcapkit.corekit.fields.numbers.EnumField._unregistered_member` + sets them in whatever order its caller passes them -- so a mismatch + there flips every unassigned port's keys relative to every declared + one's, in all four output formats, while leaving every value correct + and every test that reads by key still passing. Measured on + ``examples/captures/http.pcap``: 1117 rendered port blocks of each + kind, and with the two keyword arguments the other way round the + unassigned ones read ``port, svc, proto`` against the declared + ``svc, port, proto``. + + """ + from pcapkit.const.reg.apptype import AppType, TransportProtocol + from pcapkit.dumpkit.common import make_dumper + from pcapkit.protocols.schema.transport import tcp as tcp_schema + + dumper = make_dumper(BaseDumper)() + field = tcp_schema.PortEnumField(length=2, namespace=AppType) + + declared = dumper.object_hook(AppType.get(80, proto=TransportProtocol.tcp)) + unassigned = dumper.object_hook(field.unpack(b'\xd4\x31', {})) # 54321 + + self.assertEqual(list(declared), ['enum', 'svc', 'port', 'proto']) + self.assertEqual(list(unassigned), list(declared)) + self.assertEqual(unassigned['svc'], 'unknown') + self.assertEqual(unassigned['port'], 54321) + + def test_an_undeclared_option_code_dumps_its_addon_keys_in_order_too(self) -> None: + """The same invariant for PCAP-NG's option types. + + ``OptionType.__new__`` sets ``opt_name`` then ``opt_value``, and + :class:`~pcapkit.protocols.schema.misc.pcapng.OptionEnumField` already + passes them that way -- this pins it, since nothing else would notice + if the two keyword arguments were ever swapped. + + """ + from pcapkit.const.pcapng.option_type import OptionType + from pcapkit.dumpkit.common import make_dumper + from pcapkit.protocols.schema.misc import pcapng as pcapng_schema + + dumper = make_dumper(BaseDumper)() + field = pcapng_schema.OptionEnumField(length=2, namespace='if') + + declared = dumper.object_hook(OptionType.if_tsresol) + undeclared = dumper.object_hook(field.unpack(b'\x27\x0f', {})) # 9999 + + self.assertEqual(list(declared), ['enum', 'opt_name', 'opt_value']) + self.assertEqual(list(undeclared), list(declared)) + self.assertEqual(undeclared['opt_name'], 'if_unknown') + self.assertEqual(undeclared['opt_value'], 9999) + @unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') class DumpkitIOTests(unittest.TestCase):