Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
- **Fixed** -- the string-keyed `get()` in `pcapkit.const.ftp.command` and `pcapkit.const.http.method` tested membership with the raw key but registered `key.upper()`, so the *first* lowercase or mixed-case token raised `TypeError: 'RETR' already in use` rather than resolving. Reachable from wire data for FTP: `pcapkit.protocols.application.ftp` compiles its request pattern with `re.I` and passes the match verbatim, and [RFC 959 Section 5.3](https://datatracker.ietf.org/doc/html/rfc959#section-5.3) makes FTP commands case-insensitive -- "Upper and lower case alphabetic characters are to be treated identically", listing `RETR Retr retr ReTr rETr` as the same command -- so `retr file.txt` was a valid request this library could not parse. Both `get()` and `_missing_` now look the key up under the same canonical upper-case name they register it under, so every casing resolves to the one member that already exists instead of colliding with it. Resolving rather than registering a second member matters beyond not crashing -- a duplicate `GET` would carry neither the `safe` nor the `idempotent` attribute of the real one (#582, #583).
- **Fixed** -- `httpv1`'s `_RE_METHOD` was unanchored and `re.match` anchors only at the start, so it prefix-matched, and the request-line reader then passed the whole `para1` to `Method.get` rather than the captured `method` group. Together those meant `b'Get'` matched on the single character `G`, satisfied the guard that decides a start-line is a request, and handed the entire mixed-case token to a lookup that raised on it. Fixing either half alone still gives a wrong answer -- normalising the lookup would parse `b'Get'` as `GET` off a one-character match, and passing the group would parse it as a method named `G`. The pattern is now anchored at both ends and the captured group is what is looked up, so a token that is not a method is a malformed request line rather than a mis-parsed one. Method tokens are case-sensitive per [RFC 9110 Section 9.1](https://datatracker.ietf.org/doc/html/rfc9110#section-9.1), so no `re.I` was added: `GET` parses, `Get` and `get` are rejected (#583).
- **Fixed** -- `_RE_STATUS` in the same reader carried the same unanchored prefix defect, found by auditing `_RE_METHOD`'s siblings, and it escaped as the wrong exception type. That pattern is only a guard -- the value is taken from `int(para2)` on the raw token -- so a prefix match let a malformed status past the guard and then out of `int()` uncaught, where `_read_http_header` documents `ProtocolError`. Measured: a status of `200x` raised `ValueError: invalid literal for int() with base 10: b'200x'`, and one of `2000` raised `ValueError: 2000 is not a valid StatusCode`; both are now `ProtocolError`. [RFC 9112 Section 4](https://datatracker.ietf.org/doc/html/rfc9112#section-4) gives `status-code = 3DIGIT`, exactly three, so the anchor is what the grammar already said -- the production lives in HTTP/1.1 because `status-code` is part of its `status-line`, while [RFC 9110 Section 15](https://datatracker.ietf.org/doc/html/rfc9110#section-15) covers the code semantics and the IANA registry rather than the syntax. `_RE_VERSION` was audited at the same time and is safe as it stands, because both of its call sites read the captured group rather than the raw token (#583).
Expand Down
32 changes: 32 additions & 0 deletions docs/source/changelog/1.5.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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).
* **Fixed** -- the string-keyed ``get()`` in ``pcapkit.const.ftp.command`` and
``pcapkit.const.http.method`` tested membership with the raw key but
registered ``key.upper()``, so the *first* lowercase or mixed-case token
Expand Down
14 changes: 13 additions & 1 deletion pcapkit/protocols/internet/hopopt.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)

Expand Down
14 changes: 13 additions & 1 deletion pcapkit/protocols/internet/ipv6_opts.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)

Expand Down
Loading
Loading