ipv6/mh: fix option padding, and with it construction, which was wholly broken

[JarryShaw]

This started as the Pad1 parse defect and turned out to be much larger. The Pad1 bug is real, but it is not the headline.

Construction was completely broken for HOPOPT, IPv6-Opts and MH

Not "broken for exotic option sets" — broken for no options at all. Verified against current main:

HOPOPT     no options  -> FAIL ProtocolError: HOPOPT: invalid format
HOPOPT     empty list  -> FAIL ProtocolError: HOPOPT: invalid format
IPv6_Opts  no options  -> FAIL ProtocolError: IPv6-Opts: invalid format

and where it did not raise, it emitted a malformed header:

HOPOPT  Tunnel_Encapsulation_Limit  total_length=8  emitted=10  (04010301050000000000)
HOPOPT  Router_Alert                total_length=8  emitted=10
MH      brr advice (4 octets)       declares 16, emits 12
        -> REPARSE FAIL AttributeError: 'PadOption' object has no attribute 'timestamp'

Construction packs and then immediately re-parses its own bytes, so a length that disagrees with the octets is fatal.

After:

HOPOPT / IPv6_Opts (identical for both)
  tun (3 octets)   3b00040103010100                    8 octets, declares  8  ok
  home addr (18)   3b02c910...01020000                24 octets, declares 24  ok
  tun + alert      3b010401030101000502000001020000   16 octets, declares 16  ok
  no options       3b00010400000000                    8 octets, declares  8  ok
MH
  padn(4)          3b010000000000000104000000000000   16 octets, declares 16  ok
  no options       3b00000000000000                    8 octets, declares  8  ok

Every case re-parses and repacks byte-identically.

Two compounding faults

1. _make_opt_pad(PadN, length=pad_len) emitted two octets more than it accounted for. A PadN spends 2 octets on its own type and Opt Data Len, so filling pad_len octets needs Opt Data Len = pad_len - 2.
2. Each option was aligned to 8 octets on its own, leaving the options area a multiple of 8 — but read sizes it as hdr_ext_len * 8 + 6, six short of a multiple of 8, because the 2-octet fixed header precedes it. math.ceil((total_length - 6) / 8) rounded that shortfall away, so the header declared 6 octets more than it carried.

Alignment now derives from a running length that includes the fixed part, and make divides exactly instead of rounding up. options is None no longer short-circuits to an empty area: the minimum header is 8 octets of which 2 are fixed, so "no options" means 6 octets of padding, not nothing.

MH's modulus is its own, not 6. Its fixed part is 6 octets, so the message data is well formed at ≡ 2 (mod 8) — an option-less MH message is already aligned where an IPv6 options header needs 6 octets of padding. MH._pad_mh_message puts padding in the mobility options, where RFC 6275 §6.2.5 does. A message with an opaque body (UnknownMessage, ExperimentalMessage, raw bytes) has nowhere to put it and is reported via ProtocolWarning rather than silently misdeclared.

The Pad1 defect was not where the note said

Both schemas already declared the length octet conditionally, and Option.post_process clamps len to 0 for Pad1 — so the documented ProtocolError path was unreachable. The break was one field further along, in PadOption.pad:

pad: ... = PaddingField(length=lambda pkt: pkt.get('len', 0))

A skipped ConditionalField is recorded as NoValue, not omitted (schema.py:644), so .get('len', 0) returned the singleton rather than the default, PaddingField interpolated it into a struct template, and the parse died in the field layer:

struct.error: bad char in struct format

Fixed in the schema layer in all three modules, so a Pad1 reads one octet and nothing downstream compensates. Rejected: splitting PadOption into Pad1/PadN classes (churns __all__, the type hints, the docs, and a test asserting registry[Pad1] is registry[PadN]), and wrapping the PaddingField in a ConditionalField (bypasses the PaddingField special-case in both pack and unpack loops).

The sweep for the same defect class

