From 5e27d305615484d83c22a335cb288adc667f38ea Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 26 Oct 2026 16:37:44 -0400 Subject: [PATCH] fix(ipv4): build the TS option from its default timestamp, and print a bad one with !r (#2277) Problem: _make_opt_ts accepted only a list or a dict for timestamp, but its default is None, so a TS option made without timestamps always raised ProtocolError; the error printed the value without !r. Fix: treat None as an empty list before the type dispatch, so it builds a Timestamp-only option with `counts` empty slots; any other non-list, non-dict raises "invalid timestamp value: must be a list or a dict, not ". The docstring's Raises names ProtocolError for exactly that. Test: test_ipv4_ts_default_unit pins the default option make -> parse -> rebuild byte for byte and the message text; test_ipv4_unit's timestamp=None ProtocolError case now uses 'abc'. --- examples/generators/options.py | 3 +- pcapkit/protocols/internet/ipv4.py | 10 +-- .../internet/test_ipv4_ts_default_unit.py | 70 +++++++++++++++++++ tests/protocols/internet/test_ipv4_unit.py | 2 +- 4 files changed, 79 insertions(+), 6 deletions(-) create mode 100644 tests/protocols/internet/test_ipv4_ts_default_unit.py diff --git a/examples/generators/options.py b/examples/generators/options.py index 2c6a7dc402..af07da81fd 100644 --- a/examples/generators/options.py +++ b/examples/generators/options.py @@ -541,7 +541,8 @@ def _ipv4_overrides() -> 'dict[Any, dict[str, Any]]': OptionNumber.LSR: {'counts': 9}, OptionNumber.RR: {'counts': 9}, OptionNumber.SSR: {'counts': 9}, - # ``timestamp=None``, the default, is rejected outright. + # The default builds an option with no timestamp in it, which is + # degenerate; one is recorded. OptionNumber.TS: {'counts': 1, 'timestamp': [1]}, OptionNumber.E_SEC: {'info': b'\x00'}, } diff --git a/pcapkit/protocols/internet/ipv4.py b/pcapkit/protocols/internet/ipv4.py index 511996f78a..4d5b0a9871 100644 --- a/pcapkit/protocols/internet/ipv4.py +++ b/pcapkit/protocols/internet/ipv4.py @@ -1700,7 +1700,7 @@ def _make_opt_ts(self, kind: 'Enum_OptionNumber', option: 'Optional[Data_TSOptio timestamp: list of timestamps, i.e. the slots before the pointer, or a mapping of the addresses to their timestamps; an address may be anything :func:`~pcapkit.corekit.fields.ipaddress.parse_ip_address` - accepts for IPv4 + accepts for IPv4; :data:`None` is an empty list remaining: raw 32-bit words for the slots at or beyond the pointer; the slots left over up to ``counts`` are zero **kwargs: arbitrary keyword arguments @@ -1710,8 +1710,7 @@ def _make_opt_ts(self, kind: 'Enum_OptionNumber', option: 'Optional[Data_TSOptio Raises: ProtocolError: If ``option`` is not given and ``timestamp`` is - neither a :obj:`list` nor a :obj:`dict`, which includes the - default :data:`None`. + neither :data:`None`, a :obj:`list`, nor a :obj:`dict`. FieldValueError: If an address is a :obj:`bool` or is not a valid IPv4 address. @@ -1748,6 +1747,8 @@ def _make_opt_ts(self, kind: 'Enum_OptionNumber', option: 'Optional[Data_TSOptio else: ts_list = [] remainder = b'' + if timestamp is None: + timestamp = [] if isinstance(timestamp, list): flag = Enum_TSFlag.Timestamp_Only # type: ignore[assignment] counts = min(9, counts) # 9 is the maximum number of timestamps @@ -1785,7 +1786,8 @@ def _make_opt_ts(self, kind: 'Enum_OptionNumber', option: 'Optional[Data_TSOptio flag = Enum_TSFlag.Prespecified_IP_with_Timestamp # type: ignore[assignment] ts_list.append(ts) else: - raise ProtocolError(f'{self.alias}: [OptNo {kind}] invalid timestamp value: {timestamp}') + raise ProtocolError(f'{self.alias}: [OptNo {kind}] invalid timestamp value: ' + f'must be a list or a dict, not {timestamp!r}') pointer = 5 + len(ts_list) * 4 # NOTE: Every slot of the option is written, the empty ones as zero. diff --git a/tests/protocols/internet/test_ipv4_ts_default_unit.py b/tests/protocols/internet/test_ipv4_ts_default_unit.py new file mode 100644 index 0000000000..644e2f2b0f --- /dev/null +++ b/tests/protocols/internet/test_ipv4_ts_default_unit.py @@ -0,0 +1,70 @@ +# -*- coding: utf-8 -*- +"""The IPv4 Timestamp option builds from its default ``timestamp``. + +GitHub issue #2277: ``_make_opt_ts`` dispatched on ``timestamp`` being a +:obj:`list` or a :obj:`dict`, but its default is :data:`None`, so a ``TS`` +option made without timestamps always raised ``ProtocolError``. :data:`None` is +an empty list: a Timestamp-only option with ``counts`` empty slots. Any other +value still raises, with the value printed by :func:`repr`. + +Every case builds its own octets in memory and reads no capture. Classes are +imported inside each test, after :func:`~tests._support.reimport_once_per_class`. + +""" + +import io +import unittest + +from tests._support import reimport_once_per_class + +#: Test tier (#1958): hand-picked round-trip cases, not a sweep. +__tier__ = 'ALL' + +#: The default ``TS`` option: length 24, pointer 5, Timestamp-only, five empty slots. +DEFAULT_TS = '44180500' + '00000000' * 5 + + +class TestIPv4TimestampDefault(unittest.TestCase): + """Pin the default TS option through make, parse and rebuild.""" + + def setUp(self) -> None: + reimport_once_per_class(self) + + def make(self, option: 'dict[str, object]') -> 'bytes': + from pcapkit.const.ipv4.option_number import OptionNumber + from pcapkit.protocols.internet.ipv4 import IPv4 + + return bytes(IPv4(src='10.0.0.1', dst='10.0.0.2', ttl=64, id=0, + options=[(OptionNumber.TS, option)], payload=b'')) + + def test_default_timestamp_round_trips_byte_for_byte(self) -> None: + from pcapkit.const.ipv4.option_number import OptionNumber + from pcapkit.const.ipv4.ts_flag import TSFlag + from pcapkit.protocols.internet.ipv4 import IPv4 + + for name, option in (('omitted', {}), ('None', {'timestamp': None})): + with self.subTest(timestamp=name): + data = self.make(option) + self.assertEqual(data[20:].hex(), DEFAULT_TS) + self.assertEqual(data.hex(), self.make({'timestamp': []}).hex()) + + parsed = IPv4(io.BytesIO(data), len(data)) + ts = parsed.info.options[OptionNumber.TS] # type: ignore[attr-defined] + self.assertEqual((ts.length, ts.pointer, ts.flag, ts.timestamp), + (24, 5, TSFlag.Timestamp_Only, ())) + self.assertEqual(bytes(IPv4.from_data(parsed.info)).hex(), data.hex()) # type: ignore[attr-defined] + + def test_invalid_timestamp_message_uses_repr(self) -> None: + from pcapkit.const.ipv4.option_number import OptionNumber + from pcapkit.protocols.internet.ipv4 import IPv4 + from pcapkit.utilities.exceptions import ProtocolError + + proto = IPv4.__new__(IPv4) + with self.assertRaises(ProtocolError) as ctx: + proto._make_opt_ts(OptionNumber.TS, timestamp='abc') # type: ignore[arg-type] + self.assertEqual(str(ctx.exception), + "IPv4: [OptNo 68] invalid timestamp value: must be a list or a dict, not 'abc'") + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/internet/test_ipv4_unit.py b/tests/protocols/internet/test_ipv4_unit.py index a57e1557d0..7aab6a6a88 100644 --- a/tests/protocols/internet/test_ipv4_unit.py +++ b/tests/protocols/internet/test_ipv4_unit.py @@ -1385,7 +1385,7 @@ def test_ipv4_option_constructors_cover_common_and_error_branches(self) -> None: self.assertEqual(padded[-1].type, OptionNumber.EOOL) with self.assertRaises(ProtocolError): - proto._make_opt_ts(OptionNumber.TS, timestamp=None) + proto._make_opt_ts(OptionNumber.TS, timestamp='abc') # type: ignore[arg-type] with self.assertRaises(ProtocolError): proto._make_opt_qs(OptionNumber.QS, func=99)