Skip to content

fix(mh): stop the four RFC-inline helper enums from minting on unassigned bytes - #889

Merged
JarryShaw merged 1 commit into
mainfrom
fix-mh-880-immutable-helper-enums
Sep 29, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix-mh-880-immutable-helper-enums

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

This is the mh.py half of #880 only. ngap.py's ProcedureCode/ProtocolIE are the other two of the six sites #880 names, ruled to be sourced from pycrate through a vendor crawler instead (separate work), so #880 stays open.

Per the owner's ruling on #877, FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode, LocalizedRoutingStatus and LMAAddressCode no longer mint an Unassigned_N member for a value their RFC leaves unassigned. _missing_ raises a new EnumValueError (pcapkit.utilities.exceptions, pairing with EnumError the way FieldValueError pairs with FieldError) instead -- no existing exception there fit "no member for this value in a closed enum".

get()'s default=-1 mint path -- the shared-sentinel aliasing defect #880 measured -- is removed for the two enums that keep get(); it now raises the same way _missing_ does. LocalizedRoutingStatus.get/LMAAddressCode.get are deleted outright, verified zero callers repo-wide, tests included.

The consequence of the raise differs by wrapping protocol, and this matters for anyone reading the diff. Over IPv4, which dispatches Mobility Header through the base, non-chain-walking _import_next_layer, the @beholder decorator there degrades just the one MH message to Raw -- not a capture-level failure. Over IPv6, Mobility Header is also one of IPv6's own chained extension headers, so the fallback lands inside IPv6's own extension-header walk, which then does proto = info.next (ipv6.py:338) on it; Raw's info has no .next, so that second, unrelated AttributeError is what a further-out @beholder actually catches -- degrading the whole IPv6 packet, header fields included. That walk defect is pre-existing (a truncated or malformed extension header hits the identical AttributeError on unmodified main), but this change adds a far likelier route to it: a well-formed packet naming merely an unassigned byte, where before only a malformed one got there. Sibling frames are unaffected either way.

Two tests pinning the old minting/round-trip behaviour are re-pointed; a new test proves the aliasing defect is gone and the 0 <= value <= 255 range invariant holds, demonstrates the IPv4 degrade path, and pins the IPv6 whole-packet-to-Raw behaviour as it stands today -- deliberately, so whoever fixes ipv6.py:338 sees that assertion and knows to revisit it. Each new/changed assertion is shown to fail without this fix.

tests/protocols/internet/test_mh_unit.py: 51 run, 0 failures, 0 errors.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) bug Issues reporting a defect (set by the bug report template; a default, not an assessment) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 28, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Qualifying something I told you when recommending the raise — the safety argument is verified on a path real captures do not take.

I said raising "costs one MH message's structured parse, not the capture", because the lookups sit under _import_next_layer, which is @beholder-decorated at pcapkit/protocols/protocol.py:1827 and re-parses the layer as Raw. #889 tests exactly that and it holds — for MH wrapped in IPv4 (tests/protocols/internet/test_mh_unit.py ~:1669, wrap_in_ipv4).

But Mobility Header is an IPv6 extension header; the IPv4 wrapping is a synthetic harness. And IPv6 does not merely hand the layer back — it keeps walking its own chain:

ipv6.py:329   info = next_.info          # next_ is the _import_next_layer result
ipv6.py:338   proto = info.next

Measured: Raw's info has no .next attribute (hasattr(...) → False). So a Raw fallback structurally cannot satisfy that walk, and #889's own test comment says as much — it chose IPv4 because of it.

What I have not established: I tried to reproduce the failure on unfixed main with a hand-built IPv6 header (next-header 135, 2-byte truncated MH) and it parsed fine — my packet never reached the extension-header walk. So the AttributeError: 'Raw' object has no attribute 'next' the author reports is plausible and structurally supported, but not reproduced by me, and I am not going to relay it as fact.

The cross-review's first task is to settle it on the PR head and to test whether main fails identically for an unrelated MH parse error — which decides whether this is pre-existing (the PR merely reaches it by a new route) or introduced here.

