diff --git a/docs/source/pcapkit/utilities/exceptions.rst b/docs/source/pcapkit/utilities/exceptions.rst index 859d9471e1..117e999eb7 100644 --- a/docs/source/pcapkit/utilities/exceptions.rst +++ b/docs/source/pcapkit/utilities/exceptions.rst @@ -234,6 +234,13 @@ It is still an ordinary exception carrying its message, so ``except`` clauses an :no-members: :show-inheritance: +:exc:`EOFError` Category +------------------------ + +.. autoexception:: pcapkit.utilities.exceptions.StreamEOFError + :no-members: + :show-inheritance: + :exc:`KeyError` Category ------------------------ diff --git a/pcapkit/utilities/decorators.py b/pcapkit/utilities/decorators.py index f323862c7f..d696e81216 100644 --- a/pcapkit/utilities/decorators.py +++ b/pcapkit/utilities/decorators.py @@ -17,7 +17,7 @@ import traceback from typing import TYPE_CHECKING, cast -from pcapkit.utilities.exceptions import StructError, stacklevel +from pcapkit.utilities.exceptions import StreamEOFError, StructError, stacklevel from pcapkit.utilities.logging import DEVMODE, VERBOSE, get_logger if TYPE_CHECKING: @@ -192,9 +192,22 @@ def prepare(func: 'Callable[Concatenate[Type[R_prepare], bytes | IO[bytes], Opti func(cls: 'typing.Type[pcapkit.protocols.schema.schema.Schema]', data: 'bytes | typing.IO[bytes]', - length: 'Optional[int], - packet: 'Optional[dict[str, Any]', - *args: 'typing.Any', **kwargs: 'Any') -> 'pcapkit.protocols.schema.schema.Schema' + length: 'Optional[int]', + packet: 'Optional[dict[str, Any]]') -> 'pcapkit.protocols.schema.schema.Schema' + + No further positional or keyword arguments are read from -- or + forwarded to -- the decorated function. :func:`prepare` is applied to + exactly one function in this tree, + :meth:`Schema.unpack `, + whose real signature has never had more than these four parameters, + and nothing calls it with more; an earlier revision of this note + nonetheless promised implementors a trailing ``*args, **kwargs``, which + the wrapper below never populated. A caller relying on that promise + got extras silently discarded instead of forwarded -- see `#454 + `__ -- so the + wrapper now raises :exc:`TypeError` for a fifth positional argument or + an unconsumed keyword, the same as an ordinary call with too many + arguments would. See Also: :meth:`pcapkit.protocols.schema.schema.Schema.unpack` @@ -215,6 +228,30 @@ def unpack(*args: 'P.args', **kwargs: 'P.kwargs') -> 'R_prepare': length = cast('Optional[int]', args[2] if len(args) > 2 else kwargs.pop('length', None)) packet = cast('Optional[dict[str, Any]]', args[3] if len(args) > 3 else kwargs.pop('packet', None)) + # NOTE: The decorated function's real signature is exactly the four + # parameters above -- see #454. Anything left over here is therefore + # unwanted rather than something to forward: a fifth positional + # argument, or a keyword that ``kwargs.pop`` above never touched + # because ``length``/``packet`` arrived positionally instead. The + # latter is also what catches ``length`` (or ``packet``) supplied + # *both* positionally and by keyword -- the positional value wins + # above and the keyword is left in ``kwargs`` unconsumed, so it + # surfaces here rather than silently losing the keyword's value. + extra_args = args[4:] + if extra_args or kwargs: + culprits = ', '.join([repr(arg) for arg in extra_args] + + [f'{name}={value!r}' for name, value in kwargs.items()]) + raise TypeError(f'{func.__qualname__}() got unexpected argument(s): {culprits}') + + # Whether the caller told us exactly how much there is to read, even + # if that is zero -- e.g. a nested schema sized by a ``length`` field + # that evaluates to zero, or an otherwise genuinely empty schema -- as + # opposed to leaving ``length`` to be derived from what is actually + # left in ``data``. Only a *derived* zero means the underlying stream + # itself is exhausted; a *declared* zero means this schema legitimately + # has nothing to read. See #458. + declared_length = length is not None + if isinstance(data, bytes): length = len(data) if length is None else length data = io.BytesIO(data) @@ -224,8 +261,13 @@ def unpack(*args: 'P.args', **kwargs: 'P.kwargs') -> 'R_prepare': length = data.seek(0, io.SEEK_END) - current data.seek(current) - if length == 0: - raise EOFError + if length == 0 and not declared_length: + # Quiet: this is the frame reader's ordinary "no more packets" + # signal, caught as such by + # ``pcapkit.foundation.extraction.Extractor`` and friends -- c.f. + # ``pcapkit.protocols.protocol.ProtocolBase._read_unpack``'s + # ``StructError(..., quiet=True, eof=True)`` for the same pattern. + raise StreamEOFError('prepare: end of stream', quiet=True) if packet is None: packet = {} diff --git a/pcapkit/utilities/exceptions.py b/pcapkit/utilities/exceptions.py index 89ef29f179..7e032130cf 100644 --- a/pcapkit/utilities/exceptions.py +++ b/pcapkit/utilities/exceptions.py @@ -47,6 +47,7 @@ 'FieldValueError', 'SchemaError', 'SeekError', 'TruncateError', # ValueError 'ProtocolNotImplemented', 'VendorNotImplemented', # NotImplementedError 'StructError', # struct.error + 'StreamEOFError', # EOFError 'MissingKeyError', 'FragmentError', 'PacketError', # KeyError 'ModuleNotFound', # ModuleNotFoundError ] @@ -411,6 +412,28 @@ def __init__(self, *args: 'Any', eof: 'bool' = False, **kwargs: 'Any') -> 'None' super().__init__(*args, **kwargs) +############################################################################## +# EOFError session. +############################################################################## + + +class StreamEOFError(BaseError, EOFError): + """Underlying stream exhausted; no data left to read. + + Raised by :func:`~pcapkit.utilities.decorators.prepare` when the *length* + of a schema's read was derived by measuring what is actually left in the + stream -- rather than declared by the caller -- and that measurement came + back zero. This is the frame reader's "no more packets" signal, so it + subclasses :exc:`EOFError` rather than replacing it: existing ``except + (EOFError, StopIteration)`` handlers keep working unchanged, and a caller + that wants to be more specific can catch this instead. + + A *declared* zero length -- a nested schema legitimately sized to have + nothing to read -- is a different situation and does not raise this. + + """ + + ############################################################################## # KeyError session. ############################################################################## diff --git a/tests/utilities/test_decorators.py b/tests/utilities/test_decorators.py index 1065768ff4..c3bfe707d5 100644 --- a/tests/utilities/test_decorators.py +++ b/tests/utilities/test_decorators.py @@ -146,6 +146,43 @@ def test_prepare_accepts_keyword_length_and_packet(self) -> None: DemoSchema = self._demo_schema_for_call_shapes() self._assert_call_shape_result(DemoSchema.unpack(b'payload', length=7, packet={})) + def test_prepare_raises_typeerror_for_extra_positional_and_keyword_arguments(self) -> None: + """Extras used to vanish silently instead of being forwarded; see #454. + + ``prepare``'s own docstring promised the decorated function receives + ``*args, **kwargs``, but the wrapper never populated either -- + reproduced from the issue:: + + Probe.unpack(b'\\x07\\x08', 2, {}, 'EXTRA_POSITIONAL', extra_kw='EXTRA_KW') + -> a=7 b=8 # both extras silently gone, no error + + The chosen fix removes the promise (nothing in the tree ever passed + extras, and ``@prepare`` decorates exactly one function, whose real + signature never had room for them) and rejects extras instead of + forwarding them, so a misspelled or unsupported argument is a + ``TypeError`` rather than a parse that silently ignored it. + + """ + DemoSchema = self._demo_schema_for_call_shapes() + + with self.assertRaises(TypeError): + DemoSchema.unpack(b'payload', 7, {}, 'EXTRA_POSITIONAL', extra_kw='EXTRA_KW') + + def test_prepare_raises_typeerror_for_length_given_both_positionally_and_by_keyword(self) -> None: + """The narrower case #454 calls out. + + Passing ``length`` both positionally and by keyword used to silently + keep the positional value and drop the keyword one -- the keyword + never reached ``kwargs.pop``, since that branch only runs when the + positional slot was *not* supplied. It is now a caller error like any + other unconsumed argument. + + """ + DemoSchema = self._demo_schema_for_call_shapes() + + with self.assertRaises(TypeError): + DemoSchema.unpack(b'payload', 7, {}, length=2) + def test_prepare_raises_eof_for_empty_payloads(self) -> None: class DemoSchema: @classmethod @@ -163,6 +200,94 @@ def unpack(cls, data, length=None, packet=None): with self.assertRaises(EOFError): DemoSchema.unpack(b'', None, None) + def test_prepare_raises_stream_eof_error_not_a_bare_eof_error(self) -> None: + """The genuinely-exhausted case now raises an in-library exception. + + A caller could not previously tell a truncated capture apart from any + other ``EOFError``. ``StreamEOFError`` still *is* an ``EOFError`` -- + so ``pcapkit.foundation.extraction.Extractor``'s existing ``except + (EOFError, StopIteration)`` keeps working unchanged -- but it can now + be caught specifically via + :class:`pcapkit.utilities.exceptions.StreamEOFError`. + + """ + 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() + + with self.assertRaises(self.exceptions.StreamEOFError): + DemoSchema.unpack(b'', None, None) + + def test_prepare_still_raises_eof_for_a_truncated_stream(self) -> None: + """A file-like stream already exhausted, with no declared length, is + the frame reader's genuine end-of-capture case and must still raise -- + confirming the #458 fix only changes the *declared*-zero case below, + not this one. + + """ + 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() + + stream = io.BytesIO(b'') + with self.assertRaises(self.exceptions.StreamEOFError): + DemoSchema.unpack(stream, None, None) + + def test_prepare_accepts_a_declared_zero_length_schema(self) -> None: + """A nested, or otherwise genuinely empty, schema sized zero *by the + caller* must unpack rather than raise. Reproduced from #458:: + + @schema_final + class Empty(Schema): + pass + Empty.unpack(b'', 0, {}) + -> EOFError: EOFError() # before the fix + + Distinguishing this from the truncated-stream case above is what + ``length is not None`` (a *declared* length, even zero) checks for. + + """ + class DemoSchema: + @classmethod + def pre_unpack(cls, packet: dict[str, object]) -> None: + packet['prepped'] = True + + def __init__(self, data: bytes) -> None: + self.data = data + + def post_process(self, packet: dict[str, object]) -> dict[str, object]: + packet['data'] = self.data + return packet + + @classmethod + @self.decorators.prepare + def unpack(cls, data, length=None, packet=None): + return cls(data.read()) + + packet = DemoSchema.unpack(b'', 0, {}) + + self.assertEqual(packet['__length__'], 0) + self.assertTrue(packet['prepped']) + self.assertEqual(packet['data'], b'') + def test_prepare_leaves_the_schema_clean_after_post_process(self) -> None: """``post_process``'s revisions must not mark the schema as needing a re-pack.