diff --git a/CHANGELOG.md b/CHANGELOG.md index b6f7e877a..5c3bbd6e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,7 @@ The largest release since 1.0, and the first recorded here as it happened rather - **Fixed** -- constant lookups that rejected a value the registry defines. `RouterAlert(0)` is the only value [RFC 2113](https://datatracker.ietf.org/doc/html/rfc2113) defines and the one IGMP, RSVP and MLD actually send, and it was discarded because the vendor crawler skipped a header row IANA's CSV does not have; IPX `Socket(0)` is that protocol's own default, so `bytes(IPX(...))` crashed on its own defaults; and two FTP `_missing_` overrides were plain methods rather than classmethods, so every unregistered value raised `TypeError` instead of extending the enumeration (#492, #503). - **Fixed** -- `format='text'` raised `AttributeError` before writing anything, naming a `dictdumper.Text` that has never existed. It now points at `Tree`, as the `'txt'` alias beside it already did. - **Fixed** -- 45 places where a documentation page contradicted the code (#413), ambiguous cross-references and five autodoc signature failures (#416), and `Extractor`'s documented exception plus 40 phantom or stale `Args:` labels (#501). +- **Fixed** -- which exception a malformed TCP SACK option raised depended on unrelated process state: a clean interpreter raised `ProtocolError` as documented, but a process that had already popped `pcapkit.corekit.fields.misc` from `sys.modules` -- which the `#439` ABC-cache regression tests do in every case's `setUp`/`tearDown` -- raised `FieldValueError` instead, from a different layer entirely, before the documented check was even reached (#525). The cause was `ListField.unpack` resolving `SchemaField` through a function-local import re-run on every call; a module popped and reimported mid-process comes back as a second, distinct class, so `isinstance` against it silently misclassified the field and billed each item by its declared length instead of by what it actually consumed. **Any caller relying on the previously-observed** `FieldValueError` **for this case now gets** `ProtocolError` **instead, deterministically**, matching the method's own docstring. Fixed by importing at module level instead. - **Fixed** -- two dropped-keyword/wrong-cast defects flagged in review during this release and never filed until now: HIP's `_make_param_encrypted` passed `cipher=` to a schema with no such field, so the value was silently dropped and an AES-cipher `ENCRYPTED` parameter built through `make` packed without its IV; and IPv6-Route's `RPL.post_process`, which runs on every `Schema.pack` and not only after a parse, assumed `self.addresses` was still the concatenated `bytes` a parse leaves it as, and raised slicing the `list[bytes]` a `make`-built multi-address header actually holds there (#556). Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) and `1.5.0b3` (2026-09-19), all published as prereleases and so resolved only by `pip install --pre`. `1.5.0b1` half-shipped: the tag, the GitHub release and the Conda deployments landed, but PyPI rejected the wheel because `twine check` found a Sphinx-only `:mod:` role in `README.rst`, which `pyproject.toml` declares as the dynamic long description. `1.5.0b2` is what reshipped it -- the release workflow is version-driven, so an existing version cannot republish -- and `1.5.0b3` followed the CI change that stops a TestPyPI outage from costing a release its wheels (#497, #498). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 624741d9a..d940d2a55 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -282,6 +282,21 @@ pull requests between #326 and #509. (#413), ambiguous cross-references and five autodoc signature failures (#416), and ``Extractor``'s documented exception plus 40 phantom or stale ``Args:`` labels (#501). +* **Fixed** -- which exception a malformed TCP SACK option raised depended on + unrelated process state: a clean interpreter raised ``ProtocolError`` as + documented, but a process that had already popped + ``pcapkit.corekit.fields.misc`` from ``sys.modules`` -- which the ``#439`` + ABC-cache regression tests do in every case's ``setUp``/``tearDown`` -- + raised ``FieldValueError`` instead, from a different layer entirely, before + the documented check was even reached (#525). The cause was + ``ListField.unpack`` resolving ``SchemaField`` through a function-local + import re-run on every call; a module popped and reimported mid-process comes + back as a second, distinct class, so ``isinstance`` against it silently + misclassified the field and billed each item by its declared length instead + of by what it actually consumed. **Any caller relying on the + previously-observed** ``FieldValueError`` **for this case now gets** + ``ProtocolError`` **instead, deterministically**, matching the method's own + docstring. Fixed by importing at module level instead. * **Fixed** -- two dropped-keyword/wrong-cast defects flagged in review during this release and never filed until now: HIP's ``_make_param_encrypted`` passed ``cipher=`` to a schema with no such field, so the value was silently dropped diff --git a/pcapkit/corekit/fields/collections.py b/pcapkit/corekit/fields/collections.py index 9a7981fb8..1ad8b7192 100644 --- a/pcapkit/corekit/fields/collections.py +++ b/pcapkit/corekit/fields/collections.py @@ -6,6 +6,7 @@ from typing import TYPE_CHECKING, Generic, TypeVar, cast from pcapkit.corekit.fields.field import FieldBase +from pcapkit.corekit.fields.misc import SchemaField from pcapkit.corekit.fields.numbers import NumberField from pcapkit.corekit.multidict import OrderedMultiDict from pcapkit.utilities.compat import List @@ -140,7 +141,34 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'byte if self._item_type is None: return file.read(length) - from pcapkit.corekit.fields.misc import SchemaField + # NOTE: ``SchemaField`` used to be imported here, inside the method body, + # rather than at module level -- there is no cyclic import to dodge by + # doing so; :mod:`pcapkit.corekit.fields.misc` does not import this + # module. A local ``from ... import`` re-resolves against + # :data:`sys.modules` on *every* call, and normally that is a cheap + # dict lookup returning the same cached module -- except when something + # has popped :mod:`pcapkit.corekit.fields.misc` out of + # :data:`sys.modules` since this ``ListField`` was built (the ``#439`` + # regression tests do exactly that, to get a clean ABC-cache state + # between cases). The next call re-executes the module from scratch and + # mints a *second*, distinct ``SchemaField`` class, while + # ``self._item_type`` -- built long before, from the first one -- is + # still an instance of the original. ``isinstance`` against the new + # class then reports :data:`False` for an item that plainly is a + # ``SchemaField``, ``is_schema`` goes the wrong way, and this method's + # other branch bills each item by its *declared* length unconditionally + # instead of by what it actually consumed -- which is what let a + # malformed TCP SACK option's length check raise + # :exc:`~pcapkit.utilities.exceptions.FieldValueError` from here instead + # of :exc:`~pcapkit.utilities.exceptions.ProtocolError` from + # :meth:`TCP._read_mode_sack + # `, depending on + # unrelated things that ran earlier in the same process. C.f. #525. + # Importing at module level, once, for the life of this module object, + # closes that off: ``SchemaField`` then stays whatever class this + # module resolved at its own import time, regardless of what happens to + # :mod:`pcapkit.corekit.fields.misc`'s entry in :data:`sys.modules` + # afterwards. is_schema = isinstance(self._item_type, SchemaField) # NOTE: The item-typed branch below sizes each item by ``field.length``, diff --git a/tests/protocols/transport/test_tcp_sack_length_unit.py b/tests/protocols/transport/test_tcp_sack_length_unit.py index 9cf41ae07..09c438e72 100644 --- a/tests/protocols/transport/test_tcp_sack_length_unit.py +++ b/tests/protocols/transport/test_tcp_sack_length_unit.py @@ -33,6 +33,32 @@ *this* docstring, which is the direction that actually lets a malformed packet through. +:meth:`SACKLengthTests.test_invalid_sack_length_is_rejected` used to accept +either :exc:`~pcapkit.utilities.exceptions.ProtocolError` or +:exc:`~pcapkit.utilities.exceptions.FieldValueError`, because which one arrived +depended on process state rather than on the packet -- filed as #525. The root +cause was :meth:`ListField.unpack +` resolving +:class:`~pcapkit.corekit.fields.misc.SchemaField` through a *function-local* +import on every call, instead of once at module load: ordinarily a cheap +:data:`sys.modules` hit, but a class that has been popped from +:data:`sys.modules` since this field was built (which is exactly what the +``#439`` ABC-cache regression tests do, in every case's ``setUp``/``tearDown``, +to get a clean cache between them) comes back as a second, distinct class on +the next call. ``isinstance(self._item_type, SchemaField)`` then answers +:data:`False` for an item that plainly is one, ``ListField.unpack`` bills it by +its declared length instead of by what it actually consumed, and *that* is what +let the length check go negative and raise +:exc:`~pcapkit.utilities.exceptions.FieldValueError` from +:mod:`pcapkit.corekit.fields.collections` before ``_read_mode_sack`` was ever +reached. Hoisting the import to module level in that fix closed it: the +exception is now always :exc:`~pcapkit.utilities.exceptions.ProtocolError`, +regardless of what ran earlier in the process, and the assertion below is +narrowed to match. +:meth:`SACKLengthTests.test_sack_exception_type_survives_a_sys_modules_purge` +pins the mechanism directly, by reproducing the exact +:data:`sys.modules` manipulation that used to flip it. + One deliberate limit, so a later reader does not think it an oversight: the check implements exactly the rule the docstring states, ``(length - 2) % 8 == 0``. :rfc:`2018` also wants at least one block and at most four, so ``length=2`` @@ -49,10 +75,11 @@ from __future__ import annotations import io +import sys import unittest from pcapkit.protocols.transport.tcp import TCP -from pcapkit.utilities.exceptions import FieldValueError, ProtocolError +from pcapkit.utilities.exceptions import ProtocolError #: TCP option kind for SACK, per :rfc:`2018`. SACK = 5 @@ -117,53 +144,91 @@ def test_valid_sack_lengths_are_accepted(self) -> 'None': def test_invalid_sack_length_is_rejected(self) -> 'None': """A length that is not ``8n + 2`` is rejected. - Every one of these parsed without complaint before the check was + Every one of these parsed without complaint before the #519 check was added, which is the whole point of the case. ``11`` and ``14`` are the interesting ones -- a remainder of 1 and of 4 -- because the block list happily consumes as many whole 8-octet items as it can find and silently ignores the tail. - Two different exceptions satisfy this, and which one arrives depends on - global state rather than on the packet, so the assertion deliberately - accepts either: - - * :exc:`~pcapkit.utilities.exceptions.ProtocolError` from - :meth:`TCP._read_mode_sack - `, the check - this module exists for. This is what a freshly started interpreter - produces for all six lengths. - * :exc:`~pcapkit.utilities.exceptions.FieldValueError` from - :meth:`ListField.unpack - `, whose schema - branch decrements the remaining length by ``len(data)`` per item and - raises once it goes negative. This one was observed in a process that - had already run other parts of the suite, where the option schema is - unpacked before ``_read_mode_sack`` is reached, so the lower layer - notices first and reports instead. - - Be careful how much that second bullet is trusted. The *observation* is - real and is filed as #525, but the trigger is not pinned down: a - deliberate attempt to force it by running - :file:`tests/protocols/schema/` first in the same process did **not** - reproduce it, and ``ProtocolError`` still won for all six lengths. So - the honest statement is that the exception class depends on process - state by some route not yet identified -- not that any particular test - ordering selects it. - - Which is why the assertion is a union rather than a single class: a - caller cannot reliably catch one, and pinning whichever one happens to - arrive here would make this test fail for reasons that have nothing to - do with SACK. Naming both rather than a bare - :exc:`~pcapkit.utilities.exceptions.BaseError` still keeps a third, - unexpected exception a failure. The union records the problem; #525 - owns solving it. + This used to accept either :exc:`~pcapkit.utilities.exceptions.ProtocolError` + or :exc:`~pcapkit.utilities.exceptions.FieldValueError`, since which one + arrived depended on unrelated process state rather than on the packet + -- #525. That is now fixed at the root (see the module docstring and + :meth:`test_sack_exception_type_survives_a_sys_modules_purge` below), + so the exception is pinned to the single documented type: + :exc:`~pcapkit.utilities.exceptions.ProtocolError` from + :meth:`TCP._read_mode_sack + `, which is the + check this module exists for. """ for length in (3, 11, 14, 17, 19, 25): with self.subTest(length=length): - with self.assertRaises((ProtocolError, FieldValueError)) as caught: + with self.assertRaises(ProtocolError) as caught: parse(sack_option(length, length + 8)) - self.assertRegex(str(caught.exception), 'invalid (format|length)') + self.assertRegex(str(caught.exception), 'invalid format') + + def test_sack_exception_type_survives_a_sys_modules_purge(self) -> 'None': + """The exception type does not depend on :data:`sys.modules` state. + + This pins the actual mechanism behind #525, rather than merely + recording the symptom. :meth:`ListField.unpack + ` used to resolve + :class:`~pcapkit.corekit.fields.misc.SchemaField` through a + function-local import, re-run on every call. That is an ordinary + :data:`sys.modules` cache hit almost always -- except when something + has popped :mod:`pcapkit.corekit.fields.misc` out of + :data:`sys.modules` since this ``ListField`` was built, which is + exactly what the ``#439`` ABC-cache regression tests do in every + case's ``setUp``/``tearDown`` (see + :mod:`tests.protocols.schema.test_schema_metaclass_abc_cache_unit`) to + get a clean cache between cases. The next call then re-executes + :mod:`pcapkit.corekit.fields.misc` from scratch and mints a *second*, + distinct ``SchemaField`` class, while the ``sack`` field's + ``item_type`` -- built long before, from the first one -- is still an + instance of the original. ``isinstance`` against the new class then + answers :data:`False` for an item that plainly is a ``SchemaField``, + and ``ListField.unpack`` takes its other branch, which bills each item + by its *declared* length unconditionally instead of by what it + actually consumed -- letting the length check go negative and raise + :exc:`~pcapkit.utilities.exceptions.FieldValueError` from + :mod:`pcapkit.corekit.fields.collections` before + ``_read_mode_sack`` is ever reached. + + Reproducing that does not need the schema test suite at all -- the + :data:`sys.modules` manipulation alone is sufficient, and doing just + that is what makes this a targeted regression test rather than a + repeat of the same "run some other tests first" guess #525 opened + with. Before the fix, this exact removal made + :meth:`ListField.unpack ` + re-import :mod:`pcapkit.corekit.fields.misc` on its next call and mint + the mismatched second ``SchemaField`` class described above; asserting + that :mod:`pcapkit.corekit.fields.misc` is *not* back in + :data:`sys.modules` immediately after ``parse()`` returns is what + confirms the fix (a module-level import, resolved once) rather than + just a passing assertion for the wrong reason. + + """ + self.assertIn( + 'pcapkit.corekit.fields.misc', sys.modules, + 'test setup expects pcapkit to already be imported', + ) + removed = sys.modules.pop('pcapkit.corekit.fields.misc') + try: + with self.assertRaises(ProtocolError) as caught: + parse(sack_option(11, 11 + 8)) + self.assertRegex(str(caught.exception), 'invalid format') + + # The fix hoisted the import to module level, so nothing during + # ``parse()`` re-triggers it: the module stays absent from + # ``sys.modules`` until this test restores it below. Before the + # fix, ``ListField.unpack``'s function-local import would have put + # it right back. + self.assertNotIn('pcapkit.corekit.fields.misc', sys.modules) + finally: + # NOTE: restore, rather than leave the module state disturbed for + # whatever the test runner collects next. + sys.modules['pcapkit.corekit.fields.misc'] = removed def test_documented_rule_is_the_implemented_rule(self) -> 'None': """The check is exactly ``(length - 2) % 8 == 0``, no more.