Skip to content

MH MN-ID: an int identifier cannot be packed for any subtype except IPv6_Address, and its declared length is wrong #467

Description

@JarryShaw

Summary

MH._make_opt_mn_id accepts int as a documented identifier type, but for every
Enum_MNIDSubtype except IPv6_Address an int identifier produces a
schema that cannot be packed at all — and the declared length it computes is
wrong regardless.

This is the other half of #448. That issue fixed the IPv6_Address subtype, where
the width is now taken from subtype_val; the elif isinstance(identifier, int)
branch still sizes from the identifier's Python type for all the remaining
subtypes, which is the same type-vs-subtype confusion.

Reproduction

Measured on 074ca2c09 (current main). Identical on PR #464's branch, so this
is pre-existing and not a regression from that PR.

from pcapkit.const.mh.option import Option
from pcapkit.const.mh.mn_id_subtype import MNIDSubtype
from pcapkit.protocols.internet.mh import MH

proto = object.__new__(MH)
for st in ('NAI', 'IMSI', 'P_TMSI', 'EUI_48_address', 'EUI_64_address', 'GUTI', 'DUID'):
    s = proto._make_opt_mn_id(Option.MN_ID_OPTION_TYPE,
                              subtype=getattr(MNIDSubtype, st),
                              identifier=0x1234)
    s.pack()
NAI:            length=3  pack raises AttributeError: 'int' object has no attribute 'encode'
IMSI:           length=3  pack raises struct.error: argument for 's' must be a bytes object
P_TMSI:         length=3  pack raises struct.error: argument for 's' must be a bytes object
EUI_48_address: length=3  pack raises struct.error: argument for 's' must be a bytes object
EUI_64_address: length=3  pack raises struct.error: argument for 's' must be a bytes object
GUTI:           length=3  pack raises struct.error: argument for 's' must be a bytes object
DUID:           length=3  pack raises struct.error: argument for 's' must be a bytes object

Note NAI fails differently from the other six — AttributeError from an
attempted .encode() rather than struct.error — because that subtype's field is
a string rather than raw octets.

The length=3 is itself wrong in every case: it comes from
math.ceil(identifier.bit_length() / 8) + 1, i.e. it describes the integer's own
width rather than the octet count the subtype's field will pack.

Why it matters

int is a documented identifier type, not an accidental one:

  • pcapkit/protocols/internet/mh.py:7637 — identifier: 'bytes | str | IPv6Address | int' = '::'
  • pcapkit/protocols/schema/internet/mh.py:727 — the Schema_MNIDOption.__init__
    stub carries the same union.

So the signature promises support that does not exist for 7 of the 8 subtypes,
and no test exercises it — which is why #448 was found only for the default
subtype.

Both exception types are also bare stdlib exceptions rather than in-library ones
from pcapkit.utilities.exceptions, the same family as #465, #458 and #438
(closed).

Suggested direction

Decide per subtype what an int identifier should mean, then either convert it to
the subtype's wire form with a width taken from subtype_val (as #448 now does
for IPv6_Address), or reject it with a ProtocolError/FieldValueError naming
the subtype — and narrow the documented type union to match whichever is chosen.
Silently accepting a value that cannot pack is the worst of the three.

Provenance

Surfaced by the review of #464 (the fix for #448), which flagged it as out of
that PR's scope — correctly. I verified it independently on both trees before
filing, and the per-subtype breakdown above is mine: the review reported
struct.error generally, which holds for six of the seven but not for NAI.

No activity

Activity on this issue will appear here.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugIssues reporting a defect (set by the bug report template; a default, not an assessment)

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions