diff --git a/pcapkit/protocols/schema/schema.py b/pcapkit/protocols/schema/schema.py index 8a61b59a2c..9c0546c67f 100644 --- a/pcapkit/protocols/schema/schema.py +++ b/pcapkit/protocols/schema/schema.py @@ -635,6 +635,10 @@ class will consider negative value as a placeholder. field = field.field(packet) if isinstance(field, ForwardMatchField): + # NOTE: a forward match consumes nothing, so it contributes no + # octets to ``bytes(self)``/``len(self)`` either. :meth:`unpack` + # mirrors this for the same reason -- see the ``ForwardMatchField`` + # branch there. See #446. self.__buffer__[field.name] = b'' continue @@ -756,7 +760,23 @@ def unpack(cls, data: 'bytes | IO[bytes]', packet['__option_padding__'] = field.option_padding if isinstance(field, ForwardMatchField): + # NOTE: a forward match reads ``length`` octets so a later field + # can size itself from them, but consumes neither the stream + # (the rewind below) nor ``__length__`` (no decrement in this + # branch). ``self.__buffer__[field.name]`` above still holds the + # octets just read, though, and until here nothing undid that: + # ``__bytes__``/``__len__`` concatenate every slot in + # ``__buffer__``, so the schema over-reported its length by + # exactly the forward match's width -- the same octets are read + # again, for real, by whichever field actually needs them, so + # nothing is lost by dropping the duplicate here. ``pack()`` + # above already zeroes this slot for the same field type; + # zeroing it here as well is what makes a declared area checked + # against ``len(self)`` -- :class:`~pcapkit.corekit.fields.collections.OptionField` + # and :class:`~pcapkit.corekit.fields.collections.ListField` both + # do this -- see the octets actually consumed. See #446. data.seek(-length, io.SEEK_CUR) + self.__buffer__[field.name] = b'' elif isinstance(field, OptionField) and field.option_padding > 0: # the option list ended before the declared field length was # exhausted; give the unconsumed remainder back to ``data`` diff --git a/tests/protocols/schema/test_schema_unit.py b/tests/protocols/schema/test_schema_unit.py index 9162714cd5..70a45bd7e3 100644 --- a/tests/protocols/schema/test_schema_unit.py +++ b/tests/protocols/schema/test_schema_unit.py @@ -506,6 +506,104 @@ class MarkerListSchema(Schema): with time_limit(5): field.unpack(stream, {}) + def _make_previewed_item_schema(self): + """A schema whose length is peeked before it is read for real. + + Returns: + A :class:`~pcapkit.protocols.schema.schema.Schema` subclass with a + :class:`~pcapkit.corekit.fields.misc.ForwardMatchField` that previews + the very octet ``length`` then reads again for real -- the shape + :class:`~pcapkit.protocols.schema.internet.mh.CGAParameter`'s + ``public_key_test`` has, minimised to one octet. ``length_peek`` + consumes nothing from the stream (:meth:`Schema.unpack + ` rewinds past it), so + an instance built from ``b'\\x02AB'`` reads 3 octets off the wire -- + not 4 -- and :meth:`Schema.__len__ + ` is expected to agree. + + """ + from pcapkit.corekit.fields.misc import ForwardMatchField + from pcapkit.corekit.fields.numbers import UInt8Field + from pcapkit.corekit.fields.strings import BytesField + from pcapkit.protocols.schema.schema import Schema, schema_final + + @schema_final + class PreviewedItem(Schema): + #: Non-consuming preview of ``length``, read again below. + length_peek: int = ForwardMatchField(UInt8Field(default=0)) + #: The same octet, read for real this time. + length: int = UInt8Field(default=0) + #: Sized from the peeked (and re-read) ``length``. + data: bytes = BytesField(length=lambda pkt: pkt['length'], default=b'') + + return PreviewedItem + + def test_forward_match_field_does_not_count_toward_length(self) -> None: + """A ``ForwardMatchField`` reads octets but must not be billed for them. + + Before the fix, :meth:`Schema.unpack + ` kept the octets a + :class:`~pcapkit.corekit.fields.misc.ForwardMatchField` read in + ``__buffer__`` even though it rewinds the stream past them, so + ``len(schema)`` double-counted them: the same octet is read once by + ``length_peek`` (kept in the buffer) and again for real by ``length`` + (also kept), so a 3-octet input reported length 4. See #446. + + """ + PreviewedItem = self._make_previewed_item_schema() + + unpacked = PreviewedItem.unpack(b'\x02AB', 3, {}) + + self.assertEqual(unpacked.length, 2) + self.assertEqual(unpacked.data, b'AB') + # 1 octet for ``length_peek``/``length`` together (not 2, one per + # field) plus 2 octets of ``data`` -- the input's own 3 octets, not the + # 4 a double-counted forward match would report. + self.assertEqual(len(unpacked), 3) + self.assertEqual(bytes(unpacked), b'\x02AB') + + def test_schema_list_field_rejects_a_declared_area_that_a_forward_match_over_reports(self) -> None: + """The failure mode #446 is about: a correct declared area, rejected. + + Two ``PreviewedItem``s take two octets each off the wire -- four in + total -- and :class:`~pcapkit.corekit.fields.collections.ListField` + is given exactly that as its declared ``length``. Before the fix, each + item's over-reported ``len(data)`` (3, not 2) drains the budget one + octet too fast: ``4 - 3 = 1`` after the first item, then ``1 - 3 = -2`` + on the second, and :meth:`ListField.unpack + ` raises + ``FieldValueError`` on input that is exactly the right length. This is + the same mechanism that fails + :class:`~pcapkit.protocols.schema.internet.mh.CGAParametersOption`'s + ``parameters`` :class:`~pcapkit.corekit.fields.collections.ListField` + of :class:`~pcapkit.protocols.schema.internet.mh.CGAParameter` items -- + each carrying its own load-bearing ``public_key_test`` + ``ForwardMatchField`` -- with the identical + ``FieldValueError: Field parameters has invalid length.``, minimised so + it needs neither a CGA parameter nor its option registry. + + """ + from pcapkit.corekit.fields.collections import ListField + from pcapkit.corekit.fields.misc import SchemaField + from pcapkit.protocols.schema.schema import Schema, schema_final + + PreviewedItem = self._make_previewed_item_schema() + + @schema_final + class PreviewedItemListSchema(Schema): + #: Four octets of budget, exactly what two ``PreviewedItem``s take. + markers: list[PreviewedItem] = ListField( # type: ignore[valid-type] + length=4, + item_type=SchemaField(length=2, schema=PreviewedItem), + ) + + field = PreviewedItemListSchema.__fields__['markers'] + unpacked = field.unpack(b'\x01A\x01B', {}) + + self.assertEqual(len(unpacked), 2) + self.assertEqual(unpacked[0].data, b'A') + self.assertEqual(unpacked[1].data, b'B') + def _make_wrapped_options_schema(self): """A three-octet option area whose first option over-reads by one octet.