I exercised every length callback in pcapkit/protocols/schema/ with its class's conditional fields set to NoValue — by script, not grep. Only the three PadOption cases are genuinely reachable:

module	verdict
hopopt, ipv6_opts, mh	real defects — Pad1 is the only code that skips the length octet. Fixed
ipv4	safe — EOOL/NOP have dedicated field-less schema classes, so no lambda runs with length skipped (verified by parsing)
httpv2 (3), l2tp	structurally safe — the padding field is itself wrapped in a ConditionalField with the same predicate as the field its length reads
tcp, ipv4, pcapng (9 sites)	correct — __option_padding__ comes from OptionField, not a ConditionalField, so the .get default is reachable and intended

Also fixed a two-octet overshoot when re-making a parsed padding option: the data model's length counts the whole option, the schema's counts only Opt Data Len. MH copied one into the other unconverted; HOPOPT and IPv6-Opts ignored the argument entirely, silently collapsing a re-made padding option to a single Pad1. An existing test carried a comment calling this a known pre-existing defect — that comment now records the fix.

Seven existing assertions changed, each because it encoded the bug

None turned out to be correct. Every change carries the octet count justifying it, in a comment:

assertion	old	new	arithmetic
total_length	16	14	10 (CALIPSO) + 4 (PadN) = 14; 2 + 14 = 16 = 2×8
schema_length	8	14	8 + 6 = 14; 2 + 14 = 16
dict_length	24	22	3+3+8+6+1+1 = 22; 2 + 22 = 24 = 3×8
made_empty.options	[]	['PadOption'], + len(bytes()) == 8	minimum header is 8 octets, 2 fixed, so an empty area cannot exist on the wire
MH _make_opt_pad(...).length	4	2, + len(pack()) == 4	a 4-octet PadN spends 2 on its header

Two test inputs also changed, and that is the part worth scrutinising. pad1_length and no_pad_length had 6- and 8-octet options. Once the alignment is correct, a 6-octet option leaves the header already aligned, so the "two Pad1" case would emit no padding at all and lose its coverage entirely. The inputs moved to 4 and 6 octets so each case still exercises the branch it was written for:

• pad1_length: 4 + 1 + 1 = 6; 2 + 6 = 8 — two Pad1, as intended
• no_pad_length: 6 octets, nothing added; 2 + 6 = 8 — genuinely no padding

Every make_options case now also asserts len(emitted) == returned_length and (returned_length + 2) % 8 == 0, so the accounting cannot drift from the bytes again.

Verification

Round trips now go through the real construction path (protocol_cls(next=…, options=[…]), MH(next=…, data={…})), which is what the broken construction had been blocking — the hand-built-header round trip is gone, the wire-level tests are kept. Every new test was confirmed to fail against main's source with the tests held constant.

run	result
CI selection at HEAD (stated baseline)	539 passed, 17 skipped, 455 subtests
after the Pad1 read fix alone	543 passed, 17 skipped, 467 subtests
after everything	553 passed, 17 skipped, 503 subtests, 0 failed

mypy on all six changed modules: no new errors. The 5 failures in tests/protocols/internet are *_runtime.py only (excluded from the CI selection) and are missing generated fixtures, identical before and after.

Left alone

• _make_hopopt_options still discards a caller-supplied padding option and regenerates the canonical layout, so a byte-exact round trip of a hand-placed padding layout is not possible by design.
• A stale one-word comment in schema/internet/mh.py's Option.post_process (says length is always 1, code sets 0; the code is right — that field is Opt Data Len, absent for Pad1).

[Copilot]

🟡 Changes recommended

MH construction can still emit malformed headers for unaligned opaque bytes bodies because it warns but does not ensure emitted bytes match the declared Header Len.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR fixes IPv6 extension header (HOPOPT/IPv6-Opts) and Mobility Header (MH) option padding so that (1) Pad1 parses correctly as a 1-octet option, and (2) constructed headers declare lengths that exactly match emitted bytes (including correct 8-octet alignment rules).

Changes:

