Skip to content

fix(pcap,pcapng)!: a cut PCAP header or PCAP-NG block rebuilds as captured (#1470) - #1475

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1470-misc-short-read
Oct 9, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1470-misc-short-read

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1470
Depends on #1465, now merged as 15d5eeca2; rebased onto main.

  • Header: calls fix(schema): a header cut inside a fixed-width field rebuilds as captured (#1458, #1451) #1465's keep_short_read/replay_short_read. Header(raw[:4]) rebuilds as 4 octets (was 24).
  • PCAP-NG: a block the capture ends inside keeps its octets as __truncated_raw__, and a rebuild re-parses them. If the block's parser rejects the zero-filled fields, the block is kept as an UnknownBlock. TLS/WireGuard key logs skip a cut last line. Any other malformed line raises FieldValueError (was a bare ValueError).
  • Why __truncated_raw__ rather than __short_read__ for PCAP-NG: the cut lands in a nested block schema, and a full rebuild trimmed afterwards is not byte-exact (it recomputes lengths and options from zero-filled fields), so the captured octets themselves, not a (field, count) record, have to travel with the info.
  • !: a cut block gains __truncated_raw__, and some cut blocks now parse instead of raising.
  • New tests/protocols/misc/test_misc_short_read_1470_runtime.py cuts every PCAP header and every PCAP-NG block type at every offset, then rebuilds each from info and from info.to_dict(). Without the fix, 5320 subtests and 3 tests fail. With it, 5702 subtests pass. Dumps (json and tree) of all 22 untruncated captures are byte-identical.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
@JarryShaw
JarryShaw force-pushed the fix/1470-misc-short-read branch from 32c84bf to f61be1c Compare October 9, 2026 04:26
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at f61be1c22. Sonnet cross-review; the author ran on Opus. The PR stays a draft until #1465, its base, merges.

  • Cut PCAP header: I cut all 16 sample .pcap headers at every keep from 4 to 24 octets (336 cases). Before this commit, 320 cuts failed to rebuild from info and 320 from info.to_dict(); after it, 0 fail.
  • Cut PCAP-NG blocks: every cut keeps its parsed fields and rebuilds byte-exactly via truncated_raw. Across 162 cut extractions, every JSON dump succeeded.
  • UnknownBlock fallback: it only fires when the declared length runs past the captured data. I corrupted one byte in each of about 8,900 whole blocks, and none fell back.
  • FieldValueError: nothing in pcapkit/ or tests/ catches the old type, and FieldValueError subclasses ValueError.
  • Untruncated captures: to_dict() hashes over all 23 captures are identical before and after.
  • Tests bite: I re-ran this myself. The new module passes at this head (7 tests). With the three library files reverted to 540c8887f it gives 5263 failed.

One consequence to know about: a corrupted length field that overshoots the data now reads as a cut instead of raising. That is consistent with #1458.

One design point needs a ruling, posted on #1470: should the visible truncated_raw key become a dunder key?

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 89.55% (unit tier, Python 3.14, f078fbd96, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18795 958 2356 871 90.99%
pcapkit/corekit 2107 93 672 27 94.60%
pcapkit/dumpkit 242 2 88 2 98.79%
pcapkit/foundation 2648 98 930 48 94.80%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 17049 240 4610 193 97.95%
pcapkit/toolkit 539 74 168 3 84.58%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2342 1006 157 43.25%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw JarryShaw added the breaking Breaks public-facing behaviour or API (apply alongside the type label) label Oct 9, 2026
@JarryShaw
JarryShaw force-pushed the fix/1470-misc-short-read branch from f61be1c to cb289c5 Compare October 9, 2026 12:53
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at cb289c53a. Round-2 cross-review on Sonnet; the author ran on Opus. This round covered the #1470 ruling, which renames the key to __truncated_raw__.

  • No bare truncated_raw left. None in pcapkit/, tests/ or docs/, the commit message, or the PR body.
  • The dunder key survives. Neither Data.__excluded__ nor Info.__map__ strips it. 208 of 208 random cuts across every PCAP-NG block type rebuild byte-exactly from both info and info.to_dict(). The json, tree and plist dumps all carry the key.
  • The TYPE_CHECKING annotation has no runtime effect. It mirrors how __short_read__ is declared.
  • Tests bite. With the three library files reverted to 540c8887f, 5323 fail. to_dict() hashes over 1590 untruncated frames are identical before and after. I re-ran the new module at this head: 7 passed.

This stays a draft until #1465, its base, merges.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Still draft?

@JarryShaw
JarryShaw marked this pull request as ready for review October 9, 2026 13:24
@JarryShaw

Copy link
Copy Markdown
Owner Author

No longer a draft. Its base, #1465, merged at 13:13Z, and I've marked this ready for review. It's BEHIND main after today's merges, so I'm rebasing it now. Once that's pushed I'll compare the rebased diff against cb289c53a and re-confirm the verdict.

…tured (#1470)

- PCAP Header calls #1465's keep_short_read/replay_short_read, so a header
  cut short rebuilds from its info as the octets captured, not all 24.
- A PCAP-NG block the capture ends inside keeps its octets as
  __truncated_raw__; a rebuild parses them again instead of writing the block
  at its declared length (which wrote zeros, or raised ProtocolError).
- A cut block whose own parser refuses the zero-filled fields is kept as
  an UnknownBlock rather than raising.
- TLS and WireGuard key logs skip a last line the capture cut, and raise
  FieldValueError for any other malformed line (was a bare ValueError).

Adds tests/protocols/misc/test_misc_short_read_1470_runtime.py.
@JarryShaw
JarryShaw force-pushed the fix/1470-misc-short-read branch from cb289c5 to f078fbd Compare October 9, 2026 13:28
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict carried to f078fbd96: GOOD TO GO. This is a pure rebase onto main (6323852a7). git range-diff shows the commit as =, and its stable patch-id c420cb3b9547 is the same as cb289c53a's, so the round-2 review still applies. On the rebased tree the worker's runs pass: the new module (7 tests, 5702 subtests), the misc leg (385 tests), tests/project (419 passed) and test_capture_roundtrip_runtime.py (4 passed, 20501 subtests). Ready once CI finishes.

@JarryShaw
JarryShaw merged commit bbfd4bf into main Oct 9, 2026
36 checks passed
@JarryShaw
JarryShaw deleted the fix/1470-misc-short-read branch October 9, 2026 14:01
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(pcap,pcapng): a cut PCAP header or PCAP-NG block rebuilds at full width

1 participant