Conversation
Hole descriptors carried absolute sequence numbers from the toolkits, the first hole was seeded from a payload length, and submit() sliced a buffer indexed from its own initial sequence number with those bounds. On any capture with a SYN and a realistic ISN every slice landed past the end of the buffer, so an incomplete datagram was dropped silently instead of being returned with completed=False. Descriptors are now absolute and inclusive throughout, submit() is the only place they are converted into buffer offsets, and holes outside a buffer are dropped rather than indexing from its far end. A SYN no longer contributes its own sequence number to the payload, which had been prepending a NUL octet to complete datagrams. Closes #349.
The integration tier was three modules; the real end-to-end spectrums lived only as the assertion-free demonstrations in examples/legacy_smoke. Added seven modules covering report formats on disk, manual frame iteration, TCP and IP reassembly, flow tracing, engine parity, PCAP-NG including both byte orders, and the CLI as a subprocess. Assertions are on content rather than liveness: reassembled bodies match their own Content-Length, and the 331 traced flows partition all 1117 frames of http.pcap exactly once. Four tests are skipped against defects they would otherwise pin, each naming the file and line to fix.
test_sample_capture_reassembles_every_stream_byte_exactly reads test.pcap, which examples/generators/make_samples.py builds rather than the repository carrying it, so the unit-test workflow -- which deliberately runs without generated fixtures -- failed on every Python version with FileNotFoundError. Moved to tests/foundation/reassembly/test_tcp_runtime.py, matching the convention the ignore globs already encode: fixture-dependent cases live in *_runtime.py and *_regression.py, and the integration job generates the fixtures before running them. CI selection: 318 passed, 103 subtests passed, with no fixtures present.
This was referenced Sep 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft. Stacked on #371 — GitHub retargets this to
mainwhen that merges. Stacked because the end-to-end tests assert on PCAP-NG behaviour #371 corrects.TCP reassembly (#349)
Hole descriptors carried absolute sequence numbers from the toolkits, the first hole was seeded from a payload length, and
submit()sliced a buffer indexed from its own initial sequence number using those bounds. On any capture with a SYN and a realistic ISN every slice landed past the end of the buffer, so an incomplete datagram was dropped silently instead of coming back withcompleted=False— thecompleted=False/packet is Nonebranches of the public API were unreachable in practice.Measured, 5 frames with two segments permanently lost:
0xC0DE1234completed=False, fragments[10, 10, 10][10][10, 10, 10][11, 11, 10][10, 10, 10]Descriptors are now absolute and inclusive throughout, with
submit()the only place they become buffer offsets; holes outside a buffer are dropped rather than indexing from its far end. Absolute was chosen over relative becausehdllives onBuffer, shared acrossFragments, while a relative origin would have to live onFragment— whoseisnmutates when a segment arrives below the data already buffered.A fourth symptom the issue did not name is also fixed: a complete stream whose SYN shares an acknowledgement number came back one octet too long, the SYN's own sequence number showing up as a leading NUL.
Four of the five toolkits needed the inclusive-bound change (
pysharkhas no reassembly at all), and four existing tests were pinning the broken behaviour.End-to-end tier
tests/integration/was three modules; the real end-to-end spectrums existed only as the assertion-free demonstrations inexamples/legacy_smoke/. Seven modules added — 60 tests, 73 subtests, 23s — covering report formats written to disk and read back (tree/json/plist,files=True), manual frame iteration, TCP and IP reassembly, flow tracing, engine parity acrossdefault/dpkt/scapy, PCAP-NG including both byte orders, and the CLI as a subprocess.Assertions are on content rather than liveness: reassembled bodies match their own
Content-Length, and the 331 traced flows ofhttp.pcappartition all 1117 frames exactly once.Four tests are skipped rather than pinning defects — one each for the plist
<date>format,layer=/protocol=being ignored, the IPv6 fragment offset, and unescaped JSON mapping keys. Each names the file and line, so they can be un-skipped by whoever fixes it. That choice was deliberate:test_ipv6_extension_runtime.pyhad pinned a bug, which is why fixing #348 required editing a test.Verification
Full suite on this branch in a clean worktree, fixtures regenerated from scratch: 429 passed, 4 skipped, 183 subtests passed.
Closes #349.