Skip to content

Commit 453e388

Browse files
authored
fix(corekit): read a counted list's cut item as a short read, not a negative length (#2749) (#2755)
1 parent 1b4f970 commit 453e388

4 files changed

Lines changed: 421 additions & 5 deletions

File tree

‎pcapkit/corekit/fields/collections.py‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -87,11 +87,15 @@ class ListField(FieldBase[List[_TL]], Generic[_TL]):
8787
A list whose octets hold fewer items than ``count`` ended with the data
8888
before the rest, and reports them as :attr:`shortfall`, so that
8989
:meth:`Schema.unpack <pcapkit.protocols.schema.Schema.unpack>`
90-
records the cut; the items absent are not made up.
90+
records the cut; the items absent are not made up. A schema item it counts
91+
is read with ``__data_end__`` in its packet, since its octets end where the
92+
data does, so an item the data ends inside records the cut as its short
93+
read rather than warn that its fields run past the length it was given.
9194
9295
.. versionchanged:: 1.6.0
9396
94-
Takes ``count`` (:issue:`2711`).
97+
Takes ``count`` (:issue:`2711`), and reads a schema item it counts as
98+
ending with the data (:issue:`2749`).
9599
96100
"""
97101

@@ -307,7 +311,10 @@ def _unpack_items(self, file: 'IO[bytes]', packet: 'dict[str, Any]', is_schema:
307311
field = item_type(packet)
308312

309313
if is_schema:
310-
data = cast('SchemaField', field).unpack(file, packet)
314+
# NOTE: A counted list runs to the end of the data, so an item it counts
315+
# running past the octets left is cut, not overrunning a length (#2749).
316+
context = dict(packet, __data_end__=True) if len(temp) < self._count else packet
317+
data = cast('SchemaField', field).unpack(file, context)
311318

312319
end = file.tell()
313320
if end <= offset:

‎pcapkit/protocols/schema.py‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -759,10 +759,20 @@ def unpack(cls, data: 'bytes | IO[bytes]',
759759
read rather than refuse it. The key is cleared as unpacking starts,
760760
so a nested schema finds only its own.
761761
762+
Fields running past ``length`` warn that ``__length__`` fell below
763+
zero, unless a ``__data_end__`` key in ``packet`` says that
764+
``length`` ends where the data does, as a
765+
:class:`~pcapkit.corekit.fields.collections.ListField` says of an
766+
item it counts. The fields past it are then the cut the short read
767+
records, and warn nothing (:issue:`2749`). The key is cleared as
768+
unpacking starts too.
769+
762770
.. versionchanged:: 1.6.0
763771
764772
The field the data ends inside is given to the fields after it, as
765-
``__short_read__`` in ``packet`` (:issue:`2727`).
773+
``__short_read__`` in ``packet`` (:issue:`2727`), and a schema whose
774+
``length`` ends with the data reads its fields past it as cut, not as
775+
a negative length (:issue:`2749`).
766776
767777
"""
768778
# force cast arg type since decorator changed their signatures
@@ -774,6 +784,9 @@ def unpack(cls, data: 'bytes | IO[bytes]',
774784
# NOTE: ``packet`` may be an enclosing schema's, copied into a nested
775785
# context, so its short read is not this schema's (#2727).
776786
packet.pop('__short_read__', None)
787+
# NOTE: Fields running past a length that ends with the data are the cut its
788+
# short read records, not an overrun (#2749).
789+
data_end = packet.pop('__data_end__', False)
777790

778791
self = cls.__new__(cls)
779792
for field in self.__fields__.values():
@@ -906,7 +919,7 @@ def unpack(cls, data: 'bytes | IO[bytes]',
906919
self._note_short_read(field.name, length, byte, packet)
907920
packet['__length__'] -= length
908921

909-
if packet['__length__'] < 0:
922+
if packet['__length__'] < 0 and not data_end:
910923
warn(f'packet length < 0: {packet["__length__"]}',
911924
SchemaWarning, stacklevel=stacklevel())
912925

Lines changed: 181 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,181 @@
1+
# -*- coding: utf-8 -*-
2+
"""An IGMPv3 Report parsed on its own and cut inside a group record reads quietly.
3+
4+
GitHub issue #2749: built directly on octets that end inside a group record, an IGMPv3
5+
Report recorded the cut and read ``TRUNCATED``, and also warned ``SchemaWarning: packet
6+
length < 0`` once for each field of the record past the cut, between one and five times.
7+
Its record list counts the records the Number of Group Records gives and runs to the end
8+
of the data (#2711), and each record is sized by the octets left to it, so
9+
:meth:`Schema.unpack <pcapkit.protocols.schema.Schema.unpack>` read the fields of the
10+
record the data ends inside as running past a length it had been given.
11+
:class:`~pcapkit.corekit.fields.collections.ListField` now reads a schema item it counts
12+
as ending with the data, and ``Schema.unpack`` reads such an item's fields past its length
13+
as the cut its short read records.
14+
15+
The Report below is cut inside its first, a middle and its last record, inside a record's
16+
fixed fields, its source list and its auxiliary data, and built directly with and without
17+
a length. It must read ``TRUNCATED`` with its short-read record, report nothing on the
18+
warning channel or the ``pcapkit`` logger, and rebuild from its data model and its dict as
19+
captured. Behind IPv4 the count sizes nothing, so the change does not reach it: a whole
20+
Report there whose record counts more sources than it holds is no cut, and still warns.
21+
22+
The messages are built here; this module is unit-tier and reads no capture file.
23+
24+
"""
25+
from __future__ import annotations
26+
27+
import contextlib
28+
import logging
29+
import unittest
30+
import warnings
31+
from typing import TYPE_CHECKING
32+
from unittest import mock
33+
34+
from tests._support import reimport_once_per_class
35+
from tests.foundation import _roundtrip as wire
36+
37+
if TYPE_CHECKING:
38+
from typing import Any, Iterator
39+
40+
#: Test tier (#1958): hand-picked parse -> rebuild round trips.
41+
__tier__ = 'ALL'
42+
43+
44+
def record(sources: int, aux: int = 0, *, declared: 'int | None' = None) -> bytes:
45+
"""A MODE_IS_INCLUDE record for 239.1.1.1 with ``sources`` sources and ``aux`` words of
46+
auxiliary data, its Number of Sources ``declared``, or ``sources`` if not given."""
47+
count = sources if declared is None else declared
48+
return (bytes([1, aux]) + count.to_bytes(2, 'big') + bytes([239, 1, 1, 1])
49+
+ bytes([10, 0, 0, 1]) * sources + b'AUX!' * aux)
50+
51+
52+
def report(*records: bytes) -> bytes:
53+
"""An IGMPv3 Report of ``records``, checksummed."""
54+
rest = len(records).to_bytes(2, 'big') + b''.join(records)
55+
head = b'\x22\x00'
56+
return head + wire.checksum(head + bytes(4) + rest).to_bytes(2, 'big') + b'\x00\x00' + rest
57+
58+
59+
def wrong(msg: bytes) -> bytes:
60+
"""``msg`` with its checksum flipped in both octets."""
61+
return msg[:2] + (int.from_bytes(msg[2:4], 'big') ^ 0x0101).to_bytes(2, 'big') + msg[4:]
62+
63+
64+
#: The Report's records: two sources; one source and a word of auxiliary data; two
65+
#: sources and two words of it.
66+
RECORDS = (record(2), record(1, aux=1), record(2, aux=2))
67+
#: The Report itself.
68+
MESSAGE = report(*RECORDS)
69+
#: Each record's ordinal name, in order.
70+
ORDINALS = ('first', 'middle', 'last')
71+
72+
73+
def cuts() -> 'Iterator[tuple[str, str, int]]':
74+
"""Every cut strictly inside a record: the record, the part its first missing octet is
75+
in, and the octets kept."""
76+
start = 8
77+
for ordinal, item in zip(ORDINALS, RECORDS):
78+
sources = int.from_bytes(item[2:4], 'big')
79+
for offset in range(1, len(item)):
80+
part = 'fixed fields' if offset < 8 else 'source list' if offset < 8 + 4 * sources else 'aux data'
81+
yield ordinal, part, start + offset
82+
start += len(item)
83+
84+
85+
class _Logged(logging.Handler):
86+
"""Collect what the ``pcapkit`` logger reports."""
87+
88+
def __init__(self) -> None:
89+
super().__init__(logging.DEBUG)
90+
self.messages = [] # type: list[str]
91+
92+
def emit(self, record: 'logging.LogRecord') -> None: # pylint: disable=redefined-outer-name
93+
self.messages.append(record.getMessage())
94+
95+
96+
@contextlib.contextmanager
97+
def _reported() -> 'Iterator[tuple[list[str], list[warnings.WarningMessage]]]':
98+
"""Collect what the block reports, on the logger and the :mod:`warnings` channel."""
99+
handler = _Logged()
100+
logger = logging.getLogger('pcapkit')
101+
logger.addHandler(handler)
102+
try:
103+
with warnings.catch_warnings(record=True) as caught:
104+
warnings.simplefilter('always')
105+
yield handler.messages, caught
106+
finally:
107+
logger.removeHandler(handler)
108+
109+
110+
class ReportRecordCutTests(unittest.TestCase):
111+
"""A Report parsed on its own and cut inside a record records the cut and reads quietly."""
112+
113+
def setUp(self) -> None:
114+
reimport_once_per_class(self)
115+
from pcapkit.corekit.checksum.internet import ChecksumStatus
116+
from pcapkit.protocols.internet.igmpv3.protocol import IGMPv3
117+
from pcapkit.protocols.internet.ipv4.protocol import IPv4
118+
from pcapkit.utilities import config
119+
120+
self.IGMPv3 = IGMPv3
121+
self.IPv4 = IPv4
122+
self.Status = ChecksumStatus
123+
patch = mock.patch.object(config, 'CHECKSUM_VERIFY', True)
124+
patch.start()
125+
self.addCleanup(patch.stop)
126+
127+
def direct(self, data: bytes, sized: bool) -> 'tuple[Any, list[str]]':
128+
"""``data`` built directly as IGMPv3, given its length if ``sized``, and what it reported."""
129+
with _reported() as (logged, caught):
130+
layer = self.IGMPv3(data, len(data)) if sized else self.IGMPv3(data)
131+
return layer, logged + [f'{item.category.__name__}: {item.message}' for item in caught]
132+
133+
def test_the_cuts_reach_every_part_of_every_record(self) -> None:
134+
parts = {(ordinal, part) for ordinal, part, _ in cuts()}
135+
self.assertEqual(parts, {(ordinal, part) for ordinal in ORDINALS for part in ('fixed fields', 'source list')}
136+
| {('middle', 'aux data'), ('last', 'aux data')})
137+
138+
def test_a_cut_inside_a_record_reads_truncated_and_quietly(self) -> None:
139+
for ordinal, part, keep in cuts():
140+
for bad in (False, True):
141+
for sized in (False, True):
142+
with self.subTest(record=ordinal, part=part, captured=keep, bad=bad, sized=sized):
143+
layer, reported = self.direct((wrong(MESSAGE) if bad else MESSAGE)[:keep], sized)
144+
self.assertEqual(reported, [])
145+
self.assertEqual(layer.info.get('__short_read__'), ('body', keep - 1))
146+
self.assertIs(layer.info.checksum_status, self.Status.TRUNCATED)
147+
self.assertEqual(len(layer.info.records), ORDINALS.index(ordinal) + 1)
148+
149+
def test_a_cut_inside_a_record_rebuilds_as_captured_and_quietly(self) -> None:
150+
for ordinal, part, keep in cuts():
151+
layer, _ = self.direct(MESSAGE[:keep], True)
152+
for path, data in (('model', layer.info), ('dict', layer.info.to_dict())):
153+
with self.subTest(record=ordinal, part=part, captured=keep, path=path):
154+
with _reported() as (logged, caught):
155+
rebuilt = self.IGMPv3.from_data(data)
156+
self.assertEqual((rebuilt.data, rebuilt.info.checksum_status),
157+
(MESSAGE[:keep], self.Status.TRUNCATED))
158+
self.assertEqual((logged, [str(item.message) for item in caught]), ([], []))
159+
160+
def test_a_whole_report_reads_valid_and_quietly(self) -> None:
161+
for sized in (False, True):
162+
with self.subTest(sized=sized):
163+
layer, reported = self.direct(MESSAGE, sized)
164+
self.assertEqual(reported, [])
165+
self.assertIsNone(layer.info.get('__short_read__'))
166+
self.assertIs(layer.info.checksum_status, self.Status.VALID)
167+
self.assertEqual(self.IGMPv3.from_data(layer.info).data, MESSAGE)
168+
169+
def test_a_record_overstating_its_sources_behind_ipv4_still_warns(self) -> None:
170+
# IPv4 says where the message ends, so the record's source list and auxiliary data
171+
# run past a length the frame declares: a malformed record, not a cut.
172+
msg = report(record(1, declared=5))
173+
with _reported() as (_, caught):
174+
layer = self.IPv4(wire.ipv4(msg, proto=2)).payload
175+
self.assertIsInstance(layer, self.IGMPv3)
176+
self.assertEqual([str(item.message) for item in caught if item.category.__name__ == 'SchemaWarning'],
177+
['packet length < 0: -16', 'packet length < 0: -16'])
178+
179+
180+
if __name__ == '__main__':
181+
unittest.main()

0 commit comments

Comments
 (0)