protocols: stop next-layer dispatch writing to shared registries, and fix two transport defects by JarryShaw · Pull Request #426 · JarryShaw/PyPCAPKit

JarryShaw · 2026-09-16T21:55:44Z

Closes #421. Closes #418. Closes #423.

Five commits, each its own concern. Verified independently against origin/main — I re-ran every reproduction rather than taking the numbers on trust.

#421 — parsing a packet polluted a shared class-level registry

ProtocolBase._import_next_layer indexed a defaultdict, so a lookup miss inserted the key. Before → after:

ICMP in Internet.__proto__ after parsing:  True  →  False
register_transtype(ICMP, …) warns:         'protocol 1 already registered,
                                            overwriting'  →  nothing
TCP.__proto__ after parsing:               [21, 80, None]  →  [21, 80]

The fix went in the base, as ProtocolBase._lookup_next_layer(registry, proto): hit → resolve and memoise as before; miss → return registry.default_factory() without recording it. Six read sites route through it — ProtocolBase.analyze, ProtocolBase._import_next_layer, Internet._import_next_layer, IPv6._import_next_layer, Transport.analyze, and the ethernet schema callback. Frame, PCAPNG, Link, TCP and UDP inherit it.

Two of those the issue did not name: Transport.analyze's ports[1] branch had the same miss, and schema/link/ethernet.py:27 subscripted Ethernet.__proto__ — which is Link.__proto__ — inside a PayloadField callback, so one frame with an unregistered EtherType leaked into the shared registry. Verified fixed:

EtherType 0x88B5 registered after parsing:  True  →  False

That callback also gains the ModuleDescriptor memoisation it never had, so it stops re-importing per frame.

It takes the registry as an argument rather than reading cls.__proto__, and that is load-bearing rather than stylistic: test_protocol_base_unit.py:317 sets __proto__ on an instance, which the old self.__proto__ honoured. A classmethod broke that test; the argument preserves the semantics.

SCTP._import_next_layer is now redundant and removed. It differed from the base only in the non-inserting lookup (now in the base) and a missing __context__=self._exctx — so removing it also restores parsing-context propagation to SCTP payloads.

