Ship decode_frame(), the chain walker - #126
Merged
Merged
Conversation
The README taught a hand-rolled eight-line loop, ARCHITECTURE.md showed
it again, and the test suite kept a private copy, so the library's
most-used function was the one function it did not provide. Everyone
using it maintained their own.
decode_frame(frame) returns a Packet of every decoded layer, with the
parts a copy-pasted loop never has.
An explicit starting layer. decode_frame(buf, start=IPv4) walks a
buffer that begins mid-stack: a tunnel payload, a packet quoted inside
an ICMP error, a non-Ethernet link type. There was previously no way to
ask for this at all.
Bounded depth. max_depth (default 32) raises the new
MaxDepthExceededError, rooted at ProtocolError like everything else.
Chains already terminated, since every header validates its own
declared length against the buffer, so this bounds cost rather than
correctness. The case it actually saves is a zero-length header that
chains to itself, which nothing in the decode contract forbids and a
third-party registration can now produce: the cursor never advances and
the walk never ends. tests/test_walk.py builds exactly that and asserts
the walker returns instead of hanging. The corpus peaks at 5 layers.
A lax mode that reports instead of raising. lax=True ends the walk on a
ProtocolError and returns the layers decoded so far, with the reason on
packet.stopped_by, which is what a capture tool needs when frame
4,000,001 is malformed. It relaxes the walk, never a decoder: every
layer it returns was decoded under the ordinary strict rules, and that
is asserted. Caller mistakes -- a bad max_depth, an unknown decode_as
table -- are not absorbed by it.
Per-call decoder overrides. decode_as={"udp.port": {6969: DNS}} reads
DNS on a nonstandard port for one call without touching global state,
built on Registry.derive(). Making that work needed next_protocol() to
be redirectable, and it read the process-wide tables directly. It now
takes an optional registry, passed to the same dispatch helpers as
before, so no table knowledge is duplicated and the default path stays
a single dict.get. Measured in one process, the optional parameter
costs +2.1 ns per dispatch, about 0.1% of a frame decode; every
existing zero-argument call is unaffected.
Packet gains stopped_by (why a walk ended early; None for a packet you
built) and consumed (the bytes its headers occupy, so
frame[packet.consumed:] is what the chain did not decode). Both default
to the constructed-packet values, so Packet(eth, ip) is unchanged.
On memoryview, which the issue asked to benchmark rather than assume:
wrapping each frame in one measures 0.95x on the corpus -- 5% slower --
because for a single small frame the view costs more to build than the
copy it saves. So the walker slices whatever it is handed and never
converts. bytes stays fastest for one frame, and a memoryview over a
large capture buffer keeps slices zero-copy, which is the case worth
1.8x once a pcap reader exists. Byte-exact round-tripping through a
memoryview is asserted over the corpus.
The copies are retired: README.md and ARCHITECTURE.md now show the
shipped call, tests/test_corpus.py's walk() is a thin adapter over it,
and test_contract.py's inline copy is gone. One test still walks the
corpus with a hand-rolled loop and asserts the shipped walker agrees
layer for layer -- that is what makes retiring the others safe.
scripts/benchmark.py keeps its raw loop deliberately: it measures
decode throughput against a committed baseline, and wrapping it in
Packet construction would change what the gate measures.
Closes#88.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QJnVMNGwTRDktC4rkABtgtUh oh!
There was an error while loading. Please reload this page.
This was referenced Sep 4, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The README taught a hand-rolled eight-line loop, ARCHITECTURE.md showed it
again, and the test suite kept a private copy — so the library's most-used
function was the one function it did not provide.
Closes#88.
What's included
src/netprotocols/walk.py—decode_frame(), with the four things acopy-pasted loop never has.
decode_frame(buf, start=IPv4)for a bufferthat begins mid-stack — a tunnel payload, a packet quoted inside an ICMP
error, a non-Ethernet link type. There was previously no way to ask for this.
max_depth(default 32) raises the newMaxDepthExceededError, rooted atProtocolErrorlike everything else.lax=Trueends the walk on aProtocolErrorand returns thelayers decoded so far, with the reason on
packet.stopped_by.decode_as={"udp.port": {6969: DNS}}, builton A public protocol registry #87's
Registry.derive().Packetgainsstopped_by(why a walk ended early;Nonefor a packet youbuilt) and
consumed(bytes its headers occupy). Both default to theconstructed-packet values, so
Packet(eth, ip)is unchanged.Copies retired — README.md, ARCHITECTURE.md,
tests/test_corpus.py(now athin adapter),
tests/test_contract.py(inline copy gone).Tests —
tests/test_walk.py, 85 new tests.Verification
uv run ruff checkanduv run ruff format --checkare cleanuv run mypyis clean (strict)uv run pytestpasses locally — 882 passed, coverage 99.88% (gate 98%)CHANGELOG.mdhas an entry under## [Unreleased]New protocol or dispatch change — also:
src/netprotocols/_base.pyunchanged;next_protocol()gains an optional parameter, covered belowEtherType/IPProtocolnumberstests/test_fuzz.py::ALL_PROTOCOLSis unchangedNotes
I was wrong in #87 about this being a small change
I said
derive()would makedecode_as"a small change rather than a seconddispatch mechanism." The
derive()part held; the rest did not.next_protocol()takes no arguments and reads the process-wide tables directly, so a walker
holding a custom registry had no way to redirect it — and decision 1 on #87
explicitly ruled out changing that signature.
The resolution is an optional
registry=Noneparameter, passed to the samedispatch helpers as before. That matters because the helpers already own their
table names, so nothing is duplicated and there is no second dispatch path to
keep in sync. Every existing zero-argument call is unaffected.
Cost, measured in one process (identical bodies, one with the parameter):
+2.1 ns per dispatch, roughly 0.1% of a frame decode. A first cross-run
comparison suggested +15 ns; that was the machine noise documented in #87
(±10–15% between consecutive runs), not the parameter. The in-process A/B is
the number that resolves.
If you'd rather this signature stayed frozen, the alternative is dropping
decode_asfrom #88 and reopening Q4 — say so and I'll do that instead.memoryview: benchmarked, then rejected
The issue said to benchmark rather than assume, so I did. Wrapping each frame in
a
memoryviewmeasures 0.95× on the corpus — 5% slower — because for asingle small frame the view costs more to build than the copy it saves. That
matches the warning in #100.
So the walker slices whatever it is handed and never converts:
bytesstaysfastest for one frame, and a
memoryviewover a large contiguous capture bufferkeeps slices zero-copy, which is #100's 1.8× case. Converting internally would
have been worse than either. Byte-exact round-tripping through a
memoryviewisasserted over the corpus.
What the depth guard actually saves
Not a runaway chain — every header validates its own declared length, so chains
already terminated. The case it saves is a zero-length header that chains to
itself: the cursor never advances and the walk never ends. Nothing in the
decode contract forbids that, and since #87 a third-party registration can
produce it.
tests/test_walk.py::TestHostileChainsbuilds exactly that andasserts the walker returns rather than hangs.
Two deliberate exclusions
scripts/benchmark.pykeeps its raw loop. It measures decode throughputagainst a committed baseline; wrapping it in
Packetconstruction would changewhat the gate measures and invalidate
benchmarks/baseline.json. Worthrevisiting as a separate
--walkercomparison.#92's
decode_lax()is not in here.#92 asks that its lenient mode and thisone "share one concept, not invent two", and it wants the stop reason to be
#91's structured diagnostic. So
stopped_byholds theProtocolErroritselfrather than a new parallel type — when #91 gives those exceptions structure,
this contract gains it for free with no API change.
On the README
Touching it was this issue's acceptance criterion, so I did — but only the
decoding section, plus a short new section on registering a protocol we don't
ship (which #87 left undocumented anywhere user-facing). The coverage table and
everything comparative are untouched.
🤖 Generated with Claude Code
https://claude.ai/code/session_01QJnVMNGwTRDktC4rkABtgt
Generated by Claude Code