Skip to content

fix(toolkit): pypcapfile TCP reads past VLAN tags, warns of tunnels, skips later fragments and padding (#1537, #1576, #1577) - #1575

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1537-pypcapfile-vlan-tunnel
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1537-pypcapfile-vlan-tunnel

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 makes the pypcapfile engine's TCP agree with the default engine's. All of it is in pcapkit/toolkit/pypcapfile.py.

Tests (3.11 parity venv / 3.14 repo venv):

  • The pypcapfile toolkit and engine files give 76 passed and 59 passed, 17 skipped.
  • On origin/main's toolkit the test file fails 56 times: 11 tests and 45 subtests. Each fix's tests also fail with that fix reverted alone.
  • The pypcapfile agreement rows are unchanged: 207 labels, 0 failures, 92 skips.
  • pcapkit/toolkit/pypcapfile.py coverage goes from 100% to 100% on 3.11 and from 98% to 99% on 3.14.
  • 10k snaplen-cut tunnelled SYNs take 0.13–0.17 s, against 0.10–0.14 s on main.

Closes #1537
Closes #1576
Closes #1577

@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 10, 2026
@JarryShaw
JarryShaw force-pushed the fix/1537-pypcapfile-vlan-tunnel branch from 3d9d509 to 98e8606 Compare October 10, 2026 05:32
@JarryShaw JarryShaw changed the title fix(toolkit): the pypcapfile engine reads VLAN-tagged IPv4 and warns of tunnelled TCP (#1537) fix(toolkit): pypcapfile TCP reads past VLAN tags, warns of tunnels, skips later fragments and padding (#1537, #1576, #1577) Oct 10, 2026
@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 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at 98e8606e4. The changes are small and confined to behaviour this PR introduces. Cross-reviewed on Sonnet; the author ran Opus.

Needed:

  1. Malformed tagged IPv4 is dropped silently. A VLAN-tagged frame with IHL below 5 is dropped without a word, while an untagged one gets the "decoding failed" warning. The _network docstring says the tagged path behaves "as an untagged one is". Make the two paths behave the same.
  2. The tunnel warning can fire when the default engine sees no TCP. It also fires for tunnelled TCP whose header the default parser rejects, for example a Data Offset past the captured data, or malformed options. That was about 0.3% of fuzzed frames. Either tighten the check or correct the _upper_layer docstring, which says it "fires on the frames the default engine finds TCP in".

Confirmed:

Not this PR's: the review also found pypcapfile divergences that main already has. They are being verified, and will be filed separately.

@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.64% (unit tier, Python 3.14, 1de91947b, 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 2792 154 984 42 93.43%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 16988 230 4540 191 98.02%
pcapkit/toolkit 770 96 266 6 86.68%
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 a01fc0d4d. Sonnet delta review; the author ran Opus.

Blocking: the new tunnel check silences warnings process-wide. _default_finds_tcp (toolkit/pypcapfile.py:591-608) wraps a trial dissection in warnings.catch_warnings() + simplefilter('ignore') and sets the pcapkit root logger to CRITICAL+1. I confirmed this in the code. Measured:

  • Dedup broken: a user warning under the default filter reappears after every trial. This is the v1.4.1 registry defect that BaseWarning documents (utilities/warnings.py:146).
  • Other threads' output lost: another thread's warnings.warn and pcapkit log record vanish while a trial runs, and a filter installed mid-trial is wiped on exit.
  • Logger stuck silent: the logger level is read without a lock. In 17 of 30 two-thread rounds it stayed at 51 permanently.
  • Unbounded cost: a candidate the default engine rejects is re-dissected on every frame, and twice per frame. 10k snaplen-cut tunnelled SYNs went from 0.129 s to 9.52 s (74×).

Decision: drop the trial dissection. Detect from the header walk, plus a cheap TCP sanity check (Data Offset ≥ 5 and doff*4 within the captured segment). Say in the docstring that a malformed tunnelled header the default engine would reject can still warn.

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
…skips later fragments and padding (#1537, #1576, #1577)

pcapfile 0.12.0's Ethernet.payload_type decodes EtherType 0x0800 only, and
_transport() read the IPv4 payload to the end of the frame whatever its
fragment offset, so the engine's TCP disagreed with the default engine's.

- _network() reads past 802.1Q and 802.1ad tags, stacked to any depth, and
  decodes the IPv4 behind them with pcapfile's own IP decoder; one that
  will not decode warns as an untagged one does, once per frame (#1537).
- TCP over IPv6 behind tags gets #1535's warn-once; TCP tunnelled in IP
  (4in4, 6in4, 6in6, 4in6) gets one of its own rather than being read
  through, as the default engine keys such a flow by the outer addresses.
  Both follow the default engine's rules off the raw headers: IP lengths,
  Jumbo Payload, later fragments, a Data Offset of at least 5 words, and
  in a tunnel a TCP header that fits; malformed options are not seen
  into (#1537).
- A later IPv4 fragment is never read as a TCP header (#1576).
- The TCP segment ends at the IPv4 Total Length, so Ethernet padding is no
  longer payload; 0 (TSO) keeps the rest of the frame (#1577).
- Docs: the engines table and the toolkit note.

Closes #1537
Closes #1576
Closes #1577
@JarryShaw
JarryShaw force-pushed the fix/1537-pypcapfile-vlan-tunnel branch from a01fc0d to 1de9194 Compare October 10, 2026 07:02
@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 1de91947b. Round 3 was a Sonnet delta review; the author ran Opus. It merges cleanly with #1569.

  • No process-global state left. The diff adds no catch_warnings, simplefilter, setLevel or logger use. I checked that myself.
    • The reviewer's probes reproduce every round-2 defect on a01fc0d4d and none on this head: dedup intact, concurrent warnings and logs all seen, a mid-detection filter kept, and the logger level unchanged in 30 of 30 two-thread rounds.
    • warnings._filters_mutated is called 0 times over 50 detections; the old head called it 150 times.
  • Performance is back to normal. The 10k snaplen-cut tunnelled SYNs take 0.16 s, against 0.13 s on main and 9.4 s on the old head. http.pcap and a 50k-frame capture are flat.
  • The residual is exactly as documented. All 744 warn-without-TCP cases in a malformed-options fuzz are option rejections, and 0 are a capture-cut header. That fuzz covered 10,000 frames, seeds 11–14. No warnings were missed; main drops 617 frames silently on the same fuzz.
  • The IPv6 fit-check skip holds. Directly over IPv6, the default engine still finds TCP through its raw fallback, so warning there is right. Inside tunnels it doesn't, so not warning is right.
  • Fail-before: the new dedup test fails on the old head, and main's toolkit fails 56.

Nit: walk.call_count == 16 pins a private call count, so assertLessEqual would be sturdier.

Filed as #1596 (not this PR's): pypcapfile accepts IPv4 options the default engine rejects. On main, a malformed option length gives 1 TCP datagram on pypcapfile and 0 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
@JarryShaw
JarryShaw merged commit bfb0e99 into main Oct 10, 2026
39 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 10, 2026
@JarryShaw
JarryShaw deleted the fix/1537-pypcapfile-vlan-tunnel branch October 10, 2026 12:19
@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

Labels

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

Projects

Status: Done

1 participant