protocols: drop ipv6_opts' stray SMF_DPD test field, fix two length underflows

[JarryShaw]

Summary

Two related schema defects in pcapkit/protocols/schema/internet/, both surfaced while investigating option/parameter length arithmetic in that directory.

#441 — stray test field makes len(schema) over-report by one

Defect. pcapkit/protocols/schema/internet/ipv6_opts.py's SMFIdentificationBasedDPDOption declared:

test: 'SMFDPDTestFlag' = ForwardMatchField(BitField(length=1, namespace={
    'mode': (0, 1),
}))

right before info. Its HOPOPT twin (pcapkit/protocols/schema/internet/hopopt.py) has no such field — the two classes are otherwise line-for-line equivalent.

Mechanism. ForwardMatchField consumes no bytes from the stream but still occupies a Schema.__buffer__ slot (pcapkit/protocols/schema/schema.py), so it counts toward len(schema). The nested schema therefore over-reported its length by one octet, and OptionField (pcapkit/corekit/fields/collections.py) accumulated that over-report against the declared option area until the threshold check in pcapkit/protocols/internet/ipv6_opts.py (_read_ipv6_opts) raised ProtocolError: IPv6-Opts: invalid format on octets that parse cleanly under HOPOPT.

Why it was a stray field, not a missing one — verified against RFC 6621 §6.1.1. The I-DPD option header layout is: octet 0 Option Type, octet 1 Opt Data Len, octet 2 bit 0 the H-bit/mode (0 = I-DPD), bits 1–3 TidTy, bits 4–7 TidLen, then the TaggerID and Identifier. There is no separate "test" octet in the wire format — the mode bit is part of the same octet as TidTy/TidLen, which SMFIdentificationBasedDPDOption.info (BitField(length=1, namespace={'mode': (0,1), 'type': (1,3), 'len': (4,4)})) already reads. Confirmed by usage: pkt['test'] is read only by smf_dpd_data_selector, which operates on the enclosing _SMFDPDOption's packet dict (both modules already declare test there, at length=3), not on SMFIdentificationBasedDPDOption's own fields — smf_i_dpd_id_len and post_process on this class never reference pkt['test']. So the field was dead weight left over from before #432, not a field HOPOPT is missing.

Fix. Removed the stray field from ipv6_opts.py. HOPOPT was already correct; ipv6_opts.py now matches it exactly.

Independent corroboration from #440. After this branch was up, #440 merged to main and added tests/protocols/test_option_roundtrip_unit.py, a harness that round-trips 258 option codes and records the ones that cannot close the cycle. It had an EXPECTED_FAILURES entry for exactly this defect ('ipv6-opts-option/SMF_DPD', recorded as a PARSE gap at ipv6_opts.py:434, discovered independently of this PR). Merging origin/main into this branch and running that file turns the entry's assertion red — AssertionError: 'OK' != 'PARSE' — because the round trip now closes. That is the harness telling us, from a direction neither this PR nor #440 anticipated together, that the #441 diagnosis and fix are correct. Removed the now-stale EXPECTED_FAILURES entry in this PR (see Tests below); left the surrounding historical comment in place, updated to past tense.

#438 — a wire-derived length underflows into a negative struct format

Defect. SMFIdentificationBasedDPDOption.id (both hopopt.py and ipv6_opts.py) sized itself as:

id: 'bytes' = BytesField(length=lambda pkt: pkt['len'] - (
    1 if pkt['info']['type'] == 0 else (pkt['info']['len'] + 2)
))

with nothing floored at zero.

Mechanism. A peer declaring an Opt Data Len too small for the TaggerID it claims (e.g. Opt Data Len = 0 for a null TaggerID, which needs at least 1) drives this negative. _TextField.__call__ (pcapkit/corekit/fields/strings.py) then sets self._template = f'{length}s', i.e. '-1s', and the base FieldBase.length property (pcapkit/corekit/fields/field.py) computes struct.calcsize('-1s'), raising a bare struct.error: bad char in struct format — not one of pcapkit's own exception types, and uncatchable through pcapkit.utilities.exceptions.

Fix. Replaced the lambda with a named function, smf_i_dpd_id_len, in each module (matching the existing mpl_opt_seed_id_len convention already in both files), which raises FieldValueError when the computed length would be negative instead of letting it reach struct. FieldValueError is the exception this file already raises for every other "computed field parameter is invalid" case (smf_i_dpd_tid_selector, smf_dpd_data_selector, mpl_opt_seed_id_len), so no new exception type was needed.

HIP siblings — same guard, needed. pcapkit/protocols/schema/internet/hip.py's RegRequestParameter, RegResponseParameter and RegFailedParameter all sized their registration-type list the same unfloored way: ListField(length=lambda pkt: pkt['len'] - 1, ...). These do need the guard, though the failure mode differs from the BytesField case: ListField.length (pcapkit/corekit/fields/collections.py) returns its raw int directly rather than going through struct.calcsize, and its unpack() loop is while length > 0, so a negative length never crashes — it silently returns an empty list. Verified end-to-end with a crafted Length = 0 REG_REQUEST parameter: pre-fix, this parses to reg_type=() with no exception and no diagnostic, exactly the "silently produce a wrong parse" failure the fix is meant to avoid, not a struct.error. Fixed with a shared helper, registration_type_list_len, reused by all three parameter schemas.

Checked, post-merge, that this change does not disturb any of #440's other HIP-related EXPECTED_FAILURES entries — hip-parameter/R1_Counter, ENCRYPTED, HIP_TRANSFORM, HOST_ID, and the sixteen-parameter RECONSTRUCT/tuple-vs-list group that includes REG_REQUEST/REG_RESPONSE/REG_FAILED themselves. The RECONSTRUCT group's gap is a different defect entirely (_read_param_* returning a tuple where _make_param_* needs a list) on the well-formed construct/parse/reconstruct path; this fix only changes behavior for a malformed Length that well-formed round-trip data never produces. Confirmed by running the full round-trip file after merging origin/main: 7 passed, 299 subtests, 0 failures — the one flip is exactly the SMF_DPD entry above, nothing else moved.

Two other siblings the issue named for the same check — RegInfoParameter.reg_info (pkt['len'] - 2) in hip.py, and the CALIPSOOption/MPLOption padding fields in hopopt.py/ipv6_opts.py (pkt['len'] - 8 - pkt['cmpt_len']*4, pkt['len'] - 2 - ...) — share the identical unfloored shape but are not touched by this PR; they were outside the lines the issue asked to check, and are being filed separately.

Tests

