fix(protocols): drop the warnings of a trial parse that is kept as Raw (#2308)

pcapkit/corekit/decorators.py

@@ -24,6 +24,7 @@
    decorators either.
 
 """
+import contextvars
 import functools
 import io
 import os
@@ -53,6 +54,12 @@
 #: :data:`pcapkit.utilities.logging.logger`.
 logger = get_logger(__name__)
 
+#: contextvars.ContextVar[bool]: Whether a call :func:`beholder` wraps is under
+#: way, so that an :exc:`Exception` raised inside it ends with the layer that
+#: raised replaced; see :meth:`ProtocolBase._parse_next_layer
+#: <pcapkit.protocols.protocol.ProtocolBase._parse_next_layer>` (:issue:`2308`).
+_beholding = contextvars.ContextVar('_beholding', default=False)
+
 
 def seekset(func: 'Callable[Concatenate[ProtocolBase, P], R_seekset]') -> 'Callable[P, R_seekset]':
     """Read file from start then set back to original.
@@ -109,6 +116,12 @@ def beholder(
     :class:`~pcapkit.protocols.misc.null.NoPayload`, if the error is an
     end-of-file :exc:`~pcapkit.utilities.exceptions.StructError`.
 
+    When :meth:`ProtocolBase._parse_next_layer
+    <pcapkit.protocols.protocol.ProtocolBase._parse_next_layer>` held the
+    warnings of the layer that raised, they are dropped with it: they describe a
+    layer the result does not have, and the replacement's ``error`` reports the
+    failure (:issue:`2308`).
+
     Note:
         The decorated function should have the following signature::
 
@@ -141,7 +154,11 @@ def behold(*args: 'P.args', **kwargs: 'P.kwargs') -> 'R_beholder':
 
         try:
             # call method
-            return func(*args, **kwargs)
+            token = _beholding.set(True)
+            try:
+                return func(*args, **kwargs)
+            finally:
+                _beholding.reset(token)
         except Exception as exc:
             if isinstance(exc, StructError) and exc.eof:  # pylint: disable=no-member
                 from pcapkit.protocols.misc.null import NoPayload as protocol  # isort: skip # pylint: disable=import-outside-toplevel

pcapkit/protocols/misc/pcap/frame.py

@@ -386,14 +386,12 @@ def read(self, length: 'Optional[int]' = None, *, _read: 'bool' = True,
         # snapshot length cut short the latter exceeds what the file holds, and
         # handing it over is the declared-length-exceeds-available-octets fault.
         #
-        # The schemas count their fields down from this length, so it is what
-        # lets a truncation be noticed: a frame cut inside its IPv4 header
-        # leaves IPv4 fewer octets than its header, and the count running below
-        # zero issues a ``SchemaWarning`` (``packet length < 0``). Handed the
-        # on-wire length, IPv4 would be told of octets the file does not hold
-        # and the truncation would pass silently.
-        # ``test_truncated_ipv4_header_warns_whatever_the_on_wire_length`` pins it
-        # (#2236).
+        # A frame cut inside its IPv4 header keeps IPv4 as ``Raw``, whose
+        # ``error`` names the cut, for either length: the trial parse of IPv4
+        # raises, and its ``packet length < 0`` warnings go with the layer
+        # (#2308). As the dissection does not tell the two apart,
+        # ``test_truncated_ipv4_header_is_raw_with_its_error`` pins the argument
+        # where it is passed (#2236).
         return self._decode_next_layer(frame, self._ghdr.network, frame.cap_len)
 
     def make(self,

pcapkit/protocols/protocol.py

@@ -44,7 +44,7 @@
 import aenum
 
 from pcapkit.corekit.context import ContextRegistry
-from pcapkit.corekit.decorators import beholder, seekset
+from pcapkit.corekit.decorators import _beholding, beholder, seekset
 from pcapkit.corekit.fields.misc import ConditionalField, SchemaField, SwitchField
 from pcapkit.corekit.module import ModuleDescriptor
 from pcapkit.corekit.protochain import ProtoChain
@@ -2352,8 +2352,13 @@ def _parse_next_layer(klass: 'Type[ProtocolBase]', file_: 'bytes', length: 'int'
             an Ethernet frame of 10 octets (:issue:`1580`) -- and reported as the
             parse ends when it is kept, still inside the guard that catches them
             under an ``error`` filter. A layer kept with its header cut short
-            still warns. A layer captured at least as long as its hint cannot be
-            replaced, and is parsed holding nothing, even inside one that holds.
+            still warns. The held warnings of a parse that raises are dropped
+            too when :func:`~pcapkit.corekit.decorators.beholder` replaces the
+            layer, as it does around :meth:`_import_next_layer`, since the
+            replacement's ``error`` reports the failure (:issue:`2308`); outside
+            it, they are reported as the error leaves. A layer captured at least
+            as long as its hint cannot be replaced, and is parsed holding
+            nothing, even inside one that holds.
 
             A layer past :data:`~pcapkit.utilities.config.FRAME_LAYER_LIMIT` in its frame is not parsed:
             it and the rest of the frame are kept as
@@ -2387,7 +2392,14 @@ def _parse_next_layer(klass: 'Type[ProtocolBase]', file_: 'bytes', length: 'int'
         token = _frame_depth.set(depth)
         try:
             with hold_warnings(_short_of_hint(klass, captured)) as held:
-                next_ = klass(file_, length, **kwargs)  # type: ignore[abstract]
+                try:
+                    next_ = klass(file_, length, **kwargs)  # type: ignore[abstract]
+                except Exception:
+                    # NOTE: Under ``beholder`` the layer that raised is replaced,
+                    # so its warnings go with it; outside it, the error reaches
+                    # the caller and they are reported as it leaves (#2308).
+                    held.discard = _beholding.get()
+                    raise
 
                 length_hint = getattr(next_, '__length_hint__', None)
                 hint = None if length_hint is None else length_hint()

pcapkit/utilities/warnings.py

@@ -237,11 +237,11 @@ class hold_warnings:  # pylint: disable=invalid-name
     Every :func:`warn` call inside the block is held, with the frame it is
     attributed to. On leaving the block they are reported in order, with the
     same message and category and attributed to the same frame on both
-    channels, unless :attr:`discard` was set, in which case they are dropped. A
-    block left by an :exc:`Exception` reports them whatever :attr:`discard`
-    says; one left by any other :exc:`BaseException` -- :exc:`KeyboardInterrupt`,
-    :exc:`SystemExit`, :exc:`GeneratorExit` -- drops them, so that a filter
-    turning them into errors cannot replace it.
+    channels, unless :attr:`discard` was set, in which case they are dropped,
+    whether the block ends or is left by an :exc:`Exception`. A block left by any
+    other :exc:`BaseException` -- :exc:`KeyboardInterrupt`, :exc:`SystemExit`,
+    :exc:`GeneratorExit` -- drops them whatever :attr:`discard` says, so that a
+    filter turning them into errors cannot replace it.
 
     The line named is the frame's line when the block is left. For the frame
     :func:`~pcapkit.utilities.exceptions.stacklevel` names -- the caller of
@@ -255,8 +255,9 @@ class hold_warnings:  # pylint: disable=invalid-name
     :meth:`ProtocolBase._parse_next_layer
     <pcapkit.protocols.protocol.ProtocolBase._parse_next_layer>` uses this for the
     trial parse it replaces with :class:`~pcapkit.protocols.misc.raw.Raw` when the
-    header was not captured: the warnings describe a layer the result does not
-    have (:issue:`1580`).
+    header was not captured, or that :func:`~pcapkit.corekit.decorators.beholder`
+    replaces when it raises: the warnings describe a layer the result does not
+    have (:issue:`1580`, :issue:`2308`).
 
     Note:
         Nothing here touches ``warnings.filters`` or a module's
@@ -310,9 +311,7 @@ def __exit__(self, exc_type: 'Optional[Type[BaseException]]',
                 _HELD.set(outer)
 
         held, self._held = self._held, []
-        if self.discard and exc_type is None:
-            return
-        if exc_type is not None and not issubclass(exc_type, Exception):
+        if self.discard or (exc_type is not None and not issubclass(exc_type, Exception)):
             return
         for message, category, target in held:
             _replay(message, category, target)

tests/protocols/misc/pcap/test_frame_read_cap_len_2236_unit.py

@@ -3,11 +3,12 @@
 
 This pins the length argument :meth:`Frame.read
 <pcapkit.protocols.misc.pcap.frame.Frame.read>` passes to ``_decode_next_layer``
-(#2236). The dissector is handed ``frame.cap_len``, the octets present, so a
-frame the snapshot length cut inside its IPv4 header reports the truncation as a
-:class:`~pcapkit.utilities.warnings.SchemaWarning` ``packet length < 0``. Handed
-the on-wire ``frame.len`` instead, the schema believes the octets it lacks are
-there and the truncation passes silently.
+(#2236): ``frame.cap_len``, the octets present, never the on-wire ``frame.len``.
+A frame the snapshot length cut inside its IPv4 header keeps IPv4 as
+:class:`~pcapkit.protocols.misc.raw.Raw`, whose ``error`` names the cut, and the
+trial parse's ``packet length < 0`` warnings go with the layer (#2308). The
+dissection reads the same for either length, so the argument is read where it
+is passed.
 
 Every case builds its own octets and reads no capture. Classes are imported
 inside each test, after :func:`~tests._support.reimport_once_per_class`.
@@ -17,6 +18,7 @@
 import io
 import struct
 import unittest
+import unittest.mock
 import warnings
 
 from tests._support import reimport_once_per_class
@@ -42,29 +44,31 @@ class TestFrameReadCapLen(unittest.TestCase):
     def setUp(self) -> None:
         reimport_once_per_class(self)
 
-    def schema_warnings(self, orig_len: 'int') -> 'list[str]':
-        """The :class:`SchemaWarning` messages from parsing the record for ``orig_len``."""
+    def test_truncated_ipv4_header_is_raw_with_its_error(self) -> None:
+        """The cut IPv4 header is kept as ``Raw`` naming the cut, with no warning.
+
+        IPv4 is handed the 10 octets Ethernet leaves of the captured 24, short
+        of its 20-octet header, so the trial parse raises and is replaced. Its
+        warnings, ``packet length < 0`` down to ``-10``, describe the IPv4 layer
+        the result does not have, and are dropped with it.
+        """
         from pcapkit.protocols.misc.pcap.frame import Frame
         from pcapkit.protocols.misc.pcap.header import Header
+        from pcapkit.protocols.misc.raw import Raw
         from pcapkit.utilities.warnings import SchemaWarning
 
-        with warnings.catch_warnings(record=True) as caught:
+        decode = unittest.mock.patch.object(Frame, '_decode_next_layer', autospec=True,
+                                            side_effect=Frame._decode_next_layer)  # pylint: disable=protected-access
+        with warnings.catch_warnings(record=True) as caught, decode as spy:
             warnings.simplefilter('always')
-            frame = Frame(io.BytesIO(_record(orig_len)), num=1, header=Header(GLOBAL).info)
-        self.assertEqual(frame.info.cap_len, len(PACKET))
-        self.assertEqual(frame.info.len, orig_len)
-        return [str(item.message) for item in caught if issubclass(item.category, SchemaWarning)]
-
-    def test_truncated_ipv4_header_warns_whatever_the_on_wire_length(self) -> None:
-        """The cut IPv4 header warns, the same as for an uncut frame of the same octets.
-
-        IPv4 is handed the 10 octets Ethernet leaves of the captured 24, so its
-        header runs the remaining length to ``-10``. Were the on-wire 154 handed
-        over instead, IPv4 would be told of 140 octets and warn of nothing.
-        """
-        truncated = self.schema_warnings(ORIG_LEN)
-        self.assertIn('packet length < 0: -10', truncated)
-        self.assertEqual(truncated, self.schema_warnings(len(PACKET)))
+            frame = Frame(io.BytesIO(_record(ORIG_LEN)), num=1, header=Header(GLOBAL).info)
+        self.assertEqual((frame.info.cap_len, frame.info.len), (len(PACKET), ORIG_LEN))
+        self.assertEqual(spy.call_args.args[3], len(PACKET))
+
+        ipv4 = frame.payload.payload
+        self.assertIsInstance(ipv4, Raw)
+        self.assertEqual(ipv4.info.error, 'IPv4: header length 20 runs past the end of the data (10 octets)')
+        self.assertEqual([str(item.message) for item in caught if issubclass(item.category, SchemaWarning)], [])
 
 
 if __name__ == '__main__':

tests/protocols/test_trial_parse_raises_2308_unit.py

@@ -0,0 +1,118 @@
+# -*- coding: utf-8 -*-
+"""GitHub issue #2308: a trial parse that raises and is kept as ``Raw`` warns nothing.
+
+:meth:`~pcapkit.protocols.protocol.ProtocolBase._parse_next_layer` holds the
+warnings of a next layer captured shorter than its length hint (#1580). When
+that parse raises, :func:`~pcapkit.corekit.decorators.beholder` replaces the
+layer, and without the fix the held warnings were reported anyway: an Ethernet
+frame cut inside its IPv4 header warned ``packet length < 0`` three times for an
+IPv4 layer the result keeps as ``Raw``. They are dropped with the layer, whose
+``error`` names the cut. A held layer that is kept still warns, and so does one
+that raises where no :func:`~pcapkit.corekit.decorators.beholder` replaces it.
+
+:mod:`pcapkit` is imported inside each test, after
+:func:`~tests._support.reimport_once_per_class`.
+
+"""
+
+import re
+import unittest
+import warnings
+
+from tests._support import reimport_once_per_class
+from tests.utilities._harness import capture
+
+#: Test tier (#1958): exercises the warning machinery, which free-threaded
+#: builds make context-aware.
+__tier__ = 'ALL'
+
+#: The first 10 of the 20 octets of an IPv4 header.
+IPV4_CUT = bytes.fromhex('4500008c000100004011')
+#: An Ethernet frame carrying :data:`IPV4_CUT`.
+FRAME = bytes(6) + bytes(range(1, 7)) + b'\x08\x00' + IPV4_CUT
+#: The ``error`` of the ``Raw`` that :data:`FRAME`'s IPv4 is kept as.
+ERROR = 'IPv4: header length 20 runs past the end of the data (10 octets)'
+
+
+def _ipv6(next_header: 'int') -> 'bytes':
+    """An Ethernet frame carrying IPv6 whose ``next_header`` is cut to 1 octet."""
+    return (bytes(6) + bytes(range(1, 7)) + b'\x86\xdd'
+            + bytes.fromhex('600000000001') + bytes([next_header, 64])
+            + bytes(15) + b'\x01' + bytes(15) + b'\x02' + b'\x00')
+
+
+#: The warnings of the kept, cut ``IPv6_Ext`` that each :func:`_ipv6` frame ends in.
+IPV6_EXT_CUT = ['packet length < 0: -1',
+                'IPv6: extension header declares a length of 8 octet(s) with 1 octet(s) '
+                'left in the chain; stopping the walk instead of skipping past it']
+
+
+class TestTrialParseRaises(unittest.TestCase):
+    """Pin that a raising trial parse replaced by ``beholder`` reports nothing."""
+
+    def setUp(self) -> None:
+        reimport_once_per_class(self)
+
+    def _parse(self, frame: 'bytes', action: 'str' = 'always') -> 'tuple[object, list[str]]':
+        """Parse ``frame`` as Ethernet, with the :class:`SchemaWarning` messages it reports."""
+        from pcapkit.protocols.link.ethernet import Ethernet
+        from pcapkit.utilities.warnings import SchemaWarning
+
+        with warnings.catch_warnings(record=True) as caught:
+            warnings.simplefilter(action)
+            parsed = Ethernet(frame, len(frame))
+        return parsed, [str(item.message) for item in caught if issubclass(item.category, SchemaWarning)]
+
+    def test_cut_ipv4_header_is_raw_and_warns_nothing(self) -> None:
+        """IPv4 cut to 10 octets raises, is kept as ``Raw`` naming the cut, and reports nothing."""
+        from pcapkit.protocols.misc.raw import Raw
+        from pcapkit.utilities.logging import logger
+
+        with capture(logger) as recorder:
+            parsed, messages = self._parse(FRAME)
+        ipv4 = parsed.payload  # type: ignore[attr-defined]
+        self.assertIsInstance(ipv4, Raw)
+        self.assertEqual(ipv4.info.error, ERROR)
+        self.assertEqual(messages, [])
+        self.assertEqual([record.getMessage() for record in recorder.records
+                          if record.getMessage().startswith('packet length')], [])
+
+    def test_error_filter_keeps_the_cut_as_the_error(self) -> None:
+        """Under ``error``, the ``Raw`` names the cut header, not a warning it dropped."""
+        from pcapkit.protocols.misc.raw import Raw
+
+        parsed, messages = self._parse(FRAME, 'error')
+        self.assertIsInstance(parsed.payload, Raw)  # type: ignore[attr-defined]
+        self.assertEqual(parsed.payload.info.error, ERROR)  # type: ignore[attr-defined]
+        self.assertEqual(messages, [])
+
+    def test_replaced_by_ipv6_ext_reports_only_the_replacement(self) -> None:
+        """A cut Hop-by-Hop header raises and is parsed as ``IPv6_Ext``, which alone warns."""
+        parsed, messages = self._parse(_ipv6(0))
+        self.assertEqual(str(parsed.protochain), 'Ethernet:IPv6:IPv6-Ext')  # type: ignore[attr-defined]
+        self.assertEqual(messages, IPV6_EXT_CUT)
+
+    def test_kept_trial_still_warns(self) -> None:
+        """A cut Shim6 header is held, kept, and reports its warnings as before."""
+        parsed, messages = self._parse(_ipv6(140))
+        self.assertEqual(str(parsed.protochain), 'Ethernet:IPv6:IPv6-Ext')  # type: ignore[attr-defined]
+        self.assertEqual(messages, IPV6_EXT_CUT)
+
+    def test_raise_outside_beholder_reports(self) -> None:
+        """With no ``beholder`` to replace it, the layer's error and its warnings both reach the caller."""
+        from pcapkit.protocols.internet.ipv4 import IPv4
+        from pcapkit.protocols.protocol import ProtocolBase
+        from pcapkit.utilities.exceptions import ProtocolError
+        from pcapkit.utilities.warnings import SchemaWarning
+
+        with warnings.catch_warnings(record=True) as caught:
+            warnings.simplefilter('always')
+            with self.assertRaisesRegex(ProtocolError, re.escape(ERROR)):
+                ProtocolBase._parse_next_layer(IPv4, IPV4_CUT, len(IPV4_CUT))  # pylint: disable=protected-access
+        self.assertEqual([str(item.message) for item in caught if issubclass(item.category, SchemaWarning)],
+                         ['packet length < 0: -2', 'packet length < 0: -6', 'packet length < 0: -10',
+                          'packet length < 0: -10'])
+
+
+if __name__ == '__main__':
+    unittest.main()