Skip to content

fix(mh): keep zero-length PadN, reserved fields and BU/BA flag bits on rebuild - #1345

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1324-1329-mh-roundtrip
Oct 7, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1324-1329-mh-roundtrip

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort) — only mypy run on the 3 MH files: no new errors (2 pre-existing in mh.py); pylint/isort not run
  • make test passes, and a test case covers the change — per-module runs listed below, not the full suite
  • Added a changelog entry — N/A, added centrally

What is the purpose of your pull request?

  • fix — corrects a defect

Closes #1324
Refs #1329

#1324: RFC 6275 §6.2.3 (checked): PadN is for "two or more octets", with Option Length N-2, so length 0 is legal. As in #1310, _read_opt_pad no longer rejects it and _make_opt_pad no longer rewrites it as Pad1. Fresh-make padding of 2 octets still uses two Pad1s, also as in #1310.

#1329: the header reserved, BRR reserved and Handoff Indicator reserved are now integer fields. The IPv4 Home Address (9 bits) and ANI Network-Identifier (7 bits) get a reserved sub-field. BU/BA now model every flag in the vendored IANA registries (pcapkit/const/mh/binding_{update,ack}_flag.py): BU M R P F T B S D (RFC 4140/3963/5213/5555/5845/6602/7161/8885), BA R P T B S D, plus flags_reserved. PadN contents (the rest of #1329's repro) and the other PaddingField sites are #1223, hence Refs. BU S follows IANA/RFC 8885 (bit 10), not RFC 7161's drawing (bit 7, colliding with F).

Breaking: new required data fields: MH.reserved (every message), BindingRefreshRequestMessage.msg_reserved, BU/BA flags + flags_reserved, and reserved on HandoffIndicatorOption, IPv4HomeAddressOption and ANINetworkIdentifierSuboption. The message-level names differ from reserved because they would clash with the inherited header field.

Probe (parse, then MH.from_data): before, D14 raised ProtocolError, and header/BRR/BU/BA came back …00…, f000, 80. After, all 8 repros are byte-exact. All 99 MH frames in options-*.pcap still round-trip.

Tests: new test_mh_roundtrip_unit.py (6 tests, 41 subtests). With the three source files reverted: 34 failed, 2 passed. test_mh_unit.py is edited where it pinned the old shape: PadN 0 → Pad1, PadN 0 rejected, and fake headers/flags without the new keys. Passing: new module, test_mh_unit 52/499, test_mh_mesg_id_fraction_unit, test_option_roundtrip_unit 6/362, test_ipv6_ext_unit, test_length_hint_minimum_header_unit, and tests/project 389 passed. run_unittest_leg.py protocols/internet: 509 OK.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) breaking Breaks public-facing behaviour or API (apply alongside the type label) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on a73d66750: NEEDS CHANGES — the closing keyword, not the code (Sonnet reviewed; author Opus).

Closes #1329 would close an issue this PR only partly fixes. #1329's own repro includes PadN contents: 0106aabbccddeeff still rebuilds as 0106000000000000 here. That part was scoped to #1223 by my brief, not by the author, so the fix is Refs #1329 in the body. I will record on #1329 that its PadN half rides on #1223. Also wrap schema/internet/mh.py:593, the one new pylint finding on a changed line (C0301, 135/120).

The code held everywhere it was tested:

  • fix(mh): PadN with Option Length 0 is rejected on read and rewritten as Pad1 on make #1324: RFC 6275 §6.2.3 gives "two or more octets … Option Length N-2", so PadN length 0 is legal. D14 raises on 11ae68ebf and rebuilds here.
  • BU and BA flags: the vendored tables match the IANA registry, and the bit positions agree with RFC 3963, 4140, 5213, 5555, 5845, 6602 and 8885. All 16 BU and 8 BA single-bit patterns rebuild byte-exactly, with no overlap or gap into flags_reserved.
  • Widths: RFC 5555 gives 9 reserved bits and RFC 6757 gives 7.
  • Naming: msg_reserved/flags_reserved avoid a real collision with the inherited MH.reserved, and neither field shadows the other.
  • Revert proof: against 11ae68ebf it reproduces at 34 failed / 2 passed.
  • Fixtures: 99 MH frames rebuild. info differs from main only by the new keys.
  • Tests: the protocols/internet leg gives 509 tests, 0 failures, and 15 other modules pass.

Nit: RFC 7161 draws BU S at bit 7, colliding with RFC 5555's F. The PR rightly follows IANA and RFC 8885 (bit 10); a comment naming the conflict would help.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
…n rebuild

- PadN with Option Length 0 is a valid two-octet pad (RFC 6275 6.2.3);
  it is no longer rejected on read or rewritten as Pad1 on make (#1324).
- The fixed-header reserved octet, the BRR reserved field and the
  Handoff Indicator reserved octet are read as integers and written back
  (new data fields MH.reserved, BRR.msg_reserved, HI.reserved).
- BU and BA model every flag IANA assigns (M R P F T B S D / R P T B S D)
  and keep the remaining reserved bits (flags_reserved).
- IPv4 Home Address and Network-Identifier ANI sub-option keep their
  reserved bits (reserved).

Closes #1324
Refs #1329
@JarryShaw
JarryShaw force-pushed the fix/1324-1329-mh-roundtrip branch from a73d667 to f81c392 Compare October 7, 2026 19:38
@JarryShaw

Copy link
Copy Markdown
Owner Author

Round-2 verdict on f81c3927a: GOOD TO GO. I read the delta from a73d66750 myself: one file, +4/−1 in schema/internet/mh.py. It wraps the MH.__init__ stub, so the C0301 is gone, and adds a comment on BU S noting the RFC 7161 / RFC 5555 bit-7 collision and that IANA and RFC 8885 put it at bit 10. No behaviour changed. The body now reads Closes #1324 and Refs #1329, which leaves #1329 open for its PadN half under #1223. Everything else stands as round 1 verified it.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Coverage: 85.85% (unit tier, Python 3.14, f81c3927a, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1453 2342 822 86.81%
pcapkit/corekit 1878 92 580 24 94.22%
pcapkit/dumpkit 142 0 42 0 100.00%
pcapkit/foundation 2442 143 852 42 92.56%
pcapkit/interface 112 62 40 1 33.55%
pcapkit/protocols 15939 564 4116 337 95.14%
pcapkit/toolkit 495 163 150 6 65.43%
pcapkit/utilities 429 12 122 4 96.73%
pcapkit/vendor 4409 2395 1006 154 41.92%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw

Copy link
Copy Markdown
Owner Author

The 85.85% on this PR is a CI artefact, not this diff. coverage combine merged 29 data files here where #1341 merged 30, with every test passing in both (job 112994306184 vs 112993436318). The missing coverage is in interface and toolkit, which this PR does not touch. Filed as #1387. The real figure is about 88.9%.

@JarryShaw
JarryShaw merged commit ad1c030 into main Oct 7, 2026
38 checks passed
@JarryShaw
JarryShaw deleted the fix/1324-1329-mh-roundtrip branch October 7, 2026 22:56
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 7, 2026
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.

fix(mh): PadN with Option Length 0 is rejected on read and rewritten as Pad1 on make

1 participant