Repository navigation
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
Description
Activity
Folding in a third member of this family, found by the re-review of #468 and
verified here on both trees._make_opt_mn_idalso lets a bareTypeErrorescape from
pcapkit/protocols/internet/mh.py:7717—id_len = len(identifier)— for an
identifier that supports neitherintnor__len__:identifier=None main: TypeError #468 branch: TypeError identifier=1.5 main: TypeError #468 branch: TypeError identifier=IPv6Address('::1'), subtype=NAI main: NO RAISE, length=17 #468 branch: TypeErrorThe first two are pre-existing and unchanged. The third is a behaviour change
worth noting separately: onmain, handing anIPv6Addressto the textNAI
subtype raises nothing at all and declareslength=17, sizing it as though it
were an IPv6 address — silently wrong. #468 turns that into a loud
TypeError, which is an improvement in kind even though the exception type is
still not in-library.So the full shape of this issue is that
_make_opt_mn_idaccepts four documented
identifier types while each subtype's field accepts exactly one of them, and
every mismatch escapes as a bare stdlib exception rather than a
ProtocolError:bytesfor the text subtype (NAI) —AttributeError: 'bytes' object has no attribute 'encode'strfor the six octet subtypes —struct.error: argument for 's' must be a bytes object- anything without
__len__orintbehaviour —TypeError: object of type 'X' has no len()
intis already handled: #468 rejects it withProtocolErrorfor every subtype
butIPv6_Address. Extending that same per-subtype rejection to cover the cases
above would close this issue in one piece, and would also remove the
silently-wrongIPv6Address-for-NAIpath thatmainstill has.
Summary
MH._make_opt_mn_iddocumentsbytes | stras accepted identifier types forevery MN-ID subtype, but each subtype's field accepts only one of the two.
Passing the other builds a schema that cannot be packed, and fails with a bare
stdlib exception rather than an in-library one.
This is the same family as #467 (which covered
int) and #448 (which coveredthe
IPv6_Addresswidth), but it is aboutbytes/strand is not fixed byeither.
Reproduction
Measured on
fa128959e(currentmain). Identical on PR #468's branch, sopre-existing and not a regression from that PR — I checked both trees.
So the defect is symmetric:
bytesfails for the one text subtype, andstrfails for the octet subtypes. Only the matching pairing works.
Both failures happen at
pack()rather than at construction, and neither is aninstance ofpcapkit.utilities.exceptions.BaseError`, so a caller cannot catchthem with the library's own exception hierarchy.
Why it matters
The type union is documented, not accidental —
_make_opt_mn_id'sidentifierparameter is annotated
bytes | str | IPv6Address | int, and theSchema_MNIDOption.__init__stub carries a matching union. So the signaturepromises
bytesandstrinterchangeably for all subtypes when in fact thechoice is fixed per subtype by whether that subtype's field is a
StringFieldor a
BytesField.NAIis the only text subtype;IMSI,P_TMSI,EUI_48_address,EUI_64_address,GUTIandDUIDare octet subtypes.Suggested direction
The same shape as #467's fix: decide per subtype, then either convert (encode a
strfor the octet subtypes, decodebytesfor NAI — noting that an encodinghas to be chosen and named if so) or reject with a
ProtocolErrorsaying whichtype that subtype takes. #467's fix already added exactly that kind of
per-subtype rejection for
int, so the mechanism and message style are inplace to extend.
Whichever is chosen, the documented union should end up describing what is
actually accepted.
Provenance
Surfaced while verifying #468 (the fix for #467). Its author flagged the
bytes-for-NAI half and correctly left it out of scope; thestr-for-octet-subtypes half is mine, found while measuring the first. Both verified on
mainand on the PR branch before filing.