From 1a92f003142bc72f0692824685abaa213f48e716 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sun, 20 Sep 2026 16:56:42 -0400 Subject: [PATCH] fix(ipv4): three defects in the IPv4 option schemas (#552) - `TSOption.post_process` converted `ts_data` entries to addresses with a bare `ipaddress.ip_address`, which takes a `bool` as the `int` it subclasses, so `ts_data=[True, 5]` packed and reported `IPv4Address('0.0.0.1')` with no exception. It runs on the packing path too, and `IPv4.make` accepts a caller-built option schema, so this was reachable from the public API. All three conversions now go through `parse_ip_address`, the fifth site of the defect #481, #500, #539 and #540 fixed before it. - `quick_start_data_selector` sized the nested Quick-Start suboption with a hardcoded `SchemaField(length=5)` -- the width of a Request's `ttl` and `nonce` alone. A well-formed 8-octet option decoded its nonce as 55 rather than 933982136 and left three octets to be read as a fabricated option, so the datagram failed with `ProtocolError`. The length now comes from `quick_start_option_length`, computed from the resolved suboption, and `QuickStartReportOption` gains the RFC 4782 section 3.1 `Not Used` octet it was missing, which had made it seven octets wide against the `length=8` both `_make_opt_qs` and `_read_opt_qs` use. - `_make_opt_ts` passed `data=` where the schema field is `ts_data`, so every timestamp was dropped with an `UnknownFieldWarning` and the Timestamp option was unbuildable through `make`. The `TYPE_CHECKING` `__init__` stub that advertised `data` is corrected too. Three new tests, each shown to fail without its fix; `ipv4-option/TS` deleted from `EXPECTED_FAILURES` now that it round-trips. Full unit tier green, 1107 passed with 2666 subtests; both changed modules at 100% statement and branch coverage. --- .../pcapkit/protocols/internet/ipv4.rst | 2 + pcapkit/protocols/internet/ipv4.py | 12 +- pcapkit/protocols/schema/internet/ipv4.py | 178 ++++++++++++- tests/protocols/internet/test_ipv4_unit.py | 240 ++++++++++++++++++ tests/protocols/test_option_roundtrip_unit.py | 68 +++-- 5 files changed, 465 insertions(+), 35 deletions(-) diff --git a/docs/source/pcapkit/protocols/internet/ipv4.rst b/docs/source/pcapkit/protocols/internet/ipv4.rst index 27a4719a8..6eb79859f 100644 --- a/docs/source/pcapkit/protocols/internet/ipv4.rst +++ b/docs/source/pcapkit/protocols/internet/ipv4.rst @@ -209,6 +209,8 @@ Type Stubs Auxiliary Functions ~~~~~~~~~~~~~~~~~~~ +.. autofunction:: pcapkit.protocols.schema.internet.ipv4.quick_start_option_length + .. autofunction:: pcapkit.protocols.schema.internet.ipv4.quick_start_data_selector Data Models diff --git a/pcapkit/protocols/internet/ipv4.py b/pcapkit/protocols/internet/ipv4.py index 2048a82b4..c034e9796 100644 --- a/pcapkit/protocols/internet/ipv4.py +++ b/pcapkit/protocols/internet/ipv4.py @@ -1582,6 +1582,16 @@ def _make_opt_ts(self, kind: 'Enum_OptionNumber', option: 'Optional[Data_TSOptio raise ProtocolError(f'{self.alias}: [OptNo {kind}] invalid timestamp value: {timestamp}') pointer = 5 + len(ts_list) * 4 + # NOTE: ``ts_data``, the name of the field on + # :class:`~pcapkit.protocols.schema.internet.ipv4.TSOption`, and not the + # ``data`` this used to pass. ``data`` is the attribute that schema's + # ``post_process`` *derives* from ``ts_data``, so naming it here dropped + # every timestamp: :meth:`Schema.__update__ + # ` warns + # ``UnknownFieldWarning`` for a name it does not know and carries on, which + # left ``ts_data`` bound to its class-level ``ListField`` -- and made the + # IPv4 Timestamp option unbuildable through ``make``, since + # ``post_process`` then iterated the field object itself. See #552. return Schema_TSOption( type=kind, length=length, @@ -1590,7 +1600,7 @@ def _make_opt_ts(self, kind: 'Enum_OptionNumber', option: 'Optional[Data_TSOptio 'oflw': overflow, 'flag': flag, }, - data=ts_list, + ts_data=ts_list, ) def _make_opt_e_sec(self, kind: 'Enum_OptionNumber', option: 'Optional[Data_ESECOption]' = None, *, diff --git a/pcapkit/protocols/schema/internet/ipv4.py b/pcapkit/protocols/schema/internet/ipv4.py index e075a834f..1c6e9978d 100644 --- a/pcapkit/protocols/schema/internet/ipv4.py +++ b/pcapkit/protocols/schema/internet/ipv4.py @@ -4,7 +4,6 @@ import collections import datetime -import ipaddress from typing import TYPE_CHECKING, cast from pcapkit.const.ipv4.classification_level import ClassificationLevel as Enum_ClassificationLevel @@ -14,7 +13,7 @@ from pcapkit.const.ipv4.ts_flag import TSFlag as Enum_TSFlag from pcapkit.const.reg.transtype import TransType as Enum_TransType from pcapkit.corekit.fields.collections import ListField, OptionField -from pcapkit.corekit.fields.ipaddress import IPv4AddressField +from pcapkit.corekit.fields.ipaddress import IPv4AddressField, parse_ip_address from pcapkit.corekit.fields.misc import (ConditionalField, ForwardMatchField, PayloadField, SchemaField, SwitchField) from pcapkit.corekit.fields.numbers import EnumField, UInt8Field, UInt16Field, UInt32Field @@ -104,6 +103,89 @@ class QSNonce(TypedDict): nonce: int +def quick_start_option_length(schema: 'Type[QSOption]') -> 'int': + """On-the-wire length, in octets, of a resolved Quick-Start (``QS``) suboption. + + The Quick-Start suboption schemas re-declare the option's own ``type`` and + ``length`` octets -- they inherit them from :class:`Option` -- so what + :func:`quick_start_data_selector` has to hand the nested + :class:`~pcapkit.corekit.fields.misc.SchemaField` is the length of the + *whole* option, not of the data after its header. Per + :rfc:`4782#section-3.1` -- figure 3 for a Quick-Start Request and figure 4 + for a Report of Approved Rate -- that is eight octets for both functions, + which differ in what the fourth octet holds rather than in how many there + are: *"The second byte contains the length field, indicating an option + length of eight bytes"*, and *"For a Report of Approved Rate, the fourth + byte of the Quick-Start Option is not used"*. And + :meth:`~pcapkit.protocols.internet.ipv4.IPv4._read_opt_qs` rejects any other + value in the ``length`` field outright. + + It is summed from the resolved schema's own fields rather than written as + that literal, for two reasons. The registry is open -- + :class:`QSOption` is an :class:`~pcapkit.protocols.schema.schema.EnumSchema`, + so a caller may register a further function code with a schema of its own + width -- and a number written here has to be kept in step by hand with every + field the suboptions declare, which is precisely how #552 arose: the length + was ``5``, the width of a Quick-Start Request's ``ttl`` and ``nonce`` alone, + with the ``type``, ``length`` and ``flags`` octets in front of them + unaccounted for. + + Args: + schema: Quick-Start suboption schema, as resolved from the ``func`` + sub-field by :func:`quick_start_data_selector`. + + Returns: + Length, in octets, that ``schema`` occupies on the wire. + + Raises: + FieldValueError: If ``schema`` declares a field whose width is not + fixed. Every field of a Quick-Start suboption is fixed-width, + because the option is, and a variable-width one cannot be summed + here without a packet to size it against -- which is the one thing + a selector does not have for the schema it is about to return. It + fails rather than guessing, since guessing is the defect being + fixed. + + """ + length = 0 + for name, field in schema.__fields__.items(): + # NOTE: A forward match consumes nothing and contributes no octets to + # ``bytes(schema)`` either, c.f. the ``ForwardMatchField`` branches of + # :meth:`Schema.pack ` and + # :meth:`Schema.unpack ` + # and the double-count #441/#446 fixed. + if isinstance(field, ForwardMatchField): + continue + + # NOTE: The only conditional field a Quick-Start suboption has is the + # ``length`` octet it inherits from :class:`Option`, whose test is false + # only for ``EOOL`` and ``NOP`` -- neither of which is a Quick-Start + # function -- so it is always on the wire and always counted. + inner = field.field if isinstance(field, ConditionalField) else field + + # NOTE: ``_length_callback`` rather than a ``length < 0`` test, because a + # dynamically sized field does not report a negative width: it reports + # whatever its template says, and the templates fall back to a + # "reasonable default" of ``1024s`` -- measured on ``LSROption.remainder``, + # a ``PaddingField`` with a length callback, which reports 1024. So the + # callback's presence is the only reliable signal, and reaching for it is + # worth one private access. A :class:`~pcapkit.corekit.fields.misc.SwitchField` + # is named separately because it is the one field whose width is dynamic + # *without* a length callback: it carries a + # :class:`~pcapkit.corekit.fields.misc.NoValueField` until its own selector + # resolves it, and so reports a width of zero rather than refusing to + # answer. That is exactly what :class:`_QSOption` -- the outer wrapper, + # which is not itself a suboption -- would hand back here. + if (isinstance(inner, SwitchField) + or getattr(inner, '_length_callback', None) is not None): # pylint: disable=protected-access + raise FieldValueError( + f'IPv4: [OptNo {Enum_OptionNumber.QS}] {schema.__name__}: ' + f'{name!r} is not of a fixed width' + ) + length += field.length + return length + + def quick_start_data_selector(pkt: 'dict[str, Any]') -> 'Field': """Selector function for :attr:`_QSOption.data` field. @@ -118,6 +200,18 @@ def quick_start_data_selector(pkt: 'dict[str, Any]') -> 'Field': wrapped :class:`~pcapkit.protocols.schema.internet.ipv4.QuickStartReportOption` instance. + Notes: + The length handed to the :class:`~pcapkit.corekit.fields.misc.SchemaField` + comes from :func:`quick_start_option_length`, i.e. from the suboption that + was just resolved. It used to be the literal ``5`` for both, which is not + the width of either: a well-formed eight-octet Quick-Start Request + ``1908002adeadbee0`` parsed with ``SchemaWarning: packet length < 0: -3`` + and decoded its ``nonce`` as **55** instead of 933982136, then left three + octets to be read as a further, fabricated option -- which made the + enclosing datagram fail with ``ProtocolError: IPv4: invalid format``. That + is silent corruption on the way to a misleading failure, and it was logged + in review twice before #552 filed it. + """ func = Enum_QSFunction.get(pkt['flags']['func']) pkt['flags']['func'] = func @@ -125,7 +219,7 @@ def quick_start_data_selector(pkt: 'dict[str, Any]') -> 'Field': schema = QSOption.registry[func] if schema is None: raise FieldValueError(f'IPv4: invalid QS function: {func}') - return SchemaField(length=5, schema=schema) + return SchemaField(length=quick_start_option_length(schema), schema=schema) class Option(EnumSchema[Enum_OptionNumber]): @@ -252,6 +346,32 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Schema': Returns: Revised schema. + Raises: + FieldValueError: If an entry of :attr:`ts_data` that the timestamp + flag makes an address is a :obj:`bool`, is not a valid IP + address, or is not IPv4 -- c.f. + :func:`~pcapkit.corekit.fields.ipaddress.parse_ip_address`. + + Notes: + This runs on the **packing** path as well as the unpacking one -- + :meth:`Schema.pack ` + calls it once the buffer is filled -- so the ``ts_data`` entries it + converts below are whatever the caller passed to the constructor, + not octets read off the wire. That is why those conversions go + through :func:`~pcapkit.corekit.fields.ipaddress.parse_ip_address` + rather than :func:`ipaddress.ip_address`: this schema is reachable + from public :meth:`IPv4.make + `, which accepts a + caller-built option schema and packs it, and + :attr:`ts_data`'s :class:`~pcapkit.corekit.fields.numbers.UInt32Field` + item type takes a :obj:`bool` as the :class:`int` it is a subclass + of, so nothing downstream can question it. Measured before this + fix: ``ts_data=[True, 5]`` packed as ``0000000100000005`` and + reported ``IPv4Address('0.0.0.1')`` with no exception and no + warning. This was the fifth site of that defect -- #481, #500, + #539 and #540 are the first four -- and the reason it is the fifth + is that each of those fixed the sites it could see. See #552. + """ ts_flag = Enum_TSFlag.get(self.flags['flag']) if ts_flag == Enum_TSFlag.Timestamp_Only: @@ -275,7 +395,8 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Schema': timestamp = OrderedMultiDict() for ip, ts in zip(ts_data[::2], ts_data[1::2]): - ip_val = cast('IPv4Address', ipaddress.ip_address(ip)) + ip_val = cast('IPv4Address', parse_ip_address( + ip, f'IPv4: [OptNo {self.type}] invalid timestamp address', version=4)) self.data.add(ip_val, ts) if ts >> 31: @@ -290,7 +411,8 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Schema': timestamp = OrderedMultiDict() for ip, ts in zip(ts_data[::2], ts_data[1::2]): - ip_val = cast('IPv4Address', ipaddress.ip_address(ip)) + ip_val = cast('IPv4Address', parse_ip_address( + ip, f'IPv4: [OptNo {self.type}] invalid timestamp address', version=4)) self.data.add(ip_val, ts) if ts >> 31: @@ -302,10 +424,23 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Schema': # extract also the prespecified IP addresses # but set the timestamp to 0 + # + # NOTE: Through ``parse_ip_address`` like the two conversions above, + # even though #540 and #552 both judged this site unable to launder a + # :obj:`bool` -- ``remainder`` is a + # :class:`~pcapkit.corekit.fields.strings.PaddingField`, so what it + # holds is octets rather than anything the caller named. It is routed + # through anyway because a bare :func:`ipaddress.ip_address` here + # still raises a plain :exc:`ValueError` for a tail that is not a + # whole number of 8-octet pairs, which no ``except BaseError`` can + # catch, and because leaving one of this method's three conversions + # unguarded is exactly how #552 came to be the fifth site of #481. pad = self.remainder for index in range(0, len(pad), 8): buf_ip = pad[index:index + 4] - self.data.add(ipaddress.ip_address(buf_ip), 0) # type: ignore[arg-type] + self.data.add(parse_ip_address( # type: ignore[arg-type] + buf_ip, f'IPv4: [OptNo {self.type}] invalid prespecified address', + version=4), 0) else: warn(f'IPv4: [OptNo {self.type}] invalid format: unknown timestmap flag: {ts_flag}', ProtocolWarning) self.data = self.ts_data @@ -320,7 +455,17 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Schema': data: 'list[int] | OrderedMultiDict[IPv4Address, int]' timestamp: 'tuple[int | timedelta] | OrderedMultiDict[IPv4Address, int | timedelta]' - def __init__(self, type: 'Enum_OptionNumber', length: 'int', pointer: 'int', flags: 'TSFlags', data: 'list[int]') -> 'None': ... + # NOTE: The keyword is ``ts_data``, the name of the field above, not the + # ``data`` this signature used to advertise. ``data`` is the *derived* + # attribute :meth:`post_process` writes and is declared three lines up; + # naming it here as a constructor argument too is what + # :meth:`~pcapkit.protocols.internet.ipv4.IPv4._make_opt_ts` was written + # against, and because :meth:`Schema.__update__ + # ` answers an unknown + # field name with an + # :class:`~pcapkit.utilities.warnings.UnknownFieldWarning` rather than an + # error, the timestamps were dropped in silence. See #552. + def __init__(self, type: 'Enum_OptionNumber', length: 'int', pointer: 'int', flags: 'TSFlags', ts_data: 'list[int]') -> 'None': ... @schema_final @@ -516,6 +661,25 @@ def __init__(self, type: 'Enum_OptionNumber', length: 'int', flags: 'QuickStartF class QuickStartReportOption(QSOption, code=Enum_QSFunction.Report_of_Approved_Rate): """Header schema for IPV4 quick start report of approved rate options.""" + #: Not used. One octet, holding the place a Quick-Start Request fills with + #: ``QS TTL``: :rfc:`4782#section-3.1` says in as many words that *"for a + #: Report of Approved Rate, the fourth byte of the Quick-Start Option is not + #: used"*, and that *"bytes 5-8 contain a 30-bit QS Nonce and a 2-bit + #: Reserved field"* -- so the nonce begins at the fifth octet for both + #: functions, and figure 4 gives this option as ``Length=8`` like figure 3 + #: gives its sibling. The field was missing, so the schema was seven octets + #: wide against the eight + #: :meth:`~pcapkit.protocols.internet.ipv4.IPv4._make_opt_qs` writes into + #: ``length`` and the eight + #: :meth:`~pcapkit.protocols.internet.ipv4.IPv4._read_opt_qs` demands of it, + #: which meant a spec-correct Report of Approved Rate read off the wire + #: decoded its ``nonce`` one octet early -- measured, with the selector + #: length fixed and this field still absent: ``19088100deadbee0`` warned + #: ``packet length < 0: -1`` and then died with a bare ``struct.error: bad + #: char in struct format``, the unconsumed octet having been read as another + #: option. Declared as padding rather than as data because :rfc:`4782` gives + #: it no meaning and no caller should be setting it. See #552. + reserved: 'bytes' = PaddingField(length=1) #: QS nonce. nonce: 'QSNonce' = BitField(length=4, namespace={ 'nonce': (0, 30), diff --git a/tests/protocols/internet/test_ipv4_unit.py b/tests/protocols/internet/test_ipv4_unit.py index 445c99dc5..bd380dd9a 100644 --- a/tests/protocols/internet/test_ipv4_unit.py +++ b/tests/protocols/internet/test_ipv4_unit.py @@ -750,6 +750,246 @@ def test_ipv4_sid_option_is_four_octets_wide_on_the_wire(self) -> None: self.assertEqual(rebuilt[0] & 0x0F, 6) self.assertEqual(int.from_bytes(rebuilt[2:4], 'big'), 24) + def test_ipv4_timestamp_option_refuses_a_bool_as_an_address(self) -> None: + """``TSOption`` will not turn a ``bool`` into an address. C.f. #552. + + :meth:`TSOption.post_process + ` runs on the + **packing** path as well as the unpacking one, so the ``ts_data`` entries it + reads as addresses under the ``IP with Timestamp`` and ``Prespecified IP + with Timestamp`` flags are whatever the caller passed. It converted them + with a bare :func:`ipaddress.ip_address`, which takes any :class:`int` + below ``2**32`` -- and :obj:`bool` is an :class:`int` subclass, so + ``ts_data=[True, 5]`` became ``IPv4Address('0.0.0.1')`` with no exception + and no warning. The measured pack was + ``440c050100000001000000050000000000000000``. + + Nothing downstream could have caught it either: the field's item type is a + :class:`~pcapkit.corekit.fields.numbers.UInt32Field`, which packs ``True`` + as the ``1`` it is, so #500's guard in + :meth:`_IPAddressField.pre_process + ` is never + reached. This was the fifth site of that defect -- #481, #500, #539 and + #540 are the first four -- and the fix is the same one #539 used for its + seven: route the conversion through + :func:`~pcapkit.corekit.fields.ipaddress.parse_ip_address`. + + Both halves are asserted, because a guard that rejects everything is not a + fix: a real address still has to pack, and still has to come back out of + ``data`` and ``timestamp`` as an address. + + """ + from pcapkit.const.ipv4.option_number import OptionNumber + from pcapkit.const.ipv4.ts_flag import TSFlag + from pcapkit.protocols.internet.ipv4 import IPv4 + from pcapkit.protocols.schema.internet.ipv4 import TSOption + from pcapkit.utilities.exceptions import FieldValueError + + def option(ts_data: 'list[int]', flag: 'TSFlag') -> 'TSOption': + return TSOption(type=OptionNumber.TS, length=12, pointer=13, + flags={'oflw': 0, 'flag': flag}, ts_data=ts_data) + + addressed = (TSFlag.IP_with_Timestamp, TSFlag.Prespecified_IP_with_Timestamp) + for flag in addressed: + for value in (True, False): + with self.subTest(flag=flag, value=value): + with self.assertRaises(FieldValueError) as caught: + option([value, 5], flag).pack() + message = str(caught.exception) + self.assertIn('must not be a bool', message) + self.assertIn(f'[OptNo {OptionNumber.TS}]', message) + + # Reachable through the public construction API, which is what makes this + # a defect rather than an internal curiosity: ``make`` takes a + # caller-built option schema and packs it. + with self.assertRaises(FieldValueError): + IPv4(src='127.0.0.1', dst='127.0.0.2', ttl=64, id=1, + options=[option([True, 5], TSFlag.IP_with_Timestamp)], payload=b'') + + # The control. A real address packs, and reads back as an address. + real = option([int(ip_address('192.0.2.1')), 5], TSFlag.IP_with_Timestamp) + self.assertEqual(real.pack().hex(), '440c0d01c000020100000005') + self.assertEqual(real.data[ip_address('192.0.2.1')], 5) + self.assertEqual(real.timestamp[ip_address('192.0.2.1')], + datetime.timedelta(milliseconds=5)) + + # And the third conversion in the same method, the one that reads the + # prespecified addresses out of the option's padding. + prespecified = option([int(ip_address('192.0.2.2')), 6], + TSFlag.Prespecified_IP_with_Timestamp) + prespecified.remainder = ip_address('192.0.2.3').packed + bytes(4) + prespecified.pack() + self.assertEqual(prespecified.data[ip_address('192.0.2.2')], 6) + self.assertEqual(prespecified.data[ip_address('192.0.2.3')], 0) + + def test_ipv4_quick_start_option_is_eight_octets_wide_on_the_wire(self) -> None: + """Both Quick-Start suboptions survive the round trip whole. C.f. #552. + + :func:`~pcapkit.protocols.schema.internet.ipv4.quick_start_data_selector` + sized the nested suboption schema with a hardcoded + ``SchemaField(length=5)``, which is the width of a Quick-Start Request's + ``ttl`` and ``nonce`` alone -- the ``type``, ``length`` and ``flags`` + octets in front of them, which the suboption schema re-declares, were + unaccounted for. :rfc:`4782#section-3.1` gives the option as eight octets + for both functions -- *"the second byte contains the length field, + indicating an option length of eight bytes"*, with figure 3 for a Request + and figure 4 for a Report -- and + :meth:`~pcapkit.protocols.internet.ipv4.IPv4._read_opt_qs` rejects any + other ``length`` outright. + + So the reader consumed five, warned ``SchemaWarning: packet length < 0: + -3``, resynchronised on the second octet of the nonce and decoded it as + **55** rather than 933982136 -- silent corruption -- and then read the + three octets it had not consumed as a further, fabricated option, which + made the whole datagram fail with ``ProtocolError: IPv4: invalid format``. + + ``QuickStartReportOption`` was one octet short as well, missing the ``Not + Used`` octet that :rfc:`4782#section-3.1` puts where a Request has ``QS + TTL`` -- *"for a Report of Approved Rate, the fourth byte of the + Quick-Start Option is not used"*, and *"bytes 5-8 contain a 30-bit QS + Nonce and a 2-bit Reserved field"*, so the nonce starts at the fifth + octet either way. It therefore packed seven octets against the + ``length=8`` that ``_make_opt_qs`` writes into it, and a spec-correct + Report read off the wire decoded its nonce one octet early. Both + functions are asserted here for that reason, since fixing only the + selector leaves the Report failing on a bare ``struct.error`` -- measured + on ``19088100deadbee0`` in exactly that state. + + This starts from wire octets, like + :meth:`test_ipv4_sid_option_is_four_octets_wide_on_the_wire` and for the + same reason: the selector's length is only consulted while *parsing*, so a + construct-then-reconstruct comparison would have agreed with itself. + + """ + from pcapkit.const.ipv4.option_number import OptionNumber + from pcapkit.protocols.internet.ipv4 import IPv4 + from pcapkit.protocols.schema.internet import ipv4 as ipv4_schema + from pcapkit.utilities.exceptions import FieldValueError + + self.assertEqual( + ipv4_schema.quick_start_option_length(ipv4_schema.QuickStartRequestOption), 8) + self.assertEqual( + ipv4_schema.quick_start_option_length(ipv4_schema.QuickStartReportOption), 8) + + # A variable-width field cannot be sized without a packet, which a + # selector does not have for the schema it is about to return, so the + # helper says so rather than guessing. Both shapes of variable width are + # asserted, since they are detected differently: ``LSROption.route`` is a + # ``ListField`` with a length *callback*, while ``_QSOption.data`` is a + # ``SwitchField``, which has none and reports a width of zero until its own + # selector resolves it -- a silent nought is the one answer worse than a + # wrong one here. ``_QSOption`` also carries the ``ForwardMatchField`` that + # the sum has to skip, since a forward match consumes nothing. Neither + # class is a Quick-Start suboption, and asking about either registers + # nothing and mutates nothing. + for schema, field in ((ipv4_schema.LSROption, 'route'), + (ipv4_schema._QSOption, 'data')): + with self.subTest(schema=schema.__name__): + with self.assertRaises(FieldValueError) as caught: + ipv4_schema.quick_start_option_length(schema) + self.assertIn(f'{field!r} is not of a fixed width', str(caught.exception)) + + for label, option in (('request', bytes.fromhex('1908012adeadbee0')), + ('report', bytes.fromhex('19088100deadbee0'))): + with self.subTest(option=label): + header = bytes.fromhex('47000000 00000000 00060000 ' + '7f000001 7f000002') + option + header = header[:2] + len(header).to_bytes(2, 'big') + header[4:] + self.assertEqual(len(header), 28) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + parsed = IPv4(header, len(header)) + messages = [str(entry.message) for entry in caught] + + # The under-read was the library naming its own defect. + self.assertEqual([entry for entry in messages + if 'packet length < 0' in entry], [], messages) + + # One option, not the Quick-Start plus a fabricated tail. + self.assertEqual( + [code for code, _ in parsed.info.options.items(multi=True)], + [OptionNumber.QS], + ) + + qs = parsed.info.options[OptionNumber.QS] + self.assertEqual(qs.length, 8) + self.assertEqual(qs.nonce, 933982136) + self.assertEqual(qs.rate, 80) + + # And the option rebuilds to exactly the octets it was read from. + proto = object.__new__(IPv4) + self.assertEqual(proto._make_opt_qs(OptionNumber.QS, qs).pack(), option) + + def test_ipv4_timestamp_option_is_buildable_through_make(self) -> None: + """``make`` keeps the timestamps it is given. C.f. #552. + + ``_make_opt_ts`` passed ``data=`` to + :class:`~pcapkit.protocols.schema.internet.ipv4.TSOption`, whose field is + ``ts_data`` -- ``data`` is the attribute its ``post_process`` *derives*. + :meth:`Schema.__update__ + ` answers a name it does + not know with an + :class:`~pcapkit.utilities.warnings.UnknownFieldWarning` and carries on, so + every timestamp was dropped in silence and ``ts_data`` stayed bound to its + class-level ``ListField``, which ``post_process`` then tried to iterate: + ``TypeError: 'ListField' object is not iterable``. The IPv4 Timestamp + option was unbuildable through ``make`` for as long as that stood, which is + what the ``ipv4-option/TS`` entry of + :data:`tests.protocols.test_option_roundtrip_unit.EXPECTED_FAILURES` + recorded. + + The absence of the warning is asserted as well as the presence of the + timestamps, because the warning is the whole reason the defect was silent: + a future rename that reintroduces it would otherwise only show up as a + value that happens to be missing. + + """ + from pcapkit.const.ipv4.option_number import OptionNumber + from pcapkit.const.ipv4.ts_flag import TSFlag + from pcapkit.protocols.internet.ipv4 import IPv4 + from pcapkit.utilities.warnings import UnknownFieldWarning + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + datagram = IPv4(src='127.0.0.1', dst='127.0.0.2', ttl=64, id=1, + options=[(OptionNumber.TS, { + 'counts': 2, + 'timestamp': [datetime.timedelta(seconds=1), 2000], + })], payload=b'') + self.assertEqual( + [str(entry.message) for entry in caught + if issubclass(entry.category, UnknownFieldWarning)], [], + [str(entry.message) for entry in caught], + ) + + ts = datagram.info.options[OptionNumber.TS] + self.assertEqual(ts.flag, TSFlag.Timestamp_Only) + self.assertEqual(ts.timestamp, (datetime.timedelta(seconds=1), + datetime.timedelta(seconds=2))) + + raw = bytes(datagram) + self.assertEqual(raw[20:].hex(), '440c0d00000003e8000007d0') + + # Parsed back, and rebuilt from what was parsed, to the same octets. + again = IPv4(raw, len(raw)) + self.assertEqual(again.info.options[OptionNumber.TS].timestamp, ts.timestamp) + self.assertEqual(bytes(IPv4.from_data(again.info)), raw) + + # The other shape the maker accepts, which reaches the addressed branch of + # ``post_process`` rather than the timestamp-only one. + with_ip = IPv4(src='127.0.0.1', dst='127.0.0.2', ttl=64, id=1, + options=[(OptionNumber.TS, { + 'counts': 1, + 'timestamp': { + ip_address('192.0.2.1'): datetime.timedelta(seconds=3), + }, + })], payload=b'') + addressed = with_ip.info.options[OptionNumber.TS] + self.assertEqual(addressed.flag, TSFlag.IP_with_Timestamp) + self.assertEqual(addressed.timestamp[ip_address('192.0.2.1')], + datetime.timedelta(seconds=3)) + 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 diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index b35ad7a04..723788eea 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -211,15 +211,16 @@ class Gap(NamedTuple): # now in :meth:`IPv4UnitTests.test_ipv4_sid_option_is_four_octets_wide_on_the_wire # `. - # ``_make_opt_ts`` passes ``data=`` where the schema field is ``ts_data``. - # ``Schema.__init__`` only warns about an unknown field name and carries on, - # so the value is dropped and the attribute stays bound to the class-level - # descriptor -- which ``post_process`` then tries to iterate. - 'ipv4-option/TS': Gap( - 'CONSTRUCT', "'ListField' object is not iterable", - 'pcapkit/protocols/internet/ipv4.py:1550 -- data= should be ts_data=, ' - 'dropped with UnknownFieldWarning and surfacing at ' - 'pcapkit/protocols/schema/internet/ipv4.py:262'), + # ``_make_opt_ts`` used to be recorded here too. It passed ``data=`` where the + # schema field is ``ts_data``; ``Schema.__update__`` only warns about an + # unknown field name and carries on, so the value was dropped and the + # attribute stayed bound to the class-level ``ListField`` descriptor -- which + # ``post_process`` then tried to iterate, ``'ListField' object is not + # iterable``. The IPv4 Timestamp option was unbuildable through ``make`` for + # as long as that stood. #552 passes ``ts_data=`` and corrects the + # ``TYPE_CHECKING`` ``__init__`` stub that advertised ``data`` and is what the + # maker was written against, so ``ipv4-option/TS`` round-trips and has no + # entry here any more. # -- Quick-Start, in all three protocols that carry it -------------------- @@ -227,34 +228,47 @@ class Gap(NamedTuple): # ``_QSOption.post_process``, which runs on the parse path. Since # ``__post_init__`` packs and then re-reads, construction fails. # - # There is a second, independent defect in the same option that this case - # never reaches, and it is the more serious of the two: - # ``quick_start_data_selector`` hands the nested schema a hardcoded - # ``SchemaField(length=5)`` where the schema needs eight octets + # There was a second, independent defect in the same option that these cases + # never reach, and it was the more serious of the two: + # ``quick_start_data_selector`` handed the nested schema a hardcoded + # ``SchemaField(length=5)`` where a Quick-Start Request needs eight octets # (type 1 + length 1 + flags 1 + ttl 1 + nonce 4). Measured on a # hand-built, well-formed 8-octet IPv4 Quick-Start option - # ``1908002adeadbee0``: it parses "successfully" with - # ``SchemaWarning: packet length < 0: -3`` and decodes ``nonce`` as **55** + # ``1908002adeadbee0``: it parsed "successfully" with + # ``SchemaWarning: packet length < 0: -3`` and decoded ``nonce`` as **55** # instead of 933982136 -- silent corruption rather than a failure -- and the - # three unconsumed octets are then read as a further, fabricated option, - # which makes the enclosing IPv4 packet unparseable. The identical - # ``SchemaField(length=5)`` is at hopopt.py:224 and ipv6_opts.py:224, with - # the same measured nonce of 55. Fixing the ``func`` defect alone will not - # make these cases pass. + # three unconsumed octets were then read as a further, fabricated option, + # which made the enclosing IPv4 datagram fail with ``ProtocolError: IPv4: + # invalid format``. + # + # #552 fixed IPv4's copy: the length now comes from + # ``quick_start_option_length(schema)``, i.e. from the suboption the selector + # resolved, and ``QuickStartReportOption`` gained the ``Not Used`` octet + # :rfc:`4782#section-3.1` gives it and it was missing -- it packed seven + # octets against the ``length=8`` that ``_make_opt_qs`` writes and + # ``_read_opt_qs`` demands, so a spec-correct Report of Approved Rate read off + # the wire decoded its nonce one octet early. ``ipv4-option/QS`` is still + # recorded below because the ``func`` defect is untouched and fails first. + # + # The identical ``SchemaField(length=5)`` is still at + # ``schema/internet/hopopt.py:255`` and ``schema/internet/ipv6_opts.py:255``, + # with the same measured nonce of 55; #552 was scoped to IPv4's copy. Note a + # fix there is not a copy of this one: :rfc:`4782#section-3.2` sets the IPv6 + # option's ``length`` field to 6 rather than 8, since it excludes the common + # type and length octets the extension header already carries. Fixing + # the ``func`` defect alone will not make any of these three cases pass. 'ipv4-option/QS': Gap( 'CONSTRUCT', "no attribute 'func'", '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'), + 'post_process'), 'hopopt-option/Quick_Start': Gap( 'CONSTRUCT', "no attribute 'func'", - 'pcapkit/protocols/internet/hopopt.py:869; and separately ' - 'pcapkit/protocols/schema/internet/hopopt.py:224 -- SchemaField(length=5)'), + 'pcapkit/protocols/internet/hopopt.py:918; and separately ' + 'pcapkit/protocols/schema/internet/hopopt.py:255 -- SchemaField(length=5)'), 'ipv6-opts-option/Quick_Start': Gap( 'CONSTRUCT', "no attribute 'func'", - 'pcapkit/protocols/internet/ipv6_opts.py:881; and separately ' - 'pcapkit/protocols/schema/internet/ipv6_opts.py:224 -- ' + 'pcapkit/protocols/internet/ipv6_opts.py:921; and separately ' + 'pcapkit/protocols/schema/internet/ipv6_opts.py:255 -- ' 'SchemaField(length=5)'), # -- The non-progress loop, now fully fixed -------------------------------