Skip to content

fix: six .get implementations mint unknown keys at a shared -1, so the second aliases to the first #880

Description

@JarryShaw

Six .get implementations mint an unknown str key at a shared sentinel value -1, so the second unknown key silently resolves to a member named after the first. Measured on main at edc1b32e0:

before: members=6 v2m=6
get("bogus_one") -> <FastBindingAcknowledgmentStatus.bogus_one: -1>
after1: members=7 v2m=7
get("bogus_two") -> <FastBindingAcknowledgmentStatus.bogus_one: -1>    a is b ? True
after2: members=8 v2m=7        <- __members__ grew, _value2member_map_ did not
cls["bogus_two"] -> <FastBindingAcknowledgmentStatus.bogus_one: -1>

aenum treats the duplicate -1 as an alias request, so cls['bogus_two'] returns a member whose name is bogus_one. A name that lies about itself.

It also bypasses the class's own stated range invariant. All four mh.py _missing_ methods reject anything outside 0 <= value <= 255, yet after one get('bogus_one'):

F(-1) -> <FastBindingAcknowledgmentStatus.bogus_one: -1>

extend_enum with an explicit value never consults _missing_, so get installs a member the class itself declares invalid.

Six sites, one shape — protocols/internet/mh.py::{FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode, LocalizedRoutingStatus, LMAAddressCode} and protocols/application/ngap.py::{ProcedureCode, ProtocolIE}. The mh.py four return Cls[key] after extending and the ngap.py two return extend_enum(...) directly, but both land on the same aliased member, so the divergence saves neither.

Note the two families want different fixes, per #877's audit: the four mh.py enums cite documents that say "IANA keeps no registry of them" (e.g. mh.py:576), so they should not be minting at all; ProcedureCode/ProtocolIE are genuinely open per 3GPP TS 38.413 and need a per-key unique value rather than a shared sentinel. LocalizedRoutingStatus.get and LMAAddressCode.get have zero callers repo-wide and can simply go.

Two existing tests pin the current minting and will need re-pointing: tests/protocols/internet/test_mh_unit.py:1530 and :1534 assert get('Vendor_specific', 200) == 200.

Activity

  1. added
    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)
    fixPull requests that fix a defect (fix: subject prefix)
    on Sep 28, 2026
  2. JarryShaw commented on Sep 28, 2026

    @JarryShaw
    OwnerAuthor

    Checkable blocker: #877's per-class verdicts.

    gh issue view 877 -R JarryShaw/PyPCAPKit --json state

    Four of this defect's six sites are the mh.py enums, and #877's audit found each of them cites a document saying the opposite of what it does — e.g. mh.py:576: "IANA keeps no registry of them". Under the owner's ruling on #877 ("make them immutable - unless RFC/IANA says otherwise") those four should not mint at all, which makes "give each unknown key a unique value" the wrong fix for them and "stop minting, return a non-registering member or raise" the right one. The other two — ProcedureCode and ProtocolIE — are genuinely open per 3GPP TS 38.413, so they do want a per-key unique value rather than the shared -1.

    So the six sites want two different fixes, and which one applies to the mh.py four is decided on #877, not here. Splitting a single six-site defect across two PRs to avoid waiting would cost more than it saves, so this stays blocked until #877 settles those rows.

    What is not blocked and can be done the moment anyone wants it: LocalizedRoutingStatus.get and LMAAddressCode.get have zero callers repo-wide, tests included, so two of the four mint sites can simply be deleted rather than redesigned.

  3. added
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    wipWork in flight - a covering PR is open or an agent is actively on it
    and removed
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    on Sep 28, 2026
  4. JarryShaw commented on Sep 29, 2026

    @JarryShaw
    OwnerAuthor

    All six sites are resolved. Closing, with one vestigial leftover noted rather than left silent.

    The four mh.py enums landed in #889 (fde4cb20c); ProcedureCode and ProtocolIE landed in #899 (d90223bf5) as pycrate-sourced generated registries per your ruling. And the blocker recorded on this issue — "#877's per-class verdicts" — is discharged: #877 is ruled, its phase 1 merged as #906 (c50f442db), and the registry-wide case audit continues as #903.

    I checked for a seventh site rather than trusting the count, because git grep still finds the -1 default:

    pcapkit/const/pcapng/option_type.py:238   def get(key, default: 'int' = -1, *, namespace='opt')
    pcapkit/vendor/pcapng/option_type.py:148  (the crawler template for the above)
    

    Measured on origin/main, and OptionType is clean on both halves of this defect:

    before: members=40 values=40
    get(60000)            -> opt_unknown [60000]      registered=False
    get('zz_bogus_option') -> zz_bogus_option [-1]     registered=False
    after : members=40 values=40    names added: []   values added: []
    is -1 a registered value? False
    

    No minting — neither lookup table grows. And no aliasing, which is the part I nearly got wrong: the repr renders as <OptionType.zz_one: -1> and I first read that as the shared sentinel, but the actual value is 'zz_one [-1]' — the key is embedded, so two different unknown strings stay distinct:

    get("zz_one") -> value='zz_one [-1]'
    get("zz_two") -> value='zz_two [-1]'
    distinct values? True      both == -1? False
    int path: 'opt_unknown [60001]' vs 'opt_unknown [60002]'   distinct? True
    

    So the -1 survives only as a signature default that is never used as a value — cosmetic, and misleading in exactly the way that cost me a second look. It is worth removing, but it is not this defect and it does not keep the issue open. Anyone touching pcapng/option_type.py next should drop it, in the crawler template at vendor/pcapng/option_type.py:148 rather than the generated file, or the next regeneration restores it.

    Closing. Reopen if you read the vestigial default as in scope.

  5. removed
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Sep 29, 2026
  6. added this to the 1.5 milestone on Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

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

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions