diff --git a/pcapkit/corekit/fields/collections.py b/pcapkit/corekit/fields/collections.py index 1ad8b71924..42546b56f4 100644 --- a/pcapkit/corekit/fields/collections.py +++ b/pcapkit/corekit/fields/collections.py @@ -1,7 +1,6 @@ # -*- coding: utf-8 -*- """container field class""" -import copy import io from typing import TYPE_CHECKING, Generic, TypeVar, cast @@ -81,7 +80,7 @@ def __call__(self, packet: 'dict[str, Any]') -> 'Self': instead of updating the current instance. """ - new_self = copy.copy(self) + new_self = self.__copy__() new_self._callback(self, packet) if new_self._length_callback is not None: new_self._length = new_self._length_callback(packet) diff --git a/pcapkit/corekit/fields/field.py b/pcapkit/corekit/fields/field.py index 81ee440b2d..ffc543db91 100644 --- a/pcapkit/corekit/fields/field.py +++ b/pcapkit/corekit/fields/field.py @@ -4,7 +4,6 @@ import abc import contextlib import contextvars -import copy import struct from typing import TYPE_CHECKING, Generic, TypeVar, cast @@ -291,7 +290,7 @@ def __call__(self, packet: 'dict[str, Any]') -> 'Self': updating the current instance. """ - new_self = copy.copy(self) + new_self = self.__copy__() new_self._callback(new_self, packet) return new_self @@ -314,6 +313,19 @@ def __copy__(self) -> 'Self': done, and only that: a new instance of the same class, its :attr:`~object.__dict__` shallow-updated from this one. + Note: + Every ``__call__`` override in this module calls ``self.__copy__()`` + directly rather than :func:`copy.copy(self) `. + :func:`copy.copy` still has to *find* this method before it can call + it -- ``getattr(cls, '__copy__', None)`` -- and that lookup alone + was profiled at 55,846 calls (~1.7% of an :func:`~pcapkit.interface. + core.extract` run) on ``examples/captures/http.pcap``, one per field + per packet, all from this exact path. Calling ``__copy__`` + directly is exactly what :func:`copy.copy` would have done once it + found it, so this changes nothing about *when* a field is copied or + what the copy contains -- only the redundant dispatch is removed. + See GitHub issue #730. + Returns: A new field instance sharing this one's attribute values. @@ -576,7 +588,7 @@ def __call__(self, packet: 'dict[str, Any]') -> 'Self': updating the current instance. """ - new_self = copy.copy(self) + new_self = self.__copy__() new_self._callback(new_self, packet) if new_self._length_callback is not None: new_self._length = new_self._length_callback(packet) diff --git a/pcapkit/corekit/fields/misc.py b/pcapkit/corekit/fields/misc.py index 7f1397ecf7..4a0364de68 100644 --- a/pcapkit/corekit/fields/misc.py +++ b/pcapkit/corekit/fields/misc.py @@ -1,7 +1,6 @@ # -*- coding: utf-8 -*- """miscellaneous field class""" -import copy import io from typing import TYPE_CHECKING, TypeVar, cast @@ -145,7 +144,7 @@ def __call__(self, packet: 'dict[str, Any]') -> 'Self': instead of updating the current instance. """ - new_self = copy.copy(self) + new_self = self.__copy__() if new_self._condition(packet): new_self._field = new_self._field(packet) return new_self @@ -296,7 +295,7 @@ def __call__(self, packet: 'dict[str, Any]') -> 'Self': instead of updating the current instance. """ - new_self = copy.copy(self) + new_self = self.__copy__() new_self._callback(new_self, packet) if new_self._length_callback is not None: new_self._length = new_self._length_callback(packet) @@ -424,7 +423,7 @@ def __call__(self, packet: 'dict[str, Any]') -> 'SwitchField[_TC]': instead of updating the current instance. """ - new_self = copy.copy(self) + new_self = self.__copy__() new_self._field = new_self._selector(packet)(packet) new_self._field.name = self.name return new_self @@ -647,7 +646,7 @@ def __call__(self, packet: 'dict[str, Any]') -> 'Self': instead of updating the current instance. """ - new_self = copy.copy(self) + new_self = self.__copy__() new_self._callback(new_self, packet) if new_self._length_callback is not None: new_self._length = new_self._length_callback(packet) @@ -779,7 +778,7 @@ def __call__(self, packet: 'dict[str, Any]') -> 'Self': instead of updating the current instance. """ - new_self = copy.copy(self) + new_self = self.__copy__() new_self._field = new_self._field(packet) return new_self diff --git a/tests/corekit/test_fields_copy_dispatch_runtime.py b/tests/corekit/test_fields_copy_dispatch_runtime.py new file mode 100644 index 0000000000..5cc2b07f36 --- /dev/null +++ b/tests/corekit/test_fields_copy_dispatch_runtime.py @@ -0,0 +1,147 @@ +# -*- coding: utf-8 -*- +"""Byte-identity proof for GitHub issue #730, across the classic-PCAP sample captures. + +:mod:`tests.corekit.test_fields_copy_dispatch_unit` proves the *mechanism* the +issue asked about: every field ``__call__`` now reaches +:meth:`~pcapkit.corekit.fields.field.FieldBase.__copy__` directly instead of +going through :func:`copy.copy`. This module proves the thing that actually +matters -- that swapping the dispatch did not move a single byte of what +:func:`~pcapkit.interface.core.extract` produces. + +Every field of every protocol is copied through one of the eight call sites +#730 named, once per field per packet, so a mistake in any of them is not +confined to one protocol -- it is exactly the kind of change that wants a +broad, mechanical check rather than a handful of hand-picked assertions. Each +capture below is walked record by record using the classic-PCAP framing +itself -- a 24-octet global header, then any number of 16-octet record +headers each followed by ``incl_len`` octets of captured data -- and every +record's repack, ``bytes(frame)``, is compared against the *original file's +own bytes* at that exact offset. There is no golden/expected value baked into +this module to fall out of step with anything: the ground truth is the sample +capture itself, read directly with :func:`open`, independently of whatever +:func:`~pcapkit.interface.core.extract` makes of it. + +PCAP-NG is deliberately out of scope here: its block framing is not a fixed +16-octet record header, so the same offset walk does not apply, and its own +round-trip fidelity is already covered by +:mod:`tests.protocols.test_pcapng_regression`. The field ``__call__`` change +this module guards is format-agnostic -- every field, in every schema, of +every protocol, in every container format, goes through one of the same eight +call sites -- so classic PCAP alone already exercises the mechanism; a +passing run of the existing PCAP-NG suite (unaffected by this change) is the +rest of the proof. + +""" +from __future__ import annotations + +import importlib.util +import unittest + +from tests._support import purge_modules, sample_path + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: Every classic-PCAP sample capture this module checks. ``in.pcap`` is the one +#: git tracks; the rest are built by :file:`examples/generators/make_samples.py` +#: and read through :func:`~tests._support.sample_path`, which is why this +#: module is a ``_runtime.py`` -- the fixture-dependent tier that may read them +#: freely, rather than the unit tier that may not (see :mod:`tests._tiers`). +#: ``http.pcap`` is #730's own reproduction capture; the rest span the other +#: byte orders, timestamp resolutions and protocol families the library reads. +CLASSIC_PCAP_CAPTURES = ( + 'in.pcap', + 'http.pcap', + 'arp.pcap', + 'tcp.pcap', + 'ipv4.pcap', + 'ipv6.pcap', + 'stream.pcap', + 'test.pcap', + 'http6.cap', + 'big_endian.pcap', + 'little_endian.pcap', + 'big_endian_nanosecond.pcap', + 'options-internet.pcap', + 'options-ipv4.pcap', + 'options-ipv6.pcap', + 'options-tcp.pcap', + 'options-transport.pcap', +) + +#: Octets in a classic-PCAP global header, before the first record. +_GLOBAL_HEADER_LENGTH = 24 +#: Octets in a classic-PCAP record header, before that record's captured data. +_RECORD_HEADER_LENGTH = 16 + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class FieldCopyDispatchByteIdentityTests(unittest.TestCase): + """``bytes(frame) == ``, for every record. + + Deliberately does not re-derive ``incl_len`` independently -- that is + exactly what :mod:`tests.protocols.misc.pcap.test_frame_endian_runtime` + already does, by unpacking the record header with :mod:`struct` ahead of + the parser and comparing. Here each record's own reported ``incl_len`` + both selects the slice it is compared against *and* advances to the next + record, so a field that misreports its own length is caught the same way + a field that mis-repacks its value is: either way, the walk stops lining + up with the file's real record boundaries and the running-total assertion + at the end of :meth:`assertCaptureRoundTripsByteForByte` catches it. + + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def assertCaptureRoundTripsByteForByte(self, name: str) -> None: + """Walk every record of ``name`` and compare its repack to the file's own bytes. + + Args: + name: Bare sample-capture file name, resolved through + :func:`~tests._support.sample_path`. + + """ + from pcapkit.interface import extract + + path = sample_path(name) + with open(path, 'rb') as stream: + raw = stream.read() + + extractor = extract(fin=path, store=True, nofile=True, engine='default') + frames = list(extractor.frame) + + self.assertGreater(len(frames), 0, f'{name} produced no frames at all') + + offset = _GLOBAL_HEADER_LENGTH + for frame in frames: + with self.subTest(capture=name, frame=frame.info.number): + incl_len = frame.info.frame_info.incl_len + record_end = offset + _RECORD_HEADER_LENGTH + incl_len + + self.assertEqual( + bytes(frame), raw[offset:record_end], + f'{name} frame {frame.info.number} repacked differently from the ' + f'capture\'s own bytes at offset {offset} -- see GitHub issue #730') + offset = record_end + + # Every record accounted the whole file's own record-header-declared + # length, not just some prefix of it -- a length a field under-reports + # would still pass every per-frame comparison above and only show up + # here, as bytes of the file no frame ever claimed. + self.assertEqual( + offset, len(raw), + f'{name}: frames covered {offset} of {len(raw)} octets -- the last records ' + f'were never compared at all') + + def test_every_classic_pcap_sample_round_trips_byte_for_byte(self) -> None: + for name in CLASSIC_PCAP_CAPTURES: + with self.subTest(capture=name): + try: + self.assertCaptureRoundTripsByteForByte(name) + except FileNotFoundError as exc: + self.skipTest(str(exc)) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/corekit/test_fields_copy_dispatch_unit.py b/tests/corekit/test_fields_copy_dispatch_unit.py new file mode 100644 index 0000000000..6459ec61fd --- /dev/null +++ b/tests/corekit/test_fields_copy_dispatch_unit.py @@ -0,0 +1,156 @@ +# -*- coding: utf-8 -*- +"""Regression coverage for GitHub issue #730. + +Every field's :meth:`~pcapkit.corekit.fields.field.FieldBase.__call__` made a +defensive copy of ``self`` once per field per packet, via +``copy.copy(self)`` at eight call sites across +:mod:`pcapkit.corekit.fields.field`, :mod:`pcapkit.corekit.fields.collections` +and :mod:`pcapkit.corekit.fields.misc`. Profiling ``extract()`` on +``examples/captures/http.pcap`` attributed 55,846 ``getattr`` calls (~1.7% of +the run) to that :func:`copy.copy` -- not to anything in this package's own +code, but to :func:`copy.copy` looking itself up: it runs +``getattr(cls, '__copy__', None)`` before it can call the very +:meth:`FieldBase.__copy__` these classes already define, and that lookup is +what a profiler credits to ``builtins.getattr``. + +``copier = getattr(cls, '__copy__', None); copier(x)`` is exactly +``x.__copy__()``, so calling it directly does not change *when* a field is +copied or what the copy contains -- only the redundant lookup is removed. This +module proves that: every one of the eight call sites now reaches +:meth:`FieldBase.__copy__` without ever calling :func:`copy.copy`. + +On the tree before this fix, every test below fails: each ``__call__`` +listed in the issue called :func:`copy.copy` exactly once (twice for +:class:`~pcapkit.corekit.fields.misc.ConditionalField`, +:class:`~pcapkit.corekit.fields.misc.SwitchField` and +:class:`~pcapkit.corekit.fields.misc.ForwardMatchField`, whose ``__call__`` +also calls a nested field's own ``__call__``). + +""" +from __future__ import annotations + +import copy +import unittest +from typing import TYPE_CHECKING +from unittest import mock + +from tests._support import purge_modules + +if TYPE_CHECKING: + from typing import Any + + from pcapkit.corekit.fields.field import FieldBase + + +class FieldCallUsesCopyDunderDirectlyTests(unittest.TestCase): + """Every ``__call__`` in :mod:`pcapkit.corekit.fields` calls ``self.__copy__()``. + + Rather than :func:`copy.copy(self) `. Each test patches + :func:`copy.copy` and :meth:`FieldBase.__copy__` with wrappers that still do + the real work (``side_effect=`` the original), so the field returned is + exactly what it always was; only the call counts are new. + + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + from pcapkit.corekit.fields.field import FieldBase + + self.FieldBase = FieldBase + self.real_copy_dunder = FieldBase.__copy__ + + def assertCallReachesCopyDunderOnly(self, field: 'FieldBase[Any]') -> None: + """Call ``field({})`` once; assert it reached ``__copy__`` but not ``copy.copy``. + + Args: + field: A field instance built with whatever a real call site would + give it -- a real callback default, a real selector, etc. -- + rather than a bare :class:`~unittest.mock.Mock`, so this + exercises the exact ``__call__`` bodies :func:`copy.copy(self) + ` used to run through. + + """ + with mock.patch('copy.copy', side_effect=copy.copy) as copy_copy, \ + mock.patch.object(self.FieldBase, '__copy__', autospec=True, + side_effect=self.real_copy_dunder) as copy_dunder: + new_field = field({}) + + self.assertEqual( + copy_copy.call_count, 0, + 'field.__call__ still routes through copy.copy(self) instead of ' + 'self.__copy__() -- see GitHub issue #730') + self.assertGreaterEqual(copy_dunder.call_count, 1) + self.assertIsNot(new_field, field) + self.assertIsInstance(new_field, type(field)) + + def test_field_base_call_field_py_294(self) -> None: + """:meth:`FieldBase.__call__` (``field.py:294``), reached via :class:`NoValueField`. + + :class:`~pcapkit.corekit.fields.misc.NoValueField` is the one concrete + class in this package that never overrides ``__call__``, so it is the + only way to exercise the base implementation directly rather than + through a subclass's override. + + """ + from pcapkit.corekit.fields.misc import NoValueField + + self.assertCallReachesCopyDunderOnly(NoValueField()) + + def test_field_call_field_py_579(self) -> None: + """:meth:`Field.__call__` (``field.py:579``).""" + from pcapkit.corekit.fields.field import Field + + self.assertCallReachesCopyDunderOnly(Field(length=4)) + + def test_list_field_call_collections_py_84(self) -> None: + """:meth:`ListField.__call__` (``collections.py:84``).""" + from pcapkit.corekit.fields.collections import ListField + + self.assertCallReachesCopyDunderOnly(ListField(length=4)) + + def test_conditional_field_call_misc_py_148(self) -> None: + """:meth:`ConditionalField.__call__` (``misc.py:148``).""" + from pcapkit.corekit.fields.field import Field + from pcapkit.corekit.fields.misc import ConditionalField + + field = ConditionalField(field=Field(length=1), condition=lambda packet: True) + self.assertCallReachesCopyDunderOnly(field) + + def test_payload_field_call_misc_py_299(self) -> None: + """:meth:`PayloadField.__call__` (``misc.py:299``).""" + from pcapkit.corekit.fields.misc import PayloadField + + self.assertCallReachesCopyDunderOnly(PayloadField(length=4)) + + def test_switch_field_call_misc_py_427(self) -> None: + """:meth:`SwitchField.__call__` (``misc.py:427``).""" + from pcapkit.corekit.fields.field import Field + from pcapkit.corekit.fields.misc import SwitchField + + field = SwitchField(selector=lambda packet: Field(length=1)) + self.assertCallReachesCopyDunderOnly(field) + + def test_schema_field_call_misc_py_650(self) -> None: + """:meth:`SchemaField.__call__` (``misc.py:650``).""" + from pcapkit.corekit.fields.misc import SchemaField + from pcapkit.corekit.fields.numbers import UInt8Field + from pcapkit.protocols.schema.schema import Schema, schema_final + + @schema_final + class OneField(Schema): + a: 'int' = UInt8Field() + + self.assertCallReachesCopyDunderOnly(SchemaField(schema=OneField)) + + def test_forward_match_field_call_misc_py_782(self) -> None: + """:meth:`ForwardMatchField.__call__` (``misc.py:782``).""" + from pcapkit.corekit.fields.field import Field + from pcapkit.corekit.fields.misc import ForwardMatchField + + field = ForwardMatchField(field=Field(length=1)) + self.assertCallReachesCopyDunderOnly(field) + + +if __name__ == '__main__': + unittest.main()