Skip to content

fix(foundation): read every frame from a pipe or non-seekable stream (#1586) - #1595

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1586-non-seekable-stream-frames
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1586-non-seekable-stream-frames

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 10, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

What is the purpose of your pull request?

Tick the commit type your subject line carries.

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • release — bumps the version or rolls up a distribution
  • chore — anything else

Description of your pull request and other information

Root cause (lines on e9f3340f7), all in SeekableReader, which wraps every non-seekable fin:

  • pcapkit/corekit/io.py:351 — seek(0, SEEK_END) was the buffer's far edge. Once the 131,072-octet buffer fills, that edge is exactly how far the stream has been read, so Frame._check_record_floor (frame.py:487) measures 0 left after the first record ending past it: frame 870 of http.pcap. Now _read_ahead reads past the position, or to the real end.
  • io.py:522, io.py:538 — read(n) made one stream read, short on a pipe or raw stream (7-octet chunks gave 0 frames). io.py:386-387 — a forward seek did the same, and could overshoot. Both now go through _fill.
  • extraction.py:1391, engines/pcapng.py:372 — a short peek(4) failed the magic number, and skipped the packet-block-before-IDB check. Both fall back to read-and-seek-back.

Read-ahead covers exactly the next record, so a live pcapkit - is not held back: a PCAP record header and its incl_len; a PCAP-NG block's Total Length (BOM byte order for an SHB). The buffer grows to hold a record longer than itself, so an oversize EPB, SHB or PCAP record is read whole; a bogus length reads in 1 MiB pieces to the real end, as a file read does. A block shorter than its fixed fields, or not word-aligned, keeps the buffer's worth, and a DSB extends to its Secrets Length: both are parsed past their end from a file too. Live pipe, 100 frames 50 ms apart: PCAP 100 before close (median 0.4 ms), PCAP-NG 100 (0.6 ms); main 50 and 48.

Also: flush() skips an already-closed stream (the dpkt NeedData "Exception ignored" noise); positions before a read-ahead SEEK_END are documented as possibly gone; test_io.py's seek(-1, SEEK_END) pin moves 3→4.

Tests: 24, all failing on e9f3340f7: pipe/raw/buffered at 1/7/4096/65536 octets × no_eof 0/1, PCAP and two-section PCAP-NG, default and dpkt (scapy refuses streams); truncation; malformed and oversize records, including dpkt's seek back over an oversize SHB; live PCAP and PCAP-NG; CLI on stdin, PCAP→PCAP and PCAP-NG→JSON. Frames and warnings match the path read.

Selections run (not the whole suite): the two new test files 11 and 13 passed; tests/corekit/test_io.py 34; other test_extraction*.py and test_io*.py 98; tests/foundation/engines 142 passed, 840 skipped; tests/protocols/misc, tests/cli, test_cli_subprocess.py 250. Coverage: io.py 100%, engines/pcapng.py 99%, as before.

Closes #1586

@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 10, 2026
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 10, 2026
@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 89.66% (unit tier, Python 3.14, 4bb654921, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 3 20 3 95.16%
pcapkit/const 18858 957 2366 871 91.03%
pcapkit/corekit 2366 67 760 35 96.48%
pcapkit/dumpkit 275 3 100 3 98.40%
pcapkit/foundation 2841 154 1002 42 93.55%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 16988 227 4540 192 98.04%
pcapkit/toolkit 708 96 234 6 85.35%
pcapkit/utilities 431 4 124 4 98.56%
pcapkit/vendor 4409 2342 1006 157 43.25%

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

@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 912118bb2. Cross-reviewed on Sonnet; the author ran Opus.

Blocking: a live PCAP-NG pipe now stalls. seek(0, SEEK_END) reads ahead a full buffer (131,072 octets), so a slow PCAP-NG writer's frames are held until that much has arrived or the pipe closes.

  • In process: 100 frames sent 50 ms apart came out as 0 frames before the pipe closed. On main it was 48.
  • Via pcapkit -: 20 frames, pipe held open 6 s: no output file at all until close. On main, 7 frames.

Fix: limit the PCAP-NG read-ahead to the block's declared length, capped at the buffer. _check_floor/_captured (pcapng.py:1543, :1562) only need to know whether the whole block is present. Then add a live PCAP-NG pipe test, and a PCAP-NG→JSON case to the stdin CLI test.

Confirmed:

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 10, 2026
@JarryShaw
JarryShaw force-pushed the fix/1586-non-seekable-stream-frames branch from 912118b to 12192f5 Compare October 10, 2026 08:18
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 10, 2026
Comment thread tests/foundation/test_extraction_non_seekable_stream_1586_unit.py Fixed
@JarryShaw
JarryShaw force-pushed the fix/1586-non-seekable-stream-frames branch from 12192f5 to 35117e1 Compare October 10, 2026 08:32
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 35117e105. Round 2 was a Sonnet review of 12192f5fc. The only change in 35117e105 is the 2-line secrets→dsb_block rename, which does not affect the verdict.

Confirmed:

  • Byte-identical reads. The harness covered 26 examples/captures files plus multi-section, mixed-byte-order and DSB PCAP-NG fixtures, over 12 stream types with no_eof on and off. Frames, repr(info), warnings and exceptions match the path read on default and dpkt.
  • Live pipe. 100 frames written 50 ms apart all arrive before close for PCAP and PCAP-NG, median 0.4–0.6 ms. That holds even with an SHB written 1 octet at a time.

Required: a block or record larger than the 131,072-octet buffer still differs from a path read. I measured this on 35117e105 with three frames of 1,000, 140,000 and 500 octets on the default engine:

  • PCAP-NG EPB: the path read gives 140,000 octets. The pipe gives 131,044 octets, plus block length mismatch: 140032 != 0 and captured length 140000 runs past the block.
  • PCAP (snaplen 262144): the path read gives all three frames. The pipe raises SeekError('cannot seek before the beginning of the buffer: 1040 < 9984').

main also fails here (it drops the third PCAP-NG frame), so this is not a regression. It is still #1586's defect, and the description's claim that frames match the path read does not hold for it. The read-ahead already extends a DSB to its Secrets Length; extending it to the block's Total Length (PCAP-NG) and to incl_len (PCAP) should close the gap. Please add a test for each.

Round 3 will also cover what round 2 did not reach: malformed blocks other than oversize ones, fail-before on e9f3340f7, and the pcapkit - CLI on a live pipe.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 10, 2026
…1586)

- SeekableReader.seek(0, SEEK_END) answered with the buffer's far edge, which
  once the buffer has filled is how far the stream has been read; the record
  after the one that filled it measured nothing left, so a pipe ended at frame
  870 of 1,117. It now reads ahead past the position, or to the real end.
- SeekableReader.read and a forward seek took one short read for all there
  was; both now read until the octets asked for arrive or the stream ends, and
  a forward seek lands on its target rather than wherever the read stopped.
- Extractor and the PCAP-NG engine read the magic number and block type again
  when peek() comes back short.
- The engines read ahead exactly the record about to be read -- a PCAP record
  by its incl_len, a PCAP-NG block by its Total Length -- and the buffer grows
  to hold one longer than itself, so a live capture is not held back and an
  oversize record is read whole.
- SeekableReader.flush skips a stream its owner has already closed.

Tests: a pipe and a short-read raw stream at 1, 7, 4096 and 65536 octets,
PCAP and PCAP-NG, default and dpkt, no_eof, truncation, malformed and
oversize records, live pipes, the CLI on stdin; all fail on e9f3340.
@JarryShaw
JarryShaw force-pushed the fix/1586-non-seekable-stream-frames branch from 35117e1 to 4bb6549 Compare October 10, 2026 09:24
@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 4bb654921. Round 3 was a Sonnet review; the author ran on Opus. I re-ran the checks it did not reach.

  • Oversize records now match a path read (my measurement). These are identical through a pipe and by path, on default and dpkt: frames, lengths and a payload hash. Cases were PCAP-NG EPBs of 140,000 and 300,000 octets, a ~150 KB SHB, and a 140,000-octet PCAP record.
  • Malformed blocks (Sonnet), 23 cases. Cases: PCAP-NG Total Length 0/8/11, not ×4, or below the block minimum; mismatched trailing length; 0xFFFFFFF0; past the stream; unknown BOM; huge DSB Secrets Length; truncations; PCAP incl_len above snaplen, above the stream, 0xFFFFFFF0 or 0. Frames, warnings and exceptions match the path read with 65,536- and 7-octet pipe writes, on default and dpkt.
  • Memory (Sonnet). 20,000 records with one 300,000-octet record grow the buffer to 300,016 and hold it there, with RSS +3 MB. A bogus 0xFFFFFFF0 length in a 16 MB stream reads to the real end at +48–62 MB peak.
  • Fail-before (my run, PR tests on e9f3340f7 sources). All 11 functions in test_extraction_non_seekable_stream_1586_unit.py fail, as do all 13 in test_io_seekable_reader_short_reads_1586_unit.py. That includes the live-pipe and CLI-on-stdin tests.
  • Narrow runs on the head (mine).
    • non-seekable: 11 passed, 67 subtests
    • short reads: 13 passed
    • test_io.py: 34 passed, 41 subtests
  • Conflicts. merge-tree is clean against main and against each of the other 7 open PRs.

Non-blocking. The grown buffer never shrinks, so it is bounded by the largest record seen. A bogus 32-bit length on a never-ending live pipe can buffer up to ~4 GiB before it errors, where a file read stops at the file's end.

@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 10, 2026
@JarryShaw
JarryShaw merged commit 4ee525e into main Oct 10, 2026
41 checks passed
@JarryShaw
JarryShaw deleted the fix/1586-non-seekable-stream-frames branch October 10, 2026 12:24
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(foundation): extracting from a pipe or non-seekable stream silently stops early (870 of 1,117 frames)

2 participants