• Fix Pad1 parsing in HOPOPT/IPv6-Opts/MH schemas by making padding-data length callbacks treat skipped ConditionalField values (NoValue) as zero.
• Fix header construction/padding accounting for HOPOPT/IPv6-Opts (2-byte fixed header) and MH (6-byte fixed header) so declared lengths match emitted bytes and “no options” produces the minimum valid header.
• Expand/adjust unit tests to assert byte-identical round trips and correct length/alignment behavior; update docs to include the new helper functions.

File summaries

File	Description
tests/protocols/internet/test_mh_unit.py	Adds/updates MH padding parsing + construction round-trip tests and fixes prior assertions that encoded buggy behavior.
tests/protocols/internet/test_ipv6_extension_unit.py	Adds HOPOPT/IPv6-Opts wire-parse tests for Pad1/PadN and construction round-trip/alignment assertions.
pcapkit/protocols/schema/internet/mh.py	Introduces pad_opt_data_len and updates PadOption to size padding data safely for Pad1.
pcapkit/protocols/schema/internet/ipv6_opts.py	Same pad_opt_data_len fix pattern for IPv6-Opts padding option schema.
pcapkit/protocols/schema/internet/hopopt.py	Same pad_opt_data_len fix pattern for HOPOPT padding option schema.
pcapkit/protocols/internet/mh.py	Fixes MH construction length calculation and adds padding helpers to align MH per its 6-byte fixed header.
pcapkit/protocols/internet/ipv6_opts.py	Fixes IPv6-Opts construction so empty options still produce a valid padded minimum header and length math is exact.
pcapkit/protocols/internet/hopopt.py	Fixes HOPOPT construction so empty options still produce a valid padded minimum header and length math is exact.
docs/source/pcapkit/protocols/internet/mh.rst	Documents the new pad_opt_data_len helper.
docs/source/pcapkit/protocols/internet/ipv6_opts.rst	Documents the new pad_opt_data_len helper.
docs/source/pcapkit/protocols/internet/hopopt.rst	Documents the new pad_opt_data_len helper.

Review details

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[JarryShaw]

Copilot's finding was correct, and it is fixed in ddeacf8c2 — though not the way its patch suggested, and not the way I first tried.

Quantifying it first

The warning did not stop the malformed header from shipping. length is (len(data) + 6) // 8 - 1, which floors:

len(body)  emitted  declared
    4        10         8     MISMATCH
    6        12         8     MISMATCH
    8        14         8     MISMATCH
    2         8         8     ok
   10        16        16     ok

A parser reads the 8 octets it was promised and misinterprets the remainder. So this was a real wire-format defect, not a cosmetic one.

What I did, and a course correction

My first attempt made it raise, reasoning that appending octets to a body whose layout is unknown corrupts the body rather than the length field. Jarry overruled that, correctly: on a construction path the caller has asked for a packet and the shortfall is recoverable, so completing it beats refusing. That is Copilot's option (a).

So the padding now goes into the body itself and the warning says so — the warning is meaningful precisely because the emitted body is no longer byte-for-byte what was handed in.

One ProtocolError is kept, for a schema body with neither options nor a bytes payload, where there is genuinely nowhere to put the octets. No such body exists in-tree.

Two existing assertions changed — both pinned the bug

Worth calling out rather than burying, since they change observable output. Both constructors return raw bytes, so both take the padding branch:

call	before	after
make(type=custom, data={'value': b'dict'}).data	b'dict'	b'dict' + b'\x00' * 6
make(type=custom, data=unknown_message).data	b'model'	b'model' + b'\x00' * 5

The first emitted 10 octets while declaring 8; the second 11 while declaring 8.

The test that asserted "warns and returns unchanged" now asserts the padding, that 6 + len(body) is a multiple of 8, and that the declared length equals what is emitted. It also pins that an already-aligned body is returned untouched and unremarked — the warning should only fire when the body actually changed.

Fixture-free unit tier: 556 passed, 17 skipped, 511 subtests, 0 failed.