Repository navigation
fix(vendor): source ExtensionHeader from the extension-header registry, not the protocol-numbers flag (#925) - #926
Conversation
JarryShaw
added
fix
|
PR #926 is up but incomplete, and I have sent it back rather than labelling it ready. Recording why, because the reason is a genuine finding. The worker correctly found a dependency and stopped: But the conclusion does not follow, and the PR as shaped is a landmine. It repoints the vendor module at the 11-entry registry and leaves the const file at 12, so nothing breaks today — and the next person to run the generator silently loses The dependency is on an arbitrary example, not on 147. That test's subject is #891/#904's behaviour — an unimplemented code must stop the walk while keeping the IPv6 header intact — and its own docstring says so:
Measured on So switching the example to 253 exercises the identical code path on a code IANA actually registers, and frees the const file to drop to 11. Sent back to do that, invert the const-side guard (pin 11 members and Labelled |
…egistry, drop BIT_EMU (#925) `pcapkit.vendor.ipv6.extension_header.ExtensionHeader` crawled the *Protocol Numbers* registry (`protocol-numbers-1.csv`), filtered on its `IPv6 Extension Header` column -- a derived signal, not the registry RFC 8200 s4 names authoritative for this enumeration. That registry disagreed with the authoritative *IPv6 Extension Header Types* registry (`ipv6-parameters/extension-header.csv`) on header 147 (`BIT-EMU`): the former flagged it `Y`, citing RFC 9801; the latter omits it entirely. - Point `LINK` at `extension-header.csv` and rewrite `count`/`process` for its 3-column shape (`Protocol Number,Description,Reference` -- no flag to filter on). - That registry has no short `Keyword` column, unlike the old one, so add a `NAMES` override for the 8 members the old registry gave a keyword-derived name (`HOPOPT`, `IPv6-Route`, `IPv6-Frag`, `ESP`, `AH`, `IPv6-Opts`, `HIP`, `Shim6`) -- deriving names straight from the verbose Description column instead would silently rename all eight, which is worse than the registry defect being fixed. - Drop `BIT_EMU` from `pcapkit.const.ipv6.extension_header`, taking it to 11 members -- hand-applied to match what the fixed crawler produces from the fixture below byte-for-byte (proven by a new test, not merely asserted). - `IPv6._decode_next_layer`'s walk resolves `ExtensionHeader(proto)` at the top of its loop and used to rely on 147 succeeding there. `test_ipv6_ext_unit.py`'s `test_unimplemented_terminal_code_stops_ the_walk_not_the_packet` picked 147 only as an *example* of an unimplemented extension-header code (its own docstring: "253/254 are the same code path"); switched it to 253, which is actually in the authoritative registry, so the test exercises the identical code path without depending on a member being removed. Updated its docstring and the prose in `ipv6.py`/`ipv6_ext.py` that described BIT_EMU as one of the codes reachable through the walk/generic extractor -- it no longer is, by construction, since it is not an `ExtensionHeader` member at all any more. - `pcapkit.const.reg.transtype.TransType.BIT_EMU` is untouched: a different, correctly-sourced enumeration (147 legitimately belongs in the Protocol Numbers registry). New unit tests: the vendor suite feeds the issue's fixture CSV directly to the crawler (no network) and pins LINK, all 11 names/ values, BIT_EMU's absence, which comments genuinely diverge from history because the two registries carry different Reference text, and -- the seam between the two halves of this fix -- that `ExtensionHeader.context()` run against the fixture reproduces the committed const file byte-for-byte. The const suite pins the member count, that `ExtensionHeader(147)` now raises, that `TransType.BIT_EMU` is unaffected, and that the IPv6 walk still stops cleanly (no crash, header fields intact, no extension header recorded) on a next-header byte of 147 now that nothing maps it. Build: `python -m unittest` green on both new suites, the full `test_ipv6_ext_unit`, `test_const_registry_protocol` and `test_isort_clean`. A real vendor regeneration was not run against the live registry -- network crawls are disallowed in this environment -- so the committed const file is a hand-applied stand-in for that regeneration's output, verified only against the fixture text this issue's own investigation fetched. The owner should run the crawler for real once to confirm the live registry still matches that fixture.
172f50a to
329067a
Compare
|
GOOD TO GO at The "run the regeneration once" caveat is discharged. The reviewer drove the fixed crawler against the live registry it fetched itself and got a byte-for-byte match with the committed const file. I confirmed the chain independently: So live registry → fixture → committed file, and the fixture-to-file leg is a passing test rather than a claim. The only thing still unexercised is A correction to what I wrote when I sent this back. I said 253 exercises the same That does not weaken the substitution — 147 was also a member before, so it took the identical structural path, and the test's own coverage is unchanged. But the reviewer's mutation testing shows the PR ends up covering both guards where one test covered one: removing the structural guard breaks the 253 test with the verbatim #891 signature 147 now behaves better, not merely differently. With the member gone it dispatches as an ordinary upper-layer protocol through Scope discipline, checked by AST rather than by eye — my own comparison of Also confirmed: the Three non-blocking nits recorded, none needing a revision:
|
JarryShaw
added
review: good-to-go
JarryShaw
removed
the
review: good-to-go
…otocol-numbers-1.csv (#931) `ESP` and `AH` justified their double inheritance by citing `protocol-numbers-1.csv`'s *IPv6 Extension Header* column -- the derived signal #926 repointed the const generator away from, precisely because it disagreed with the authoritative registry (it flagged BIT-EMU/147 as an extension header; the real registry omits it). - esp.py:979-982 -- now cites IANA's *IPv6 Extension Header Types* registry (ESP = 50) and RFC 4303 section 3.1.1's IPv6-context sentence -- "should appear after hop-by-hop, routing, and fragmentation extension headers" -- which supports IPv6 extension-header membership, the claim being made. - ah.py:55-58 -- same fix, citing RFC 4302 section 3.1.1's identical IPv6-context sentence for AH. - Kept the second half of each sentence (that the package's own ExtensionHeader registry agrees), now a check against the authoritative source rather than corroboration of the wrong one. - Checked hip.py for the same stale citation; it already cites the extension-header registry and RFC 7401, so no change needed there. Both sections also carry an IPv4-context sentence about transport-mode placement, which supports a different claim (standalone-protocol status under the extension-header-subclassing convention) and does not evidence IPv6 membership; quoting it here was caught in review and corrected to the IPv6-context sentence instead. Added a regression test per protocol asserting the docstring cites the IPv6-context wording and not protocol-numbers-1.csv or any IPv4 placement phrasing; both fail against 3823758's docstrings and pass here. Build: mypy 321/38 (unchanged), pylint 8.67/10 exit 30 (+0.00, unchanged), isort clean, targeted esp/ah unit tests 35 passed.
…otocol-numbers-1.csv (#931) `ESP` and `AH` justified their double inheritance by citing `protocol-numbers-1.csv`'s *IPv6 Extension Header* column -- the derived signal #926 repointed the const generator away from, precisely because it disagreed with the authoritative registry (it flagged BIT-EMU/147 as an extension header; the real registry omits it). - esp.py:978-989 / ah.py:54-63 -- extension-header *membership* now rests on IANA's *IPv6 Extension Header Types* registry and this package's own `ExtensionHeader`, per conventions.rst's "being in that registry is what makes something an extension header" ruling. RFC 4303/4302 section 3.1.1 is quoted too, but only for its actual IPv6-context placement sentence (after hop-by-hop, routing and fragmentation), not as membership evidence -- an earlier revision of this fix rephrased that sentence as "places it among the IPv6 extension headers", which overstated the RFC and contradicted this same docstring's `Note:` on RFC 8200 declining ESP's extension-header status; caught in review and reworded to a plain placement description. - Added the standalone-protocol half #931 asks for: the same RFC section's separate IPv4-context sentence (placed after the IP header, before the next-layer protocol) is the primary-source evidence for ESP/AH's other base, exactly as hip.py cites its own IPv4 appendix. - Checked hip.py for the same stale citation; already correct. Regression tests assert the registry citation, the IPv6-context placement wording, and the standalone-protocol sentence, while only banning the specific wrong phrasing from the first fix attempt rather than "IPv4" outright -- the IPv4 sentence is legitimate prose here. Both fail against 3823758, and against the fix's two prior (defective) revisions, before passing here. Build: mypy 321/38 (unchanged), pylint 8.67/10 exit 30 (unchanged), isort clean, targeted docstring tests pass (full esp/ah suite not re-run: test_esp_unit.py is slow, run targeted instead).
…otocol-numbers-1.csv (#931) `ESP` and `AH` justified their double inheritance by citing `protocol-numbers-1.csv`'s *IPv6 Extension Header* column -- the derived signal #926 repointed the const generator away from, precisely because it disagreed with the authoritative registry (it flagged BIT-EMU/147 as an extension header; the real registry omits it). - esp.py:978-993 / ah.py:54-67 -- extension-header *membership* now rests on IANA's *IPv6 Extension Header Types* registry and this package's own `ExtensionHeader`, per conventions.rst's "being in that registry is what makes something an extension header" ruling. RFC 4303/4302 section 3.1.1 is quoted too, but only for its actual IPv6-context placement sentence, not as membership evidence -- an earlier revision rephrased that sentence as "places it among the IPv6 extension headers", overstating the RFC and contradicting this docstring's own `Note:` on RFC 8200 declining ESP's extension-header status; caught in review and reworded to a plain placement description. - Added the standalone-protocol half #931 asks for: the section's separate IPv4-context sentence is the primary-source evidence that ESP/AH also travel directly as an IPv4 payload, which *qualifies* an already-standalone header for a second base -- it does not *make* the header standalone (conventions.rst's MH case settles that), and the RFC has no say in which base is named. An earlier revision said the RFC "makes it a standalone protocol" and "names IPsec as the second base"; the latter was positionally false (`ESP.__bases__` puts IPsec first, not second) and unsupported by the cited section. Caught in review and reworded to match hip.py's own phrasing. - Checked hip.py for the same stale citation; already correct. Regression tests assert the registry citation and the corrected prose, ban the specific wrong phrasings from all three earlier fix attempts, and pin the registry-membership clause structurally (via a regex on the parenthetical immediately following "registry lists ... at <n>") so that a future rewording of the IPv4 evidence, not just this exact phrase, would still be caught if attached to the wrong claim. All fail against 3823758 and against the fix's three prior revisions, before passing here. Build: mypy 321/38 (unchanged), pylint 8.67/10 exit 30 (unchanged), isort clean, targeted docstring tests pass (full esp/ah suite not re-run: test_esp_unit.py's module import alone is ~11 minutes).
1 participant
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 #925.
ExtensionHeader's crawler pointed at the Protocol Numbers registry filtered on itsIPv6 Extension Headerflag, not the authoritative IPv6 Extension Header Types registry -- the two disagree on header 147 (BIT-EMU).LINKnow points atextension-header.csv, andcount/processare rewritten for its 3-column shape. That registry has no shortKeywordcolumn, so aNAMESoverride keeps the 8 affected members (HOPOPT,IPv6-Route,IPv6-Frag,ESP,AH,IPv6-Opts,HIP,Shim6) from being silently renamed -- verified by feeding the issue's own fixture CSV straight to the crawler (no network) and pinning all 11 surviving names/values.pcapkit/const/ipv6/extension_header.pynow dropsBIT_EMU, taking it to 11 members.IPv6._decode_next_layer's walk resolvesExtensionHeader(proto)at the top of its loop and used to rely on 147 succeeding there, viatest_ipv6_ext_unit.py::test_unimplemented_terminal_code_stops_the_walk_not_the_packet-- but that test's own docstring says 147 was only ever the example: "253/254 are the same code path (also unregistered, also resolve toRaw)". Unlike 147, 253 is in the authoritative registry, so the test now builds its packet around 253 instead -- identical code path, no longer tied to a member being removed. That freesBIT_EMUto actually go. Updated the prose inipv6.py/ipv6_ext.pythat describedBIT-EMUas reachable through the walk/generic extractor accordingly;TransType.BIT_EMU(a different, correctly-sourced enumeration) is untouched.The hand-applied const file is verified against the crawler itself, not just asserted: a new test feeds the fixture through
ExtensionHeader.context()and diffs it byte-for-byte against the committed file. A real vendor regeneration against the live registry was still not run -- this environment disallows live crawler/network invocation -- so that byte-for-byte match is only against the fixture text the issue's own investigation fetched. The owner should runpython -m pcapkit.vendor.ipv6.extension_headeronce to confirm the live registry still matches.