Skip to content

AppType.get(name) never matches an existing member and silently mints a port=-1 member instead #734

Closed

Description

@JarryShaw

Describe the bug

AppType.get has three defects on the same code path. The second silently returns wrong data.

>>> AppType.get('http')
TypeError: 'http' already in use as <AppType.http: 80 [tcp|udp|sctp]>
>>> AppType.get('ssh')
<AppType.ssh: -1 [undefined]>        # real ssh port is 22; enum grew 8182 -> 8183
>>> AppType.get(999999, proto='tcp')
unknown [999999 - tcp]              # ports are 16-bit

Root cause, pcapkit/const/reg/apptype.py:30620 — the string path tests key in AppType.__members_proto__, but that dict is keyed by TransportProtocol, so a str key never matches. It almost certainly meant AppType.__members__. Every name lookup therefore falls through to the mint branch:

  • name already a member → extend_enum raises TypeError: already in use
  • name not a member → mints a new member with port=-1, returns it, and permanently grows the enum

Separately, the same except ValueError wrapper swallows the guard that should reject out-of-range ports, so get(999999) mints instead of raising.

Expected behavior

get('http') returns the existing member. get('ssh') returns port 22. get(999999) raises, as every other registry does for an out-of-range code (see tests/const/test_const_enum_builtin_parity.py, issue #647).

Additional context

AppType is generated. The fix belongs in the template at pcapkit/vendor/reg/apptype.py and the generated pcapkit/const/reg/apptype.py, kept in lockstep — test_const_enum_builtin_parity.py lists pcapkit.vendor.reg.apptype in BESPOKE_TEMPLATES and renders the template to compare it character-for-character against the generated module. The crawler hits live IANA endpoints, so it cannot be re-run in CI.

Independent of #732, which would restructure these files: the #732 plan puts this fix first precisely because it carries no design dependency. Worth fixing once here rather than replicating into N classes later.

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 24, 2026
  2. JarryShaw commented on Sep 24, 2026

    @JarryShaw
    OwnerAuthor

    Correction: the "meant __members__" diagnosis in this issue is right about the typo but wrong as the fix. Verified directly:

    'ssh'  in AppType.__members__  -> False
    'http' in AppType.__members__  -> True
    ssh-ish member names           -> ['sshell', 'ssh_mgmt', 'ssh_22', 'ssh_null']
    AppType.ssh_22                 -> svc='ssh'  port=22
    

    The service name lives in .svc, not in the member's Python name. pcapkit/vendor/default.py's rename() appends _<port> whenever a service name occurs more than once in the IANA registry, and ssh occurs twice — port 22 and a no-port legacy row (ssh_null, -1). So swapping __members_proto__ → __members__ fixes get('http') (unique name, no suffix) and still leaves get('ssh') falling into the mint branch. A correct fix needs a lookup keyed on .svc.

    Also: the line number in the body is off by two. :30620 is the int-path mint call; the broken membership test is :30622, if key in AppType.__members_proto__.

    And the ambiguity is wider than one service. Over 7,245 distinct .svc values:

    count
    .svc names with more than one member 53
    ...with exactly one real port (rest are -1) 16 — tractable, "prefer the non--1 port"
    ...with two or more genuinely distinct real ports 37
    ...of those, real named services (not reserved/unassigned/unknown) 35

    Examples with no honest single answer: admind (3279, 8403), contentserver (454, 3365), creativepartnr (455, 3366), creativeserver (453, 3364). Picking one would reintroduce exactly the silent-wrong-data defect this issue is about, on a different input.

    Fourth defect on the same path, not in the original body: get's proto= argument is accepted but entirely unused on the string branch, so get('ssh', proto='tcp') behaves identically to get('ssh'). It could disambiguate some of the 37, though not all — several share proto flags at both ports.

    So this is not a one-line typo fix: it needs a .svc index plus a stated tie-break policy, written once in pcapkit/vendor/reg/apptype.py's LINE template and mirrored character-for-character into the generated file.

    Deferred to #732 per the maintainer, whose layout decision discards both files outright. Left open so the redesign has to answer these rather than inherit them.

  3. JarryShaw commented on Sep 24, 2026

    @JarryShaw
    OwnerAuthor

    Deferred to #732 by maintainer decision. Their words: "#734 will be resolved once #732's design landed i think."

    The decisive reason: #732's layout ruling discards pcapkit/{const,vendor}/reg/apptype.py outright in favour of a per-transport package, and those two files are the only ones a fix here would touch. A patch would be thrown away wholesale rather than rebased.

    Left open deliberately, so the redesign has to answer this rather than inherit it. What it must answer, per the corrections above:

    • A name lookup needs a .svc index — __members__ is keyed by the suffixed attribute name (ssh_22), not the service name.
    • A stated tie-break for the 37 .svc names mapping to two or more genuinely distinct real ports.
    • Whether proto= participates in that lookup; it is currently accepted and ignored on the string branch.

    The cost of waiting, stated plainly: AppType.get('ssh') keeps returning a member with port == -1 instead of 22, silently, until the redesign lands. The #732 plan's own sequence had put this fix first precisely because it was independent — that ordering is superseded by the decision to delete the files.

  4. added
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    on Sep 24, 2026
  5. JarryShaw commented on Sep 24, 2026

    @JarryShaw
    OwnerAuthor

    Re-measured on 932cb48d1 (#754). Two of the three defects are gone; the third is live, so this stays open — re-scoped to it.

    Dead — both string-path defects, by deliberate removal. _dispatch rejects a non-int key before the registry or the mint path (apptype.py:2356-2357), and its comment names this issue:

    AppType.get('ssh')  -> ValueError: 'ssh' is not a valid port number for AppType   (:2357)
    AppType.get('http') -> ValueError: 'http' is not a valid port number for AppType
    

    Member counts unchanged. Per #732 this is removed, not repaired — no .svc index and no name lookup survive, so the 37-service tie-break never had to be answered.

    Live — the out-of-range mint. The original diagnosis was exactly right. _missing_ does carry the range guard and does raise it (:2459-2460), but get's except ValueError (:2412-2418) swallows that very rejection and mints anyway:

    AppType.get(999999, proto='tcp') -> <TCP.unknown: 999999 [tcp]>  name='PORT_999999_tcp'  TCP 6147 -> 6148
    AppType.get(-1,     proto='tcp') -> <TCP.unknown: -1 [tcp]>      name='PORT_-1_tcp'      TCP 6147 -> 6148
    

    PORT_-1_tcp is not a valid Python identifier — the same hazard the old PORT_80_tcp|udp|sctp had, surviving on the negative branch.

    Why CI is green on it: the parity guard tests the constructor, which does raise — TCP(999999), TCP(-1), TCP(65536) all ValueError from :2460 — and test_every_registry_rejects_a_negative_value (:189-204) asserts cls(-1)/cls(65536), never .get().

    Dropping blocked: this defect never had a #732 dependency. It is a plain try/except bug in code #754 rewrote anyway, so the label was over-broad for this issue.

  6. removed
    blockedDeferred pending another issue or decision; see the last comment for what unblocks it
    on Sep 24, 2026
  7. JarryShaw commented on Sep 24, 2026

    @JarryShaw
    OwnerAuthor

    Closing this one, and keeping #758 for what is actually left.

    The defect in this issue's title and body — AppType.get(<name>) never matching a member and silently minting a port=-1 one — is gone as of #754. _dispatch now rejects a non-int key at apptype.py:2356-2357 before the registry or the mint path, and the name lookup no longer exists at all. Per #732 that is removed, not repaired.

    What survives is the out-of-range mint, which is a different mechanism — get's except ValueError swallowing _missing_'s range guard — and it is filed with its own reproduction as #758. Keeping this issue open under a title that describes a fixed defect would misrepresent it, and I created the overlap by filing #758 while re-scoping this one.

  8. 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