From 2e84ed2a604feac6ecfec6e07fa9482c15524cfb Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Thu, 8 Oct 2026 14:46:39 -0400 Subject: [PATCH] fix(generators): correct stale options.py comments and the code they justified (#1337) - ILNP nonce: the width is the ceiling of the bit length in octets, at least one; nonce=0 is one octet and every width 1-16 round-trips. - _httpv2_rebuild: pass the raw flags octet and the reserved bit on, as HTTP._make_data does. Since #1341 make ORs flags in, so withholding it dropped undefined flag bits and the reserved bit on rebuild. - SKIP: TCP EOOL and NOP are no longer dropped by _make_tcp_options (#1163), so they get cases. The rationale now covers only HOPOPT and IPv6-Opts PadN, which list input still collapses onto Pad1. - IPv4 extract comment: the padding drop applies to a fresh option list. New tests/protocols/test_option_generator_1337_unit.py; 9 subtests fail without the fix. --- examples/generators/options.py | 41 +++--- .../test_option_generator_1337_unit.py | 120 ++++++++++++++++++ 2 files changed, 141 insertions(+), 20 deletions(-) create mode 100644 tests/protocols/test_option_generator_1337_unit.py diff --git a/examples/generators/options.py b/examples/generators/options.py index 2fceb7751f..89256ac33a 100644 --- a/examples/generators/options.py +++ b/examples/generators/options.py @@ -525,8 +525,8 @@ def _ipv4_parse(octets: 'bytes') -> 'Any': def _ipv4_extract(parsed: 'Any') -> 'Any': from pcapkit.corekit.multidict import OrderedMultiDict - # EOOL and NOP are dropped by ``_make_ipv4_options`` as padding, so the - # header they produce has no options at all and ``info`` carries no + # ``_make_ipv4_options`` drops EOOL and NOP from a fresh option list as + # padding, so the header a case builds from either has no options at all and ``info`` carries no # ``options`` attribute. An empty collection is the honest reading of that, # and it keeps the two padding codes from being reported as parse failures. return getattr(parsed.info, 'options', OrderedMultiDict()) @@ -558,9 +558,10 @@ def _ipv6_option_overrides() -> 'dict[Any, dict[str, Any]]': # HOPOPT and IPv6-Opts key on the same enumeration and behave identically # on every code, so one table serves both. return { - # ``nonce=0`` gives a zero-octet nonce. Only widths 3, 5, 6 and 7 pack - # at all, because a callable-length NumberField never clears the flag - # that says "hand struct a bytes"; 0xFFFFFF is three octets. + # The nonce is sized to its bit length rounded up to whole octets, at + # least one, so the default ``nonce=0`` is a one-octet zero. A + # three-octet value exercises a multi-octet width; every width from 1 + # to 16 octets builds and round-trips. Enum_Option.ILNP_Nonce: {'nonce': 0xFFFFFF}, Enum_Option.Line_Identification_Option: {'id': b'line-1'}, } @@ -1163,10 +1164,14 @@ def _httpv2_extract(parsed: 'Any') -> 'Any': def _httpv2_rebuild(frame: 'Any') -> 'Any': from pcapkit.protocols.application.httpv2 import HTTP - # ``flags`` is deliberately not passed on: ``HTTP.make`` rebinds it from - # the ``_make_http_*`` return whenever ``frame`` is a mapping or a Data - # object, so passing it would be silently ignored. - return HTTP(type=frame.type, sid=frame.sid, frame=frame) + # ``HTTP.make`` ORs ``flags`` into the bits the ``_make_http_*`` constructor + # derives from ``frame``, and that constructor knows only the flags its + # frame type defines. So the raw flags octet is passed on, as + # ``HTTP._make_data`` does, or a bit the type leaves undefined is dropped. + # ``reserved`` is passed on for the same reason: ``make`` defaults it to 0. + return HTTP(type=frame.type, sid=frame.sid, reserved=frame.reserved, + flags=frame.flags.__value__ if frame.flags is not None else 0, + frame=frame) ############################################################################### @@ -1625,21 +1630,17 @@ def _pcapng_secrets_build(code: 'Any', kwargs: 'dict[str, Any]') -> 'Any': #: Codes deliberately left out of the case list, with the reason. #: -#: The padding options are dropped by every ``_make_*_options`` in the tree -- -#: it discards them and inserts its own padding to reach the alignment the -#: protocol wants -- so a case for one would assert only that construction -#: ignores its own input, which it does. They are kept for TCP, where the -#: dropping is symmetric and the case is a genuine round trip of an -#: option-less header, and skipped where a second padding code would merely -#: duplicate the first. +#: Every padding option has a case except PadN in HOPOPT and IPv6-Opts. Their +#: ``_make_*_options`` keep the padding of parsed options in place, but drop +#: any in a list of fresh options and insert their own to reach 8-octet +#: alignment. A case is built from such a list, so the Pad1 and PadN cases +#: would both emit the same option-less header; Pad1 is kept, PadN skipped. +#: ``_make_tcp_options`` emits ``EOOL`` and ``NOP`` as given, so the TCP +#: padding cases are distinct headers and both round-trip. SKIP = { - ('tcp-option', 'End_of_Option_List'): 'padding, dropped by _make_tcp_options', - ('tcp-option', 'No_Operation'): 'padding, dropped by _make_tcp_options', # Multipath TCP is the envelope for the whole tcp-mptcp family, so a case # here would silently duplicate whichever subtype it defaulted to. ('tcp-option', 'Multipath_TCP'): 'envelope for the tcp-mptcp family', - # Pad1 and PadN are both discarded by _make_hopopt_options, so the two - # cases emit byte-identical option-less headers. ('hopopt-option', 'PadN'): 'padding, indistinguishable from Pad1 here', ('ipv6-opts-option', 'PadN'): 'padding, indistinguishable from Pad1 here', } diff --git a/tests/protocols/test_option_generator_1337_unit.py b/tests/protocols/test_option_generator_1337_unit.py new file mode 100644 index 0000000000..654c1798dd --- /dev/null +++ b/tests/protocols/test_option_generator_1337_unit.py @@ -0,0 +1,120 @@ +# -*- coding: utf-8 -*- +"""What #1337 changed in :file:`examples/generators/options.py`. + +Two comments there justified code by limitations that no longer hold. + +* ``_httpv2_rebuild`` withheld ``flags``, on the grounds that ``HTTP.make`` + ignored it. Since #1341 ``make`` ORs ``flags`` into the bits the frame + constructor derives, and that constructor knows only the bits its frame type + defines. So a parsed frame with an undefined flag bit, or the reserved bit, + lost it on the generator's rebuild although ``HTTP.from_data`` keeps it. +* ``SKIP`` left out TCP ``EOOL`` and ``NOP`` as "dropped by + ``_make_tcp_options``". Since #1163 that maker emits them as given, so each is + a distinct header that round-trips, and both now have a case. + +This module is unit tier: it builds everything in memory and reads no capture. + +""" + +from __future__ import annotations + +import importlib.util +import sys +import types +import unittest +import warnings + +from tests._support import time_limit +from tests._tiers import ROOT + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: Parsed HTTP/2 frames whose flags octet or reserved bit the frame constructor +#: cannot derive. Each is ``(label, frame octets)``. +HTTPV2_FRAMES = ( + # DATA defines END_STREAM (0x01) and PADDED (0x08) only. + ('DATA, undefined flag 0x02', '000003000200000001616263'), + ('DATA, undefined flag 0x80', '000003008000000001616263'), + # PING defines ACK (0x01) only. + ('PING, undefined flag 0x40', '0000080640000000000000000000000000'), + # WINDOW_UPDATE defines no flags at all. + ('WINDOW_UPDATE, flag 0x02', '00000408020000000100000000'), + # The reserved bit is the top bit of the stream identifier. + ('DATA, reserved bit set', '000003000080000001616263'), +) + +#: TCP header octets a TCP padding case must carry after the fixed 20-octet +#: header: the option as given, then alignment to 32 bits. +TCP_PADDING = { + 'End_of_Option_List': '00000000', + 'No_Operation': '01010100', +} + + +def _load_generator() -> 'types.ModuleType': + """Load :file:`examples/generators/options.py` by path, under a name of its own. + + Returns: + The generator module. + + Raises: + RuntimeError: If the module cannot be found or loaded. + + """ + path = ROOT / 'examples' / 'generators' / 'options.py' + spec = importlib.util.spec_from_file_location('pcapkit_samples_options_1337', path) + if spec is None or spec.loader is None: # pragma: no cover + raise RuntimeError(f'cannot load the option generator from {path}') + + module = importlib.util.module_from_spec(spec) + sys.modules[spec.name] = module + spec.loader.exec_module(module) + return module + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class OptionGenerator1337Tests(unittest.TestCase): + """The generator code the stale #1337 comments used to justify.""" + + options = None # type: types.ModuleType + + @classmethod + def setUpClass(cls) -> None: + cls.options = _load_generator() + + def test_httpv2_rebuild_keeps_the_flags_octet_and_reserved_bit(self) -> None: + """A parsed frame rebuilds byte-exact through ``_httpv2_rebuild``.""" + family = self.options.FAMILY_MAP['httpv2-frame'] + for label, hexstr in HTTPV2_FRAMES: + with self.subTest(frame=label): + octets = bytes.fromhex(hexstr) + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + with time_limit(): + info = family.extract(family.parse(octets)) + rebuilt = family.rebuild(info).data + self.assertEqual(rebuilt.hex(), hexstr) + + def test_tcp_padding_codes_have_cases(self) -> None: + """``EOOL`` and ``NOP`` are cases, not ``SKIP`` entries.""" + for name in TCP_PADDING: + with self.subTest(code=name): + self.assertNotIn(('tcp-option', name), self.options.SKIP) + + def test_tcp_padding_cases_round_trip_as_distinct_headers(self) -> None: + """Each TCP padding case is ``OK`` and carries its own option octets.""" + family = self.options.FAMILY_MAP['tcp-option'] + cases = {case.name: case for case in self.options.cases((family,))} + for name, tail in TCP_PADDING.items(): + with self.subTest(code=name): + self.assertIn(name, cases) + with warnings.catch_warnings(): + warnings.simplefilter('ignore') + outcome = self.options.roundtrip(cases[name]) + self.assertEqual(outcome.status, 'OK', outcome.detail) + self.assertEqual(outcome.octets[20:].hex(), tail) + + +if __name__ == '__main__': + unittest.main()