Duplicated wire-unit arithmetic wants a shared module: 25 MH sites, 126 option sites, and two near-identical option files

[JarryShaw]

Found by strand 5 of the post-wave-1 consistency sweep — the strand the owner added at c54727865: check if there're duplicated common methods/logic that can be extracted into a shared module. Measurements below are mine, taken on 5959a437c.

Unlike the other sweep issues, nothing here is currently producing a wrong answer. Every site is consistent today. The case for acting is that this exact duplication has already caused defects twice — #487 and #398 — and the remaining copies are the same pattern awaiting the same fate. Filed as structural work, not as a defect.

The evidence that this is not aesthetic: #398 fixed one bug six times

29dfd351b — "ipv6/mh: fix option padding, and with it construction, which was wholly broken (#398)":

pcapkit/protocols/internet/hopopt.py           | 130 ++++++++---
pcapkit/protocols/internet/ipv6_opts.py        | 131 ++++++++---
pcapkit/protocols/internet/mh.py              | 130 ++++++++++-
pcapkit/protocols/schema/internet/hopopt.py    |  46 +++-
pcapkit/protocols/schema/internet/ipv6_opts.py |  46 +++-
pcapkit/protocols/schema/internet/mh.py        |  47 +++-

One conceptual bug — a +2/-2 mismatch between the whole-option length and the on-wire Option Length field — diagnosed once and then patched independently in six places with near-identical diffs. The surviving explanation, mh.py:7446-7453:

Data_PadOption.length counts the whole option, whereas Schema_PadOption.length is the Option Length field — two octets fewer... Copying one into the other unconverted is why re-making a parsed PadN used to come back two octets too long.

#487 is the other precedent: Hdr Ext Len computed in four places, with four different wrong answers.

1. hopopt.py and ipv6_opts.py are the same implementation twice

The strongest case in the codebase, because there is no specification reason for these two to differ: Hop-by-Hop Options and Destination Options draw on the same IANA registry (RFC 8200 §4.3/§4.6), which is why every per-option method is the same in both files.

Measured:

hopopt.py    1953 lines
ipv6_opts.py 1965 lines
full diff:    406 changed lines out of ~1950

The overwhelming majority of those 406 lines is renaming — HOPOPT→IPv6_Opts, hopopt→opt, docstring rewrap — not different logic.

Two corrections to how this reached me. The sweep reported 666 changed lines; I measure 406. It also reported that diffing the def lines "produces zero output"; it does not — ipv6_opts.py has an extra alias method, and the options entry point is named _read_hopopt_options versus _read_ipv6_opts_options. The method sets are near-identical, not identical. The conclusion holds; the specific claims were overstated.

A shared options base class, or a mixin the two thin header classes compose, would collapse this and would have made #398 a one-place fix.

2. MH message length: 25 identical read-side sites, while the write side was already unified

Every _read_msg_* in mh.py computes length=(header.length + 1) * 8. Confirmed count: 25 occurrences, all textually identical — no drift yet.

That value feeds self._decode_next_layer(mh, schema.next, length - mh.length) at mh.py:1369, which is precisely the role Hdr Ext Len played in #487.

The asymmetry is the point: the write side already got the treatment, unified into one expression at mh.py:1438 (length=(len(data_val) + 6) // 8 - 1), while the read side kept 25 copies. One _mh_message_length(hdr_len_field) helper, called from all 25, mirrors what IPv6_Route._make_hdr_ext_len/ipv6_route_data_length already do.

3. IPv6_Route read side: the unfinished half of #487/#489

ipv6_route.py:447, 514, 562, 615 — all four _read_data_type_* methods repeat length=header.length * 8 + 8. Confirmed count: 4.

#489 unified the write side into _make_hdr_ext_len/ipv6_route_data_length. This read-side "total header octets" figure is a third related-but-distinct quantity — 8 + 8k here versus ipv6_route_data_length's 4 + 8k — and is still computed four ways. Small blast radius, but immediately adjacent to work just merged, so cheap to finish now: an ipv6_route_header_length() beside the existing helper.

4. Option +2 arithmetic: 126 sites across three files

Every _read_opt_* across mh.py (92 sites), hopopt.py (17) and ipv6_opts.py (17) computes length = <schema>.length + 2, adding back the Type+Length prefix that RFC 6275 §6.2 and RFC 8200 §4.3 exclude. This is the quantity #398 got wrong.

Only four make-side places consume a stored .length (mh.py:7457, mh.py:9139, hopopt.py:1384, ipv6_opts.py:1396) and all four correctly subtract 2 — four correct sites beside 126 duplicated ones with no shared helper between them.

The honest severity, which the sweep stated well: the option round-trip harness would not catch a fresh regression here, because _make_opt_* methods other than Pad recompute length from len(value) rather than reading .length back. Wire bytes stay correct; what drifts is the informational .length on a parsed packet. That is also why it has not bitten again — but nothing stops a new option's maker from following Pad's original pattern rather than its fixed one.

Deliberately not proposed: the TCP/IP reassembly pair

Recorded so a future sweep does not "fix" it. reassembly/tcp.py and reassembly/ip.py look like near-duplicates and must not be merged: they resolve overlapping data in opposite directions because their specifications demand it. RFC 791 lines 1892-1894 — "either identically or through a partial overlap, this procedure will use the more recently arrived copy in the data buffer and datagram delivered" — is last-write-wins; RFC 9293 §3.10 is first-write-wins. IP is last-write-wins, TCP is first-write-wins, by specification. Verified against the RFC text; #443 settled the TCP side deliberately.

Retired: extraction not worth it, with reasons

• pcap.py/pcapng.py don't scale fragment offset by 8 while dpkt.py/scapy.py/pypcapfile.py do. Not duplication that drifted: the native engines consume pcapkit's own already-scaled Data_IPv4.offset, whereas the third-party engines expose the raw 8-octet-unit field and must scale it themselves. The same relationship, correctly expressed differently because the inputs' units differ. (Note this is adjacent to the real defect filed separately for IPv4._make_data, which is a missing conversion.)
• pypcap.py / pcap_ct.py raise UnsupportedCall throughout — no dissection capability, nothing to extract.
• HIP's length=schema.len * 8 + 8 (hip.py:545) — a single call site, since HIP has one top-level packet format. No extraction candidate.

Not covered

pcapkit/protocols/link/** and application/** were not swept for the same arithmetic patterns, and tcp.py, hip.py's parameter handlers and arp.py were not read method-by-method — targeted greps surfaced nothing there, which is weaker than having checked.