refactor(protocols): move shared value pack/unpack helpers into the Protocol utilities

The owner asked, in session, for the request on #2595 and #2607 to be applied to shipped code. Paraphrased: value-type pack and unpack helpers belong in the base Protocol's utility methods, where every protocol can share them.

Scope: audit pcapkit/protocols/ on main for helpers that pack or unpack values and that more than one protocol could use, then migrate them into Protocol. That covers module-level functions and per-class methods, such as fixed-width and bit-width unsigned integers, lists of them, length-prefixed vectors, and bitfield packing. Protocol-specific encoders stay where they are, as do helpers that _read_unpack/_make_pack already cover.

Plan:

A read-only audit lists every candidate with its call sites and semantics, and proposes the shared Protocol API. It is in progress.
The migration builds on the helpers feat(application): parse TLS handshake extensions #2595 adds to Protocol (vector, uint list, length prefix, and a bit-width uint check shared with feat(link): dissect the IEEE 802.11 MAC header behind radiotap #2607), so the codebase ends up with one helper set.

Blocked after the audit: the migration starts when #2595 merges (gh pr view 2595 --json state -q .state prints MERGED).

Audit of shipped code on main (3ccdc3271) for value pack/unpack helpers that belong on the base protocol. Four Sonnet auditors each covered one layer, and a fifth agent verified and deduplicated their findings. Every "move" and a sample of the "covered" verdicts were re-checked against main.

Proposed additions to ProtocolBase:

Already in feat(application): parse TLS handshake extensions #2595's fix round: the vector, whole-vector and uint-list readers, _make_uint, and the vector and uint-list makers.
Backed by evidence:
- _read_octets takes over TLS's _take.
- _read_uint replaces the bytes-buffer big-endian reads in ssl/tls.
- The MAC/EUI-64 pair replaces four copies in Ethernet, PCAPNG (MAC and EUI) and ARP's Ethernet branch. feat(link): dissect the IEEE 802.11 MAC header behind radiotap #2607 adds a fifth.
- _read_text merges the identical FTP._decode_args and HTTPv1._decode_text.
Optional: _uint_size, which counts the octets an unsigned int needs and is used by HIP, MH and HOPOPT.
Alignment helper: a pad_length helper replaces arithmetic spelled five ways in PCAPNG, HIP, IPv4, the IPv6 options and MH. It is a plain function rather than a method, because schema callbacks cannot call a protocol method. A PaddingField(align=…) option would replace most of the schema lambdas.

Plan. After #2595 merges, the migration runs one commit group per module family. Every group is byte-identical on the regenerated samples and has tests that fail before the change. Groups that share a file land one after another, and the ones that overlap #2607 or #2574 land after those. The open questions are in the next comment. Two findings are split out: the ContextVar question goes to its own issue, and a Quick-Start codec defect gets a bug issue.

