Skip to content

fix(ipv6-route): compute Hdr Ext Len in 8-octet units on both sides - #489

Merged
JarryShaw merged 2 commits into
mainfrom
fix-487-ipv6-route-source-route-roundtrip
Sep 18, 2026
Merged

JarryShaw merged 2 commits into
mainfrom
fix-487-ipv6-route-source-route-roundtrip

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Sep 18, 2026 •

Copy link
Copy Markdown
Owner

Closes #487

Root cause (both halves, confirmed with measurements)

Per RFC 8200 §4.4, Hdr Ext Len counts the Routing header in 8-octet units, not including the first 8 octets (4 fixed octets + the 4-octet Reserved field every routing type's data starts with). So total header size = 8 + 8*HdrExtLen, and type-specific data size = 4 + 8*HdrExtLen.

  1. Write side: IPv6_Route.make()'s dict branch wrote raw octets into length (len(data_val.pack())); its Schema branch used the wrong sign ((len+4)/8 instead of (len-4)/8). For a 24-octet, one-address header they emitted 20 and 3 where 2 is correct.
  2. Write side (not previously isolated): ipv6_route_data_selector (schema module) sized the nested routing-data schema at hdr_ext_len * 8 octets — 4 octets short of the wire, missing the Reserved field. This is why a hand-built, spec-correct 24-octet header (Hdr Ext Len=2) also failed to parse, independent of anything make() produced — confirmed by hand-building that exact header and walking the selector by hand before touching any code.

Measured before fix:

selector computes SchemaField(length=pkt["length"]*8) = 16 octets
but the on-wire type-specific data (4 reserved + 1 address) is 20 octets
SourceRoute.unpack(len=16) raised: FieldValueError('Field ip has invalid length.')

What changed

  • Added one shared helper, IPv6_Route._make_hdr_ext_len, and used it on all three make() branches (bytes/dict/Schema) instead of two separate (both wrong) expressions.
  • Added its inverse, ipv6_route_data_length, and used it in ipv6_route_data_selector.
  • Fixed the two read-side guards flagged in the issue: _read_data_type_src now checks Hdr Ext Len is even (each 16-octet address costs 2 units); _read_data_type_2 now checks it equals 2 (a Type 2 header is fixed at 24 octets).
  • _read_data_type_rpl's % 16 guard has the same surface shape but is left as-is: RPL's construction already fails earlier from an unrelated, separately tracked defect (RPL.post_process assumes bytes on a path Schema.pack also runs — schema: Schema.pack rejects the tuple its own data models declare, breaking parse-then-reconstruct for 16 HIP parameters #476/schema: let ListField.pack accept the tuple its own data models declare #480), so there's no working round trip to validate a replacement guard against. Flagged in a code comment for follow-up rather than guessed at.

Tests

  • Extended the tests: pin ipv6-route Source Route tuple/list address packing #485 tuple-vs-list test to go through the full IPv6_Route round trip (previously it could only unpack the nested schema directly, because the full path could never pass) — kept the tuple-vs-list assertion, updated the expected Hdr Ext Len bytes.
  • Added a construct-then-parse round-trip test over 0/1/2/3 addresses.
  • Added a test parsing a hand-built wire form this library never constructed — the more important check, since a round-trip test alone can pass on two mistakes cancelling out.
  • Removed the now-stale ipv6-route-type/Source_Route and Type_2_Routing_Header entries from EXPECTED_FAILURES in tests/protocols/test_option_roundtrip_unit.py (verified they now come back 'OK'); RPL_Source_Route_Header's entry stays, for the unrelated reason above.

Verified the new/updated tests actually catch the regression: reverted just the two production files to main's version, confirmed all three tests fail with FieldValueError: Field ip has invalid length., restored, confirmed they pass.

Test plan

  • tests/protocols/internet/ — 185 passed, 631 subtests
  • tests/protocols/schema/ — 25 passed, 5 subtests
  • tests/protocols/test_option_roundtrip_unit.py — 6 passed, 358 subtests (up from 356; two stale gap entries removed)
  • Reverted-fix check: new/updated tests fail pre-fix with the exact reported FieldValueError, pass post-fix
  • Full tests/ (excluding *_runtime.py/*_regression.py, matching CI's unit-tests job) — 917 passed, 5 skipped, 1784 subtests passed, no failures

Out of scope, deliberately not touched

IPv6_Route.make() emitted a Source-Route header its own parser rejected,
for every address count, and a hand-built spec-correct header failed too
-- the parse path was broken independently of anything make() produced.

Root cause, per RFC 8200 4.4: Hdr Ext Len counts the header in 8-octet
units "not including the first 8 octets" (4 fixed + the 4-octet Reserved
field every routing type's data starts with), so a header's total size is
8 + 8*HdrExtLen octets and its type-specific data is 4 + 8*HdrExtLen.

- Write side: make()'s dict branch wrote raw octets into `length`; its
  Schema branch used the wrong sign on the offset. For a 24-octet,
  one-address header they emitted 20 and 3 where 2 is correct. Replaced
  both (and the bytes branch) with one shared helper,
  IPv6_Route._make_hdr_ext_len, so the field is computed once.
- Read side (not previously isolated): ipv6_route_data_selector sized the
  nested routing-data schema at `hdr_ext_len * 8` octets, 4 short of the
  wire -- missing the Reserved field -- so even a correct hand-built
  header's `ip` ListField saw an invalid length. Added the inverse helper,
  ipv6_route_data_length, and used it in the selector.
- Fixed the two read-side guards the issue flagged as suspect for the same
  unit confusion: Source Route's now checks Hdr Ext Len is even (each
  16-octet address costs 2 units), Type 2's now checks it equals 2 (fixed
  24-octet header). RPL's guard has the same surface shape but is left
  as-is: its construction already fails from an unrelated, separately
  tracked defect (#476/#480), so there is no working round trip to
  validate a replacement against.

Measured before: make()+pack() for 0/1/2/3 addresses produced Hdr Ext Len
bytes 4/20/36/52 (octets, wrong); reading any of them, or a correct
hand-built 24-octet header (Hdr Ext Len=2), raised FieldValueError: Field
ip has invalid length. After: bytes are 0/2/4/6 and all four round-trip,
and the hand-built header parses.

Extended the #485/#480 tuple-vs-list test to go through the full
IPv6_Route round trip instead of the schema alone (its prior reason for
not doing so no longer applies), added a dedicated construct-then-parse
test over 0-3 addresses, and a hand-built-wire-form test independent of
make(). Removed the now-stale ipv6-route-type/Source_Route and
Type_2_Routing_Header entries from EXPECTED_FAILURES in
test_option_roundtrip_unit.py; RPL_Source_Route_Header's entry stays.

Build: tests/protocols/internet/ (185 passed, 631 subtests),
tests/protocols/schema/ (25 passed), tests/protocols/test_option_roundtrip_unit.py
(6 passed, 358 subtests).

Closes #487
@JarryShaw

Copy link
Copy Markdown
Owner Author

Full unit-test suite finished (matching CI's unit-tests job selection: pytest -q --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py'):

917 passed, 5 skipped, 226 warnings, 1784 subtests passed in 737.66s (0:12:17)

No failures anywhere in the tree, not just in ipv6_route. Checking off the last test-plan item.

@JarryShaw

Copy link
Copy Markdown
Owner Author

Reviewed at c3ed92464fdb66d6941f36d726feff300058e1a7 (confirmed via git rev-parse HEAD after git checkout c3ed92464 in an isolated worktree; diffed with git diff origin/main...c3ed92464, three-dot throughout). Note: the PR branch has since moved to cff25011b — that commit is only a merge from main plus an unrelated one-line docs change to docs/source/pep.rst; it does not touch any of this PR's five files, so it doesn't affect anything below.

1. Is 4 + hdr_ext_len * 8 right for every registered routing type? Enumerated the registry directly (grep -rn "RoutingType, code=" plus the class body) — there are exactly four: UnknownType (default), SourceRoute, Type2, RPL. No fifth type is registered anywhere in the tree.

The formula is correct for all four, and it's correct by construction rather than by luck: per RFC 8200 §4.4, the total on-wire Routing header is 8 + 8*HdrExtLen octets, and the first 4 of those are always Next/HdrExtLen/Type/SegLeft, already consumed by IPv6_Route itself before the nested RoutingType schema ever sees a byte. So the type-specific data is always (8 + 8*HdrExtLen) - 4 = 4 + 8*HdrExtLen octets — this is header-framing arithmetic, not an assumption about any particular type's internal layout (the docstring's "4 reserved octets" framing is a bit narrow; the real invariant is framing-level and holds independently of whether a given type happens to spend those 4 octets on a Reserved field). Verified against each subclass's own field declarations:

  • SourceRoute: PaddingField(length=4) + ListField(length=lambda pkt: pkt['__length__']) — traced through Schema.unpack (pcapkit/protocols/schema/schema.py:813,826,880) to confirm __length__ is decremented as fields consume it, so the ListField sees exactly the remainder after the 4-octet padding, i.e. hdr_ext_len*8 — divisible by 16 iff hdr_ext_len is even, matching the new guard.
  • Type2: PaddingField(length=4) + fixed 16-octet IPv6AddressField → always 20 octets → hdr_ext_len always 2.
  • RPL: cmpr_i(1) + cmpr_e(1) + pad(3, BitField) + addresses + padding — the fixed part is 4 octets by coincidence of its own field layout, but the formula doesn't depend on that; it depends only on the base header framing.
  • UnknownType: BytesField(length=lambda pkt: pkt['__length__']) — takes the whole 4+8k octets as opaque bytes, which is exactly correct for a type this code cannot interpret.

2. Are _make_hdr_ext_len and ipv6_route_data_length true inverses? Yes, exactly, over the full domain that matters. ipv6_route_data_length(_make_hdr_ext_len(4+8k)) == 4+8k for every k in [0, 255] (checked the algebra: _make_hdr_ext_len(4+8k) = ceil(8k/8) = k). At the boundaries: k=0 → data length 4 (the minimum valid frame), k=255 → data length 2044, both round-trip exactly. For the raw-bytes make() branch, non-boundary lengths round up monotonically (e.g. 5–12 octets all map to hdr_ext_len=1, i.e. padded to 12) and never truncate, and max(0, data_length-4) keeps hdr_ext_len from going negative for data_length<4 — that branch also .ljust()s the actual bytes to match, so it stays self-consistent.

One thing not self-consistent, though pre-existing and not introduced or worsened by this PR: the dict/Data_IPv6_Route branch and the raw-Schema-instance branch of make() (pcapkit/protocols/internet/ipv6_route.py:305-319) compute length from _make_hdr_ext_len(len(data_val.pack())) but never pad data_val itself to match — unlike the bytes branch, which explicitly .ljust()s. This is harmless for the three concrete constructors actually wired up (_make_data_type_none already pads internally to 4+8k; SourceRoute/Type2's own field layout is always exactly boundary-aligned), but a caller passing a raw Schema instance directly with a non-boundary-aligned pack() length (e.g. a hand-built UnknownType(data=b'\x01\x02\x03')) gets a length field that overstates what's actually on the wire, with no error and no padding. Not a regression from this PR (the old formula had the same gap, just with different — arguably worse — numbers), and arguably out of scope for #487, but worth a follow-up test/guard if anyone exercises that branch for real.

3. Read-side guards. Source_Route's header.length % 2 != 0 and Type2's header.length != 2 are both correct against RFC 8200/6275 units — confirmed by hand and by the new construct-then-parse tests. RPL was left alone; I independently reproduced why, in a throwaway script (object.__new__(IPv6_Route).make(type=Routing.RPL_Source_Route_Header, data={'ip': [...]}) → .pack()):

ValueError: [b' \x01\r\xb8...\x01', b' \x01\r\xb8...\x02'] does not appear to be an IPv4 or IPv6 address

raised from RPL.post_process (pcapkit/protocols/schema/internet/ipv6_route.py:207), because post_process runs on the pack path too (Schema.pack → schema.py:731) and treats self.addresses as a single concatenated bytes buffer, but at pack time it's still the list[bytes] the constructor built. This confirms the PR's stated reasoning: there is no working RPL round trip today to validate a corrected guard against, independent of #487. Same error string is also what examples/generators/make_samples.py reports as "left out" for ipv6-route-type/RPL_Source_Route_Header, matching the EXPECTED_FAILURES entry byte-for-byte.

4. Test adequacy. Ran the actual suites (PYTHONPATH forced onto this worktree, pcapkit.__file__ asserted to start with the worktree path, no pip-installed pcapkit in the venv to shadow it):

  • tests/protocols/internet/test_ipv6_extension_unit.py -v: 52 passed, 78 subtests passed (46s).
  • tests/protocols/internet/ -v: 185 passed, 631 subtests passed (178s) — matches the reported baseline exactly.
  • tests/protocols/schema/ -v: 25 passed, 5 subtests passed (20s) — matches.
  • tests/protocols/test_option_roundtrip_unit.py -v: 6 passed, 358 subtests passed (<1s) — matches.

The #485 tuple-vs-list byte-identity assertion (self.assertEqual(packed_tuple, packed_list), immediately followed by self.assertEqual(packed_tuple, expected)) is present and unchanged in the rewritten test_ipv6_route_source_route_make_accepts_tuple_and_list_addresses — it now additionally goes through a full IPv6_Route(io.BytesIO(...)).info read rather than Schema_SourceRoute.unpack directly, which is strictly stronger, and doesn't drop the original invariant.

Still uncovered, worth flagging rather than a blocker: Type 2 has no dedicated hand-built-wire-form parse test analogous to Source Route's new test_ipv6_route_source_route_parses_hand_built_wire_form (it does get a real construct→parse→reconstruct cycle through the shared test_option_roundtrip_unit.py harness, which is what the removed EXPECTED_FAILURES entry now proves closes — just not a library-independent hand-built-bytes check). RPL and the dict/Schema-branch padding gap above remain open. Source Route itself isn't tested beyond 3 addresses or with a non-block-aligned/odd Hdr Ext Len on the negative side — though the existing header = types.SimpleNamespace(..., length=1, ...) fixture at line 303 does incidentally exercise the odd-length rejection via _read_data_type_src(SourceRoute(ip=[]), header=header).

5. EXPECTED_FAILURES removals. Imported the module directly (can't grep/ast-parse it — the dict body uses **{...} unpacking at lines 385/429) and confirmed only ipv6-route-type/RPL_Source_Route_Header remains under the ipv6-route prefix; Source_Route and Type_2_Routing_Header are gone. The full roundtrip suite run above (358 subtests, all green) is exactly the check that would go red if either removed case still failed silently under a different signature — it didn't, so both genuinely close now.

6. Docs. .rst, references the real defining modules (pcapkit.protocols.internet.ipv6_route._make_hdr_ext_len via automethod inside the IPv6_Route class block; pcapkit.protocols.schema.internet.ipv6_route.ipv6_route_data_length via autofunction) — no re-export indirection.

CI, polled to 0 pending on c3ed92464: 18 success, 2 skipped (Docs test gate, Gate (full suite, Python 3.14) — skip by design on pull_request), 3 cancelled (Integration Python 3.10/3.12/3.15). The cancellations are not test failures: the annotation on each is "Canceling since a higher priority waiting request for unit-tests-Unit Tests-refs/pull/489/merge exists" — GitHub's concurrency-group cancellation fired when the branch received the later cff25011b push (merge-from-main + the unrelated pep.rst docs line) while these three legs were still running. The sibling Integration Python 3.11/3.13/3.14 legs and every non-integration Python version (3.10–3.15) for this exact commit completed and passed. I did not find any real failure signal for c3ed92464.

No blocking findings. The unit-conversion fix is correct and provably general across the whole registry, the two helper functions are genuine inverses, the RPL non-fix is independently justified, and the test and doc changes hold up under an adversarial re-check.

GOOD TO MERGE c3ed92464fdb66d6941f36d726feff300058e1a7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

IPv6_Route Source-Route headers cannot round-trip: make() emits a wrong Hdr Ext Len and the parse path rejects even a correct wire form

1 participant