Repository navigation
fix(reassembly): bypass copyreg's MRO walk for Deferred's protocol<2 pickling - #2435
Merged
Merged
Conversation
JarryShaw
force-pushed
the
fix/deferred-pickle-graalpy-generic-2433
branch
from
October 29, 2026 19:15
780ff6b to
1d529f0
Compare
JarryShaw
added
fix
Owner
Author
|
Review: NEEDS CHANGES at
Also: the PR body's corekit count (866/7649) is actually the combined reassembly+corekit run, not corekit alone (693/6767) — fix the label. And since GraalPy was actually run against this fix (not just reasoned about), replace "GraalPy isn't available here" with the real result before this closes #2433. Confirmed correct and unaffected: protocol ≥2 is byte-identical to |
JarryShaw
added
review: needs-changes
Contributor
|
Coverage: 94.44% (unit tier, Python 3.15,
Per-file detail: the |
JarryShaw
force-pushed
the
fix/deferred-pickle-graalpy-generic-2433
branch
from
October 29, 2026 19:57
1d529f0 to
641331f
Compare
JarryShaw
added
review: running
Owner
Author
|
Review: NEEDS CHANGES at Only the new test file's docstrings need fixing — numbers borrowed from the wrong module, one self-contradiction, and a count that shouldn't be there per the owner's no-counted-numbers-in-docs ruling:
Fix: drop the counts (owner ruling, no counted numbers in docs/prose) and the "cannot be run" sentence; state plainly that it's confirmed on GraalPy 25.4.4 now that it has been. Non-blocking, noted for awareness only: |
JarryShaw
added
review: needs-changes
…pickling copyreg._reduce_ex, which object.__reduce_ex__ hands protocol 0/1 to, walks cls.__mro__ for a base to compute state from, before __getstate__ is ever consulted. On CPython that walk passes through Generic (a heap type whose __new__ is just the inherited, differently-bound object.__new__) and stops at object (state=None); on GraalPy 25.4 Generic is itself a non-heap (built-in) type, so the walk's first check breaks there directly and calls Generic(self), raising "TypeError: typing.Generic() takes no arguments" -- independent of #2424/#2425's __getstate__/__setstate__ fix, since those only supply the dict half of a reduction the walk fails to even build. - Deferred.__reduce_ex__ now hard-codes base=object, state=None for protocol 0/1 -- the only outcome the walk ever reached for this class -- and builds the reduction tuple directly via copyreg._reconstructor, with __getstate__()'s own (dict_or_None, slots) pair restored through __setstate__ exactly as before. Protocol >= 2 defers to object.__reduce_ex__ unchanged. - It also checks for, and defers to, a subclass's own __reduce__ override before building that hard-coded tuple, matching what object.__reduce_ex__ itself already does for every protocol. Without the check, a Deferred subclass overriding __reduce__ had it silently dropped at protocol 0/1 while protocol 2+ (via object.__reduce_ex__ unchanged) kept honouring it -- a regression caught in cross-review, now covered by a new test alongside the GraalPy-failure regression test. - Dropping Generic[_KT] instead was considered and rejected: test_deferred_proto_type_2417_unit.py reads Deferred.__parameters__ at runtime, which only exists because Generic is a real base, not a TYPE_CHECKING-only annotation. No longer invokes copyreg._reduce_ex's MRO walk for protocol<2 at all, so Generic is no longer part of the route pickling takes. Confirmed on real GraalPy 25.4.4 by cross-review: test_deferred_pickle_2423_unit.py goes from 8 failed on main to 16 passed/18 subtests on this fix, and the full tests/foundation/reassembly/ suite passes 173/854 subtests, 0 failed. Build/test (CPython): mypy 98 errors/32 files (baseline, unchanged), pylint 10.00/10, isort clean; tests/foundation/reassembly (176 tests/890 subtests, including the two new regression tests) and tests/corekit (693 tests/6767 subtests, unaffected by this change) pass; 100% coverage on the changed file. Closes #2433
JarryShaw
force-pushed
the
fix/deferred-pickle-graalpy-generic-2433
branch
from
October 29, 2026 20:21
641331f to
b8ac765
Compare
JarryShaw
added
review: running
Owner
Author
|
Review: GOOD TO GO at Together with rounds 1–2's findings (both fixed and verified, including a real run on GraalPy 25.4.4), this PR is ready to merge. |
JarryShaw
added
review: good-to-go
JarryShaw
removed
the
review: good-to-go
JarryShaw
added a commit
that referenced
this pull request
Oct 30, 2026
Closes #2436. ICMP (RFC 792) and ICMPv6 (RFC 4443) had no implementation at all -- Internet.__proto__ had no entry for either protocol number, so a packet carrying one fell back to Raw with only the protocol number as a hint. - Add ICMP_Family between Internet and ICMP/ICMPv6, following the IP/IPsec precedent: nothing but id(). The base's own name is "_Family"-suffixed (matching IPv6_Route, IPv6_Ext) because, unlike IP/IPsec, the family's generic name collides with one of its own members (ICMP). - ICMP: Echo/Echo Reply, Destination Unreachable, Time Exceeded, Parameter Problem, plus a raw fallback for any other type. Own type registry (__type__, register_type), own schema. Both classes construct with no arguments (default body is the minimum valid length) and compute/verify their own checksum via a new shared pcapkit.utilities.checksum.inet_checksum (RFC 1071): ICMP sums the message alone, ICMPv6 additionally sums the IPv6 pseudo-header (RFC 2460 Section 8.1) when the enclosing IPv6 addresses are known; a bad checksum warns rather than raises. - ICMPv6: Destination Unreachable, Packet Too Big, Time Exceeded, Parameter Problem, Echo Request/Reply, plus the same raw fallback. Neighbor Discovery (RFC 4861) and MLD (RFC 9777, which obsoletes RFC 3810) are explicitly out of scope and not yet tracked by an issue of their own. - New const/vendor registries: icmp.type and its three code registries, and the icmpv6 counterparts, as real IANA CSV crawlers (not hand-typed data), so a vendor-cron run picks up registry changes the way every other protocol's constants do. - Wire ICMP/IPv6_ICMP into Internet.__proto__, and delete the two 0-byte NotImplemented stubs. - docs/source/contributing/pep.rst: correct the ICMP/ICMPv6 follow-up prose (MLD's RFC citation, the Neighbor Discovery/NDP cross-reference, and a stale "wave 2 deliverable" claim about MLD elsewhere in the same file). - Update every registry-shape test the new entries touch: the const enum sweep-size pins, the dispatch registry's case count and PINNED_TARGETS, the package re-export and public-API surface checks, the vendor reachability and missing-body sweeps, and the handful of runtime/ integration tests whose protochain string literally said "IPv6_ICMP" where it now says "ICMPv6". Also moved an unrelated test fixture off IGMP's protocol number, which #2439 is about to register. - Cross-review round: checksum verification used to warn on genuinely valid traffic -- a first IP/IPv6 fragment, a short-snaplen capture, and an IPv6 Routing Header with Segments Left set -- by summing a slice of the message against a checksum computed over the whole thing; all three now skip verification instead of warning. ICMPv6 reached over IPv4 (protocol 58) used to crash the layer to Raw (IPv4Address fed to IPv6Address); it now skips verification the same way. inet_checksum's end-around-carry fold was hardcoded to two rounds, wrong for inputs around 131078+ octets; it now loops until no carry remains, per RFC 1071. Three docstrings claiming the checksum is never verified/computed are corrected, and icmpv6.py's MLD citation is brought in line with pep.rst's own RFC 9777 correction. - Cross-review round 3: a Mobile IPv6 node (RFC 6275 Section 11.3.1) checksums against its home address, not the wire-visible care-of address in ipv6.src; IPv6._decode_next_layer now threads the Destination Options Home Address option through __packet__['home_address'] for ICMPv6.read to use in its place. A malformed Routing header (wrong Hdr Ext Len) falls back to the generic IPv6_Ext parser, whose info has no seg_left key; info.get('seg_left', 0) silently read that as zero instead of "unknown", letting a non-zero Segments Left through unflagged. Segments Left is now read from the header's raw 4th octet whenever the structured parse did not leave it behind. - Cross-review round 6: two more instances of the same root cause, each found by a payload that reaches ICMP/ICMPv6 through a path the earlier flags never covered. (A) IP reassembly's Deferred(self.protocol.analyze, ...) threaded no packet context at all, so a reasm_strict=False datagram -- a zero-filled hole or a truncated prefix -- reached ICMP/ICMPv6 looking exactly like a complete message, including through a tunnel (4-in-4, 6-in-6) whose inner layer's own fresh parse has no way to know the outer datagram might be incomplete. (B) A malformed sibling option (e.g. a wrong-length Tunnel Encapsulation Limit) knocks IPv6_Opts into its generic IPv6_Ext fallback, whose info carries no options at all, silently losing a genuine Home Address option the same way round 3's Routing header bug lost Segments Left. Rather than add a sixth ad-hoc __packet__ key, the five this grew across five rounds (mf, route_seg_left, home_address, and two unnamed checks) are replaced by one ChecksumContext (pcapkit/protocols/internet/icmp_family.py): `incomplete` (sticky across tunnel nesting -- a fragment or a reassembly dispatch), `misrouted` (scoped to one layer -- a Routing header; keeping it separate from `incomplete` is what a regression caught mid-round -- conflating the two let an outer Routing header silence a genuinely corrupt inner checksum), and `src`/`dst` (never inherited by a nested layer, and may name ADDRESS_UNKNOWN when an address override was due but could not be recovered, as opposed to "no override is due"). ICMP and ICMPv6 now call the same verifiable_for_icmp()/verifiable_for_icmpv6() rather than reading ad-hoc flags apiece; fail-closed by construction and documented in one docstring instead of five scattered ones. Deferred gained an optional packet= parameter, purely additive (every existing call site omits it and sees no change) and still picklable under every protocol (#2423/#2425/#2435), so IP.submit can seed every reassembly dispatch already `incomplete`, unconditionally: that second parse never verifies a checksum of its own, complete datagram or not, since whichever fragments needed checking were already checked, with real context, on their own first pass through the frame. IPv6._home_address recovers a Destination Options Home Address by walking the raw option TLVs directly when the dedicated parser's fallback info has none -- the same move round 3 made for a Routing header's Segments Left -- and reports ADDRESS_UNKNOWN rather than guessing "no Home Address here" when the walk cannot complete cleanly. Build: coverage run -m pytest over the ICMP/ICMPv6/checksum unit tests plus tests/const, tests/vendor, tests/integration and tests/foundation (which exercises the reassembly dispatch and Deferred pickling) passes clean (1299 passed, 17 skipped, 28882 subtests, zero "invalid checksum" warnings on genuinely valid traffic).
JarryShaw
added a commit
that referenced
this pull request
Oct 30, 2026
Closes #2436. ICMP (RFC 792) and ICMPv6 (RFC 4443) had no implementation at all -- Internet.__proto__ had no entry for either protocol number, so a packet carrying one fell back to Raw with only the protocol number as a hint. - Add ICMP_Family between Internet and ICMP/ICMPv6, following the IP/IPsec precedent: nothing but id(). The base's own name is "_Family"-suffixed (matching IPv6_Route, IPv6_Ext) because, unlike IP/IPsec, the family's generic name collides with one of its own members (ICMP). - ICMP: Echo/Echo Reply, Destination Unreachable, Time Exceeded, Parameter Problem, plus a raw fallback for any other type. Own type registry (__type__, register_type), own schema. Both classes construct with no arguments (default body is the minimum valid length) and compute/verify their own checksum via a new shared pcapkit.utilities.checksum.inet_checksum (RFC 1071): ICMP sums the message alone, ICMPv6 additionally sums the IPv6 pseudo-header (RFC 2460 Section 8.1) when the enclosing IPv6 addresses are known; a bad checksum warns rather than raises. - ICMPv6: Destination Unreachable, Packet Too Big, Time Exceeded, Parameter Problem, Echo Request/Reply, plus the same raw fallback. Neighbor Discovery (RFC 4861) and MLD (RFC 9777, which obsoletes RFC 3810) are explicitly out of scope and not yet tracked by an issue of their own. - New const/vendor registries: icmp.type and its three code registries, and the icmpv6 counterparts, as real IANA CSV crawlers (not hand-typed data), so a vendor-cron run picks up registry changes the way every other protocol's constants do. - Wire ICMP/IPv6_ICMP into Internet.__proto__, and delete the two 0-byte NotImplemented stubs. - docs/source/contributing/pep.rst: correct the ICMP/ICMPv6 follow-up prose (MLD's RFC citation, the Neighbor Discovery/NDP cross-reference, and a stale "wave 2 deliverable" claim about MLD elsewhere in the same file). - Update every registry-shape test the new entries touch: the const enum sweep-size pins, the dispatch registry's case count and PINNED_TARGETS, the package re-export and public-API surface checks, the vendor reachability and missing-body sweeps, and the handful of runtime/ integration tests whose protochain string literally said "IPv6_ICMP" where it now says "ICMPv6". Also moved an unrelated test fixture off IGMP's protocol number, which #2439 is about to register. - Cross-review round: checksum verification used to warn on genuinely valid traffic -- a first IP/IPv6 fragment, a short-snaplen capture, and an IPv6 Routing Header with Segments Left set -- by summing a slice of the message against a checksum computed over the whole thing; all three now skip verification instead of warning. ICMPv6 reached over IPv4 (protocol 58) used to crash the layer to Raw (IPv4Address fed to IPv6Address); it now skips verification the same way. inet_checksum's end-around-carry fold was hardcoded to two rounds, wrong for inputs around 131078+ octets; it now loops until no carry remains, per RFC 1071. Three docstrings claiming the checksum is never verified/computed are corrected, and icmpv6.py's MLD citation is brought in line with pep.rst's own RFC 9777 correction. - Cross-review round 3: a Mobile IPv6 node (RFC 6275 Section 11.3.1) checksums against its home address, not the wire-visible care-of address in ipv6.src; IPv6._decode_next_layer now threads the Destination Options Home Address option through __packet__['home_address'] for ICMPv6.read to use in its place. A malformed Routing header (wrong Hdr Ext Len) falls back to the generic IPv6_Ext parser, whose info has no seg_left key; info.get('seg_left', 0) silently read that as zero instead of "unknown", letting a non-zero Segments Left through unflagged. Segments Left is now read from the header's raw 4th octet whenever the structured parse did not leave it behind. - Cross-review round 6: two more instances of the same root cause, each found by a payload that reaches ICMP/ICMPv6 through a path the earlier flags never covered. (A) IP reassembly's Deferred(self.protocol.analyze, ...) threaded no packet context at all, so a reasm_strict=False datagram -- a zero-filled hole or a truncated prefix -- reached ICMP/ICMPv6 looking exactly like a complete message, including through a tunnel (4-in-4, 6-in-6) whose inner layer's own fresh parse has no way to know the outer datagram might be incomplete. (B) A malformed sibling option (e.g. a wrong-length Tunnel Encapsulation Limit) knocks IPv6_Opts into its generic IPv6_Ext fallback, whose info carries no options at all, silently losing a genuine Home Address option the same way round 3's Routing header bug lost Segments Left. Rather than add a sixth ad-hoc __packet__ key, the five this grew across five rounds (mf, route_seg_left, home_address, and two unnamed checks) are replaced by one ChecksumContext (pcapkit/protocols/internet/icmp_family.py): `incomplete` (sticky across tunnel nesting -- a fragment or a reassembly dispatch), `misrouted` (scoped to one layer -- a Routing header; keeping it separate from `incomplete` is what a regression caught mid-round -- conflating the two let an outer Routing header silence a genuinely corrupt inner checksum), and `src`/`dst` (never inherited by a nested layer, and may name ADDRESS_UNKNOWN when an address override was due but could not be recovered, as opposed to "no override is due"). ICMP and ICMPv6 now call the same verifiable_for_icmp()/verifiable_for_icmpv6() rather than reading ad-hoc flags apiece; fail-closed by construction and documented in one docstring instead of five scattered ones. Deferred gained an optional packet= parameter, purely additive (every existing call site omits it and sees no change) and still picklable under every protocol (#2423/#2425/#2435), so IP.submit can seed every reassembly dispatch already `incomplete`, unconditionally: that second parse never verifies a checksum of its own, complete datagram or not, since whichever fragments needed checking were already checked, with real context, on their own first pass through the frame. IPv6._home_address recovers a Destination Options Home Address by walking the raw option TLVs directly when the dedicated parser's fallback info has none -- the same move round 3 made for a Routing header's Segments Left -- and reports ADDRESS_UNKNOWN rather than guessing "no Home Address here" when the walk cannot complete cleanly. - Cross-review round 7: round 6's own ChecksumContext fix had three further gaps, plus one unrelated pickling regression it introduced. (A) Eight extension-header parsers (AH, IPv6_Opts, IPv6_Route, HOPOPT, IPv6_Frag, MH, HIP, IPv6_Ext) dispatch to their own next layer with no packet= of their own whenever they are the outermost protocol rather than a header IPv6._decode_next_layer is itself walking, silently dropping ChecksumContext and leaving ICMP/ICMPv6 with a fresh, never-populated one -- reproduced by an AH-protected first IP fragment losing the fragment's own `incomplete` flag and warning on a checksum that only ever covered part of the message. Fixed structurally rather than at each of the eight sites: a new ambient `_checksum_context` contextvars.ContextVar (the same pattern `_frame_depth` already uses) is set by IPv4.read, IPv6.read and Deferred.__call__ around their own dispatch, and ChecksumContext.from_packet falls back to it whenever __packet__ carries no context of its own -- so any intervening layer, these eight or one written later with no knowledge of this feature at all, is covered without a fix of its own. (B) IP.submit seeded every reassembly dispatch's ChecksumContext as unconditionally `incomplete`, which went too far: a genuinely corrupt checksum spread across multiple fragments became unreachable everywhere, never checked on any non-first fragment's own pass (there isn't one) and no longer checked on the reassembled pass either, complete or not. It is now `incomplete` only while reassembly itself is still incomplete, or when at most one fragment contributed (already checked on its own direct first pass) -- a multi-fragment datagram that reassembled in whole is finally verified, on the only pass that was ever going to see the whole thing. (C) IPv6._home_address's raw-byte-scan fallback treated a Home Address option of any length other than 16 the same as no option at all, letting ICMPv6 verify against the wire source as if Mobile IPv6 were not in play; a present-but-wrong-length option now reports ADDRESS_UNKNOWN, same as the framing breaks it already handled. (D) Deferred.__call__ read self.packet directly, so a Deferred pickled before round 6's packet= parameter existed raised AttributeError on call despite that parameter's "purely additive" versionchanged note; it now reads packet through getattr with a None default. Build: the round's own command -- coverage run -m pytest over the ICMP/ ICMPv6/checksum unit tests, tests/foundation/reassembly, tests/const, tests/vendor and tests/integration -- passes clean (915 passed, 27918 subtests, zero "invalid checksum" warnings on genuinely valid traffic, and a genuinely corrupt reassembled checksum now warns). The full tests/protocols/internet (623 passed, 12530 subtests) and tests/foundation (580 passed, 17 skipped, 1858 subtests) directories, covering every prior round's own regression test by name, pass clean as well.
JarryShaw
added a commit
that referenced
this pull request
Oct 30, 2026
Closes #2436. ICMP (RFC 792) and ICMPv6 (RFC 4443) had no implementation at all -- Internet.__proto__ had no entry for either protocol number, so a packet carrying one fell back to Raw with only the protocol number as a hint. - Add ICMP_Family between Internet and ICMP/ICMPv6, following the IP/IPsec precedent: nothing but id(). The base's own name is "_Family"-suffixed (matching IPv6_Route, IPv6_Ext) because, unlike IP/IPsec, the family's generic name collides with one of its own members (ICMP). - ICMP: Echo/Echo Reply, Destination Unreachable, Time Exceeded, Parameter Problem, plus a raw fallback for any other type. Own type registry (__type__, register_type), own schema. Both classes construct with no arguments (default body is the minimum valid length) and compute/verify their own checksum via a new shared pcapkit.utilities.checksum.inet_checksum (RFC 1071): ICMP sums the message alone, ICMPv6 additionally sums the IPv6 pseudo-header (RFC 2460 Section 8.1) when the enclosing IPv6 addresses are known; a bad checksum warns rather than raises. - ICMPv6: Destination Unreachable, Packet Too Big, Time Exceeded, Parameter Problem, Echo Request/Reply, plus the same raw fallback. Neighbor Discovery (RFC 4861) and MLD (RFC 9777, which obsoletes RFC 3810) are explicitly out of scope and not yet tracked by an issue of their own. - New const/vendor registries: icmp.type and its three code registries, and the icmpv6 counterparts, as real IANA CSV crawlers (not hand-typed data), so a vendor-cron run picks up registry changes the way every other protocol's constants do. - Wire ICMP/IPv6_ICMP into Internet.__proto__, and delete the two 0-byte NotImplemented stubs. - docs/source/contributing/pep.rst: correct the ICMP/ICMPv6 follow-up prose (MLD's RFC citation, the Neighbor Discovery/NDP cross-reference, and a stale "wave 2 deliverable" claim about MLD elsewhere in the same file). - Update every registry-shape test the new entries touch: the const enum sweep-size pins, the dispatch registry's case count and PINNED_TARGETS, the package re-export and public-API surface checks, the vendor reachability and missing-body sweeps, and the handful of runtime/ integration tests whose protochain string literally said "IPv6_ICMP" where it now says "ICMPv6". Also moved an unrelated test fixture off IGMP's protocol number, which #2439 is about to register. - Cross-review round: checksum verification used to warn on genuinely valid traffic -- a first IP/IPv6 fragment, a short-snaplen capture, and an IPv6 Routing Header with Segments Left set -- by summing a slice of the message against a checksum computed over the whole thing; all three now skip verification instead of warning. ICMPv6 reached over IPv4 (protocol 58) used to crash the layer to Raw (IPv4Address fed to IPv6Address); it now skips verification the same way. inet_checksum's end-around-carry fold was hardcoded to two rounds, wrong for inputs around 131078+ octets; it now loops until no carry remains, per RFC 1071. Three docstrings claiming the checksum is never verified/computed are corrected, and icmpv6.py's MLD citation is brought in line with pep.rst's own RFC 9777 correction. - Cross-review round 3: a Mobile IPv6 node (RFC 6275 Section 11.3.1) checksums against its home address, not the wire-visible care-of address in ipv6.src; IPv6._decode_next_layer now threads the Destination Options Home Address option through __packet__['home_address'] for ICMPv6.read to use in its place. A malformed Routing header (wrong Hdr Ext Len) falls back to the generic IPv6_Ext parser, whose info has no seg_left key; info.get('seg_left', 0) silently read that as zero instead of "unknown", letting a non-zero Segments Left through unflagged. Segments Left is now read from the header's raw 4th octet whenever the structured parse did not leave it behind. - Cross-review round 6: two more instances of the same root cause, each found by a payload that reaches ICMP/ICMPv6 through a path the earlier flags never covered. (A) IP reassembly's Deferred(self.protocol.analyze, ...) threaded no packet context at all, so a reasm_strict=False datagram -- a zero-filled hole or a truncated prefix -- reached ICMP/ICMPv6 looking exactly like a complete message, including through a tunnel (4-in-4, 6-in-6) whose inner layer's own fresh parse has no way to know the outer datagram might be incomplete. (B) A malformed sibling option (e.g. a wrong-length Tunnel Encapsulation Limit) knocks IPv6_Opts into its generic IPv6_Ext fallback, whose info carries no options at all, silently losing a genuine Home Address option the same way round 3's Routing header bug lost Segments Left. Rather than add a sixth ad-hoc __packet__ key, the five this grew across five rounds (mf, route_seg_left, home_address, and two unnamed checks) are replaced by one ChecksumContext (pcapkit/protocols/internet/icmp_family.py): `incomplete` (sticky across tunnel nesting -- a fragment or a reassembly dispatch), `misrouted` (scoped to one layer -- a Routing header; keeping it separate from `incomplete` is what a regression caught mid-round -- conflating the two let an outer Routing header silence a genuinely corrupt inner checksum), and `src`/`dst` (never inherited by a nested layer, and may name ADDRESS_UNKNOWN when an address override was due but could not be recovered, as opposed to "no override is due"). ICMP and ICMPv6 now call the same verifiable_for_icmp()/verifiable_for_icmpv6() rather than reading ad-hoc flags apiece; fail-closed by construction and documented in one docstring instead of five scattered ones. Deferred gained an optional packet= parameter, purely additive (every existing call site omits it and sees no change) and still picklable under every protocol (#2423/#2425/#2435), so IP.submit can seed every reassembly dispatch already `incomplete`, unconditionally: that second parse never verifies a checksum of its own, complete datagram or not, since whichever fragments needed checking were already checked, with real context, on their own first pass through the frame. IPv6._home_address recovers a Destination Options Home Address by walking the raw option TLVs directly when the dedicated parser's fallback info has none -- the same move round 3 made for a Routing header's Segments Left -- and reports ADDRESS_UNKNOWN rather than guessing "no Home Address here" when the walk cannot complete cleanly. - Cross-review round 7: round 6's own ChecksumContext fix had three further gaps, plus one unrelated pickling regression it introduced. (A) Eight extension-header parsers (AH, IPv6_Opts, IPv6_Route, HOPOPT, IPv6_Frag, MH, HIP, IPv6_Ext) dispatch to their own next layer with no packet= of their own whenever they are the outermost protocol rather than a header IPv6._decode_next_layer is itself walking, silently dropping ChecksumContext and leaving ICMP/ICMPv6 with a fresh, never-populated one -- reproduced by an AH-protected first IP fragment losing the fragment's own `incomplete` flag and warning on a checksum that only ever covered part of the message. Fixed structurally rather than at each of the eight sites: a new ambient `_checksum_context` contextvars.ContextVar (the same pattern `_frame_depth` already uses) is set by IPv4.read, IPv6.read and Deferred.__call__ around their own dispatch, and ChecksumContext.from_packet falls back to it whenever __packet__ carries no context of its own -- so any intervening layer, these eight or one written later with no knowledge of this feature at all, is covered without a fix of its own. (B) IP.submit seeded every reassembly dispatch's ChecksumContext as unconditionally `incomplete`, which went too far: a genuinely corrupt checksum spread across multiple fragments became unreachable everywhere, never checked on any non-first fragment's own pass (there isn't one) and no longer checked on the reassembled pass either, complete or not. It is now `incomplete` only while reassembly itself is still incomplete, or when at most one fragment contributed (already checked on its own direct first pass) -- a multi-fragment datagram that reassembled in whole is finally verified, on the only pass that was ever going to see the whole thing. (C) IPv6._home_address's raw-byte-scan fallback treated a Home Address option of any length other than 16 the same as no option at all, letting ICMPv6 verify against the wire source as if Mobile IPv6 were not in play; a present-but-wrong-length option now reports ADDRESS_UNKNOWN, same as the framing breaks it already handled. (D) Deferred.__call__ read self.packet directly, so a Deferred pickled before round 6's packet= parameter existed raised AttributeError on call despite that parameter's "purely additive" versionchanged note; it now reads packet through getattr with a None default. - Cross-review round 10: round 9 confirmed the design closes every reachable path; mutation testing found Deferred.__call__'s own _checksum_context.set() -- unlike the equivalent set() in IPv4.read and IPv6.read -- had no regression test. Pinned with a new extract()-driven case: AH riding directly on IPv4 (no IPv6) wrapping a corrupt-checksum ICMP, reassembled as a single fragment -- the one reachable shape where AH's own _decode_next_layer drops packet= on its way to ICMP, making the Deferred's ambient-context seed the ICMP layer's only source of context. Confirmed to actually depend on the fix: temporarily removing the .set() call makes the new test catch the corrupt checksum twice instead of once. Build: the round's own command -- coverage run -m pytest over the ICMP/ ICMPv6/checksum unit tests, tests/foundation/reassembly, tests/const, tests/vendor and tests/integration -- passes clean (915 passed, 27918 subtests, zero "invalid checksum" warnings on genuinely valid traffic, and a genuinely corrupt reassembled checksum now warns). The full tests/protocols/internet (623 passed, 12530 subtests) and tests/foundation (580 passed, 17 skipped, 1858 subtests) directories, covering every prior round's own regression test by name, pass clean as well. Round 10's own command -- coverage run -m pytest over tests/foundation/reassembly, the ICMP/ICMPv6 roundtrip suites and the checksum unit tests -- passes clean as well (244 passed, 910 subtests).
JarryShaw
added a commit
that referenced
this pull request
Oct 30, 2026
Closes #2436. ICMP (RFC 792) and ICMPv6 (RFC 4443) had no implementation at all -- Internet.__proto__ had no entry for either protocol number, so a packet carrying one fell back to Raw with only the protocol number as a hint. - Add ICMP_Family between Internet and ICMP/ICMPv6, following the IP/IPsec precedent: nothing but id(). The base's own name is "_Family"-suffixed (matching IPv6_Route, IPv6_Ext) because, unlike IP/IPsec, the family's generic name collides with one of its own members (ICMP). - ICMP: Echo/Echo Reply, Destination Unreachable, Time Exceeded, Parameter Problem, plus a raw fallback for any other type. Own type registry (__type__, register_type), own schema. Both classes construct with no arguments (default body is the minimum valid length) and compute/verify their own checksum via a new shared pcapkit.utilities.checksum.inet_checksum (RFC 1071): ICMP sums the message alone, ICMPv6 additionally sums the IPv6 pseudo-header (RFC 2460 Section 8.1) when the enclosing IPv6 addresses are known; a bad checksum warns rather than raises. - ICMPv6: Destination Unreachable, Packet Too Big, Time Exceeded, Parameter Problem, Echo Request/Reply, plus the same raw fallback. Neighbor Discovery (RFC 4861) and MLD (RFC 9777, which obsoletes RFC 3810) are explicitly out of scope and not yet tracked by an issue of their own. - New const/vendor registries: icmp.type and its three code registries, and the icmpv6 counterparts, as real IANA CSV crawlers (not hand-typed data), so a vendor-cron run picks up registry changes the way every other protocol's constants do. - Wire ICMP/IPv6_ICMP into Internet.__proto__, and delete the two 0-byte NotImplemented stubs. - docs/source/contributing/pep.rst: correct the ICMP/ICMPv6 follow-up prose (MLD's RFC citation, the Neighbor Discovery/NDP cross-reference, and a stale "wave 2 deliverable" claim about MLD elsewhere in the same file). - Update every registry-shape test the new entries touch: the const enum sweep-size pins, the dispatch registry's case count and PINNED_TARGETS, the package re-export and public-API surface checks, the vendor reachability and missing-body sweeps, and the handful of runtime/ integration tests whose protochain string literally said "IPv6_ICMP" where it now says "ICMPv6". Also moved an unrelated test fixture off IGMP's protocol number, which #2439 is about to register. - Cross-review round: checksum verification used to warn on genuinely valid traffic -- a first IP/IPv6 fragment, a short-snaplen capture, and an IPv6 Routing Header with Segments Left set -- by summing a slice of the message against a checksum computed over the whole thing; all three now skip verification instead of warning. ICMPv6 reached over IPv4 (protocol 58) used to crash the layer to Raw (IPv4Address fed to IPv6Address); it now skips verification the same way. inet_checksum's end-around-carry fold was hardcoded to two rounds, wrong for inputs around 131078+ octets; it now loops until no carry remains, per RFC 1071. Three docstrings claiming the checksum is never verified/computed are corrected, and icmpv6.py's MLD citation is brought in line with pep.rst's own RFC 9777 correction. - Cross-review round 3: a Mobile IPv6 node (RFC 6275 Section 11.3.1) checksums against its home address, not the wire-visible care-of address in ipv6.src; IPv6._decode_next_layer now threads the Destination Options Home Address option through __packet__['home_address'] for ICMPv6.read to use in its place. A malformed Routing header (wrong Hdr Ext Len) falls back to the generic IPv6_Ext parser, whose info has no seg_left key; info.get('seg_left', 0) silently read that as zero instead of "unknown", letting a non-zero Segments Left through unflagged. Segments Left is now read from the header's raw 4th octet whenever the structured parse did not leave it behind. - Cross-review round 6: two more instances of the same root cause, each found by a payload that reaches ICMP/ICMPv6 through a path the earlier flags never covered. (A) IP reassembly's Deferred(self.protocol.analyze, ...) threaded no packet context at all, so a reasm_strict=False datagram -- a zero-filled hole or a truncated prefix -- reached ICMP/ICMPv6 looking exactly like a complete message, including through a tunnel (4-in-4, 6-in-6) whose inner layer's own fresh parse has no way to know the outer datagram might be incomplete. (B) A malformed sibling option (e.g. a wrong-length Tunnel Encapsulation Limit) knocks IPv6_Opts into its generic IPv6_Ext fallback, whose info carries no options at all, silently losing a genuine Home Address option the same way round 3's Routing header bug lost Segments Left. Rather than add a sixth ad-hoc __packet__ key, the five this grew across five rounds (mf, route_seg_left, home_address, and two unnamed checks) are replaced by one ChecksumContext (pcapkit/protocols/internet/icmp_family.py): `incomplete` (sticky across tunnel nesting -- a fragment or a reassembly dispatch), `misrouted` (scoped to one layer -- a Routing header; keeping it separate from `incomplete` is what a regression caught mid-round -- conflating the two let an outer Routing header silence a genuinely corrupt inner checksum), and `src`/`dst` (never inherited by a nested layer, and may name ADDRESS_UNKNOWN when an address override was due but could not be recovered, as opposed to "no override is due"). ICMP and ICMPv6 now call the same verifiable_for_icmp()/verifiable_for_icmpv6() rather than reading ad-hoc flags apiece; fail-closed by construction and documented in one docstring instead of five scattered ones. Deferred gained an optional packet= parameter, purely additive (every existing call site omits it and sees no change) and still picklable under every protocol (#2423/#2425/#2435), so IP.submit can seed every reassembly dispatch already `incomplete`, unconditionally: that second parse never verifies a checksum of its own, complete datagram or not, since whichever fragments needed checking were already checked, with real context, on their own first pass through the frame. IPv6._home_address recovers a Destination Options Home Address by walking the raw option TLVs directly when the dedicated parser's fallback info has none -- the same move round 3 made for a Routing header's Segments Left -- and reports ADDRESS_UNKNOWN rather than guessing "no Home Address here" when the walk cannot complete cleanly. - Cross-review round 7: round 6's own ChecksumContext fix had three further gaps, plus one unrelated pickling regression it introduced. (A) Eight extension-header parsers (AH, IPv6_Opts, IPv6_Route, HOPOPT, IPv6_Frag, MH, HIP, IPv6_Ext) dispatch to their own next layer with no packet= of their own whenever they are the outermost protocol rather than a header IPv6._decode_next_layer is itself walking, silently dropping ChecksumContext and leaving ICMP/ICMPv6 with a fresh, never-populated one -- reproduced by an AH-protected first IP fragment losing the fragment's own `incomplete` flag and warning on a checksum that only ever covered part of the message. Fixed structurally rather than at each of the eight sites: a new ambient `_checksum_context` contextvars.ContextVar (the same pattern `_frame_depth` already uses) is set by IPv4.read, IPv6.read and Deferred.__call__ around their own dispatch, and ChecksumContext.from_packet falls back to it whenever __packet__ carries no context of its own -- so any intervening layer, these eight or one written later with no knowledge of this feature at all, is covered without a fix of its own. (B) IP.submit seeded every reassembly dispatch's ChecksumContext as unconditionally `incomplete`, which went too far: a genuinely corrupt checksum spread across multiple fragments became unreachable everywhere, never checked on any non-first fragment's own pass (there isn't one) and no longer checked on the reassembled pass either, complete or not. It is now `incomplete` only while reassembly itself is still incomplete, or when at most one fragment contributed (already checked on its own direct first pass) -- a multi-fragment datagram that reassembled in whole is finally verified, on the only pass that was ever going to see the whole thing. (C) IPv6._home_address's raw-byte-scan fallback treated a Home Address option of any length other than 16 the same as no option at all, letting ICMPv6 verify against the wire source as if Mobile IPv6 were not in play; a present-but-wrong-length option now reports ADDRESS_UNKNOWN, same as the framing breaks it already handled. (D) Deferred.__call__ read self.packet directly, so a Deferred pickled before round 6's packet= parameter existed raised AttributeError on call despite that parameter's "purely additive" versionchanged note; it now reads packet through getattr with a None default. - Cross-review round 10: round 9 confirmed the design closes every reachable path; mutation testing found Deferred.__call__'s own _checksum_context.set() -- unlike the equivalent set() in IPv4.read and IPv6.read -- had no regression test. Pinned with a new extract()-driven case: AH riding directly on IPv4 (no IPv6) wrapping a corrupt-checksum ICMP, reassembled as a single fragment -- the one reachable shape where AH's own _decode_next_layer drops packet= on its way to ICMP, making the Deferred's ambient-context seed the ICMP layer's only source of context. Confirmed to actually depend on the fix: temporarily removing the .set() call makes the new test catch the corrupt checksum twice instead of once. Build: the round's own command -- coverage run -m pytest over the ICMP/ ICMPv6/checksum unit tests, tests/foundation/reassembly, tests/const, tests/vendor and tests/integration -- passes clean (915 passed, 27918 subtests, zero "invalid checksum" warnings on genuinely valid traffic, and a genuinely corrupt reassembled checksum now warns). The full tests/protocols/internet (623 passed, 12530 subtests) and tests/foundation (580 passed, 17 skipped, 1858 subtests) directories, covering every prior round's own regression test by name, pass clean as well. Round 10's own command -- coverage run -m pytest over tests/foundation/reassembly, the ICMP/ICMPv6 roundtrip suites and the checksum unit tests -- passes clean as well (244 passed, 910 subtests).
JarryShaw
added a commit
that referenced
this pull request
Oct 30, 2026
Closes #2436. ICMP (RFC 792) and ICMPv6 (RFC 4443) had no implementation at all -- Internet.__proto__ had no entry for either protocol number, so a packet carrying one fell back to Raw with only the protocol number as a hint. - Add ICMP_Family between Internet and ICMP/ICMPv6, following the IP/IPsec precedent: nothing but id(). The base's own name is "_Family"-suffixed (matching IPv6_Route, IPv6_Ext) because, unlike IP/IPsec, the family's generic name collides with one of its own members (ICMP). - ICMP: Echo/Echo Reply, Destination Unreachable, Time Exceeded, Parameter Problem, plus a raw fallback for any other type. Own type registry (__type__, register_type), own schema. Both classes construct with no arguments (default body is the minimum valid length) and compute/verify their own checksum via a new shared pcapkit.utilities.checksum.inet_checksum (RFC 1071): ICMP sums the message alone, ICMPv6 additionally sums the IPv6 pseudo-header (RFC 2460 Section 8.1) when the enclosing IPv6 addresses are known; a bad checksum warns rather than raises. - ICMPv6: Destination Unreachable, Packet Too Big, Time Exceeded, Parameter Problem, Echo Request/Reply, plus the same raw fallback. Neighbor Discovery (RFC 4861) and MLD (RFC 9777, which obsoletes RFC 3810) are explicitly out of scope and not yet tracked by an issue of their own. - New const/vendor registries: icmp.type and its three code registries, and the icmpv6 counterparts, as real IANA CSV crawlers (not hand-typed data), so a vendor-cron run picks up registry changes the way every other protocol's constants do. - Wire ICMP/IPv6_ICMP into Internet.__proto__, and delete the two 0-byte NotImplemented stubs. - docs/source/contributing/pep.rst: correct the ICMP/ICMPv6 follow-up prose (MLD's RFC citation, the Neighbor Discovery/NDP cross-reference, and a stale "wave 2 deliverable" claim about MLD elsewhere in the same file). - Update every registry-shape test the new entries touch: the const enum sweep-size pins, the dispatch registry's case count and PINNED_TARGETS, the package re-export and public-API surface checks, the vendor reachability and missing-body sweeps, and the handful of runtime/ integration tests whose protochain string literally said "IPv6_ICMP" where it now says "ICMPv6". Also moved an unrelated test fixture off IGMP's protocol number, which #2439 is about to register. - Cross-review round: checksum verification used to warn on genuinely valid traffic -- a first IP/IPv6 fragment, a short-snaplen capture, and an IPv6 Routing Header with Segments Left set -- by summing a slice of the message against a checksum computed over the whole thing; all three now skip verification instead of warning. ICMPv6 reached over IPv4 (protocol 58) used to crash the layer to Raw (IPv4Address fed to IPv6Address); it now skips verification the same way. inet_checksum's end-around-carry fold was hardcoded to two rounds, wrong for inputs around 131078+ octets; it now loops until no carry remains, per RFC 1071. Three docstrings claiming the checksum is never verified/computed are corrected, and icmpv6.py's MLD citation is brought in line with pep.rst's own RFC 9777 correction. - Cross-review round 3: a Mobile IPv6 node (RFC 6275 Section 11.3.1) checksums against its home address, not the wire-visible care-of address in ipv6.src; IPv6._decode_next_layer now threads the Destination Options Home Address option through __packet__['home_address'] for ICMPv6.read to use in its place. A malformed Routing header (wrong Hdr Ext Len) falls back to the generic IPv6_Ext parser, whose info has no seg_left key; info.get('seg_left', 0) silently read that as zero instead of "unknown", letting a non-zero Segments Left through unflagged. Segments Left is now read from the header's raw 4th octet whenever the structured parse did not leave it behind. - Cross-review round 6: two more instances of the same root cause, each found by a payload that reaches ICMP/ICMPv6 through a path the earlier flags never covered. (A) IP reassembly's Deferred(self.protocol.analyze, ...) threaded no packet context at all, so a reasm_strict=False datagram -- a zero-filled hole or a truncated prefix -- reached ICMP/ICMPv6 looking exactly like a complete message, including through a tunnel (4-in-4, 6-in-6) whose inner layer's own fresh parse has no way to know the outer datagram might be incomplete. (B) A malformed sibling option (e.g. a wrong-length Tunnel Encapsulation Limit) knocks IPv6_Opts into its generic IPv6_Ext fallback, whose info carries no options at all, silently losing a genuine Home Address option the same way round 3's Routing header bug lost Segments Left. Rather than add a sixth ad-hoc __packet__ key, the five this grew across five rounds (mf, route_seg_left, home_address, and two unnamed checks) are replaced by one ChecksumContext (pcapkit/protocols/internet/icmp_family.py): `incomplete` (sticky across tunnel nesting -- a fragment or a reassembly dispatch), `misrouted` (scoped to one layer -- a Routing header; keeping it separate from `incomplete` is what a regression caught mid-round -- conflating the two let an outer Routing header silence a genuinely corrupt inner checksum), and `src`/`dst` (never inherited by a nested layer, and may name ADDRESS_UNKNOWN when an address override was due but could not be recovered, as opposed to "no override is due"). ICMP and ICMPv6 now call the same verifiable_for_icmp()/verifiable_for_icmpv6() rather than reading ad-hoc flags apiece; fail-closed by construction and documented in one docstring instead of five scattered ones. Deferred gained an optional packet= parameter, purely additive (every existing call site omits it and sees no change) and still picklable under every protocol (#2423/#2425/#2435), so IP.submit can seed every reassembly dispatch already `incomplete`, unconditionally: that second parse never verifies a checksum of its own, complete datagram or not, since whichever fragments needed checking were already checked, with real context, on their own first pass through the frame. IPv6._home_address recovers a Destination Options Home Address by walking the raw option TLVs directly when the dedicated parser's fallback info has none -- the same move round 3 made for a Routing header's Segments Left -- and reports ADDRESS_UNKNOWN rather than guessing "no Home Address here" when the walk cannot complete cleanly. - Cross-review round 7: round 6's own ChecksumContext fix had three further gaps, plus one unrelated pickling regression it introduced. (A) Eight extension-header parsers (AH, IPv6_Opts, IPv6_Route, HOPOPT, IPv6_Frag, MH, HIP, IPv6_Ext) dispatch to their own next layer with no packet= of their own whenever they are the outermost protocol rather than a header IPv6._decode_next_layer is itself walking, silently dropping ChecksumContext and leaving ICMP/ICMPv6 with a fresh, never-populated one -- reproduced by an AH-protected first IP fragment losing the fragment's own `incomplete` flag and warning on a checksum that only ever covered part of the message. Fixed structurally rather than at each of the eight sites: a new ambient `_checksum_context` contextvars.ContextVar (the same pattern `_frame_depth` already uses) is set by IPv4.read, IPv6.read and Deferred.__call__ around their own dispatch, and ChecksumContext.from_packet falls back to it whenever __packet__ carries no context of its own -- so any intervening layer, these eight or one written later with no knowledge of this feature at all, is covered without a fix of its own. (B) IP.submit seeded every reassembly dispatch's ChecksumContext as unconditionally `incomplete`, which went too far: a genuinely corrupt checksum spread across multiple fragments became unreachable everywhere, never checked on any non-first fragment's own pass (there isn't one) and no longer checked on the reassembled pass either, complete or not. It is now `incomplete` only while reassembly itself is still incomplete, or when at most one fragment contributed (already checked on its own direct first pass) -- a multi-fragment datagram that reassembled in whole is finally verified, on the only pass that was ever going to see the whole thing. (C) IPv6._home_address's raw-byte-scan fallback treated a Home Address option of any length other than 16 the same as no option at all, letting ICMPv6 verify against the wire source as if Mobile IPv6 were not in play; a present-but-wrong-length option now reports ADDRESS_UNKNOWN, same as the framing breaks it already handled. (D) Deferred.__call__ read self.packet directly, so a Deferred pickled before round 6's packet= parameter existed raised AttributeError on call despite that parameter's "purely additive" versionchanged note; it now reads packet through getattr with a None default. - Cross-review round 10: round 9 confirmed the design closes every reachable path; mutation testing found Deferred.__call__'s own _checksum_context.set() -- unlike the equivalent set() in IPv4.read and IPv6.read -- had no regression test. Pinned with a new extract()-driven case: AH riding directly on IPv4 (no IPv6) wrapping a corrupt-checksum ICMP, reassembled as a single fragment -- the one reachable shape where AH's own _decode_next_layer drops packet= on its way to ICMP, making the Deferred's ambient-context seed the ICMP layer's only source of context. Confirmed to actually depend on the fix: temporarily removing the .set() call makes the new test catch the corrupt checksum twice instead of once. Build: the round's own command -- coverage run -m pytest over the ICMP/ ICMPv6/checksum unit tests, tests/foundation/reassembly, tests/const, tests/vendor and tests/integration -- passes clean (915 passed, 27918 subtests, zero "invalid checksum" warnings on genuinely valid traffic, and a genuinely corrupt reassembled checksum now warns). The full tests/protocols/internet (623 passed, 12530 subtests) and tests/foundation (580 passed, 17 skipped, 1858 subtests) directories, covering every prior round's own regression test by name, pass clean as well. Round 10's own command -- coverage run -m pytest over tests/foundation/reassembly, the ICMP/ICMPv6 roundtrip suites and the checksum unit tests -- passes clean as well (244 passed, 910 subtests).
JarryShaw
added a commit
that referenced
this pull request
Oct 30, 2026
Closes #2436. ICMP (RFC 792) and ICMPv6 (RFC 4443) had no implementation at all -- Internet.__proto__ had no entry for either protocol number, so a packet carrying one fell back to Raw with only the protocol number as a hint. - Add ICMP_Family between Internet and ICMP/ICMPv6, following the IP/IPsec precedent: nothing but id(). The base's own name is "_Family"-suffixed (matching IPv6_Route, IPv6_Ext) because, unlike IP/IPsec, the family's generic name collides with one of its own members (ICMP). - ICMP: Echo/Echo Reply, Destination Unreachable, Time Exceeded, Parameter Problem, plus a raw fallback for any other type. Own type registry (__type__, register_type), own schema. Both classes construct with no arguments (default body is the minimum valid length) and compute/verify their own checksum via a new shared pcapkit.utilities.checksum.inet_checksum (RFC 1071): ICMP sums the message alone, ICMPv6 additionally sums the IPv6 pseudo-header (RFC 2460 Section 8.1) when the enclosing IPv6 addresses are known; a bad checksum warns rather than raises. - ICMPv6: Destination Unreachable, Packet Too Big, Time Exceeded, Parameter Problem, Echo Request/Reply, plus the same raw fallback. Neighbor Discovery (RFC 4861) and MLD (RFC 9777, which obsoletes RFC 3810) are explicitly out of scope and not yet tracked by an issue of their own. - New const/vendor registries: icmp.type and its three code registries, and the icmpv6 counterparts, as real IANA CSV crawlers (not hand-typed data), so a vendor-cron run picks up registry changes the way every other protocol's constants do. - Wire ICMP/IPv6_ICMP into Internet.__proto__, and delete the two 0-byte NotImplemented stubs. - docs/source/contributing/pep.rst: correct the ICMP/ICMPv6 follow-up prose (MLD's RFC citation, the Neighbor Discovery/NDP cross-reference, and a stale "wave 2 deliverable" claim about MLD elsewhere in the same file). - Update every registry-shape test the new entries touch: the const enum sweep-size pins, the dispatch registry's case count and PINNED_TARGETS, the package re-export and public-API surface checks, the vendor reachability and missing-body sweeps, and the handful of runtime/ integration tests whose protochain string literally said "IPv6_ICMP" where it now says "ICMPv6". Also moved an unrelated test fixture off IGMP's protocol number, which #2439 is about to register. - Cross-review round: checksum verification used to warn on genuinely valid traffic -- a first IP/IPv6 fragment, a short-snaplen capture, and an IPv6 Routing Header with Segments Left set -- by summing a slice of the message against a checksum computed over the whole thing; all three now skip verification instead of warning. ICMPv6 reached over IPv4 (protocol 58) used to crash the layer to Raw (IPv4Address fed to IPv6Address); it now skips verification the same way. inet_checksum's end-around-carry fold was hardcoded to two rounds, wrong for inputs around 131078+ octets; it now loops until no carry remains, per RFC 1071. Three docstrings claiming the checksum is never verified/computed are corrected, and icmpv6.py's MLD citation is brought in line with pep.rst's own RFC 9777 correction. - Cross-review round 3: a Mobile IPv6 node (RFC 6275 Section 11.3.1) checksums against its home address, not the wire-visible care-of address in ipv6.src; IPv6._decode_next_layer now threads the Destination Options Home Address option through __packet__['home_address'] for ICMPv6.read to use in its place. A malformed Routing header (wrong Hdr Ext Len) falls back to the generic IPv6_Ext parser, whose info has no seg_left key; info.get('seg_left', 0) silently read that as zero instead of "unknown", letting a non-zero Segments Left through unflagged. Segments Left is now read from the header's raw 4th octet whenever the structured parse did not leave it behind. - Cross-review round 6: two more instances of the same root cause, each found by a payload that reaches ICMP/ICMPv6 through a path the earlier flags never covered. (A) IP reassembly's Deferred(self.protocol.analyze, ...) threaded no packet context at all, so a reasm_strict=False datagram -- a zero-filled hole or a truncated prefix -- reached ICMP/ICMPv6 looking exactly like a complete message, including through a tunnel (4-in-4, 6-in-6) whose inner layer's own fresh parse has no way to know the outer datagram might be incomplete. (B) A malformed sibling option (e.g. a wrong-length Tunnel Encapsulation Limit) knocks IPv6_Opts into its generic IPv6_Ext fallback, whose info carries no options at all, silently losing a genuine Home Address option the same way round 3's Routing header bug lost Segments Left. Rather than add a sixth ad-hoc __packet__ key, the five this grew across five rounds (mf, route_seg_left, home_address, and two unnamed checks) are replaced by one ChecksumContext (pcapkit/protocols/internet/icmp_family.py): `incomplete` (sticky across tunnel nesting -- a fragment or a reassembly dispatch), `misrouted` (scoped to one layer -- a Routing header; keeping it separate from `incomplete` is what a regression caught mid-round -- conflating the two let an outer Routing header silence a genuinely corrupt inner checksum), and `src`/`dst` (never inherited by a nested layer, and may name ADDRESS_UNKNOWN when an address override was due but could not be recovered, as opposed to "no override is due"). ICMP and ICMPv6 now call the same verifiable_for_icmp()/verifiable_for_icmpv6() rather than reading ad-hoc flags apiece; fail-closed by construction and documented in one docstring instead of five scattered ones. Deferred gained an optional packet= parameter, purely additive (every existing call site omits it and sees no change) and still picklable under every protocol (#2423/#2425/#2435), so IP.submit can seed every reassembly dispatch already `incomplete`, unconditionally: that second parse never verifies a checksum of its own, complete datagram or not, since whichever fragments needed checking were already checked, with real context, on their own first pass through the frame. IPv6._home_address recovers a Destination Options Home Address by walking the raw option TLVs directly when the dedicated parser's fallback info has none -- the same move round 3 made for a Routing header's Segments Left -- and reports ADDRESS_UNKNOWN rather than guessing "no Home Address here" when the walk cannot complete cleanly. - Cross-review round 7: round 6's own ChecksumContext fix had three further gaps, plus one unrelated pickling regression it introduced. (A) Eight extension-header parsers (AH, IPv6_Opts, IPv6_Route, HOPOPT, IPv6_Frag, MH, HIP, IPv6_Ext) dispatch to their own next layer with no packet= of their own whenever they are the outermost protocol rather than a header IPv6._decode_next_layer is itself walking, silently dropping ChecksumContext and leaving ICMP/ICMPv6 with a fresh, never-populated one -- reproduced by an AH-protected first IP fragment losing the fragment's own `incomplete` flag and warning on a checksum that only ever covered part of the message. Fixed structurally rather than at each of the eight sites: a new ambient `_checksum_context` contextvars.ContextVar (the same pattern `_frame_depth` already uses) is set by IPv4.read, IPv6.read and Deferred.__call__ around their own dispatch, and ChecksumContext.from_packet falls back to it whenever __packet__ carries no context of its own -- so any intervening layer, these eight or one written later with no knowledge of this feature at all, is covered without a fix of its own. (B) IP.submit seeded every reassembly dispatch's ChecksumContext as unconditionally `incomplete`, which went too far: a genuinely corrupt checksum spread across multiple fragments became unreachable everywhere, never checked on any non-first fragment's own pass (there isn't one) and no longer checked on the reassembled pass either, complete or not. It is now `incomplete` only while reassembly itself is still incomplete, or when at most one fragment contributed (already checked on its own direct first pass) -- a multi-fragment datagram that reassembled in whole is finally verified, on the only pass that was ever going to see the whole thing. (C) IPv6._home_address's raw-byte-scan fallback treated a Home Address option of any length other than 16 the same as no option at all, letting ICMPv6 verify against the wire source as if Mobile IPv6 were not in play; a present-but-wrong-length option now reports ADDRESS_UNKNOWN, same as the framing breaks it already handled. (D) Deferred.__call__ read self.packet directly, so a Deferred pickled before round 6's packet= parameter existed raised AttributeError on call despite that parameter's "purely additive" versionchanged note; it now reads packet through getattr with a None default. - Cross-review round 10: round 9 confirmed the design closes every reachable path; mutation testing found Deferred.__call__'s own _checksum_context.set() -- unlike the equivalent set() in IPv4.read and IPv6.read -- had no regression test. Pinned with a new extract()-driven case: AH riding directly on IPv4 (no IPv6) wrapping a corrupt-checksum ICMP, reassembled as a single fragment -- the one reachable shape where AH's own _decode_next_layer drops packet= on its way to ICMP, making the Deferred's ambient-context seed the ICMP layer's only source of context. Confirmed to actually depend on the fix: temporarily removing the .set() call makes the new test catch the corrupt checksum twice instead of once. Build: the round's own command -- coverage run -m pytest over the ICMP/ ICMPv6/checksum unit tests, tests/foundation/reassembly, tests/const, tests/vendor and tests/integration -- passes clean (915 passed, 27918 subtests, zero "invalid checksum" warnings on genuinely valid traffic, and a genuinely corrupt reassembled checksum now warns). The full tests/protocols/internet (623 passed, 12530 subtests) and tests/foundation (580 passed, 17 skipped, 1858 subtests) directories, covering every prior round's own regression test by name, pass clean as well. Round 10's own command -- coverage run -m pytest over tests/foundation/reassembly, the ICMP/ICMPv6 roundtrip suites and the checksum unit tests -- passes clean as well (244 passed, 910 subtests).
JarryShaw
added a commit
that referenced
this pull request
Oct 30, 2026
Closes #2436. ICMP (RFC 792) and ICMPv6 (RFC 4443) had no implementation at all -- Internet.__proto__ had no entry for either protocol number, so a packet carrying one fell back to Raw with only the protocol number as a hint. - Add ICMP_Family between Internet and ICMP/ICMPv6, following the IP/IPsec precedent: nothing but id(). The base's own name is "_Family"-suffixed (matching IPv6_Route, IPv6_Ext) because, unlike IP/IPsec, the family's generic name collides with one of its own members (ICMP). - ICMP: Echo/Echo Reply, Destination Unreachable, Time Exceeded, Parameter Problem, plus a raw fallback for any other type. Own type registry (__type__, register_type), own schema. Both classes construct with no arguments (default body is the minimum valid length) and compute/verify their own checksum via a new shared pcapkit.utilities.checksum.inet_checksum (RFC 1071): ICMP sums the message alone, ICMPv6 additionally sums the IPv6 pseudo-header (RFC 2460 Section 8.1) when the enclosing IPv6 addresses are known; a bad checksum warns rather than raises. - ICMPv6: Destination Unreachable, Packet Too Big, Time Exceeded, Parameter Problem, Echo Request/Reply, plus the same raw fallback. Neighbor Discovery (RFC 4861) and MLD (RFC 9777, which obsoletes RFC 3810) are explicitly out of scope and not yet tracked by an issue of their own. - New const/vendor registries: icmp.type and its three code registries, and the icmpv6 counterparts, as real IANA CSV crawlers (not hand-typed data), so a vendor-cron run picks up registry changes the way every other protocol's constants do. - Wire ICMP/IPv6_ICMP into Internet.__proto__, and delete the two 0-byte NotImplemented stubs. - docs/source/contributing/pep.rst: correct the ICMP/ICMPv6 follow-up prose (MLD's RFC citation, the Neighbor Discovery/NDP cross-reference, and a stale "wave 2 deliverable" claim about MLD elsewhere in the same file). - Update every registry-shape test the new entries touch: the const enum sweep-size pins, the dispatch registry's case count and PINNED_TARGETS, the package re-export and public-API surface checks, the vendor reachability and missing-body sweeps, and the handful of runtime/ integration tests whose protochain string literally said "IPv6_ICMP" where it now says "ICMPv6". Also moved an unrelated test fixture off IGMP's protocol number, which #2439 is about to register. - Cross-review round: checksum verification used to warn on genuinely valid traffic -- a first IP/IPv6 fragment, a short-snaplen capture, and an IPv6 Routing Header with Segments Left set -- by summing a slice of the message against a checksum computed over the whole thing; all three now skip verification instead of warning. ICMPv6 reached over IPv4 (protocol 58) used to crash the layer to Raw (IPv4Address fed to IPv6Address); it now skips verification the same way. inet_checksum's end-around-carry fold was hardcoded to two rounds, wrong for inputs around 131078+ octets; it now loops until no carry remains, per RFC 1071. Three docstrings claiming the checksum is never verified/computed are corrected, and icmpv6.py's MLD citation is brought in line with pep.rst's own RFC 9777 correction. - Cross-review round 3: a Mobile IPv6 node (RFC 6275 Section 11.3.1) checksums against its home address, not the wire-visible care-of address in ipv6.src; IPv6._decode_next_layer now threads the Destination Options Home Address option through __packet__['home_address'] for ICMPv6.read to use in its place. A malformed Routing header (wrong Hdr Ext Len) falls back to the generic IPv6_Ext parser, whose info has no seg_left key; info.get('seg_left', 0) silently read that as zero instead of "unknown", letting a non-zero Segments Left through unflagged. Segments Left is now read from the header's raw 4th octet whenever the structured parse did not leave it behind. - Cross-review round 6: two more instances of the same root cause, each found by a payload that reaches ICMP/ICMPv6 through a path the earlier flags never covered. (A) IP reassembly's Deferred(self.protocol.analyze, ...) threaded no packet context at all, so a reasm_strict=False datagram -- a zero-filled hole or a truncated prefix -- reached ICMP/ICMPv6 looking exactly like a complete message, including through a tunnel (4-in-4, 6-in-6) whose inner layer's own fresh parse has no way to know the outer datagram might be incomplete. (B) A malformed sibling option (e.g. a wrong-length Tunnel Encapsulation Limit) knocks IPv6_Opts into its generic IPv6_Ext fallback, whose info carries no options at all, silently losing a genuine Home Address option the same way round 3's Routing header bug lost Segments Left. Rather than add a sixth ad-hoc __packet__ key, the five this grew across five rounds (mf, route_seg_left, home_address, and two unnamed checks) are replaced by one ChecksumContext (pcapkit/protocols/internet/icmp_family.py): `incomplete` (sticky across tunnel nesting -- a fragment or a reassembly dispatch), `misrouted` (scoped to one layer -- a Routing header; keeping it separate from `incomplete` is what a regression caught mid-round -- conflating the two let an outer Routing header silence a genuinely corrupt inner checksum), and `src`/`dst` (never inherited by a nested layer, and may name ADDRESS_UNKNOWN when an address override was due but could not be recovered, as opposed to "no override is due"). ICMP and ICMPv6 now call the same verifiable_for_icmp()/verifiable_for_icmpv6() rather than reading ad-hoc flags apiece; fail-closed by construction and documented in one docstring instead of five scattered ones. Deferred gained an optional packet= parameter, purely additive (every existing call site omits it and sees no change) and still picklable under every protocol (#2423/#2425/#2435), so IP.submit can seed every reassembly dispatch already `incomplete`, unconditionally: that second parse never verifies a checksum of its own, complete datagram or not, since whichever fragments needed checking were already checked, with real context, on their own first pass through the frame. IPv6._home_address recovers a Destination Options Home Address by walking the raw option TLVs directly when the dedicated parser's fallback info has none -- the same move round 3 made for a Routing header's Segments Left -- and reports ADDRESS_UNKNOWN rather than guessing "no Home Address here" when the walk cannot complete cleanly. - Cross-review round 7: round 6's own ChecksumContext fix had three further gaps, plus one unrelated pickling regression it introduced. (A) Eight extension-header parsers (AH, IPv6_Opts, IPv6_Route, HOPOPT, IPv6_Frag, MH, HIP, IPv6_Ext) dispatch to their own next layer with no packet= of their own whenever they are the outermost protocol rather than a header IPv6._decode_next_layer is itself walking, silently dropping ChecksumContext and leaving ICMP/ICMPv6 with a fresh, never-populated one -- reproduced by an AH-protected first IP fragment losing the fragment's own `incomplete` flag and warning on a checksum that only ever covered part of the message. Fixed structurally rather than at each of the eight sites: a new ambient `_checksum_context` contextvars.ContextVar (the same pattern `_frame_depth` already uses) is set by IPv4.read, IPv6.read and Deferred.__call__ around their own dispatch, and ChecksumContext.from_packet falls back to it whenever __packet__ carries no context of its own -- so any intervening layer, these eight or one written later with no knowledge of this feature at all, is covered without a fix of its own. (B) IP.submit seeded every reassembly dispatch's ChecksumContext as unconditionally `incomplete`, which went too far: a genuinely corrupt checksum spread across multiple fragments became unreachable everywhere, never checked on any non-first fragment's own pass (there isn't one) and no longer checked on the reassembled pass either, complete or not. It is now `incomplete` only while reassembly itself is still incomplete, or when at most one fragment contributed (already checked on its own direct first pass) -- a multi-fragment datagram that reassembled in whole is finally verified, on the only pass that was ever going to see the whole thing. (C) IPv6._home_address's raw-byte-scan fallback treated a Home Address option of any length other than 16 the same as no option at all, letting ICMPv6 verify against the wire source as if Mobile IPv6 were not in play; a present-but-wrong-length option now reports ADDRESS_UNKNOWN, same as the framing breaks it already handled. (D) Deferred.__call__ read self.packet directly, so a Deferred pickled before round 6's packet= parameter existed raised AttributeError on call despite that parameter's "purely additive" versionchanged note; it now reads packet through getattr with a None default. - Cross-review round 10: round 9 confirmed the design closes every reachable path; mutation testing found Deferred.__call__'s own _checksum_context.set() -- unlike the equivalent set() in IPv4.read and IPv6.read -- had no regression test. Pinned with a new extract()-driven case: AH riding directly on IPv4 (no IPv6) wrapping a corrupt-checksum ICMP, reassembled as a single fragment -- the one reachable shape where AH's own _decode_next_layer drops packet= on its way to ICMP, making the Deferred's ambient-context seed the ICMP layer's only source of context. Confirmed to actually depend on the fix: temporarily removing the .set() call makes the new test catch the corrupt checksum twice instead of once. Build: the round's own command -- coverage run -m pytest over the ICMP/ ICMPv6/checksum unit tests, tests/foundation/reassembly, tests/const, tests/vendor and tests/integration -- passes clean (915 passed, 27918 subtests, zero "invalid checksum" warnings on genuinely valid traffic, and a genuinely corrupt reassembled checksum now warns). The full tests/protocols/internet (623 passed, 12530 subtests) and tests/foundation (580 passed, 17 skipped, 1858 subtests) directories, covering every prior round's own regression test by name, pass clean as well. Round 10's own command -- coverage run -m pytest over tests/foundation/reassembly, the ICMP/ICMPv6 roundtrip suites and the checksum unit tests -- passes clean as well (244 passed, 910 subtests).
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.
Please follow the guide below
make pylint,make mypy,make isort)make testpasses, and a test case covers the changedocs/source/changelog/and regeneratedCHANGELOG.mdWhat is the purpose of your pull request?
fix— corrects a defectDescription of your pull request and other information
Closes #2433.
Deferredstill failed to pickle under protocol 0/1 on GraalPy 25.4 even after #2424's__getstate__/__setstate__fix, withTypeError: typing.Generic() takes no argumentsfromcopyreg._reduce_ex.Root cause: for protocol < 2,
copyreg._reduce_exwalkscls.__mro__for a base to compute a base-object state from, before__getstate__is ever consulted. On CPython,Genericis a heap type whose__new__is just the inherited, differently-boundobject.__new__, so the walk passes it and stops atobject(state=None). On GraalPy,Genericis itself a non-heap (built-in) type, so the walk's first check (not base.__flags__ & _HEAPTYPE) breaks onGenericdirectly and callsGeneric(self), which raises.__getstate__/__setstate__never run in that failure, since the crash happens before the walk's result is combined with them.Fix:
Deferred.__reduce_ex__hard-codes thebase=object, state=Noneoutcome the walk always reached on CPython for protocol 0/1, bypassingcopyreg._reduce_ex's MRO walk (andGenericwith it) entirely. It also checks for, and defers to, a subclass's own__reduce__override before building that hard-coded tuple — a regression caught in cross-review: without the check, aDeferredsubclass overriding__reduce__had it silently dropped at protocol 0/1 while protocol 2+ (viaobject.__reduce_ex__, unchanged) kept honouring it. Protocol >= 2 is otherwise untouched. DroppingGeneric[_KT]instead was considered and rejected:test_deferred_proto_type_2417_unit.pyreadsDeferred.__parameters__at runtime, which only exists becauseGenericis a real base, notTYPE_CHECKING-only.Confirmed on real GraalPy 25.4.4 by cross-review:
test_deferred_pickle_2423_unit.pygoes from 8 failed onmainto 16 passed/18 subtests on this fix, and the fulltests/foundation/reassembly/suite passes 173/854 subtests, 0 failed, on GraalPy. A new regression test (test_deferred_pickle_graalpy_2433_unit.py) reproduces the failure mode locally by monkeypatchingcopyreg._reduce_exto raise the sameTypeError, and a second new test pins the subclass-__reduce__fix.CPython: mypy 98/32 (baseline, unchanged), pylint 10.00/10, isort clean,
tests/foundation/reassembly(176/890 subtests, including the two new regression tests) andtests/corekit(693/6767 subtests, unaffected by this change) pass, 100% coverage on the changed file.