Skip to content

fix(reassembly): a TSO fragment's length is its captured length, and RCVBT covers the last block (#1555, #1565) - #1564

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1555-tso-ipv4-reassembly
Oct 10, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1555-tso-ipv4-reassembly

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 10, 2026 •

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 #1555. Closes #1565.

Both fixes are in pcapkit/foundation/reassembly/ip.py only.

#1555. IP.reassembly took a fragment's data length from tl - ihl. A Total Length of 0 (TSO) gave -20: every TSO datagram came out PARTIAL and empty, and a zeroed final fragment completed its datagram truncated. With tl == 0, the data length is now the payload the adapter passed, which since #1547 is the rest of the captured frame. Wireshark 4.6.9 does the same before it reassembles ("presumed … TSO", then "[2 IPv4 Fragments (104 bytes)]", whichever fragment is zeroed).

  • TSO capture: the zeroed tcp.pcap (fixed and stale checksums) gives the unmodified capture's 4 × COMPLETE (44/32/244/32) under default, dpkt, scapy and pypcapfile.
  • Over 65,535 octets (BIG TCP): the buffer grows, RCVBT with it.
  • Cut non-final TSO fragment: its partial last block stays a hole, as it would with the real Total Length. A cut final or unfragmented TSO frame still comes out COMPLETE, because Packet carries no original length.
  • IPv6: not affected; its adapters pass tl = hdr_len + len(payload).

#1565. RCVBT had 8,191 blocks for a 65,535-octet buffer. A fragment reaching octet 65,528 made _detect_conflicts raise IndexError. It now has 8,192 blocks. Tests cover offset 65,528 with 1 and 7 octets, MF set and clear, both strict modes, IPv4 and IPv6.

Reassembly output on every sample capture is byte-identical to main (all four engines, both strict modes).

@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 16d81b965. Cross-reviewed on Sonnet; the author ran Opus. The TSO fix is right; one edge needs a guard.

Confirmed:

  • TSO capture: tcp.pcap with TL zeroed now gives 4 COMPLETE datagrams (44/32/244/32) under default, dpkt and scapy, plus pypcapfile on 3.11.
  • Large datagrams: a 70,000-octet TSO datagram reassembles byte-equal.
  • Zeroed fragment: a zeroed real fragment matches tshark 4.6.9's "presumed TSO" reassembly.
  • Every sample capture: 9,454 reassembly rows are byte-identical to main.
  • Fail-before: 9 + 24 failures.
  • With fix(protocols): a later IPv4 fragment's data is raw, not an upper-layer header (#1553) #1562: merged cleanly.

To fix: when TL is 0 and MF is set, a snaplen-cut fragment is taken at its cut length, but its last partial 8-octet block still gets marked as received. I reproduced it: a first fragment cut from 56 to 52 octets with TL 0 gives COMPLETE, 104 octets, with octets 52–55 zero-filled. With the real TL the same input is PARTIAL. The fix is to mark only whole 8-octet blocks when TL == 0 and MF. The reviewer tested if held == length and not (TL == 0 and MF): at ip.py:205, and the PR's tests still pass with it. Add a test for this case.

Also widen the code comment: a cut final fragment with TL 0 still comes out COMPLETE (Packet carries no original length, so ip.py can't tell). Say that it applies to fragments too, not only unfragmented frames.

Separately: a fragment at the maximum offset (65,528) crashes reassembly on main. That is now #1565, outside this PR.

@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
…RCVBT covers the last block (#1555, #1565)

A TSO frame's IPv4 header carries Total Length 0, and since #1547 the
adapters hand over the rest of the captured frame as its payload. The
reassembler still took the data length from tl - ihl (-20), so every
such datagram came out PARTIAL and empty, and a zeroed final fragment
completed its datagram truncated. Separately, RCVBT had 8191 blocks for
a 65535-octet buffer, so a fragment reaching octet 65528 raised
IndexError.

- ip.py: with tl 0, take the data length from the payload, as Wireshark
  does ("presumed TSO"); grow the buffer past 65535 octets (BIG TCP);
  leave a non-final TSO fragment's partial last block unmarked, since
  it can only be a snaplen cut.
- ip.py: size RCVBT at 8192 blocks, and keep it in step on growth.
- tests: the zeroed tcp.pcap under default, dpkt and scapy, fixed and
  stale checksums, reassembles as the unmodified capture; zeroed and
  cut fragments; a 70000-octet datagram; offset 65528 with 1 and 7
  octets, MF set and clear, under IPv4 and IPv6.

Reassembly output on every sample capture is unchanged.
@JarryShaw
JarryShaw force-pushed the fix/1555-tso-ipv4-reassembly branch from 16d81b9 to e7c9e5f Compare October 10, 2026 04:21
@JarryShaw JarryShaw changed the title fix(reassembly): an IPv4 Total Length of 0 is the fragment's captured length (#1555) fix(reassembly): a TSO fragment's length is its captured length, and RCVBT covers the last block (#1555, #1565) Oct 10, 2026
@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 e7c9e5fba. This was a Sonnet delta review; the author ran Opus. It merges cleanly into current main.

  • Cut-fragment guard:
    • I fed TL-0 fragments in all 6 arrival orders, both strict and lax, and through hand-built pcaps on the default, dpkt and scapy engines.
    • A cut first or middle fragment now reads PARTIAL. That includes a cut that ends on an 8-octet boundary, which the gap catches.
    • The only cuts it can't detect are a cut final fragment and a cut unfragmented frame, as the comment says.
  • fix(reassembly): IPv4 reassembly raises IndexError for a fragment reaching octet 65,528 #1565: an exhaustive sweep of every Fragment Offset 0–65528 × length × TL × MF × strict × IPv4/IPv6 (655,360 cases) gives 0 exceptions. main gives 6,688 IndexErrors.
  • RCVBT grows with the buffer: len(RCVBT) == ceil(len(datagram)/8) held in all 286 buffers checked, and I found no off-by-one.
  • Round trips: TL-0 and real-TL datagrams reassemble byte-equal on 3 engines. The sample-capture dump of 9,454 rows is identical to main's. pypcapfile couldn't be checked here.
  • Fail-before: I reproduced both: 4 failures with the prior ip.py, and 36 with main's.

Residuals this PR doesn't introduce. I re-measured both and am filing them as follow-ups:

  • A crafted datagram over 65,535 octets can read COMPLETE but short.
  • A first IPv6 fragment cut short is zero-filled and reported COMPLETE. main does the same.

@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, e7c9e5fba, 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 2240 67 722 34 96.32%
pcapkit/dumpkit 258 2 90 2 98.85%
pcapkit/foundation 2772 129 984 46 94.28%
pcapkit/interface 108 7 36 5 91.67%
pcapkit/protocols 16996 230 4548 191 98.02%
pcapkit/toolkit 635 98 198 5 83.31%
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 e0d3c85 into main Oct 10, 2026
39 checks passed
@JarryShaw
JarryShaw deleted the fix/1555-tso-ipv4-reassembly branch October 10, 2026 05:17
@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