Skip to content

fix(engines): the pypcapfile engine reads the PCAP records pypcapfile drops (#1512) - #1569

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1512-pypcapfile-big-endian
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1512-pypcapfile-big-endian

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 10, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

  • You will be asked some questions, please read them carefully and answer honestly

  • Put an x into all the boxes [ ] relevant to your pull request (like that [x])

  • Use Preview tab to see how your pull request will actually look like

  • Searched for similar pull requests

  • Followed the coding style (make pylint, make mypy, make isort)

  • make test passes, and a test case covers the change — every module that mentions pypcapfile, plus tests/interface, on 3.14 and 3.11, not the whole suite

  • Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible — N/A — centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

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

Closes #1512. Closes #1571.

  • Root cause (upstream, pypcapfile 0.12.0 savefile.py): _read_a_packet tests hdrp[0].byteorder == 'big' (:214). The field reads back as b'big' (:98), so a big-endian record header is unpacked little-endian (:217) and the file ends at its first record. :221-222 ends the file at any empty or short frame: a zero-length record drops itself and everything after it, and a truncated final record is lost.
  • Fix: pypcapfile still loads the header. The engine now reads the records itself in both byte orders, with the new _read_records, into pypcapfile's own pcap_packet built as _read_a_packet builds it with layers=0. It stops at a tail shorter than 16 octets, and keeps a record that runs past the data with a ProtocolWarning, as Frame.read does. _decode leaves a zero-length frame undecoded and unwarned.
  • Agreement: the frame count and timestamps match default for zero-length middle/first and truncated-final records in both byte orders, and for both big-endian samples. The fix(engines): pypcapfile engine reads zero frames from a big-endian PCAP, without warning #1512 Gap row is deleted; no row cited fix(engines): the pypcapfile engine stops reading at a zero-length PCAP record #1571.
  • Byte-exact: all 4,863 files written for the 15 little-endian samples (json/plist/tree, whole, per-frame and flows) have the same sha256 as main.
  • Tests: in test_pypcapfile_engine.py, stand-ins (3.14 and 3.11) plus a real-pypcapfile check against an independent struct reader (3.11). On main, 12 stand-in subtests fail on 3.14 and 14 real-pypcapfile subtests fail on 3.11. test_core.py's stand-in now consumes the header and carries a ctypes header, as the real one does. Engine coverage goes from 93.53% to 94.34%.

@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
@JarryShaw

Copy link
Copy Markdown
Owner Author

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

Blocking: tests/interface/test_core.py::EngineConstantTests::test_pypcapfile_constant_selects_the_pypcapfile_engine passes on main and fails here. I reproduced it: 1 failed on the PR, 1 passed on main.

  • The error is AttributeError: 'types.SimpleNamespace' object has no attribute 'byteorder', raised at engines/pypcapfile.py:228.
  • The test's FakeSaveFile (test_core.py:59) builds a header without byteorder.
  • A real pypcapfile header always has byteorder, so the fix belongs in the test fake.

Also folded in here: pypcapfile stops at a zero-length record, filed as #1571. I measured it on main: records of 75, 0 and 77 octets give 3 frames on the default engine and 1 on pypcapfile.

Confirmed:

  • Frames: all 17 sample captures, and byte-swapped copies of each, give records byte-identical to an independent struct reader. They match the default engine's bytes, lengths and timestamps, including the nanosecond file.
  • Little-endian output: the 15 little-endian files give output identical to main, down to the sha256 of every written file.
  • Reader: _read_big_endian is pypcapfile's _read_a_packet with '>IIII'.
  • Fail-before: 5 tests fail on 3.14 and 7 on 3.11.
  • Gap table: the PR removes exactly the fix(engines): pypcapfile engine reads zero frames from a big-endian PCAP, without warning #1512 row.

@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
@github-actions

github-actions Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 89.63% (unit tier, Python 3.14, 67d20e993, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18858 957 2366 871 91.03%
pcapkit/corekit 2334 67 746 35 96.43%
pcapkit/dumpkit 275 3 100 3 98.40%
pcapkit/foundation 2808 154 988 42 93.47%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 16991 230 4544 191 98.02%
pcapkit/toolkit 708 97 234 7 85.14%
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.

… drops (#1512)

pypcapfile 0.12.0's _read_a_packet drops records the default engine
reads: it tests `hdrp[0].byteorder == 'big'`, bytes against str, so a
big-endian savefile ends at its first record (#1512); and it ends the
savefile at an empty or short frame, so a zero-length record drops
itself and every later one, and a truncated final record is lost (#1571).

- PyPCAPFile.run: pypcapfile still loads the header; the records are
  read by the new _read_records, never by pypcapfile's generator.
- PyPCAPFile._read_records: reads each record in the header's byte
  order into pypcapfile's own pcap_packet, as _read_a_packet builds it
  with layers=0; stops at a tail shorter than a record header, and keeps
  a record whose capture length runs past the data, with a
  ProtocolWarning, as the default engine does.
- PyPCAPFile._decode: a zero-length frame is left undecoded, unwarned.
- tests: both byte orders, over stand-ins and the real pypcapfile; the
  #1512 Gap row is deleted; test_core's pypcapfile stand-in reads the
  header and carries a ctypes header, as the real one does.
- docs: the engine note and Internal Definitions cover the reader.
@JarryShaw
JarryShaw force-pushed the fix/1512-pypcapfile-big-endian branch from 8722317 to 67d20e9 Compare October 10, 2026 05:31
@JarryShaw JarryShaw changed the title fix(engines): the pypcapfile engine reads the records of a big-endian PCAP itself (#1512) fix(engines): the pypcapfile engine reads the PCAP records pypcapfile drops (#1512) Oct 10, 2026
@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
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 67d20e993. This was a Sonnet cross-review; the author ran Opus. The PR merges cleanly into current main.

  • Little-endian output is unchanged. The 15 little-endian samples, run through json, plist and tree (whole and per-frame) with IPv4/TCP reassembly and tracing, write 5,214 files. All are sha256-identical to main's, and the warnings match. The 8,934 frame records match on timestamp, caplen, wire length and header.
  • It agrees with the default engine in all 80 edge-case captures. These are 20 cases × both byte orders × µs/ns: zero-length first, middle, last and consecutive records, a truncated first or final record, 1–15-octet tails, caplen above snaplen or wire length, and empty files. Frames, timestamps, lengths, bytes and ProtocolWarning text are identical. main diverges in 62 of the 80.
  • _read_records is faithful to pypcapfile. For all 17 samples its packets equal pypcapfile's own pcap_packets, with the header pointer intact. Seekable, peek-less and non-seekable streams give the same frames as a path.
  • test_core still tests engine selection, not a bent stand-in. It passes on 3.14 and 3.11, and I checked that myself.
  • Fail-before is reproduced: 12 failing subtests on 3.14 and 14 on 3.11 with main's engine file.

Not this PR's: the review saw the default engine read only part of a capture from a non-seekable stream. confirmed and filed as #1586.

@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 39f4308 into main Oct 10, 2026
39 checks passed
@JarryShaw
JarryShaw deleted the fix/1512-pypcapfile-big-endian branch October 10, 2026 12:18
@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

1 participant