From 41682cb284f6ae063a633bbf718696dfbe8dcd2a Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 21 Sep 2026 21:32:01 -0400 Subject: [PATCH] fix(hopopt,ipv6-opts): size the ILNP nonce with a real ceiling, not a floored one (#601) The ILNP Nonce option builders sized the option with `math.ceil(nonce.bit_length() // 8)` at hopopt.py:1889 and ipv6_opts.py:1892. `//` floors, and `math.ceil` of an `int` is a no-op, so the ceiling was never taken. The nonce is packed by a `NumberField` whose width *is* that declared `len` (schema/internet/hopopt.py:722, ipv6_opts.py:727), so an under-declared length did not merely mis-state the option -- it silently truncated the nonce on the wire, with nothing raised. - Small nonces are the worst case, not boundary values. Any nonce below 256 was declared as *zero* octets and dropped from the packet: measured, `nonce=9` packed to `b'\x8b\x00'` and parsed back as `0`, and `nonce=256` and `nonce=65536` truncated to `0` as well. Only a bit length that is an exact multiple of eight gave the right answer. - Fixed to `max(1, math.ceil(nonce.bit_length() / 8))`, the form already used at five other sizing sites -- hip.py:3156, :3334, :4289, :4315 and mh.py:7947. - The one-octet floor is mh.py's convention and is load-bearing here because `nonce` defaults to `0`, whose bit length is `0`. Without it the default argument builds an ILNP Nonce option carrying no Nonce Value field at all, collapsing "the nonce is 0" into "there is no nonce" when RFC 6744 gives the option that field. Value round-trip is unaffected either way, so this is structural rather than a data fix, and it is called out as a deliberate addition rather than folded in silently. - The read path was never affected: it takes the width from the `len` octet on the wire rather than recomputing it. New cases in tests/protocols/internet/test_ipv6_extension_unit.py, one test per protocol, assert the declared length, the exact packed octets and the construct-pack-parse cycle over ten nonces. Reverting hopopt.py alone fails test_hopopt_... with 8 SUBFAILED subtests (`0 != 1`, `1 != 2`, `2 != 3`, `4 != 5`) while test_ipv6_opts_... passes, and reverting ipv6_opts.py alone mirrors it exactly -- so each site is pinned by its own test. The two byte-aligned controls, 255 and 0xFFFFFF, pass either way. The suite missed this because the only ILNP nonce it exercised was `0xFFFFFF` (examples/generators/options.py:537), bit length 24, precisely where floor division and the ceiling agree -- the same blind spot that hid the identical typo in numbers.py. There is no ILNP entry in EXPECTED_FAILURES (45 entries, confirmed by importing it), and none starts passing, so that file is untouched. The new table guards itself: it asserts at least six of its own values stay non-byte-aligned and one stays below 256. tests/protocols/internet/ 221 passed, 718 subtests, exit 0. Coverage of the two modules 99% statement and branch, the one uncovered line in each being a pre-existing gap in `_make_opt_pad` from #398. Statement coverage of the changed line was already 100% before this change, which is exactly why the defect survived; what the new tests add is assertions on the length it produces. `python util/changelog_md.py --check` exits 0. Fixes #601 --- CHANGELOG.md | 2 + docs/source/changelog/1.5.0.rst | 32 +++++++ pcapkit/protocols/internet/hopopt.py | 14 ++- pcapkit/protocols/internet/ipv6_opts.py | 14 ++- .../internet/test_ipv6_extension_unit.py | 95 +++++++++++++++++++ 5 files changed, 155 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f2f9e7f2..51e50a241 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,8 @@ This is the resolution of #548, which reported `TransType.L2TP` (115) as registe - **Added** -- `tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py`, covering all three MP_JOIN layouts through the *public* constructor, the construct-pack-parse cycle for each, the stale-flags case that rules out a zero-valued default, the statement order itself, and controls that the parse path and the flag-independent options are unaffected. The gap it closes is why 100% statement and branch coverage of the two changed modules coexisted with a completely broken public path: the pre-existing cases reach `_make_mptcp_join` by assigning a Python `set` to `_flags` on a bare `TCP.__new__(TCP)`, which executes every branch while bypassing both the ordering and the accumulator's type. The now-stale `tcp-mptcp/MP_JOIN` entry is deleted from `EXPECTED_FAILURES`, and the MP_JOIN exclusion in `test_tcp_mptcp_subtype_unit.py` is lifted (#587). - **Fixed** -- `NumberField.pre_process` sized a value with floor division dressed up as a ceiling. When a field is packed while its `length` is still the `-1` placeholder, the width is derived from the value, and it was derived with `math.ceil(value.bit_length() // 8)`. `math.ceil` of an integer is that integer, so the `//` had already floored the quotient and the outer call did nothing at all; the expression was plain floor division and the width came out one octet short. `256` was sized at one octet, `65536` at two, `16777216` at three, and both `int.to_bytes` and `struct.pack` refuse a value that does not fit the width they are given. **The reach is wider than "just past a boundary"**: floor division is wrong for every bit length that is not an exact multiple of eight, so `1` -- bit length 1, floored to *zero* octets -- failed too, and every value from 1 to 127 with it. Now written as the ceiling it was meant to be, matching the `math.ceil(n / 8)` idiom used elsewhere in the package. The repair is reached only by packing a field the caller never resolved, since a schema resolves every field before packing it and `__call__` installs a real width; that narrowness is why the defect survived the suite added for #591, whose five repair-path values -- `0xFF`, `0xFFFF`, `0xFFFFFFFF`, `0xFFFFFFFFFFFFFFFF` and `0x800001` -- have bit lengths of 8, 16, 32, 64 and 24 and so sat exactly where floor division and the ceiling agree. #591's own fix neither caused nor masked this, but it did change what the failure looks like: with `_need_process` now recomputed from the width in force, a mis-sized 1, 2 or 4 octets surfaces from `struct.pack` as `'B' format requires 0 <= number <= 255` where it used to surface from `int.to_bytes` as `OverflowError`, which is why the exception named in the report is no longer the one a mis-sized octet boundary raises. Two things on this path are deliberately left alone, both independent of the arithmetic: a signed field is sized without room for its sign bit, so an unresolved signed field still cannot pack `128`; and an unresolved field's bit mask is `-1`, which makes the masking and the sign remap above no-ops. The identical `math.ceil(x.bit_length() // 8)` expression also survives at the two ILNP nonce option builders in `hopopt.py` and `ipv6_opts.py`, which are a separate change (#599). - **Added** -- `tests/corekit/test_fields_numbers_width_repair.py`, covering each octet boundary in its own method rather than one parametrised sweep, since the defect is a pattern and a single case would pass against a fix that special-cased the reported width. Each boundary is asserted as a pair -- the value below it, which always packed, and the value above it, which did not -- so that a width shifted by one in the other direction fails too. Also swept over all eight boundaries, pinned as the `ceil(bit_length / 8)` invariant, checked for the smallest mis-sized value being `1`, round-tripped through pack and unpack, and given controls for the bit lengths that divide by eight and for the reachability of the repair at all. The suite deliberately asserts widths and octets rather than exception types, because the exception depends on whether the mis-sized width happens to have a native `struct` code (#599). +- **Fixed** -- the ILNP Nonce option builders in `HOPOPT` and `IPv6_Opts` sized the option with `math.ceil(nonce.bit_length() // 8)`, which is floor division dressed up as a ceiling: `//` floors, and `math.ceil` of an `int` is a no-op, so the ceiling was never actually taken. The nonce is packed by a `NumberField` whose width *is* that declared `len`, so an under-declared length did not merely mis-state the option -- it silently truncated the nonce on the wire, with nothing raised. Every nonce whose bit length was not an exact multiple of eight was affected, and **small values were the worst case rather than boundary values**: any nonce below 256 was declared as *zero* octets and dropped from the packet altogether, so `nonce=9` packed to `b'\x8b\x00'` and parsed back as `0`, while `nonce=256` and `nonce=65536` each truncated to `0` as well. Fixed to `max(1, math.ceil(nonce.bit_length() / 8))`, the form already used at five other sizing sites across `hip.py` and `mh.py`. The one-octet floor is the `mh.py` convention and is load-bearing here because `nonce` defaults to `0`, whose bit length is `0`: without it the default argument builds an ILNP Nonce option carrying no Nonce Value field at all, collapsing "the nonce is 0" into "there is no nonce" when [RFC 6744](https://datatracker.ietf.org/doc/html/rfc6744) gives the option that field. The read path was never affected, since it takes the width from the `len` octet on the wire rather than recomputing it (#601). +- **Added** -- ILNP nonce sizing coverage in `tests/protocols/internet/test_ipv6_extension_unit.py`, one test per protocol, asserting the declared length, the exact packed octets and the construct-pack-parse cycle over ten nonces. The reason the existing suite missed this is that the only ILNP nonce it ever exercised was `0xFFFFFF` -- bit length 24, an exact multiple of eight, precisely where floor division and the ceiling agree -- the same blind spot that hid the identical typo in `numbers.py` behind bit lengths 8, 16, 24, 32 and 64. Every new case bar two deliberate controls therefore has a bit length that is *not* a multiple of eight, several of them below 256. The table also guards itself: the test asserts that at least six of its own values stay non-byte-aligned and that one stays below 256, so rounding them off to convenient constants later cannot quietly disarm the regression (#601). Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) and `1.5.0b3` (2026-09-19), all published as prereleases and so resolved only by `pip install --pre`. `1.5.0b1` half-shipped: the tag, the GitHub release and the Conda deployments landed, but PyPI rejected the wheel because `twine check` found a Sphinx-only `:mod:` role in `README.rst`, which `pyproject.toml` declares as the dynamic long description. `1.5.0b2` is what reshipped it -- the release workflow is version-driven, so an existing version cannot republish -- and `1.5.0b3` followed the CI change that stops a TestPyPI outage from costing a release its wheels (#497, #498). diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 0a24545b8..cc97b982f 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -688,6 +688,38 @@ pull requests between #326 and #509. and for the reachability of the repair at all. The suite deliberately asserts widths and octets rather than exception types, because the exception depends on whether the mis-sized width happens to have a native ``struct`` code (#599). +* **Fixed** -- the ILNP Nonce option builders in ``HOPOPT`` and ``IPv6_Opts`` sized + the option with ``math.ceil(nonce.bit_length() // 8)``, which is floor division + dressed up as a ceiling: ``//`` floors, and ``math.ceil`` of an ``int`` is a + no-op, so the ceiling was never actually taken. The nonce is packed by a + ``NumberField`` whose width *is* that declared ``len``, so an under-declared + length did not merely mis-state the option -- it silently truncated the nonce on + the wire, with nothing raised. Every nonce whose bit length was not an exact + multiple of eight was affected, and **small values were the worst case rather + than boundary values**: any nonce below 256 was declared as *zero* octets and + dropped from the packet altogether, so ``nonce=9`` packed to ``b'\x8b\x00'`` and + parsed back as ``0``, while ``nonce=256`` and ``nonce=65536`` each truncated to + ``0`` as well. Fixed to ``max(1, math.ceil(nonce.bit_length() / 8))``, the form + already used at five other sizing sites across ``hip.py`` and ``mh.py``. The + one-octet floor is the ``mh.py`` convention and is load-bearing here because + ``nonce`` defaults to ``0``, whose bit length is ``0``: without it the default + argument builds an ILNP Nonce option carrying no Nonce Value field at all, + collapsing "the nonce is 0" into "there is no nonce" when :rfc:`6744` gives the + option that field. The read path was never affected, since it takes the width + from the ``len`` octet on the wire rather than recomputing it (#601). +* **Added** -- ILNP nonce sizing coverage in + ``tests/protocols/internet/test_ipv6_extension_unit.py``, one test per protocol, + asserting the declared length, the exact packed octets and the + construct-pack-parse cycle over ten nonces. The reason the existing suite missed + this is that the only ILNP nonce it ever exercised was ``0xFFFFFF`` -- bit length + 24, an exact multiple of eight, precisely where floor division and the ceiling + agree -- the same blind spot that hid the identical typo in ``numbers.py`` behind + bit lengths 8, 16, 24, 32 and 64. Every new case bar two deliberate controls + therefore has a bit length that is *not* a multiple of eight, several of them + below 256. The table also guards itself: the test asserts that at least six of + its own values stay non-byte-aligned and that one stays below 256, so rounding + them off to convenient constants later cannot quietly disarm the regression + (#601). Preceded by ``1.5.0a1`` (2026-09-15), ``1.5.0b1`` and ``1.5.0b2`` (both 2026-09-18) and ``1.5.0b3`` (2026-09-19), all published as prereleases and so diff --git a/pcapkit/protocols/internet/hopopt.py b/pcapkit/protocols/internet/hopopt.py index 19802bd46..b10118173 100644 --- a/pcapkit/protocols/internet/hopopt.py +++ b/pcapkit/protocols/internet/hopopt.py @@ -1884,9 +1884,21 @@ def _make_opt_ilnp(self, code: 'Enum_Option', opt: 'Optional[Data_ILNPOption]' = if opt is not None: nonce = opt.nonce + # NOTE: ``nonce`` is packed by a NumberField whose width is this very + # ``len`` (c.f. pcapkit.protocols.schema.internet.hopopt.ILNPOption), so + # the declared octet count has to be the ceiling of the bit length over + # eight -- ``bit_length() // 8`` floors instead, and wrapping a float-free + # floor division in ``math.ceil`` is a no-op, so every nonce whose bit + # length is not a multiple of eight used to be sized short and silently + # truncated on the wire (a nonce below 256 was declared as *zero* octets + # and vanished outright). ``bit_length()`` is 0 for 0 itself, which would + # likewise declare a zero-octet nonce -- collapsing "the nonce is 0" into + # "there is no nonce", when RFC 6744 gives the option a Nonce Value field + # -- so the width is floored at one octet, matching + # pcapkit.protocols.internet.mh.MH._make_opt_mn_id (c.f. #601). return Schema_ILNPOption( type=code, - len=math.ceil(nonce.bit_length() // 8), + len=max(1, math.ceil(nonce.bit_length() / 8)), nonce=nonce, ) diff --git a/pcapkit/protocols/internet/ipv6_opts.py b/pcapkit/protocols/internet/ipv6_opts.py index 73b20c077..af6894648 100644 --- a/pcapkit/protocols/internet/ipv6_opts.py +++ b/pcapkit/protocols/internet/ipv6_opts.py @@ -1887,9 +1887,21 @@ def _make_opt_ilnp(self, code: 'Enum_Option', opt: 'Optional[Data_ILNPOption]' = if opt is not None: nonce = opt.nonce + # NOTE: ``nonce`` is packed by a NumberField whose width is this very + # ``len`` (c.f. pcapkit.protocols.schema.internet.ipv6_opts.ILNPOption), + # so the declared octet count has to be the ceiling of the bit length over + # eight -- ``bit_length() // 8`` floors instead, and wrapping a float-free + # floor division in ``math.ceil`` is a no-op, so every nonce whose bit + # length is not a multiple of eight used to be sized short and silently + # truncated on the wire (a nonce below 256 was declared as *zero* octets + # and vanished outright). ``bit_length()`` is 0 for 0 itself, which would + # likewise declare a zero-octet nonce -- collapsing "the nonce is 0" into + # "there is no nonce", when RFC 6744 gives the option a Nonce Value field + # -- so the width is floored at one octet, matching + # pcapkit.protocols.internet.mh.MH._make_opt_mn_id (c.f. #601). return Schema_ILNPOption( type=code, - len=math.ceil(nonce.bit_length() // 8), + len=max(1, math.ceil(nonce.bit_length() / 8)), nonce=nonce, ) diff --git a/tests/protocols/internet/test_ipv6_extension_unit.py b/tests/protocols/internet/test_ipv6_extension_unit.py index 3d1fdd503..c9fa6a421 100644 --- a/tests/protocols/internet/test_ipv6_extension_unit.py +++ b/tests/protocols/internet/test_ipv6_extension_unit.py @@ -2334,6 +2334,101 @@ def test_ipv6_opts_constructed_header_round_trips(self) -> None: self._assert_constructed_header_round_trips(IPv6_Opts) + def _assert_ilnp_nonce_option_is_sized_by_the_ceiling(self, protocol_cls: type) -> None: + """The ILNP nonce option must declare enough octets to hold its nonce. + + ``nonce`` is packed by a + :class:`~pcapkit.corekit.fields.numbers.NumberField` whose width *is* + the option's own declared ``len`` (c.f. + :class:`~pcapkit.protocols.schema.internet.hopopt.ILNPOption`), so an + under-declared ``len`` does not merely mis-state a length -- it + silently truncates the nonce on the wire, with no exception raised. + + The builder used to size with ``math.ceil(nonce.bit_length() // 8)``. + ``//`` floors, and :func:`math.ceil` of an :class:`int` is a no-op, so + the ceiling was never actually taken: every nonce whose bit length was + not an exact multiple of eight got sized short, and any nonce below 256 + was declared as *zero* octets and dropped from the wire altogether + (``nonce=9`` packed to ``b'\\x8b\\x00'`` and parsed back as ``0``). + + ``0xFFFFFF`` -- bit length 24, the value the option-round-trip + generator happens to use -- is one of the few values floor division + gets right, which is why the round-trip suite never caught this. Every + case below bar the two marked controls therefore has a bit length that + is deliberately **not** a multiple of eight, and several are below 256 + (c.f. #601). + + """ + from pcapkit.const.ipv6.option import Option + from pcapkit.protocols.data.internet import hopopt as hopopt_data + from pcapkit.protocols.data.internet import ipv6_opts as opts_data + from pcapkit.protocols.schema.internet import hopopt as hopopt_schema + from pcapkit.protocols.schema.internet import ipv6_opts as opts_schema + + is_hopopt = protocol_cls.__name__ == 'HOPOPT' + data = hopopt_data if is_hopopt else opts_data + schema = hopopt_schema if is_hopopt else opts_schema + proto = object.__new__(protocol_cls) + + #: nonce value -> the octet width it needs to survive the wire + cases = [ + (0, 1), # bit length 0 -- floored to one octet + (1, 1), # bit length 1 + (9, 1), # bit length 4 + (42, 1), # bit length 6 + (127, 1), # bit length 7 + (255, 1), # bit length 8 -- control: floor was right here + (256, 2), # bit length 9 + (65536, 3), # bit length 17 + (0xFFFFFF, 3), # bit length 24 -- control: floor was right here + (0x1FFFFFFFF, 5), # bit length 33 + ] + + # Guard the table itself. These values only discriminate between the + # floor and the ceiling while their bit lengths are *not* multiples of + # eight, so rounding them off to convenient constants later would + # quietly disarm this regression -- which is exactly how the defect + # survived in the first place. + discriminating = [nonce for nonce, _ in cases if nonce.bit_length() % 8] + self.assertGreaterEqual(len(discriminating), 6) + self.assertTrue(any(nonce < 256 for nonce in discriminating)) + + for nonce, width in cases: + with self.subTest(nonce=nonce): + opt = proto._make_opt_ilnp(Option.ILNP_Nonce, nonce=nonce) + self.assertEqual(opt.len, width) + + # the nonce has to survive being packed, not merely be handed + # back from the schema object it was passed into + raw = bytes(opt) + self.assertEqual(len(raw), 2 + width) + self.assertEqual(raw[1], width) + self.assertEqual(raw[2:], nonce.to_bytes(width, 'big')) + + parsed = schema.ILNPOption.unpack(io.BytesIO(raw), len(raw), {}) + self.assertEqual(parsed.len, width) + self.assertEqual(parsed.nonce, nonce) + + # the ``opt=`` branch takes the nonce from the data model instead of the + # keyword, and has to size it the same way + from_data = proto._make_opt_ilnp( + Option.ILNP_Nonce, + data.ILNPOption(type=Option.ILNP_Nonce, length=4, nonce=127, + action=0, change=False), + ) + self.assertEqual(from_data.len, 1) + self.assertEqual(bytes(from_data)[2:], b'\x7f') + + def test_hopopt_ilnp_nonce_option_is_sized_by_the_ceiling(self) -> None: + from pcapkit.protocols.internet.hopopt import HOPOPT + + self._assert_ilnp_nonce_option_is_sized_by_the_ceiling(HOPOPT) + + def test_ipv6_opts_ilnp_nonce_option_is_sized_by_the_ceiling(self) -> None: + from pcapkit.protocols.internet.ipv6_opts import IPv6_Opts + + self._assert_ilnp_nonce_option_is_sized_by_the_ceiling(IPv6_Opts) + def test_option_registries_are_not_clobbered_by_a_nested_enum_registry(self) -> None: """Option type 0 must resolve to the padding option in every module.