From ca2568dabee09c047be567202be6bb284d14da28 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sat, 19 Sep 2026 22:25:33 -0400 Subject: [PATCH] fix(protocols): let from_data rebuild a parsed packet Closes #506. `IPv4.from_data()` raised `ProtocolUnbound: unsupported type ... Raw` on any datagram read off the wire. Two independent faults, both downstream of the `_make_data` that #494 fixed: - schema/schema.py: the `PayloadField` branch of `Schema.pack` imported `Protocol` where every other site in the tree imports `ProtocolBase as Protocol`, so it tested `isinstance(data, Protocol)`. `Protocol` has 0 descendants and `ProtocolBase` has 43 (measured), so the branch that packs a protocol payload was unreachable and every protocol instance fell through to `ProtocolUnbound` -- including the `Raw` that parsing yields. Strictly a widening: `Protocol` is itself a `ProtocolBase`. - internet/ipv4.py: both branches of `_make_ipv4_options` appended a bare `Enum_OptionNumber.EOOL` as end-of-list padding where the option field takes only schemas and bytes. Now an `EOOL` option schema, as the adjacent `NOP` padding always was. - tests: round-trip three *parsed* datagrams, pack a `ProtocolBase` payload through `Schema.pack` directly, and assert both padding branches emit an option. `EXPECTED_FAILURES` drops from 59 entries to 55 as `ipv4-option/RR`, `LSR`, `SSR` and `SID` now round-trip. `SID` closes only because padding is no longer fatal; its 6-octet pack asymmetry survives and is pinned by a new test and filed as #534. --- pcapkit/protocols/internet/ipv4.py | 20 +- pcapkit/protocols/schema/schema.py | 20 +- tests/protocols/internet/test_ipv4_unit.py | 269 +++++++++++++++++- tests/protocols/test_option_roundtrip_unit.py | 153 ++++++++-- 4 files changed, 428 insertions(+), 34 deletions(-) diff --git a/pcapkit/protocols/internet/ipv4.py b/pcapkit/protocols/internet/ipv4.py index 25a1bbb252..042eb9cd0b 100644 --- a/pcapkit/protocols/internet/ipv4.py +++ b/pcapkit/protocols/internet/ipv4.py @@ -1255,12 +1255,23 @@ def _make_ipv4_options(self, options: 'list[Schema_Option | tuple[Enum_OptionNum # force alignment to 32-bit boundary if data_len % 4: pad_len = 4 - (data_len % 4) + # NOTE: The terminator goes in as an EOOL option *schema*, the + # way the padding above goes in as a NOP schema. What used to be + # appended was ``Enum_OptionNumber.EOOL`` itself -- the wire code + # rather than an option -- and the enclosing ``options`` field + # takes only schemas and :obj:`bytes`, so packing the header + # failed with ``FieldValueError: Field options has invalid + # value``. Any option whose length is not already a multiple of + # four reaches this branch, so that made the packet unpackable + # whether it was built by hand or rebuilt from a parsed one. See + # #506. pad_opt = self._make_opt_nop(Enum_OptionNumber.NOP) # type: ignore[arg-type] + end_opt = self._make_opt_eool(Enum_OptionNumber.EOOL) # type: ignore[arg-type] total_length += pad_len for _ in range(pad_len - 1): options_list.append(pad_opt) - options_list.append(Enum_OptionNumber.EOOL) # type: ignore[arg-type] + options_list.append(end_opt) return options_list, total_length options_list = [] @@ -1286,12 +1297,17 @@ def _make_ipv4_options(self, options: 'list[Schema_Option | tuple[Enum_OptionNum # force alignment to 32-bit boundary if data_len % 4: pad_len = 4 - (data_len % 4) + # NOTE: An EOOL option schema rather than the bare wire code, for the + # reason spelled out in the list branch above. This is the branch the + # ``from_data`` path takes, since a parsed packet hands its options + # back as a container. See #506. pad_opt = self._make_opt_nop(Enum_OptionNumber.NOP) # type: ignore[arg-type] + end_opt = self._make_opt_eool(Enum_OptionNumber.EOOL) # type: ignore[arg-type] total_length += pad_len for _ in range(pad_len - 1): options_list.append(pad_opt) - options_list.append(Enum_OptionNumber.EOOL) # type: ignore[arg-type] + options_list.append(end_opt) return options_list, total_length def _make_opt_unassigned(self, kind: 'Enum_OptionNumber', option: 'Optional[Data_UnassignedOption]' = None, *, diff --git a/pcapkit/protocols/schema/schema.py b/pcapkit/protocols/schema/schema.py index dad88d5582..9a4dcb54ac 100644 --- a/pcapkit/protocols/schema/schema.py +++ b/pcapkit/protocols/schema/schema.py @@ -667,12 +667,28 @@ class will consider negative value as a placeholder. data = self.__dict__.get(self.__map__.get(field.name, field.name)) if isinstance(field, PayloadField): + # NOTE: ``ProtocolBase``, not ``Protocol``. The two were one class + # until the metaclass revision split them, which renamed the base to + # ``ProtocolBase`` and kept ``Protocol`` as a thin subclass that adds + # auto-registration for externally defined engines. Every module under + # ``pcapkit.protocols.schema`` was updated to import the base under the + # old name; this one was missed, because its import is a runtime import + # inside a method rather than a ``TYPE_CHECKING`` one at module level. + # No protocol in the library subclasses ``Protocol``, so the branch + # below had been unreachable ever since: handing a payload field any + # protocol instance -- which is exactly what :meth:`ProtocolBase._make_payload + # ` returns, and + # what ``make``'s own ``bytes | Protocol | Schema`` signature advertises + # -- fell through to the ``ProtocolUnbound`` below instead of being + # packed. That is what stopped ``from_data`` reconstructing any parsed + # packet. An external ``Protocol`` subclass is still a ``ProtocolBase``, + # so nothing that worked before is affected. See #506. from pcapkit.protocols.protocol import \ - Protocol # pylint: disable=import-outside-toplevel + ProtocolBase # pylint: disable=import-outside-toplevel if data is None: self.__buffer__[field.name] = b'' - elif isinstance(data, Protocol): + elif isinstance(data, ProtocolBase): self.__buffer__[field.name] = bytes(data) elif isinstance(data, bytes): self.__buffer__[field.name] = data diff --git a/tests/protocols/internet/test_ipv4_unit.py b/tests/protocols/internet/test_ipv4_unit.py index 42cd85a5b9..1cf8cff264 100644 --- a/tests/protocols/internet/test_ipv4_unit.py +++ b/tests/protocols/internet/test_ipv4_unit.py @@ -106,6 +106,265 @@ def test_ipv4_make_data_scales_offset_and_defaults_missing_options(self) -> None payload=b'\xaa' * 8).pack() self.assertEqual(raw, raw2) + def test_ipv4_from_data_rebuilds_a_parsed_datagram(self) -> None: + """Regression test for #506. + + :meth:`IPv4.from_data ` + could not rebuild a datagram that had been *parsed*, only one that had + been built by hand. The test above covers :meth:`IPv4._make_data + ` in isolation, which is + why #494 went in without this surfacing: ``_make_data`` is one input to + ``from_data``, and both remaining faults were downstream of it. + + The wire literals below are parsed rather than constructed, so the info + they yield carries what parsing actually produces -- a + :class:`~pcapkit.protocols.misc.raw.Raw` or + :class:`~pcapkit.protocols.misc.null.NoPayload` instance for the payload, + and an option *container* for the options -- rather than the + constructor-shaped values :meth:`IPv4.make + ` is normally handed. Each one + raised before the fix: the first two with ``ProtocolUnbound: unsupported + type ``, the third with the same + naming ``NoPayload``. + + The third literal is the ``RTRALT`` frame of ``options-ipv4.pcap`` as + generated by :file:`examples/generators/make_samples.py`, inlined so the + test does not depend on the fixture captures being present. + + It is named by its option rather than by its index because this fix + renumbers that capture: the option-padding half below lets the generator + emit the ``RR``, ``LSR``, ``SID`` and ``SSR`` frames it previously could + not write at all, which grows ``options-ipv4.pcap`` from 8 frames (456 + octets) to 12 (784) and moves this frame from 8th to 12th. An index here + would have been correct when written and wrong once the fix landed. + + """ + from pcapkit.const.ipv4.option_number import OptionNumber + from pcapkit.protocols.internet.ipv4 import IPv4 + from pcapkit.protocols.misc.null import NoPayload + from pcapkit.protocols.misc.raw import Raw + + cases = [ + # the issue's own repro: fragment offset 5, no options, 8 octets of + # unparseable TCP that come back as ``Raw`` + ('no options', '4500001c00000005000600007f00000100000000aaaaaaaaaaaaaaaa', + None, Raw), + # the same, with a 4-octet Router Alert option ahead of the payload + ('with options', '4600002000000005000600007f0000010000000094040001aaaaaaaaaaaaaaaa', + [OptionNumber.RTRALT], Raw), + # options-ipv4.pcap frame 8: options, and no payload at all + ('with options, no payload', '460000180000000000060000c0000201c633640194040001', + [OptionNumber.RTRALT], NoPayload), + ] + + for label, hexstr, codes, payload_type in cases: + with self.subTest(label): + raw = bytes.fromhex(hexstr) + + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + parsed = IPv4(io.BytesIO(raw), len(raw)) + + # the parsed shapes the make path has to cope with + self.assertIsInstance(parsed.payload, payload_type) + if codes is None: + self.assertFalse(hasattr(parsed.info, 'options')) + else: + self.assertEqual(list(parsed.info.options.keys()), codes) + + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + rebuilt = IPv4.from_data(parsed.info) + + self.assertEqual(bytes(rebuilt), raw) + + def test_ipv4_make_accepts_a_protocol_instance_as_payload(self) -> None: + """A protocol instance is a payload ``make`` has to accept. C.f. #506. + + This is the mechanism behind the round trip above, tested on its own + because it is a documented part of the construction API rather than + something only ``from_data`` reaches: every ``make`` in the library + annotates its ``payload`` as ``bytes | Protocol | Schema``, where + ``Protocol`` is :class:`~pcapkit.protocols.protocol.ProtocolBase` + imported under its historical name. :meth:`Schema.pack + ` nonetheless rejected every + protocol instance in the library, because its own ``isinstance`` check + named the *other* class -- the thin + :class:`~pcapkit.protocols.protocol.Protocol` subclass, which nothing + subclasses -- so the branch that packs a protocol payload was + unreachable. + + Asserting on the shape rather than only on the bytes is deliberate: a + ``_make_payload`` changed to hand back :obj:`bytes` would make the round + trip above pass again while leaving this API broken, so the two tests + fail for different reasons. + + """ + from pcapkit.protocols.internet.ipv4 import IPv4 + from pcapkit.protocols.misc.null import NoPayload + from pcapkit.protocols.misc.raw import Raw + from pcapkit.protocols.protocol import Protocol, ProtocolBase + + proto = object.__new__(IPv4) + payload = b'\xaa' * 8 + + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + expected = proto.make(protocol=6, payload=payload).pack() + + # a protocol instance carrying the same octets packs identically ... + raw_payload = Raw(packet=payload) + self.assertIsInstance(raw_payload, ProtocolBase) + # ... and it is *not* a ``Protocol``, which is the whole of the + # defect. Asserting the negative matters because the branch was not + # uncovered before the fix -- it was covered by the only class in the + # tree that satisfied it, ``DummyProtocol`` in + # tests/protocols/schema/test_schema_unit.py:167, which subclasses + # ``Protocol`` and so packed happily while every protocol in the + # library raised. Without this line the same hole could be reopened + # by making a protocol subclass ``Protocol`` rather than by fixing + # the check. + self.assertNotIsInstance(raw_payload, Protocol) + self.assertEqual(proto.make(protocol=6, payload=raw_payload).pack(), + expected) + + # ... and an empty one is the same as no payload at all + self.assertEqual(proto.make(protocol=6, payload=NoPayload()).pack(), + proto.make(protocol=6, payload=b'').pack()) + + # the shape ``from_data`` feeds to ``make``, named here so that routing + # around the packing layer instead of fixing it does not go unnoticed + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + parsed = IPv4(io.BytesIO(expected), len(expected)) + self.assertIsInstance(IPv4._make_data(parsed.info)['payload'], ProtocolBase) + + def test_schema_pack_packs_a_protocol_base_payload_on_its_own(self) -> None: + """The packing layer itself, with no ``make`` in front of it. C.f. #506. + + The test above reaches :meth:`Schema.pack + ` through :meth:`IPv4.make + `, so it cannot say which of + the two was at fault. This one builds the header schema directly and + packs it, which is where the defect actually lived: the ``PayloadField`` + branch of ``Schema.pack`` tested ``isinstance(data, Protocol)`` where it + meant :class:`~pcapkit.protocols.protocol.ProtocolBase`. + + The three assertions are the three things the widened check has to get + right at once, and they pull in different directions: + + * a :class:`~pcapkit.protocols.protocol.ProtocolBase` payload packs to its + own octets -- the defect; + * :class:`~pcapkit.protocols.protocol.Protocol` is still a subclass of + ``ProtocolBase``, so an externally defined engine that *did* satisfy the + old check still satisfies the new one. That is the claim the ``NOTE`` on + the fix makes about not regressing anything, and it is the reason + widening the check is safe rather than merely correct; + * anything that is neither protocol, schema nor :obj:`bytes` still raises + :exc:`~pcapkit.utilities.exceptions.ProtocolUnbound`, so the branch was + widened and not simply removed. + + """ + from pcapkit.protocols.misc.raw import Raw + from pcapkit.protocols.protocol import Protocol, ProtocolBase + from pcapkit.protocols.schema.internet.ipv4 import IPv4 as Schema_IPv4 + from pcapkit.utilities.exceptions import ProtocolUnbound + + payload = b'\xaa' * 8 + + def header(value: 'object') -> 'Schema_IPv4': + return Schema_IPv4( + vihl={'version': 4, 'ihl': 5}, + tos={'pre': 0, 'del': 0, 'thr': 0, 'rel': 0, 'ecn': 0}, + length=20 + len(payload), id=0, + flags={'df': 0, 'mf': 0, 'offset': 0}, + ttl=0, proto=6, chksum=b'\x00\x00', + src='127.0.0.1', dst='127.0.0.2', + options=[], payload=value, + ) + + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + expected = header(payload).pack() + + raw_payload = Raw(packet=payload) + self.assertIsInstance(raw_payload, ProtocolBase) + self.assertNotIsInstance(raw_payload, Protocol) + self.assertEqual(header(raw_payload).pack(), expected) + + # an external ``Protocol`` engine is a ``ProtocolBase`` too, so widening + # the check cannot have cost anything that used to work + self.assertTrue(issubclass(Protocol, ProtocolBase)) + + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + with self.assertRaises(ProtocolUnbound): + header(object()).pack() + + def test_ipv4_make_options_pads_with_an_eool_option_not_its_wire_code(self) -> None: + """Option padding has to be an option, not an option number. C.f. #506. + + :meth:`IPv4._make_ipv4_options + ` pads each + option out to a 32-bit boundary with ``NOP`` options and a terminating + ``EOOL``. The ``NOP``\\ s went in as option schemas but the ``EOOL`` went + in as :attr:`OptionNumber.EOOL + ` itself, and the + enclosing option field takes only schemas and :obj:`bytes`, so packing + the header failed with ``FieldValueError: Field options has invalid + value``. + + Both of the method's branches are exercised: the ``list`` branch a caller + reaches through ``make``, and the container branch ``from_data`` reaches, + since a parsed datagram hands its options back as an + :class:`~pcapkit.corekit.multidict.OrderedMultiDict`. Any option whose + length is not already a multiple of four reaches the padding; ``SEC`` is + three octets, so one octet of it is padding. + + """ + from pcapkit.const.ipv4.option_number import OptionNumber + from pcapkit.protocols.internet.ipv4 import IPv4 + from pcapkit.protocols.schema.schema import Schema + + proto = object.__new__(IPv4) + sec = (OptionNumber.SEC, {}) + + # list branch: three octets of option, one of padding + options, total_length = proto._make_ipv4_options([sec]) + self.assertEqual(total_length, 4) + for entry in options: + self.assertIsInstance(entry, (Schema, bytes)) + self.assertIsInstance(options[-1], Schema) + self.assertEqual(options[-1].type, OptionNumber.EOOL) + + # ... and the header it feeds actually packs, with the padding octet where + # the terminator belongs + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + raw = proto.make(protocol=6, options=[sec], payload=b'\xaa' * 4).pack() + self.assertEqual(raw[20], OptionNumber.SEC) + self.assertEqual(raw[21], 3) + self.assertEqual(raw[23], OptionNumber.EOOL) + + # container branch: the same option area, arriving the way a parsed + # datagram hands it back, and rebuilding to the same octets + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + parsed = IPv4(io.BytesIO(raw), len(raw)) + options, total_length = proto._make_ipv4_options(parsed.info.options) + rebuilt = IPv4.from_data(parsed.info) + self.assertEqual(total_length, 4) + for entry in options: + self.assertIsInstance(entry, (Schema, bytes)) + # the terminator is asserted on this branch too, not only on the list + # branch above, and against the method's own return value rather than + # against the assembled packet. The bytes comparison at the end of this + # test does catch a wrong terminator, but it reports it as two hex + # strings; this says which entry was wrong. + self.assertIsInstance(options[-1], Schema) + self.assertEqual(options[-1].type, OptionNumber.EOOL) + self.assertEqual(bytes(rebuilt), raw) + def test_ipv4_properties_read_and_make_cover_packet_paths(self) -> None: from pcapkit.const.ipv4.option_number import OptionNumber from pcapkit.const.reg.transtype import TransType @@ -483,8 +742,10 @@ def test_ipv4_option_constructors_cover_common_and_error_branches(self) -> None: (OptionNumber.NOP, {}), ]) self.assertEqual(total_length, 12) - self.assertEqual([type(item).__name__ for item in options], ['bytes', 'SIDOption', 'NOPOption', 'OptionNumber']) - self.assertEqual(options[-1], OptionNumber.EOOL) + # The padding terminator is an EOOL *option*, not the bare wire code: + # these two assertions pinned the latter, which is what #506 fixed. + self.assertEqual([type(item).__name__ for item in options], ['bytes', 'SIDOption', 'NOPOption', 'EOOLOption']) + self.assertEqual(options[-1].type, OptionNumber.EOOL) with self.assertRaises(ProtocolError): proto._make_opt_ts(OptionNumber.TS, timestamp=None) @@ -694,7 +955,9 @@ def opt_type(code): mapped_options, mapped_total = proto._make_ipv4_options(option_map) self.assertEqual(mapped_total, 20) self.assertEqual(mapped_options[0].sid, 123) - self.assertIn(OptionNumber.EOOL, mapped_options) + # As above: the terminator is an EOOL option schema, so look for its type + # rather than for the wire code itself. See #506. + self.assertIn(OptionNumber.EOOL, [item.type for item in mapped_options]) self.assertEqual(mapped_options[-1].mtu, 1500) def test_ipv4_option_readers_cover_common_and_error_branches(self) -> None: diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index 07805bdfb4..6d82f35ac6 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -25,6 +25,16 @@ by case, which cycles do not close today and which defect stops each one. A case absent from that table has to come back ``'OK'``. +That table can only speak about cycles that *fail*, though, and a defect can +leave the cycle closed -- the generator constructing and reconstructing the same +wrong octets, which match each other and so match the assertion. Those are +pinned as tests of their own rather than as entries, since an entry would have to +record ``'OK'`` as a failure: +:meth:`OptionRoundTripTests.test_a_parsed_sid_option_re_emits_two_octets_too_wide` +for IPv4's ``SID`` option width, tracked as #534, and +:meth:`OptionRoundTripTests.test_a_single_hip_parameter_cannot_be_constructed` +for the HIP header arithmetic the generator's ``HIP_COPIES`` routes around. + Why the table is asserted in both directions -------------------------------------------- @@ -55,6 +65,7 @@ import sys import types import unittest +import warnings from typing import TYPE_CHECKING, NamedTuple from tests._support import purge_modules, time_limit @@ -164,31 +175,34 @@ class Gap(NamedTuple): 'pcapkit/protocols/transport/tcp.py:2675 -- _make_mptcp_join reads ' 'self._flags, which exists only while parsing'), - # -- IPv4 ----------------------------------------------------------------- - - # ``_make_ipv4_options`` appends a bare enumeration member to the option - # list as its end-of-list padding, and ``OptionField.pack`` accepts only - # bytes or a Schema. It fires for every option whose packed length is not a - # multiple of four, which is what these four have in common. LSR, RR and SSR - # cannot be brought to a multiple of four by any argument: their length is - # ``3 + counts * 4``. - 'ipv4-option/LSR': Gap( - 'CONSTRUCT', 'Field options has invalid value', - 'pcapkit/protocols/internet/ipv4.py:1225 and :1252 -- a bare ' - 'Enum_OptionNumber.EOOL is appended to the option list'), - 'ipv4-option/RR': Gap( - 'CONSTRUCT', 'Field options has invalid value', - 'pcapkit/protocols/internet/ipv4.py:1225 and :1252'), - 'ipv4-option/SSR': Gap( - 'CONSTRUCT', 'Field options has invalid value', - 'pcapkit/protocols/internet/ipv4.py:1225 and :1252'), - # SID reaches the same padding branch for a second reason of its own: - # ``_make_opt_sid`` declares ``length=4`` while ``SIDOption.sid`` is a - # 32-bit field, so the option packs to six octets (``880400000000``). - 'ipv4-option/SID': Gap( - 'CONSTRUCT', 'Field options has invalid value', - 'pcapkit/protocols/internet/ipv4.py:1683 -- _make_opt_sid declares ' - 'length=4 but packs 6 octets, which then trips the :1225 padding branch'), + # -- IPv4, whose option padding is now fixed ------------------------------ + + # ``_make_ipv4_options`` used to append a bare enumeration member to the + # option list as its end-of-list padding, where ``OptionField.pack`` accepts + # only bytes or a Schema, so every option whose packed length is not a + # multiple of four failed to construct. #506 appends an ``EOOL`` option + # *schema* instead, the way the ``NOP`` options beside it always did, so + # ``LSR``, ``RR`` and ``SSR`` round-trip and have no entry here any more -- + # and they could not have been routed around, their length being + # ``3 + counts * 4`` and so never a multiple of four for any argument. + # + # ``SID`` reached that same branch for a second reason of its own, and #506 + # fixes only the padding half of it. Its cycle now closes, because the + # generator constructs and reconstructs the *same* six octets either side of + # the trip -- but six is not what the wire holds. ``SIDOption.sid`` is a + # ``UInt32Field`` at pcapkit/protocols/schema/internet/ipv4.py:368 where RFC + # 791's Stream ID is 16 bits, so ``_make_opt_sid`` packs ``880400000037`` + # where the option is ``88040037``, and a genuine 4-octet option read off the + # wire does not survive being re-emitted. + # + # That asymmetry cannot be recorded as a ``Gap``, because the status such an + # entry would have to name is ``'OK'`` -- the one value + # :meth:`test_round_trip_is_identity_or_a_recorded_gap` reads as "no entry + # needed". So it is pinned as an assertion instead, by + # :meth:`OptionRoundTripTests.test_a_parsed_sid_option_re_emits_two_octets_too_wide`, + # which is the record this table would otherwise have carried and which is + # what turns red when the field is narrowed. It is tracked as #534, so that + # dropping the entry from this table does not drop the defect with it. # ``_make_opt_ts`` passes ``data=`` where the schema field is ``ts_data``. # ``Schema.__init__`` only warns about an unknown field name and carries on, @@ -196,7 +210,7 @@ class Gap(NamedTuple): # descriptor -- which ``post_process`` then tries to iterate. 'ipv4-option/TS': Gap( 'CONSTRUCT', "'ListField' object is not iterable", - 'pcapkit/protocols/internet/ipv4.py:1488 -- data= should be ts_data=, ' + 'pcapkit/protocols/internet/ipv4.py:1550 -- data= should be ts_data=, ' 'dropped with UnknownFieldWarning and surfacing at ' 'pcapkit/protocols/schema/internet/ipv4.py:262'), @@ -222,7 +236,7 @@ class Gap(NamedTuple): # make these cases pass. 'ipv4-option/QS': Gap( 'CONSTRUCT', "no attribute 'func'", - 'pcapkit/protocols/internet/ipv4.py:1144 -- func is set only by ' + 'pcapkit/protocols/internet/ipv4.py:1178 -- func is set only by ' 'post_process; and separately ' 'pcapkit/protocols/schema/internet/ipv4.py:128 -- SchemaField(length=5) ' 'for an 8-octet option, which decodes nonce as 55'), @@ -695,6 +709,91 @@ def test_a_single_hip_parameter_cannot_be_constructed(self) -> None: again = bytes(HIP(parameters=reparsed.info.parameters, extension=True, **base)) self.assertEqual(paired, again) + def test_a_parsed_sid_option_re_emits_two_octets_too_wide(self) -> None: + """RFC 791's four-octet Stream ID option comes back six octets wide. + + Tracked as #534. This is the record that ``ipv4-option/SID`` used to + carry in :data:`EXPECTED_FAILURES`, kept here because it can no longer be + carried there, and filed as an issue as well so that the defect is + tracked somewhere a passing test suite cannot hide it. + + #506 fixed the option-padding defect that made the ``SID`` case + fail to construct at all, and with that gone the generator's cycle + closes and the case reports ``'OK'`` -- so a ``Gap`` for it would have to + record ``'OK'`` as a failure status, which is the one value + :meth:`test_round_trip_is_identity_or_a_recorded_gap` reads as "this case + needs no entry". + + The cycle closes for a reason that is worth being precise about: the + generator constructs the option and reconstructs it through the *same* + ``_make_opt_sid``, so both halves emit the same six octets and match each + other. It is only against the wire that the width shows, which is why + this test starts from wire octets rather than from the generator's case. + + ``SIDOption.sid`` is a :class:`~pcapkit.corekit.fields.numbers.UInt32Field` + at ``pcapkit/protocols/schema/internet/ipv4.py:368``, where RFC 791 + section 3.1 gives the Stream ID two octets inside a four-octet option -- + which is also what ``_make_opt_sid`` itself writes into ``length``. So + the field over-reads a well-formed option by exactly two octets on the + way in, and over-writes it by two on the way out. + + Narrowing that field to + :class:`~pcapkit.corekit.fields.numbers.UInt16Field` makes every + assertion below wrong at once -- measured: the option re-emits as + ``88040037``, the datagram stays 24 octets, and the padding branch is not + reached at all. That is the intended fix, and deleting this test is how + it gets recorded, exactly as deleting an :data:`EXPECTED_FAILURES` entry + would have been. + + """ + from pcapkit.const.ipv4.option_number import OptionNumber + from pcapkit.protocols.internet.ipv4 import IPv4 + + # A minimal IPv4 header, ihl=6, carrying one well-formed SID option: + # kind 136, length 4, and the two-octet stream id 0x0037. + option = bytes.fromhex('88040037') + header = bytes.fromhex('46000018 00000000 00060000 ' + '7f000001 7f000002') + option + self.assertEqual(len(header), 24) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + parsed = IPv4(header, len(header)) + + # The over-read, named by the library itself: a four-octet option minus a + # six-octet schema is the -2 in this warning. + self.assertIn('packet length < 0: -2', + [str(entry.message) for entry in caught]) + + # The value still survives the trip in, and the option still declares the + # four octets it occupies -- so nothing here is a parsing failure. + sid = parsed.info.options[OptionNumber.SID] + self.assertEqual(sid.sid, 0x37) + self.assertEqual(sid.length, 4) + + # Out again, the same option is six octets: two of stream id have become + # four, and the declared length no longer describes it. + proto = object.__new__(IPv4) + self.assertEqual(proto._make_opt_sid(OptionNumber.SID, sid).pack(), + bytes.fromhex('880400000037')) + + # Six is not a multiple of four, so the option area now reaches the + # padding branch that #506 fixed. That branch is no longer fatal, which + # is what lets the defect below through instead of stopping at it. + options, total_length = proto._make_ipv4_options(parsed.info.options) + self.assertEqual([type(entry).__name__ for entry in options], + ['SIDOption', 'NOPOption', 'EOOLOption']) + self.assertEqual(total_length, 8) + + # And so the rebuilt datagram is four octets longer than the one it was + # read from, with ihl and total length grown to match. + rebuilt = bytes(IPv4.from_data(parsed.info)) + self.assertNotEqual(rebuilt, header) + self.assertEqual(rebuilt[20:], bytes.fromhex('8804000000370100')) + self.assertEqual(len(rebuilt), 28) + self.assertEqual(rebuilt[0] & 0x0F, 7) + self.assertEqual(int.from_bytes(rebuilt[2:4], 'big'), 28) + def test_recorded_gaps_are_a_minority(self) -> None: """Most of the option space round-trips, and the rest is accounted for.