reassembly: record IP fragment overlap conflicts instead of losing them silently

[JarryShaw]

Closes #477

Summary

pcapkit/foundation/reassembly/ip.py:133 slice-assigned an arriving fragment's payload into the datagram buffer unconditionally, with no check of RCVBT beforehand and no record kept when two fragments claimed the same octets with different bytes. This is the IP analogue of #443 (fixed for TCP in unmerged PR #478), but a separate implementation, governed by a different RFC, and — per the research below — resolved the opposite way.

• pcapkit/foundation/reassembly/data/ip.py: new Buffer.conflict (list, accumulated) and Datagram.conflict (tuple, additive) — (first, last) absolute, inclusive octet ranges, mirroring reassembly: resolve conflicting TCP overlaps first-write-wins, per RFC 9293 #478's TCP conflict field and its naming/convention.
• pcapkit/foundation/reassembly/ip.py: new IP._detect_conflicts, called before the existing overwrite. It consults RCVBT block bits clipped by TDL to find which octets already held genuinely-received data, then compares those bytes directly against the arriving payload; a differing run is recorded, an equal one (a duplicate) is not. The overwrite itself is unchanged — still last-write-wins, not first-write-wins.
• tests/foundation/reassembly/test_ip.py: 7 new cases (identical duplicate, full block-aligned conflict, partial overlap extending each direction, a conflict confined to the final fragment's partial 8-octet block, a three-way conflict, and a conflict resolved before a later fragment completes the datagram).
• docs/source/pcapkit/foundation/reassembly/ip/{ipv4,ipv6}.rst: document conflict on both Datagram and Buffer, and the RCVBT/TDL clipping that keeps a reported conflict range from over-reaching into a fragment's never-really-sent padding.

RFC finding — the resolution is the opposite of TCP's

RFC 791 answers "which fragment wins" explicitly, and it is the opposite of TCP's RFC 9293 first-write-wins:

In the case that two or more fragments contain the same data either identically or through a partial overlap, this procedure will use the more recently arrived copy in the data buffer and datagram delivered.

RFC 815's hole-descriptor algorithm says nothing directly about differing content, but its unconditional "copy the data from the fragment into the reassembly buffer" is consistent with the same last-write-wins resolution. So this fix keeps the existing overwrite direction and adds only the missing record of disagreement — it does not port #478's first-write-wins merge logic, which would contradict RFC 791's own text.

RCVBT's 8-octet granularity

RCVBT records receipt in 8-octet blocks, coarser than an octet conflict needs. Every fragment but the last is required to be block-aligned (and FO is always a multiple of 8 — it is wire-encoded in 8-octet units), so the only block that can be partially real is the one holding the final fragment's own tail. _detect_conflicts treats octet pos as genuinely already-received iff rcvbt[pos // 8] is set and (tdl < 0 or pos < tdl), where tdl is Buffer.TDL as it stood before the current fragment's own update.

Verified against a case built for exactly this: a final fragment covering only 3 of an 8-octet block's octets, then a later fragment claiming the full block — the reported conflict is (8, 10), not (8, 15); octets 11-15 were never really sent by anyone and are correctly treated as new territory, not a conflict. Comparing bytes directly was sufficient once combined with the TDL clip — no new per-octet bytearray was needed.

Reproduction and revert-proof

Pre-fix, through the public API: two conflicting 8-octet fragments at the same offset silently produced a complete datagram with the second fragment's bytes, and AttributeError on .conflict (the field did not exist). Post-fix: same payload (last-write-wins, per RFC 791), plus conflict == ((0, 7),).

Reverted the fix via a tagged git stash and confirmed all 7 new boundary cases fail immediately with AttributeError: 'Datagram' object has no attribute 'conflict'; restored via git stash apply + git stash drop.

Test plan

• tests/foundation/reassembly/ — 63 passed (7 new), 0 failed
• tests/foundation/ — 200 passed, 11 skipped
• mypy pcapkit, mypy --config-file mypy.ini pcapkit, and the Makefile's --follow-imports=silent --ignore-missing-imports --show-column-numbers --show-error-codes pcapkit — all three report zero errors in the two files touched here (122 total errors in 39 files is the pre-existing, unrelated baseline: toolkit/scapy.py, protocols/internet/ipv6.py, foundation/engines/*, etc.)

Accepted behaviour break

Same reasoning as #478: a capture that hits this path is anomalous by definition, and a caller reading .conflict on an existing Datagram for the first time is the point of the fix, not a compatibility concern.

Noticed but not fixed here

pcapkit/toolkit/scapy.py:160 sets fo=ipv4.frag with no * 8, but scapy's IP.frag is the raw 13-bit wire field in 8-octet units, not a byte offset — confirmed by constructing scapy.all.IP(frag=5) and observing the raw fragment-offset field on the wire is 5, not 40. Every other adapter (dpkt.py, pypcapfile.py, pcap.py, pcapng.py, the native ipv4.py) multiplies by 8 or uses an already-scaled .offset. This looks like a real, separate defect in the scapy IPv4 fragment-offset handling; not yet filed as its own issue for lack of time in this session — flagging here so it isn't lost.

[JarryShaw]

Review of 6fbfb4185 (first review of this PR)

Setup: worktree checked out at git rev-parse HEAD = 6fbfb4185dcac0830e1fa267c9154ac5dc807094, confirmed twice. Diff taken as git diff $(git merge-base origin/main 6fbfb4185)..6fbfb4185 (merge-base == current main tip 46def3416).

CI

gh api repos/JarryShaw/PyPCAPKit/commits/6fbfb4185/check-runs --paginate --jq '...'

23/23 finished: 21 success, 2 skipped. The 2 skipped are Gate (full suite, Python 3.14) and Docs test gate, which skip by design on pull_request events — fully green, no waivers needed.

RFC 791 last-write-wins claim

Traced the actual overwrite: buf.datagram[start:stop] = info.payload in reassembly() (ip.py) is unconditional, unchanged by this PR, and always runs after _detect_conflicts reads the pre-write state. So the code genuinely implements last-write-wins, matching the quoted RFC 791 text, and it is documented in three places (the inline comment above the overwrite, Datagram.conflict's docstring, and both .rst notes) specifically to warn a future reader against "fixing" it into TCP's first-write-wins. That documentation is doing real work here — good.

TDL-before-update ordering and the tdl < 0 sentinel

Read the source directly: _detect_conflicts(buf.RCVBT, buf.TDL, ...) is a statement that executes, and its arguments are evaluated, strictly before the if not MF: TDL = ...; buf.__update__(TDL=TDL) block later in the same function body — plain sequential execution, no laziness involved. Buffer.TDL is initialized to -1 at buffer creation (ip.py, buffer-init block), confirming tdl < 0 is exactly "no final fragment has arrived yet."

I then constructed two of my own cases (not the author's), run against this worktree's pcapkit (pcapkit.__file__ asserted to start under the worktree root):

• Case A — a 5-octet tail (author's case used 3), with the arriving fragment's comparison split by a match in the middle (byte 8 agrees, bytes 9-12 disagree, bytes 13-15 sit past TDL=13 and must be excluded): got conflict == ((9, 12),), exactly as predicted by hand-tracing the algorithm. Also incidentally confirmed the docstring's claim that submit() never reports a payload past TDL — the reassembled payload came back as 13 octets even though 3 more octets ('YYY') were physically written into the raw buffer past TDL.
• Case B — reversed arrival order relative to the author's test: a full-block non-final fragment arrives before the final fragment establishes TDL for that block, then a second full-block fragment arrives after. Got conflict == ((8, 12), (8, 12)), matching hand-traced expectations for both calls.

Both matched predictions exactly; no evidence of the ordering being wrong.

Test coverage of the new cases (7 new tests in test_ip.py)

Enumerated what's actually exercised: identical-duplicate (no conflict), full block-aligned overlap, partial overlap extending left, partial overlap extending right, the RCVBT/TDL tail-clipping edge case, a three-way conflict (confirms comparison is always against current buffer contents, i.e. the last writer, not the original), and conflict-survives-a-clean-completed. All 7 pass, plus the other 10 pre-existing tests in the file (17/17), and the full tests/foundation/reassembly/ suite: 63 passed, 0 failed (python -m unittest discover -s tests/foundation/reassembly), reproducing the number quoted in the commit message.

Two genuine (non-blocking) coverage gaps, since the ask was to enumerate rather than trust the names:

• No test has two different datagrams (BUFIDs) in flight at once to confirm conflict is correctly buffer-scoped and doesn't leak across them. Low risk — conflict is a per-Buffer dataclass field created fresh ([]) per BUFID, so nothing shared — but untested.
• No test exercises a single fragment's own write producing a match-then-conflict-then-match split (multiple non-adjacent runs from one comparison pass, as opposed to the three-way test's two separate arrivals each producing one run). The inner while loop in _detect_conflicts clearly supports this (breaks a run on a match, restarts detection afterward), and my Case A above exercises half of this (a leading match before a conflict run), but nothing exercises a conflict sandwiched between two matching regions in one write.

Neither gap points at a suspected bug; both are just untested branches of otherwise-verified-correct logic.

Other checks

• conflict type/convention: Datagram.conflict: tuple[tuple[int, int], ...], Buffer.conflict: list[tuple[int, int]], both absolute + inclusive — checked against PR reassembly: resolve conflicting TCP overlaps first-write-wins, per RFC 9293 #478's TCP fields directly (gh pr diff 478) and they match exactly.
• completed / strict independence: read submit() in full — conflict=conflict (a tuple(buf.conflict) snapshot taken once at the top of submit()) is passed unconditionally in both the strict/partial-runs branch and the complete/contiguous branch, regardless of completion (COMPLETE/TIMEOUT/PARTIAL). Matches the stated design decision. The tuple snapshot (rather than aliasing the live list) is the right call defensively, though I didn't find a path where it would currently matter.
• Docs: docs/source/pcapkit/foundation/reassembly/ip/{ipv4,ipv6}.rst (.rst, not .md) document conflict on both Datagram and Buffer, and reference the real defining module (pcapkit.foundation.reassembly.ip.IP._detect_conflicts), not a re-export. Checked both .. code-block:: text ASCII diagrams end-to-end (read the full files) — the new conflict lines are appended inside the existing block at the correct indent, and the new .. note:: blocks are placed after the code-block closes, not wedged into it — the sibling-PR defect described in my brief does not appear here.
• mypy: mypy pcapkit/foundation/reassembly/ip.py pcapkit/foundation/reassembly/data/ip.py --follow-imports=silent --ignore-missing-imports --show-column-numbers --show-error-codes → 0 errors. Full-package run with the same (Makefile) flags, mypy pcapkit --follow-imports=silent --ignore-missing-imports --show-column-numbers --show-error-codes, reproduces exactly "122 errors in 39 files" as claimed, none of them in the two touched files (grep "reassembly/ip" on the log → no output).
• Issue scapy toolkit adapter passes raw 13-bit fragment offset unscaled to IP reassembly #483 (scapy fo=ipv4.frag unscaled): confirmed filed and open (gh issue view 483). Correctly left out of this PR/diff. As a bonus, reproduced it end-to-end through the real reassembler (not just at the field level): built two scapy IPv4 fragments (frag=0/MF then frag=5/final) and fed them through pcapkit.toolkit.scapy.ipv4_reassembly → pcapkit.foundation.reassembly.ipv4.IPv4. The unscaled fo=5 (should be 40) lands fragment 2 inside fragment 1's payload; the datagram reports completed=COMPLETE with a truncated 13-octet payload (b'AAAAABBBBBBBB', should be 48 octets) and, incidentally, this PR's own new field surfaces the corruption as conflict=((5, 12),). That's a real, currently-reproducible data-loss bug, but it's scapy toolkit adapter passes raw 13-bit fragment offset unscaled to IP reassembly #483's bug, not IP fragment reassembly overwrites already-received bytes on an overlapping fragment, unconditionally #477's/reassembly: record IP fragment overlap conflicts instead of losing them silently #482's — not asking for it to be touched here.

Verdict

No defects found in this PR's own diff. Implementation matches its documented claims under independent construction, tests pass, CI is green, mypy is clean, docs are consistent and correctly scoped.

GOOD TO MERGE 6fbfb4185