A sweep confirmed six is the whole set for __proto__: of nine __proto__[ hits, seven are assignments inside register() and one is a plain dict. grep across protocols/schema/ returns that one ethernet line and nothing else.

#418 — TCP and UDP anonymised the port they could not place

tcp.info.dstport          : ssh [22 - tcp|udp|sctp]
tcp.payload.info.protocol : None  →  22
frame.protochain          : Ethernet:IPv4:TCP:Raw   (unchanged)

Transport._decode_next_layer now forwards sort_port[0] — the lower port, already its primary lookup key — instead of None. The protochain deliberately does not become ssh_22: ports arrive as plain int, because AppType is a StrEnum and sorted() over members would order them by string and break the lower-port rule. A plain int has no .name, so Raw still labels itself Raw, and Data_Raw.protocol is declared Optional[int], so an int is exactly the contract.

test_tcp_runtime.py:85 asserted assertIsNone(...) — that was the defect rather than a contract, and is now 22. test_http_runtime.py:105 still asserts None and still passes: that payload reaches Raw through beholder, a different path.

A pointer correction for anyone reading #418: its reference to test_sciop_unit.py:966 is wrong. At the commit the issue was filed against, line 966 was test_register_rejects_a_non_protocol; the test it means is test_unregistered_ppid_does_not_mutate_the_class_registry, now at 951–1016 since #417 grew the file. Find it by name. SCTP's behaviour is the reference here and is unchanged.

#423 — `TCP(**kwargs)` raised

Decision: the integer is accepted and coerced. TCP.make advertises srcport: 'Enum_AppType | int', so an int is documented input, not misuse. The schema field converted one only on the way out to bytes and never wrote it back, and ProtocolBase.unpack reuses the schema make built rather than re-parsing — so read saw an int where a parsed packet carries an AppType.

>>> TCP(srcport=80, dstport=443)
srcport=<AppType.www-http: 80 [tcp|udp]>  dstport=<AppType.https: 443 [tcp|udp|sctp]>

Checking the siblings widened it: UDP raised identically, and SCTP did not raise only because it keys its next layer on the DATA chunk PPID and never reads .port — it simply left an int in a field declared AppType. All three now normalise through Transport._make_port(port, proto). The transport protocol is a parameter because it distinguishes TCP/1 (tcpmux) from SCTP/1 (unassigned); a test pins that.

Fifth commit — one exception-type outlier

Transport.register raised bare TypeError where five sibling register() methods raise RegistryError with the identical message (internet.py:141, link.py:119, misc/pcapng.py:782, misc/pcap/frame.py:139, foundation/registry/protocols.py:148) — and the line above it already raised UnsupportedCall, so the method was inconsistent with itself. RegistryError is (BaseError, TypeError), so anything catching TypeError still catches it.

Verification

Byte comparison (15 captures × tree+json, ip=True, tcp=True, reassembly=True, against a pristine git archive of the base): the Parsing a packet pollutes the shared __proto__ registry, so a later register_* warns about a protocol nobody registered #421 commit alone is byte-identical, 30/30. The full stack differs in 24 of 30 files, and every differing line without exception is a Raw payload's protocol going null→N / NIL→N — 120 lines in each format, no protochain change, no other field. Ports now recorded: 67, 53, 443, 123, 22, 5353, 7000, 17500, 12345, 10352, 12089, 39645, 5001, 0.
Full suite: 836 passed, 17 skipped, 0 failed (808 subtests). The pristine baseline is 812 passed / 35 skipped; the 18 extra skips there are test_tier_guard.py declining to run outside a git checkout, so 812 + 18 + 7 new tests = 836. Nothing that passed before fails.
New tests fail on a pristine tree — checked rather than assumed. test_internet_unit.py's reproduction gives Items in the first set but not the second: <TransType.ICMP: 1>.
mypy: same pre-existing errors as baseline, none on new lines. isort and pylint clean.
No perf regression from dropping the miss-memoisation: http.pcap ×3 at 7.50 s against 7.61 s baseline.

Not folded in

#426 — the sibling registry family has the identical defect. __option__, __chunk__, __block__, __param__ and __cause__ leak the same way; eight registries share the shape, and one TCP packet with option kind 156 makes register_tcp_option(156, …) warn about an overwrite that never happened. Three of the files involved are outside this change's scope, so it is filed separately.

Not rebased, deliberately. origin/main moved to #419 while this was being verified, so the branch is 5 ahead / 1 behind. Rebasing would rewrite four already-pushed commits and need a force-push. #419 touches only docs/source/pep.rst, which this never touches; git merge-tree --write-tree origin/main HEAD exits 0 with no conflicts, which I confirmed.

`__proto__` is a `collections.defaultdict` held on a class attribute, so `self.__proto__[proto]` in `_import_next_layer` *inserted* every code it missed. Parsing a single 24-byte IPv4 datagram carrying `proto=1` therefore put `TransType.ICMP -> Raw` into `Internet.__proto__`, which every instance in the process shares, and a later and entirely legitimate `register_transtype(TransType.ICMP, ...)` warned that ICMP was "already registered, overwriting" about a registration that never happened. The registry also grew one entry per distinct unknown code seen, for the process lifetime, which makes "is this protocol registered?" depend on what has been parsed. Nothing consumed the inserted value -- it is the fallback the default factory would have produced anyway -- so the fix is to read the fallback without recording it: - add `ProtocolBase._lookup_next_layer`, which resolves a registered code exactly as before (including memoising a `ModuleDescriptor` back into the registry, which is an import cache rather than a new entry) and otherwise asks the registry's `default_factory` for the fallback. It takes the registry as an argument rather than reading `cls.__proto__`, so a caller that reached it through an instance keeps doing so. - route the five dispatch sites through it: `ProtocolBase.analyze` and `ProtocolBase._import_next_layer`, `Internet._import_next_layer`, `IPv6._import_next_layer`, and `Transport.analyze`, whose `ports[1]` branch had the same miss. `Frame`, `PCAPNG`, `Link`, `TCP` and `UDP` all use the base method, so they inherit the fix rather than each having to remember it. `Transport._decode_next_layer` passed `proto=None` for an unregistered port pair, which was leaking a `None` key into `TCP.__proto__`; that goes too. - drop `SCTP._import_next_layer`. It existed only to avoid this, and was otherwise the base method minus the `__context__=self._exctx` the base forwards -- so removing it also restores context propagation to an SCTP payload. Two tests: the base lookup in isolation, and the issue's reproduction against the real `Internet.__proto__` -- registry untouched after two datagrams, and `register_transtype` silent afterwards. Every capture in `examples/captures/` serialises byte-identically to `tree` and `json` with `ip=True, tcp=True, reassembly=True`. Closes #421.

`Data_Raw.protocol` is documented as "the original enumeration of this protocol", and it is worth most precisely when the protocol is unknown -- it is then the only record of what the payload claimed to be. SCTP and IPv4 both populate it for a code they cannot place (`SCTP:Unassigned_4243`, `IPv4:Use_for_experimentation_and_testing_253`), because they reach `Raw` through `_import_next_layer`, which forwards the code as `alias`. `Transport._decode_next_layer` did not: when neither port was registered it passed `proto=None`, so TCP and UDP anonymised the port they could not place. A consumer walking frames could not tell "TCP payload on some port we do not decode" from "TCP payload on port 22". It now forwards the lower port -- the same one it already uses as the primary lookup key -- registered or not. Ports arrive here as plain `int` (`srcport.port`, since `AppType` members do not order numerically), so the protochain still ends in `Raw` rather than in the port's name; only the recorded number changes. >>> tcp.info.dstport # ssh [22 - tcp|udp|sctp] >>> tcp.payload.info.protocol # was None, now 22 Tests: `test_tcp_runtime.py` had pinned the `None`, which was the defect rather than an intended contract, so that expectation is corrected and both UDP runtime cases gained the same assertion -- one with distinct ports (12345) and one with equal ports (5353). A new `test_transport_unit.py` case covers the branch directly, alongside the two that already pin which port wins when one of them is registered. Every capture in `examples/captures/` was reserialised to `tree` and `json` with `ip=True, tcp=True, reassembly=True`: 12 of the 15 differ, and every differing line without exception is the `protocol` field of a `Raw` payload going from `null`/`NIL` to a port number (120 of them). The three unchanged captures are unchanged for a reason: `arp.pcap` has no transport layer, and the `Raw` payloads in `http.pcap` and `http6.cap` are reached through `beholder`'s failure path, which already forwards the alias since #417 -- so they were labelled before this commit and are untouched by it. Closes #418.

>>> TCP(srcport=80, dstport=443) AttributeError: 'int' object has no attribute 'port' `TCP.make` advertises `srcport: 'Enum_AppType | int'`, so an integer is part of the contract rather than misuse. The schema field converts one on its way out to bytes (`PortEnumField.pre_process`) but never writes the converted value back, and `ProtocolBase.unpack` reuses the schema `make` built rather than re-parsing the bytes it just packed -- so `read` saw a plain `int` where a parsed packet carries an `AppType`, and `tcp.srcport.port` on line 479 raised. The three siblings disagreed about this: TCP and UDP both raised, while SCTP constructed fine only because it keys its next layer on the DATA chunk's PPID and so never reads `.port` -- it just left the `int` sitting in a schema field declared as `AppType`. So the integer is accepted and coerced, at the boundary where it enters: - add `Transport._make_port(port, proto)`, which returns an `AppType` for an integer and passes an `AppType` through untouched. The transport protocol is a parameter because it is what distinguishes TCP/1 (`tcpmux`) from SCTP/1. - call it from `TCP.make`, `UDP.make` and `SCTP.make`, so a constructed packet and a parsed one now agree on the type the schema declares. Parsing is untouched: every capture in `examples/captures/` serialises identically to the previous commit, byte for byte, in both `tree` and `json`. Closes #423.

`Transport.register` was the only one of six identical guards raising a bare `TypeError`. `internet.py:141`, `link.py:119`, `misc/pcapng.py:782`, `misc/pcap/frame.py:139`, `transport/sctp.py:597` and `foundation/registry/protocols.py:148` all raise `RegistryError` with the very same message, and the line above this one in the same method already raises `UnsupportedCall` -- so the method was inconsistent with itself as well as with its siblings. `RegistryError` is `(BaseError, TypeError)`, so a caller catching `TypeError` still catches it and nothing observable changes for them. The test now asserts both, to keep that documented rather than assumed. Also comments the `raise KeyError(name)` in `ProtocolBase._make_index`, which is *not* a stdlib exception escaping the library -- it is caught by the handler two lines below and converted to `ProtocolNotImplemented`. A pcapkit exception there would log at CRITICAL for something already handled, so the comment says so before somebody "fixes" it.

…t sees The sixth and last instance of #421, missed by the first pass because it is not in a `_import_next_layer` at all: `callback_payload` in the Ethernet *schema* resolved the next layer by subscripting the registry directly. protocol = Ethernet.__proto__[type_] `Ethernet.__proto__` *is* `Link.__proto__`, a class-level `defaultdict`, so one frame with an unregistered EtherType recorded it: registered before: False payload: Raw protocol: 34997 registered after : True new keys: {<EtherType.IEEE_Std_802_Local_Experimental_Ethertype_0x88B5: 34997>} Link.register warns: ['protocol ... already registered, overwriting'] and afterwards, with the lookup routed through `ProtocolBase._lookup_next_layer` as the other five sites now are: registered after : False new keys: set() Link.register warns: [] The payload still resolves to `Raw` and still carries the EtherType as `Data_Raw.protocol` (34997) -- only the registry write is gone. Resolving a registered `ModuleDescriptor` now also memoises the imported class, which this callback never did, so it stops re-importing on every frame. The existing `callback_payload` test covered both hit branches and neither miss; it now covers the miss and the memoisation as well. `ModuleDescriptor` is no longer named in the module and its import goes. A sweep for the same shape across the package -- `grep -rn "__proto__\[" pcapkit/` -- finds nothing else: every other occurrence is a `cls.__proto__[code] = protocol` *write* inside a `register()` method, which cannot inject, and `pcapkit/protocols/schema/` holds no other reference to a `__proto__` at all. Six sites is the whole set for this registry. Every capture in `examples/captures/` serialises identically to the previous commit, byte for byte, in both `tree` and `json`. Also, for the next reader of #418: it cites `tests/protocols/transport/test_sctp_unit.py:966` for the SCTP behaviour it uses as its reference, and at the commit it was filed against that line was `def test_register_rejects_a_non_protocol`, which is not the test it means. The one it means is `test_unregistered_ppid_does_not_mutate_the_class_registry` -- lines 871-938 then, 951-1016 now that #417 has grown the file above it. Find it by name rather than by line.

JarryShaw · 2026-09-16T22:26:09Z

this only applies to protocol registry? what about all other defaultdict registries. seems like they're all in scope. are they covered?

Copilot

🔵 Needs a closer look

It changes core next-layer dispatch behavior across multiple protocol layers and schema callbacks, so it warrants final human review for subtle compatibility impacts despite strong test coverage.

Pull request overview

This PR hardens next-layer dispatch to avoid mutating shared class-level registries during parsing, and fixes transport-layer behaviors around unknown ports and constructor port coercion (TCP/UDP/SCTP), with accompanying tests and docs updates.

Changes:

Add ProtocolBase._lookup_next_layer() and route next-layer lookups through it to prevent defaultdict miss-insertion while still memoising resolved ModuleDescriptor hits.
Fix transport dispatch to preserve unknown ports (forward lower port as alias) and normalise constructed port fields via Transport._make_port().
Align Transport.register() error type with sibling registries (RegistryError) and expand unit/runtime tests covering the regressions.

File summaries

File	Description
tests/protocols/transport/test_udp_runtime.py	Assert Raw payloads record the forwarded port for unregistered UDP payloads.
tests/protocols/transport/test_transport_unit.py	Add unit coverage for non-mutating lookups and `_make_port` behavior; update register error expectations.
tests/protocols/transport/test_tcp_udp_unit.py	Add construction tests ensuring TCP/UDP accept and coerce bare integer ports.
tests/protocols/transport/test_tcp_runtime.py	Update runtime expectation: unregistered TCP payload records port instead of `None`.
tests/protocols/transport/test_sctp_unit.py	Add unit test ensuring SCTP-constructed ports are coerced to `AppType`.
tests/protocols/test_protocol_base_unit.py	Add unit test for non-inserting registry lookup and descriptor memoisation.
tests/protocols/link/test_link_unit.py	Extend callback tests to ensure missing EtherType lookups don’t mutate `Link.__proto__`.
tests/protocols/internet/test_internet_unit.py	Add regression test preventing `Internet.__proto__` pollution on parse.
pcapkit/protocols/transport/udp.py	Coerce ports during UDP construction using `_make_port()`.
pcapkit/protocols/transport/transport.py	Use `_lookup_next_layer` in `analyze`, add `_make_port`, forward lower port for unknown dispatch, and raise `RegistryError` on invalid registration.
pcapkit/protocols/transport/tcp.py	Coerce ports during TCP construction using `_make_port()`.
pcapkit/protocols/transport/sctp.py	Coerce ports during SCTP construction and remove redundant `_import_next_layer` override.
pcapkit/protocols/schema/link/ethernet.py	Avoid registry mutation in schema callback by using `_lookup_next_layer`.
pcapkit/protocols/protocol.py	Implement `_lookup_next_layer` and route `analyze`/`_import_next_layer` through it.
pcapkit/protocols/internet/ipv6.py	Route next-layer selection through `_lookup_next_layer`.
pcapkit/protocols/internet/internet.py	Route next-layer selection through `_lookup_next_layer`.
docs/source/pcapkit/protocols/transport/transport.rst	Document new `_make_port` method.
docs/source/pcapkit/protocols/protocol.rst	Document new `_lookup_next_layer` method.

Review details

Files reviewed: 18/18 changed files
Comments generated: 0
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

… count (#719) - tests/protocols/test_dispatch_default_resolution_unit.py: this layer's reporter is #421 and its fix #426 (next-layer __proto__); #428 extended it to the option/chunk/block registries and #560 is the schema layer. The paragraph is re-wrapped to ~80 columns. - pcapkit/protocols/protocol.py (_lookup_next_layer): the same note cited #425 as this layer; it now cites #421 here, #425 for the sibling option, chunk and block registries, and #555 for the schema layer. - pcapkit/corekit/sentinels.py: the pointer to that note tracks it. - docs/source/contributing/pep.rst: ten protocols parse a checksum field, not eight -- IPX and MH were missing; "the other seven" is now nine. Prose only. Docs build warning set identical to origin/main.

JarryShaw added 5 commits September 16, 2026 17:15

JarryShaw mentioned this pull request Sep 16, 2026

Option, chunk and block registries leak on lookup miss, the same way __proto__ did #425

Closed

JarryShaw requested a lite review from Copilot September 16, 2026 22:25

Merge branch 'main' into fix/dispatch-and-registry

9f3642a

Copilot started reviewing on behalf of JarryShaw September 16, 2026 22:25 View session

Copilot AI reviewed Sep 16, 2026

View reviewed changes

JarryShaw commented Sep 16, 2026

View reviewed changes

Comment thread pcapkit/protocols/protocol.py

JarryShaw merged commit 209eabb into main Sep 17, 2026
25 checks passed

JarryShaw deleted the fix/dispatch-and-registry branch September 17, 2026 00:46

JarryShaw added the fix Pull requests that fix a defect (fix: subject prefix) label Sep 22, 2026

JarryShaw mentioned this pull request Oct 3, 2026

docs(changelog,protocols): credit the registry-leak fixes to the right layer #1009

Merged

4 of 12 tasks

JarryShaw added this to the 1.5 milestone Oct 6, 2026

Navigation Menu

Uh oh!

protocols: stop next-layer dispatch writing to shared registries, and fix two transport defects - #426

protocols: stop next-layer dispatch writing to shared registries, and fix two transport defects#426
JarryShaw merged 6 commits into
mainfrom
fix/dispatch-and-registry

JarryShaw commented Sep 16, 2026

Uh oh!

JarryShaw commented Sep 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

JarryShaw commented Sep 16, 2026

#421 — parsing a packet polluted a shared class-level registry

#418 — TCP and UDP anonymised the port they could not place

#423 — TCP(**kwargs) raised

Fifth commit — one exception-type outlier

Verification

Not folded in

Uh oh!

JarryShaw commented Sep 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

#423 — `TCP(**kwargs)` raised