Skip to content

fix(toolkit): pypcapfile checks IPv4 and TCP headers as the default engine does (#1590, #1591, #1592, #1593, #1596, #1597) - #1600

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1590-pypcapfile-header-checks
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1590-pypcapfile-header-checks

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?

  • fix — corrects a defect

Description of your pull request and other information

This follows #1575. The pypcapfile toolkit now reads these headers as the default engine does. All the changes are in pcapkit/toolkit/pypcapfile.py.

Trade-off. Judging options with the default engine's parsers costs about 200 µs per frame that has them. 10k Router Alert frames take 2.61 s, against 0.49 s before. Each frame is parsed once across the three adapters, and frames without options are never parsed. A header the default parsers reject logs the same CRITICAL ProtocolError the default engine logs, once per distinct header since results are cached. A header rejected on length alone (Data Offset below 5, IHL past the packet, AH Payload Length 0, truncated TCP) logs nothing, where the default engine logs one. The warnings filters and the logger level are left as they were.

Tests (3.11 parity venv / 3.14 repo venv):

  • The pypcapfile toolkit files give 75 passed on 3.11 and 51 passed, 24 skipped on 3.14.
  • Reverting each fix on its own fails that fix's tests.
  • The agreement table has the same 207 pypcapfile outcomes as on main.
  • Coverage of pypcapfile.py stays at 100% on 3.11 and goes from 98.5% to 98.9% on 3.14.
  • A fuzz of 3 × 2,500 frames (options, IHL, Data Offset, AH, extension-header chains, 4in6) shows no input differences. Tunnelled TCP is excluded, since it is left out on purpose.

Closes #1590
Closes #1591
Closes #1592
Closes #1593
Closes #1596
Closes #1597

…ngine does (#1590, #1591, #1592, #1593, #1596, #1597)

pcapfile 0.12.0 slices an IPv4 header at its IHL without a bounds check,
never parses IPv4 options, and has no IPv6 or AH decoder, and the toolkit
read a TCP Data Offset below 5 as 20 octets, so its inputs disagreed with
the default engine's.

- A TCP Data Offset below 5 words is no TCP (#1590).
- An IPv4 header whose IHL runs past the packet is no IPv4 (#1591).
- IPv4 options are judged by the default engine's own IPv4 parser, given
  the header alone and once per frame; a header it rejects is no IPv4 and
  no TCP (#1596).
- ipv4_reassembly() finds IPv4 carried in IPv6, as the default engine
  takes a frame's first IPv4 layer; TCP inside it is still a tunnel (#1592).
- TCP behind IPv4 AH headers is read past them (#1593), and behind the
  IPv6 extension headers the default engine dissects inside IPv4, each
  read by its own parser (#1597). There the TCP header must be one the
  default TCP parser accepts; a tunnel behind them warns.

Closes #1590
Closes #1591
Closes #1592
Closes #1593
Closes #1596
Closes #1597
@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 JarryShaw moved this to In review in PyPCAPKit Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at a532960cc. This was a Sonnet cross-review; the author ran on Opus.

  • Repros: all six reproduce on main and agree on the head, across 62 crafted frames. A fuzz of 5 seeds × 2,500 frames gives 0 differences, against 592 on main. Option-kind sweeps over 2,560 frames give identical inputs, warnings and logs.
  • No global state changes: warnings.filters, showwarning and the logger levels and handlers are unchanged after an extract. simplefilter('error') is honoured as on the default engine.
  • Dispatch: the walk covers 0, 43, 44, 51, 60, 135 and 139, which matches Internet.__proto__. ESP and Shim6 stop it.
  • Fail-before: the new tests fail on main. Narrow runs pass on 3.11 and 3.14, and the agreement module's outcomes are identical to main's.
  • Cost: frames without options take the same time. A frame with options costs about 170–290 µs.

Non-blocking:

  • PR body corrected. Headers rejected on length alone (Data Offset below 5, IHL past the packet, AH Payload Length 0, truncated TCP) log no CRITICAL, where the default engine logs one. Repeated identical headers log once, because results are cached.
  • Long extension-header chains. With roughly 80 or more extension headers chained, the default engine finds no TCP: on 3.14, 80 still parses and 100 does not. pypcapfile walks iteratively and does find it. The divergence is the default engine's, filed as fix(protocols): a chain of ~80+ IPv6 extension headers exhausts recursion and leaves the payload undissected #1604.
  • Cost of long chains. Each header costs one parse, so 3 frames of 7,990 headers take 3.2 s on pypcapfile, against 0.04 s on the default engine.

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

Copy link
Copy Markdown
Contributor

Coverage: 89.72% (unit tier, Python 3.14, a532960cc, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 3 20 3 95.16%
pcapkit/const 18858 956 2366 870 91.04%
pcapkit/corekit 2366 67 760 35 96.48%
pcapkit/dumpkit 275 3 100 3 98.40%
pcapkit/foundation 2855 154 1004 42 93.57%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 16996 227 4540 192 98.04%
pcapkit/toolkit 843 96 296 6 87.88%
pcapkit/utilities 494 4 146 4 98.75%
pcapkit/vendor 4409 2342 1006 157 43.25%

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

@JarryShaw
JarryShaw merged commit 6a9548b into main Oct 10, 2026
40 checks passed
@JarryShaw
JarryShaw deleted the fix/1590-pypcapfile-header-checks branch October 10, 2026 14:49
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 10, 2026
JarryShaw added a commit that referenced this pull request Oct 14, 2026
…1721)

Third-party engines are best effort (#1687), so the pypcapfile code that
exists only to match the default engine comes out:

- revert #1682, #1600, #1575 and #1535, and the pypcapfile hunks of #1655,
  #1608 and #1605: reading loopback and raw-IP link types, the frame layer
  bound, the VLAN and tunnel walks, keying tunnelled TCP by the innermost
  header, stepping over AH and extension headers, and checking options
  with the default engine's parsers
- keep the helpers the dpkt and scapy adapters import, unchanged, until
  their own reverts drop them; their docstrings no longer say pypcapfile
  asks the default engine's parsers too
- re-land #1576, #1577, #1590 and #1591 as guards in _network and
  _transport, #1513's once-per-capture warning for TCP straight over
  IPv6, and #1667's packet2dict fix: an undecoded frame is reported as
  its octets, not as hex text twice as long
- keep #1569's record reader and #1690's packet2timestamp
- delete the three test modules those reverts added, the pypcapfile
  timestamp agreement class, and #1682's link-type warning test; the
  re-landed guards' tests assert constants
- drop what the reverts removed from the docs, READ_LINKTYPES included,
  and say that TCP behind a VLAN tag, in a tunnel or behind an
  extension header (an IPv6 one, or AH in IPv4) is now left out
  without a warning

The pypcapfile modules, test_new_engine_parity_runtime.py,
test_engine_runtime.py, the timestamp module and the project guards pass
on 3.11 (pypcapfile 0.12.0) and 3.14.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment