Skip to content

fix(engines): write real PCAP traces for dpkt and scapy (#1507) - #1511

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1507-engine-pcap-trace
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1507-engine-pcap-trace

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

Both engines expose the frame bytes, so trace_format='pcap' now writes real PCAP instead of falling back to JSON.

  • Frame shape kept: tcp_traceflow still reports main's packet2dict mapping, as a FrameRecord (a picklable dict). Its packet, frame_info and exact-Decimal time_epoch attributes are read only by the PCAP dumper; fix(dumpkit): scale a PCAP trace record's fraction to the trace's resolution (#1500) #1529's _make_timestamp infers the record's resolution from time_epoch. JSON, plist and tree output is byte-identical to main; the JSON trace of tcp.pcap is pinned by hash.
  • dpkt: the engine's PCAP-NG reader and a dpkt.pcap.Reader subclass yield a Timestamp (float), or a DecimalTimestamp for nanosecond PCAP. Each is dpkt's own value and type, also carrying the exact time, the resolution and orig_len, so fix(engines): the dpkt engine passes a Decimal timestamp for nanosecond PCAP #1501 is untouched; a plain Decimal would spread its mismatch to every reassembly input and flow label. Under a dpkt lacking Reader's private _Reader__f/_Reader__ph/_divisor, the subclass falls back to dpkt's loop with a DPKTWarning naming the version.
  • scapy: the engine reads through PcapReader/PcapNgReader subclasses passed to sniff(opened_socket=…), which stamp each packet with its PCAP or interface resolution, because scapy drops tsresol. A guarded close() in a finally covers a raising sniff without double-closing gzip readers.
  • test(foundation): reassembly, traceflow, engine and registry round trips (#1202) #1508: its Gap(1507) dpkt and scapy flow rows are deleted; the pypcapfile part stays, as pypcapfile still falls back.
  • Tests: PCAP byte-compared with default at both trace_nanosecond settings, on 5 real captures and synthetic ns PCAP and if_tsresol=9 PCAP-NG captures, unaligned and µs-aligned, including the cross-resolution rescale and snapped frames. The new file had 7 failures against 508e3aad0; it now passes 11 tests and 80 subtests.
  • Not this PR, already on main: scapy raises MissingKeyError('IP') on raw-IP (101) PCAP-NG frames. The engines also group different frames into flows on snapped/TSO captures, and scapy itself misdates a second PCAP-NG section.

Refs #1507

@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 b2b57e2a8. Cross-reviewed on Sonnet; the author ran Opus.

Blocking: wrong timestamps on nanosecond PCAP-NG (if_tsresol=9).

  • dpkt: the engine's PCAPNGReader returns float(timestamp) (engines/dpkt.py:171), which drops the sub-µs digits. I measured the first record with trace_nanosecond=True: default and scapy write (1500000000, 774789), but dpkt writes (1500000000, 775) in an ns-magic file.
  • dpkt and scapy both: when a nanosecond timestamp is an exact µs multiple, Decimal keeps no trailing zeros, so the exponent < -6 test reads it as µs. The record then comes out 1000x off: 774 instead of 774000.
  • Needed: an exact timestamp from dpkt's PCAP-NG reader, a resolution check that does not depend on trailing zeros, and a PCAP-NG tsresol=9 test.

Should fix:

Confirmed: byte identity with the default engine on 6 real captures, synthetic µs/ms PCAP-NG and nanosecond classic PCAP; mixed link types on dpkt; fail-before 9 + 30 failures; with the fix, 33 passed and 102 subtests.

@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/1507-engine-pcap-trace branch from b2b57e2 to 508e3aa Compare October 9, 2026 21:35
@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: NEEDS CHANGES at 508e3aad0. This is round 2 of the Sonnet cross-review; the author ran Opus.

Fixed since round 1: traces are now byte-identical with the default engine on all nanosecond PCAP-NG and PCAP cases, both aligned and unaligned. dpkt PCAP-NG now writes orig_len. Timestamp arithmetic returns a plain float, survives copy, deepcopy and pickle, and does not leak into json, plist, tree or xml dumps (md5 identical to main). The scapy readers always close.

Still needs changing:

  • The DPKTWarning about original length fires with no PCAP trace. I measured 7 warnings on tcp.pcap with trace_format='json', and it also fires from follow_tcp_stream. Better: read orig_len from classic PCAP too. dpkt's pcap.Reader parses PktHdr.len and drops it, so a reader subclass, like the PCAP-NG one, removes the limitation. Otherwise, warn only when a PCAP trace is written.
  • dpkt and scapy JSON, plist and tree traces change shape. The top-level timestamp key is replaced by frame_info, time and so on. Keep those formats as they were on main, or state the change in the PR body as user-visible.

Nits: Timestamp needs a default resolution, because statistics.mean calls type(x)(value). The scapy reader is closed twice per run.

Pre-existing: scapy's interface table goes stale after a second Section Header Block (twosec.pcapng). That 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 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 89.99% (unit tier, Python 3.14, 8e8777030, 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 2186 67 710 34 96.24%
pcapkit/dumpkit 258 2 90 2 98.85%
pcapkit/foundation 2737 133 946 45 93.97%
pcapkit/interface 108 7 36 5 91.67%
pcapkit/protocols 17009 229 4574 189 98.04%
pcapkit/toolkit 599 98 178 5 82.11%
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
JarryShaw force-pushed the fix/1507-engine-pcap-trace branch from 508e3aa to d8d631e Compare October 9, 2026 22:11
@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
JarryShaw force-pushed the fix/1507-engine-pcap-trace branch from d8d631e to 0eed63f Compare October 9, 2026 22:39
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO on its own content at d8d631ef8; NEEDS CHANGES to merge. Round 3 was cross-reviewed on Sonnet; the author ran Opus.

What the reviewer confirmed:

Required: #1508 has merged, so its dpkt and scapy Gap(1507) rows would now "come back OK" on this branch. Rebase onto ffca64637 and delete those rows; the pypcapfile part stays.

Hardening in the same push:

  • dpkt is unbounded in pyproject, but the reader reads dpkt's private _Reader__f, _Reader__ph and _divisor. Raise a clear error if any is missing.
  • Close the scapy reader when sniff itself raises.
  • Make FrameRecord picklable under protocols 0 and 1.
  • Correct the fail-before count: 7 failures, not 9.

@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 review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 9, 2026
@JarryShaw JarryShaw added the review: running A cross-review is in flight against the current head - no verdict yet label Oct 9, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

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

  • test(foundation): reassembly, traceflow, engine and registry round trips (#1202) #1508's Gap(1507) rows: the dpkt and scapy rows are deleted, and the pypcapfile row remains, with correct cites (toolkit/pypcapfile.py:620, extraction.py:1439-1443). Flipping that row either way fails as it should.
  • dpkt fallback: with renamed private attributes, the flow files stay byte-identical to the default engine's on 10 PCAPs, both magics and both resolutions, including whole-second timestamps. The only difference is orig_len on a snapped frame, which is the documented loss. One version-naming DPKTWarning is emitted per extraction.
  • Scapy reader: it is closed exactly once on every path, including a sniff that raises RuntimeError or KeyboardInterrupt. No fd leaks.
  • FrameRecord: it now round-trips through pickle protocols 0–5, copy and deepcopy.
  • Real captures: traces are byte-identical, unchanged from round 3.

Nit, not blocking: this PR shifts lines that the Gap(1501) and Gap(1503) rows cite (engines/dpkt.py:309 is now :389, and toolkit/scapy.py:186 is now :258). The fixes for #1501 and #1503 delete those rows anyway.

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

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES. Do not merge as-is. Merged into current main (f96368b43), this PR breaks dpkt and scapy PCAP traces.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 10, 2026
- toolkit/dpkt, toolkit/scapy: tcp_traceflow reports a FrameRecord, the
  engine's packet2dict mapping as before, carrying the captured octets, the
  PCAP record header and the exact time_epoch that only the PCAP trace
  dumper reads.
- engines/dpkt: the PCAP and PCAP-NG readers yield a Timestamp (float) or,
  for a nanosecond PCAP, a DecimalTimestamp: the value dpkt yields, carrying
  the exact timestamp, the resolution and the original length. Under a dpkt
  without the reader internals this relies on, it reads with dpkt's own loop
  and warns.
- engines/scapy: captures are read through PcapReader subclasses that stamp
  each packet with its PCAP or PCAP-NG interface resolution, and close once.
- foundation/traceflow/data: new FrameRecord, a dict with packet, frame_info
  and time_epoch attributes, picklable with every protocol.
- foundation/extraction, interface/misc: dpkt and scapy leave the
  trace_format='pcap' -> JSON fallback; #1508's Gap(1507) keeps pypcapfile.

Traced PCAP files are byte-identical to the default engine's at both trace
resolutions, and JSON, plist and tree output byte-identical to main's.
@JarryShaw
JarryShaw force-pushed the fix/1507-engine-pcap-trace branch from 0eed63f to 8e87770 Compare October 10, 2026 00:09
@JarryShaw JarryShaw removed the review: needs-changes Cross-review at the current head says changes are required; see the verdict comment label Oct 10, 2026
@JarryShaw JarryShaw added the review: running A cross-review is in flight against the current head - no verdict yet label Oct 10, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 8e8777030. This was a Sonnet delta review; the author ran Opus. The head is rebased onto main f96368b43, so the #1529 interaction is resolved.

Nit, not blocking: the Gap(1501) and Gap(1503) line cites are stale (dpkt.py:309→:389, scapy.py:186→:259). The fixes for those issues delete the rows.

@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 2d088a7 into main Oct 10, 2026
40 checks passed
@JarryShaw
JarryShaw deleted the fix/1507-engine-pcap-trace branch October 10, 2026 01:48
@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

Development

Successfully merging this pull request may close these issues.

1 participant