Skip to content

fix(toolkit): dpkt reassembly adapters build their data from dpkt's re-serialised bytes and mutate the packet - #1335

Merged
JarryShaw merged 1 commit into
mainfrom
fix/dpkt-reassembly-rawbytes
Oct 7, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/dpkt-reassembly-rawbytes

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

  • Searched for similar pull requests
  • Followed the coding style — isort -l100 -ppcapkit clean; mypy clean; pylint 45 findings before and after, all pre-existing in engines/dpkt.py, none in toolkit/dpkt.py
  • make test passes, and a test case covers the change (modules listed below)
  • Added a changelog entry — N/A, added centrally

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1297

The DPKT engine attaches each record's octets to the frame beside its timestamp (new attach_buffer/packet2bytes). ipv4_reassembly, ipv6_reassembly, tcp_reassembly, tcp_traceflow and packet2dict slice the layer out of those octets instead of calling .pack(). A hand-built packet without a buffer is serialised from a deep copy, so it is not mutated, but its bytes are still dpkt's re-serialisation — pinned by a test.

The offset comes from the advance dpkt's own unpack() made, not from len(): IP6.__len__ sums len() of the extension headers, which overshoots by the rest of the frame for an Authentication header and falls 8 octets short for a Routing header with an odd Hdr Ext Len. The slice is then checked against the layer's own pack_hdr() — exact, since dpkt recomputes nothing there — and a ProtocolError raised rather than wrong bytes returned. ipv6_hdr_len walks the headers in parsed order for the same reason.

Probe. Issue repro: header 45000034…4084f8f2… → 450000380000000040840000… (wire), ip.sum 63730 → 0. All 23 fixture captures: 0 off-wire records, 0 changed on a second call, no ProtocolError. IPv6 + AH + TCP: header '' → the wire's 20 octets. Routing Hdr Ext Len 3: the routing header's tail → the wire's 20 octets.

Tests. New tests/toolkit/test_dpkt_reassembly_rawbytes_unit.py, 12 tests / 70 subtests, every frame written to a PCAP and read back through the engine. Against origin/main: 62 failed / 7 passed, every failure in the behaviour class an AssertionError. Restoring only the len() walk fails exactly the 6 extension-header subtests whose len() disagrees with the wire. Also green: test_dpkt_unit (20), run_unittest_leg.py toolkit (280, 0 failures), tests/project (389), integration/test_reassembly_engine_parity, integration/test_engine_parity, integration/test_engine_runtime, foundation/engines/test_runtime_engines, foundation/test_extraction, interface/test_misc.

Stored frames now hold one extra copy of their record octets. ipv6_hdr_len keeps its signature but counts a different set of headers, and ProtocolError is newly reachable from the four adapters.

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

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 19b78ab6c: NEEDS CHANGES (Sonnet reviewed; the author ran on Opus).

1. An offset regression on AH and odd-length Routing headers — blocking. _layer2bytes takes the offset as len(current) - len(payload), which is only right where dpkt's __len__ equals the true wire length. It does not for two headers:

  • IPv6 + AH + TCP: dpkt.ip6.IP6AHHeader.data is the whole remainder of the buffer, so len(ah) swallows the TCP segment and tcp_reassembly gets an empty header. main returned the correct 20 octets. Nothing fails loudly.
  • Routing header with an odd Hdr Ext Len: dpkt's length is len*8+8 but len(hh) is 8 + (len//2)*16; at len=3 the TCP header comes back as the routing header's tail. Plausible with SRH TLVs.

Derive the offset from the buffer rather than from dpkt __len__, or have _layer2bytes validate what it sliced (for TCP, wire[12] >> 4 == tcp.off) and raise rather than return wrong bytes. Both repros are reproducible.

2. The hard-coded 44 has to go, and 43-vs-44 is not the real problem. The review reported 43 and the leg failing; the PR asserts 44. I measured the fixture in four trees myself and both numbers are real: the shared checkout and the reviewer's worktree give 43 frames / 3347 bytes, while two other worktrees at the same head 19b78ab6c with the same examples/generators/options.py give 44 / 3431. So make_samples is not reproducible, and any test pinning an absolute count against a generated capture is flaky whichever number it picks. Derive the expected count from the capture instead of hard-coding it. I have filed the generator defect separately and will link it here.

