Skip to content

fix(reassembly): only the final fragment's partial block counts, and an oversized datagram never completes (#1566, #1567) - #1582

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1566-1567-reassembly-edges
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1566-1567-reassembly-edges

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

Please follow the guide below

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 #1566. Closes #1567.

The code change is in pcapkit/foundation/reassembly/ip.py only; no toolkit adapter is touched.

Sample-capture reassembly is byte-identical to main: 9,456 rows across default, dpkt and scapy, in both modes.

…an oversized datagram never completes (#1566, #1567)

- #1566: both completion checks tested all(RCVBT[0:(TDL + 7) // 8]), a
  slice that stops at the table's end, so a datagram declaring more data
  than the 65,535-octet buffer completed short. One helper, IP._is_complete,
  now also requires TDL <= len(datagram).
- #1566: strict mode dropped a datagram with no run received while lax mode
  reported it. Both now report it, strict with payload ().
- #1567: a non-final fragment ending mid-block marked that block, so a
  snaplen-cut IPv6 fragment (tl comes from the captured payload) completed
  its datagram zero-filled. Only the final fragment's partial last block is
  marked now; this replaces #1564's Total Length 0 special case and also
  covers a malformed IPv4 one. A fragment the buffer's end clips stays
  counted as cut there, so 7 and 8 octets at offset 65,528 now agree.

Sample-capture reassembly is unchanged (9,456 rows, default/dpkt/scapy).
@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 commented Oct 10, 2026 •

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at e8273d9d6. This was a Sonnet cross-review; the author ran Opus. It merges cleanly into current main, and on the merged tree tests/foundation/reassembly gives 126 passed.

  • Legitimate datagrams are untouched. The 9,456-row sample dump is identical to main. A seeded differential run of 6,072 cases (3,000 random IPv4/IPv6 datagrams, IHL 20–60, every arrival order, overlap, duplicates, TL 0 up to 100,000 octets) also shows 0 diffs. IPv4 fragments with IP options reassemble byte-exact on all three engines, and tshark agrees.
  • The non-final partial-block rule matches Wireshark 4.6.9. It agrees on six crafted IPv4/IPv6 inputs, where main zero-filled the gap case. It also matches Linux: ip_frag_queue trims a non-final fragment to a multiple of 8, and IPv6 rejects one that isn't a multiple of 8.
  • payload=() datagrams are safe. to_dict, json.dumps, count/index and the json/plist/tree/pcap dumpers all handle them.
  • _is_complete: the issue's own candidate fix still lets TDL 65,536 through as COMPLETE; this PR's bound reads PARTIAL. TL 0 growth stays correct up to 100,000 octets.
  • Fail-before: with main's ip.py, 46 tests fail. Reverting one part of the fix at a time gives 31, 8 and 9 failures.
  • I re-ran the repro captures myself. The over-65,535 case now reads PARTIAL, and a cut IPv6 first fragment gives runs (56,136) on all three engines.

Non-blocking, filed as #1585 (blocked on this PR):

  • No header-aware 65,535 check. A datagram whose header plus TDL exceeds 65,535 (65,523 + 20) still reads COMPLETE. Linux's ip_frag_reasm rejects it.
  • Stale _detect_conflicts docstring. It still says a non-final fragment's range is block-aligned, so the octets of an unmarked cut tail record no conflict.

@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: 89.63% (unit tier, Python 3.14, e8273d9d6, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18858 957 2366 871 91.03%
pcapkit/corekit 2334 67 746 35 96.43%
pcapkit/dumpkit 275 3 100 3 98.40%
pcapkit/foundation 2790 154 982 42 93.43%
pcapkit/interface 138 7 52 5 93.68%
pcapkit/protocols 16991 230 4544 191 98.02%
pcapkit/toolkit 708 96 234 6 85.35%
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 merged commit 98d7d80 into main Oct 10, 2026
40 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 10, 2026
@JarryShaw
JarryShaw deleted the fix/1566-1567-reassembly-edges branch October 10, 2026 12:19
@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

1 participant