This does not change your ruling — the four enums should still raise, and the get(default=-1) aliasing defect is real either way. What it changes is the cost line. If the IPv6 walk really does break on the Raw fallback, then for the path real captures take a raise is not contained to one message, and that is a pre-existing gap in the degrade path worth its own issue rather than a reason to keep minting. I will report the measurement either way before #889 is handed over.

…gned bytes

Per the owner's ruling on #877, FastBindingAcknowledgmentStatus,
IPv6AddressPrefixCode, LocalizedRoutingStatus and LMAAddressCode stay
immutable: none of their RFCs keep an IANA registry, so an unassigned
byte is closed rather than pending assignment.

- `_missing_` raises a new `EnumValueError` (`pcapkit.utilities.exceptions`,
  paired with `EnumError` the way `FieldValueError` pairs with `FieldError`)
  instead of minting an `Unassigned_N` member. Over IPv4 that costs one MH
  message, via the `@beholder`-decorated `_import_next_layer`. Over IPv6,
  where Mobility Header chains as one of IPv6's own extension headers, it
  costs the whole IPv6 packet: the walk's `proto = info.next`
  (`ipv6.py:338`) has nothing to read off the `Raw` fallback, and that
  second, unrelated `AttributeError` is what a further-out `@beholder`
  actually catches. That walk defect is pre-existing -- a truncated or
  malformed extension header hits it on unmodified `main` too -- but a
  well-formed packet naming merely an unassigned byte is a far likelier way
  to reach it. Neither case is a capture-level failure; sibling frames are
  unaffected either way.
- `get()`'s `default=-1` mint path is removed for the two enums that keep
  `get()`. It now raises the same `EnumValueError` for an unknown key,
  closing the defect where a second unknown name aliased the first at the
  shared sentinel `-1` and bypassed `_missing_`'s own range guard.
- `LocalizedRoutingStatus.get`/`LMAAddressCode.get` are deleted outright:
  zero callers repo-wide, tests included.
- Two existing tests pinning the old minting/aliasing behaviour are
  re-pointed to the new contract. A new test proves the aliasing defect,
  the range invariant and the IPv4 degrade path, each shown to fail
  without this fix; it also pins the IPv6 whole-packet defect as it
  stands today, so whoever fixes ipv6.py:338 sees this test and knows to
  revisit it.

tests/protocols/internet/test_mh_unit.py: 51 run, 0 failures, 0 errors.

Refs #880 (mh.py half only; ngap.py's ProcedureCode/ProtocolIE are
separate, per-key work tracked there)
@JarryShaw
JarryShaw force-pushed the fix-mh-880-immutable-helper-enums branch from 93bb2e0 to a47d231 Compare September 29, 2026 00:03
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO on a47d23186 — cross-review was opus (author sonnet); the NEEDS CHANGES it raised was on the disclosure, and that is now fixed and verified.

The code was never in question. What was wrong is what the code said, in four shipping docstrings. Verified on the amended head:

"degrades just that one message"  ->  0 occurrences
"whole IPv6 packet"              ->  4 occurrences  (one per class)
EnumValueError docstring         ->  """No member of a closed enumeration carries this value."""  (one line, matching all 33 siblings)
run=51 failures=0 errors=0

Each of the four now cites ipv6.py:338 (proto = info.next on a Raw whose info carries no .next) as the mechanism, says the whole IPv6 packet degrades rather than the one MH message, and records that the walk defect predates this change — a well-formed packet naming an unassigned byte is simply a likelier way to reach it than a malformed one.

Three things the author did beyond what I asked, all right calls:

  • Found a third site carrying the same false claim — test_mh_rfc5568_local_enums_cover_unregistered_value_sets's docstring at test_mh_unit.py:1444, not one of the six I listed. Now "one message's worth of damage over IPv4, the whole packet's over IPv6".
  • Split the degrade test into two named subTests rather than relabelling one: 'over IPv4, an unassigned wire byte degrades just that one MH layer' (:1663) and 'over IPv6, the same raise costs the whole packet (pre-existing defect)' (:1701), the latter asserting str(eth.protochain) == 'Ethernet:Internet_Protocol_version_6'.
  • Reproduced my measurement independently before editing, rather than taking the table on trust.