• tests/protocols/internet/test_ipv6_extension_unit.py: two new wire-level tests per protocol (HOPOPT/IPv6-Opts) — one reproducing ipv6_opts' SMFIdentificationBasedDPDOption has a stray test field HOPOPT lacks, so identical octets fail there #441's exact octets and asserting both protocols now produce the identical option layout, one reproducing A wire-derived length underflows into a struct format string, raising bare struct.error on untrusted input #438's exact octets and asserting FieldValueError instead of a crash. Extended the existing schema-helpers test with direct calls to smf_i_dpd_id_len covering both branches (null and non-null TaggerID) and the underflow guard. Updated a docstring and two test fixtures that had encoded the ipv6_opts' SMFIdentificationBasedDPDOption has a stray test field HOPOPT lacks, so identical octets fail there #441 field-count mismatch as expected behavior (an ident_kwargs['test'] = ... conditional that only applied to ipv6_opts, and an unconditional test=... kwarg shared by both) — both are now symmetric between the two protocols.
• tests/protocols/internet/test_hip_unit.py: one new wire-level test constructing a full HIP packet with a Length = 0 REG_REQUEST/REG_RESPONSE/REG_FAILED parameter, asserting FieldValueError. Extended the existing schema-selectors test with direct calls to registration_type_list_len.
• tests/protocols/test_option_roundtrip_unit.py (added by tests: round-trip 258 option codes from the registries, recording the 86 that cannot close the cycle #440, merged in): removed the now-fixed 'ipv6-opts-option/SMF_DPD' EXPECTED_FAILURES entry — the table's own design is that a fixed defect deletes its entry rather than leaving it behind, and the assertion message says exactly that. Kept the surrounding comment block, rewritten to past tense, as history alongside the existing note about corekit: stop the option and list loops spinning forever on a truncated area (#431) #432's half of the same story; did not comment the entry out.
• Every new/extended assertion (including the round-trip table's own reconstructed assertion) was verified to fail on the pre-fix schema code and pass after, via a targeted git stash push -- pcapkit/protocols/schema/internet/*.py (tests left in place, never a bare git stash).
• Docs: added autofunction entries for the two new helper functions in the corresponding .rst files, matching the existing entries for mpl_opt_seed_id_len etc.

Test plan

• mypy clean on the three modified schema modules
• Targeted runs of tests/protocols/internet/test_ipv6_extension_unit.py and tests/protocols/internet/test_hip_unit.py pass (55 tests, 125 subtests)
• tests/protocols/test_option_roundtrip_unit.py passes in full post-merge: 7 passed, 299 subtests, 0 failures
• Branch merged with origin/main (git merge --no-ff, no rebase, no force-push) — clean, zero conflicts
• Full suite before, on main at a4c8d62b1 (tests: round-trip 258 option codes from the registries, recording the 86 that cannot close the cycle #440's own merge commit; Python 3.14.7, examples/generators/make_samples.py run first): 926 passed, 17 skipped, 1268 subtests
• Full suite after, on this branch at its current tip (merge of a4c8d62b1 + this fix): 931 passed, 17 skipped, 1271 subtests — net +5 passed test methods, +3 subtests, 0 failures, no new skips
• pcapkit.__file__ verified against this worktree for both measurements

(Earlier figures of 916/921 passed against f50436a8a were the pre-#440 baseline and are superseded by the above, taken after merging origin/main.)

Closes #441
Closes #438

[JarryShaw]

the PR's note reads like the ForwardMatchField's logic or handling logic is defect. maybe worth double checking and fixing.

[JarryShaw]

You're right, and it is a defect in its own right — filed as #446, deliberately kept out of this PR. Two separate faults meet at this line, and I want to be clear which is which because only one of them is fixed here.

What this PR fixes is a real defect independent of ForwardMatchField. The test field should not exist at all. Its HOPOPT twin does not have it, and :rfc:6621 §6.1.1 puts the I-DPD mode bit in octet 2 alongside TidTy/TidLen — which the existing info BitField(length=1, namespace={'mode': (0,1), 'type': (1,3), 'len': (4,4)}) already reads. There is no separate wire octet for it, and pkt['test'] was read only by smf_dpd_data_selector against the enclosing _SMFDPDOption, never against this schema. So the two schemas disagreed about the same bytes, and HOPOPT was the correct one. That stands whatever happens to ForwardMatchField.

What you're pointing at is the reason the stray field had teeth, and it is the shared machinery. ForwardMatchField exists to look ahead without consuming, but the bytes it matched still occupy a __buffer__ slot, and Schema.__len__ is len(self.__bytes__()) (pcapkit/protocols/schema/schema.py:443). So any schema containing a forward match reports itself longer than the octets it actually read, by exactly the width of the match — and where that length is then checked against a declared area, correct input fails. Measured on the same octets 1100080100010100:

hopopt    __fields__ = [type, len, info, tid, id]        len(schema) = 3
ipv6_opts __fields__ = [type, len, test, info, tid, id]  len(schema) = 4

Why deleting the field cannot be the general fix. The CGA Parameters option is the case that proves it: pcapkit/protocols/schema/internet/mh.py's CGAParameter carries

public_key_test: 'ANSIKeyLengthTest' = ForwardMatchField(BitField(length=2, namespace={'len': (8, 8)}))

and that one is load-bearing — the public key's length genuinely has to be read before the key can be sized, so it cannot be deleted. With the other blocker in that path fixed (#445, a nested schema being unable to reach the enclosing packet's fields), the same option then fails at FieldValueError: Field parameters has invalid length., which is #446 and nothing else. I measured that chain: KeyError: 'length' first, then FieldValueError once the lookup is corrected.

So #446 is the root and it needs answering, but it is a design question rather than a one-line fix, which is why it is its own issue rather than folded in here. len(schema) is used as "how many octets did this schema account for", and a non-consuming field should arguably not contribute — but changing that also changes what bytes(schema) round-trips, and both OptionField and ListField depend on the answer. The alternative is to keep __len__ as the buffer length and give schemas a separate consumed-octet measure, then use that wherever a declared area is checked, which is more honest and touches every length check. #446 lays both out.

I've dispatched work on #446 now — the sequencing reason for holding it back was that this PR was already reasoning about ForwardMatchField length accounting and I did not want two changes to the same machinery developed in parallel. That reason has expired now this PR is done.

This PR stays as-is: it deletes a field that should never have been there, and #446 fixes why its presence broke anything. Happy to fold #446 in here instead if you'd rather see them land together — say so and I'll re-scope rather than open a second PR.

[JarryShaw]

Reviewing on behalf of Copilot (out of tokens). Reviewed commit d60916b85ee3ac2003511d5dd509e5808cb5fb76 (branch fix/smf-dpd-length-defects), fetched via refs/pull/449/head and confirmed identical to the stated head sha. All code was read with git show <ref>:<path> against that sha and origin/main, never the ambient working tree.

CI

Polled gh pr view 449 --json statusCheckRollup from mid-run (1 success, 19 pending/queued, 2 skipped at dispatch) through to completion. Final state: 24/24 checks resolved, zero failures — 22 SUCCESS (Analyze/CodeQL, all 6 Compat Python 3.1{0..5}, all 6 Python 3.1{0..5} unit-test legs, all 6 Integration Python 3.1{0..5} legs, deploy-pages, pyup.io/safety-ci), 2 SKIPPED as expected (Docs test gate, Gate (full suite, Python 3.14)). No red legs at all on this commit — the 7 red legs mentioned as historical noise belong to an earlier revision of this branch (the now-deleted EXPECTED_FAILURES entry) and are not reproduced here.

Branch position

git rev-list --count d60916b85..origin/main = 1 (e2d8ed6d1, "give IP reassembly the RFC timeout, and trace TCP flows bidirectionally"). git merge-base = a4c8d62b1, exactly the commit this PR's own merge and baseline measurement used. git show --stat e2d8ed6d1 touches only pcapkit/foundation/{reassembly,engines,extraction} and traceflow — zero overlap with this PR's files. Doesn't matter for this review or for merging.

#441 (stray test field) — verified against RFC 6621 and the code

Fetched RFC 6621 directly (§6.1.1, Figure 3, the I-DPD layout): octet 0 = Option Type, octet 1 = Opt Data Len, octet 2 bit 0 = the H-bit/mode, bits 1–3 = TidTy, bits 4–7 = TidLen, then TaggerId (if TidTy≠0) then Identifier. That is exactly what SMFIdentificationBasedDPDOption.info already reads (BitField(length=1, namespace={'mode': (0,1), 'type': (1,3), 'len': (4,4)})) — there is no separate wire octet for the removed test field to have matched. Confirmed the "only reader" claim by reading smf_dpd_data_selector directly: it reads pkt['test']['mode']/pkt['test']['len'] off the caller's pkt, which is _SMFDPDOption's own dict (that class already declares its own test: ForwardMatchField(BitField(length=3, ...)), unchanged by this PR) — the selector never sees SMFIdentificationBasedDPDOption's nested pkt. Diff confirmed: the stray field and nothing else was removed from ipv6_opts.py; hopopt.py's twin was already correct and is untouched. Diagnosis and fix are correct.

#438 (BytesField length underflow) — verified mechanism and fix

Read _TextField.__call__ (pcapkit/corekit/fields/strings.py) directly: new_self._template = f'{new_self._length}s', and FieldBase.length (field.py:109) calls struct.calcsize(self.template) — confirms a negative length reaches struct.calcsize('-Ns') exactly as claimed, for any _TextField subclass (BytesField, PaddingField, ...), not only the one fixed here. smf_i_dpd_id_len/registration_type_list_len match the file's existing mpl_opt_seed_id_len convention (protocol-prefixed FieldValueError message, same docstring shape). Read ListField.unpack (collections.py): its loop is while length > 0, and its length property returns the raw int with no calcsize involved — confirmed a negative/zero length for an item_type-based ListField silently returns [] rather than crashing, exactly the "fails differently" claim for the three HIP siblings. Diffs for both fixes are minimal and correct.

Deletion of 'ipv6-opts-option/SMF_DPD' from EXPECTED_FAILURES

Confirmed the entry is gone and the surrounding comment block is rewritten accurately to past tense, correctly citing "Fixed by #441/PR #449." No other entries touched.

Test amendments — not weakened

tests/protocols/internet/test_ipv6_extension_unit.py: the two amended existing tests dropped a test={'mode': ...} kwarg (one unconditional, one behind if schema.__name__.endswith('ipv6_opts')) that had encoded #441's asymmetry as expected behavior. Post-fix they construct SMFIdentificationBasedDPDOption identically for both protocols — this is a correction to match reality, not a coverage reduction; nothing that was asserted before is no longer asserted. New tests added (both files) reproduce #441 and #438's exact wire-level repros and assert FieldValueError / identical option layout.

Scope decision: RegInfoParameter.reg_info and CALIPSOOption/MPLOption padding — disclosed but still live, not filed

The PR body discloses, honestly, that these share "the identical unfloored shape" but are deliberately left unfixed, "being filed separately." I searched gh issue list for CALIPSO/MPLOption/RegInfoParameter/underflow and found nothing beyond #438 itself — no follow-up issue exists yet. I reproduced the CALIPSOOption.pad crash live against this commit:

from pcapkit.protocols.internet.hopopt import HOPOPT
HOPOPT(bytes.fromhex('3b0007000000000000000000000000'), 16)
# struct.error: bad char in struct format
#   pcapkit/corekit/fields/field.py:109, in length: return struct.calcsize(self.template)

Posted as inline comments on hopopt.py:144, ipv6_opts.py:144, and hip.py:166 (anchored at the nearest diff-hunk lines, since GitHub's review-comment API rejects lines outside a hunk and these sites are untouched by design). Detail worth a small correction: the PR body says "the issue named" RegInfoParameter.reg_info alongside the padding fields, but #438 only names the padding fields by line (hopopt.py:361/:614, shifted to :392/:653 by later commits) — RegInfoParameter looks like the PR authors' own additional find, not something #438 asked for. Doesn't change the substance. Not a blocker: nothing here is introduced or worsened by this PR, and it's the honest, disclosed kind of scope-narrowing rather than a hidden one — but the "filed separately" needs to actually happen.

Measurements — all independently reproduced, not taken on trust

Interpreter /local/home/jarryx/GitHub/PyPCAPKit/.venv/bin/python (3.14.7), PYTHONSAFEPATH=1, PYTHONPATH pointed at a fresh clone per tree, pcapkit.__file__ asserted against that clone before every measurement, examples/generators/make_samples.py run first in each clone.

Measurement	Claimed	Reproduced
mypy on the 3 modified schema modules	clean	clean ("Success: no issues found in 3 source files")
tests/protocols/test_option_roundtrip_unit.py	7 passed / 299 subtests / 0 failures	7 passed, 299 subtests passed
test_ipv6_extension_unit.py + test_hip_unit.py	55 tests / 125 subtests	55 passed, 125 subtests passed
Full suite, baseline a4c8d62b1	926 passed / 17 skipped / 1268 subtests	926 passed, 17 skipped, 1268 subtests passed (own clone, own run, 586.91s/564.43s)
Full suite, PR tip d60916b85	931 passed / 17 skipped / 1271 subtests	931 passed, 17 skipped, 1271 subtests passed (own clone, own run, 635.19s/594.66s)

Every claimed number matches exactly, including the +5 passed / +3 subtests delta.

Verdict

GOOD TO MERGE at d60916b85. #441 and #438 are both correctly diagnosed (verified independently against RFC 6621 and the actual field/selector code, not just re-asserted), correctly and minimally fixed, covered by new wire-level tests that reproduce the original repros, and every quantitative claim in the PR description reproduces exactly on independent measurement. CI is fully green with no red legs on this commit. The one real gap — CALIPSOOption/MPLOption padding and RegInfoParameter.reg_info sharing the same unguarded-underflow shape, one of them (CALIPSO/MPL) still crashing with a bare struct.error today — is honestly disclosed in the PR body rather than hidden, is not introduced or worsened by this change, and is flagged above and inline so the promised follow-up issue actually gets filed.