Repository navigation
fix(ipv6): parse unrecognised extension headers generically (#891) - #904
Conversation
|
CI is red on four legs — Python 3.11, 3.12, 3.13, 3.14 — and it is essentially one defect, not four. Run The dominant cause: Both Second, independent: Third, and I believe pre-existing rather than caused here: two Note the first two are genuine and would have been caught locally: the public-API sweep and |
|
NEEDS CHANGES on 1. Codes 147, 253 and 254 still collapse the whole IPv6 packet. The #891 defect survives for 3 of the 12 IANA codes — and the docstrings call that benign.
Where this came from: my exception table. I wrote " @JarryShaw — your ruling was given on that false premise. You wrote " 2. 3. The registration leaks out of IPv6 and changes IPv4 parsing — a second breaking change the PR does not declare. 4. New Sphinx warning. 5. Confirmed good, and some of it strongly. The core fix works and the new assertion is the truth rather than code-shaped — no And the author was right to push back on my Shim6 instruction. I told it to remove Shim6 from tables and docstrings; it kept the wire-format row and disclaimed the parser separately at |
4581fa0 to
9a4b0b8
Compare
|
Both rounds fixed on The structural fix works, and it is genuinely structural. Against The IPv4 leak is closed.
Why a re-review rather than my sign-off. This is a 503-line new protocol class plus a change to the extension-header walk itself, fixed across two rounds of five and six items. And the author reports catching its own regression mid-fix: moving So the re-review's first job is a chain walk per header — Label moved to Also worth recording: the author corrected its own earlier diffstat, which I had to publicly flag as unreliable. It now reports |
|
CI is red on three legs (Python 3.10, 3.11, 3.13; run The test is behaving correctly and its own message says what to do. From What needs updating in
Update the pins with a comment naming this issue, so the next reader sees a deliberate change rather than a loosened assertion. A bare This is a genuinely good test and worth saying so: it detected a registry shape change from a PR that touches neither the registry file nor the test, and refused to pass silently. Exactly the class of drift that #899's census break and #909's index duplication were both about. Sent to the author. The re-review in flight has been told, since its chain-walk task and this overlap — both turn on what protocol 140 now resolves to. |
|
NEEDS CHANGES on The headline first, because it is the thing I could not check myself. Every chain that worked on So the And the fix is bigger than I framed it. I described it as three codes plus the IPv4 leak. D1 — dead code at D2 — the design prose is factually wrong about ESP, in four places, and the error invites a dangerous change. Verified myself: ESP has a dedicated, registered parser at Why this blocks rather than being a nit: excluding ESP from Also stale: the PR body still writes Verified clean: the The One correction to my own brief worth flagging publicly, since this author's self-reported numbers have been quoted before: its claim of "3 pylint findings pre-existing with shifted line numbers" does not reproduce. Sent to the author together with the CI fix. |
- Added IPv6_GenericExt, a generic RFC 6564 §4 parser for IPv6 extension headers: it reads the guaranteed next-header/Hdr-Ext-Len octets and derives next/length/protocol from the actual parse, applying AH's and IPv6-Frag's own length rules where they diverge. Gated to version==6, since it is registered into the Internet-wide dispatch table for Shim6 and would otherwise also activate for an IPv4 payload carrying protocol byte 140. - IPv6._decode_next_layer now stops its walk structurally -- on any layer whose parsed info carries no `next` field -- instead of reading `info.next` unconditionally and crashing the whole packet when it hits ESP, BIT-EMU, 253, 254, or any other IANA code with no dedicated parser (the actual #891 defect). IPv6._import_next_layer substitutes IPv6_GenericExt for a recognised header whose own parser raises. - IPv6_GenericExt.alias is hyphenated ('IPv6-GenericExt'), matching its siblings, so IPv6's `alias.lstrip('IPv6-')` keys the packet dict as 'genericext' rather than '_genericext'. - An overrun warns in the house SchemaWarning convention, then stops the walk rather than clipping: the length also decides where the next header starts, so a clipped skip distance would fabricate a layer. - Exported IPv6_GenericExt from the pcapkit, pcapkit.protocols, pcapkit.protocols.data and pcapkit.protocols.schema aggregators. - Re-pointed the #889 test pinning the pre-fix whole-packet collapse, and added coverage for both dispatch paths, the AH/Frag rules, the overrun stop, the version gate, and an unimplemented terminal code (BIT-EMU). Build/test: tests/protocols/internet/ (260 passed, 868 subtests) and tests/project/{test_public_api,test_isort_clean,test_module_isolation}.py via the worktree's own pcapkit; coverage on the touched files rose from 81% to 94% with the new tests added. `make isort` clean; the two remaining mypy findings and the pylint findings pre-exist on main.
978b993 to
de7177f
Compare
|
GOOD TO GO on The defect my own exception table caused is fixed structurally, and I measured it rather than reading the diff. Against The three round-three items, each checked:
It also found something my brief did not anticipate, and this is the part worth recording. Pinning the registry target made
Carried forward from the previous review, which remains the strongest evidence here: the chain walk across 19 chains showed every chain that worked on Ready for you to merge. I am not merging. |
|
Wait, we decided the name to be IPv6_Ext and used as a base class for all IPv6 extension headers. See #891 for the full conversion and decision. |
|
You are right about the decision, and #904 is deliberately not it — by a split recorded on #891 and never contested. Quoting that thread:
and, in the same thread's last planning comment:
So both names were meant to coexist: Measured: #904 matches its siblings exactly and introduces no inconsistency. My recommendation: merge #904 as it stands, and I file the Filing it also unblocks #895, which is If you would rather have the base first, say so and I will hold #904 and dispatch the refactor instead — but then |
|
My initial thought was to directly rename IPv6_GenericExt to IPv6_Ext, and use it as the base class and the unknown extension header's fallback parser. |
|
Measured your proposal, and the obstacle I expected is not there — one class can do both jobs. Recording the mistake first: I started from "its Every one defines it on itself, so a raising So the real cost is narrower than a design clash: it is the loss of a safety net for anything added later. A future extension header that forgets to override inherits concrete values and silently reports itself as "Generic IPv6 Extension Header" with alias My recommendation: do it your way — rename to Where: a follow-up, not this PR. #904 is reviewed, verified and mergeable at Still |
|
Go |
|
Ruling recorded, verbatim:
Taken as approving the plan in my last comment: merge #904 as it stands, and the rename lands as its own change rather than being folded in here. Filed as #917, labelled #917 carries the whole design so none of it is rediscovered: the rename to It is gh pr view 904 -R JarryShaw/PyPCAPKit --json state -q .state # MERGEDRemoving |
…up (#877) Phase 2 of #877, the 11 classes across 8 files not held by #913/#904: TransportProtocol, FinalisedState, Completion, ftp.Type, httpv1.Type, Criticality, PDUKind, PacketDirection, PacketReception, WireGuardKeyLabel, and FrameType.Flags (carrying its 6 per-frame subclasses transitively). Each now mixes in EnumLookup ahead of its enum base for the shared get/get_all contract. TransportProtocol and Criticality already had their own get, both as a staticmethod against EnumLookup.get's classmethod (the #908/#915 trap). Both are now classmethods delegating to super().get(), keeping only what the base does not reproduce -- TransportProtocol's case-fold and no-mint refusal, Criticality's case-sensitive miss -- each re-raised as the ValueError callers already depend on rather than the base's KeyError. Each gained a default parameter forwarded to the base, since dropping one the base declares is a real classmethod-override violation under mypy. Updated test_const_enum_get.py's exclusion set for TransportProtocol's new default, and added test_enum_lookup_reparent_877_unit.py pinning the re-parenting, both preserved overrides, and no member-table growth.
…up (#877) Phase 2 of #877, the 11 classes across 8 files not held by #913/#904: TransportProtocol, FinalisedState, Completion, ftp.Type, httpv1.Type, Criticality, PDUKind, PacketDirection, PacketReception, WireGuardKeyLabel, and FrameType.Flags (carrying its 6 per-frame subclasses transitively). Each now mixes in EnumLookup ahead of its enum base for the shared get/get_all contract. TransportProtocol and Criticality already had their own get, both as a staticmethod against EnumLookup.get's classmethod (the #908/#915 trap). Both are now classmethods delegating to super().get(), keeping only what the base does not reproduce -- TransportProtocol's case-fold and no-mint refusal, Criticality's case-sensitive miss -- each re-raised as the ValueError callers already depend on rather than the base's KeyError. Each gained a default parameter forwarded to the base, since dropping one the base declares is a real classmethod-override violation under mypy. Updated test_const_enum_get.py's exclusion set for TransportProtocol's new default, and added test_enum_lookup_reparent_877_unit.py pinning the re-parenting, both preserved overrides, and no member-table growth.
…up (#877) (#921) Phase 2 of #877, the 11 classes across 8 files not held by #913/#904: TransportProtocol, FinalisedState, Completion, ftp.Type, httpv1.Type, Criticality, PDUKind, PacketDirection, PacketReception, WireGuardKeyLabel, and FrameType.Flags (carrying its 6 per-frame subclasses transitively). Each now mixes in EnumLookup ahead of its enum base for the shared get/get_all contract. TransportProtocol and Criticality already had their own get, both as a staticmethod against EnumLookup.get's classmethod (the #908/#915 trap). Both are now classmethods delegating to super().get(), keeping only what the base does not reproduce -- TransportProtocol's case-fold and no-mint refusal, Criticality's case-sensitive miss -- each re-raised as the ValueError callers already depend on rather than the base's KeyError. Each gained a default parameter forwarded to the base, since dropping one the base declares is a real classmethod-override violation under mypy. Updated test_const_enum_get.py's exclusion set for TransportProtocol's new default, and added test_enum_lookup_reparent_877_unit.py pinning the re-parenting, both preserved overrides, and no member-table growth.
… onto EnumLookup (#930) Finishes #877's phase 2, which #921 left seven classes out of because their files were held by #913/#904 at the time: CommandType and ConformanceRequirement (const/ftp/command.py), ESPStatus (protocols/internet/esp.py), and FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode, LMAAddressCode and LocalizedRoutingStatus (protocols/internet/mh.py). Both blockers have since merged. Each now mixes in EnumLookup ahead of its enum base; member-table sizes are unchanged. FastBindingAcknowledgmentStatus and IPv6AddressPrefixCode kept their own get, still a staticmethod that never calls super() -- left untouched, since #923's EnumKeyError name-miss conversion already matches the base's shape. mypy's [override] and pylint's arguments-differ against the kept decorator are suppressed rather than resolved by widening it. The other five are pure re-parents. Brought conventions.rst and its own doc-claims test in line with #929, which merged in the interim: phase 2 is now 24 of 24, zero enumerations outside the hierarchy. Added test_enum_lookup_reparent_930_unit.py pinning the re-parenting, the kept overrides, no growth, and the zero-outside census; fixed three tests whose claims this change made stale (test_const_enum_get, test_const_ftp_featcode_case_903_unit, test_mh_unit). Build: mypy/pylint/isort clean against baseline; affected test files pass.
… onto EnumLookup (#930) Finishes #877's phase 2, which #921 left seven classes out of because their files were held by #913/#904 at the time: CommandType and ConformanceRequirement (const/ftp/command.py), ESPStatus (protocols/internet/esp.py), and FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode, LMAAddressCode and LocalizedRoutingStatus (protocols/internet/mh.py). Both blockers have since merged. Each now mixes in EnumLookup ahead of its enum base; member-table sizes are unchanged. FastBindingAcknowledgmentStatus and IPv6AddressPrefixCode kept their own get, still a staticmethod that never calls super() -- left untouched, since #923's EnumKeyError name-miss conversion already matches the base's shape. mypy's [override] and pylint's arguments-differ against the kept decorator are suppressed rather than resolved by widening it. The other five are pure re-parents. Brought conventions.rst and its own doc-claims test in line with #929, which merged in the interim: phase 2 is now 24 of 24, zero enumerations outside the hierarchy. Added test_enum_lookup_reparent_930_unit.py pinning the re-parenting, the kept overrides, no growth, and the zero-outside census; fixed three tests whose claims this change made stale (test_const_enum_get, test_const_ftp_featcode_case_903_unit, test_mh_unit). Build: mypy/pylint/isort clean against baseline; affected test files pass.
… onto EnumLookup (#930) Finishes #877's phase 2, which #921 left seven classes out of because their files were held by #913/#904 at the time: CommandType and ConformanceRequirement (const/ftp/command.py), ESPStatus (protocols/internet/esp.py), and FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode, LMAAddressCode and LocalizedRoutingStatus (protocols/internet/mh.py). Both blockers have since merged. Each now mixes in EnumLookup ahead of its enum base; member-table sizes are unchanged. FastBindingAcknowledgmentStatus and IPv6AddressPrefixCode kept their own get, still a staticmethod that never calls super() -- left untouched, since #923's EnumKeyError name-miss conversion already matches the base's shape. mypy's [override] and pylint's arguments-differ against the kept decorator are suppressed rather than resolved by widening it. The other five are pure re-parents. Brought conventions.rst and its own doc-claims test in line with #929, which merged in the interim: phase 2 is now 24 of 24, zero enumerations outside the hierarchy. Added test_enum_lookup_reparent_930_unit.py pinning the re-parenting, the kept overrides, no growth, and the zero-outside census; fixed three tests whose claims this change made stale (test_const_enum_get, test_const_ftp_featcode_case_903_unit, test_mh_unit). Build: mypy/pylint/isort clean against baseline; affected test files pass.
… onto EnumLookup (#932) Finishes #877's phase 2, which #921 left seven classes out of because their files were held by #913/#904 at the time: CommandType and ConformanceRequirement (const/ftp/command.py), ESPStatus (protocols/internet/esp.py), and FastBindingAcknowledgmentStatus, IPv6AddressPrefixCode, LMAAddressCode and LocalizedRoutingStatus (protocols/internet/mh.py). Both blockers have since merged. Each now mixes in EnumLookup ahead of its enum base; member-table sizes are unchanged. FastBindingAcknowledgmentStatus and IPv6AddressPrefixCode kept their own get, still a staticmethod that never calls super() -- left untouched, since #923's EnumKeyError name-miss conversion already matches the base's shape. mypy's [override] and pylint's arguments-differ against the kept decorator are suppressed rather than resolved by widening it. The other five are pure re-parents. Brought conventions.rst and its own doc-claims test in line with #929, which merged in the interim: phase 2 is now 24 of 24, zero enumerations outside the hierarchy. Added test_enum_lookup_reparent_930_unit.py pinning the re-parenting, the kept overrides, no growth, and the zero-outside census; fixed three tests whose claims this change made stale (test_const_enum_get, test_const_ftp_featcode_case_903_unit, test_mh_unit). Build: mypy/pylint/isort clean against baseline; affected test files pass.
Please follow the guide below
make pylint,make mypy,make isort)make testpasses, and a test case covers the changedocs/source/changelog/-- N/A -- changelog centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657What is the purpose of your pull request?
fix— corrects a defectDescription of your pull request and other information
Fixes #891: a failed or unrecognised IPv6 extension header used to collapse the whole packet to
Raw(proto = info.nexton aRawfallback has no.next). AddsIPv6_GenericExt, a generic RFC 6564 §4 parser for the closed set of extension headers whose wire format is guaranteed (AH/IPv6-Fraguse their own length rule).IPv6._import_next_layersubstitutes it for a recognised header whose own parser raises, andShim6(no dedicated parser at all) now dispatches to it directly instead of defaulting toRaw.IPv6._decode_next_layer's walk now stops structurally -- on any layer whose parsed info carries nonextattribute -- rather than readinginfo.nextunconditionally: this is what fixesBIT-EMU/253/254 (no dedicated parser, so they resolve to plainRaw) without naming them, and keeps the packet's own header intact instead of losing it.ESPis unaffected either way: it has its own dedicated parser, and that parser's info always carries anext(None, since RFC 4303 encrypts the real value), so the walk already ended cleanly after it. An overrun warns (houseSchemaWarningconvention) then stops the walk rather than clipping, since a clipped skip distance would fabricate a layer from trailing bytes.Breaking:
protochainfor an affected packet gains a real segment instead of collapsing (e.g.Ethernet:IPv6:IPv6-GenericExt:UDPinstead ofEthernet:Internet_Protocol_version_6). The #889 test pinning the old collapse is re-pointed at the corrected chain.