protocols: reject an int MN-ID identifier for every subtype but IPv6_Address - #468
Conversation
…Address _make_opt_mn_id's `elif isinstance(identifier, int)` branch sized the identifier from the int's own bit_length() regardless of subtype, the same type-vs-subtype confusion #464 fixed for IPv6_Address. mn_id_selector resolves NAI to a StringField and every other non-IPv6 subtype to a BytesField, and neither field type converts an int, so the declared length was wrong and pack() always failed -- AttributeError for NAI ('int' has no .encode()), struct.error for the rest. This is the other half of #448, pre-existing on main and unrelated to #464. See #467. - pcapkit/protocols/internet/mh.py: the int branch now raises ProtocolError naming the resolved subtype and the type it actually accepts (str for NAI, bytes for the rest), instead of silently building an unpackable schema. IPv6_Address is unaffected -- #464 already normalises it via ipaddress.IPv6Address independent of identifier's Python type, and int stays valid there. - pcapkit/protocols/schema/internet/mh.py: MNIDOption's TYPE_CHECKING __init__ stub drops int from the identifier union, matching the class attribute's own already-narrower annotation two lines above it. - tests/protocols/internet/test_mh_unit.py: new test_mh_mn_id_option_rejects_int_identifier_for_non_ipv6_subtypes, one subTest per rejected subtype (NAI, IMSI, P_TMSI, EUI_48_address, EUI_64_address, GUTI, DUID) plus a check that IPv6_Address still converts. Full suite (PYTHONSAFEPATH=1, interpreter 3.14.7): 1007 passed, 17 skipped, 1560 subtests passed. Baseline at fa12895: 1006 passed, 17 skipped, 1553 subtests passed. mypy: 125 errors before and after, byte-identical modulo line numbers.
|
Reviewed at CI
Field-class reasoning (Q1: is "reject" the right call?)Read
This confirms the author's claim exactly: the int-conversion capability is unique to mypy trade-off (Q2: keeping
|
…Error _make_opt_mn_id's new int-identifier rejection (9b26fa3) named the resolved subtype by round-tripping it through Enum_MNIDSubtype(subtype_val) for a friendly repr. MNIDSubtype._missing_ only auto-extends 9-15 and 16-255, so an out-of-range subtype (0, negative, or above 255) made that constructor call itself raise a bare ValueError, escaping in place of the ProtocolError this branch exists to raise instead. #468 review. - pcapkit/protocols/internet/mh.py: wrap the round-trip in try/except ValueError and fall back to the raw subtype_val for the message on failure, so the branch always raises ProtocolError regardless of what subtype_val holds. Every real subtype (and the Reserved_N/Unassigned_N extensions in 9-255) still gets the named repr; only genuinely unrepresentable values fall back to the bare int. - tests/protocols/internet/test_mh_unit.py: extends test_mh_mn_id_option_rejects_int_identifier_for_non_ipv6_subtypes with subtype 0, -1, 300, 999, asserting isinstance(e, BaseError). Checked mh.py for the same pattern elsewhere (an enum constructor called while formatting a raised message): none found; the only occurrence was the one being fixed here. _make_index (protocol.py) is unrelated -- for an int or Enum argument it returns the value/`.value` unvalidated, by design, for every enum it is used with, not specific to MNIDSubtype. Full suite (PYTHONSAFEPATH=1, interpreter 3.14.7): 37 passed in test_mh_unit.py, 283 subtests. mypy: 125 errors, unchanged.
|
Re-review of the follow-up commit CI
The fix itselftry:
subtype_repr = repr(Enum_MNIDSubtype(subtype_val))
except ValueError:
subtype_repr = repr(subtype_val)
raise ProtocolError(f'{self.alias}: [OptNo {type}] MN-ID subtype '
f'{subtype_repr} identifier must be '
f'{expected}, not int')This is a hybrid of "guard the constructor" and "format the raw value": the round-trip is attempted for a friendly Re-measuring the six values, plus my own adversarial probingConfirmed the six measured values exactly (
So for every input that reaches the branch this commit touches, the general claim holds: no non- However, I found a distinct, pre-existing bare exception in the same function that survives this fix, and it does break the fully general claim for I verified this is pre-existing, not introduced by either commit in this PR: reproduced identically on a clean checkout of "Same trap elsewhere" — checked independently
Test adequacy, verified to actually discriminateThe extended test adds
Spot-checking the rest of the PR at the new shaRe-verified myself:
Taken on trust from the prior review rather than re-run myself: the full 1007-test/17-skipped suite figure (I ran the VerdictThis supersedes the verdict at GOOD TO MERGE at sha |
| subtype_namespace: MN-ID subtype namespace. | ||
| subtype_reversed: MN-ID subtype reversed flag. | ||
| identifier: Identifier. | ||
| identifier: Identifier. An :obj:`int` remains accepted for the |
There was a problem hiding this comment.
We must not accept int? But can't we directly use Ipv6Address class to convert and validate an int?
There was a problem hiding this comment.
Done — int is accepted again as of a8c214ab4, and you were right that the earlier rejection was the wrong call.
On IPv6Address: that path never changed and still does exactly what you describe — ipaddress.IPv6Address(identifier) both converts and validates, at the spec-fixed 16 octets (#448). The rejection I had added only ever covered the other subtypes, and the reasoning behind it was wrong: I claimed there was no non-arbitrary width for them. There is. id_len = math.ceil(identifier.bit_length() / 8) was right all along — self-consistent with the declared length by construction. The actual pre-#467 defect was never the sizing; it was that identifier stayed an int afterwards and reached BytesField unconverted, which struct.pack() cannot do anything with. So the six BytesField subtypes now convert via identifier.to_bytes(id_len, 'big'), floored at one octet so identifier 0 does not collapse into "no identifier" (bit_length() is 0 for 0).
Measured through the maker → pack() → unpack(), not a hand-built schema — all 8 subtypes × {0, 1, 0xff, 0x1234, 0x100000000, 2**128-1, True}:
| subtype | 0 |
0x1234 |
2**128-1 |
|---|---|---|---|
IPv6_Address |
len=17 :: |
len=17 ::1234 |
len=17 ffff:…:ffff |
IMSI / P_TMSI / EUI_48_address / EUI_64_address / GUTI / DUID |
len=2 b'\x00' |
len=3 b'\x124' |
len=17 16×\xff |
Every one round-trips identically through unpack().
NAI is the one subtype that still refuses an int, and that is a judgement call rather than a mechanical limit — say the word and I will change it. Its field is a StringField, so str(identifier) packs and round-trips perfectly well; the argument for refusing is that an NAI is a network access identifier (user@realm, RFC 4283), so a bare decimal-digit string is mechanically valid and semantically nonsense — the same "accepts a value that means the wrong thing" #467 removed, just relocated. The error message names str(...) explicitly so a caller who really wants that can spell it, and subtype=NAI, identifier=str(0x1234) packs to a payload decoding as '4660'.
One thing I found while checking this, which was not in the revision you commented on: the negative-int guard lived inside the elif isinstance(identifier, int) branch, which the IPv6_Address dispatch never reaches. So identifier=-5, subtype=IPv6_Address leaked AddressValueError: -5 (< 0) is not permitted as an IPv6 address — and AddressValueError subclasses ValueError, i.e. a bare stdlib exception escaping the very handler meant to stop that. Hoisted the guard above the subtype dispatch; -5 now raises ProtocolError on all eight subtypes. Measured before and after, not inferred.
Wrong-type identifiers (bytes for NAI, str for the octet subtypes, and so on) still escape as bare stdlib exceptions. That is unchanged from main and tracked separately as #469, deliberately not folded in here.
tests/protocols/internet/test_mh_unit.py: 37 passed, 318 subtests, 0 failed.
There was a problem hiding this comment.
On the __init__ stub: the maker does already accept int — pcapkit/protocols/internet/mh.py:7637 declares identifier: 'bytes | str | IPv6Address | int'. What excludes int is the schema stub and the data-model stub, and that is deliberate, because the three annotations describe three different boundaries:
| annotation | what it describes | int? |
|---|---|---|
MH._make_opt_mn_id (mh.py:7637) |
what a caller may pass | yes |
Schema_MNIDOption.__init__ (schema/internet/mh.py:734) |
what the schema can hold | no |
Data_MNIDOption.__init__ (data/internet/mh.py:756) |
what a parse produces | no |
The maker converts before it ever constructs the schema, so no int reaches either of the lower two. Measured through the maker rather than read off the code:
IMSI identifier=b'\x124' type=bytes
DUID identifier=b'\x124' type=bytes
IPv6_Address identifier=IPv6Address('::1234') type=IPv6Address
All three from identifier=0x1234. So widening the schema stub would document a value the schema can never hold — and it would actively mislead, because it would imply Schema_MNIDOption(identifier=0x1234) works. It does not: mn_id_selector resolves every subtype but IPv6_Address to a StringField or BytesField, and handing either a raw int is precisely the #467 defect, since struct.pack() cannot consume it. The data-model stub is the same story from the other direction — a parse yields str, bytes or IPv6Address, never an int, so int there would describe a state that cannot occur.
Your question did expose a real problem with the comment rather than the annotation, now fixed in 4bebfd4c0 (comment only, no annotation and no executable line changed). The old note said just "neither of which accepts an int (c.f. #467)", which was true but read as though int were rejected outright — no longer the case anywhere a caller can see, and confusing precisely because the maker above now takes one happily. It now states the boundary distinction and carries the measurement.
If you would rather the schema stub admit int anyway — for symmetry with the maker, on the grounds that a reader looking at one will look at the other — say so and I will widen it, but it would be documenting an unreachable state and I would want to note that inline.
Also merged origin/main (f7b5cc5cd, #470) in; clean, and it touches only ipaddress.py and its tests, neither owned by this branch. tests/protocols/internet/test_mh_unit.py unchanged at 37 passed, 332 subtests, 0 failed. Since 4bebfd4c0 is comment-only, the GOOD TO MERGE at 6a9e4e427 still stands by content.
) The owner asked for int to stay accepted rather than be turned away, and the reasoning behind the earlier rejection was wrong. `id_len = math.ceil(identifier.bit_length() / 8)` was the right, non-arbitrary width all along -- self-consistent with the declared `length` by construction. The pre-#467 defect was never the sizing; it was that `identifier` stayed an `int` afterwards and reached `BytesField` unconverted, which `struct.pack()` cannot do anything with. So the six `BytesField` subtypes (IMSI, P_TMSI, EUI_48/64_address, GUTI, DUID) now convert via `identifier.to_bytes(id_len, 'big')`, with the width floored at one octet so that identifier 0 does not collapse into "no identifier" (`bit_length()` is 0 for 0). NAI still rejects an int, and that is a judgement call rather than a mechanical limit: its field is a `StringField`, and `str(identifier)` packs and round-trips perfectly well -- but an NAI is a network access identifier (`user@realm`, RFC 4283), so a bare decimal-digit string is mechanically valid and semantically nonsense. That is the same "accept a value that means the wrong thing" #467 removed, only relocated. The message names `str(...)` so a caller who really wants that can spell it. A negative int is rejected for *every* subtype, and the guard sits before the subtype dispatch rather than inside the int branch. Inside it, the `IPv6_Address` dispatch never reaches the guard, so `identifier=-5, subtype=IPv6_Address` leaked `AddressValueError: -5 (< 0) is not permitted as an IPv6 address` -- a bare ValueError escaping the handler whose whole purpose is to stop that. Measured before the hoist, not inferred. `IPv6_Address` itself is untouched: still `ipaddress.IPv6Address(...)`, which converts and validates, at a fixed 16 octets (#448). Verified through the maker, not a hand-built schema: every one of the eight subtypes x {0, 1, 0xff, 0x1234, 0x100000000, 2**128-1, True} run maker -> pack() -> unpack(), all round-tripping identically, and -5 raising ProtocolError on all eight. Wrong-*type* identifiers (bytes for NAI, str for the octet subtypes, and so on) still escape as bare stdlib exceptions -- unchanged by this PR, tracked separately as #469. tests/protocols/internet/test_mh_unit.py: 37 passed, 318 subtests, 0 failed. Merged origin/main (5182ad0) first.
Review of PR #468 at
|
The review of a8c214a found the upper-bound mirror of the leak that sha's own guard had just closed: an int identifier at or above 2**128 with subtype=IPv6_Address still reached ipaddress.IPv6Address() and raised AddressValueError, which subclasses ValueError -- a bare stdlib exception escaping the handler whose purpose is to stop that. Verified before fixing: 2**128 and 2**140 both leaked, while 2**128-1 packed normally at length 17. The bound is subtype-*dependent*, so the check goes inside the IPv6_Address branch rather than into the shared negative-int guard above it: 2**140 is a perfectly good identifier for the six BytesField subtypes, which simply produce more octets (length 19 for 2**140, 18 for 2**128). The test now asserts that too, so a future guard cannot be hoisted to cover them by mistake. Checked explicitly rather than by wrapping the IPv6Address construction, because wrapping would also swallow the wrong-TYPE AddressValueError a str or None produces there -- that is #469's subject and not this PR's to annex. Pre-existing since #448, not a regression from this branch: the construction line is byte-for-byte unchanged from origin/main. How the gap survived the first pass, since it is the more useful part: my probe swept every subtype but stopped at 2**128-1, exactly one value below where the answer changes. Probing a large value is not probing the boundary. Verified: 2**128 and 2**140 now raise ProtocolError for IPv6_Address and still pack for the six BytesField subtypes. Disabling only the new guard condition fails the two new subtests and nothing else; restoring it passes. tests/protocols/internet/test_mh_unit.py: 37 passed, 332 subtests, 0 failed (318 before, +14 new).
|
|
||
| Raises: | ||
| ProtocolError: If ``identifier`` is a negative :obj:`int` (no | ||
| subtype has a wire form for one), or an :obj:`int` of any value |
There was a problem hiding this comment.
Non-blocking, minor: this Raises: enumeration is now stale.
It lists two conditions -- a negative int, and any int with NAI -- but the guard added a few lines below in this same commit (if isinstance(identifier, int) and identifier >= 1 << 128: ... raise ProtocolError(...)) introduces a third: an int identifier >= 2**128 with subtype=IPv6_Address. Verified directly against 6a9e4e427:
proto._make_opt_mn_id(Option.MN_ID_OPTION_TYPE, subtype=MNIDSubtype.IPv6_Address, identifier=2**128)
# ProtocolError: MH: [OptNo 8] MN-ID subtype IPv6_Address identifier must be an int below 2**128, not 340282366920938463463374607431768211456
That's a third way this method raises ProtocolError that the docstring doesn't mention. Suggest appending something like:
or an ``int`` of ``2**128`` or above with the ``IPv6_Address`` subtype
Not requesting changes for it -- it's a doc-completeness gap, not a behavioral one, and everything the docstring does say remains true.
There was a problem hiding this comment.
Correct, and fixed in 5cf1740ad — docstring only, no executable line changed.
You caught a defect I introduced in the very commit you were reviewing: I rewrote that Raises: clause two commits earlier, then added the upper-bound guard without going back to it. Three ways to raise ProtocolError, two documented.
The clause now lists all three, and I measured each rather than reading the code and assuming:
negative int, any subtype -> ProtocolError (BaseError=True)
int with NAI -> ProtocolError (BaseError=True)
int >= 2**128 with IPv6_Address -> ProtocolError (BaseError=True)
I also added why the ceiling exists only for IPv6_Address — its wire form is a fixed 16 octets, where the other subtypes have no ceiling and simply pack into more — since the asymmetry is the part a caller would otherwise read as an inconsistency rather than a rule.
tests/protocols/internet/test_mh_unit.py unchanged at 37 passed, 332 subtests, 0 failed. Since this is a docs-only follow-up, your GOOD TO MERGE at 6a9e4e427 stands by content; only the sha has moved.
Two other things from your review worth acknowledging rather than letting pass:
Your worktree started at 5182ad0ce, not at the PR head, despite the branch name matching — and you caught it yourself by diffing test-method names and checking git rev-parse HEAD, then re-ran everything from git checkout 6a9e4e427. That is the most useful thing in the report. An agent worktree does not start at the PR head, and a grep or a pytest run against the plain working tree silently measures mainline. Every brief from here on will say so explicitly.
You declined to confirm the "disabling the guard fails exactly the two new subtests" claim rather than repeating it, because reproducing it needs an edit to tracked source and your mandate was review-only — and because setUp's purge_modules(['pcapkit']) reloads the package from disk before every test, defeating an in-memory monkeypatch. Both correct. For the record, that claim is mine and I did verify it by editing the guard's condition to if False: in my own worktree: exactly the two new IPv6_Address subtests failed, nothing else, and restoring it passed. Reporting it as unverified-but-plausible was the right call on your side.
Review of PR #468 at
|
| input | result |
|---|---|
2**128 - 1 |
OK, length=17 |
2**128 |
ProtocolError: "...must be an int below 2**128, not 340282366920938463463374607431768211456" |
2**128 + 1 |
ProtocolError |
2**140 |
ProtocolError |
True (bool, IPv6_Address) |
OK, length=17 -- isinstance(True, int) is True but True == 1 < 2**128, so the guard correctly leaves it alone |
False (bool, IPv6_Address) |
OK, length=17 |
ipaddress.IPv6Address(2**128-1) passed directly |
OK, length=17 -- not an int instance, guard doesn't fire, unaffected |
'::1' (str) |
OK -- bypasses the guard, reaches ipaddress.IPv6Address('::1') as before |
1 << 128 == 2**128 and the comparison is >=, so the boundary is exactly right: the last value ipaddress.IPv6Address accepts (2**128-1) still packs, the first value it rejects (2**128) is now caught in-library instead of leaking. The guard is isinstance-scoped to int specifically, so it does not affect bool (still an int subtype but numerically in-range), pre-built IPv6Address instances, or strings -- all confirmed above.
2. Was the narrow fix (vs. wrap-in-try/except) the right call?
Agree with the scoping. Verified the two behaviors the PR body claims are the deciding factor:
- Wrong-type inputs to the same
ipaddress.IPv6Address(identifier)line still leak a bareAddressValueError, unchanged:str,None,float,list,dictall raiseAddressValueError: At least 3 parts expected in '...', andbytesraisesAddressValueError: b'xyz' (len 3 != 16) is not permitted as an IPv6 address. None of these hit the new guard becauseisinstance(identifier, int)isFalsefor all of them. - The reasoning given in the new comment block and PR body -- that wrapping the construction in try/except would also swallow those wrong-type
AddressValueErrors, which is MH MN-ID: bytes and str are documented interchangeably but each subtype accepts only one, and the wrong one fails with a bare stdlib exception #469's subject, not MH MN-ID: an int identifier cannot be packed for any subtype except IPv6_Address, and its declared length is wrong #467/protocols: reject an int MN-ID identifier for every subtype but IPv6_Address #468's -- checks out empirically: the guard as written is precise enough to change only the int-magnitude case and leave every wrong-type case exactly as it already leaked onorigin/main.
A try/except wrapper would have been the shorter diff, but it would have silently pre-empted #469 with an untested, undiscussed exception-message shape for the wrong-type cases, and conflated two independent defects (wrong magnitude vs. wrong type) under one CR. The explicit isinstance + bound check is a few more lines for a fix that stays inside its own issue's boundary and keeps #469 fully open and well-defined. I'd have made the same call.
3. Did the fix disturb anything confirmed at a8c214ab4?
Re-ran, against 6a9e4e427, everything the previous review recorded as confirmed:
- All 8 subtypes (
NAIexcluded from the int round-trip,IPv6_Address,IMSI,P_TMSI,EUI_48_address,EUI_64_address,GUTI,DUID) ×{0, 1, 255, 256, 2**64-1, 2**64, 2**128-1, True}, through maker →pack()→unpack(): declaredlengthmatches actual packed octet count, and the recovered value (int.from_bytesfor the sixBytesFieldsubtypes,int(ipaddress.IPv6Address(...))forIPv6_Address) equals the original input exactly, for all 56 combinations. Zero mismatches. identifier=-5on all 8 subtypes: all raiseProtocolError(none leak a different exception type).NAI+0x1234: message containsstr(4660)-- confirmed ('str(4660)' in str(e)isTrue).NAI+-5: gives the generic non-negative-int message ("...must be a non-negative int, not -5"), not the NAI-specific "must be str, not int" message -- confirmed, since the negative-int guard runs before the subtype dispatch.NAI+str(0x1234): round-trips, packed payload decodes to'4660'-- confirmed.
Also re-derived the subtest counts independently rather than trusting the brief's numbers: checked out a8c214ab4, regenerated fixtures, ran tests/protocols/internet/test_mh_unit.py → 37 passed, 8 warnings, 318 subtests passed. Checked out 6a9e4e427 (PR head), regenerated fixtures, ran the same file → 37 passed, 8 warnings, 332 subtests passed. 332 - 318 = 14, matching the diff's two new subTest loops (2 cases for IPv6_Address over-bound + 6 subtypes × 2 id_len cases = 14).
4. Wrong-type leaks vs. origin/main
Compared the same battery of wrong-type inputs (bytes/str/float/None/list/dict) against NAI, IMSI, and IPv6_Address on origin/main (5182ad0ce, which predates the entire #467 branch) and on 6a9e4e427. Exception type and message text are identical in every case I tested -- e.g. IPv6_Address + bytes(b'xyz') gives AddressValueError: b'xyz' (len 3 != 16) is not permitted as an IPv6 address on both; NAI + bytes gives AttributeError: 'bytes' object has no attribute 'encode' on both; IMSI + str gives struct.error: argument for 's' must be a bytes object on both. The only input where the two commits diverge is IPv6_Address + int >= 2**128, which goes from AddressValueError on origin/main to ProtocolError on 6a9e4e427 -- exactly the intended change, and exactly nothing else. These wrong-type leaks belong to #469 and are unaffected by this PR, as claimed.
5. Comment block, docstring, and PR body -- anything measurably false?
- The new in-code comment block (explaining why the guard sits inside the
IPv6_Addressbranch rather than the shared negative-int guard, and why it's an explicit check rather than a try/except) matches everything I measured above. - One real gap, filed as an inline comment (id 4047785617, line 7666): the docstring's
Raises:clause still lists only twoProtocolErrorconditions (negative int; any int withNAI) and was not updated to mention the third this same commit adds (IPv6_Addressint>= 2**128). Non-blocking doc-completeness issue, not a behavioral one. - PR body claim "the construction line is byte-for-byte unchanged from
origin/main" (referring toif not isinstance(identifier, ipaddress.IPv6Address): identifier = ipaddress.IPv6Address(identifier)) -- confirmed viagit diff origin/main..6a9e4e427 -- pcapkit/protocols/internet/mh.py: those two lines appear only as unchanged context, never touched by either commit in this branch's history. - PR body claim "disabling only the new guard's condition fails exactly the two new subtests and nothing else" -- could not independently execute this exact mutation without editing the tracked source file, which is outside my review-only mandate. I tried an in-memory-only equivalent (monkeypatching a modified copy of
MH._make_opt_mn_idonto the class), buttest_mh_unit.py'ssetUpcallspurge_modules(['pcapkit'])before every test, which forces a fresh disk-based re-import of the wholepcapkitpackage (and hence a brand-new, unpatchedMHclass) before each test method runs -- so a pure in-memory patch has no effect on the suite. I confirmed the patch mechanism itself was sound (calling the patched function directly, outside the test harness, correctly raised the underlyingAddressValueErrorinstead ofProtocolErrorwhen the guard was disabled), which is consistent with the claim, but I can't independently execute the exact mutation-testing claim as stated without editingmh.py. Flagging this as an unverified-but-plausible claim rather than repeating it as confirmed.
Test suite
tests/protocols/internet/test_mh_unit.py, at a8c214ab4: 37 passed, 8 warnings, 318 subtests passed in 34.60s
tests/protocols/internet/test_mh_unit.py, at 6a9e4e427: 37 passed, 8 warnings, 332 subtests passed in 34.22s
Both runs used PYTHONPATH=<worktree root> with pcapkit.__file__ asserted to start with the worktree root beforehand, and examples/generators/make_samples.py re-run against each checkout before testing.
Verdict
One non-blocking documentation gap filed inline (stale Raises: enumeration); everything else -- the guard's placement, its exact boundary, its bool/IPv6Address-instance/str bypass behavior, the narrow-vs-wrap scoping decision, all previously-confirmed round-trip and rejection behavior, and the wrong-type leaks' byte-for-byte equivalence to origin/main -- checks out. CI is fully green (21 SUCCESS + 2 SKIPPED-by-design CheckRuns, plus pyup.io/safety-ci SUCCESS).
GOOD TO MERGE at 6a9e4e427
Docstring only; no executable line changes. The review of 6a9e4e4 caught that its Raises: clause was left stale by that same commit. It enumerated two conditions -- a negative int, and an int of any value with NAI -- while the guard added three lines below introduces a third: an int of 2**128 or above with IPv6_Address. Three ways to raise ProtocolError, two documented. Verified all three raise ProtocolError (a BaseError) at this sha, rather than reading the code and assuming. The clause now also says why the ceiling exists only for IPv6_Address -- its wire form is a fixed 16 octets, where the other subtypes have no ceiling and simply pack into more -- because the asymmetry is the part a caller would otherwise read as an inconsistency. tests/protocols/internet/test_mh_unit.py: 37 passed, 332 subtests, 0 failed, unchanged.
| # subtype resolves to a :class:`~pcapkit.corekit.fields.strings.StringField` | ||
| # or :class:`~pcapkit.corekit.fields.strings.BytesField`, neither of | ||
| # which accepts an ``int`` (c.f. #467). | ||
| def __init__(self, type: 'Enum_Option', length: 'int', subtype: 'Enum_MNIDSubtype', identifier: 'bytes | str | IPv6Address') -> 'None': ... |
There was a problem hiding this comment.
we still drop the int here?
…r takes one (#467) Comment only; no executable line changes and no annotation changes. The owner asked, reasonably, why the __init__ type stub does not accept int now that the maker does. The answer is that the two annotations describe different boundaries and the old comment did not say so: the maker's `identifier: 'bytes | str | IPv6Address | int'` is what a caller may pass, while the schema's is what the schema can hold, and the maker converts between them before constructing the schema at all. Measured through the maker rather than read off the code: identifier 0x1234 arrives at the schema as b'\x124' for IMSI and DUID and as IPv6Address('::1234') for IPv6_Address. No int ever reaches the constructor, so admitting one in the stub would document a value the schema cannot hold -- and would mislead, because handing a raw int to the StringField or BytesField that mn_id_selector resolves for every subtype but IPv6_Address is exactly the #467 defect: struct.pack() cannot consume it. The previous comment said only "neither of which accepts an int", which was true but read as though int were rejected outright, which is no longer the case anywhere the caller can see. Merged origin/main (f7b5cc5, #470) first -- clean, touching only ipaddress.py and its tests, neither owned by this branch. tests/protocols/internet/test_mh_unit.py: 37 passed, 332 subtests, 0 failed, unchanged.
1 participant
Closes #467.
MH._make_opt_mn_idacceptedintas a documented identifier type, but forevery
Enum_MNIDSubtypeexceptIPv6_Addressanintproduced a schema thatcould not be packed at all, with a declared
lengththat was wrong regardless.This is the other half of #448. PR #464 fixed the
IPv6_Addresssubtype bytaking the width from
subtype_val; theelif isinstance(identifier, int)branch still sized from the identifier's Python type for the remaining seven.
Before
Measured on
fa128959e,identifier=0x1234:lengthpack()AttributeError: 'int' object has no attribute 'encode'struct.error: argument for 's' must be a bytes objectstruct.errorstruct.errorstruct.errorstruct.errorstruct.errorlength=3came frommath.ceil(identifier.bit_length() / 8) + 1— theinteger's own width, not the octet count the subtype's field packs. NAI fails
differently from the other six because its field is text (
StringField) ratherthan raw octets (
BytesField).The decision: reject, for all seven
Neither
StringFieldnorBytesFieldhas any int-conversion logic, unlikeIPv6AddressField, which does its ownipaddress.ip_address()conversion —that is why an
intonly ever worked forIPv6_Address. Every othersubtype's field is variable-length, sized from the wire
lengthheader ratherthan from anything
subtype_valfixes, so there is no non-arbitrary width toconvert an
intinto; picking one would reintroduce the same type-vs-subtypeconfusion without the crash.
So
_make_opt_mn_idnow raisesProtocolError— matching this file's ownconstruction-error convention — naming the subtype and the type it accepts
(
strfor NAI,bytesfor the rest).IPv6_Addresscontinues to acceptintunchanged.
After
All seven raise an in-library
ProtocolErrorrather than a bare stdlibexception, and the
IPv6_Addresspath is untouched — verified independently ofthe change's own tests:
Type union
Schema_MNIDOption'sTYPE_CHECKING __init__stub dropsint, matching itsown class attribute's already-narrower annotation.
_make_opt_mn_id's top-levelidentifierunion deliberately keepsint:narrowing it made mypy flag the
isinstance(identifier, int)branch asunreachable (3 "disjoint bases" plus 1 "unreachable statement"), and
intremains genuinely handled for
IPv6_Address, so the union is accurate as itstands. The promise is corrected in the docstring's
Args:/Raises:proseinstead.
Verification
_mh_option_overrideshas noMN_ID_OPTION_TYPEentry, so the generator exercises the real default(
identifier='::', subtypeIPv6_Address), a path this fix does not touch.All five
examples/captures/options-*.pcaparecmp-identical against apristine
fa128959eworktree.tests/protocols/internet/test_mh_unit.py:git merge-treeauto-merges thatfile with no
CONFLICT.tests/protocols/test_option_roundtrip_unit.pyuntouched, and it has noMN_IDentry either before or after.subtests; after 1007 passed / 17 skipped / 1560 subtests — exactly the new
test and its 7 subtests. Zero failures either side.
pcapkit/: 125 errors before and after, identical once line andcolumn numbers are normalised. No new
# type: ignore.Related, deliberately not fixed here
The same union lists
bytesas accepted for NAI, butbyteshas no.encode(), so NAI fails identically for abytesidentifier — and the mirrorcase holds too, a
stridentifier for one of the six octet subtypes dying instruct.error. Both are the same family as this issue but are aboutbytes/strrather than
int, so they are filed separately rather than folded in.Revision at
a8c214ab4— this supersedes the approach described aboveAppended rather than edited in, because the review comments above quote the original text.
The body above is wrong on its central claim. It says there is no non-arbitrary width to convert an
intinto for the sixBytesFieldsubtypes. There is:math.ceil(identifier.bit_length() / 8), which is self-consistent with the declaredlengthby construction and round-trips exactly. The pre-#467 defect was never the sizing — that was already right — it was thatidentifierstayed anintafterwards and reachedBytesFieldunconverted, whichstruct.pack()cannot do anything with. I accepted that "no non-arbitrary width" conclusion from a subagent and repeated it here and in the issue without re-deriving it. It should not have gone in.At the repo owner's request, this revision converts instead of rejecting:
BytesFieldsubtypes (IMSI,P_TMSI,EUI_48_address,EUI_64_address,GUTI,DUID) convert viaidentifier.to_bytes(max(1, math.ceil(identifier.bit_length() / 8)), 'big'). Themax(1, …)floor matters:bit_length()is0for0itself, which would otherwise declare a zero-octet identifier and collapse "the identifier's value is 0" into "there is no identifier".IPv6_Addressis untouched — stillipaddress.IPv6Address(…), which both converts and validates, at the spec-fixed 16 octets (MH MN-ID option sizes its identifier from the Python type rather than the subtype, so even the default arguments mis-declare length #448).NAIstill rejects anint, and that is a judgement call rather than a mechanical limit.str(identifier)packs and round-trips fine; the argument for refusing is that an NAI is a network access identifier (user@realm, RFC 4283), so a bare decimal-digit string is mechanically valid and semantically nonsense — the same "accepts a value that means the wrong thing" MH MN-ID: an int identifier cannot be packed for any subtype except IPv6_Address, and its declared length is wrong #467 removed, only relocated. The message namesstr(…)so a caller who wants that can spell it explicitly.intis rejected for every subtype, by a guard placed before the subtype dispatch. Inside theintbranch — where it was at1ea8b56ab— theIPv6_Addressdispatch never reaches it, soidentifier=-5, subtype=IPv6_AddressleakedAddressValueError: -5 (< 0) is not permitted as an IPv6 address.AddressValueErrorsubclassesValueError, i.e. a bare stdlib exception escaping the very handler whose purpose is to stop that. Measured directly at1ea8b56ab, then hoisted.Verification, through the maker →
pack()→unpack()rather than a hand-built schema carrying a self-consistentlengththe maker would never produce — all 8 subtypes ×{0, 1, 0xff, 0x1234, 0x100000000, 2**128-1, True}. EveryBytesFieldsubtype round-trips identically;0→len=2/b'\x00',0x1234→len=3/b'\x124',2**128-1→len=17/16 ×\xff.-5raisesProtocolErroron all eight.NAI+str(0x1234)packs to a payload decoding as'4660'.tests/protocols/internet/test_mh_unit.py: 37 passed, 318 subtests, 0 failed.Deliberately not fixed here: wrong-type identifiers still escape as bare stdlib exceptions (
bytesforNAI→AttributeError,strfor the octet subtypes →struct.error, and so on). Unchanged frommain, tracked as #469.Follow-up at
6a9e4e427— the upper boundThe review of
a8c214ab4found the mirror of the leak that sha's own guard had just closed: anintidentifier at or above2**128withsubtype=IPv6_Addressstill reachedipaddress.IPv6Address()and raisedAddressValueError, aValueErrorsubclass. Verified before fixing —2**128and2**140both leaked,2**128 - 1packed normally atlength17.The check went inside the
IPv6_Addressbranch rather than into the shared negative-int guard, because this bound is subtype-dependent where the negative bound is not:2**140is a valid identifier for the sixBytesFieldsubtypes, producinglength19 (and 18 for2**128). The test asserts that as well, so a future guard cannot be hoisted to cover them by mistake. It is also not implemented by wrapping theIPv6Addressconstruction, which would have swallowed the wrong-typeAddressValueErrorat the same line — #469's subject, not this PR's to annex.Pre-existing since #448, not a regression from this branch: the construction line is byte-for-byte unchanged from
origin/main.Regression-proved: disabling only the new guard's condition fails exactly the two new subtests and nothing else; restoring it passes.
tests/protocols/internet/test_mh_unit.py: 37 passed, 332 subtests, 0 failed (318 before).Why it survived the first sweep, which is the transferable part: my probe covered every subtype but stopped at
2**128 - 1, one value below where the answer changes. Probing a large value is not probing the boundary.5cf1740ad— docstring only. The review of6a9e4e427caught that its ownRaises:clause had been left stale by that commit: it listed a negativeintand anintwithNAI, but not the third condition the new guard introduces, anintof2**128or above withIPv6_Address. All three now listed, each measured rather than read off the code, along with why the ceiling exists only forIPv6_Address. No executable line changed, so theGOOD TO MERGEat6a9e4e427stands by content.