Skip to content

fix(schema): a header cut inside a fixed-width field rebuilds as captured (#1458, #1451) - #1465

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1458-corekit-short-read
Oct 9, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1458-corekit-short-read

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 9, 2026 •

Copy link
Copy Markdown
Owner

Closes #1458
Closes #1451

When a capture ends inside a field, Schema.unpack now records that field and how many octets it read, as __short_read__. Schema.pack writes back only those octets. Schema.to_dict and Schema.from_dict carry the record, as they already do __remainder__ (#1380). The Link, Internet, Transport and Application bases copy the record into info after a parse, and from_data cuts the rebuild back to it.

The record is an ordinary info key, present only on a truncated header. Info.to_dict therefore carries it, and from_data(info) and from_data(info.to_dict()) both rebuild the capture exactly. Decoded values are unchanged, and no exceptions change, so there is no !.

Dumps: all 69 dumps of the 23 captures (JSON, plist and tree) are byte-identical. A dump of a truncated header gains one entry under that layer, __short_read__: tuple ('data', 19).

Edge tables: the #1454 tables drop their #1451 and #1458 rows. Five SCTP rows for padding of the last chunk remain, now filed under #1474. The two harness defect strings they used are removed.

Sweep: the #1454 edge cases cut at every offset take 45,756 runs, and failures fall from 14,012 to 22. The 22 left are the SCTP last-chunk padding (#1455) and the HOPOPT extension-mode trim (#1446).

tests/protocols/test_short_read_roundtrip_unit.py has 8 tests and 1340 subtests. On main it fails 821. On this PR's previous head, its to_dict assertions fail 812.

@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 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 89.59% (unit tier, Python 3.14, 540c8887f, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18795 955 2356 868 91.02%
pcapkit/corekit 2043 89 636 25 94.77%
pcapkit/dumpkit 242 2 88 2 98.79%
pcapkit/foundation 2648 98 930 48 94.80%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 16965 221 4584 186 98.07%
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

Copy link
Copy Markdown
Owner Author

Verdict: NEEDS CHANGES at e48586e11 (Sonnet cross-review; the author ran on Opus).

Blocking: the dict-form rebuild of a cut header still writes the field at full width. Data.__excluded__ (pcapkit/protocols/data/data.py:19) hides __short_read__ from Info.to_dict(), so replay_short_read never sees it. I reproduced this on this head:

e = Ethernet(bytes(12) + b'\x08', 13)
len(Ethernet.from_data(e.info).data)            # 13
len(Ethernet.from_data(e.info.to_dict()).data)  # 14

The reviewer saw this in 615 of 3894 cut runs. #1447/#1452 made the dict form a first-class round trip, so this needs fixing, with a test. The PR body's claim that "to_dict/from_dict carry the record" holds for Schema.to_dict, not Info.to_dict.

Holds

Found outside this PR's scope, and filed:

@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 9, 2026
@JarryShaw
JarryShaw force-pushed the fix/1458-corekit-short-read branch from e48586e to 857e497 Compare October 9, 2026 04:17
@JarryShaw JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 9, 2026
…ured (#1458, #1451)

When the data ended inside a field, Schema.unpack read the missing octets
as zeros and recorded nothing, so from_data(info) wrote that field, and
every field after it, at full width: Ethernet(bytes(12) + b'\x08')
rebuilt as 14 octets, and a Routing header with 23 of 24 octets as 24.

- Schema.unpack records the first field it reads short, and how many
  octets it got, as __short_read__; Schema.pack writes back only those,
  and Schema.to_dict/from_dict carry the record like __remainder__.
- keep_short_read/replay_short_read carry the record through info: the
  Link, Internet, Transport and Application bases add it after a parse
  and cut a from_data rebuild back to it. It is an ordinary info key, so
  Info.to_dict carries it and from_data(info.to_dict()) replays it too;
  only a truncated header has it.
- The #1454 edge tables drop the #1451/#1458 rows this closes, and the
  harness its two now-unused defect strings; five SCTP last-chunk pad
  rows remain, under #1474.

Decoded values are unchanged: a truncated number still reads as
zero-filled.
@JarryShaw
JarryShaw force-pushed the fix/1458-corekit-short-read branch from 857e497 to 540c888 Compare October 9, 2026 04:20
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 9, 2026
JarryShaw added a commit that referenced this pull request Oct 9, 2026
…tured (#1470)

- PCAP Header calls #1465's keep_short_read/replay_short_read, so a header
  cut short rebuilds from its info as the octets captured, not all 24.
- A PCAP-NG block the capture ends inside keeps its octets as
  truncated_raw; a rebuild parses them again instead of writing the block
  at its declared length (which wrote zeros, or raised ProtocolError).
- A cut block whose own parser refuses the zero-filled fields is kept as
  an UnknownBlock rather than raising.
- TLS and WireGuard key logs skip a last line the capture cut, and raise
  FieldValueError for any other malformed line (was a bare ValueError).

Adds tests/protocols/misc/test_misc_short_read_1470_runtime.py.
@JarryShaw

Copy link
Copy Markdown
Owner Author

Verdict: GOOD TO GO at 540c8887f. Round-2 cross-review on Sonnet; the author ran on Opus.

One point for the changelog: truncated headers now expose __short_read__ in info, iteration and dumps (e.g. ["icv", 8]). It is the first dunder bookkeeping key in a protocol info.

@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 9, 2026
JarryShaw added a commit that referenced this pull request Oct 9, 2026
…tured (#1470)

- PCAP Header calls #1465's keep_short_read/replay_short_read, so a header
  cut short rebuilds from its info as the octets captured, not all 24.
- A PCAP-NG block the capture ends inside keeps its octets as
  __truncated_raw__; a rebuild parses them again instead of writing the block
  at its declared length (which wrote zeros, or raised ProtocolError).
- A cut block whose own parser refuses the zero-filled fields is kept as
  an UnknownBlock rather than raising.
- TLS and WireGuard key logs skip a last line the capture cut, and raise
  FieldValueError for any other malformed line (was a bare ValueError).

Adds tests/protocols/misc/test_misc_short_read_1470_runtime.py.
@JarryShaw
JarryShaw merged commit 15d5eec into main Oct 9, 2026
40 checks passed
@JarryShaw
JarryShaw deleted the fix/1458-corekit-short-read branch October 9, 2026 13:13
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 9, 2026
JarryShaw added a commit that referenced this pull request Oct 9, 2026
…tured (#1470)

- PCAP Header calls #1465's keep_short_read/replay_short_read, so a header
  cut short rebuilds from its info as the octets captured, not all 24.
- A PCAP-NG block the capture ends inside keeps its octets as
  __truncated_raw__; a rebuild parses them again instead of writing the block
  at its declared length (which wrote zeros, or raised ProtocolError).
- A cut block whose own parser refuses the zero-filled fields is kept as
  an UnknownBlock rather than raising.
- TLS and WireGuard key logs skip a last line the capture cut, and raise
  FieldValueError for any other malformed line (was a bare ValueError).

Adds tests/protocols/misc/test_misc_short_read_1470_runtime.py.
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

1 participant