Skip to content

fix(hip): rebuild parsed packets byte-exactly (Header Length, no parameters, PUZZLE lifetime, reserved bits) - #1342

Merged
JarryShaw merged 1 commit into
mainfrom
fix/hip-roundtrip-1313
Oct 7, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/hip-roundtrip-1313

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style — pylint (Makefile flags), mypy and isort on the three HIP modules; one new pylint W0613, on a TYPE_CHECKING __init__ stub, as for every sibling field
  • make test passes, and a test case covers the change — ran the modules listed below, not the full suite
  • Changelog entry — N/A, added centrally

What is the purpose of your pull request?

  • fix — corrects a defect

Description

Repro (from_data round trip) before after
#1313 raw 12-octet / 7-octet param via make() Header Length 5 over 52 octets / 4 over 47 padded to 8 octets: 6 over 56 / 5 over 48
#1314 Header Length 4, no parameters AttributeError byte-exact
#1323 PUZZLE Lifetime octet 0..255 59/256 256/256
#1328 Controls fffe, ESP_INFO reserved abcd 0000, 0000 byte-exact

All 49 HIP frames in options-internet.pcap still round-trip.

Breaking: these data-class fields are new and required: Control.reserved, ESPInfoParameter.reserved and PuzzleParameter.lifetime_exponent. PuzzleParameter.lifetime is now derived from the octet, and is None above octet 78. The new make() keyword is controls_reserved=0. _make_param_esp_info gains reserved=0, and _make_param_puzzle gains lifetime_exponent=None.

Not done here: the unknown-parameter padding (7ff00001 aa ffffff → aa000000) is a PaddingField, so it belongs to #1223. Other HIP reserved bits still rebuild as zero: 12 PaddingField sites (now listed on #1223) and the Locator flag bits (#1365).

Tests: test_hip_roundtrip_unit.py is new. Run against the unfixed modules, all 6 of its tests fail (197 PUZZLE subtests fail). In test_hip_unit.py I updated fixtures for the new fields. One assertion there pinned the old raise for PUZZLE octet 0x00; it now checks that the octet rebuilds. Results: test_hip_roundtrip 6 passed; test_hip_unit 34 passed; test_option_roundtrip 6 passed; 7 other HIP modules passed; tests/project 389 passed; run_unittest_leg.py protocols/internet 509 tests, 0 failures.

Closes #1313
Closes #1314
Closes #1323
Closes #1328

…meters, PUZZLE lifetime, reserved bits)

- make(): pad a raw bytes parameter to 8 octets (RFC 7401 §5.2.1) and
  compute Header Length with ceiling division (#1313).
- _make_data(): a packet with no parameters rebuilds; read only sets
  `parameters` when some exist (#1314).
- PUZZLE: keep the raw Lifetime octet as `lifetime_exponent` and write it
  back; `lifetime` is derived, and None past timedelta.max (#1323).
- Controls: name the 15 reserved bits (`Control.reserved`,
  `controls_reserved`); ESP_INFO: read `reserved` as a value (#1328).
- test_hip_unit: fixtures gain the new fields; the octet-0x00 PUZZLE case
  now rebuilds instead of raising.

Closes #1313
Closes #1314
Closes #1323
Closes #1328
@JarryShaw JarryShaw added breaking Breaks public-facing behaviour or API (apply alongside the type label) fix Pull requests that fix a defect (fix: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 7, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 7, 2026
@JarryShaw
JarryShaw merged commit 4849d16 into main Oct 7, 2026
39 checks passed
@JarryShaw
JarryShaw deleted the fix/hip-roundtrip-1313 branch October 7, 2026 20:11
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on fb09b737d: GOOD TO GO (Sonnet reviewed; author Opus). The reviewer returned NEEDS CHANGES only on the PR body's out-of-scope paragraph. The code passed every check, so I am granting the verdict and asking for the body fix separately.

PR body correction: it says 13 parameters drop reserved octets because each is a PaddingField. There are 12 PaddingField sites (added to #1223), and the 13th is Locator.flags, a BitField (:594-599) that drops seven bits. That one is filed as #1365.

Nits: make(controls_reserved=-1) raises a bare OverflowError from BitField. An over-range ESP_INFO reserved is silently masked. Editing lifetime on parsed data is ignored because the exponent octet wins, which is worth a docstring line.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed 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

Correction to my verdict above. I wrote that the PR body claimed 13 parameters drop their reserved octets "because each is a PaddingField". It never did. I checked both versions in the body's edit history, and neither contains that claim: it was in the author's report to me, and I misattributed it to the PR. The body now has a sentence that points to #1223 for the 12 PaddingField sites and to #1365 for the Locator flag bits. The head is unchanged at fb09b737d, so the GOOD TO GO verdict stands.

@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

1 participant