Repository navigation
refactor(protocols): move shared value pack/unpack helpers into the Protocol utilities #2614
Description
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
ProtocolAPI. 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).
Activity
JarryShaw commentedon Nov 2, 2026
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 againstmain.Proposed additions to
ProtocolBase:_make_uint, and the vector and uint-list makers._read_octetstakes over TLS's_take._read_uintreplaces the bytes-buffer big-endian reads inssl/tls._read_textmerges the identicalFTP._decode_argsandHTTPv1._decode_text._uint_size, which counts the octets an unsigned int needs and is used by HIP, MH and HOPOPT.pad_lengthhelper 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. APaddingField(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.
JarryShaw commentedon Nov 2, 2026
Needs deciding before the migration (it starts only after #2595 merges):
pad_lengthlives. (a)pcapkit.corekit.fields.strings, besidePaddingField; (b) a newpcapkit.utilitiesmodule. Recommend (a): padding is part of what a schema is built from._make_uintturns silent wraps into errors.main: TCPwindow=70000packs0x1170,seq_no=-1packs0xffffffff, and UDPtotal_length=70000packs0x1170.boolin RPL's reserved field and to negative values in MH and MPL.ProtocolError; (b) keep the wraps.section_length=-1, keep masking and get a field-by-field 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.JarryShaw commentedon Nov 2, 2026
JarryShaw commentedon Nov 2, 2026
The owner's answers, paraphrased:
pad_lengthgoes incorekit.fields.strings._make_uintraises on out-of-range values instead of wrapping them. Signed and sentinel PCAP-NG fields keep masking and get a per-field review._make_octetsis 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_sintmove to the base class, even though MH is their only user.Every question is answered. The padding helper (
pad_length, withPaddingField(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…
4 remaining items
JarryShaw commentedon Nov 2, 2026
take 2
JarryShaw commentedon Nov 2, 2026
The owner chose 2, paraphrased.
section_lengthkeeps-1as its "not specified" sentinel and accepts the range from-2**63to2**64 - 1, with-1and0xFFFFFFFFFFFFFFFFreading back alike. Anything outside that range, and anybool, raisesProtocolError, per #2617. Masking survives only where it maps the signed spelling onto the unsigned one.This settles the question, so
needs: decisioncomes off. It goes into the PCAP-NG group's commit, with a test for each refused case and for each edge of the range. Thedrop_countdecision recorded above stands.JarryShaw commentedon Nov 2, 2026
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) (
JarryShaw commentedon Nov 2, 2026
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…
refactor(protocols)!: group each protocol into one package (#2534)
refactor(protocols)!: group each protocol into one package (#2534)
refactor(protocols)!: group each protocol into one package (#2534)
refactor(protocols)!: group each protocol's protocol, schema and data…