From abb8447c315465a27e2c2d949d39c11e3df72c30 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 17 Sep 2026 20:55:03 -0400 Subject: [PATCH 1/2] schema: a forward match consumes nothing, so bill it nothing in len(schema) Schema.unpack() kept the octets a ForwardMatchField read in __buffer__ even though it rewinds the stream past them, so __bytes__()/__len__() -- which concatenate every __buffer__ slot -- double-counted them: once in the forward-matched slot, once more where the real field re-reads the same octets. OptionField and ListField size a declared area by subtracting len(item) as they go, so the over-count broke correct input, e.g. CGAParameter's public_key_test (mh.py:509), which cannot simply drop the field the way #441's stray one can. See #446. - pcapkit/protocols/schema/schema.py: unpack() now zeroes a ForwardMatchField's __buffer__ slot after the rewind, mirroring what pack() already does for the same field type -- pack() has zeroed it since #422 and a schema built from field values already tests that way (test_schema_unit.py:69-70). unpack() was the one path left inconsistent; bytes(schema) now agrees with pack()'s existing convention on both paths, and now reproduces the octets actually consumed rather than double counting the previewed region. - tests/protocols/schema/test_schema_unit.py: two new cases -- a minimal schema with one ForwardMatchField asserting len(schema) equals the octets consumed, and a ListField whose declared area is exactly right but was rejected before the fix with the same FieldValueError CGAParameter hits. - tests/protocols/test_option_roundtrip_unit.py: deletes the 'ipv6-opts-option/SMF_DPD' EXPECTED_FAILURES entry, which this fix turns 'OK' on its own (independent of #449's removal of the stray field that exposed it). Full suite: 864 passed, 13 skipped, at PYTHONSAFEPATH=1, interpreter 3.14.7. Baseline at e2d8ed6d1 (origin/main): 862 passed, 13 skipped, before the 2 new tests existed. --- pcapkit/protocols/schema/schema.py | 20 ++++ tests/protocols/schema/test_schema_unit.py | 96 +++++++++++++++++++ tests/protocols/test_option_roundtrip_unit.py | 45 +++------ 3 files changed, 131 insertions(+), 30 deletions(-) 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..c5cda20231 100644 --- a/tests/protocols/schema/test_schema_unit.py +++ b/tests/protocols/schema/test_schema_unit.py @@ -506,6 +506,102 @@ 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.CGAParameter`'s + ``extensions`` :class:`~pcapkit.corekit.fields.collections.OptionField` + with ``FieldValueError: Field parameters has invalid length.``, minimised + to a :class:`~pcapkit.corekit.fields.collections.ListField` so it needs no + 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. diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index f7830b7a65..1e779f4e54 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -253,43 +253,28 @@ class Gap(NamedTuple): 'pcapkit/protocols/schema/internet/ipv6_opts.py:224 -- ' 'SchemaField(length=5)'), - # -- The non-progress loop, now half fixed -------------------------------- + # -- The non-progress loop, now fixed -------------------------------------- # Both of these used to *hang* rather than fail, which is why the cycle is # run under a deadline at all. #432 landed the progress guard in # ``OptionField``/``ListField`` and fixed the ``_SMFDPDOption`` sizing in # *both* schema modules symmetrically -- 26 lines each, ``'len': (1, 8)`` to # ``(8, 8)`` and the ``+ 2`` on the selector's ``SchemaField`` -- so - # ``hopopt-option/SMF_DPD`` now round-trips and has no entry here at all. + # ``hopopt-option/SMF_DPD`` round-trips and has no entry here at all. # - # ``IPv6-Opts`` still fails, and not because it missed that fix. The two - # modules differ in exactly one line of code, and it is older than #432: - # ``ipv6_opts.SMFIdentificationBasedDPDOption`` declares a second, redundant - # ``test`` ``ForwardMatchField`` that ``hopopt``'s does not. The enclosing - # ``_SMFDPDOption`` already has one, in both modules, and it is that outer - # field the selector reads -- nothing reads the nested copy. But a - # ``ForwardMatchField`` does not consume the stream while still occupying a - # slot in ``__buffer__``, so the nested schema over-reports its own size by - # one octet, and ``OptionField`` then mis-counts the option area against the - # header. Measured on the *same* octets, ``1100080100010100``: - # - # hopopt __fields__ = [type, len, info, tid, id] len(schema) = 3 - # ipv6_opts __fields__ = [type, len, test, info, tid, id] len(schema) = 4 - # - # HOPOPT(...) -> options=[SMF_DPD, PadN] - # IPv6_Opts(...) -> ProtocolError: IPv6-Opts: invalid format - # - # Identical on 3.10.20 and 3.14.7, so this one is not interpreter-dependent. - # - # The deadline in the sweep stays regardless. It is protection against the - # *next* non-progress defect, not against this one. - 'ipv6-opts-option/SMF_DPD': Gap( - 'PARSE', 'IPv6-Opts: invalid format', - 'pcapkit/protocols/schema/internet/ipv6_opts.py:434 -- a redundant second ' - "'test' ForwardMatchField that hopopt.py's equivalent does not have; it " - 'consumes nothing but is counted in __buffer__, so the nested schema ' - 'reports 4 octets where it read 3, and the threshold check at ' - 'pcapkit/protocols/internet/ipv6_opts.py:497 rejects the option area'), + # ``ipv6-opts-option/SMF_DPD`` used to fail here too, for a second and + # independent reason: a ``ForwardMatchField`` does not consume the stream + # but still occupied a slot in ``__buffer__``, so a nested schema carrying + # one over-reported its own size by the width of the match, and + # ``OptionField`` then mis-counted the option area against the header. + # ``ipv6_opts.SMFIdentificationBasedDPDOption`` declared a redundant, stray + # ``test`` ``ForwardMatchField`` that ``hopopt``'s equivalent did not, which + # made this the case that exposed the mechanism -- fixed generally in + # ``Schema.unpack`` (#446, ``pcapkit/protocols/schema/schema.py``), which + # stops crediting a forward match's octets to ``len(schema)`` regardless of + # which schema carries one. #449 separately deletes the stray field itself + # (:file:`pcapkit/protocols/schema/internet/ipv6_opts.py`:434) as its own + # defect, but either fix alone already turns this case ``'OK'``. # -- IPv6-Route ----------------------------------------------------------- From 3fecd93f2b982d9d98f1b964f60fbf38e45f8073 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 17 Sep 2026 20:58:58 -0400 Subject: [PATCH 2/2] tests: name the ListField that actually raises, not CGAParameter's OptionField CGAParametersOption.parameters is the ListField of CGAParameter items whose budget the forward-match over-count drains; CGAParameter.extensions is an OptionField and never raises an invalid-length FieldValueError itself. Fix the docstring to name the field that matches the issue's own traceback. --- tests/protocols/schema/test_schema_unit.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/protocols/schema/test_schema_unit.py b/tests/protocols/schema/test_schema_unit.py index c5cda20231..70a45bd7e3 100644 --- a/tests/protocols/schema/test_schema_unit.py +++ b/tests/protocols/schema/test_schema_unit.py @@ -574,11 +574,13 @@ def test_schema_list_field_rejects_a_declared_area_that_a_forward_match_over_rep ` raises ``FieldValueError`` on input that is exactly the right length. This is the same mechanism that fails - :class:`~pcapkit.protocols.schema.internet.mh.CGAParameter`'s - ``extensions`` :class:`~pcapkit.corekit.fields.collections.OptionField` - with ``FieldValueError: Field parameters has invalid length.``, minimised - to a :class:`~pcapkit.corekit.fields.collections.ListField` so it needs no - option registry. + :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