Skip to content

Commit 4615c31

Browse files
authored
fix(protocols): drop the warnings of a trial parse that is kept as Raw (#2308) (#2339)
Problem: a held trial parse in _parse_next_layer that raised had its warnings replayed by hold_warnings.__exit__ whatever discard said, and beholder then kept the layer as Raw, so an Ethernet frame cut inside its IPv4 header warned "packet length < 0" for a layer it does not have. Fix: hold_warnings honours discard on an Exception exit too; beholder marks its call in a context variable, and _parse_next_layer discards a raising trial's warnings under it. Outside beholder the error reaches the caller and the warnings are still reported. The Raw's error names the cut. Test: tests/protocols/test_trial_parse_raises_2308_unit.py; the #2236 test pins the cap_len argument directly and asserts the Raw's error; the #1580 tests that pinned the leak are retargeted to kept layers.
1 parent 5df1238 commit 4615c31

7 files changed

Lines changed: 238 additions & 87 deletions

File tree

‎pcapkit/corekit/decorators.py‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
decorators either.
2525
2626
"""
27+
import contextvars
2728
import functools
2829
import io
2930
import os
@@ -53,6 +54,12 @@
5354
#: :data:`pcapkit.utilities.logging.logger`.
5455
logger = get_logger(__name__)
5556

57+
#: contextvars.ContextVar[bool]: Whether a call :func:`beholder` wraps is under
58+
#: way, so that an :exc:`Exception` raised inside it ends with the layer that
59+
#: raised replaced; see :meth:`ProtocolBase._parse_next_layer
60+
#: <pcapkit.protocols.protocol.ProtocolBase._parse_next_layer>` (:issue:`2308`).
61+
_beholding = contextvars.ContextVar('_beholding', default=False)
62+
5663

5764
def seekset(func: 'Callable[Concatenate[ProtocolBase, P], R_seekset]') -> 'Callable[P, R_seekset]':
5865
"""Read file from start then set back to original.
@@ -109,6 +116,12 @@ def beholder(
109116
:class:`~pcapkit.protocols.misc.null.NoPayload`, if the error is an
110117
end-of-file :exc:`~pcapkit.utilities.exceptions.StructError`.
111118
119+
When :meth:`ProtocolBase._parse_next_layer
120+
<pcapkit.protocols.protocol.ProtocolBase._parse_next_layer>` held the
121+
warnings of the layer that raised, they are dropped with it: they describe a
122+
layer the result does not have, and the replacement's ``error`` reports the
123+
failure (:issue:`2308`).
124+
112125
Note:
113126
The decorated function should have the following signature::
114127
@@ -141,7 +154,11 @@ def behold(*args: 'P.args', **kwargs: 'P.kwargs') -> 'R_beholder':
141154

142155
try:
143156
# call method
144-
return func(*args, **kwargs)
157+
token = _beholding.set(True)
158+
try:
159+
return func(*args, **kwargs)
160+
finally:
161+
_beholding.reset(token)
145162
except Exception as exc:
146163
if isinstance(exc, StructError) and exc.eof: # pylint: disable=no-member
147164
from pcapkit.protocols.misc.null import NoPayload as protocol # isort: skip # pylint: disable=import-outside-toplevel

‎pcapkit/protocols/misc/pcap/frame.py‎

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

399397
def make(self,

‎pcapkit/protocols/protocol.py‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@
4444
import aenum
4545

4646
from pcapkit.corekit.context import ContextRegistry
47-
from pcapkit.corekit.decorators import beholder, seekset
47+
from pcapkit.corekit.decorators import _beholding, beholder, seekset
4848
from pcapkit.corekit.fields.misc import ConditionalField, SchemaField, SwitchField
4949
from pcapkit.corekit.module import ModuleDescriptor
5050
from pcapkit.corekit.protochain import ProtoChain
@@ -2352,8 +2352,13 @@ def _parse_next_layer(klass: 'Type[ProtocolBase]', file_: 'bytes', length: 'int'
23522352
an Ethernet frame of 10 octets (:issue:`1580`) -- and reported as the
23532353
parse ends when it is kept, still inside the guard that catches them
23542354
under an ``error`` filter. A layer kept with its header cut short
2355-
still warns. A layer captured at least as long as its hint cannot be
2356-
replaced, and is parsed holding nothing, even inside one that holds.
2355+
still warns. The held warnings of a parse that raises are dropped
2356+
too when :func:`~pcapkit.corekit.decorators.beholder` replaces the
2357+
layer, as it does around :meth:`_import_next_layer`, since the
2358+
replacement's ``error`` reports the failure (:issue:`2308`); outside
2359+
it, they are reported as the error leaves. A layer captured at least
2360+
as long as its hint cannot be replaced, and is parsed holding
2361+
nothing, even inside one that holds.
23572362
23582363
A layer past :data:`~pcapkit.utilities.config.FRAME_LAYER_LIMIT` in its frame is not parsed:
23592364
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'
23872392
token = _frame_depth.set(depth)
23882393
try:
23892394
with hold_warnings(_short_of_hint(klass, captured)) as held:
2390-
next_ = klass(file_, length, **kwargs) # type: ignore[abstract]
2395+
try:
2396+
next_ = klass(file_, length, **kwargs) # type: ignore[abstract]
2397+
except Exception:
2398+
# NOTE: Under ``beholder`` the layer that raised is replaced,
2399+
# so its warnings go with it; outside it, the error reaches
2400+
# the caller and they are reported as it leaves (#2308).
2401+
held.discard = _beholding.get()
2402+
raise
23912403

23922404
length_hint = getattr(next_, '__length_hint__', None)
23932405
hint = None if length_hint is None else length_hint()

‎pcapkit/utilities/warnings.py‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -237,11 +237,11 @@ class hold_warnings: # pylint: disable=invalid-name
237237
Every :func:`warn` call inside the block is held, with the frame it is
238238
attributed to. On leaving the block they are reported in order, with the
239239
same message and category and attributed to the same frame on both
240-
channels, unless :attr:`discard` was set, in which case they are dropped. A
241-
block left by an :exc:`Exception` reports them whatever :attr:`discard`
242-
says; one left by any other :exc:`BaseException` -- :exc:`KeyboardInterrupt`,
243-
:exc:`SystemExit`, :exc:`GeneratorExit` -- drops them, so that a filter
244-
turning them into errors cannot replace it.
240+
channels, unless :attr:`discard` was set, in which case they are dropped,
241+
whether the block ends or is left by an :exc:`Exception`. A block left by any
242+
other :exc:`BaseException` -- :exc:`KeyboardInterrupt`, :exc:`SystemExit`,
243+
:exc:`GeneratorExit` -- drops them whatever :attr:`discard` says, so that a
244+
filter turning them into errors cannot replace it.
245245
246246
The line named is the frame's line when the block is left. For the frame
247247
:func:`~pcapkit.utilities.exceptions.stacklevel` names -- the caller of
@@ -255,8 +255,9 @@ class hold_warnings: # pylint: disable=invalid-name
255255
:meth:`ProtocolBase._parse_next_layer
256256
<pcapkit.protocols.protocol.ProtocolBase._parse_next_layer>` uses this for the
257257
trial parse it replaces with :class:`~pcapkit.protocols.misc.raw.Raw` when the
258-
header was not captured: the warnings describe a layer the result does not
259-
have (:issue:`1580`).
258+
header was not captured, or that :func:`~pcapkit.corekit.decorators.beholder`
259+
replaces when it raises: the warnings describe a layer the result does not
260+
have (:issue:`1580`, :issue:`2308`).
260261
261262
Note:
262263
Nothing here touches ``warnings.filters`` or a module's
@@ -310,9 +311,7 @@ def __exit__(self, exc_type: 'Optional[Type[BaseException]]',
310311
_HELD.set(outer)
311312

312313
held, self._held = self._held, []
313-
if self.discard and exc_type is None:
314-
return
315-
if exc_type is not None and not issubclass(exc_type, Exception):
314+
if self.discard or (exc_type is not None and not issubclass(exc_type, Exception)):
316315
return
317316
for message, category, target in held:
318317
_replay(message, category, target)

‎tests/protocols/misc/pcap/test_frame_read_cap_len_2236_unit.py‎

Lines changed: 27 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,12 @@
33
44
This pins the length argument :meth:`Frame.read
55
<pcapkit.protocols.misc.pcap.frame.Frame.read>` passes to ``_decode_next_layer``
6-
(#2236). The dissector is handed ``frame.cap_len``, the octets present, so a
7-
frame the snapshot length cut inside its IPv4 header reports the truncation as a
8-
:class:`~pcapkit.utilities.warnings.SchemaWarning` ``packet length < 0``. Handed
9-
the on-wire ``frame.len`` instead, the schema believes the octets it lacks are
10-
there and the truncation passes silently.
6+
(#2236): ``frame.cap_len``, the octets present, never the on-wire ``frame.len``.
7+
A frame the snapshot length cut inside its IPv4 header keeps IPv4 as
8+
:class:`~pcapkit.protocols.misc.raw.Raw`, whose ``error`` names the cut, and the
9+
trial parse's ``packet length < 0`` warnings go with the layer (#2308). The
10+
dissection reads the same for either length, so the argument is read where it
11+
is passed.
1112
1213
Every case builds its own octets and reads no capture. Classes are imported
1314
inside each test, after :func:`~tests._support.reimport_once_per_class`.
@@ -17,6 +18,7 @@
1718
import io
1819
import struct
1920
import unittest
21+
import unittest.mock
2022
import warnings
2123

2224
from tests._support import reimport_once_per_class
@@ -42,29 +44,31 @@ class TestFrameReadCapLen(unittest.TestCase):
4244
def setUp(self) -> None:
4345
reimport_once_per_class(self)
4446

45-
def schema_warnings(self, orig_len: 'int') -> 'list[str]':
46-
"""The :class:`SchemaWarning` messages from parsing the record for ``orig_len``."""
47+
def test_truncated_ipv4_header_is_raw_with_its_error(self) -> None:
48+
"""The cut IPv4 header is kept as ``Raw`` naming the cut, with no warning.
49+
50+
IPv4 is handed the 10 octets Ethernet leaves of the captured 24, short
51+
of its 20-octet header, so the trial parse raises and is replaced. Its
52+
warnings, ``packet length < 0`` down to ``-10``, describe the IPv4 layer
53+
the result does not have, and are dropped with it.
54+
"""
4755
from pcapkit.protocols.misc.pcap.frame import Frame
4856
from pcapkit.protocols.misc.pcap.header import Header
57+
from pcapkit.protocols.misc.raw import Raw
4958
from pcapkit.utilities.warnings import SchemaWarning
5059

51-
with warnings.catch_warnings(record=True) as caught:
60+
decode = unittest.mock.patch.object(Frame, '_decode_next_layer', autospec=True,
61+
side_effect=Frame._decode_next_layer) # pylint: disable=protected-access
62+
with warnings.catch_warnings(record=True) as caught, decode as spy:
5263
warnings.simplefilter('always')
53-
frame = Frame(io.BytesIO(_record(orig_len)), num=1, header=Header(GLOBAL).info)
54-
self.assertEqual(frame.info.cap_len, len(PACKET))
55-
self.assertEqual(frame.info.len, orig_len)
56-
return [str(item.message) for item in caught if issubclass(item.category, SchemaWarning)]
57-
58-
def test_truncated_ipv4_header_warns_whatever_the_on_wire_length(self) -> None:
59-
"""The cut IPv4 header warns, the same as for an uncut frame of the same octets.
60-
61-
IPv4 is handed the 10 octets Ethernet leaves of the captured 24, so its
62-
header runs the remaining length to ``-10``. Were the on-wire 154 handed
63-
over instead, IPv4 would be told of 140 octets and warn of nothing.
64-
"""
65-
truncated = self.schema_warnings(ORIG_LEN)
66-
self.assertIn('packet length < 0: -10', truncated)
67-
self.assertEqual(truncated, self.schema_warnings(len(PACKET)))
64+
frame = Frame(io.BytesIO(_record(ORIG_LEN)), num=1, header=Header(GLOBAL).info)
65+
self.assertEqual((frame.info.cap_len, frame.info.len), (len(PACKET), ORIG_LEN))
66+
self.assertEqual(spy.call_args.args[3], len(PACKET))
67+
68+
ipv4 = frame.payload.payload
69+
self.assertIsInstance(ipv4, Raw)
70+
self.assertEqual(ipv4.info.error, 'IPv4: header length 20 runs past the end of the data (10 octets)')
71+
self.assertEqual([str(item.message) for item in caught if issubclass(item.category, SchemaWarning)], [])
6872

6973

7074
if __name__ == '__main__':
Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
# -*- coding: utf-8 -*-
2+
"""GitHub issue #2308: a trial parse that raises and is kept as ``Raw`` warns nothing.
3+
4+
:meth:`~pcapkit.protocols.protocol.ProtocolBase._parse_next_layer` holds the
5+
warnings of a next layer captured shorter than its length hint (#1580). When
6+
that parse raises, :func:`~pcapkit.corekit.decorators.beholder` replaces the
7+
layer, and without the fix the held warnings were reported anyway: an Ethernet
8+
frame cut inside its IPv4 header warned ``packet length < 0`` three times for an
9+
IPv4 layer the result keeps as ``Raw``. They are dropped with the layer, whose
10+
``error`` names the cut. A held layer that is kept still warns, and so does one
11+
that raises where no :func:`~pcapkit.corekit.decorators.beholder` replaces it.
12+
13+
:mod:`pcapkit` is imported inside each test, after
14+
:func:`~tests._support.reimport_once_per_class`.
15+
16+
"""
17+
18+
import re
19+
import unittest
20+
import warnings
21+
22+
from tests._support import reimport_once_per_class
23+
from tests.utilities._harness import capture
24+
25+
#: Test tier (#1958): exercises the warning machinery, which free-threaded
26+
#: builds make context-aware.
27+
__tier__ = 'ALL'
28+
29+
#: The first 10 of the 20 octets of an IPv4 header.
30+
IPV4_CUT = bytes.fromhex('4500008c000100004011')
31+
#: An Ethernet frame carrying :data:`IPV4_CUT`.
32+
FRAME = bytes(6) + bytes(range(1, 7)) + b'\x08\x00' + IPV4_CUT
33+
#: The ``error`` of the ``Raw`` that :data:`FRAME`'s IPv4 is kept as.
34+
ERROR = 'IPv4: header length 20 runs past the end of the data (10 octets)'
35+
36+
37+
def _ipv6(next_header: 'int') -> 'bytes':
38+
"""An Ethernet frame carrying IPv6 whose ``next_header`` is cut to 1 octet."""
39+
return (bytes(6) + bytes(range(1, 7)) + b'\x86\xdd'
40+
+ bytes.fromhex('600000000001') + bytes([next_header, 64])
41+
+ bytes(15) + b'\x01' + bytes(15) + b'\x02' + b'\x00')
42+
43+
44+
#: The warnings of the kept, cut ``IPv6_Ext`` that each :func:`_ipv6` frame ends in.
45+
IPV6_EXT_CUT = ['packet length < 0: -1',
46+
'IPv6: extension header declares a length of 8 octet(s) with 1 octet(s) '
47+
'left in the chain; stopping the walk instead of skipping past it']
48+
49+
50+
class TestTrialParseRaises(unittest.TestCase):
51+
"""Pin that a raising trial parse replaced by ``beholder`` reports nothing."""
52+
53+
def setUp(self) -> None:
54+
reimport_once_per_class(self)
55+
56+
def _parse(self, frame: 'bytes', action: 'str' = 'always') -> 'tuple[object, list[str]]':
57+
"""Parse ``frame`` as Ethernet, with the :class:`SchemaWarning` messages it reports."""
58+
from pcapkit.protocols.link.ethernet import Ethernet
59+
from pcapkit.utilities.warnings import SchemaWarning
60+
61+
with warnings.catch_warnings(record=True) as caught:
62+
warnings.simplefilter(action)
63+
parsed = Ethernet(frame, len(frame))
64+
return parsed, [str(item.message) for item in caught if issubclass(item.category, SchemaWarning)]
65+
66+
def test_cut_ipv4_header_is_raw_and_warns_nothing(self) -> None:
67+
"""IPv4 cut to 10 octets raises, is kept as ``Raw`` naming the cut, and reports nothing."""
68+
from pcapkit.protocols.misc.raw import Raw
69+
from pcapkit.utilities.logging import logger
70+
71+
with capture(logger) as recorder:
72+
parsed, messages = self._parse(FRAME)
73+
ipv4 = parsed.payload # type: ignore[attr-defined]
74+
self.assertIsInstance(ipv4, Raw)
75+
self.assertEqual(ipv4.info.error, ERROR)
76+
self.assertEqual(messages, [])
77+
self.assertEqual([record.getMessage() for record in recorder.records
78+
if record.getMessage().startswith('packet length')], [])
79+
80+
def test_error_filter_keeps_the_cut_as_the_error(self) -> None:
81+
"""Under ``error``, the ``Raw`` names the cut header, not a warning it dropped."""
82+
from pcapkit.protocols.misc.raw import Raw
83+
84+
parsed, messages = self._parse(FRAME, 'error')
85+
self.assertIsInstance(parsed.payload, Raw) # type: ignore[attr-defined]
86+
self.assertEqual(parsed.payload.info.error, ERROR) # type: ignore[attr-defined]
87+
self.assertEqual(messages, [])
88+
89+
def test_replaced_by_ipv6_ext_reports_only_the_replacement(self) -> None:
90+
"""A cut Hop-by-Hop header raises and is parsed as ``IPv6_Ext``, which alone warns."""
91+
parsed, messages = self._parse(_ipv6(0))
92+
self.assertEqual(str(parsed.protochain), 'Ethernet:IPv6:IPv6-Ext') # type: ignore[attr-defined]
93+
self.assertEqual(messages, IPV6_EXT_CUT)
94+
95+
def test_kept_trial_still_warns(self) -> None:
96+
"""A cut Shim6 header is held, kept, and reports its warnings as before."""
97+
parsed, messages = self._parse(_ipv6(140))
98+
self.assertEqual(str(parsed.protochain), 'Ethernet:IPv6:IPv6-Ext') # type: ignore[attr-defined]
99+
self.assertEqual(messages, IPV6_EXT_CUT)
100+
101+
def test_raise_outside_beholder_reports(self) -> None:
102+
"""With no ``beholder`` to replace it, the layer's error and its warnings both reach the caller."""
103+
from pcapkit.protocols.internet.ipv4 import IPv4
104+
from pcapkit.protocols.protocol import ProtocolBase
105+
from pcapkit.utilities.exceptions import ProtocolError
106+
from pcapkit.utilities.warnings import SchemaWarning
107+
108+
with warnings.catch_warnings(record=True) as caught:
109+
warnings.simplefilter('always')
110+
with self.assertRaisesRegex(ProtocolError, re.escape(ERROR)):
111+
ProtocolBase._parse_next_layer(IPv4, IPV4_CUT, len(IPV4_CUT)) # pylint: disable=protected-access
112+
self.assertEqual([str(item.message) for item in caught if issubclass(item.category, SchemaWarning)],
113+
['packet length < 0: -2', 'packet length < 0: -6', 'packet length < 0: -10',
114+
'packet length < 0: -10'])
115+
116+
117+
if __name__ == '__main__':
118+
unittest.main()

0 commit comments

Comments
 (0)