diff --git a/CHANGELOG.md b/CHANGELOG.md index 721b795c3..b944d395f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,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** -- two more gaps the #514 keyword audit turned up, neither previously covered by a test: `StreamEOFError`'s docstring did not say that `@prepare` always raises it with `quiet=True` -- the same end-of-stream convention `StructError` follows via its own `eof=True` -- so nothing pinned that silence against a future regression; and `register_extractor_engine`'s real keyword, `name`, was not itself under test, only its already-corrected docstring, so a future rename could put the two out of step again exactly as quietly as before. - **Fixed** -- `FieldBase.unpack` zero-padded straight up to a field's declared `length` with `rjust()`, regardless of how little data `buffer` actually held; a ~40-octet PCAP-NG Decryption Secrets Block with a bogus inner length was enough to force a multi-gigabyte allocation, since `length` is frequently wire-derived and so attacker-controlled. A declared length past 262144 octets -- libpcap's own `MAXIMUM_SNAPLEN`, and this package's own default `snaplen` -- that the buffer cannot back now raises `FieldValueError` instead of padding for it; the option and list loops' own tolerance for a short read past a truncated area (#431) is far under that ceiling and is untouched (#554). - **Fixed** -- `TCP._make_mptcp_addaddr` could not build an `ADD_ADDR` option end to end: its `kind=`/`length=` arguments were rejected with `UnknownFieldWarning` and silently dropped, and `.pack()` then raised `KeyError: 'length'` from `port`'s own condition, `pkt['length'] in (10, 22)`. The cause was one layer up -- `MPTCP`, the base class every Multipath TCP subtype schema inherits, declared `kind` and `length` only under `typing.TYPE_CHECKING` rather than as real fields, unlike `Option`, which every non-Multipath TCP option schema inherits instead. That silently dropped `kind=`/`length=` for every `_make_mptcp_*` constructor, not only `ADD_ADDR`'s, so `MPTCP` now declares both for real, the same way `Option` already did (#541). The same missing fields broke parsing too: with no `kind`/`length` fields ahead of it, a Multipath TCP subtype schema's own leading field read the `kind` octet itself rather than the octet meant for it, an off-by-two in field alignment rather than a wire-format change -- a correct sender's octets were always right, only this library's reading of them was shifted. Spec-correct `ADD_ADDR` and `MP_PRIO` options failed to parse with `FieldError: TCP: [OptNo 30] 3 invalid IP version` and `KeyError: 'length'` respectively; both parse correctly now. - **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. diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 23344a4a7..5a1f45f78 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -336,6 +336,14 @@ 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** -- two more gaps the #514 keyword audit turned up, neither + previously covered by a test: ``StreamEOFError``'s docstring did not say + that ``@prepare`` always raises it with ``quiet=True`` -- the same + end-of-stream convention ``StructError`` follows via its own ``eof=True`` -- + so nothing pinned that silence against a future regression; and + ``register_extractor_engine``'s real keyword, ``name``, was not itself + under test, only its already-corrected docstring, so a future rename could + put the two out of step again exactly as quietly as before. * **Fixed** -- ``FieldBase.unpack`` zero-padded straight up to a field's declared ``length`` with ``rjust()``, regardless of how little data ``buffer`` actually held; a ~40-octet PCAP-NG Decryption Secrets Block with a diff --git a/pcapkit/utilities/exceptions.py b/pcapkit/utilities/exceptions.py index 72d6c9a48..0f68782eb 100644 --- a/pcapkit/utilities/exceptions.py +++ b/pcapkit/utilities/exceptions.py @@ -431,6 +431,14 @@ class StreamEOFError(BaseError, EOFError): A *declared* zero length -- a nested schema legitimately sized to have nothing to read -- is a different situation and does not raise this. + Note: + :func:`~pcapkit.utilities.decorators.prepare` always raises this with + ``quiet=True``: reaching end of stream is the frame reader's ordinary + way of finding out there is nothing left to parse, not a fault to + log -- the same convention + :exc:`~pcapkit.utilities.exceptions.StructError` follows for the + same situation via its own ``eof=True``. + """ diff --git a/tests/foundation/registry/test_foundation_keyword_names.py b/tests/foundation/registry/test_foundation_keyword_names.py new file mode 100644 index 000000000..8659a1ab6 --- /dev/null +++ b/tests/foundation/registry/test_foundation_keyword_names.py @@ -0,0 +1,62 @@ +"""The registry helpers' documented keyword names must be the real ones. + +:func:`~pcapkit.foundation.registry.foundation.register_extractor_engine` +once documented its first argument as ``engine`` while the signature +declared ``name``, so a caller following the docstring got a +:exc:`TypeError` rather than a registration -- noted in passing while +auditing the ``__init_subclass__`` keyword rename in #557. The docstring +has since been corrected to say ``name``, matching the signature, but +nothing pinned the pairing itself, so a future rename could put the two +back out of step just as silently as before. + +These pin the keyword spelling directly against the real signature. + +""" +from __future__ import annotations + +import importlib.util +import inspect +import unittest +from unittest import mock + +from tests._support import purge_modules + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class RegistryKeywordNameTests(unittest.TestCase): + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_register_extractor_engine_takes_name_not_engine(self) -> None: + """``name=`` registers; ``engine=`` -- as documented -- does not.""" + from pcapkit.corekit.module import ModuleDescriptor + from pcapkit.foundation.registry import foundation as registry + + descriptor = ModuleDescriptor('pcapkit.foundation.engines', 'Engine') + + with mock.patch.object(registry.Extractor, 'register_engine') as register_engine: + registry.register_extractor_engine(name='unit-engine-by-keyword', + module=descriptor) + register_engine.assert_called_once_with('unit-engine-by-keyword', descriptor) + + with mock.patch.object(registry.Extractor, 'register_engine') as register_engine: + with self.assertRaises(TypeError) as caught: + registry.register_extractor_engine(engine='unit-engine-by-keyword', + module=descriptor) + register_engine.assert_not_called() + self.assertIn('engine', str(caught.exception)) + + def test_register_extractor_engine_signature_names_name(self) -> None: + """The documented keyword has to be the one the signature declares.""" + from pcapkit.foundation.registry import foundation as registry + + parameters = inspect.signature(registry.register_extractor_engine).parameters + self.assertIn('name', parameters) + self.assertNotIn('engine', parameters) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/utilities/test_quiet_exceptions.py b/tests/utilities/test_quiet_exceptions.py index e130b156f..66b91aa0d 100644 --- a/tests/utilities/test_quiet_exceptions.py +++ b/tests/utilities/test_quiet_exceptions.py @@ -17,6 +17,7 @@ """ from __future__ import annotations +import io import os import sys import traceback @@ -56,6 +57,7 @@ def setUp(self) -> None: modules = bootstrap(devmode=False) self.exceptions = modules['exceptions'] self.multidict = modules['multidict'] + self.decorators = modules['decorators'] self.logger = modules['logging'].logger def tearDown(self) -> None: @@ -133,6 +135,62 @@ def test_quiet_error_leaves_tracebacklimit_alone(self) -> None: self.assertEqual(_unrelated_failure(), before) self.assertGreater(before, 1) + def test_prepare_raises_stream_eof_error_quietly(self) -> None: + """End of stream is control flow, so ``prepare`` raises it silently. + + :func:`~pcapkit.utilities.decorators.prepare` passes ``quiet=True`` + when a *measured* -- as opposed to caller-declared -- read length comes + back zero, because that is how the frame reader learns a capture is + exhausted rather than a fault worth a log record. It is the same + convention :exc:`~pcapkit.utilities.exceptions.StructError` follows via + its own ``eof=True``. + + Dropping the ``quiet=True`` would put one ``CRITICAL`` record on + :data:`sys.stderr` for every capture parsed to completion, which is the + #362 defect this module exists for -- and would also set + :data:`sys.tracebacklimit` to ``0`` process-wide. + + """ + class DemoSchema: + @classmethod + def pre_unpack(cls, packet): + return None + + def post_process(self, packet): + return packet + + @classmethod + @self.decorators.prepare + def unpack(cls, data, length=None, packet=None): + return cls() + + if hasattr(sys, 'tracebacklimit'): + del sys.tracebacklimit + + with capture(self.logger) as recorder: + with self.assertRaises(self.exceptions.StreamEOFError) as caught: + DemoSchema.unpack(io.BytesIO(b''), None, None) + + self.assertEqual(recorder.messages, []) + self.assertEqual(str(caught.exception), 'prepare: end of stream') + self.assertFalse(hasattr(sys, 'tracebacklimit')) + + def test_loud_stream_eof_error_still_logs(self) -> None: + """The control for the test above: ``quiet`` is what silences it. + + Without this, ``recorder.messages == []`` would also pass if + :exc:`~pcapkit.utilities.exceptions.StreamEOFError` had simply stopped + logging altogether. + + """ + if hasattr(sys, 'tracebacklimit'): + del sys.tracebacklimit + + with capture(self.logger) as recorder: + self.exceptions.StreamEOFError('boom') + + self.assertEqual(recorder.messages, [('CRITICAL', 'StreamEOFError: boom')]) + def test_loud_error_still_limits_the_traceback(self) -> None: """The feature a loud error provides is unchanged.""" if hasattr(sys, 'tracebacklimit'):