Needs deciding before the migration (it starts only after #2595 merges):

Where pad_length lives. (a) pcapkit.corekit.fields.strings, beside PaddingField; (b) a new pcapkit.utilities module. Recommend (a): padding is part of what a schema is built from.
_make_uint turns silent wraps into errors.
- Measured on main: TCP window=70000 packs 0x1170, seq_no=-1 packs 0xffffffff, and UDP total_length=70000 packs 0x1170.
- The same applies to bool in RPL's reserved field and to negative values in MH and MPL.
- Options: (a) raise ProtocolError; (b) keep the wraps.
- Recommend (a): a wrapped value builds a different packet from the one asked for. Signed and sentinel PCAP-NG fields, such as section_length=-1, keep masking and get a field-by-field review.
The member-order page's worked example. It uses Ethernet's MAC pair, which moves to the base class. Recommend: the MAC commit picks a replacement class that still carries a full reader/maker pair, and the PR shows it for your review.
_make_octets. Adopting it means re-pinning a dozen whole-string error messages from fix(protocols): makers reject bad arguments with the reader's 'invalid format' text #2296. Recommend skipping it.
_read_sint/_make_sint. MH is the only user. Recommend skipping them and keeping MH's own code.

(a)
(a)
take your recommendations
if it's used, i'd stil take it. but if not used, then skip it.
no, move to Protocol still.

The owner's answers, paraphrased:

pad_length goes in corekit.fields.strings.
_make_uint raises on out-of-range values instead of wrapping them. Signed and sentinel PCAP-NG fields keep masking and get a per-field review.
The MAC commit picks the member-order page's new worked example, and its PR shows the choice for review.
_make_octets is taken if it has call sites and skipped if not. The audit found it replacing about a dozen fix(protocols): makers reject bad arguments with the reader's 'invalid format' text #2296 messages, so it is taken, and those messages are re-pinned.
_read_sint/_make_sint move to the base class, even though MH is their only user.

Every question is answered. The padding helper (pad_length, with PaddingField(align=…) adopted across the PCAP-NG, SCTP and HIP schemas) does not depend on #2595, so a worker is on it now. The other commit groups use #2595's helpers and are dispatched once it merges.

refactor(corekit): add pad_length and PaddingField(align=, after=) (#…

refactor(protocols): finish the shared value helpers and move TLS ont…

A check of main (7dc47f062) against the plan here and the owner's answers shows the merged PRs (#2624, #2629, #2630, #2638) finished part of #2614, not all of it.

Open items:

_make_uint in the remaining makers. On main, TCP(window=70000) packs 0x1170 and TCP(seq_no=-1) packs 0xffffffff. The RPL reserved check in hopopt stays inline, and MH, MPL and UDP were not migrated.
PCAP-NG's signed and sentinel fields (section_length, tzone, tsoffset) have not had their per-field review.
pad_length in the protocol classes. The hand-written padding in PCAP-NG, HIP, SCTP, IPv4, HOPOPT, IPv6-Opts, IPv6-Route, MH, TCP, DCCP and the two capture engines remains, which refactor(corekit): add pad_length and PaddingField(align=, after=) #2624 left for later.
_read_text has no caller: FTP and HTTPv1 keep their own copies.
_read_sint / _make_sint have no caller: MH keeps _decode_signed.
_uint_size has no caller: HIP, HOPOPT, IPv6-Opts, MH and DCCP keep their own bit_length code.
MAC/EUI-64: ARP's Ethernet branch and PCAP-NG's EUI pair keep their own code.

The work proceeds in three groups by module family: transport, internet, and PCAP-NG with ARP. Item 4 waits for #2628, which owns httpv1.py. Raising instead of wrapping in TCP and UDP changes the behaviour of released makers, so those PRs are labelled breaking.

Needs deciding: how section_length handles values outside its field. It is PCAP-NG's signed Section Header Block field, where -1 means "not specified". Following your answer 2, it keeps masking. Measured: 2**64 writes 0, True writes 1, and 1.5 raises a bare TypeError. So beyond the -1 sentinel, values outside the field are rewritten silently.

Keep masking as is.
Keep -1 as the sentinel, accept -2**63 to 2**64 - 1, and refuse anything else, including a bool, with ProtocolError, per fix(internet): the Quick-Start rate codec raises in IPv4 and clamps in TCP #2617.

Recommendation: 2. It keeps the sentinel, and -1 and 0xFFFFFFFFFFFFFFFF still read back alike. The only values it stops are ones the field cannot carry.

Also recorded, and decided on the branch: the obsolete Packet Block's drop_count moves to _make_uint. Its sentinel 0xFFFF is in range and has no signed spelling in the spec, which puts it with snaplen rather than section_length. So drop_count=-1, which used to write 0xFFFF, now raises. The PCAP-NG group's PR names this as breaking.

take 2

The owner chose 2, paraphrased. section_length keeps -1 as its "not specified" sentinel and accepts the range from -2**63 to 2**64 - 1, with -1 and 0xFFFFFFFFFFFFFFFF reading back alike. Anything outside that range, and any bool, raises ProtocolError, per #2617. Masking survives only where it maps the signed spelling onto the unsigned one.

This settles the question, so needs: decision comes off. It goes into the PCAP-NG group's commit, with a test for each refused case and for each edge of the range. The drop_count decision recorded above stands.

The transport group is #2670 and the PCAP-NG/ARP group is #2671. The internet group follows.

refactor(protocols)!: range-check the transport makers and adopt the …

refactor(protocols)!: finish the PCAP-NG and ARP value helpers (#2614) (

The PR is #2677. It is the last group; #2669 and #2674 unblock when it merges.

refactor(protocols)!: range-check the IPv6 extension, MH, HIP and IPv…

Navigation Menu

Repository navigation

refactor(protocols): move shared value pack/unpack helpers into the Protocol utilities #2614

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

refactor(protocols): move shared value pack/unpack helpers into the Protocol utilities #2614

Description

Activity

JarryShaw commented on Nov 2, 2026

JarryShaw commented on Nov 2, 2026

JarryShaw commented on Nov 2, 2026

JarryShaw commented on Nov 2, 2026

JarryShaw commented on Nov 2, 2026

JarryShaw commented on Nov 2, 2026

JarryShaw commented on Nov 2, 2026

JarryShaw commented on Nov 2, 2026

JarryShaw commented on Nov 2, 2026

JarryShaw commented on Nov 2, 2026

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions