Skip to content

fix(protocols)!: a declared length running past the data is accepted and zero-filled on rebuild - #1398

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1325-declared-length-overrun
Oct 8, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1325-declared-length-overrun

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 8, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style — only mypy (no new errors on the touched source files) and isort were run; pylint was not
  • make test passes, and a test case covers the change — not the full suite; legs below
  • Added a changelog entry — N/A, added centrally

What is the purpose of your pull request?

  • fix — corrects a defect
  • feat · [ ] perf · [ ] refactor · [ ] test · [ ] docs · [ ] ci · [ ] release · [ ] chore

Description of your pull request and other information

Closes #1325. Implements ruling (1), strict everywhere. Breaking: what a malformed option looks like in info changes.

Leg results, all 0 failures: protocols/internet 552, protocols --exclude protocols/internet 1013, project 586, corekit 633. make_samples printed 0 case(s) left out.

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) breaking Breaks public-facing behaviour or API (apply alongside the type label) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 8, 2026
@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 89.39% (unit tier, Python 3.14, bc5d90f7b, 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 2038 89 634 25 94.76%
pcapkit/dumpkit 234 2 84 2 98.74%
pcapkit/foundation 2637 98 928 48 94.78%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 16354 219 4338 182 98.02%
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.

…and zero-filled on rebuild

- OptionField.unpack reads each option through a short-read proxy and
  raises ProtocolError when an option that began with octets present
  reads past the end of its data; one starting at the end keeps the
  end-of-option-list handling. A parent layer keeps the octets as
  captured and rebuilds them byte for byte.
- PCAP-NG: bounded_option refuses a payload running past its area
  instead of clamping it, and OptionAreaField keeps a malformed option
  or record area whole as one bytes item. The block parses with no
  options, exposes the area as options_raw / records_raw, and make
  writes it back verbatim, so extraction continues and the round trip
  is byte-exact.

BREAKING CHANGE: a truncated IPv4 or TCP option no longer parses with
zero-filled data, and a malformed PCAP-NG option area is no longer
clamped into options. The #431/#572 pins, the #1394 raw-item cases and
the PCAP-NG clamp pins are rewritten to match.

Closes #1325
@JarryShaw
JarryShaw force-pushed the fix/1325-declared-length-overrun branch from f984cb7 to bc5d90f Compare October 8, 2026 03:08
@JarryShaw
JarryShaw marked this pull request as ready for review October 8, 2026 03:09
@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 8, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review on bc5d90f7b: the reviewer's verdict was NEEDS CHANGES. My verdict is GOOD TO GO for #1325's scope (Sonnet reviewed; author Opus).

Why NEEDS CHANGES: "strict everywhere" is not fully met. None of the findings is a defect in this diff or a regression from main, so I filed them separately:

What the reviewer measured:

  • Tests: the foundation leg is 522/0 and tests/integration is 90 passed with 2 skipped. These were the two legs the author had not run.
  • Fixtures: across 1,614 frames from 23 captures, nothing differs from main and nothing raises.
  • Fuzz, 3,000 seeds per family: rebuild failures fell from 1,252 on main to 213 here (IPv4/TCP/HOPOPT/IPv6-Opts), and from 4,165 to 543 (PCAP-NG). The PR adds no new failure, and no parse raises out of its parent.
  • Test rewrites: the IPv4/TCP pins still assert real behaviour: a Raw payload, then an exact parent rebuild.
  • Revert proof: reverting collections.py fails 19 tests.

@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
@JarryShaw
JarryShaw merged commit 6f4bf9d into main Oct 8, 2026
41 checks passed
@JarryShaw
JarryShaw deleted the fix/1325-declared-length-overrun branch October 8, 2026 03:31
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaks public-facing behaviour or API (apply alongside the type label) fix Pull requests that fix a defect (fix: subject prefix)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

fix(protocols): a declared length running past the data is accepted and zero-filled on rebuild

1 participant