fix(pcapng): keep the captured octets every packet block declares (#646)

[JarryShaw]

Fixes #646.

The defect, re-measured

The issue reported against 375e9d411 and against the generated test.pcapng. Re-measured here on the committed fixture examples/captures/dhcp.pcapng, on CPython 3.14.7, with pcapkit.__file__ pinned to this worktree:

idx block_type              captured_len original_len len(packet)
  0 Enhanced_Packet_Block            314          314           0
  1 Enhanced_Packet_Block            342          342           0
  2 Enhanced_Packet_Block            314          314           0
  3 Enhanced_Packet_Block            342          342           0

The octets are read correctly and then discarded. Same run, same blocks, reading the schema directly:

frame 0  schema.get_payload()  -> 314 octets
  b'\xff\xff\xff\xff\xff\xff\x00\x0b\x82\x01\xfcB\x08\x00E\x00\x01,\xa86\x00\x00\xfa\x11\x17\x8b\x00\x00\x00\x00\xff\xff'...
frame 0  info.packet           -> b''

After this change, info.packet is those 314 octets, and frame.packet.header is the 28-octet Enhanced Packet Block prefix rather than the whole 348-octet block.

The overwrite site, re-located

Every line number in the issue predates #640, which added _DECLARED_KEYWORDS and the _Absent machinery to pcapkit/protocols/protocol.py.

Site	Issue says	Actually (post-#640)
the overwrite	protocol.py:647	protocol.py:994 — self._info.__update__(packet=self.packet.payload)
ProtocolBase.packet	protocol.py:196-205	protocol.py:471-480
PCAPNG.length	misc/pcapng.py:678-681	misc/pcapng.py:678-681 (unmoved)
PCAPNG.unpack's extraction	misc/pcapng.py:902-909	misc/pcapng.py:902-909 (unmoved)

The mechanism is exactly as reported and was verified rather than taken on trust: PCAPNG.length returns self._info.length, which is assigned length=schema.length — the wire's Block Total Length — so _read_packet(header=self.length) consumes the entire per-block buffer as header and leaves nothing for the payload. Measured on the fixture: frame.length == 348 and len(frame.packet.header) == 348 for a 348-octet block.

Which block types are affected

All three that carry captured octets, i.e. exactly PCAPNG.PACKET_TYPES and exactly the three schemas declaring __payload__ = 'packet_data':

Block	Code	Payload offset	Before	After
Enhanced Packet Block	0x00000006	28	b''	its own octets
Simple Packet Block	0x00000003	12	b''	its own octets
Packet Block (obsolete)	0x00000002	28	b''	its own octets

No committed fixture has an SPB or a Packet Block — dhcp.pcapng is all EPBs — so a synthetic capture carrying one of each was built to measure them, and it is built in the test too. Every other block type (Section Header, Interface Description, Name Resolution, Interface Statistics, Decryption Secrets, Custom, systemd Journal Export) declares no payload field, reported b'' before, and still reports b''.

Where the fix belongs, and why

At the computation, not at the overwrite. The issue offered both. The overwrite at protocol.py:994 is correct for every protocol laid out as a header followed by its payload, which is all of them but this one, so it is not the defect.

Two reasons the injection site is the wrong place to fix it:

1. No value of length can make the inherited split work. A PCAP-NG block carries a trailer after its payload — the option list, its padding, and a repeat of the Block Total Length. _read_packet(header=N) takes everything after N as payload, so even a correct 28 would yield packet_data + padding + options + trailer, not the 314 captured octets.
2. Skipping the injection would leave the public API broken. Teaching __init__ to not overwrite an already-set packet heals frame.info.packet and leaves frame.packet — a documented property — still reporting the whole block as header and b'' as payload. That is the same defect seen from the other side.

So PCAPNG.packet is overridden to take the payload from the block schema's __payload__ field and the header from the octets ahead of it, summed out of the schema buffers so that the three different payload offsets need not be hard-coded per block type. PCAPNG.unpack then reads that property instead of extracting the payload a second time of its own, which leaves one source of truth where there were two attempts at one.

PCAPNG.packet is a plain @property where the inherited one is a cached_property, which is deliberate. The inherited one caches because it reads the stream, and a second read would consume octets that are gone; this one only walks buffers the schema layer has already filled, so it costs a few dict lookups and has nothing to amortise. Caching it would also reintroduce the same class of staleness this change removes by a different route: unpack now reports the payload through this property, so a cache would make a second unpack on one instance hand back the first call's octets with get_payload never reached, where the code before this change recomputed from the schema every time. Nothing in the tree calls unpack twice on one instance today — __post_init__ is its only caller — so that is an invariant being kept rather than a bug being fixed, and tests/protocols/misc/test_pcapng_unit.py now asserts it directly: swap the block on a live instance, unpack again, and the payload must be the new block's. With cached_property that assertion fails b'payload' != b'cached'.

pcapkit/protocols/protocol.py carries a docstring change only — ProtocolBase.packet now states the contract it relies on and names this as what happens when a protocol breaks it. No executable line changed there (503 statements before and after).

The dump, which is the part that corrupted a file

PCAPIO._append_value writes value.packet after each 16-octet record header, so the empty payload reached the file. Dumping the four dhcp.pcapng blocks produced 104 octets: 24 of global header plus four record headers, each declaring hundreds of octets and delivering none. Every reader that walks by incl_len — which is all of them — loses frame sync at the first record. It now round-trips, and the test asserts both the dumped file's total size and each record's octets against the source capture's own hand-parsed bytes.

Failing-then-passing evidence

Exit codes read from files, not from a pipeline, and each run printing the pcapkit.__file__ it resolved.

Before — pristine git archive of the base commit, pinned by a root conftest.py:

[conftest] pcapkit.__file__ = /local/home/jarryx/scratch-646-pcapng-payload/base/pcapkit/__init__.py
17 failed, 5 passed, 2 warnings, 2 subtests passed in 7.07s
exit code from file: 1

After — this branch:

[preamble] pcapkit.__file__ = .../.claude/worktrees/agent-ad04d3ab1119c4c65/pcapkit/__init__.py
7 passed, 21 subtests passed in 7.03s
exit code from file: 0

Note that several per-test lines read PASSED in the before-run while every one of their subtests SUBFAILED; the exit code and the subtest tally are the signal, not the per-test line.

The assertions are on real payload bytes, not on lengths — a length assertion passes under several wrong fixes, including a payload read from the wrong offset and one that picked up the block's 32-bit padding. Each synthetic payload has a different length modulo 4 (0, 1 and 1 octets of padding) for the same reason, and the dhcp.pcapng expectation is derived twice over: once as spelled-out head and tail literals, once by hand-parsing the file with struct alone so the expected value owes nothing to the code under test.

Shapes neither fixture exercised

Two gaps were found in the first draft of these tests and closed. Neither dhcp.pcapng nor the first synthetic capture had an option area after the payload — dhcp.pcapng's four blocks all have options: len=0 — and neither had a big-endian section. Options sitting after the captured data are exactly what a payload offset walked from the wrong end would swallow, and a block with none cannot tell the difference. Six shapes were then measured directly, all passing, each checking the actual payload bytes, that header + payload equals the leading octets of the raw block, and that len(packet) == captured_len:

Shape	Result
EPB with epb_flags + opt_endofopt after the payload	header 28, bytes match
big-endian section, all three block types	headers 28 / 12 / 28, bytes match
EPB with captured_len (19) < original_len (9999)	header 28, bytes match
SPB with snaplen (20) < original_len (34)	header 12, bytes match
payload length an exact multiple of 4 (no padding)	header 28, bytes match
obsolete Packet Block with options after the payload	header 28, bytes match

The first, second and fourth are now in the committed test: the synthetic capture carries options on both blocks that can have them, the three-block-type test runs over both byte orders, and test_a_snapped_block_carries_the_octets_that_are_present covers the two ways a block can be snapped. The payload offsets are stable at 28 / 12 / 28 regardless of byte order and of whether options are present, which is the property the walk has to have.

Regression check

Scoped, on the rebased tree, pcapkit.__file__ printed and pinned:

tests/protocols tests/toolkit tests/dumpkit
tests/integration/test_pcapng_end_to_end.py tests/foundation/engines/test_pcapng_engine.py

769 passed, 5 skipped, 7531 warnings, 1796 subtests passed in 864.71s (0:14:24)
exit code from file: 0

Zero FAILED / SUBFAILED / ERROR lines. One pre-existing test did have to change: tests/protocols/misc/test_pcapng_unit.py::PCAPNGUnitTests::test_pcapng_remaining_constructor_branches_and_custom_dispatch stubs the schema with types.SimpleNamespace, and since unpack now reaches the payload through self.packet, those stubs needed the __fields__ / __buffer__ surface that property reads, plus _data on the no-payload stub. The assertion get_payload.assert_called_once_with() became assert_called_once_with('packet_data') because the call now names the field explicitly — which is deliberate, since the same name drives the offset walk and passing it keeps header and payload from being able to disagree. The branch that proves unpack skips re-unpacking when __header__ is already set is still proven, by the shared __schema__.unpack mock still reading assert_called_once; it needed a second instance rather than a reused one only because packet is a cached_property.

Coverage

coverage run -m pytest (the repo uses coverage, not pytest-cov) over tests/protocols/misc/test_pcapng_unit.py, tests/protocols/test_pcapng_regression.py and tests/foundation/engines/test_pcapng_engine.py.

Both trees run this PR's current tests, which is the only comparison that isolates the code change: measuring the base with the old tests and the branch with the new ones moves the test suite and the library at once and then credits the difference to whichever you please.

File	Stmts	Miss	Branch	BrPart	Cover
misc/pcapng.py base	1527	1	666	1	99.91%
misc/pcapng.py PR	1545	1	672	1	99.91%
protocols/protocol.py base	503	228	170	16	49.18%
protocols/protocol.py PR	503	228	170	16	49.18%

Every one of the 18 new statements and 6 new branches in misc/pcapng.py is executed: the miss count is flat at 1, the partial-branch count flat at 1, and the percentage identical to two decimal places. That single miss is the same pre-existing statement in both — return cast('timezone', tzinfo) in _get_timezone — renumbered 1153 to 1265 by the insertions above it.

protocol.py is identical in every column, which is what a docstring-only change should produce and is the check that it really is docstring-only.

tests/protocols/test_pcapng_regression.py grows 4 tests to 11 and 3 subtests to 24.

A note on how not to measure this, since it cost a wrong table once: coverage re-parses the source at report time, so a data file produced before an edit and reported after one maps recorded line numbers onto shifted source and reads far worse than reality — misc/pcapng.py appeared to drop to 38% that way. Every number above was reported from a run made against the source as committed.

Adjacency

• fix(pcapng): bound an option's payload to the area its block declares (#594) #676 (pcapkit/protocols/schema/misc/pcapng.py, the A 16-bit padding shortfall band is still unbudgeted after #593: a crafted capture amplifies 1,637x, indistinguishable from a truncated one at the field layer #594 bound fix) — not touched. Its author's claim holds on inspection: its only changes to the two blocks I read wrap the options field's length in bounded_area(...), and options sits after packet_data in field order, so the offset walk here stops before reaching it. captured_len, captured_length and packet_data are untouched by it.
  One thing worth flagging: fix(pcapng): bound an option's payload to the area its block declares (#594) #676 also modifies tests/protocols/misc/test_pcapng_unit.py, which this PR modifies too. The hunks do not overlap — fix(pcapng): bound an option's payload to the area its block declares (#594) #676 appends 400 lines at line 3180 with zero deletions, this PR edits lines 1881-1956 — so they should merge cleanly, but they are not independent files and whichever merges second should confirm it.

• Every EOF-truncated PCAP-NG file raises an uncaught ValueError: pcapng_block_selector passes a negative __length__ to SchemaField #678 (every EOF-truncated PCAP-NG raising an uncaught ValueError) — neither better nor worse. Not fixed, as instructed. This change touches neither pcapng_block_selector nor any length computation.

  The truncation set, stated so the tally is reproducible rather than asserted — "101 levels" admits several readings and they do not all give the same counts:

  raw = pathlib.Path('examples/captures/dhcp.pcapng').read_bytes()   # 1508 octets
  lengths = [max(1, len(raw) * i // 100) for i in range(101)]        # 101 distinct: 1, 15, 30, ... 1508

  Whole-percent prefixes, with i=0 raised to 1 octet so no case is the empty file. 1508 * i // 100 happens to repeat no value, so the 101 levels are 101 distinct prefix lengths. On that set, both trees:

  Outcome	Count
  ValueError: read length must be non-negative or -1	96
  FormatError: unknown file format: b'\n'	1
  ProtocolError: PCAP-NG: [if_tsresol] invalid length (expected 1, got 0)	1
  parsed (0, 2 and 4 frames)	3

  Compared level by level rather than only in aggregate: the SHA-256 of the sorted {prefix_length: outcome} mapping is ca44d3ee658087cf2a667c454f839dd931c1c04790b2fe32cee8e1a2e861b182 on both trees, so no level changed behaviour even in a way that a matching aggregate could hide.

• EXPECTED_FAILURES in tests/protocols/test_option_roundtrip_unit.py — imported rather than grepped, since ** unpacking defeats a grep. 45 entries in both trees, byte-identical when sorted and serialised. No entry moved, and none was deleted.

One behavioural consequence not fixed here

examples/captures/pcapng.txt is a committed legacy-smoke reference regenerated by hand from examples/legacy_smoke/Makefile, and its four frame-level |-- packet -> NIL lines are now stale — those blocks do carry octets. No test asserts on it, it is outside this PR's file ownership, and regenerating it would pull in the unrelated pre-existing drift already present in the sibling examples/captures/out.*. Filed separately rather than fixed in passing.

Labels: fix + breaking

fix is uncontroversial — this is dropped data with a wire-format consequence.

breaking is the argued one, and it is warranted. This is not a change that only corrects an error path: it changes the parse output of every PCAP-NG capture, for two properties, on the success path.

• frame.info.packet goes from b'' to hundreds of octets for every packet block. Any caller that serialises info — and the JSON, PList and tree dumpers all do — produces different output for the same input file.
• frame.packet.header goes from the entire block to the pre-payload prefix (348 octets to 28 on the fixture's first frame).
• Files written through PCAPIO change size and content. That is the point of the fix, but it is still a change to an artefact a caller may be diffing.

The counter-argument is that nobody can sensibly have depended on an empty payload, and that restoring correct data is not a "break". That argument is about whether the change is desirable, which it is; breaking is about whether output moves, which it does, everywhere, for every PCAP-NG file. A caller with a golden-file test will see it fail, so it should be announced rather than discovered — and breaking is additive, so carrying it costs nothing that fix conveys.

Not claimed

CI is not claimed green. The verification here is local, scoped (tests/protocols, tests/toolkit, tests/dumpkit, the PCAP-NG engine and end-to-end modules), and on 3.14.7 only. The four changed files also compile clean on 3.12.14 and 3.13.15, and the change introduces no version-gated syntax or semantics — cached_property comes through the existing pcapkit.utilities.compat shim that ProtocolBase.packet already uses — but a single interpreter cannot see a version boundary and only the matrix can.

pylint and mypy were run with the repository's own flag sets on both changed library files, in both trees, and the findings are identical: 50 pre-existing line-too-long and the rest unchanged by category and count for pylint, the same 10 pre-existing errors for mypy. Neither tool has a new complaint.

Changelog

No changelog on this branch, per the #657 arrangement. The entry goes to docs/changelog-1.5.0.

[JarryShaw]

Follow-up filed for the one behavioural consequence this PR deliberately does not fix: #685 — examples/captures/pcapng.txt is a committed legacy-smoke reference whose four frame-level packet -> NIL lines are now stale. Not fixed here because examples/captures/ is outside this change's file ownership and because the sibling out.* files carry unrelated pre-existing drift that the legacy-smoke Makefile would regenerate at the same time.

Changelog entry for this PR is on docs/changelog-1.5.0 (#657) as dbcd0e9aa, fast-forwarded from 33464d923; python util/changelog_md.py --check exits 0 at that commit.

[JarryShaw]

Amended and re-pushed as 428baeb5c (was 41ac27c88): the library change is unaltered, the tests are hardened. Two shapes the first draft did not cover turned up while re-reading the fixture — an option area after the captured data (all four of dhcp.pcapng's blocks have options: len=0, so neither it nor the first synthetic capture exercised it) and a big-endian section. Both are now in the test, along with a snapped block in both the shapes that express it. Four additional shapes were measured directly and pass. Counts move 4 tests → 11 and 3 subtests → 24; coverage unchanged at 99% for misc/pcapng.py with its single miss unmoved and no new partial branch.

Changelog on docs/changelog-1.5.0 (#657) is now two commits, both fast-forward, no force:

• dbcd0e9aa — the entry, on top of 33464d923
• 2069ae45c — corrects the counts to 11/24, records the byte-order and options-after-payload coverage, and reattaches six inline literals whose trailing punctuation my first line-wrapper had split off (````PCAPNG.PACKET_TYPES`` ,``` and five like it)

python util/changelog_md.py --check exits 0 at both.

[JarryShaw]

NEEDS CHANGES — cross-review verdict, since resolved. Recorded here because nothing in GitHub tracks a cross-review's result the way a check tracks CI's.

Every agent-raised CR/PR here gets a review from a subagent on a different model, briefed to falsify rather than bless. This PR was authored on Opus 5 and reviewed on Sonnet (no model substitution was needed). It ran read-only, built its own synthetic captures rather than reusing this PR's helpers, and fanned out internally to attack the load-bearing claim from several directions.

It returned NEEDS CHANGES with three findings. All three were real and all three are fixed in c332e1f31.

1. packet as a cached_property reintroduced staleness — real, and fixed

The strongest finding, and a genuine regression I had introduced. unpack now reports the payload through self.packet, and packet was a cached_property, so a second unpack on the same instance returned the first call's octets with get_payload never reached. The pre-#646 code recomputed from the schema on every call, so this was an invariant the old code held and mine quietly dropped.

The reviewer also caught that my own test restructuring had routed around the evidence: I had split the old reused unpacker into unpacker + cached_unpacker to make the test pass, which is exactly the scenario that used to prove reuse-safety, and I did so without saying why. That is a fair hit — the split was made because the assertion failed, and I should have asked why it failed instead of making it pass.

Fixed by making PCAPNG.packet a plain @property. The inherited one caches because it reads the stream, where a second read consumes octets that are gone; mine only walks buffers the schema layer has already filled, so it has nothing to amortise. That removes the invariant rather than restoring a fragile one, and a data descriptor also wins over __dict__, so a stale entry left by the inherited cached_property cannot shadow it either.

The lost coverage is restored: the test is back to swapping the block on one live instance and unpacking again, now asserting get_payload is called on the second block. Demonstrated to bite — with cached_property reinstated, that assertion fails:

E       AssertionError: b'payload' != b'cached'
exit code from file: 1

and with the plain property, exit code from file: 0.

2. The coverage table's protocol.py "before" row was stale — fixed

The reviewer re-measured the base tree with this PR's current tests and got 503 / 228 / 170 / 16 / 49.18% — identical to the "after" row, not the 232 / 17 / 48% I had quoted. Correct: I had compared base-with-old-tests against branch-with-new-tests, which moves the suite and the library at once and then lets you credit the difference to whichever you like. protocol.py's change is docstring-only, so the honest comparison must show no delta, and it does.

Re-measured with the same tests on both trees:

File	Stmts	Miss	Branch	BrPart	Cover
misc/pcapng.py base	1527	1	666	1	99.91%
misc/pcapng.py PR	1545	1	672	1	99.91%
protocols/protocol.py base	503	228	170	16	49.18%
protocols/protocol.py PR	503	228	170	16	49.18%

A related trap found while re-measuring, now noted in the PR body: coverage re-parses the source at report time, so a data file produced before an edit and reported after one reads far worse than reality — misc/pcapng.py appeared to fall to 38% that way.

3. The #678 truncation tally was unreproducible — fixed

The reviewer confirmed the comparative claim (base and PR byte-identical at every level) under three different methodologies, but could not reproduce the specific 96 / 1 / 1 / 3 breakdown, because "101 truncation levels" admits several readings that disagree. Fair. The set is now stated as the expression that produced it:

lengths = [max(1, len(raw) * i // 100) for i in range(101)]   # 1508 octets -> 101 distinct: 1, 15, 30, ... 1508

and compared level by level rather than only in aggregate, since a matching total can hide two levels that swapped. The SHA-256 of the sorted {prefix_length: outcome} mapping is ca44d3ee658087cf2a667c454f839dd931c1c04790b2fe32cee8e1a2e861b182 on both trees.

What it could not overturn

The load-bearing claim — that the payload is now correct for every affected block type, not just the one a fixture happens to have — survived. The reviewer independently derived the three __payload__ declarations and the three offsets (28 / 12 / 28) from the schemas rather than taking them from the PR body, then attacked with from-scratch captures across big-endian sections, EPB and SPB truncation, every mod-4 payload residue, EPB and Packet Block with real trailing options, multiple interfaces, SPB snaplen bounding, and the construction/pack path. Zero counterexamples across all of it.

It also independently built two plausible wrong fixes — hardcoding header offset 28 for every block type, and taking the payload as everything to end-of-buffer including the trailer — and confirmed the current tests catch both. It noted the SPB-offset guarantee rests on a single test method; recorded rather than actioned, and worth knowing.

Claims 1, 2, 4, 9, 10 and 12 reproduced exactly. The house-style sweep came back clean.

Post-fix state

c332e1f31, one commit on top of a18846c8f. tests/protocols + tests/toolkit + tests/dumpkit + the PCAP-NG engine and end-to-end modules: 770 passed, 5 skipped, 1801 subtests passed, exit code 0 read from a file, zero FAILED/SUBFAILED/ERROR. pylint and mypy under the repo's own flag sets remain byte-identical to base by finding category and count.

CI is not claimed green.