Skip to content

fix(ipv4)!: the TS option zeroes the timestamp slots beyond the pointer on rebuild (#1349) - #1390

Merged
JarryShaw merged 3 commits into
mainfrom
fix/1349-ipv4-ts-slots
Oct 8, 2026
Merged

JarryShaw merged 3 commits into
mainfrom
fix/1349-ipv4-ts-slots

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort) — not run
  • make test passes, and a test case covers the change — scoped: every tests/protocols/internet/ module, test_option_roundtrip_unit.py, tests/project
  • 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 #1349. TS ts_data covered only pointer - 5 octets, and the rest was a zero-packing PaddingField (schema/internet/ipv4.py:321, :326). As #1319 did for LSR/SSR/RR, ts_data now holds every 32-bit slot. The slots at or beyond the pointer go in the new remaining data field (raw words), also a make() keyword. A paired flag never splits a pair. A pre-specified flag still keeps every whole pair; an odd trailing slot now goes in remaining instead of being dropped.

Breaking: TSOption data gains a required remaining field.

Before → after, from_data option bytes: 440c0500 0000000100000002 → …0000000000000000 / exact; 44100900 000000010000000200000003 → …000000010000000000000000 / exact.

Tests: new test_ipv4_ts_slots_unit.py (3 tests, 6 subtests); with the three source files reverted to origin/main, 8 fail. test_ipv4_unit.py updated in four places: they pinned the removed padding-as-pre-specified-address path or built TSOption without remaining. Results: 41 modules gave 384 passed and 2433 subtests passed. Leg protocols/internet ran 549 tests with 0 failures. tests/project gave 389 passed.

…r on rebuild

`ts_data` now models every 32-bit slot of the option; the slots at or
beyond the pointer are the new data field `remaining` (raw words) and a
make() keyword, as #1319 did for LSR/SSR/RR. A paired flag never splits a
pair, and a pre-specified flag keeps every whole pair as before, with an odd
trailing slot kept in `remaining` instead of dropped.

Closes #1349
@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: running A cross-review is in flight against the current head - no verdict yet labels Oct 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on a15053649: GOOD TO GO (Sonnet reviewed; author Opus).

  • Repros. Both issue repros now rebuild byte-exactly. The revert proof gives 8 failed, as claimed. I re-ran the new module myself and got 3/0.
  • Fuzz. 12,102 random TS options, with all 16 flags, lengths 4–40, pointers 0–44 and the overflow nibbles. The PR regresses no case. Exact round trips go from 153 to 2,962, and rebuild errors from 125 to 0. The 1,324 parse errors left are the same inputs main already rejects. A further ~3,800 make → parse → pack cases raise nothing.
  • The removed remainder path. It was dead on the parse path. The only callers were the two unit tests this PR rewrites.
  • breaking is justified. TSOption data gains a required remaining field, the same precedent as fix(ipv4): LSR/SSR/RR zero the route slots beyond the pointer on rebuild #1319. Nothing in the library reads its fields. Across the 22 fixture captures, the only info difference from main is remaining=() on options-ipv4.pcap frame 5.
  • Test edits. The edits to test_ipv4_unit.py are needed, and the bool-rejection assertions still hold. protocols/internet gives 549/0.

Nit: make() silently drops remaining entries beyond the option's slot count.

The reviewer found two round-trip defects that main already has. I am filing them separately:

  • a timestamp that comes back 1 ms off through timedelta;
  • a trailing partial slot that is zeroed on rebuild.

It also saw that options-ipv4.pcap frame 7 does not rebuild byte-exactly, on both trees.

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

Copy link
Copy Markdown
Owner Author

Follow-up to the verdict above.

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 89.40% (unit tier, Python 3.14, 4deb00a9f, 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 16268 203 4298 165 98.17%
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

The red Python 3.x legs are main's, not this diff. test_protocol_make_unit.py::TestMakeRawItems::test_unparsable_raw_item_raises_protocol_error also fails on main at 66950b1f2, where I reproduced it locally. It is a semantic conflict between #1344 and #1352, tracked in #1394, and a fix is in progress. This PR needs a re-run once #1394 lands.

@JarryShaw
JarryShaw merged commit 8874639 into main Oct 8, 2026
36 checks passed
@JarryShaw
JarryShaw deleted the fix/1349-ipv4-ts-slots branch October 8, 2026 02:35
@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(ipv4): the TS option zeroes the timestamp slots beyond the pointer on rebuild

1 participant