Skip to content

fix(schema): a SchemaField's unread octets are lost through from_data, so a direct SchemaField rebuilds zero-filled - #1397

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1380-schemafield-from-data-remainder
Oct 8, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1380-schemafield-from-data-remainder

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 8, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests

  • Followed the coding style (make pylint, make mypy, make isort) — only pylint E/W and mypy run on the touched files; no new findings

  • make test passes, and a test case covers the change — modules listed below

  • Added a changelog entry — N/A, added centrally

  • fix — corrects a defect


Closes #1380

Cause. Schema.pack re-emitted a nested schema's unread octets from __buffer__ (schema.py:876-878 on main). A schema rebuilt through from_dict or from_data was never unpacked, so its buffer was empty and the span was zero-filled.

Fix. Schema.unpack now stores the unread octets on the nested schema as __remainder__, plus the width it read as __remainder_offset__. Both are builtin names, so neither is a field or an item. to_dict() emits both as keys of the nested mapping, and from_dict() accepts them. pack re-emits the remainder only while the nested schema still packs to that width. A nested schema that shrank or grew is zero-filled, as on main. Probe: Out2.from_dict(o.to_dict()).pack() was 0302000004 and is now 0302eeee04.

Not breaking for shipped protocols: none of the 446 shipped schema classes declares a SchemaField directly or behind a ConditionalField, so their to_dict() never carries the key. User schemas: to_dict() gains inner['__remainder__'] and inner['__remainder_offset__']. A parsed nested schema that is mutated but keeps its width now keeps the unread octets, where it used to zero-fill them.

Tests: the new tests/protocols/schema/test_schema_field_remainder_unit.py has 11 cases. They include Protocol.from_data, and an in-place and a from_dict shrink, each of which gives 01aa0000000004. test_schema_field_keeps_declared_width now expects 0302eeee04. With main's schema.py, 5 of the 11 new cases fail, plus the updated case. The two shrink cases fail on the round-1 head cdda5807e. All 32 corekit modules and all 5 schema modules pass, as do test_protocol_make_unit (12), test_make_raw_item_read_unit (2) and tests/project (391 passed, 1 skipped). The protocols --exclude protocols/internet leg ran 993 tests, OK.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 8, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on cdda5807e: NEEDS CHANGES. One item (Sonnet reviewed, author Opus; I reproduced the regression myself).

  • The regression: a stale remainder is re-emitted at a shifted offset after an edit. Parse 02aabbccddee04 (Inner{k, body[k]} in a 6-octet SchemaField), then shrink the body to 1 octet in place. The PR packs 01aaccddee0004, while main packs 01aa0000000004. The old tail now sits in the wrong place, which is a regression against fix(schema): Schema.pack keeps option padding, SchemaField spans and iterated keys (#1289, #1291, #1292) #1374's buffer guard. Suggested fix: re-emit __remainder__ only when the nested schema packs to the same length it was parsed at, and zero-fill otherwise. Add the shrink case as a test.

Everything else holds:

  • __remainder__ is not a field: repr, dict and KeyError all behave as before, and copy, deepcopy and pickle keep it.
  • Two nested levels round-trip exactly.
  • The boundary where the remainder stops fitting works.
  • The info.to_dict() hashes of all 22 fixtures (1,588 frames) are byte-identical to main, with no __remainder__ keys, so no shipped protocol is affected.
  • The revert proof gives 4 of 7 failing, plus the updated pin. tests/protocols/schema passes 51.

Nit: unpack now calls len(self.__bytes__()), which packs the nested schema on every direct SchemaField unpack.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 8, 2026
…, so a direct SchemaField rebuilds zero-filled

Schema.unpack now records the octets of a direct SchemaField's span that
the nested schema left unread on the nested schema as __remainder__,
with the width it read as __remainder_offset__. to_dict() emits both as
keys of the nested mapping, from_dict() accepts them back, and
Schema.pack re-emits the remainder wherever the nested value goes, as
long as the nested schema still packs to that width; a nested schema
that shrank or grew is zero-filled as before. from_dict(to_dict()) and
Protocol.from_data now rebuild byte-exactly. The keys are absent when
nothing was left unread.

Closes #1380
@JarryShaw
JarryShaw force-pushed the fix/1380-schemafield-from-data-remainder branch from cdda580 to 7a1e11b Compare October 8, 2026 01:44
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 8, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Round-2 verdict on 7a1e11b5b: GOOD TO GO (Sonnet reviewed; author Opus). The round-1 regression is fixed. I re-ran the in-place shrink myself: 01aa0000000004, the same as main.

  • The width guard is exact. pack re-emits __remainder__ only when the nested schema packs to the __remainder_offset__ it was parsed at. Every from_dict case behaves:
    • shrink to k=1, grow to k=3 and grow to k=4 all zero-fill, as main does;
    • unchanged k=2 round-trips exactly (02aabbccddee04);
    • growing to fill the span (k=5) also round-trips exactly (05aabbccddee04).
  • A same-width edit keeps the unread tail (for example body=1122 gives 021122ccddee04). Those octets were never read by the nested schema, so keeping them is correct.
  • Hostile offsets zero-fill and never crash. Tried: -1, 99, 2**70, 'x', None, 1.5, True, [1].
  • Tests. The remainder and round-trip modules pass 17/0. Reverting to cdda5807e fails the two new shrink tests and the to_dict pin.

Nit: a non-bytes __remainder__ raises a raw TypeError in pack. That path is unchanged from round 1.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Coverage: 89.40% (unit tier, Python 3.14, 7a1e11b5b, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18791 968 2352 869 90.90%
pcapkit/corekit 2018 88 628 24 94.78%
pcapkit/dumpkit 234 2 84 2 98.74%
pcapkit/foundation 2573 99 910 44 94.63%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 16279 204 4312 166 98.16%
pcapkit/toolkit 539 74 168 3 84.58%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2342 1006 157 43.25%

Per-file detail: the coverage-html artifact of this run.

@JarryShaw
JarryShaw merged commit 79949ec into main Oct 8, 2026
40 checks passed
@JarryShaw
JarryShaw deleted the fix/1380-schemafield-from-data-remainder branch October 8, 2026 02:37
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 8, 2026
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(schema): a SchemaField's unread octets are lost through from_data, so a direct SchemaField rebuilds zero-filled

1 participant