diff --git a/tests/protocols/internet/test_ipv4_unit.py b/tests/protocols/internet/test_ipv4_unit.py index 499afcf6c..f1b6753fe 100644 --- a/tests/protocols/internet/test_ipv4_unit.py +++ b/tests/protocols/internet/test_ipv4_unit.py @@ -2011,9 +2011,9 @@ def test_a_truncated_option_still_parses_its_declared_length(self) -> None: option that *does* start, declares more data than the capture actually holds, and runs out partway through, the shape of a datagram cut short by the snapshot length rather than one with no options at all. A - candidate fix for #554 (PR #571) turns that into an unwrapped - ``FieldValueError`` while the rest of the suite stays green, because - nothing exercises it. See + candidate fix for #554 (PR #571) turned that into an unwrapped + ``FieldValueError`` while the rest of the suite stayed green, because + nothing exercised it. See ``TCPUDPUnitTests.test_a_truncated_option_still_parses_its_declared_length`` for the same case on TCP, where the shortfall is simpler to reach. @@ -2030,19 +2030,25 @@ def test_a_truncated_option_still_parses_its_declared_length(self) -> None: though, which the TCP case does not need. Unlike TCP, :meth:`~pcapkit.protocols.internet.ipv4.IPv4._read_ipv4_options` sums each option's self-*declared* ``length`` -- not what it actually - consumed -- and raises ``IPv4: invalid format`` if that sum exceeds - the declared option area; declaring exactly 8 would make the single - option's own ``length=12`` trip that check before the accommodation - under test is ever reached. Declaring 16 leaves headroom, at the cost - of a second effect: once the option loop's 16-octet budget outlives - the 8 octets its one real option consumed, the loop reads one further, - fully exhausted phantom option, decodes it as end-of-option-list (the - same mechanism the test above pins), and the #431 machinery in - :meth:`~pcapkit.corekit.fields.collections.OptionField.unpack` - rewinds and hands the same 8 octets to the schema a second time as - padding. That is why ``bytes(proto.__header__)`` does not round-trip - to ``raw`` here and is not asserted -- immaterial to what this test - pins, which is solely the ``data`` field's short-read reconstruction. + consumed -- and raises ``IPv4: invalid format`` once its loop over the + parsed options finishes, if that sum exceeds the declared option + area. Declaring exactly 8 does not skip the accommodation -- the short + ``data`` field is still read and left-padded as above -- it just trips + that check afterwards, discarding the result before the test can + assert on it. Declaring 16 leaves headroom, at the cost of a second + effect: once the option loop's 16-octet budget outlives the 8 octets + its one real option consumed, the loop reads one further, fully + exhausted phantom option, decodes it as end-of-option-list (the same + mechanism the test above pins), and :meth:`Schema.unpack + ` hands the same 8 + octets to the schema a second time as padding, via the + ``option_padding`` rewind that #371 added -- code that predates #431 + and lives outside :meth:`OptionField.unpack + `. That is why + ``bytes(proto.__header__)`` does not round-trip to ``raw`` here and is + not asserted -- immaterial to what this test pins: the option's + declared ``length``, the full parsed options list, and the ``data`` + field's short-read reconstruction. """ from pcapkit.const.ipv4.option_number import OptionNumber @@ -2050,7 +2056,7 @@ def test_a_truncated_option_still_parses_its_declared_length(self) -> None: from tests._support import time_limit custom = OptionNumber.get(31) - trailing = bytes([0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff]) + trailing = bytes.fromhex('aabbccddeeff') raw = (bytes.fromhex('4900001c00010000400600000a0000010a000002') + bytes([custom, 12]) + trailing) with time_limit(5): diff --git a/tests/protocols/transport/test_tcp_udp_unit.py b/tests/protocols/transport/test_tcp_udp_unit.py index 5a47fd5eb..400cdef7d 100644 --- a/tests/protocols/transport/test_tcp_udp_unit.py +++ b/tests/protocols/transport/test_tcp_udp_unit.py @@ -1265,22 +1265,21 @@ def test_an_option_area_longer_than_the_segment_still_parses(self) -> None: self.assertEqual(mss.mss, 1460) def test_a_truncated_option_still_parses_its_declared_length(self) -> None: - """A capture cut short mid-option is tolerated, not just mid-header. C.f. #431, #572. + """An option declaring more data than its option area holds is tolerated. C.f. #431, #572. :meth:`test_an_option_area_longer_than_the_segment_still_parses` above pins the *empty*-tail half of the #431 accommodation: an option area that runs out before it starts, so the type byte decodes as 0 and the loop reads end-of-option-list. Nothing pinned the other half -- an - option that *does* start, declares more data than the capture actually - holds, and runs out partway through, the shape of a segment cut short - by the snapshot length rather than one with no options at all. A - candidate fix for #554 (PR #571) turned that into an unwrapped - ``FieldValueError`` while the rest of the suite stayed green, because - nothing exercised it. + option that *does* start, declares more data than its own option area + actually holds, and runs out partway through its ``data`` field, + rather than one with no options at all. A candidate fix for #554 (PR + #571) turned that into an unwrapped ``FieldValueError`` while the rest + of the suite stayed green, because nothing exercised it. The segment below sets a data offset of 7 -- 8 octets of option area - -- for an unassigned option kind (``0x4f``) declaring ``length=12``, - which asks + -- for a reserved option kind (``0x4f``, which resolves to + ``Option.Reserved_79``) declaring ``length=12``, which asks :class:`~pcapkit.protocols.schema.transport.tcp.UnassignedOption`'s ``data`` field (``BytesField(length=lambda pkt: pkt['length'] - 2)``, 10 octets here) for more than the 6 octets actually behind it. @@ -1288,17 +1287,17 @@ def test_a_truncated_option_still_parses_its_declared_length(self) -> None: left-pads the short read with zero octets rather than raising, so the option parses with its declared ``length`` intact and a ``data`` value of four zero octets followed by the six real ones. That is reachable - here because :meth:`OptionField.unpack - ` sizes this - option's schema by what it actually consumed (8 octets) rather than by - its self-reported ``length``, so the separate ``TCP: invalid format`` - threshold in :meth:`~pcapkit.protocols.transport.tcp.TCP._read_tcp_options` - never sees the shortfall -- unlike IPv4's equivalent check, which sums - the *declared* lengths instead and does see it (see + here because :meth:`~pcapkit.protocols.transport.tcp.TCP._read_tcp_options` + sizes each parsed option by ``len(schema)`` -- what it actually + consumed (8 octets) -- rather than by its self-reported ``length``, so + its own ``TCP: invalid format`` threshold never sees the shortfall -- + unlike IPv4's equivalent check, which sums the *declared* lengths + instead and does see it (see ``IPv4UnitTests.test_a_truncated_option_still_parses_its_declared_length``). ``length=32`` (30 octets of data wanted, still only 6 available) is - checked alongside 12, since the fix under discussion would reject both - identically. + checked alongside 12 because the pad width tracks ``length - 2``: 32 + yields 24 zero octets where 12 yields 4, pinning that the padding + scales with the declared length rather than being a fixed 4. """ import struct @@ -1312,7 +1311,7 @@ def segment(data_offset: 'int', options: 'bytes') -> 'bytes': 0x10, 0, 0, 0) + options custom = Option.get(0x4f) - trailing = bytes([0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff]) + trailing = bytes.fromhex('aabbccddeeff') for declared_length, zeroes in ((12, 4), (32, 24)): with self.subTest(declared_length=declared_length): raw = segment(7, bytes([custom, declared_length]) + trailing)