Advance past the last IPv6 extension header (#348) - #373
Conversation
The extension-header loop broke out of the walk before advancing the payload, so a fragmented datagram handed the fragment header to the next layer as if it were the transport header: a UDP fragment reported srcport 4352, dstport 1 and length 1, which are the fragment header's next-header and reserved octets, its offset-and-flags hextet, and the top half of the identification. The advance now runs before every exit from the loop. tests/protocols/internet was pinning the wrong values and now pins the real header (51234 -> 5001, length 4778), plus a synthetic chain per shape - none, fragment only, hop-by-hop with destination options, and hop-by-hop with a 16-octet routing header then fragment - since no committed capture carries more than one extension header. Closes #348.
There was a problem hiding this comment.
🟢 Approval recommended
The payload-advance fix is minimal and correctly covered by updated and expanded regression tests that specifically fail under the prior buggy behavior.
Pull request overview
Fixes IPv6 extension-header traversal so the payload pointer is advanced past the last extension header (including the fragment header) before dispatching to the upper-layer protocol, preventing the next layer from decoding extension-header bytes as a transport header (closes #348).
Changes:
- Update
IPv6._decode_next_layerto advancepayloadbefore any loop exit, including the fragment-headerbreak. - Correct and strengthen the runtime IPv6 extension-header tests, and add synthetic multi-extension-header regression coverage.
- Update the sample PCAP generator module docstring to describe the fixture behavior correctly (docstring-only change).
File summaries
| File | Description |
|---|---|
pcapkit/protocols/internet/ipv6.py |
Ensures payload is advanced past the last IPv6 extension header (including fragment) before decoding the upper-layer protocol. |
tests/protocols/internet/test_ipv6_extension_runtime.py |
Updates the previously bug-pinning assertions and adds regression coverage for multiple extension-header chains and correct UDP handoff bytes. |
examples/generators/pcap.py |
Updates documentation describing what the IPv6 fixture contains and why the first fragment’s UDP header values can look surprising. |
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.
There was a problem hiding this comment.
🟢 Approval recommended
The fix is small and targeted, and the updated plus newly added tests provide strong regression coverage; only minor docstring path corrections were noted.
Review details
Suppressed comments (1)
tests/protocols/internet/test_ipv6_extension_runtime.py:134
- This docstring points to
examples/captures/, but the.pcapsample fixtures (includingipv6.pcap) are underexamples/sample/in this repo. Updating the path avoids confusion when trying to reproduce or extend these tests.
``examples/captures/``: no committed capture carries more than one IPv6
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
Co-authored-by: JarryShaw <15666417+JarryShaw@users.noreply.github.com>
Addressed in 0ce947d. |
Draft. Independent of #371 and #372 — one file plus its tests.
The defect
pcapkit/protocols/internet/ipv6.pybroke out of the extension-header walk before advancing the payload, so the next layer received the bytes starting at the last extension header rather than after it. On a fragmented datagram that means the fragment header gets decoded as the transport header:11 00 00 01 00 01 ae e4c8 22 13 89 12 aa 0f 75Those numbers are not a malformed header:
4352is0x1100, the fragment header's next-header (17) and reserved octet;1is its offset-and-flags hextet; the length is the top half of identification 110308.The advance now runs before every exit from the loop. Each branch was checked rather than assumed — the fall-through to another extension header must advance (unchanged, only moved earlier), the fragment
breakmust advance and did not, theexcept ValueErrorexit must not advance again and does not, and a datagram with no extension headers never enters the loop.An existing test was pinning the bug
tests/protocols/internet/test_ipv6_extension_runtime.pyasserted the wrong ports and length; it now asserts the real header, plus an invariant (bytes(udp.packet.header) == bytes(ipv6.info.fragment.payload)[:8]) instead of three more magic constants.test_ip_runtime.pyneeded no change — itsipv6.pcapcase is an ICMPv6 frame with no extension headers.examples/generators/pcap.py's module docstring documented the odd ports as expected behaviour and now describes what the fixture contains. Docstring only; the captures regenerate byte-identically.Regression test
A subtested table over four chains — none, fragment only, hop-by-hop plus destination options, and hop-by-hop plus a 16-octet routing header plus fragment — asserting the extension-header list,
hdr_len,raw_len, and that UDP was handed exactly the UDP header. The datagrams are synthetic and parsed straight throughIPv6(...), because no committed capture carries more than one extension header and the fixtures are pinned byte-for-byte. The routing header is deliberately 16 octets, so a walk advancing by a fixed 8-octet stride instead of each header's own length is also caught.Confirmed to catch the bug: with the fix reverted, the two fragment chains fail and the two non-fragment chains pass.
Two incidental defects found and not fixed here
Both in
pcapkit/protocols/internet/ipv6_route.py, both independent of this change and worth their own issues: well-formed routing headers are rejected because_read_data_type_2compares the schema's raw Hdr Ext Len octet against 24 (it is 2 for an RFC 6275 type-2 header), with the same shape in_read_data_type_srcand_read_data_type_rpl; and a malformed extension header crashes the walk withAttributeError: 'Raw' object has no attribute 'next'when_import_next_layerdegrades toRaw, rather than ending it.Closes #348.