Repository navigation
fix(mh): read/make FBU and FBack Lifetime as plain seconds per RFC 5568 - #502
Merged
Merged
Conversation
…its (#493) RFC 5568 section 6.2.2/6.2.3 define the Fast Binding Update and Fast Binding Acknowledgment Lifetime fields as plain seconds ("the requested time in seconds" / "the granted lifetime ... in seconds"); the phrase "time unit" never appears in the RFC. RFC 6275's 4-second unit applies only to the Binding Update/Acknowledgement Lifetime it explicitly defines that way (section 6.1.7). - pcapkit/protocols/internet/mh.py: drop the `* 4` / `/ 4` scaling in _read_msg_fbu, _read_msg_fback, _make_msg_fbu, _make_msg_fback, and correct the _read_msg_fbu Note that had cited RFC 5568 6.2.2's "identical to BU" wording -- about message layout, not field units -- as justification for the wrong scaling. - pcapkit/protocols/schema/internet/mh.py and data/internet/mh.py: update the FBU/FBack Lifetime field comments to cite the RFC 5568 sections that define the unit as seconds. - Also converts the Binding Refresh Advice option's interval from a raw int to a timedelta, for consistency with every other 4-second- unit field in this module (RFC 6275 section 6.2.4); the wire bytes already round-tripped correctly, so this is a representation fix only, with _read_opt_bra/_make_opt_bra updated to match. - tests/protocols/internet/test_mh_unit.py: correct the stale FBU/FBack lifetime assertions (including one enshrining the bug in a comment), update BRA interval assertions for the timedelta, and add a dedicated test with a BU/BA control case proving the 4-second scaling is preserved where it is correct. Build: brazil n/a (GitHub project). Tests: tests/protocols/internet/ test_mh_unit.py (46 passed, 782 subtests) and tests/protocols/test_option_roundtrip_unit.py (6 passed, 358 subtests, unchanged from baseline) both green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #493.
The RFC contrast is what makes this decisive
RFC 6275 §6.1.7, the Binding Update Lifetime — the field the ×4 scaling is correct for:
RFC 5568 §6.2.2 (FBU) and §6.2.3 (FBack) — the fields the code applied the same scaling to:
Both say "in seconds", and the phrase "time unit" appears in RFC 5568 zero times across the whole document. Both RFCs were fetched and read rather than quoted from memory.
The docstring cited, as its authority, the very section that refutes it. The
Note:above_read_msg_fbusaid RFC 5568 §6.2.2 states the FBU is identical to the BU message, "so the lifetime is read in units of 4 seconds". §6.2.2 does say identical — about message layout; two paragraphs later the same section defines the Lifetime as seconds. That Note is rewritten, not just the arithmetic, because leaving it would re-justify the bug for the next reader.What changed
The
* 4// 4removed from the four sites in #493:_read_msg_fbu,_read_msg_fback,_make_msg_fbu,_make_msg_fback, plus the misleading schema and data-model comments, which now cite RFC 5568 §6.2.2/§6.2.3.Measured, on both trees, with the control that proves it is surgical
The BA control reading 100 on both trees is the point: the ×4 came out of FBU/FBack only, not out of a whole-module convention. Read side likewise: wire 100 was reported as 400.0s, now 100.0s, while BA still reports 400.0s.
Secondary — the Binding Refresh Advice
interval, converted totimedeltaRFC 6275 §6.2.4, also fetched:
Every other 4-second-unit field in this module is a
timedelta— BU/BA lifetime, the ANI Update-Timer, the LMA-controlled-MAG re-registration start time.BindingRefreshAdviceOption.intervalwas the lone bareint. It is now atimedelta, read asinterval * 4and made withceil(total_seconds() / 4), with_make_opt_braacceptingint | timedelta(a bareintstill means wire ticks, mirroring the LMA-controlled-MAG convention).This is a behaviour break for anyone reading
.intervalas a bare number. It is deliberate: the wire bytes always round-tripped correctly, so this is a representation fix, and leaving one field in a module oftimedeltas as a raw tick count is the thing that misleads.Verification
Revert-proof. Restoring the ×4 / ÷4 at the FBU/FBack sites:
Restored:
test_mh_unit.py46 passed, 782 subtests passed;test_option_roundtrip_unit.py6 passed, 358 subtests passed, unchanged from baseline.EXPECTED_FAILURESinspected by importing the module (it cannot be grepped —**unpacking): 59 entries before and after, none referencingmh, so no entry went stale.About the changed existing tests
Several existing assertions had enshrined the bug — including a golden-bytes test expecting
timedelta(seconds=40)for wire value10, with a comment citing the very reasoning that was wrong. Those are corrected to match the RFCs.No wire-bytes literal was altered. I checked the diff for every
b'...'/\xliteral in the test file: the only addition is a new stub'schksum=b'\x12\x34'in a new helper. The golden wire format is fixed by the RFC and is untouched; only its interpretation changed, which is the whole defect.Note on the broader suite
A full
tests/protocols/run in the working tree showed 28 failures intest_pcapng_regression.pyand the TCP/UDP runtime tests. I verified these are fixture artefacts, not regressions: that worktree had 1 generated capture instead of 13, and after runningexamples/generators/make_samples.pyall three files pass (11 passed, 3 subtests) — identical tomain. Nothing in them imports MH.