- main merged #437 (MH registry completion, including the four
mh-extension codes and the _make_ext_multiprefix arithmetic fix) and
#456/#446 (the ForwardMatchField double-count in Schema.__len__)
since this branch's last merge. Combined with this PR's own fix,
all four mh-extension/{Multi_Prefix,Exp_FFFD,Exp_FFFE,Exp_FFFF} cases
now round-trip cleanly -- verified directly against the round-trip
harness (all four return 'OK'), not assumed from the PR descriptions.
Deleted their EXPECTED_FAILURES entries; a stale PARSE/KeyError
expectation would otherwise have failed this module outright, per its
own two-way assertion.
- The issue's own 40-octet CGA Parameters reproduction now parses
completely end to end (confirmed directly: MH(raw, len(raw),
extension=True) returns a populated CGAParametersOption, no
exception). Rewrote test_cga_parameters_option_reaches_the_446_boundary
_not_a_keyerror, which asserted the (now stale) FieldValueError
boundary, as test_cga_parameters_option_now_parses_end_to_end,
asserting the parsed fields directly.
- #437 had pinned the pre-fix KeyError as
test_mh_cga_parameters_option_is_unparsable_upstream, explicitly so
that "whoever fixes it finds out here" -- and it did: this run turned
that test red once the merge above landed. Replaced it with
test_mh_cga_parameters_option_now_parses, asserting the option parses
and its fields are what the wire says, and fixed the now-stale
cross-reference and claim in
test_mh_pmipv6_options_round_trip_byte_for_byte's docstring (CGA_Parameters
is still excluded from that test's cases, but no longer because it
cannot be parsed -- that is now a separate, deliberate scope decision
for whoever adds its full round-trip identity).
- Merged origin/main (0283a6d) with one conflict, in this exact
region of tests/protocols/test_option_roundtrip_unit.py, resolved by
re-deriving the correct entries from the actual post-merge behaviour
rather than picking either side.
Verified: mypy pcapkit -> 123 errors/40 files (a fresh main, 0283a6d,
is 124 -- unchanged from before this merge). Round-trip harness: 7
passed, 363 subtests passed, 0 failed (up from 299 subtests before
#437 grew the mh-extension family to four codes). tests/protocols/
internet/test_mh_unit.py: 35 passed, 266 subtests passed, 0 failed.
Full local suite result to follow in the PR description.
Closes #446
The defect and mechanism
ForwardMatchFieldpeeks ahead without consuming:Schema.unpack()readsfield.lengthoctets, unpacks them into the field's value, then rewinds thestream past them (
data.seek(-length, io.SEEK_CUR),pcapkit/protocols/schema/schema.py:759onmain) and skips thepacket['__length__'] -= lengthdecrement that every other field gets. But itleft the raw octets it had just read sitting in
self.__buffer__[field.name](set unconditionally two lines earlier, atschema.py:748).Schema.__bytes__()/__len__()(
schema.py:437-443) concatenate every slot in__buffer__, with noexception for a forward match, so a schema containing one over-reports its
own length by exactly the match's width — the same octets are counted twice:
once in the forward-matched slot, once more where the field that actually
needs them reads them for real.
Where a declared area is then checked against that length —
ListField.unpack(pcapkit/corekit/fields/collections.py:187-189for theschema-item branch,
:191-193for the fixed-width branch, bothlength -= len(data)thenif length < 0: raise FieldValueError(...)) —the over-count makes correct input fail. That is exactly what blocks
CGAParametersOption.parameters(pcapkit/protocols/schema/internet/mh.py:533),a
ListFieldofCGAParameteritems: eachCGAParameter(
mh.py:499-521) carries a load-bearingForwardMatchField,public_key_test(mh.py:509), that has to preview the public key's lengthbefore
public_keycan be sized, so it cannot simply be deleted the way#441's stray HOPOPT/IPv6-Opts twin can. (
OptionField.unpack,collections.py:399-451, is the other declared-area check the issue names,but it never raises an "invalid length" error itself — it runs the option
list to length exhaustion instead — so the
FieldValueError: Field parameters has invalid length.the issue quotes namesparameters, the enclosingListField, notCGAParameter.extensions's ownOptionField.)Design chosen, and the evidence for it
The issue lays out two options: exclude forward-match slots from
__bytes__/__len__(less disruptive, but changes whatbytes(schema)round-trips), or add a separate consumed-octet measure and use that
everywhere a declared area is checked (more honest, touches every check).
I chose the first, implemented by having
unpack()zero aForwardMatchField's__buffer__slot right after the rewind — and thedecisive evidence is that
pack()already does exactly this, and has sincee1a406c8a(2023-04-06):and it is already asserted by an existing, passing test —
FeatureSchemaintests/protocols/schema/test_schema_unit.pyhas apeekForwardMatchField,and
test_schema_pack_unpack_and_mapping_methods(lines 69-70) alreadyexpects
peek's contribution to be absent from bothbytes(schema)andlen(schema), for a schema built from field values (which goes throughpack()).unpack()was the only path where this wasn't true — it storedthe real matched octets instead of
b'', for no reason connected to what thefield means. This fix makes
unpack()agree with the conventionpack()already established and already has test coverage for; it does not introduce
a new convention.
There's a second, load-bearing reason this is correct rather than merely
convenient: since a forward match doesn't advance the stream, the octets it
reads are always re-read by whichever field actually needs them (that's the
whole point of previewing). So the octets are never lost by dropping the
duplicate —
bytes(schema)after this fix reproduces exactly the octetsSchema.unpack()consumed from the wire, where before the fix it reproducedmore bytes than were consumed (the previewed region, twice).
The trade-off
bytes(schema)semantics do change for a schema obtained via.unpack()that contains a
ForwardMatchField: before this fix,bytes(schema)for sucha schema was longer than the octets actually consumed (the previewed region
counted twice); after, it equals the octets consumed. I'm treating this as a
bug fix rather than a behaviour change worth gating, because:
pack()vsunpack()) that werealready inconsistent with each other on
main, not introducing a newinconsistency;
expectation for the
pack()path;bytes(unpacked_schema)and expects the previewed region counted twice —see the census below.
Census:
len(schema)/bytes(schema)consumersgit grepagainstorigin/mainfor both, outsideschema.pyitself:pcapkit/corekit/fields/collections.py—ListField.unpackandOptionField.unpack, bothlength -= len(data)against a declared area(the two named in the issue).
pcapkit/protocols/internet/hip.py:757,2882,hopopt.py:482,1265,ipv6_opts.py:493,1277,transport/tcp.py:698—counter += len(schema)/opt_len = len(schema)while walking a sequence of options/parameters,same "how many octets did this account for" semantics.
pcapkit/protocols/protocol.py:474—self.__init__(bytes(schema), len(schema))inProtocol.from_schema(), reconstructing a protocolinstance's raw bytes from its schema. This one is a live consumer of the
exact inconsistency this PR fixes: depending on whether
Schema.unpack's__updated__flag got reset by a later field assignment (see the note atpcapkit/utilities/decorators.py:237-260, which documents that quirk foran unrelated reason),
bytes(schema)onmaincould take either thebuggy long form or the already-correct
pack()-repacked form for thesame schema object. After this fix both agree.
tests/protocols/misc/test_pcapng_unit.pyassertslen(schema) == len(raw)six times, andtest_pcapng_section_header_block_round_trips_ through_bytesround-trips a schema whosematchfield is aForwardMatchField(PCAP-NG's byte-order probe) — still green after thisfix (checked explicitly, see below).
tests/protocols/schema/test_schema_unit.py:69-71,280,283— thepack()path's existing round-trip assertions, unaffected (this fix touches
unpack()only).No consumer anywhere expects
bytes(schema)/len(schema)to include aforward match's previewed octets.
Census:
ForwardMatchFieldusersgit grep -n ForwardMatchFieldagainstorigin/main, outsidemisc.py/schema.py(9 declarations, 8 in shipped schemas + 1 in the test suite):hopopt.py:393andipv6_opts.py:393—_SMFDPDOption.test(length 3),legitimate, present symmetrically in both modules.
ipv6_opts.py:434— the stray secondtest(length 1) inside the nestedSMFIdentificationBasedDPDOption, ipv6_opts' SMFIdentificationBasedDPDOption has a stray test field HOPOPT lacks, so identical octets fail there #441/PR protocols: drop ipv6_opts' stray SMF_DPD test field, fix two length underflows #449's target.merged and deleted it, so
ipv6_opts.pynow carries 2 declarations, not 3.hopopt.py:521,ipv4.py:450,ipv6_opts.py:524—_QSOption/Quick-Startflags(length 3), one per protocol that carries the option.mh.py:509—CGAParameter.public_key_test(length 2), this issue'sload-bearing case.
misc/pcapng.py:541— PCAP-NG Section Header Block'smatch(length 8),the byte-order probe.
transport/tcp.py:576— Multipath TCP's generictest(length 3).tests/protocols/schema/test_schema_unit.py:36—FeatureSchema.peek.None are wrapped in a
ConditionalField, so theConditionalField-unwrapbranch already present in both
pack()andunpack()(field = field.field(packet)) was not a new case to handle — it already runs beforeeither function's
ForwardMatchFieldcheck.Test evidence
Two new cases in
tests/protocols/schema/test_schema_unit.py, both verifiedin both directions at
e2d8ed6d1(fails before, passes after):test_forward_match_field_does_not_count_toward_length— a two-fieldschema (
length_peekaForwardMatchField(UInt8Field),lengththe sameoctet read for real,
datasized from it) unpacked fromb'\x02AB'(3octets). Before the fix:
After:
len(unpacked) == 3andbytes(unpacked) == b'\x02AB'.test_schema_list_field_rejects_a_declared_area_that_a_forward_match_over_reports— a
ListField(length=4, item_type=SchemaField(length=2, schema=...))overtwo of the items above (
b'\x01A\x01B', exactly 4 octets, the correctdeclared area). Before the fix:
— the same exception class and message shape
CGAParameter.extensionshits(
FieldValueError: Field parameters has invalid length.). After the fix itunpacks two items cleanly.
EXPECTED_FAILURESchangeUpdate after merging
main: this PR originally deleted the'ipv6-opts-option/SMF_DPD'entry itself, because this fix, on its own(without #449's removal of the stray field), independently turns that case
from a recorded
PARSEfailure into'OK'— verified at the time:test_round_trip_is_identity_or_a_recorded_gapfailed withAssertionError: 'OK' != 'PARSE'until the entry was deleted.#449 has since merged to
main(73edb096f) and deleted the same entryfirst, for its own reason (removing the stray field), rewriting the
surrounding comment block into past tense to cover both the
#432/hopopthalf and the
#441/ipv6-opts half of the story. I mergedmaininto thisbranch (
git merge --no-ff) and resolved the one resulting conflict intests/protocols/test_option_roundtrip_unit.pyby takingmain's commentblock as-is and dropping my own version — there is nothing left for this PR
to delete, since #449 already got there. The file is now byte-identical to
main's copy; this PR's only remaining diff isschema.pyand the two newtests in
test_schema_unit.py.I re-ran the round-trip harness on the merged tree
(
bf95d1e07, fixtures regenerated fresh) to check for further fallout fromhaving #446's fix, #449's field removal, and #453's
ListFielditem-unpackfix all present together:
test_round_trip_is_identity_or_a_recorded_gapstill passes, and no other recorded case flipped.
mh-extension/Multi_Prefix(the case that exercises
CGAParameterthrough the public API) is stillunaffected: it fails exactly as recorded, with
KeyError: 'length', because#445's nested-
__packet__fault hits first, before this issue'sFieldValueErrorwould ever be reached.Suite counts
Earlier figures in this PR were wrong and are withdrawn. They were
measured with
python -m unittest discover, which collected 875(862 passed + 13 skipped) at
e2d8ed6d1, wherepytestcollects 977 at thatsame sha — 102 short, for a reason I did not chase down (
unittest discoverapparently misses some tests
pytestfinds; there is nothing pytest-nativein
tests/— no baredef test_*, no@pytest.mark.parametrize,no
@pytest.fixture— so the exact mechanism is still unexplained). Theyalso compared a baseline measured before fixtures were regenerated against
an after-fix run measured after regenerating them, so the two runs were not
even over the same fixture set. Both problems are fixed below by using
pytestthroughout and generating fixtures exactly once before either run.That
977(collected ate2d8ed6d1) and the994/977pair below(collected/passed at
da2422728) are unrelated measurements of differentquantities at different shas, and their sharing a digit is coincidence, not
confirmation of anything —
da2422728is four merged PRs ahead ofe2d8ed6d1, and those four PRs added 17 tests between them, which is theentire reason collection moved from 977 to 994. Read the two figures below as
standalone, sha-qualified measurements, not as corroborating the paragraph
above.
mainmoved four commits while this was in flight(
#449,#453,#451,#450); the figures below are against the mergedtree, with
mainatda2422728as the baseline and this branch's mergecommit
bf95d1e07as "after". Fixtures were regenerated once, frombf95d1e07, withexamples/generators/make_samples.py, before measuringeither side; the baseline measurement below reused those same fixture files
without touching them again.
Collected (
pytest tests --collect-only -q | grep -c '^tests/.*::'):da2422728(baseline): 994bf95d1e07(this branch, merged): 996 (+2, this PR's two new tests)Full run (
pytest tests -q):da2422728(baseline): 977 passed, 17 skipped (994 total)bf95d1e07(this branch, merged): 979 passed, 17 skipped (996 total)The delta is exactly the two new tests in
test_schema_unit.py, bothpassing; skipped count is unchanged.
test_option_roundtrip_unit.pyandtest_schema_unit.pytogether: 22/22 passed on the merged tree (seeabove).
Run with
PYTHONSAFEPATH=1,PYTHONPATHat the worktree root, interpreter.venv/bin/python3.14.7;pcapkit.__file__printed and confirmed toresolve inside the worktree before each run.
Coordination
mainsince this PR was opened;this branch is merged up to
da2422728(merge commitbf95d1e07), notrebased, per instruction not to force-push a branch with a real PR on it.
pcapkit/corekit/fields/misc.py. Issue A nested schema cannot reach the enclosing packet's fields by name, so CGA Parameters raises KeyError: 'length' #445 (PR corekit: let a nested schema's field callbacks reach the enclosing schema #457, open)owns that file and added
nested_packet_context()there; this fix livesentirely in
schema.py'sunpack()and does not intersect with it.EXPECTED_FAILURESconflict flagged earlier against protocols: drop ipv6_opts' stray SMF_DPD test field, fix two length underflows #449 has alreadyresolved itself by protocols: drop ipv6_opts' stray SMF_DPD test field, fix two length underflows #449 merging first — see above.
Two corrections to this description, from the review, both verified independently and neither affecting the code or tests.
The
pack()side has zeroed aForwardMatchField's buffer slot sincee1a406c8a(2023-04-06), not since schema_final's generated typed __init__ is dead code, so Schema.__post_init__ never runs #422.git blame -L 636,640 origin/main -- pcapkit/protocols/schema/schema.pyattributes those three lines toe1a406c8a7, and schema_final's generated typed __init__ is dead code, so Schema.__post_init__ never runs #422's own commits never touch that branch. The claim was wrong about when, not about whether — the precedent this fix mirrors is real and considerably older than stated, which if anything strengthens the "restore a long-standing invariant" reading.The
ForwardMatchFieldcensus was stale rather than miscounted: it was taken before PR protocols: drop ipv6_opts' stray SMF_DPD test field, fix two length underflows #449 merged, and protocols: drop ipv6_opts' stray SMF_DPD test field, fix two length underflows #449 deleted the strayipv6_opts.py:434declaration that ipv6_opts' SMFIdentificationBasedDPDOption has a stray test field HOPOPT lacks, so identical octets fail there #441 was about. Current per-module counts onmainare hopopt 2, ipv4 1, ipv6_opts 2, mh 1, pcapng 1, tcp 1 — 8 shipped declarations plus 1 in the test suite, not 9 + 1. The blast radius is one declaration smaller than described.The fix, the tests and the measurements are unaffected: 996 collected / 979 passed / 17 skipped at
bf95d1e07, mypy 124 errors in 40 files with or without the fix, andCGAParameter.public_key_test— the load-bearing case — still exists on currentmain(faf86d26b), so the fix is not obsoleted by #437'smh.pyrewrite.