Confirmed as the author described (each re-derived, not taken on trust): the zero-length fallback matches dpkt (dpkt/ip.py:119-122, dpkt/ip6.py:76-79), including the non-jumbogram zero case; no mutation remains on any of 43 frames, buffered or not, compared by pack_hdr() plus __dict__ (33 mutate on main); memory grows 194 KiB / 3.7% on the largest fixture under tracemalloc; all 23 captures show 0 diffs against main for packet2dict, IPv4, IPv6 and TCP. test_dpkt_unit.py 20 passed. Both out-of-scope findings — the pcapng frame count and ipv6_hdr_len's missing ordering check — are genuinely pre-existing and untouched by the diff.

Also worth doing, not blocking: the four adapters' docstrings should carry the unbuffered-fallback caveat that today sits only on packet2bytes, and a test should pin what the unbuffered path returns — nothing asserts it, and it differs from the wire on 33 of 43 frames.

@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 7, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 7, 2026
…e-serialised bytes and mutate the packet

The DPKT engine now attaches each record's octets to the frame beside its
timestamp (attach_buffer / packet2bytes). ipv4_reassembly, ipv6_reassembly,
tcp_reassembly, tcp_traceflow and packet2dict slice the header and payload
out of those octets instead of calling .pack(), which recomputed zeroed
checksums and lengths and wrote them back into the packet. A hand-built
packet with no buffer attached is serialised from a deep copy, so it is
never mutated either.

The layer's offset in those octets comes from the advance dpkt's own
unpack() made, not from len(): IP6.__len__ sums len() of the extension
headers, which overshoots by the rest of the frame for an Authentication
header and falls 8 octets short for a Routing header whose Hdr Ext Len is
odd. The slice is then checked against the layer's own pack_hdr() and a
ProtocolError raised rather than wrong bytes returned. ipv6_hdr_len walks
the headers in parsed order for the same reason, so an Authentication
header before the Fragment header is counted and one after it is not.

Closes #1297
@JarryShaw
JarryShaw force-pushed the fix/dpkt-reassembly-rawbytes branch from 19b78ab to b0af61f Compare October 7, 2026 19:23
@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 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Round-2 verdict on b0af61f5b: GOOD TO GO (Sonnet reviewed; the author ran on Opus). Both round-1 blockers are fixed.

  • Offsets come from dpkt's own walk: dpkt/ip6.py:87 (buf = buf[ext.length:]) and dpkt/ip.py:87 (__len__ = __hdr_len__ + len(opts) + len(data)). The AH and Routing (lengths 2, 3, 4) repros now slice correctly, and so do VLAN, QinQ, IP-in-IP, MPLS, SLL, IPv4 options, a truncated capture and len=0 with a trailer.
  • The pack_hdr() check never fires on valid data: pack_hdr is a plain struct.pack of stored fields, with recomputation living in __bytes__. 20,000 fuzzed IPv4/TCP/IPv6 headers gave 0 mismatches, and 1,609 fixture frames produced 0 ProtocolError.
  • The extension-header table re-derives exactly: only odd-length Routing and AH disagree with the wire.
  • ipv6_hdr_len's new walk is correct: AH, then Fragment → 52; a repeated Dst → 56; a header after the Fragment correctly stays in the payload; no Fragment gives None.
  • No absolute count remains. The revert proof against origin/main reproduces at 62 failed / 7 passed, with behaviour failures only as assertions.
  • Tests and parity: toolkit leg 280 tests, 0 failures. Both parity modules pass. dpkt and the default engine agree on IPv4, IPv6 fragments, TCP and traceflow across every fixture.

Nits: dpkt also crashes on Fragment followed by ESP, not just Dst Options, so the docstring should name both. Two Fragment headers make ipv6_hdr_len count the first, but that packet is invalid 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 7, 2026
@JarryShaw
JarryShaw merged commit 1fd8897 into main Oct 7, 2026
36 checks passed
@JarryShaw
JarryShaw deleted the fix/dpkt-reassembly-rawbytes branch October 7, 2026 20:42
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 7, 2026
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 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.

fix(toolkit): dpkt reassembly adapters build their data from dpkt's re-serialised bytes and mutate the packet

1 participant