Repository navigation
dpkt: key IPv6 reassembly on the fragment ID, not the flow label - #395
Merged
Merged
Conversation
#389 changed the IPv6 reassembly key from the Flow Label to the Fragment header's Identification, but it touched `pcapkit/toolkit/pcap.py`, `pcapng.py`, `scapy.py` and `pcapkit/protocols/internet/ipv6_frag.py` and missed `pcapkit/toolkit/dpkt.py`. The dpkt adapter was the only engine still passing `ipv6.flow` as `bufid[2]`, which feeds `pcapkit.foundation.reassembly.data.ip.DatagramID.id`. That is not cosmetic. The Flow Label is optional and is zero on all four fragments of the `ipv6.pcap` fixture, while their Identification is 110308. Keying on the label therefore collapses every fragmented datagram between one address pair carrying the same next-header into a single reassembly buffer and interleaves their fragments. Two such datagrams reach the machinery as one and it fails with `TypeError: 'bytes' object cannot be interpreted as an integer`. Also lifted the `@unittest.skip` at `tests/integration/test_reassembly_end_to_end.py`, which named the IPv6 offset scaling that #389 fixed. The test passes with no assertions weakened, and it now additionally asserts `datagram.id.id == 110308` -- the check the old docstring had explicitly declined to make while the key was wrong. That class was also the only one in the file missing its `@unittest.skipUnless(HAS_RUNTIME)` guard, which would have errored rather than skipped on a machine without the optional runtime dependencies once un-skipped. Investigated and found *correct*, so left alone with an explanatory comment: `fo=ipv6_frag.frag_off * 8`. dpkt declares the fragment header's flags word via `__bit_fields__`, so `frag_off` is the 13-bit offset already shifted out of it -- a count of 8-octet units -- and scaling once is right. On `ipv6.pcap` the four fragments read 0, 181, 362, 543 units, giving 0, 1448, 2896, 4344 octets, exactly the 1448/1448/1448/434 fragment boundaries. Had `frag_off` been the raw word, frame 14 would have read 1449. Verified: reverting the one-line fix fails three tests, including both new ones (`AssertionError: 74565 != 110308` and the `TypeError` above), so they genuinely catch the defect. Post-fix the dpkt engine agrees with the default engine octet-for-octet on `ipv6.pcap` -- same id, proto, length and payload digest. Integration 70 passed / 3 skipped; dpkt toolkit 17 passed; fixture-free unit tier 545 passed / 14 skipped.
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, matches established behavior in other engines, and is backed by strengthened unit and integration regression tests.
Pull request overview
This pull request aligns the DPKT toolkit’s IPv6 fragment reassembly behavior with the other engines by keying reassembly on the Fragment header Identification (RFC 8200) rather than the IPv6 Flow Label, preventing unrelated fragmented datagrams from being merged. It also re-enables and strengthens end-to-end payload assertions for IPv6 reassembly now that the historical offset/keying defects are fixed.
Changes:
- Fix
pcapkit.toolkit.dpkt.ipv6_reassembly()to useipv6_frag.id(Fragment Identification) inbufid[2]instead ofipv6.flow(Flow Label). - Update/add DPKT unit tests to ensure
bufid[2]tracks Fragment Identification and that fragment offsets are scaled exactly once into octets. - Lift the stale integration skip and assert exact IPv6 reassembled payload contents and
datagram.id.idin the end-to-end integration test.
File summaries
| File | Description |
|---|---|
pcapkit/toolkit/dpkt.py |
Corrects IPv6 reassembly buffer keying to use Fragment Identification and documents dpkt fragment-offset semantics. |
tests/toolkit/test_dpkt_unit.py |
Adjusts fixtures and adds regression tests to pin correct keying and single-scaling of fragment offsets. |
tests/integration/test_reassembly_end_to_end.py |
Removes obsolete skip and asserts full IPv6 reassembled payload plus correct Identification in integration coverage. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
Two of the deferred items from this session, plus a defect found while checking one of them.
The suspected defect was a false alarm — but next to it was a real one
I went in to check a note that
pcapkit/toolkit/dpkt.pywas misreading the fragment offset. It isn't. dpkt declares the fragment header's flags word through__bit_fields__:so
frag_offis the 13-bit offset already shifted out of the word — a count of 8-octet units — and* 8scales it exactly once. Onipv6.pcapthe four fragments read0, 181, 362, 543units →0, 1448, 2896, 4344octets, precisely the 1448/1448/1448/434 boundaries. Hadfrag_offbeen the raw word, frame 14 would have read 1449. Left as-is, with a comment so the next reader doesn't re-open it.The line above it was wrong, though. #389 moved the IPv6 reassembly key from the Flow Label to the Fragment header's Identification — but it touched
toolkit/pcap.py,toolkit/pcapng.py,toolkit/scapy.pyandprotocols/internet/ipv6_frag.py, and missedtoolkit/dpkt.py:Leaving dpkt the odd one out:
bufid[2]toolkit/pcap.pyipv6_frag_info.id— identificationtoolkit/pcapng.pyipv6_frag_info.id— identificationtoolkit/scapy.pyipv6_frag.id— identificationtoolkit/dpkt.pyipv6.flow— labelThat slot feeds
DatagramID.id. The Flow Label is optional and is 0 on all four fragments ofipv6.pcap, while their Identification is 110308 — so keying on it collapses every fragmented datagram between one address pair with the same next-header into a single buffer and interleaves their fragments. Two such datagrams arrive as one and the machinery dies withTypeError: 'bytes' object cannot be interpreted as an integer.The stale skip is lifted
tests/integration/test_reassembly_end_to_end.pycarried:#389 fixed that. The test now passes with no assertions weakened, and gained the one its docstring had explicitly declined to make while the key was wrong:
assertEqual(datagram.id.id, 110308).That class was also the only one in the file missing
@unittest.skipUnless(HAS_RUNTIME, ...)— invisible while everything in it was skipped, but it would have errored rather than skipped on a machine without the optional runtime deps the moment it started running.Verification
The new tests were checked against the bug rather than just against the fix. Reverting the one-line change:
Post-fix, the dpkt engine agrees with the default engine octet-for-octet on
ipv6.pcap— sameid.id=110308,proto=17,len=4778, identical payload digest.tests/integration/test_reassembly_end_to_end.pytests/integrationtests/toolkit/test_dpkt_unit.pyThe 14 fixture-free skips are missing optional third-party packages (
pypcap,pypcapfile) and are pre-existing. The 3 remaining integration skips are unrelated known blockers — parse-limit plumbing, and two dictdumper bugs.Note on scope
This touches
tests/toolkit/test_dpkt_unit.pybecause its line 254 assertedbufid[2] == 7— the buggy flow-label value — so the fix could not land without updating it.