From 5affdf31d0c03bebd74bb39adce99ccd4e5b8740 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Mon, 21 Sep 2026 20:12:26 -0400 Subject: [PATCH] fix(corekit): size the width repair with a real ceiling, not a floored one (#599) `NumberField.pre_process` derives a width from the value when a field is packed while its `length` is still the `-1` placeholder. It derived it with `math.ceil(value.bit_length() // 8)`, and `math.ceil` of an integer is that integer -- the `//` had already floored the quotient, so the outer call did nothing and the expression was plain floor division. The width came out one octet short, which both `int.to_bytes` and `struct.pack` refuse. - Now `math.ceil(value.bit_length() / 8)`, matching the `math.ceil(n / 8)` idiom already used in `ipv6_route.py`, `hip.py` and `pcapng.py`. Float division is exact here: `bit_length()` would have to exceed 2**53 to lose a bit. - The reach is wider than the report's "just past an octet boundary". Floor division is wrong for *every* bit length that is not a multiple of eight, so `1` -- bit length 1, floored to zero octets -- failed too, and every value from 1 to 127 with it. Measured: `256` sized at 1, `65536` at 2, `16777216` at 3, `1` at 0. - #591's 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 pre-#591 it surfaced from `int.to_bytes` as `OverflowError`. The `OverflowError` named in #599 is therefore only what a mis-sized 3 octets still raises. - Reachability, which #599 leaves open: the repair runs only on a field the caller never resolved. `-1` is assigned in exactly one place, where a callable `length` is swapped for a placeholder, and `__call__` installs the real width; a field omitting `length` is refused with `IntError` instead. `Schema.pack` resolves every field before packing it, so this is reached through the field-level API, on `NumberField` or `EnumField` -- the subclasses that fix `__template__` keep `_need_process` false and never enter the branch. Deliberately not fixed, both independent of the arithmetic and neither made worse: an unresolved signed field is sized without room for its sign bit, so it still cannot pack `128`; and an unresolved field's `_bit_mask` is `-1`, which makes the masking and the sign remap at the top of `pre_process` no-ops. The identical `math.ceil(x.bit_length() // 8)` expression also survives at the ILNP nonce builders in `hopopt.py:1889` and `ipv6_opts.py:1892`, outside this change's files. New tests in tests/corekit/test_fields_numbers_width_repair.py proven to fail without the fix: 31 failures across 13 of 15 tests before, all 15 passing after. Each boundary gets its own method rather than one sweep, since the defect is a pattern and a single case would pass against a width-specific fix, and each is asserted as a pair so a width shifted the other way fails too. The 2 that pass either way are the coverage/contract cases -- the `IntError` refusal and the explicit `bit_length` branch. They take `numbers.py` from 93% to 100% statement and branch coverage. tests/corekit/ 149 passed, 239 subtests; tests/protocols/test_option_roundtrip_unit.py 6 passed, 358 subtests, with no `EXPECTED_FAILURES` entry flipped. No new mypy finding. Fixes #599 --- CHANGELOG.md | 2 + docs/source/changelog/1.5.0.rst | 44 ++ pcapkit/corekit/fields/numbers.py | 13 +- .../test_fields_numbers_width_repair.py | 397 ++++++++++++++++++ 4 files changed, 455 insertions(+), 1 deletion(-) create mode 100644 tests/corekit/test_fields_numbers_width_repair.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 69299f3ab..8f2f9e7f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,8 @@ This is the resolution of #548, which reported `TransType.L2TP` (115) as registe - **Fixed** -- a `NumberField` whose `length` was a callable could not pack or parse at any width `struct` has a native integer code for. `length` is a placeholder of `-1` until the callable is resolved, `-1` has no native code, and the template builder raised `_need_process` for it and never put it back -- so the flag was a latch. Resolving the real width rebuilt the template and left the latch set, and `pre_process` then handed `bytes` to a template that had become `>Q`, raising `struct.error: required argument is not an integer`; parsing failed in the mirror direction, calling `int.from_bytes` on the integer that `struct.unpack` had already produced. The flag is now recomputed from the width actually in force rather than only ever raised, which is also what keeps a callable resolving to a width with no native code -- 3 octets, say -- byte-packed as it must be. **All four native widths were affected, not only the 8 that was reported**: the latch has nothing to do with the width it latches into, so 1, 2 and 4 failed identically, on `NumberField` and on `EnumField`, both of which leave `__template__` unset. This is what made every extended 8-octet MPTCP DSS form unbuildable, since those widths are chosen at runtime from the DSS flags and so must come from a callable; #585 worked around it in the TCP schema alone, leaving every other caller exposed (#591). - **Fixed** -- building any `MP_JOIN` option raised `AttributeError: 'TCP' object has no attribute '_flags'`. `TCP.make` constructed the options *before* it assigned the `self._flags` that the option makers read, and `_make_mptcp_join` branches on that attribute to choose between the three layouts [RFC 8684](https://datatracker.ietf.org/doc/html/rfc8684) section 3.2 gives for MP_JOIN -- figure 5 for SYN at 12 octets, figure 6 for SYN/ACK at 16, figure 7 for ACK at 24. The parse path was never affected: `read` assigns the flags before it parses the options, so the identical branches in `_read_mptcp_join` always had them. Fixed by hoisting the flag resolution above the `_make_tcp_options` call, leaving only the header's data-offset computation -- which genuinely needs the options' total length -- after it. **Not** fixed by giving `_flags` a zero default, which would have been worse: `Protocol.pack` is public and calls `make`, so an instance that had already parsed a segment always had the attribute set, and it silently built the option for the segment it had *read* rather than the one it was asked to write. Measured pre-fix, a parsed MP_JOIN-SYN instance asked to pack an MP_JOIN-ACK segment emitted an ACK header carrying figure 5's 12-octet SYN option, with the caller's 20-octet HMAC -- the whole authentication payload of figure 7's form -- replaced by an all-zero phantom token and nonce, and nothing raised. The crash was the benign symptom; a default value fixes only that and leaves the silent corruption. Hoisting also made one branch reachable for the first time, an MP_JOIN asked for on a segment with neither SYN nor ACK set: the accumulator was seeded with `cast('Enum_Flags', 0)`, and `typing.cast` being a runtime no-op, `self._flags` stayed a plain `int` on which the first membership test raised `TypeError` rather than the `ProtocolError` the method documents. It is now seeded with `Enum_Flags(0)`, a real flagless member that compares equal to `0` and ORs identically. The read path's own seed is deliberately unchanged -- a flagless MP_JOIN is rejected by `mptcp_data_selector` before `_read_mptcp_join` runs, so it cannot reach those branches (#587). - **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). 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 d8ddf44e6..0a24545b8 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -644,6 +644,50 @@ pull requests between #326 and #509. 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). 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/corekit/fields/numbers.py b/pcapkit/corekit/fields/numbers.py index 2fff3b0a3..cb44606b8 100644 --- a/pcapkit/corekit/fields/numbers.py +++ b/pcapkit/corekit/fields/numbers.py @@ -223,13 +223,24 @@ def pre_process(self, value: 'int', packet: 'dict[str, Any]') -> 'int | bytes': second is how the template and the value being returned came to disagree in the first place. C.f. #591. + That width is a **ceiling** of the bit length over eight, and it is + written as one. It used to read + ``math.ceil(value.bit_length() // 8)``, which is not a ceiling at + all: :func:`math.ceil` of an :obj:`int` is that :obj:`int`, so the + ``//`` had already floored the quotient and the outer call did + nothing. Every value whose bit length is not an exact multiple of + eight was therefore sized one octet short -- ``256`` at one octet, + ``65536`` at two, and ``1`` itself at *zero* -- which + :meth:`int.to_bytes` and :func:`struct.pack` both refuse. See GitHub + issue #599. + """ value = value & self._bit_mask if self._signed and value > self._bit_mask >> 1: value -= self._bit_mask + 1 if self._need_process and self._length < 0: - self._length = math.ceil(value.bit_length() // 8) + self._length = math.ceil(value.bit_length() / 8) endian = '>' if self._byteorder == 'big' else '<' struct_fmt = self.build_template(self._length, self._signed) diff --git a/tests/corekit/test_fields_numbers_width_repair.py b/tests/corekit/test_fields_numbers_width_repair.py new file mode 100644 index 000000000..c81e64c91 --- /dev/null +++ b/tests/corekit/test_fields_numbers_width_repair.py @@ -0,0 +1,397 @@ +"""``pre_process``'s width repair sized values with floor division. + +GitHub issue #599. When a :class:`~pcapkit.corekit.fields.numbers.NumberField` +is packed while ``length`` is still the ``-1`` placeholder, ``pre_process`` +derives a width from the value itself. It derived it with:: + + self._length = math.ceil(value.bit_length() // 8) + +:func:`math.ceil` on an :obj:`int` is a no-op -- ``//`` has already floored it -- +so the expression was plain floor division and the width came out one octet short +for every value whose bit length is *not* an exact multiple of eight. The +intended expression is ``math.ceil(value.bit_length() / 8)``. + +Two things about this suite are deliberate. + +**The boundaries are tested one method each, not one parametrised sweep.** The +defect is a pattern rather than an instance -- it fails at *every* octet boundary +-- so a single case would pass against a fix that special-cased the reported +width. Standalone methods also keep each failure individually visible: the +pytest in use has no ``pytest-subtests``, so a failing :meth:`~unittest.TestCase +.subTest` still leaves its parent reported as passed. + +**Each boundary is asserted as a pair.** The value *below* the boundary already +packed correctly before the fix and must go on doing so; only the value *above* +it was broken. Asserting the pair is what distinguishes a fix from a width that +has merely been shifted by one in the other direction. + +The failure was not a single exception type, and this suite deliberately does not +assert one. Since #591's fix, ``_need_process`` is recomputed from the width in +force, so a mis-sized width that lands on 1, 2 or 4 octets now comes back from +:func:`struct.pack` as ``struct.error: 'B' format requires 0 <= number <= 255`` +while a mis-sized 3 octets still comes back from :meth:`int.to_bytes` as +``OverflowError: int too big to convert``. What these tests pin instead is the +width and the octets -- the thing that is actually wrong. +""" + +from __future__ import annotations + +import importlib.util +import math +import unittest + +from tests._support import purge_modules + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + +#: The reported table, as ``(value, octets the value needs)``. The first of each +#: pair sits on an octet boundary and packed correctly before the fix; the second +#: is one past it and did not. +BOUNDARY_PAIRS = ( + (255, 1, 256, 2), + (65535, 2, 65536, 3), + (16777215, 3, 16777216, 4), +) + + +def _unresolved_field(**kwargs: 'object'): + """A field that reaches the repair, built the only way that does. + + ``_length`` is ``-1`` only while a callable ``length`` is unresolved, and the + repair additionally needs ``_need_process``, which ``__init__`` raises for the + ``-1`` placeholder only when no ``__template__`` fixes the format. So a bare + :class:`~pcapkit.corekit.fields.numbers.NumberField` given a callable and + **not** called is what gets there. Calling it resolves the width and the + repair never runs -- see + :meth:`WidthRepairReachabilityTests.test_resolving_the_field_bypasses_the_repair_entirely`. + + """ + from pcapkit.corekit.fields.numbers import NumberField + + return NumberField(length=lambda pkt: 8, **kwargs) # type: ignore[arg-type] + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class WidthRepairBoundaryTests(unittest.TestCase): + """One method per octet boundary, each asserting the pair across it.""" + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_the_one_octet_boundary_needs_two_octets_past_255(self) -> None: + """``255`` fits one octet; ``256`` needs two and was given one. + + ``255`` has a bit length of 8 and ``8 // 8`` is 1, so it was sized + correctly by luck. ``256`` has a bit length of 9 and ``9 // 8`` is also 1, + which is one octet short of what it needs. + + """ + below = _unresolved_field() + self.assertEqual(below.pack(255, dict()), b'\xff') + self.assertEqual(below._length, 1) + + above = _unresolved_field() + self.assertEqual(above.pack(256, dict()), b'\x01\x00') + self.assertEqual(above._length, 2, + '256 needs two octets; floor division sized it at one') + + def test_the_two_octet_boundary_needs_three_octets_past_65535(self) -> None: + """``65535`` fits two octets; ``65536`` needs three and was given two. + + Bit lengths 16 and 17, floored to 2 and 2. The second is wrong. + + """ + below = _unresolved_field() + self.assertEqual(below.pack(65535, dict()), b'\xff\xff') + self.assertEqual(below._length, 2) + + above = _unresolved_field() + self.assertEqual(above.pack(65536, dict()), b'\x01\x00\x00') + self.assertEqual(above._length, 3, + '65536 needs three octets; floor division sized it at two') + + def test_the_three_octet_boundary_needs_four_octets_past_16777215(self) -> None: + """``16777215`` fits three octets; ``16777216`` needs four and was given three. + + Bit lengths 24 and 25, floored to 3 and 3. This crossing is the one that + still surfaced as ``OverflowError`` from :meth:`int.to_bytes` after #591, + because 3 octets has no native :func:`struct` integer code and so is + genuinely byte-packed. + + """ + below = _unresolved_field() + self.assertEqual(below.pack(16777215, dict()), b'\xff\xff\xff') + self.assertEqual(below._length, 3) + + above = _unresolved_field() + self.assertEqual(above.pack(16777216, dict()), b'\x01\x00\x00\x00') + self.assertEqual(above._length, 4, + '16777216 needs four octets; floor division sized it at three') + + def test_every_octet_boundary_up_to_eight_is_sized_correctly(self) -> None: + """The pattern, rather than the three instances above. + + For each ``n``, ``(1 << 8n) - 1`` is the largest value fitting ``n`` + octets and ``1 << 8n`` is the smallest needing ``n + 1``. Floor division + sized both at ``n``. Swept to eight octets so the fix cannot be a table of + the reported cases. + + """ + for n in range(1, 9): + largest_fitting, smallest_past = (1 << (8 * n)) - 1, 1 << (8 * n) + + with self.subTest(octets=n, value=largest_fitting): + field = _unresolved_field() + self.assertEqual(field.pack(largest_fitting, dict()), + largest_fitting.to_bytes(n, 'big')) + self.assertEqual(field._length, n) + + with self.subTest(octets=n + 1, value=smallest_past): + field = _unresolved_field() + self.assertEqual(field.pack(smallest_past, dict()), + smallest_past.to_bytes(n + 1, 'big')) + self.assertEqual(field._length, n + 1) + + def test_the_smallest_mis_sized_value_is_one_not_two_hundred_and_fifty_six(self) -> None: + """The issue understates the reach, and this is where it shows. + + #599 frames the defect as hitting values "just past an octet boundary", + which reads as though small values were safe. They were not: floor + division is wrong for *every* bit length that is not a multiple of eight, + so ``1`` -- bit length 1, floored to **zero** octets -- was mis-sized + too, and every value from 1 to 127 with it. Measured on the unfixed tree, + ``pack(1, {})`` raised ``OverflowError: int too big to convert`` against a + zero-octet width. + + """ + for value in (1, 2, 42, 127): + with self.subTest(value=value): + field = _unresolved_field() + self.assertEqual(field.pack(value, dict()), bytes([value])) + self.assertEqual(field._length, 1, + f'{value} needs one octet; floor division sized it at zero') + + def test_the_repaired_width_is_the_ceiling_of_the_bit_length_over_eight(self) -> None: + """The invariant the fix establishes, stated as such. + + Every width the repair picks is ``ceil(bit_length / 8)``. Holding it over + a spread that straddles several boundaries is what makes the property the + subject of the test rather than the sample. + + """ + values = (1, 2, 127, 128, 255, 256, 257, 4095, 65535, 65536, + 16777215, 16777216, 1 << 40, (1 << 64) - 1, 1 << 64) + + for value in values: + expected = math.ceil(value.bit_length() / 8) + with self.subTest(value=value, expected_octets=expected): + field = _unresolved_field() + + self.assertEqual(field.pack(value, dict()), value.to_bytes(expected, 'big')) + self.assertEqual(field._length, expected) + + def test_a_bit_length_that_is_a_multiple_of_eight_is_untouched_by_the_fix(self) -> None: + """The control, and the reason this survived #591's test suite. + + Floor division and the ceiling agree exactly when the bit length divides + by eight. Every value #591's own repair-path test used -- ``0xFF``, + ``0xFFFF``, ``0xFFFFFFFF``, ``0xFFFFFFFFFFFFFFFF``, ``0x800001`` -- has a + bit length of 8, 16, 32, 64 or 24, so all five sat precisely on the + agreement and the defect stayed invisible. They must keep packing + identically. + + """ + for value in (0xFF, 0xFFFF, 0xFFFFFFFF, 0xFFFFFFFFFFFFFFFF, 0x800001): + octets = value.bit_length() // 8 + with self.subTest(value=hex(value)): + field = _unresolved_field() + + self.assertEqual(field.pack(value, dict()), value.to_bytes(octets, 'big')) + self.assertEqual(field._length, octets) + + def test_a_repaired_field_can_read_back_what_it_wrote(self) -> None: + """A width that is one octet short truncates as well as raising. + + The repair rewrites ``_template`` too, so it decides what + :meth:`~pcapkit.corekit.fields.field.FieldBase.unpack` will read. Packing + and unpacking through the same repaired field is what shows the width is + right rather than merely large enough not to raise. + + """ + for value in (256, 65536, 16777216, 1 << 40): + with self.subTest(value=value): + field = _unresolved_field() + + buffer = field.pack(value, dict()) + self.assertEqual(len(buffer), math.ceil(value.bit_length() / 8)) + self.assertEqual(field.unpack(buffer, dict()), value) + + def test_math_ceil_of_a_floor_division_is_the_floor_division(self) -> None: + """The root cause, pinned directly rather than only described. + + The old expression *looked* like a ceiling and was not one. Stating that + as an assertion keeps the next reader of ``pre_process`` from + reintroducing it: the two expressions differ for seven of every eight bit + lengths, and coincide only on the eighth. + + """ + differing = [n for n in range(1, 65) + if math.ceil(n // 8) != math.ceil(n / 8)] + + self.assertEqual(differing, [n for n in range(1, 65) if n % 8], + 'math.ceil(n // 8) is n // 8, so it differs from the real ' + 'ceiling for every n that is not a multiple of 8') + self.assertEqual(math.ceil(9 // 8), 1) + self.assertEqual(math.ceil(9 / 8), 2) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class WidthRepairReachabilityTests(unittest.TestCase): + """How a caller gets to the repair at all, which bounds what this fix touches.""" + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_resolving_the_field_bypasses_the_repair_entirely(self) -> None: + """The repair is a net under the *unresolved* field, nothing more. + + ``__call__`` assigns the callable's width to ``_length``, so a resolved + field never satisfies ``_length < 0`` and packs through the ordinary path + at the width it was told -- zero-padded, not shrunk to fit the value. The + schema packer resolves every field before packing it, which is why this + path is reached through the field-level API rather than through a + protocol. + + """ + resolved = _unresolved_field()(dict()) + + self.assertEqual(resolved._length, 8) + self.assertIs(resolved._need_process, False) + self.assertEqual(resolved.pack(256, dict()), (256).to_bytes(8, 'big')) + + def test_an_unresolved_field_is_what_satisfies_the_guard(self) -> None: + """Both halves of ``if self._need_process and self._length < 0``. + + Stated explicitly so that a later change which stops the placeholder + reaching here shows up as this test failing rather than as the boundary + tests passing vacuously. + + """ + field = _unresolved_field() + + self.assertEqual(field._length, -1) + self.assertIs(field._need_process, True) + + def test_an_enum_field_is_affected_identically(self) -> None: + """:class:`~pcapkit.corekit.fields.numbers.EnumField` leaves ``__template__`` unset. + + Which is the condition for ``__init__`` to call ``build_template`` on the + placeholder and so raise ``_need_process`` -- the same reason #591 hit + ``EnumField`` too. The ``Int``/``UInt`` subclasses fix ``__template__``, + keep ``_need_process`` false, and never reach the repair. + + """ + from pcapkit.corekit.fields.numbers import EnumField, UInt32Field + + affected = EnumField(length=lambda pkt: 8) + self.assertIs(affected._need_process, True) + self.assertEqual(affected.pack(256, dict()), b'\x01\x00') + self.assertEqual(affected._length, 2) + + unaffected = UInt32Field(length=lambda pkt: 8) + self.assertIs(unaffected._need_process, False) + self.assertEqual(unaffected._length, -1) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class WidthRepairSurroundingContractTests(unittest.TestCase): + """The rest of the field's contract that the repair leans on. + + Each case here pins a statement or branch of ``numbers.py`` that the repair + depends on and that nothing reached before, which is why they live with the + #599 suite rather than apart from it. + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_a_field_with_no_length_at_all_is_refused_outright(self) -> None: + """Ruling out the other way the placeholder might have been thought to arise. + + ``-1`` is assigned in exactly one place -- where a non-integer ``length`` + is swapped for a placeholder and stashed as a callback -- so a field + *omitting* ``length`` never becomes an unresolved one. It is refused at + construction with the library's own ``IntError``, which is what bounds the + repair's reachability to a callable that was never resolved. + + """ + from pcapkit.corekit.fields.numbers import NumberField + from pcapkit.utilities.exceptions import IntError + + with self.assertRaises(IntError): + NumberField() + + # ... whereas a subclass fixing ``__length__`` supplies its own default. + from pcapkit.corekit.fields.numbers import UInt16Field + + self.assertEqual(UInt16Field()._length, 2) + + def test_an_explicit_bit_length_survives_resolution(self) -> None: + """The branch that decides whether the mask is derived or kept. + + ``__call__`` derives ``_bit_length`` from the octet width only when it was + not given one. A field told its bit length keeps it, mask included, which + is the case that makes the masking in ``pre_process`` meaningful at all -- + an unresolved field's mask is ``-1`` and masks nothing. + + """ + from pcapkit.corekit.fields.numbers import NumberField + + narrow = NumberField(length=2, bit_length=12) + self.assertEqual(narrow.bit_length, 12) + self.assertEqual(narrow._bit_mask, 0xFFF) + + resolved = narrow(dict()) + self.assertEqual(resolved.bit_length, 12, 'a given bit length must not be overwritten') + self.assertEqual(resolved._bit_mask, 0xFFF) + self.assertEqual(resolved.pack(0xABCD, dict()), b'\x0b\xcd', + 'the value is truncated to 12 bits, not to 16') + + derived = NumberField(length=2)(dict()) + self.assertEqual(derived.bit_length, 16) + self.assertEqual(derived._bit_mask, 0xFFFF) + + def test_a_repaired_enum_field_reads_back_a_namespaced_member(self) -> None: + """The repair decides the template ``unpack`` then reads through. + + So an :class:`~pcapkit.corekit.fields.numbers.EnumField` sized by the + repair has to round-trip into its namespace, and into the synthetic + ```` member when it has none. Both directions of that are + exercised at a width -- two octets for ``256`` -- that the repair only + produces once the ceiling is a ceiling. + + """ + import enum + + from pcapkit.corekit.fields.numbers import EnumField + + class Width(enum.IntEnum): + TWO_OCTETS = 256 + + namespaced = EnumField(length=lambda pkt: 8, namespace=Width) + buffer = namespaced.pack(256, dict()) + self.assertEqual(buffer, b'\x01\x00') + self.assertIs(namespaced.unpack(buffer, dict()), Width.TWO_OCTETS) + + anonymous = EnumField(length=lambda pkt: 8) + buffer = anonymous.pack(256, dict()) + self.assertEqual(buffer, b'\x01\x00') + + unknown = anonymous.unpack(buffer, dict()) + self.assertEqual(int(unknown), 256) + self.assertEqual(unknown.name, '') + + +if __name__ == '__main__': + unittest.main()