Skip to content

fix(toolkit): the pypcapfile engine warns that it leaves TCP over IPv6 out (#1513) - #1535

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1513-pypcapfile-ipv6-tcp
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1513-pypcapfile-ipv6-tcp

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 9, 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

pypcapfile 0.12.0 has no IPv6 decoder (Ethernet.payload_type maps only 0x0800), so TCP over IPv6 cannot be handled. Instead, tcp_reassembly and tcp_traceflow now raise an AttributeWarning once per capture, on the first frame carrying TCP over IPv6. That covers TCP directly and TCP behind the headers the default engine steps over: RFC 8200's (except ESP), plus Mobility, HIP and Shim6. A first fragment is read as if unfragmented and a later one never counts, as the default engine does since #1539 and #1545. Detection matches default on all 1,495 sample frames and on 10,000 fuzzed chains, with 0 mismatches. Captures are keyed by their savefile header's address, and the last 32 warned about are remembered.

Boundary: VLAN-tagged, QinQ and tunnelled frames are not looked into, for IPv6 and IPv4 alike, so TCP inside them is still left out without a warning.

Gap(1513) is gone. Where pypcapfile warns, its tcp/trace inputs and TCP datagrams are compared with default's over IPv4 only, and flows skip. The engine docstring and the engines docs page now say so.

  • Parity venv (3.11 + pypcapfile): engine agreement 3 passed, 621 skipped, 621 subtests passed; test_pypcapfile_unit.py 35 passed; tests/foundation/engines 130 passed, 627 skipped.
  • Repo venv: tests/foundation/engines 124 passed, 840 skipped; tests/toolkit 127 passed, 10 skipped.
  • The new tests fail on origin/main's toolkit, which issues no warning.

Closes #1513

@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 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

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

Confirmed:

  • The warn route is correct. pcapfile 0.12.0's Ethernet.payload_type maps only 0x0800; its IPv6 branch is commented out (as 0x08DD), and pcapfile.protocols.network.ipv6 does not exist.
  • Detection matches the default engine on all 1,489 frames of the 17 PCAP samples: 47 IPv6 TCP frames, 0 mismatches. A 4,000-chain differential fuzz found no case where the default engine sees TCP and this PR stays silent.
  • The narrowed harness still catches a dropped IPv4 TCP frame, and it narrows only when the warning is actually raised.
  • Fail-before is 11.

Needs changing:

  • A false claim at toolkit/pypcapfile.py:88-93, in the test label 'hip, which carries no payload' and in the PR body. The default engine does dissect TCP after Mobility, HIP and Shim6 (for example Ethernet:IPv6:MH:TCP:Raw). Either walk those headers like the default engine does, or reword the claim to "RFC-mandated payload 59".
  • Stale docs. The PyPCAPFile row at docs/source/pcapkit/foundation/engines/index.rst:92-96 and the engine docstring at engines/pypcapfile.py:66-77 still list only the IPv6 reassembly refusal.
  • State the boundary. 802.1Q/QinQ-tagged and tunnelled frames are still dropped without a warning, for IPv4 too. That behaviour predates this PR and is 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 9, 2026
@JarryShaw
JarryShaw force-pushed the fix/1513-pypcapfile-ipv6-tcp branch from 471e395 to 3989f9e Compare October 9, 2026 23:27
@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 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 3989f9e13. This was a Sonnet delta review; the author ran Opus.

  • Matches the default engine. On a 4,000-chain differential there is no case where the default engine sees TCP and this PR stays silent. All 86 mismatches go the documented safe way: the PR warns and the default engine is silent, always behind a Fragment header. A targeted Fragment-first set gives 102 mismatches, all in that same direction. The 17 sample captures agree on all 47 IPv6 TCP frames.
  • Mobility, HIP and Shim6 now match the default engine.
  • The 32-capture memory works. It warns exactly once per capture for up to 32 interleaved captures, is bounded at 32, and gives 1 warning per capture under 6 threads.
  • The docs grid table parses.
  • Fail-before: 7 new failures on 471e39541, 17 on main. The two mutations tried are both caught.

Non-blocking: the memory is FIFO, so strict round-robin over 33 or more captures warns once per frame. It is never silent. The default engine's own Fragment-header inconsistency is filed separately.

@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 10, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 90.03% (unit tier, Python 3.14, da66f152e, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18795 842 2356 756 92.09%
pcapkit/corekit 2240 67 722 34 96.32%
pcapkit/dumpkit 258 2 90 2 98.85%
pcapkit/foundation 2766 129 980 47 94.23%
pcapkit/interface 108 7 36 5 91.67%
pcapkit/protocols 16996 230 4548 191 98.02%
pcapkit/toolkit 685 98 220 5 84.64%
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

Resolve conflicts

@JarryShaw

Copy link
Copy Markdown
Owner Author

On it. The worker is rebasing onto main (c3d412015). The conflict is in the Gap table of tests/foundation/engines/test_engine_agreement_runtime.py, where #1511 and #1527 edited the neighbouring rows. The rebased head gets a delta review.

@JarryShaw
JarryShaw force-pushed the fix/1513-pypcapfile-ipv6-tcp branch from 3989f9e to d60a162 Compare October 10, 2026 02:07
@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 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at d60a162be. This was a Sonnet delta review; the author ran Opus.

  • Rebase only: comparing the PR's own diff before and after the rebase shows only the two conflict resolutions, the _ipv6_carries_tcp docstring narrowing and the three unit-case changes. The toolkit code is unchanged, and against main the agreement test loses only Gap(1513).
  • Over-reports only: I compared the check against 'TCP' in frame, the default engine's own test (toolkit/pcap.py:164), with an independent fuzz of 15,000 frames on new seeds. There were 0 under-reports and 18 over-reports, all behind a later fragment. All 247 Ethernet/IPv6 sample frames agree. Once fix(protocols): a non-first IPv6 fragment's data is dissected as an upper-layer header #1545 lands, the later-fragment case becomes a pure over-report; no test breaks.
  • Merge-check: merged into 16fbea054, test_pypcapfile_unit.py gives 25 passed and 10 skipped. With main's toolkit/pypcapfile.py swapped in, 16 tests fail. I ran both myself. In the 3.11 venv the reviewer measured 18 failed / 31 passed, which matches the claim.

Not blocking:

@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

Copy link
Copy Markdown
Owner Author

resolve conflicts

@JarryShaw

Copy link
Copy Markdown
Owner Author

On it. #1531 merged, so the worker is rebasing onto main. The conflict is in the Gap table of test_engine_agreement_runtime.py again. The rebased head gets a delta review.

@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 10, 2026
…6 out (#1513)

pypcapfile 0.12.0 has no IPv6 decoder: Ethernet.payload_type maps only
0x0800, so an IPv6 frame's payload stays hex-encoded and _network()
returns None. tcp_reassembly and tcp_traceflow dropped those frames
without a word.

- Both adapters now warn (AttributeWarning) on the first frame of a
  capture that carries TCP over IPv6, directly or behind the extension
  headers the default engine steps over (RFC 8200's less ESP, plus
  Mobility, HIP and Shim6), once per capture rather than per frame.
- Captures are keyed by the address of their savefile header, each
  pinned by an anchor; the last 32 warned about are remembered.
- The toolkit, engine docstring and engines docs page state the loss;
  VLAN, QinQ and tunnelled frames stay out of reach, without a warning.
- test_engine_agreement_runtime: the Gap(1513) row is gone. Where the
  engine warns, its tcp/trace inputs and TCP datagrams are compared
  with default's over IPv4 only, and its flows skip.

Closes #1513
@JarryShaw
JarryShaw force-pushed the fix/1513-pypcapfile-ipv6-tcp branch from d60a162 to da66f15 Compare October 10, 2026 03:43
@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: GOOD TO GO at da66f152e. This was a Sonnet delta review; the author ran Opus.

  • Gap table: against main, the only change is the removal of Gap(1513). Every other row matches main.
  • Semantic change after fix(protocols): a later IPv6 fragment's data is raw, not an upper-layer header (#1545) #1552: I compared the check with the default engine's own test ('TCP' in frame) over 56,000 independently fuzzed frames on new seeds. There were 0 under-reports: the check never misses TCP the default engine sees. The 1,489 sample frames give 0 mismatches either way. Later, first, atomic and out-of-order fragments all agree with the default engine.
  • Over-reports: the check warns on 4–7% of the fuzzed frames whose TCP header is malformed, such as a data offset below 5 or a zero-length option. That is an extra warning, never a missed one.
  • Fail-before: against d60a162be's toolkit, exactly the 3 new later-fragment cases fail. Against main's toolkit, 16 fail.
  • Tests: on 3.11 the pypcapfile tests give 35 passed. In the repo venv, tests/toolkit and tests/foundation/engines are green.
  • With fix(toolkit): trace and reassemble a TCP segment whose header the snapshot cut (#1518) #1556: merged together, everything passes and there are still 0 under-reports.

Pre-existing, not blocking: VLAN, QinQ and IPv6-in-IPv6 TCP are still under-reported, as the docstring says (#1537).

@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 ea883a6 into main Oct 10, 2026
39 checks passed
@JarryShaw
JarryShaw deleted the fix/1513-pypcapfile-ipv6-tcp branch October 10, 2026 04:37
@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 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 (#1537).
- TCP over IPv6 behind tags gets #1535's warn-once; TCP tunnelled in IP
  (4in4, 6in4, 6in6, 4in6) gets a warn-once of its own rather than being
  read through, as the default engine keys such a flow by the outer
  header's addresses (#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 added a commit that referenced this pull request 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.
  Either fires only where the default engine's own dissection of the
  rebuilt frame finds the TCP (#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 added a commit that referenced this pull request 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 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

Development

Successfully merging this pull request may close these issues.

fix(toolkit): pypcapfile engine silently leaves TCP over IPv6 out of reassembly and tracing

1 participant