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

Closed

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.

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