Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
8 changes: 8 additions & 0 deletions docs/source/changelog/1.5.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 8 additions & 0 deletions pcapkit/utilities/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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``.

"""


Expand Down
62 changes: 62 additions & 0 deletions tests/foundation/registry/test_foundation_keyword_names.py
Original file line number Diff line number Diff line change
@@ -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()
58 changes: 58 additions & 0 deletions tests/utilities/test_quiet_exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
"""
from __future__ import annotations

import io
import os
import sys
import traceback
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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'):
Expand Down
Loading