From 87cabdf8425db4c1657dceb9e956054885acd986 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 17 Sep 2026 01:12:10 -0400 Subject: [PATCH 1/8] corekit: bound the option loop by the octets it consumed, not by len(data) (#431) `OptionField.unpack` never returned for a well-formed eight-octet HOPOPT header: from pcapkit.protocols.internet.hopopt import HOPOPT buf = bytes.fromhex('3b00' '0803810203' '00') # SMF_DPD len=3, then Pad1 HOPOPT(buf, len(buf)) # 15s of CPU, no return `len=0` means an 8-octet header, and the 6-octet option area holds an SMF_DPD option of length 3 followed by one `Pad1`. Nothing about it is malformed. A remote peer that can put such an option in a Hop-by-Hop header hangs the process, with no exception and no diagnostic -- and `pcapkit` parses untrusted input. The loop decremented its remaining `length` by `len(data)`, the size of the schema the option *reported*, which is not the number of octets the option took from the stream. The two part company for the six wrapper schemas whose `post_process` returns a *nested* schema, since `len(data)` then measures the nested schema and not what the wrapper read. Instrumented, on the bytes above: step 0: length=6 SMF_DPD len(data)=5 advanced 6 -> length 1 step 1: length=1 Pad1 len(data)=0 advanced 0 -> length 1 step 2..n: identical, forever Step 0 left `length` at 1 with the stream already exhausted, so from step 1 on every field read `b''`, `len(data)` was 0, and `length -= 0` made no progress. - Remember where the previous option ended, and require each iteration to move the stream forward at least one octet. That is a measure of progress the loop can take independently of what a schema reports, and it is what bounds the iteration count. A `FieldValueError` naming the option code, the offset and the octets left to parse replaces the hang; here it lands in 1ms and reads `at offset 6 of 6`, which is the tell that an earlier option over-read. - `length` is still decremented by `len(data)`, deliberately: every option this package can parse consumes at least its own type field, so no option area that parses today can reach the guard, and none parses differently. The `_SMFDPDOption` over-read that starts the mismatch is a separate defect and is fixed separately. Tests: `tests/protocols/schema/test_schema_unit.py` builds the smallest schema that separates the two measures -- a wrapper reading two octets and returning a nested schema that recorded one -- and asserts the loop raises rather than spins. `tests/_support.time_limit` gives it a `signal.alarm` deadline, so a regression fails the suite in five seconds instead of wedging CI; a watchdog thread cannot do that job, because the loop holds the GIL for the whole of an iteration. On the pristine tree the test fails with `TimeoutError: did not finish within 5s`. 858 passed, 17 skipped (857 before). Every capture in `examples/captures/` dumped to `tree` and `json` with `ip=True, tcp=True, reassembly=True` is byte-identical before and after. --- pcapkit/corekit/fields/collections.py | 33 ++++++++++++++ tests/_support.py | 53 +++++++++++++++++++++- tests/protocols/schema/test_schema_unit.py | 52 ++++++++++++++++++++- 3 files changed, 136 insertions(+), 2 deletions(-) diff --git a/pcapkit/corekit/fields/collections.py b/pcapkit/corekit/fields/collections.py index ae0c7ec202..e099d78f78 100644 --- a/pcapkit/corekit/fields/collections.py +++ b/pcapkit/corekit/fields/collections.py @@ -244,6 +244,10 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'list as the remaining length to the ``packet`` argument such that the next fields can be aware of such informations. + Raises: + FieldValueError: If an option consumes nothing from ``buffer``, since + the loop below has then no way to get past it. + """ length = self._length if isinstance(buffer, bytes): @@ -251,6 +255,25 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'list else: file = buffer + # NOTE: The loop below sizes each option by ``len(data)`` -- the size of + # the schema the option reported -- and that is not always the number of + # octets the option took from ``file``. The two part company for a schema + # whose ``post_process`` returns a *nested* schema, since ``len(data)`` + # then measures the nested schema rather than what the outer one read. So + # ``len(data)`` cannot be the loop's progress measure: an option that + # over-reads leaves ``length`` above zero with ``file`` already exhausted, + # every field of the next option reads ``b''``, and an option that read + # nothing reports ``len(data) == 0`` and leaves ``length`` untouched -- + # which spins forever, with no exception and no diagnostic. C.f. #431. + # + # Remembering where the previous option ended gives the loop a measure of + # progress that does not depend on what a schema reports, and one octet of + # it per iteration is what bounds the iteration count. ``length`` is still + # decremented by ``len(data)``, so that no option area which parses today + # parses differently: every option this package can parse consumes at + # least its own type field, so the guard cannot fire on one. + offset = file.tell() + # make a copy of the ``packet`` dict so that we can include # parsed option schema in the ``packet`` dict new_packet = packet.copy() @@ -271,6 +294,16 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'list new_packet[self.name].add(code, data) temp.append(data) + # insist on progress through ``file`` before trusting ``len(data)`` + end = file.tell() + if end <= offset: + raise FieldValueError( + f'Field {self.name} has an option that consumed no data: ' + f'{code!r} at offset {offset} of {self._length}, with ' + f'{length} octet(s) of the option area left to parse' + ) + offset = end + # update length length -= len(data) diff --git a/tests/_support.py b/tests/_support.py index a0e6d5082d..e7be92c219 100644 --- a/tests/_support.py +++ b/tests/_support.py @@ -2,17 +2,68 @@ import abc import collections.abc +import contextlib import importlib.util import inspect import pathlib +import signal import sys import types -from typing import Iterable +import unittest +from typing import Iterable, Iterator from tests._tiers import (ROOT, SAMPLE_ROOT, REGENERATE_SAMPLES_CMD, GeneratedFixtureInUnitTierError, check_unit_tier_read) +@contextlib.contextmanager +def time_limit(seconds: int = 5) -> Iterator[None]: + """Fail the calling test if its body has not finished in ``seconds`` seconds. + + A parser defect that degenerates into a loop making no progress -- GitHub + issue #431 is one -- offers a test nothing to assert on: the call under test + simply never returns. A test written for it without a deadline does not fail, + it *wedges*, taking the rest of the run with it, so the deadline is as much a + part of the regression test as the assertion is. + + :func:`signal.alarm` is what interrupts the body, rather than a watchdog + thread: the loops this guards are pure Python and hold the GIL for the whole + of an iteration, so nothing in another thread gets to run and stop them, + whereas a signal is delivered between bytecodes. That also rules out + :data:`signal.SIGTERM` from an outer :program:`timeout`, which such a loop + likewise never gets around to handling. + + Args: + seconds: Whole seconds to allow the body. :func:`signal.alarm` counts in + whole seconds, so this cannot usefully be fractional. + + Yields: + Nothing. The deadline applies to the body of the ``with`` statement. + + Raises: + TimeoutError: If the body has not finished within ``seconds`` seconds. + + """ + # An interval timer is a POSIX facility, and the deadline is the whole point + # of this helper: silently running the body without one would restore exactly + # the wedged run it exists to prevent, so the test is skipped instead. + if not hasattr(signal, 'SIGALRM'): + raise unittest.SkipTest('signal.alarm is unavailable on this platform') + + def expire(signum: int, frame: object) -> None: + raise TimeoutError(f'did not finish within {seconds}s') + + previous = signal.signal(signal.SIGALRM, expire) + signal.alarm(seconds) + try: + yield + finally: + # Cancel before restoring, so that an alarm which fires between the two + # cannot be delivered to whatever handler was installed before. + signal.alarm(0) + signal.signal(signal.SIGALRM, previous) + + def sample_path(name: str) -> str: """Resolve a sample capture file name to its absolute path. diff --git a/tests/protocols/schema/test_schema_unit.py b/tests/protocols/schema/test_schema_unit.py index 37a96a4733..f82499be0b 100644 --- a/tests/protocols/schema/test_schema_unit.py +++ b/tests/protocols/schema/test_schema_unit.py @@ -7,7 +7,7 @@ import unittest from unittest import mock -from tests._support import purge_modules +from tests._support import purge_modules, time_limit RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) @@ -330,6 +330,56 @@ class TinyOptionsSchema(Schema): self.assertEqual(unpacked.pad, b'') self.assertEqual(observed_padding, [1]) + def test_schema_option_field_unpack_rejects_an_option_consuming_nothing(self) -> None: + """An option area that cannot be advanced past is an error, not a hang. + + :meth:`OptionField.unpack + ` sizes each + option by ``len(data)``, the size of the schema the option reported, + which is not the number of octets it took from the stream. ``Wrapper`` + below is the smallest thing that separates the two, and is the shape + every wrapper option schema in the package has: it reads two octets and + returns a nested schema that recorded one. So the first option leaves the + stream one octet ahead of where ``length`` thinks it is, the second + option consumes the last of it, and the third finds the stream exhausted, + reads ``b''`` for every field, and reports ``len(data) == 0`` -- against + which ``length -= len(data)`` makes no progress at all. C.f. #431, where + this spun forever on an eight-octet HOPOPT header. + + The deadline is part of the test: without it a regression here does not + fail, it hangs the run. + + """ + from pcapkit.corekit.fields.collections import OptionField + from pcapkit.corekit.fields.misc import SchemaField + from pcapkit.corekit.fields.numbers import UInt8Field + from pcapkit.protocols.schema.schema import Schema, schema_final + from pcapkit.utilities.exceptions import FieldValueError + + @schema_final + class Marker(Schema): + type: int = UInt8Field(default=0) + + @schema_final + class Wrapper(Schema): + #: Two octets of stream, of which ``Marker`` records only the first. + body: Marker = SchemaField(length=2, schema=Marker) + + def post_process(self, packet: dict) -> Schema: + return self.body + + @schema_final + class WrappedOptionsSchema(Schema): + options: list[Marker] = OptionField( + length=3, + base_schema=Marker, + registry=collections.defaultdict(lambda: Marker, {1: Wrapper}), + ) + + with self.assertRaisesRegex(FieldValueError, 'consumed no data'): + with time_limit(5): + WrappedOptionsSchema.unpack(b'\x01\xff\x00', 3, {}) + if __name__ == '__main__': unittest.main() From b07f7839ac52d9f6c06552fda054cb7867029995 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 17 Sep 2026 01:27:12 -0400 Subject: [PATCH 2/8] hopopt, ipv6-opts: read an SMF_DPD option's declared length from the right octet (#431) `_SMFDPDOption` is the read-side wrapper that peeks at an `SMF_DPD` option to decide which of the two DPD mode schemas should parse it. It over-read the stream, which is what started the option-loop accounting mismatch behind the hang in #431. Two separate mistakes, both in the sizing: - `test`'s `BitField` namespace declared `'len': (1, 8)`. Those tuples are `(bit_offset, bit_width)` into the whole three-octet forward match, and `Opt Data Len` is octet 1, i.e. bits 8 to 15 -- not bits 1 to 8, which straddle the `Option Type` octet and take one bit of the length. For the option `08 03 81` it read `len` as **16** where the wire says 3, so `smf_dpd_data_selector` built a `SchemaField(length=16)` and `Schema.unpack` read 16 octets, or as much of the option area as there was. That is where the stray octet in #431 came from: six octets consumed against a nested schema that recorded five. - `smf_dpd_data_selector` then sized the field at `Opt Data Len` exactly. `Opt Data Len` counts only what follows the option header (RFC 8200 section 4.2), while both `SMFIdentificationBasedDPDOption` and `SMFHashBasedDPDOption` inherit `Option.type` and `Option.len` and parse those two octets themselves, so the area has to be `Opt Data Len + 2` octets wide. Fixing only the bit offsets would have swapped an over-read for a two-octet under-read. `ipv6_opts.py` carries the same two lines and gets the same fix; HOPOPT and IPv6-Opts now agree on a hash-based DPD option, which they did not before. With both, #431's eight-octet reproducer parses in 1ms instead of spinning forever: `SMF_DPD` of length 5 carrying `hav=b'\x81\x02\x03'` in H-DPD mode, then a `Pad1` of length 1, accounting for all six octets of the option area, and repacking to the octets it was read from. Deliberately not fixed here: `ipv6_opts.SMFIdentificationBasedDPDOption` carries an extra forward-matched `test` octet that HOPOPT's does not. `Schema.__buffer__` records it although the stream never consumes it, so `len(data)` overshoots by one and the option loop charges one octet too many against the area, losing a trailing `Pad1`. It is the same `len(data)`-is-not-consumed confusion, in the other direction, and it is not a hang -- it wants its own change. Tests: `tests/protocols/internet/test_ipv6_extension_unit.py` parses #431's reproducer verbatim plus two more hash-based shapes and two identification-based ones, for both HOPOPT and IPv6-Opts, asserting the option lengths account for the whole option area and that the header repacks byte-for-byte. Each runs under `tests._support.time_limit`, so on the pristine tree they fail with `TimeoutError` rather than hanging the run: 7 of the 10 cases do. The other three happen to size correctly despite the misread -- an option that fills the area exactly cannot over-read past it -- which is why one case is not enough. 862 passed, 17 skipped (857 before this branch). Every capture in `examples/captures/` dumped to `tree` and `json` with `ip=True, tcp=True, reassembly=True` is byte-identical before and after: no capture in the suite carries an `SMF_DPD` option. --- pcapkit/protocols/schema/internet/hopopt.py | 26 +++- .../protocols/schema/internet/ipv6_opts.py | 26 +++- .../internet/test_ipv6_extension_unit.py | 135 +++++++++++++++++- 3 files changed, 180 insertions(+), 7 deletions(-) diff --git a/pcapkit/protocols/schema/internet/hopopt.py b/pcapkit/protocols/schema/internet/hopopt.py index 6532eccab7..1e0ba35dc4 100644 --- a/pcapkit/protocols/schema/internet/hopopt.py +++ b/pcapkit/protocols/schema/internet/hopopt.py @@ -155,12 +155,22 @@ def smf_dpd_data_selector(pkt: 'dict[str, Any]') -> 'Field': wrapped :class:`~pcapkit.protocols.schema.internet.hopopt.SMFHashBasedDPDOption` instance. + Note: + The field is sized ``Opt Data Len + 2`` rather than ``Opt Data Len``. + ``Opt Data Len`` counts only what follows the option header + [:rfc:`8200#section-4.2`], while both schemas this may return inherit + :attr:`Option.type` and :attr:`Option.len` and so parse those two octets + themselves. Sizing the field at ``Opt Data Len`` handed them an area two + octets short of the option they read, which + :class:`~pcapkit.corekit.fields.collections.OptionField` then mis-counted + against the option area -- c.f. #431. + """ mode = Enum_SMFDPDMode.get(pkt['test']['mode']) schema = SMFDPDOption.registry[mode] if schema is None: raise FieldValueError(f'HOPOPT: invalid SMF DPD mode: {mode}') - return SchemaField(length=pkt['test']['len'], schema=schema) + return SchemaField(length=pkt['test']['len'] + 2, schema=schema) def smf_i_dpd_tid_selector(pkt: 'dict[str, Any]') -> 'Field': @@ -367,11 +377,21 @@ def __init__(self, type: 'Enum_Option', len: 'int', domain: 'int', cmpt_len: 'in @schema_final class _SMFDPDOption(Schema): - """Header schema for HOPOPT SMF DPD options with generic representation.""" + """Header schema for HOPOPT SMF DPD options with generic representation. + + The ``test`` field forward-matches the first three octets of the option -- + ``Option Type``, ``Opt Data Len`` and the octet carrying the DPD mode bit -- + without consuming them, so that :func:`smf_dpd_data_selector` can size and + choose the schema which then reads the option properly. Its ``namespace`` + offsets are therefore bit offsets into the *option*, not into any one field + of it: ``Opt Data Len`` is octet 1, i.e. bits 8 to 15, and the mode bit is + the first bit of octet 2 [:rfc:`6621#section-8.1`]. + + """ #: SMF DPD mode. test: 'SMFDPDTestFlag' = ForwardMatchField(BitField(length=3, namespace={ - 'len': (1, 8), + 'len': (8, 8), 'mode': (16, 1), })) #: SMF DPD data. diff --git a/pcapkit/protocols/schema/internet/ipv6_opts.py b/pcapkit/protocols/schema/internet/ipv6_opts.py index 6d2daae4c9..01ac119990 100644 --- a/pcapkit/protocols/schema/internet/ipv6_opts.py +++ b/pcapkit/protocols/schema/internet/ipv6_opts.py @@ -155,12 +155,22 @@ def smf_dpd_data_selector(pkt: 'dict[str, Any]') -> 'Field': wrapped :class:`~pcapkit.protocols.schema.internet.ipv6_opts.SMFHashBasedDPDOption` instance. + Note: + The field is sized ``Opt Data Len + 2`` rather than ``Opt Data Len``. + ``Opt Data Len`` counts only what follows the option header + [:rfc:`8200#section-4.2`], while both schemas this may return inherit + :attr:`Option.type` and :attr:`Option.len` and so parse those two octets + themselves. Sizing the field at ``Opt Data Len`` handed them an area two + octets short of the option they read, which + :class:`~pcapkit.corekit.fields.collections.OptionField` then mis-counted + against the option area -- c.f. #431. + """ mode = Enum_SMFDPDMode.get(pkt['test']['mode']) schema = SMFDPDOption.registry[mode] if schema is None: raise FieldValueError(f'IPv6-Opts: invalid SMF DPD mode: {mode}') - return SchemaField(length=pkt['test']['len'], schema=schema) + return SchemaField(length=pkt['test']['len'] + 2, schema=schema) def smf_i_dpd_tid_selector(pkt: 'dict[str, Any]') -> 'Field': @@ -367,11 +377,21 @@ def __init__(self, type: 'Enum_Option', len: 'int', domain: 'int', cmpt_len: 'in @schema_final class _SMFDPDOption(Schema): - """Header schema for IPv6-Opts SMF DPD options with generic representation.""" + """Header schema for IPv6-Opts SMF DPD options with generic representation. + + The ``test`` field forward-matches the first three octets of the option -- + ``Option Type``, ``Opt Data Len`` and the octet carrying the DPD mode bit -- + without consuming them, so that :func:`smf_dpd_data_selector` can size and + choose the schema which then reads the option properly. Its ``namespace`` + offsets are therefore bit offsets into the *option*, not into any one field + of it: ``Opt Data Len`` is octet 1, i.e. bits 8 to 15, and the mode bit is + the first bit of octet 2 [:rfc:`6621#section-8.1`]. + + """ #: SMF DPD mode. test: 'SMFDPDTestFlag' = ForwardMatchField(BitField(length=3, namespace={ - 'len': (1, 8), + 'len': (8, 8), 'mode': (16, 1), })) #: SMF DPD data. diff --git a/tests/protocols/internet/test_ipv6_extension_unit.py b/tests/protocols/internet/test_ipv6_extension_unit.py index 22870d5703..0118474203 100644 --- a/tests/protocols/internet/test_ipv6_extension_unit.py +++ b/tests/protocols/internet/test_ipv6_extension_unit.py @@ -8,7 +8,7 @@ import unittest from unittest import mock -from tests._support import purge_modules +from tests._support import purge_modules, time_limit RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) @@ -1390,6 +1390,139 @@ def test_ipv6_opts_padding_options_parse_from_the_wire(self) -> None: self._assert_padding_options_parse_from_the_wire(IPv6_Opts) + def _assert_smf_dpd_options_parse_from_the_wire(self, protocol_cls: type) -> None: + """A hash-based ``SMF_DPD`` option must consume exactly what it declares. + + Per :rfc:`6621#section-8.1` the option is an ``Option Type`` octet, an + ``Opt Data Len`` octet, and ``Opt Data Len`` further octets whose first + bit is the DPD mode -- set, here, for hash-based DPD, so that the whole of + the hash assist value is the ``Opt Data Len`` octets. Two things have to + hold for that to read correctly, and each case below breaks if either + does not: the declared length has to be read from the ``Opt Data Len`` + octet, and the area handed to the mode schema has to be two octets wider + than it, since that schema parses the option header itself. + + The first case is #431's reproducer verbatim. On the pristine tree it does + not fail, it never returns -- 15 seconds of CPU with no exception and no + diagnostic -- so every case runs under a deadline: a regression here has + to fail the suite rather than wedge it. + + """ + from pcapkit.const.ipv6.option import Option + from pcapkit.const.ipv6.smf_dpd_mode import SMFDPDMode + + cases = ( + ('#431: hash-based DPD of length 3, then a pad1', + b'\x3b\x00' + b'\x08\x03\x81\x02\x03' + b'\x00', + [(Option.SMF_DPD, 5), (Option.Pad1, 1)], + [b'\x81\x02\x03']), + ('hash-based DPD filling the option area', + b'\x3b\x00' + b'\x08\x04\x81\x02\x03\x04', + [(Option.SMF_DPD, 6)], + [b'\x81\x02\x03\x04']), + ('two hash-based DPD options back to back', + b'\x3b\x01' + b'\x08\x02\x81\x01' + b'\x08\x02\x81\x02' + b'\x00' * 6, + [(Option.SMF_DPD, 4), (Option.SMF_DPD, 4)] + [(Option.Pad1, 1)] * 6, + [b'\x81\x01', b'\x81\x02']), + ) + + for name, raw, expected, expected_hav in cases: + with self.subTest(case=name): + with time_limit(5): + proto = protocol_cls(raw, extension=True) + + options = list(proto.info.options.items(multi=True)) + + self.assertEqual(proto.info.length, len(raw)) + self.assertEqual([(code, opt.length) for code, opt in options], expected) + + # every octet of the option area has to be accounted for by an + # option, which is what the mis-sized read got wrong + self.assertEqual(sum(length for _, length in expected), len(raw) - 2) + + self.assertEqual( + [opt.hav for code, opt in options if code == Option.SMF_DPD], + expected_hav, + ) + self.assertEqual( + [opt.dpd_type for code, opt in options if code == Option.SMF_DPD], + [SMFDPDMode.H_DPD] * len(expected_hav), + ) + + # the reader and the writer must agree: repacking the parsed + # schema has to give back the very bytes it was read from + self.assertEqual(bytes(proto.__header__), raw) + + def test_hopopt_smf_dpd_options_parse_from_the_wire(self) -> None: + from pcapkit.protocols.internet.hopopt import HOPOPT + + self._assert_smf_dpd_options_parse_from_the_wire(HOPOPT) + + def test_ipv6_opts_smf_dpd_options_parse_from_the_wire(self) -> None: + from pcapkit.protocols.internet.ipv6_opts import IPv6_Opts + + self._assert_smf_dpd_options_parse_from_the_wire(IPv6_Opts) + + def _assert_identification_based_dpd_options_parse_from_the_wire(self, protocol_cls: type) -> None: + """The other ``SMF_DPD`` mode, which sizes itself from a TaggerID. + + Identification-based DPD reaches the same mis-sized read by a different + route: its length comes from ``Opt Data Len`` too, but the octets it + covers are a TaggerID whose own width comes from the ``TidLen`` nibble + [:rfc:`6621#section-8.1`], so a wrong ``Opt Data Len`` mis-sizes the + identifier rather than the whole option. Both cases below hang the + pristine tree exactly as the hash-based ones do. + + Only the option itself is asserted, not the padding after it: HOPOPT and + IPv6-Opts disagree on how much of the option area an + ``SMFIdentificationBasedDPDOption`` leaves over, because the IPv6-Opts + schema carries an extra forward-matched octet which + :attr:`Schema.__buffer__ ` + records although the stream never consumes it. That is its own + ``len(data)``-is-not-consumed defect, distinct from the one #431 is about, + and pinning either count here would bless one of the two. + + """ + from pcapkit.const.ipv6.option import Option + from pcapkit.const.ipv6.smf_dpd_mode import SMFDPDMode + from pcapkit.const.ipv6.tagger_id import TaggerID + + cases = ( + ('null taggerID, two-octet identifier', + b'\x3b\x00' + b'\x08\x03\x00\x01\x02' + b'\x00', + 5, TaggerID.NULL, 0, None, b'\x01\x02'), + ('four-octet taggerID, one-octet identifier', + b'\x3b\x01' + b'\x08\x06\x13\x0a\x00\x00\x01\xff' + b'\x00' * 6, + 8, TaggerID.DEFAULT, 3, b'\x0a\x00\x00\x01', b'\xff'), + ) + + for name, raw, length, tid_type, tid_len, tid, identifier in cases: + with self.subTest(case=name): + with time_limit(5): + proto = protocol_cls(raw, extension=True) + + option = next(opt for code, opt in proto.info.options.items(multi=True) + if code == Option.SMF_DPD) + + self.assertEqual(option.length, length) + self.assertEqual(option.dpd_type, SMFDPDMode.I_DPD) + self.assertEqual(option.tid_type, tid_type) + self.assertEqual(option.tid_len, tid_len) + self.assertEqual(option.tid, tid) + self.assertEqual(option.id, identifier) + + self.assertEqual(bytes(proto.__header__), raw) + + def test_hopopt_identification_based_dpd_options_parse_from_the_wire(self) -> None: + from pcapkit.protocols.internet.hopopt import HOPOPT + + self._assert_identification_based_dpd_options_parse_from_the_wire(HOPOPT) + + def test_ipv6_opts_identification_based_dpd_options_parse_from_the_wire(self) -> None: + from pcapkit.protocols.internet.ipv6_opts import IPv6_Opts + + self._assert_identification_based_dpd_options_parse_from_the_wire(IPv6_Opts) + def _assert_padding_option_schema_sizes_itself(self, protocol_cls: type) -> None: """The padding option schema, on its own. From 54378e3df4978cb3803637485190e399d06e4656 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 17 Sep 2026 01:48:58 -0400 Subject: [PATCH 3/8] corekit: give ListField the same progress guarantee as its OptionField subclass (#431) Found by sweeping for siblings of #431: the non-progress loop is not confined to `OptionField`. `ListField.unpack`'s schema branch has it too, and a single TCP segment reaches it: TCP(bytes.fromhex('0001000200000000000000006010000000000000051636cc'), 24) 24 octets. A data offset of 6 gives a four-octet option area, holding a `SACK` option that declares 22. `SACK.sack` is a `ListField` sized `Length - 2` with a `SchemaField` item type, so the loop is handed a 20-octet budget over two octets of stream: the first `SACKBlock` records what there is, the second finds the stream exhausted, records nothing, and `length -= len(data)` never falls. Randomly generated TCP headers with a plausible option area hang 5 times in 1500 on `origin/main`, and all 5 are this loop rather than the `OptionField` one -- the guard from the first commit of this branch does not cover them. - Require each schema item to move the stream forward, exactly as `OptionField` now does, and raise a `FieldValueError` naming the item index, the offset and the octets left when it does not. The item-typed branch is left alone: it sizes each item by `field.length`, which it read rather than recorded, so it already makes progress. `SCTP` had already met this and worked around it locally: `bounded()` in `schema/transport/sctp.py` clamps its two `ListField` lengths to the octets on hand, and its docstring describes precisely this hang -- "a `SchemaField` item that runs out of bytes parses to nothing, subtracts nothing, and the loop never terminates". That fix stays where it is; it turns the overrun into a short list the read handlers reject, which is a better answer for SCTP than an exception. The guard is what covers the sites with no such clamp: `tcp.SACK`, `hip.LocatorSetParameter` and `mh.CGAParametersOption` all size a `ListField` straight from a wire length field. Tests: `tests/protocols/transport/test_tcp_udp_unit.py` parses the segment above and a well-formed one-block `SACK` segment beside it, so that a guard which rejected real `SACK` options could not pass. Under `tests._support.time_limit`, so on the pristine tree it fails with `TimeoutError` rather than hanging. 863 passed, 17 skipped (857 before this branch). Captures still byte-identical to `origin/main` in `tree` and `json` with `ip=True, tcp=True, reassembly=True`. --- pcapkit/corekit/fields/collections.py | 25 +++++++++ .../protocols/transport/test_tcp_udp_unit.py | 51 +++++++++++++++++++ 2 files changed, 76 insertions(+) diff --git a/pcapkit/corekit/fields/collections.py b/pcapkit/corekit/fields/collections.py index e099d78f78..8f6ba6e1ca 100644 --- a/pcapkit/corekit/fields/collections.py +++ b/pcapkit/corekit/fields/collections.py @@ -125,6 +125,10 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'byte Returns: Unpacked field value. + Raises: + FieldValueError: If the items overrun the field, or if a schema item + consumes nothing from ``buffer`` -- see the note below. + """ length = self._length if isinstance(buffer, bytes): @@ -138,6 +142,18 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'byte from pcapkit.corekit.fields.misc import SchemaField is_schema = isinstance(self._item_type, SchemaField) + # NOTE: The item-typed branch below sizes each item by ``field.length``, + # which is what it read, but the schema branch sizes it by ``len(data)``, + # which is only what the schema *recorded*. A schema reading a stream that + # has already run out records nothing, so ``length -= len(data)`` makes no + # progress and the loop spins forever. Reachable from a TCP segment: a + # ``SACK`` option declaring more octets than the option area holds leaves + # ``sack``'s ``ListField`` reading ``SACKBlock`` off an exhausted stream. + # Remembering where the previous item ended is what bounds the iteration + # count, since it does not depend on what the schema reports. C.f. #431, + # which is the same defect in the ``OptionField`` subclass. + offset = file.tell() + temp = [] # type: list[_TL] while length > 0: field = self._item_type(packet) @@ -145,6 +161,15 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'byte if is_schema: data = cast('SchemaField', self._item_type).unpack(file, packet) + end = file.tell() + if end <= offset: + raise FieldValueError( + f'Field {self.name} has an item that consumed no data: ' + f'item {len(temp)} at offset {offset} of {self._length}, ' + f'with {length} octet(s) of the field left to parse' + ) + offset = end + length -= len(data) if length < 0: raise FieldValueError(f'Field {self.name} has invalid length.') diff --git a/tests/protocols/transport/test_tcp_udp_unit.py b/tests/protocols/transport/test_tcp_udp_unit.py index 0e4e010599..696b86a9ea 100644 --- a/tests/protocols/transport/test_tcp_udp_unit.py +++ b/tests/protocols/transport/test_tcp_udp_unit.py @@ -1174,6 +1174,57 @@ def test_construction_accepts_bare_integer_ports(self) -> None: self.assertEqual(unnamed.info.srcport.port, 53406) self.assertIs(unnamed.info.dstport, member) + def test_a_sack_option_overrunning_the_option_area_raises(self) -> None: + """An over-long ``SACK`` option must be an error, not a hang. + + ``SACK``'s blocks are a + :class:`~pcapkit.corekit.fields.collections.ListField` sized ``Length - 2`` + from the wire, with a + :class:`~pcapkit.corekit.fields.misc.SchemaField` item type -- and that + loop subtracts each item's *parsed* size from its budget. A ``Length`` + larger than the option area leaves it reading ``SACKBlock`` off an + exhausted stream, where every field reads ``b''``, the item parses to + nothing, and the budget never falls. This is the same defect as #431, in + ``OptionField``'s base class rather than in ``OptionField`` itself, and it + is reachable from a single TCP segment. + + The segment below is 24 octets: a data offset of 6, so a four-octet option + area, holding a ``SACK`` option that declares 22. The well-formed segment + is checked alongside it, because a guard that rejected real ``SACK`` + options would pass this test on its own. + + """ + import struct + + from pcapkit.const.tcp.option import Option + from pcapkit.protocols.transport.tcp import TCP + from pcapkit.utilities.exceptions import FieldValueError + from tests._support import time_limit + + def segment(data_offset: 'int', options: 'bytes') -> 'bytes': + return struct.pack('!HHIIBBHHH', 1, 2, 0, 0, data_offset << 4, + 0x10, 0, 0, 0) + options + + # one 8-octet block, declared as 10 octets, then two no-operations + good = segment(8, bytes([Option.SACK, 10]) + struct.pack('!II', 1, 2) + b'\x01\x01') + with time_limit(5): + proto = TCP(good, len(good)) + + self.assertEqual(proto.info.hdr_len, len(good)) + self.assertEqual( + [(code, opt.length) for code, opt in proto.info.options.items(multi=True)], + [(Option.SACK, 10), (Option.No_Operation, 1), (Option.No_Operation, 1)], + ) + sack = next(opt for code, opt in proto.info.options.items(multi=True) + if code == Option.SACK) + self.assertEqual([(block.left, block.right) for block in sack.sack], [(1, 2)]) + self.assertEqual(bytes(proto.__header__), good) + + bad = segment(6, bytes([Option.SACK, 22]) + b'\x36\xcc') + with self.assertRaisesRegex(FieldValueError, 'consumed no data'): + with time_limit(5): + TCP(bad, len(bad)) + if __name__ == '__main__': unittest.main() From 3aed5187c407db1bcb2894a90a0d60e27d96eb0c Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 17 Sep 2026 02:37:31 -0400 Subject: [PATCH 4/8] corekit: check an option's progress after the end-of-option-list break, not before (#431) The progress guard added earlier on this branch ran before the loop's `code == self._eool` break, and that turned three protocols' tolerance of a truncated option area into an error. Found by sweeping every option registry for what the exhausted-stream read decodes to. An area declared longer than the octets behind it -- an over-long `ihl` or data offset, or a capture cut short by the snapshot length -- exhausts the stream early, and every field of the next option then reads `b''`, which decodes the type field as **0**. What 0 means is per-registry, and that is the whole story: - IPv4 `EOOL`, TCP `End_of_Option_List`, PCAP-NG `opt_endofopt` and `nrb_record_end` are all 0, and 0 is those registries' `eool`. The break has always absorbed the phantom option and reported the rest of the area as padding, which is how a snaplen-truncated packet parses at all. - HOPOPT, IPv6-Opts and MH read 0 as `Pad1`, HIP as an unassigned parameter and SCTP as a DATA chunk, and all five pass `eool=None` or no `eool` at all. They never reach the break, which is why they spin. So the guard belongs after the break: it then bounds exactly the registries that can spin and leaves the ones that cannot untouched. Measured, on `main` and on this branch before this commit: IPv4(bytes.fromhex('4a00001800010000400600000a0000010a000002'), 20) # ihl=10, 20 octets of options declared, none present # main: parses, options=[EOOL] before: FieldValueError TCP(bytes.fromhex('00501f900000000100000002a002ffff00000000020405b4'), 24) # data offset 10, four option octets present # main: parses, options=[MSS 1460, EOOL] before: FieldValueError Both parse again, to the same options `main` gives them. The captures were byte-identical either way -- none of them carries a truncated option area -- so the dump comparison could not have caught this, and only sweeping the registries did. The sweep also turned up a much simpler reproducer for #431 than the SMF_DPD one, involving no option schema at all: `HOPOPT(b'\x3b\x01', 2)`. Two octets, a declared 14-octet option area with nothing behind it, and `main` spins on the phantom `Pad1` forever. Tests: the truncated area is pinned for HOPOPT and IPv6-Opts in `test_ipv6_extension_unit.py` (raises; hangs on `main`), and the tolerated one for IPv4 in `test_ipv4_unit.py` and TCP in `test_tcp_udp_unit.py` (parses, with the options spelled out; these pass on `main` too, which is the point of them). 867 passed, 17 skipped. Captures still byte-identical to `origin/main`. --- pcapkit/corekit/fields/collections.py | 38 +++++++++++++------ tests/protocols/internet/test_ipv4_unit.py | 28 ++++++++++++++ .../internet/test_ipv6_extension_unit.py | 38 +++++++++++++++++++ .../protocols/transport/test_tcp_udp_unit.py | 31 +++++++++++++++ 4 files changed, 124 insertions(+), 11 deletions(-) diff --git a/pcapkit/corekit/fields/collections.py b/pcapkit/corekit/fields/collections.py index 8f6ba6e1ca..c442d30a0c 100644 --- a/pcapkit/corekit/fields/collections.py +++ b/pcapkit/corekit/fields/collections.py @@ -294,9 +294,8 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'list # Remembering where the previous option ended gives the loop a measure of # progress that does not depend on what a schema reports, and one octet of # it per iteration is what bounds the iteration count. ``length`` is still - # decremented by ``len(data)``, so that no option area which parses today - # parses differently: every option this package can parse consumes at - # least its own type field, so the guard cannot fire on one. + # decremented by ``len(data)``, so that an option area which parses today + # parses identically. offset = file.tell() # make a copy of the ``packet`` dict so that we can include @@ -319,7 +318,31 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'list new_packet[self.name].add(code, data) temp.append(data) - # insist on progress through ``file`` before trusting ``len(data)`` + # update length + length -= len(data) + + # check for EOOL + if code == self._eool: + break + + # NOTE: The progress check comes *after* the end-of-option-list break, + # and that order is not incidental. An area declared longer than the + # octets behind it -- an over-long ``ihl``, or a capture cut short by + # the snapshot length -- exhausts ``file`` early, and the exhausted + # read then decodes the type field as 0. For the IPv4, TCP and PCAP-NG + # registries 0 *is* the end-of-option-list code, so the break above has + # always absorbed that case and reported the rest of the area as + # padding. Checking progress first turned all of those into errors: + # measured on ``IPv4(bytes.fromhex('4a00001800010000400600000a0000010a000002'))``, + # 20 octets of options declared with none present, and on a TCP segment + # with a data offset of 10 and four option octets, both of which parse + # on ``main``. + # + # The registries that spin are the ones where 0 is *not* the + # end-of-option-list code, so they never reach the break: HOPOPT, + # IPv6-Opts and MH read 0 as ``Pad1``, HIP as an unassigned parameter, + # SCTP as a DATA chunk. Those are what this guards, and one octet of + # measured progress per surviving iteration is what bounds the loop. end = file.tell() if end <= offset: raise FieldValueError( @@ -329,12 +352,5 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'list ) offset = end - # update length - length -= len(data) - - # check for EOOL - if code == self._eool: - break - self._option_padding = length return temp diff --git a/tests/protocols/internet/test_ipv4_unit.py b/tests/protocols/internet/test_ipv4_unit.py index 59ed459c91..4e1ac9713d 100644 --- a/tests/protocols/internet/test_ipv4_unit.py +++ b/tests/protocols/internet/test_ipv4_unit.py @@ -833,6 +833,34 @@ def test_ipv4_schema_helpers_and_post_process_branches(self) -> None: self.assertEqual(tuple(unknown.timestamp), (1,)) warn.assert_called_once() + def test_an_option_area_longer_than_the_datagram_still_parses(self) -> None: + """An ``ihl`` promising more options than are there is tolerated. + + The header below sets ``ihl`` to 10 -- a 20-octet option area -- and stops + after the fixed 20 octets, so there are no option octets at all. Reading + past them yields ``b''``, which decodes the option number as 0, and 0 is + IPv4's end-of-option-list, so the option loop breaks there and reports the + whole area as padding. That is how a datagram cut short by the snapshot + length parses at all, and it is why :meth:`OptionField.unpack + ` checks each + option's progress *after* its end-of-option-list break rather than before: + checking first turns every such header into an error. C.f. #431. + + """ + from pcapkit.const.ipv4.option_number import OptionNumber + from pcapkit.protocols.internet.ipv4 import IPv4 + from tests._support import time_limit + + raw = bytes.fromhex('4a00001800010000400600000a0000010a000002') + with time_limit(5): + proto = IPv4(raw, len(raw)) + + self.assertEqual(proto.info.hdr_len, 40) + self.assertEqual( + [(code, opt.length) for code, opt in proto.info.options.items(multi=True)], + [(OptionNumber.EOOL, 1)], + ) + if __name__ == '__main__': unittest.main() diff --git a/tests/protocols/internet/test_ipv6_extension_unit.py b/tests/protocols/internet/test_ipv6_extension_unit.py index 0118474203..20b9044cd5 100644 --- a/tests/protocols/internet/test_ipv6_extension_unit.py +++ b/tests/protocols/internet/test_ipv6_extension_unit.py @@ -1523,6 +1523,44 @@ def test_ipv6_opts_identification_based_dpd_options_parse_from_the_wire(self) -> self._assert_identification_based_dpd_options_parse_from_the_wire(IPv6_Opts) + def _assert_a_truncated_option_area_is_diagnosed(self, protocol_cls: type) -> None: + """An option area with nothing behind it is an error, not a hang. + + The simplest form of #431, and the one that needs no option schema at all: + a header extension length of 1 declares a 14-octet option area, and the + two octets given here are the whole header. The option loop reads ``b''`` + for every field, which decodes the type octet as 0 -- ``Pad1`` in this + registry, not an end-of-option-list -- so it appends a phantom one-octet + ``Pad1``, subtracts the zero octets it actually read, and goes round + again forever. + + Note that this is registry-dependent, and IPv4, TCP and PCAP-NG behave + differently on purpose: 0 is the end-of-option-list code there, so the + loop's ``eool`` break has always absorbed a truncated area and reported + the rest as padding. Their tolerance is pinned in their own tests. + + """ + from pcapkit.utilities.exceptions import FieldValueError + + for name, raw in ( + ('no option octets at all', b'\x3b\x01'), + ('four of fourteen octets', b'\x3b\x01' + b'\x01\x02\x00\x00'), + ): + with self.subTest(case=name): + with self.assertRaisesRegex(FieldValueError, 'consumed no data'): + with time_limit(5): + protocol_cls(raw, len(raw), extension=True) + + def test_hopopt_truncated_option_area_is_diagnosed(self) -> None: + from pcapkit.protocols.internet.hopopt import HOPOPT + + self._assert_a_truncated_option_area_is_diagnosed(HOPOPT) + + def test_ipv6_opts_truncated_option_area_is_diagnosed(self) -> None: + from pcapkit.protocols.internet.ipv6_opts import IPv6_Opts + + self._assert_a_truncated_option_area_is_diagnosed(IPv6_Opts) + def _assert_padding_option_schema_sizes_itself(self, protocol_cls: type) -> None: """The padding option schema, on its own. diff --git a/tests/protocols/transport/test_tcp_udp_unit.py b/tests/protocols/transport/test_tcp_udp_unit.py index 696b86a9ea..28fd8f2520 100644 --- a/tests/protocols/transport/test_tcp_udp_unit.py +++ b/tests/protocols/transport/test_tcp_udp_unit.py @@ -1225,6 +1225,37 @@ def segment(data_offset: 'int', options: 'bytes') -> 'bytes': with time_limit(5): TCP(bad, len(bad)) + def test_an_option_area_longer_than_the_segment_still_parses(self) -> None: + """A data offset promising more options than are there is tolerated. + + The segment below declares a data offset of 10 -- a 20-octet option area -- + and carries four option octets. Reading past them yields ``b''``, which + decodes the option kind as 0, and 0 is TCP's end-of-option-list, so the + option loop breaks there and reports the remaining 16 octets as padding. + That is how a capture cut short by the snapshot length parses at all, and + it is why :meth:`OptionField.unpack + ` checks each + option's progress *after* its end-of-option-list break rather than before: + checking first turns every such segment into an error. + + """ + from pcapkit.const.tcp.option import Option + from pcapkit.protocols.transport.tcp import TCP + from tests._support import time_limit + + raw = bytes.fromhex('00501f900000000100000002a002ffff00000000020405b4') + with time_limit(5): + proto = TCP(raw, len(raw)) + + self.assertEqual(proto.info.hdr_len, 40) + self.assertEqual( + [(code, opt.length) for code, opt in proto.info.options.items(multi=True)], + [(Option.Maximum_Segment_Size, 4), (Option.End_of_Option_List, 1)], + ) + mss = next(opt for code, opt in proto.info.options.items(multi=True) + if code == Option.Maximum_Segment_Size) + self.assertEqual(mss.mss, 1460) + if __name__ == '__main__': unittest.main() From d52d987890eb978992a1ac48164662ec591964c1 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 17 Sep 2026 10:45:43 -0400 Subject: [PATCH 5/8] corekit: count the non-progress diagnostic's offset from the field, not from the stream (#431) Review feedback on #432. Both guards reported `offset` straight from `file.tell()`, which is a position in whatever stream the field is reading and only a position in the *field* when that stream happens to start at zero. It does start at zero on the path these guards were written against, since `Schema.unpack` hands a `ListField` or an `OptionField` a `bytes` buffer -- but both methods are public and take an `IO[bytes]`, and a `SchemaField` passes the live file straight down. Off that path the number named an offset outside the field it was measuring: a three-octet option area read from a stream two octets in reported `at offset 5 of 3`. - Remember where the field begins and subtract it in the message, at both sites, so the two agree and the number is bounded by the length beside it. - The comparison keeps raw stream positions; only the diagnostic is relative. Progress is a fact about the stream, and rebasing the arithmetic would have been a second change dressed up as a wording fix. Tests: `tests/protocols/schema/test_schema_unit.py` calls both fields on a stream positioned two octets in and pins the relative offsets -- `at offset 3 of 3` for the option area and `item 2 at offset 2 of 8` for the list. Before this commit they read `offset 5 of 3` and `offset 4 of 8`. The `ListField` test also covers that guard on its own for the first time; it had only been exercised through the TCP `SACK` segment in `test_tcp_udp_unit.py`. --- pcapkit/corekit/fields/collections.py | 23 ++++-- tests/protocols/schema/test_schema_unit.py | 95 +++++++++++++++++++++- 2 files changed, 109 insertions(+), 9 deletions(-) diff --git a/pcapkit/corekit/fields/collections.py b/pcapkit/corekit/fields/collections.py index 94a4bb72e4..f7de9cbe4b 100644 --- a/pcapkit/corekit/fields/collections.py +++ b/pcapkit/corekit/fields/collections.py @@ -153,7 +153,13 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'byte # Remembering where the previous item ended is what bounds the iteration # count, since it does not depend on what the schema reports. C.f. #431, # which is the same defect in the ``OptionField`` subclass. - offset = file.tell() + # + # ``start`` is where the field itself begins. The comparison needs stream + # positions, but the diagnostic wants an offset into the field, and the two + # only coincide when the field happens to be reading from the front of its + # stream -- which it does when handed a :obj:`bytes` buffer and does not + # when handed a live file. + start = offset = file.tell() temp = [] # type: list[_TL] while length > 0: @@ -166,8 +172,9 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'byte if end <= offset: raise FieldValueError( f'Field {self.name} has an item that consumed no data: ' - f'item {len(temp)} at offset {offset} of {self._length}, ' - f'with {length} octet(s) of the field left to parse' + f'item {len(temp)} at offset {offset - start} of ' + f'{self._length}, with {length} octet(s) of the field ' + f'left to parse' ) offset = end @@ -348,7 +355,13 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'list # it per iteration is what bounds the iteration count. ``length`` is still # decremented by ``len(data)``, so that an option area which parses today # parses identically. - offset = file.tell() + # + # ``start`` is where the option area itself begins. The comparison needs + # stream positions, but the diagnostic wants an offset into the area, and + # the two only coincide when the field happens to be reading from the front + # of its stream -- which it does when handed a :obj:`bytes` buffer and does + # not when handed a live file. + start = offset = file.tell() # make a copy of the ``packet`` dict so that we can include # parsed option schema in the ``packet`` dict @@ -431,7 +444,7 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'list if end <= offset: raise FieldValueError( f'Field {self.name} has an option that consumed no data: ' - f'{code!r} at offset {offset} of {self._length}, with ' + f'{code!r} at offset {offset - start} of {self._length}, with ' f'{length} octet(s) of the option area left to parse' ) offset = end diff --git a/tests/protocols/schema/test_schema_unit.py b/tests/protocols/schema/test_schema_unit.py index d1de232828..e8473ba835 100644 --- a/tests/protocols/schema/test_schema_unit.py +++ b/tests/protocols/schema/test_schema_unit.py @@ -3,6 +3,7 @@ import collections import enum import importlib.util +import io import unittest from unittest import mock @@ -425,12 +426,100 @@ def test_schema_option_field_unpack_rejects_an_option_consuming_nothing(self) -> fail, it hangs the run. """ - from pcapkit.corekit.fields.collections import OptionField + from pcapkit.utilities.exceptions import FieldValueError + + schema = self._make_wrapped_options_schema() + + with self.assertRaisesRegex(FieldValueError, 'consumed no data'): + with time_limit(5): + schema.unpack(b'\x01\xff\x00', 3, {}) + + def test_schema_option_field_unpack_reports_a_field_relative_offset(self) -> None: + """The offset in the diagnostic counts from the option area, not the stream. + + :meth:`OptionField.unpack + ` is handed a + :obj:`bytes` buffer by + :meth:`Schema.unpack `, so + its stream starts at zero and the two readings coincide -- but the method + is public and takes an ``IO[bytes]`` as well, and a + :class:`~pcapkit.corekit.fields.misc.SchemaField` hands the live file + straight down. Reporting the raw stream position then names an offset + outside the field: the same three-octet area that fails at offset 3 below + reported offset 5 when the stream was two octets in. + + """ + from pcapkit.utilities.exceptions import FieldValueError + + schema = self._make_wrapped_options_schema() + field = schema.__fields__['options'] + + stream = io.BytesIO(b'\xde\xad' + b'\x01\xff\x00') + stream.seek(2) + + with self.assertRaisesRegex(FieldValueError, r'at offset 3 of 3\b'): + with time_limit(5): + field.unpack(stream, {}) + + def test_schema_list_field_unpack_rejects_a_schema_item_consuming_nothing(self) -> None: + """A list of schema items must be advanced past too, or reported. + + :meth:`ListField.unpack + ` sizes a schema item + by ``len(data)`` in the same way, so a budget larger than the octets behind + it leaves the item schema reading an exhausted stream, recording nothing, + and subtracting nothing. Reachable from a TCP segment whose ``SACK`` option + declares more octets than the option area holds, which + :file:`tests/protocols/transport/test_tcp_udp_unit.py` pins; this covers + the field on its own, and pins the offset as a count into the field rather + than into the stream -- the same reading as its ``OptionField`` subclass. + + """ + from pcapkit.corekit.fields.collections import ListField from pcapkit.corekit.fields.misc import SchemaField from pcapkit.corekit.fields.numbers import UInt8Field from pcapkit.protocols.schema.schema import Schema, schema_final from pcapkit.utilities.exceptions import FieldValueError + @schema_final + class Marker(Schema): + type: int = UInt8Field(default=0) + + @schema_final + class MarkerListSchema(Schema): + #: Eight octets of budget, however few are really there. + markers: list[Marker] = ListField( + length=8, + item_type=SchemaField(length=2, schema=Marker), + ) + + field = MarkerListSchema.__fields__['markers'] + + stream = io.BytesIO(b'\xde\xad' + b'\x01\x02') + stream.seek(2) + + with self.assertRaisesRegex(FieldValueError, r'item 2 at offset 2 of 8\b'): + with time_limit(5): + field.unpack(stream, {}) + + def _make_wrapped_options_schema(self): + """A three-octet option area whose first option over-reads by one octet. + + Returns: + A :class:`~pcapkit.protocols.schema.schema.Schema` subclass carrying a + single :class:`~pcapkit.corekit.fields.collections.OptionField`. + + ``Wrapper`` is the smallest thing that separates the octets an option takes + from the stream from the ``len(data)`` it reports, and is the shape every + wrapper option schema in the package has: it reads two octets and returns a + nested schema that recorded one. + + """ + from pcapkit.corekit.fields.collections import OptionField + from pcapkit.corekit.fields.misc import SchemaField + from pcapkit.corekit.fields.numbers import UInt8Field + from pcapkit.protocols.schema.schema import Schema, schema_final + @schema_final class Marker(Schema): type: int = UInt8Field(default=0) @@ -451,9 +540,7 @@ class WrappedOptionsSchema(Schema): registry=collections.defaultdict(lambda: Marker, {1: Wrapper}), ) - with self.assertRaisesRegex(FieldValueError, 'consumed no data'): - with time_limit(5): - WrappedOptionsSchema.unpack(b'\x01\xff\x00', 3, {}) + return WrappedOptionsSchema if __name__ == '__main__': From 7e9f9a135f5a094ddd531e8c6a74442098aa961d Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 17 Sep 2026 11:36:23 -0400 Subject: [PATCH 6/8] tests: put back the alarm `time_limit` displaced, instead of cancelling it (#431) Review feedback on #432. A process has one pending alarm, so `signal.alarm` does not add a deadline, it *replaces* one -- and `time_limit` discarded the seconds `signal.alarm(seconds)` returned, then called `signal.alarm(0)` unconditionally on the way out. Anything already scheduled was therefore cancelled and never put back. Measured: an enclosing `signal.alarm(30)` reads back as `0` after the `with`, so the outer deadline is silently gone -- and a helper whose whole purpose is that a hang fails rather than wedges was quietly removing other people's protection against exactly that. Nothing in the suite nests them today, but nothing stops it either. - Keep the return value of `signal.alarm(seconds)`; it is the only record of what was displaced and cannot be asked for again afterwards. - Re-arm it in the `finally` with the time spent in the body deducted, so an enclosing deadline keeps counting down across the `with` rather than restarting. - The existing cancel-then-restore order is unchanged and still deliberate: the alarm is cancelled before the handler is swapped back, so one firing in between cannot reach the old handler, and the re-armed alarm belongs to that handler rather than to `expire`. An enclosing deadline that *expired* while the body ran cannot be delivered when it was due -- the body held the process past that moment -- so it is re-armed for one second rather than cancelled. Honouring it a moment late is the lesser wrong; cancelling is how the outer timeout goes missing altogether. The same clamp covers an enclosing deadline shorter than `seconds`, which this one necessarily overran. Said in the docstring, since it is a decision rather than an implementation detail. Tests: `tests/test_support_helpers.py` gains `TimeLimitTests` -- an enclosing alarm survives with the handler restored, one overtaken by the body is re-armed rather than dropped, nothing is left pending when nothing was, and the deadline still fires on a body that overruns. That last one matters: without it the other three could be satisfied by never arming anything. The first two fail against the previous helper with `0 not greater than 0` and `0 not greater than or equal to 1`; the other two pass either way, which is the point of them. The module docstring said it covered `close_extractor`; it now says it covers the helpers in `tests._support`, of which that is one. --- tests/_support.py | 37 +++++++++++++-- tests/test_support_helpers.py | 87 ++++++++++++++++++++++++++++++++++- 2 files changed, 117 insertions(+), 7 deletions(-) diff --git a/tests/_support.py b/tests/_support.py index e7be92c219..94b8445719 100644 --- a/tests/_support.py +++ b/tests/_support.py @@ -5,9 +5,11 @@ import contextlib import importlib.util import inspect +import math import pathlib import signal import sys +import time import types import unittest from typing import Iterable, Iterator @@ -33,6 +35,20 @@ def time_limit(seconds: int = 5) -> Iterator[None]: :data:`signal.SIGTERM` from an outer :program:`timeout`, which such a loop likewise never gets around to handling. + There is only ever one pending alarm per process, so arming this one cancels + whatever was already scheduled -- an enclosing ``time_limit``, or a deadline the + test runner set for itself. Both the handler and that pending alarm are put back + on the way out, the alarm with the seconds spent in the body deducted, so an + enclosing deadline keeps counting down across the ``with`` rather than being + silently dropped. + + An enclosing deadline that *expired* while the body ran cannot be delivered at + the moment it was due, since the body held the process until then. It is + re-armed for one second instead of cancelled: honouring it a moment late is the + lesser wrong, and cancelling it is how the enclosing timeout goes missing + altogether. The same clamp applies when the enclosing deadline was shorter than + ``seconds`` and this one therefore fired first. + Args: seconds: Whole seconds to allow the body. :func:`signal.alarm` counts in whole seconds, so this cannot usefully be fractional. @@ -53,15 +69,26 @@ def time_limit(seconds: int = 5) -> Iterator[None]: def expire(signum: int, frame: object) -> None: raise TimeoutError(f'did not finish within {seconds}s') - previous = signal.signal(signal.SIGALRM, expire) - signal.alarm(seconds) + previous_handler = signal.signal(signal.SIGALRM, expire) + + # NOTE: ``signal.alarm`` returns the seconds left on the alarm it replaces, or + # zero when there was none. That return value is the only record of an + # enclosing deadline, so it is read here rather than discarded -- there is no + # way to ask for it again afterwards. + pending = signal.alarm(seconds) + started = time.monotonic() try: yield finally: - # Cancel before restoring, so that an alarm which fires between the two - # cannot be delivered to whatever handler was installed before. + # Cancel first, so that an alarm which fires between here and the handler + # being restored cannot be delivered to whatever handler was installed + # before -- and so that the alarm re-armed below belongs to that handler + # rather than to ``expire``. signal.alarm(0) - signal.signal(signal.SIGALRM, previous) + signal.signal(signal.SIGALRM, previous_handler) + if pending: + left = pending - (time.monotonic() - started) + signal.alarm(max(1, math.ceil(left))) def sample_path(name: str) -> str: diff --git a/tests/test_support_helpers.py b/tests/test_support_helpers.py index 4b45c0dc16..d159e5baae 100644 --- a/tests/test_support_helpers.py +++ b/tests/test_support_helpers.py @@ -1,5 +1,11 @@ # -*- coding: utf-8 -*- -"""Tests for :func:`tests._support.close_extractor`. +"""Tests for the helpers in :mod:`tests._support`. + +Two of them are pinned here, both for the same reason: they are *machinery* the +rest of the suite leans on, so a fault in either reports itself as a failure in +whichever test happened to be running rather than as a fault in the helper. +:func:`~tests._support.time_limit` is covered by :class:`TimeLimitTests` at the +end; the rest of the module is :func:`~tests._support.close_extractor`. That helper is teardown machinery: nearly every runtime and integration test hands it an extractor from ``addCleanup`` or a ``finally`` block. Teardown code @@ -24,9 +30,11 @@ """ from __future__ import annotations +import signal +import time import unittest -from tests._support import close_extractor +from tests._support import close_extractor, time_limit class Closeable: @@ -169,5 +177,80 @@ def test_base_exception_is_not_swallowed(self) -> None: close_extractor(Extractor(Closeable(KeyboardInterrupt()), Closeable())) +@unittest.skipUnless(hasattr(signal, 'SIGALRM'), 'signal.alarm is unavailable') +class TimeLimitTests(unittest.TestCase): + """A deadline that arrives, and an enclosing one that survives. + + A process has one pending alarm, so arming a deadline cancels whatever was + already scheduled. The helper reads what it displaced and puts it back; these + pin that, because an enclosing deadline going missing is invisible until the + run it should have bounded hangs instead. + + """ + + def setUp(self) -> None: + # Whatever a test leaves behind, the next one starts from nothing pending + # and from a handler this class owns rather than the helper's. + self.handled = [] # type: list[int] + previous = signal.signal(signal.SIGALRM, lambda signum, frame: self.handled.append(signum)) + self.addCleanup(signal.signal, signal.SIGALRM, previous) + self.addCleanup(signal.alarm, 0) + + def test_the_deadline_fires_on_a_body_that_overruns(self) -> None: + """The point of the helper, pinned so the rest cannot be met by disarming.""" + with self.assertRaises(TimeoutError): + with time_limit(1): + while True: + pass + + def test_an_enclosing_alarm_is_restored(self) -> None: + """An outer deadline keeps counting down across the ``with``. + + Before this was fixed the helper cancelled the pending alarm on the way out + and never re-armed it, so an outer ``signal.alarm(30)`` read back as ``0`` + afterwards: the enclosing deadline was gone, silently. + + """ + own_handler = signal.getsignal(signal.SIGALRM) + + signal.alarm(30) + with time_limit(5): + pass + + # Reading the remaining seconds cancels the alarm, which is the cleanup + # this test wanted anyway. + remaining = signal.alarm(0) + + self.assertGreater(remaining, 0) + self.assertLessEqual(remaining, 30) + self.assertIs(signal.getsignal(signal.SIGALRM), own_handler) + self.assertEqual(self.handled, []) + + def test_an_enclosing_alarm_that_expired_in_the_body_is_re_armed(self) -> None: + """An outer deadline overtaken by the body is honoured late, not dropped. + + The body holds the process past the moment the outer alarm was due, so it + cannot be delivered on time. The helper re-arms it for a second rather than + cancelling it, since cancelling is how an outer timeout goes missing + altogether. + + """ + signal.alarm(1) + with time_limit(5): + time.sleep(1.2) + + remaining = signal.alarm(0) + + self.assertGreaterEqual(remaining, 1) + self.assertEqual(self.handled, []) + + def test_nothing_is_re_armed_when_nothing_was_pending(self) -> None: + """The common case: no enclosing deadline, nothing left behind.""" + with time_limit(5): + pass + + self.assertEqual(signal.alarm(0), 0) + + if __name__ == '__main__': unittest.main() From 4124d3784f98fa1de51fc208c40bc914fcda7bb6 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 17 Sep 2026 11:36:36 -0400 Subject: [PATCH 7/8] corekit: name the failing list item as a count, not as an ordinal (#431) Review feedback on #432. `ListField.unpack`'s non-progress diagnostic read `item {len(temp)}`, and `len(temp)` is the number of items already parsed -- a 0-based index sitting behind a word that reads as a 1-based ordinal. So `item 2` denoted the *third* item, which is the wrong thing for whoever is reading the error to conclude. Reworded to `after 2 item(s), at offset 2 of 8`: a count of what was parsed, with no convention left for a reader to guess at. Picking a base instead would have made the number correct only for readers who knew which base had been picked. The `OptionField` message keeps naming the option code in that slot, so the two stay consistent -- there is no index there to be read either way. The review suggested `len(temp) + 1`. That fixes the ambiguity by choosing the 1-based reading, but it also moves the number, so it would have to move the expectation in `test_schema_unit.py` to `item 3` in the same commit; the review's claim that the *current* code fails that test is wrong, since the test seeks two octets in, parses two items, and fails on the third with `len(temp) == 2`. On the TCP `SACK` segment that started this, the message now reads: `Field sack has an item that consumed no data: after 1 item(s), at offset 2 of 20, with 18 octet(s) of the field left to parse` -- one block read off the two octets there were, the second finding nothing. Tests: the `ListField` expectation in `tests/protocols/schema/test_schema_unit.py` moves to `after 2 item\(s\), at offset 2 of 8`, and its docstring now says why 2 is a count rather than an ordinal, so the next reader does not have to re-derive it. 893 passed, 17 skipped. Captures still byte-identical to `origin/main`. --- pcapkit/corekit/fields/collections.py | 8 +++++++- tests/protocols/schema/test_schema_unit.py | 6 +++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/pcapkit/corekit/fields/collections.py b/pcapkit/corekit/fields/collections.py index f7de9cbe4b..4465eac4c2 100644 --- a/pcapkit/corekit/fields/collections.py +++ b/pcapkit/corekit/fields/collections.py @@ -170,9 +170,15 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'byte end = file.tell() if end <= offset: + # NOTE: ``len(temp)`` counts the items already parsed, so it + # names the failing one as a count rather than as an ordinal -- + # "after 2 item(s)" rather than "item 2", which would read as + # the second item when it is the third. The ``OptionField`` + # message below names the option code in this slot and so has + # no index to be read either way. raise FieldValueError( f'Field {self.name} has an item that consumed no data: ' - f'item {len(temp)} at offset {offset - start} of ' + f'after {len(temp)} item(s), at offset {offset - start} of ' f'{self._length}, with {length} octet(s) of the field ' f'left to parse' ) diff --git a/tests/protocols/schema/test_schema_unit.py b/tests/protocols/schema/test_schema_unit.py index e8473ba835..9162714cd5 100644 --- a/tests/protocols/schema/test_schema_unit.py +++ b/tests/protocols/schema/test_schema_unit.py @@ -474,6 +474,10 @@ def test_schema_list_field_unpack_rejects_a_schema_item_consuming_nothing(self) the field on its own, and pins the offset as a count into the field rather than into the stream -- the same reading as its ``OptionField`` subclass. + Two items parse off the two octets below and the third finds the stream + exhausted, so the diagnostic's ``after 2 item(s)`` is a count of what was + parsed rather than an ordinal naming the second item. + """ from pcapkit.corekit.fields.collections import ListField from pcapkit.corekit.fields.misc import SchemaField @@ -498,7 +502,7 @@ class MarkerListSchema(Schema): stream = io.BytesIO(b'\xde\xad' + b'\x01\x02') stream.seek(2) - with self.assertRaisesRegex(FieldValueError, r'item 2 at offset 2 of 8\b'): + with self.assertRaisesRegex(FieldValueError, r'after 2 item\(s\), at offset 2 of 8\b'): with time_limit(5): field.unpack(stream, {}) From 7a73ee35ffcf692aa7dc0c167457feba607b5b82 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 17 Sep 2026 12:27:38 -0400 Subject: [PATCH 8/8] tests: say why an enclosing deadline is late -- this helper displaced it, not the body --- tests/_support.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tests/_support.py b/tests/_support.py index 94b8445719..a431bf9564 100644 --- a/tests/_support.py +++ b/tests/_support.py @@ -42,12 +42,14 @@ def time_limit(seconds: int = 5) -> Iterator[None]: enclosing deadline keeps counting down across the ``with`` rather than being silently dropped. - An enclosing deadline that *expired* while the body ran cannot be delivered at - the moment it was due, since the body held the process until then. It is - re-armed for one second instead of cancelled: honouring it a moment late is the - lesser wrong, and cancelling it is how the enclosing timeout goes missing - altogether. The same clamp applies when the enclosing deadline was shorter than - ``seconds`` and this one therefore fired first. + An enclosing deadline whose moment falls inside the body is not delivered on + time, and the reason is this helper rather than the body: arming an alarm + *replaces* the pending one, so the enclosing deadline was already cancelled + before the body began and there was nothing left to fire when it came due. It + is re-armed for one second on the way out rather than dropped -- honouring it + late is the lesser wrong, and dropping it is how an enclosing timeout goes + missing altogether. That one-second floor covers every case where the body ran + for longer than the enclosing deadline had left. Args: seconds: Whole seconds to allow the body. :func:`signal.alarm` counts in