Skip to content

fix(ipv4,tcp)!: a header length declared past the data zero-fills on rebuild; TS make() rejects string address keys (#1402, #1404) - #1411

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1402-1404-ipv4-tcp
Oct 8, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1402-1404-ipv4-tcp

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) — mypy clean on both files; isort flags only pre-existing wrapping; pylint not run
  • make test passes, and a test case covers the change — legs protocols/internet 559/0/0, protocols --exclude protocols/internet 1025/0/0, integration 186/0/0; tests/project 399 passed, 1 skipped
  • Added a changelog entry — N/A, added centrally

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1402
Closes #1404

@JarryShaw JarryShaw added the fix Pull requests that fix a defect (fix: subject prefix) label Oct 8, 2026
@JarryShaw JarryShaw added breaking Breaks public-facing behaviour or API (apply alongside the type label) 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, 98c97bf06, 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 16412 219 4374 182 98.03%
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

Cross-review verdict on e1158688d: NEEDS CHANGES. The code fix holds; one test-coverage regression (Sonnet reviewed; author Opus; I reproduced the finding).

Everything else holds:

  • Fixtures. Across 1,588 frames, the only change is test.pcapng frame 2, which is Raw on both trees and now carries an informative error.
  • Fuzz. 1,500 Ethernet frames (IPv4/TCP with random ihl/offset, IPv4-in-IPv4, 6-in-4, 4-in-6, GRE): mismatches drop from 198 on main to 1, and that 1 also mismatches on main.
  • Reassembly is unaffected.
  • fix(ipv4): make() rejects a TS timestamp dict keyed by address strings #1402. All 30 key types give FieldValueError or correct octets.
  • Legs: internet 559, protocols 1025, integration 186, toolkit 287, corekit 633, foundation 522, all with 0 failures.

PR body: bool, float, IPv6Address and digit-string keys used to be silently coerced and now raise. Please list these under the breaking change.

@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
…rebuild; TS make() rejects string address keys (#1402, #1404)

- IPv4.read() and TCP.read() raise ProtocolError when ihl / Data Offset
  declares more header than the data holds, so the parent keeps the
  octets as a Raw payload and rebuilds them byte for byte (#1404).
- IPv4._make_opt_ts() converts timestamp address keys with
  parse_ip_address(version=4) instead of int(), so a dotted-quad string
  works like an IPv4Address and a bool raises FieldValueError (#1402).
- Tests that pinned the zero-filling parse now pin the refusal, and keep
  #431's end-of-option-list loop pinned at the schema level. The IPv4
  option-overrun fixtures get a complete header plus payload, so they
  still reach #1325's option check, and each check is asserted on its
  own message.

Closes #1402
Closes #1404
@JarryShaw
JarryShaw force-pushed the fix/1402-1404-ipv4-tcp branch from e115868 to 98c97bf Compare October 8, 2026 04:34
@JarryShaw

Copy link
Copy Markdown
Owner Author

Delta verdict on 98c97bf06: GOOD TO GO. The round-1 coverage gap is closed.

  • Tests-only delta. git diff e1158688d touches three test files (+21/−19) and no source.
  • fix(protocols): a declared length running past the data is accepted and zero-filled on rebuild #1325's option check is covered through IPv4 again. The 'IPv4 unassigned' fixture and the IPv4 truncated-option pin now use a complete header (ihl=7) plus payload, and assert the option check's own message. The header-length tests assert header length N runs past separately.
  • Revert proof. With collections.py taken from 6f4bf9dc8^ (before fix(protocols): a declared length running past the data is accepted and zero-filled on rebuild #1325), every option-check assertion fails, including 'IPv4 unassigned', while the header-length assertions still pass.
  • Results. I re-ran the four touched modules myself at this head and got 55/0. The author reports internet 559/0 and protocols 1025/0.
  • PR body. It now lists the newly refused TS keys (bool, float, IPv6Address, digit strings) under breaking, and corrects the earlier "unreachable" claim.

@JarryShaw JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 8, 2026
@JarryShaw
JarryShaw merged commit d3c375b into main Oct 8, 2026
38 checks passed
@JarryShaw
JarryShaw deleted the fix/1402-1404-ipv4-tcp branch October 8, 2026 12:57
@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

1 participant