That IPv6 subTest is the part worth keeping: it pins today's defective behaviour as an explicit assertion, so whoever fixes #891 (the ipv6.py chain-walk, filed separately) breaks this test and is forced to look here rather than discovering the change by accident.

The earlier verdict's other findings stand confirmed and needed no rework: the four enums are correctly closed with extend_enum gone from all of them, EnumValueError is placed as the ValueError counterpart to EnumError per the FieldError/FieldValueError pairing and is explicitly sanctioned by #877's item-3 ruling, get()'s parameterless contract matches Criticality.get's precedent with zero affected callers, and the new tests genuinely fail without the fix — 7 failures + 1 error, reproduced exactly, now 8 with the IPv6 case added.

Still references #880 rather than closing it: ProcedureCode and ProtocolIE in ngap.py remain, per the ruling that they be sourced from pycrate through a crawler. CI is mid-run on the new head; I will confirm green before handing over. Unpublished and unmerged, yours to merge.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Sep 29, 2026
@JarryShaw
JarryShaw merged commit fde4cb2 into main Sep 29, 2026
63 checks passed
@JarryShaw
JarryShaw deleted the fix-mh-880-immutable-helper-enums branch September 29, 2026 00:57
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Sep 29, 2026
JarryShaw added a commit that referenced this pull request Sep 29, 2026
…red -1 mint (#880)

Completes #880's ngap.py half (the mh.py half landed in #889). Per the
owner's ruling, ProcedureCode/ProtocolIE are genuinely open per 3GPP TS
38.413, so they want a per-key unique value rather than the shared -1
sentinel #880 measured aliasing on -- sourced from pycrate's own compiled
NGAP specification through a vendor crawler, not hand-maintained or
minted on demand.

- Add pcapkit/vendor/ngap/{procedure_code,protocol_ie}.py: crawlers whose
  _request() imports pycrate_asn1dir.NGAP (no network fetch -- pycrate is
  already an installed optional dependency) and reads NGAP_Constants'
  (name, value) pairs, filtered by each value's own typeref.
- Add pcapkit/const/ngap/{procedure_code,protocol_ie}.py, generated:
  EnumRegistry + IntEnum, matching the house pattern. get() no longer
  mints on an unresolvable string key (raises, like every other converted
  registry); _missing_ answers an in-range value it has not seen with a
  throwaway, non-registering member instead of a shared-value extend_enum
  mint, which is what stops two different unknown keys from aliasing.
- pcapkit/protocols/application/ngap.py now re-exports both from
  pcapkit.const.ngap under their historical names, dropping ~600 lines of
  hand-rolled IntEnum + extend_enum logic.
- Extend four tests/const/ generic sweeps' pinned counts (+2 registries);
  tests/const/test_const_registry_protocol.py's own census is untouched
  (out of scope) and now needs 122->124, 105->107.

New tests in tests/vendor/ and tests/protocols/application/, each shown
to fail without this fix. `pytest tests/const/ tests/vendor/ (excluding
the reachability test) tests/protocols/application/test_ngap_unit.py`:
384 passed, 41026 subtests, 1 known pre-existing failure (the census
above); coverage on the touched files 96-100%.
JarryShaw added a commit that referenced this pull request Sep 29, 2026
…red -1 mint (#880)

Completes #880's ngap.py half (the mh.py half landed in #889). Per the
owner's ruling, ProcedureCode/ProtocolIE are genuinely open per 3GPP TS
38.413, so they want a per-key unique value rather than the shared -1
sentinel #880 measured aliasing on -- sourced from pycrate's own compiled
NGAP specification through a vendor crawler, not hand-maintained or
minted on demand.

- Add pcapkit/vendor/ngap/{procedure_code,protocol_ie}.py: crawlers whose
  _request() imports pycrate_asn1dir.NGAP (no network fetch -- pycrate is
  already an installed optional dependency) and reads NGAP_Constants'
  (name, value) pairs, filtered by each value's own typeref.
- Add pcapkit/const/ngap/{procedure_code,protocol_ie}.py, generated:
  EnumRegistry + IntEnum, matching the house pattern. get() no longer
  mints on an unresolvable string key; _missing_ answers an in-range
  value it has not seen with a throwaway, non-registering member instead
  of a shared-value extend_enum mint, which is what stops two different
  unknown keys from aliasing.
- pcapkit/protocols/application/ngap.py now re-exports both from
  pcapkit.const.ngap under their historical names, dropping ~600 lines of
  hand-rolled IntEnum + extend_enum logic.
- Update tests/const/test_const_registry_protocol.py's own census (122
  const files -> 124, 105 generated-shape -> 107) and its narrative
  paragraph, since the two new const files land on the generated side.
  Extend four other tests/const/ generic sweeps' pinned counts the same
  way (+2 registries each).
- .github/workflows/cron-vendor.yml installs pycrate for the scheduled
  regeneration job: it runs pcapkit-vendor with no target, which now
  reaches both new crawlers, and pycrate is deliberately excluded from
  the `[all]` extra end users install.

New tests in tests/vendor/ and tests/protocols/application/, each shown
to fail without this fix. tests/const/test_const_registry_protocol.py in
full via plain unittest: Ran 74 tests, OK. pytest tests/const/ tests/vendor/
(excluding the reachability test) tests/protocols/application/test_ngap_unit.py:
384 passed, 41026 subtests, 0 known failures; coverage on the touched
files 96-100%.
JarryShaw added a commit that referenced this pull request Sep 29, 2026
…red -1 mint (#880)

Completes #880's ngap.py half (the mh.py half landed in #889). Per the
owner's ruling, ProcedureCode/ProtocolIE are genuinely open per 3GPP TS
38.413, so they want a per-key unique value rather than the shared -1
sentinel #880 measured aliasing on -- sourced from pycrate's own compiled
NGAP specification through a vendor crawler, not hand-maintained or
minted on demand.

- Add pcapkit/vendor/ngap/{procedure_code,protocol_ie}.py: crawlers whose
  _request() imports pycrate_asn1dir.NGAP (no network fetch -- pycrate is
  already an installed optional dependency) and reads NGAP_Constants'
  (name, value) pairs, filtered by each value's own typeref.
- Add pcapkit/const/ngap/{procedure_code,protocol_ie}.py, generated:
  EnumRegistry + IntEnum, matching the house pattern. get() no longer
  mints on an unresolvable string key; _missing_ answers an in-range
  value it has not seen with a throwaway, non-registering member instead
  of a shared-value extend_enum mint, which is what stops two different
  unknown keys from aliasing.
- pcapkit/protocols/application/ngap.py now re-exports both from
  pcapkit.const.ngap under their historical names, dropping ~600 lines of
  hand-rolled IntEnum + extend_enum logic.
- Update tests/const/test_const_registry_protocol.py's own census (122
  const files -> 124, 105 generated-shape -> 107) and its narrative
  paragraph, since the two new const files land on the generated side.
  Extend four other tests/const/ generic sweeps' pinned counts the same
  way (+2 registries each).
- Per the owner's ruling, declare pycrate in pyproject.toml's `vendor`
  extra (developer-facing, alongside requests/beautifulsoup4) rather than
  installing it ad hoc in a workflow; `all` still excludes it, unchanged.
  cron-vendor.yml installs `.[all,vendor]` so the scheduled regeneration
  job -- which runs pcapkit-vendor with no target and so now reaches both
  new crawlers -- has pycrate_asn1dir.NGAP importable. Extended one
  dependency-gate test (tests/test_tier_guard.py) that pinned `vendor`'s
  exact requirement-name set.

New tests in tests/vendor/ and tests/protocols/application/, each shown
to fail without this fix. tests/const/test_const_registry_protocol.py in
full via plain unittest: Ran 74 tests, OK. tests/test_tier_guard.py: Ran
107 tests, OK. pytest tests/const/ tests/vendor/ (excluding the
reachability test) tests/protocols/application/test_ngap_unit.py: 384
passed, 41026 subtests, 0 known failures; coverage on the touched files
96-100%.
JarryShaw added a commit that referenced this pull request Sep 29, 2026
- Added IPv6_GenericExt, a generic RFC 6564 §4 parser for IPv6 extension
  headers: it reads the guaranteed next-header/Hdr-Ext-Len octets and
  derives next/length/protocol from the actual parse, applying AH's and
  IPv6-Frag's own length rules where they diverge. Gated to version==6,
  since it is registered into the Internet-wide dispatch table for Shim6
  and would otherwise also activate for an IPv4 payload carrying protocol
  byte 140.
- IPv6._decode_next_layer now stops its walk structurally -- on any layer
  whose parsed info carries no `next` field -- instead of reading
  `info.next` unconditionally and crashing the whole packet when it hits
  ESP, BIT-EMU, 253, 254, or any other IANA code with no dedicated parser
  (the actual #891 defect). IPv6._import_next_layer substitutes
  IPv6_GenericExt for a recognised header whose own parser raises.
- IPv6_GenericExt.alias is hyphenated ('IPv6-GenericExt'), matching its
  siblings, so IPv6's `alias.lstrip('IPv6-')` keys the packet dict as
  'genericext' rather than '_genericext'.
- An overrun warns in the house SchemaWarning convention, then stops the
  walk rather than clipping: the length also decides where the next
  header starts, so a clipped skip distance would fabricate a layer.
- Exported IPv6_GenericExt from the pcapkit, pcapkit.protocols,
  pcapkit.protocols.data and pcapkit.protocols.schema aggregators.
- Re-pointed the #889 test pinning the pre-fix whole-packet collapse, and
  added coverage for both dispatch paths, the AH/Frag rules, the overrun
  stop, the version gate, and an unimplemented terminal code (BIT-EMU).

Build/test: tests/protocols/internet/ (260 passed, 868 subtests) and
tests/project/{test_public_api,test_isort_clean,test_module_isolation}.py
via the worktree's own pcapkit; coverage on the touched files rose from
81% to 94% with the new tests added. `make isort` clean; the two
remaining mypy findings and the pylint findings pre-exist on main.
JarryShaw added a commit that referenced this pull request Sep 29, 2026
- Added IPv6_GenericExt, a generic RFC 6564 §4 parser for IPv6 extension
  headers: it reads the guaranteed next-header/Hdr-Ext-Len octets and
  derives next/length/protocol from the actual parse, applying AH's and
  IPv6-Frag's own length rules where they diverge. Gated to version==6,
  since it is registered into the Internet-wide dispatch table for Shim6
  and would otherwise also activate for an IPv4 payload carrying protocol
  byte 140.
- IPv6._decode_next_layer now stops its walk structurally -- on any layer
  whose parsed info carries no `next` field -- instead of reading
  `info.next` unconditionally and crashing the whole packet when it hits
  ESP, BIT-EMU, 253, 254, or any other IANA code with no dedicated parser
  (the actual #891 defect). IPv6._import_next_layer substitutes
  IPv6_GenericExt for a recognised header whose own parser raises.
- IPv6_GenericExt.alias is hyphenated ('IPv6-GenericExt'), matching its
  siblings, so IPv6's `alias.lstrip('IPv6-')` keys the packet dict as
  'genericext' rather than '_genericext'.
- An overrun warns in the house SchemaWarning convention, then stops the
  walk rather than clipping: the length also decides where the next
  header starts, so a clipped skip distance would fabricate a layer.
- Exported IPv6_GenericExt from the pcapkit, pcapkit.protocols,
  pcapkit.protocols.data and pcapkit.protocols.schema aggregators.
- Re-pointed the #889 test pinning the pre-fix whole-packet collapse, and
  added coverage for both dispatch paths, the AH/Frag rules, the overrun
  stop, the version gate, and an unimplemented terminal code (BIT-EMU).

Build/test: tests/protocols/internet/ (260 passed, 868 subtests) and
tests/project/{test_public_api,test_isort_clean,test_module_isolation}.py
via the worktree's own pcapkit; coverage on the touched files rose from
81% to 94% with the new tests added. `make isort` clean; the two
remaining mypy findings and the pylint findings pre-exist on main.
@JarryShaw JarryShaw added this to the 1.5 milestone Oct 6, 2026
@JarryShaw JarryShaw moved this to Done in PyPCAPKit Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Issues reporting a defect (set by the bug report template; a default, not an assessment) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant