Skip to content

fix(protocols): report a rebuilt frame's messages once and record cuts on the layer (#2711) - #2738

Merged
JarryShaw merged 1 commit into
mainfrom
fix/2711-rebuild-noise-cut-record
Nov 3, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/2711-rebuild-noise-cut-record

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner

What is the purpose of your pull request?

  • fix — corrects a defect

Three rebuild and cut-recording gaps found while reviewing #2703.

  • An outer rebuild repeated its payload's messages. While a payload is rebuilt inside _make_payload, its warnings are held, and beholder absorbs errors quietly. The outer layer's own parse still reports each message once. The owner ruled for this on fix(protocols): rebuild noise and cut-recording gaps found reviewing #2703 #2711, and the "Handled Refusals" page records the exception.
  • A directly built IGMPv3 Report cut on a field boundary read INVALID. The record count now sizes the records in a standalone parse, so cuts at 8, 20 and 24 read TRUNCATED, as they do behind IPv4. A count past IP_MAX_LENGTH raises ProtocolError. Where the zero-fill budget would be exceeded, the parse reads as on main, so it never newly raises.
  • A layer records a cut when its captured octets fall short of the length it declares (owner, option 1). Schema.unpack notes a cut in a declared payload. A frame cut is recorded on a layer only through Schema._frame_end_field, whose last field is sized by the length the layer was given. That covers GRE, ICMP, IGMP, Ethernet and TSO IPv4. A layer that declares its own extent records nothing past it, and neither does one that ends in a list. Those are IPv4, IPv6, UDP, HTTP/2, SCTP and the option-area headers. ICMPv4.read reads a recorded cut as TRUNCATED.

The shipped captures keep their octets and messages, and every per-layer rebuild is exact; only genuinely cut layers gain __short_read__. The new tests fail on main. A trial merge with #2737 passes both sides' tests.

Closes #2711

@JarryShaw JarryShaw added this to the PyPCAPKit v1.6 milestone Nov 3, 2026
@JarryShaw JarryShaw added bug Issues reporting a defect (set by the bug report template; a default, not an assessment) review: good-to-go Cross-review at the current head says ready; CI state is separate labels Nov 3, 2026
@JarryShaw JarryShaw moved this to In review in PyPCAPKit Nov 3, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at c9e0c968a, from a Sonnet review (round 4).

@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 94.44% (unit tier, Python 3.15, 2ade9f4b3, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 2 95.16%
pcapkit/const 20918 175 2908 156 98.57%
pcapkit/corekit 2889 76 928 37 96.83%
pcapkit/dumpkit 286 3 102 3 98.45%
pcapkit/foundation 3505 27 1216 32 98.75%
pcapkit/interface 138 4 52 3 96.32%
pcapkit/protocols 22694 154 5898 126 99.01%
pcapkit/toolkit 961 26 342 8 96.62%
pcapkit/utilities 559 7 150 5 98.31%
pcapkit/vendor 5590 2286 1280 214 55.75%

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

@JarryShaw

Copy link
Copy Markdown
Owner Author

NEEDS CHANGES, following the ruling on #2711 (option 2). An overstated count in a standalone header-only IGMPv3 Report must read deterministically. Records past those present are never zero-filled, and the cut is recorded through a new list-field mechanism, not the zero-fill budget. Round 5 is in progress; the rest of the PR stands.

@JarryShaw JarryShaw added review: needs-changes Cross-review at the current head says changes are required; see the verdict comment and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Nov 3, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

resolve conflicts

@JarryShaw

Copy link
Copy Markdown
Owner Author

Will do. #2737 has merged, so this PR now conflicts with main in the Schema.unpack docstring; the two changes were already known to compose. Round 5, for the option-2 ruling, is in progress on c9e0c968a. It will be rebased onto main with #2737, the composed tests rerun, and pushed as one revision.

…s on the layer (#2711)

- Rebuilding an outer layer rebuilt its payload by from_data, whose own parse
  reported what the octets carry, and then parsed them again itself, so
  Ethernet over a cut IGMPv3 reported its messages twice. Under rebuilding,
  ProtocolBase.__post_init__ drops the payload's parse warnings with
  hold_warnings, and beholder builds the errors it absorbs quiet through
  CaughtByCaller and logs none; the enclosing layer's parse reports them once.
  The owner ruled on #2711 that this carve-out from the handled-refusals rule
  stands, as protocol-family-grouping.rst now records.
- An IGMPv3 Report parsed on its own and cut on a field boundary before its
  counted records read INVALID. ListField now takes a count: a list whose
  octets hold fewer items than it counts reports the shortfall, and
  Schema.unpack records it as a short read, the items absent never made up;
  the fields after it find the record in packet, as #2727 gives them.
  Report.records counts num_group_records for a message parsed on its own, so
  such a cut reads TRUNCATED whatever the context parsed before; inside a frame
  the count sizes nothing. A count no IP payload could carry raises
  ProtocolError.
- A message cut inside the length its context gives it read TRUNCATED in its
  frame but INVALID rebuilt on its own. Schema.unpack records a cut in a payload
  of a declared length; keep_short_read records a frame cut on a layer only
  where Schema._frame_end_field finds its last field sized by what is left of
  the length it was given, so a layer whose header declares its end (IPv4,
  IPv6 and its extension headers, UDP, an HTTP/2 frame) records none at or past
  it, nor one ending in a list, as SCTP's chunks, whose buffer holds only what
  its items took. The TSO IPv4 payload is sized that way, and ICMPv4 reads a
  recorded cut as TRUNCATED, as GRE and IGMP do.
- Tests that pinned no record for those cuts now pin the record.

The new modules fail without the fix; the protocols, corekit, foundation,
utilities and project modules pass. Sample captures give the same octets and
messages; cut frames gain __short_read__ on Ethernet, IPv4 and UDP.

Closes #2711
@JarryShaw
JarryShaw force-pushed the fix/2711-rebuild-noise-cut-record branch from c9e0c96 to 2ade9f4 Compare November 3, 2026 18:01
@JarryShaw

Copy link
Copy Markdown
Owner Author

Round 5 (option 2) is pushed, rebased onto main dea098b15: c9e0c968a → 2ade9f4b3. Conflicts resolved.

@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 Nov 3, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

GOOD TO GO at 2ade9f4b3, from a Sonnet review of round 5.

  • Deterministic: tested at counts 1, 2, 17, 100, 1000 and 8190 with 0–2 records present. Each reads the same fresh and after the context had read ~40 KB. It is TRUNCATED with the same short-read record, and nothing is zero-filled. 8191 and up raises ProtocolError both times.
  • Round-trips are byte-exact, overstated count included. Nine IPv4 frames come out byte-identical to round 4.
  • ListField without count behaves as before, and only Report opts in. The packet test fails without the argument. Project tests and Lint pass.
  • The two calls: a count-only ceiling on parse is fine, since make() is where too-large values raise. The direct-parse packet length < 0 warning is unchanged from main and is tracked separately.

@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 Nov 3, 2026
@JarryShaw
JarryShaw merged commit bdf5c99 into main Nov 3, 2026
65 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in PyPCAPKit Nov 3, 2026
@JarryShaw
JarryShaw deleted the fix/2711-rebuild-noise-cut-record branch November 3, 2026 19:58
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Nov 3, 2026
JarryShaw added a commit that referenced this pull request Nov 3, 2026
- Fixed: a layer rebuilt by from_data reports its payload's messages once,
  where 1.5.x logged them twice, and IPv4 and the frame-end layers record a
  cut as their own short read (#2738), a new entry; old values measured
  against v1.5.1.
- Folded into unreleased entries (#2738): a standalone IGMPv3 Report sized
  by its record count, into the IGMP entry; the recorded cuts' TRUNCATED
  status, TSO IPv4 and ICMPv4 included, into the checksum entry; ListField's
  count, into its zero-fill entry; the quiet rebuilt payload, into the
  conventions entry.
- #2748: pcapkit.utilities.lazy becomes pcapkit.utilities.importlib in the
  lazy-import and conventions entries; the old name appears nowhere.
- No entry for the direct-pushed 21ec0a6, which has no pull request or
  issue to cite.
- CHANGELOG.md regenerated; util/changelog_md.py --check passes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Issues reporting a defect (set by the bug report template; a default, not an assessment)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(protocols): rebuild noise and cut-recording gaps found reviewing #2703

1 participant