From c5ddaf73095c4a20bb227cc49044586aed80c9da Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 2 Nov 2026 18:11:44 -0500 Subject: [PATCH] fix(corekit): give back the octets read when a zero-filled list fails (#2705) - _zero_fill_items debited the zeros from the ledger's octets read before the items were parsed and never restored them when an item raised, so one failed list lowered the allowance of every later cut in the context. It is now the context manager _zero_filled_items, ListField.unpack reads the items inside it, and a failure sets the octets read back; the fill stays charged. - Trimmed the NOTE in HTTPv2._make_data to two lines. The corekit and application modules pass; the new test fails without the restore. Closes #2705 --- pcapkit/corekit/fields/collections.py | 35 +++++++++++++++---- pcapkit/corekit/fields/field.py | 20 +++++++---- pcapkit/protocols/application/httpv2.py | 6 ++-- .../test_fields_list_zero_fill_2686_unit.py | 31 ++++++++++++++++ 4 files changed, 74 insertions(+), 18 deletions(-) diff --git a/pcapkit/corekit/fields/collections.py b/pcapkit/corekit/fields/collections.py index d6eb8b411b..d5702818e9 100644 --- a/pcapkit/corekit/fields/collections.py +++ b/pcapkit/corekit/fields/collections.py @@ -4,7 +4,7 @@ import io from typing import TYPE_CHECKING, Generic, TypeVar, cast -from pcapkit.corekit.fields.field import FieldBase, _zero_fill_items +from pcapkit.corekit.fields.field import FieldBase, _zero_filled_items from pcapkit.corekit.fields.misc import SchemaField from pcapkit.corekit.fields.numbers import NumberField from pcapkit.corekit.multidict import OrderedMultiDict @@ -175,8 +175,8 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'byte Raises: FieldValueError: If the items overrun the field, if a schema item - consumes nothing from ``buffer`` -- see the note below -- or if - schema items short of the field's length cannot be zero-filled. + consumes nothing from ``buffer``, or if schema items short of the + field's length cannot be zero-filled. When ``buffer`` falls short of the field's length, schema items are read from its octets zero-filled to that length, as @@ -212,7 +212,7 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'byte # before, from the first one -- is still an instance of the original. # ``isinstance`` against the new class would report :data:`False` for an # item that plainly is a ``SchemaField``, ``is_schema`` would go the - # wrong way, and this method's other branch would bill each item by its + # wrong way, and the item loop's other branch would bill each item by its # *declared* length instead of by what it actually consumed -- so a # malformed TCP SACK option's length check could raise # :exc:`~pcapkit.utilities.exceptions.FieldValueError` from here instead @@ -231,13 +231,34 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'byte present = file.seek(0, io.SEEK_END) - start file.seek(start, io.SEEK_SET) if present < length: - file = io.BytesIO(_zero_fill_items(self.name, length, file.read())) + with _zero_filled_items(self.name, length, file.read()) as filled: + return self._unpack_items(io.BytesIO(filled), packet, is_schema) + return self._unpack_items(file, packet, is_schema) + + def _unpack_items(self, file: 'IO[bytes]', packet: 'dict[str, Any]', is_schema: 'bool') -> 'list[_TL]': + """Unpack the field's items from ``file``. + + Args: + file: Stream the items are read from. + packet: Packet data. + is_schema: Whether the items are schemas. + + Returns: + Unpacked items. + + Raises: + FieldValueError: If the items overrun the field, or if a schema item + consumes nothing from ``file`` -- see the note below. + + """ + length = self._length + item_type = cast('FieldBase', self._item_type) # 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 that reads nothing # records nothing, so ``length -= len(data)`` makes no progress and the - # loop spins forever. The zero-fill above gives every item octets to read, + # loop spins forever. The zero-fill in ``unpack`` gives every item octets, # but an item schema may still consume none of them. Remembering where the # previous item ended is what bounds the iteration count, since it does # not depend on what the schema reports. Cf. #431, which is the same @@ -252,7 +273,7 @@ def unpack(self, buffer: 'bytes | IO[bytes]', packet: 'dict[str, Any]') -> 'byte temp = [] # type: list[_TL] while length > 0: - field = self._item_type(packet) + field = item_type(packet) if is_schema: data = cast('SchemaField', field).unpack(file, packet) diff --git a/pcapkit/corekit/fields/field.py b/pcapkit/corekit/fields/field.py index 164528be47..924a1b3ac0 100644 --- a/pcapkit/corekit/fields/field.py +++ b/pcapkit/corekit/fields/field.py @@ -83,7 +83,7 @@ #: *not* weighed against it: a shortfall within :data:`~pcapkit.utilities.config.MAX_ZERO_PAD_SHORTFALL` #: neither consults this ledger nor is charged to it, which is what keeps every #: legitimate short read answering the same however much preceded it. Only the -#: 32-bit band above reads and writes ``[1]``, and :func:`_zero_fill_items`, whose +#: 32-bit band above reads and writes ``[1]``, and :func:`_zero_filled_items`, whose #: fill of a list of schema items has no free band (:issue:`2686`). _zero_pad_ledger = contextvars.ContextVar( 'pcapkit.corekit.fields.field._zero_pad_ledger', @@ -119,21 +119,22 @@ def _zero_pad_budget() -> 'Iterator[list[int]]': _zero_pad_ledger.reset(token) -def _zero_fill_items(name: 'str', length: 'int', data: 'bytes') -> 'bytes': +@contextlib.contextmanager +def _zero_filled_items(name: 'str', length: 'int', data: 'bytes') -> 'Iterator[bytes]': """Zero-fill the octets of a list of schema items to its declared width. :meth:`FieldBase.unpack` reads a field the data ends inside as though its missing octets were zeros, and :meth:`ListField.unpack ` gives a list of schema items the same reading here, so its items cover the width it - declares (:issue:`2686`). + declares (:issue:`2686`). The items are read inside the block. Args: name: Field name. length: Octets the field declares. data: Octets actually present, fewer than ``length``. - Returns: + Yields: ``data``, zero-filled to ``length``. Raises: @@ -172,10 +173,15 @@ def _zero_fill_items(name: 'str', length: 'int', data: 'bytes') -> 'bytes': ) ledger[1] += padding - # NOTE: The items credit the zeros they read as octets supplied, so they are - # taken off in advance and the fill earns no allowance of its own. + # NOTE: The items credit the zeros they read as octets supplied, so they are taken off in + # advance; items that fail credit an unknown share, so the octets read go back as they were. + read = ledger[0] ledger[0] -= padding - return data + bytes(padding) + try: + yield data + bytes(padding) + except BaseException: + ledger[0] = read + raise #: Pattern[str]: Matches a leading, optionally byte-order-prefixed negative diff --git a/pcapkit/protocols/application/httpv2.py b/pcapkit/protocols/application/httpv2.py index 4afffc3abd..f0aa8242c1 100644 --- a/pcapkit/protocols/application/httpv2.py +++ b/pcapkit/protocols/application/httpv2.py @@ -604,10 +604,8 @@ def _make_data(cls, data: 'Data_HTTP') -> 'dict[str, Any]': # type: ignore[over Key-value pairs for protocol construction. """ - # NOTE: No ``length``: :meth:`make` computes the frame's Length from the - # frame it builds, and a ``length`` keyword would instead be taken by - # ``ProtocolBase.__init__`` as the number of octets to parse back -- - # the payload length alone, nine octets short of the frame. + # NOTE: No ``length``: ``make`` computes the Length from the frame it builds, and + # ``ProtocolBase.__init__`` would take a ``length`` as the octets to parse back. return { 'type': data.type, 'flags': data.flags.__value__ if data.flags is not None else 0, diff --git a/tests/corekit/test_fields_list_zero_fill_2686_unit.py b/tests/corekit/test_fields_list_zero_fill_2686_unit.py index 94b7ecb793..a941cb002e 100644 --- a/tests/corekit/test_fields_list_zero_fill_2686_unit.py +++ b/tests/corekit/test_fields_list_zero_fill_2686_unit.py @@ -121,6 +121,37 @@ def test_the_zeros_earn_no_allowance(self) -> None: self.field(ratio * 6 + 1).unpack(b'', {}) self.assertEqual(ledger, [6, ratio * 6]) + def test_a_failed_fill_gives_back_the_octets_read(self) -> None: + """Items that fail inside the fill leave the octets read as they were, so one + bad list does not refuse the later cuts of its context; the fill stays charged. + Only a failure restores them: a later fill that succeeds is counted as usual.""" + from pcapkit.corekit.fields.numbers import UInt16Field + from pcapkit.protocols.schema.schema import Schema, schema_final + from pcapkit.utilities.exceptions import ProtocolError + + @schema_final + class NonZero(Schema): + """A 16-bit value that refuses to be zero once parsed.""" + + value: int = UInt16Field() + + def post_process(self, packet: 'dict') -> 'Schema': + if self.value == 0: + raise ProtocolError('a zero item') + return self + + field = self.ListField(length=12, item_type=self.SchemaField(length=2, schema=NonZero)) + with self.budget() as ledger: + self.field(12).unpack(bytes(12), {}) + self.assertEqual(ledger, [12, 0]) + with self.assertRaisesRegex(ProtocolError, 'a zero item'): + field.unpack(b'\x00\x01', {}) + self.assertEqual(ledger, [12, 10]) + + # a fill that succeeds keeps the octets its items read, as before the failure + self.field(12).unpack(bytes(2), {}) + self.assertEqual(ledger, [14, 20]) + def test_a_fresh_context_gets_a_ledger_of_its_own(self) -> None: """A fill that is the first read of a context starts that context's ledger.""" import contextvars