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