Skip to content

fix(toolkit): trace and reassemble a TCP segment whose header the snapshot cut (#1518) - #1556

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1518-snapped-tcp-trace
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1518-snapped-tcp-trace

Conversation

@JarryShaw

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) — on the two changed modules: mypy and isort clean, pylint adds no message

  • make test passes, and a test case covers the change — new tests cover it; narrow selections run locally, the full suite is left to CI

  • 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 #1518 (root cause A; B is #1547).

On tcp.pcap snapped by 4 octets, the cut lands inside four TCP headers. The parser rejects them by design (#1404): frame 1 at tcp.py:518, frames 2, 4 and 7 at collections.py:511. TCP becomes Raw, so the default engine's PCAP and PCAP-NG adapters skipped those frames and traced (3,5) (6,). dpkt, scapy and Wireshark (tcp.stream, re-checked) trace (1,2,6,7) (3,4,5).

A shared _tcp_segment helper in pcapkit/toolkit/pcap.py now feeds all four TCP adapters, tcp_traceflow and tcp_reassembly in both modules. It returns the parsed TCP layer when there is one. Otherwise it reads the fixed 20-octet header of a Raw IP payload, if all of these hold: TCP rejected it (Raw.info.error is set), the protocol is TCP, the datagram is not a later fragment, at least 20 octets were captured, and the Data Offset is at least 5. Header and payload are the captured octets on either side of the Data Offset.

Reassembly used the same 'TCP' in frame gate, so it gets the same fix. Its datagrams now match dpkt's: (2,6) (3,) (4,5).

Tests:

  • tests/foundation/traceflow/test_traceflow_snapped_capture_runtime.py builds the snapped capture as PCAP and PCAP-NG. It checks bidirectional and unidirectional flows, agreement with dpkt and scapy, trace PCAPs byte-equal to the captured records and to dpkt's, and reassembly. All 20 subtests fail on main.
  • tests/toolkit/test_pcap_tcp_segment_unit.py uses frames built by hand. It covers the decoded cases (these fail on main) and pins each gate; dropping any one gate fails its test.

No Gap row cites #1518.

@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: NEEDS CHANGES at e63079a60. Cross-reviewed on Sonnet; the author ran Opus. The fix is correct, but one of its gates is untested.

Confirmed:

To fix:

  1. The protocol gate is untested. I replaced ip_info.protocol != Enum_TransType.TCP (toolkit/pcap.py:112) with False, and tests/toolkit plus tests/foundation/traceflow still give 163 passed. Without that gate, a truncated IPv4-in-IPv4 payload (proto=4, 30 octets, byte 12 = 0x50) traces as TCP, which dpkt and scapy do not do. Add a test for that case.
  2. The RST bit is untested. The tests only use flags 0x12, so mutating RST decoding survives. Add a case with RST set.

Not blocking:

  • test_engine_agreement_runtime.py:359 cites pcap.py:73, which is now docstring text; the line is now 183. Fix it after the Gap-table PRs land, to avoid another conflict there.

@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
…pshot cut (#1518)

The TCP parser rejects a header whose Data Offset runs past the captured
octets (#1404), so on a capture snapped inside the TCP header the segment
is Raw and the default engine's PCAP and PCAP-NG adapters skipped it:
tcp.pcap snapped by 4 octets traced (3,5) (6,) where dpkt, scapy and
Wireshark trace (1,2,6,7) (3,4,5).

- pcapkit/toolkit/pcap.py: a shared _tcp_segment helper reports the parsed
  TCP layer, or else reads the fixed 20-octet header of a Raw IP payload
  the TCP parser rejected (protocol TCP, not a later fragment, at least 20
  octets, Data Offset >= 5), with the captured octets as header/payload.
- tcp_reassembly and tcp_traceflow in pcap.py and pcapng.py use it.
- tests: the snapped tcp.pcap as PCAP and PCAP-NG traces, reassembles and
  writes trace PCAPs as dpkt/scapy do; unit cases pin each gate.
@JarryShaw
JarryShaw force-pushed the fix/1518-snapped-tcp-trace branch from e63079a to a1d732a Compare October 10, 2026 03:07
@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 a1d732a68. This was a Sonnet delta review; the author ran Opus. Both round-1 gaps are closed, and the delta touches only tests/toolkit/test_pcap_tcp_segment_unit.py.

  • Protocol gate: the new IPv4 in IPv4 case (proto=4, a 30-octet payload, byte 12 = 0x50) asserts that the payload is neither traced nor reassembled. Changing pcap.py:112's gate to or False now fails it in both containers. I ran this myself: 4 passed with the gate, 2 failed without it.
  • RST: the new cases with flags 0x14 and 0x15 fail under rst=False or under a wrong mask. Flipping any single one of the four rst=segment.rst consumers (pcap.py:287, :327, pcapng.py:183, :225) fails only its own container, so the trace and reassembly checks are independent.
  • Tests: tests/toolkit gives 125 passed and 9 skipped; tests/foundation/traceflow gives 38 passed. Round 1's behaviour checks still hold, since the production code is unchanged.

@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: 90.02% (unit tier, Python 3.14, a1d732a68, 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 2204 67 712 34 96.26%
pcapkit/dumpkit 258 2 90 2 98.85%
pcapkit/foundation 2797 132 986 48 94.18%
pcapkit/interface 108 7 36 5 91.67%
pcapkit/protocols 16992 229 4546 190 98.03%
pcapkit/toolkit 647 99 210 6 83.55%
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.

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(engines): TCP trace flows disagree between engines on snapped frames and on IPv4 total_length=0

1 participant