Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 23 additions & 17 deletions tests/protocols/internet/test_ipv4_unit.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -2030,27 +2030,33 @@ 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
<pcapkit.protocols.schema.schema.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
<pcapkit.corekit.fields.collections.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
from pcapkit.protocols.internet.ipv4 import IPv4
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):
Expand Down
37 changes: 18 additions & 19 deletions tests/protocols/transport/test_tcp_udp_unit.py
Original file line number Diff line number Diff line change
Expand Up @@ -1265,40 +1265,39 @@ 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.
:meth:`FieldBase.unpack <pcapkit.corekit.fields.field.FieldBase.unpack>`
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
<pcapkit.corekit.fields.collections.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
Expand All @@ -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)
Expand Down
Loading