Skip to content

fix(ipv4): rebuild option areas, route slots, timestamps and reserved bits byte-exactly - #1344

Merged
JarryShaw merged 1 commit into
mainfrom
fix/ipv4-roundtrip
Oct 7, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/ipv4-roundtrip

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort): mypy clean and isort clean on the three modules; pylint adds no new message kinds over main
  • make test passes, and a test case covers the change: the modules listed below only, not the whole suite
  • Added a changelog entry — N/A, added centrally after the wave

What is the purpose of your pull request?

  • fix — corrects a defect

Closes #1316
Closes #1317
Closes #1318
Closes #1319
Closes #1320
Closes #1321
Closes #1322
Closes #1326

All 21 issue repros, plus non-zero octets after EOOL, used to rebuild wrong or raise, and now rebuild byte-exactly. All 1361 IPv4 layers in examples/captures/* still rebuild their header byte-exactly. #1321 was checked against RFC 1108 §2.4(a): the low-order bit is 0 on the final octet and 1 when more follow.

Breaking changes

  • New required data fields: Flags.reserved; remaining on LSROption/RROption/SSROption; reserved on both QS options; unused on QuickStartReportOption.
  • New data field IPv4.padding (octets after EOOL, kept verbatim). New make() keywords: reserved and padding on IPv4.make, remaining on LSR/RR/SSR, and unused/reserved on QS.
  • Schema changes: the IPv4 flags and the QS nonce BitFields now require a reserved key. LSR/RR/SSR route now covers every slot. QuickStartReportOption.reserved is now a UInt8Field. quick_start_data_selector falls back to UnassignedOption instead of raising FieldValueError.
  • Wire changes: make() writes SEC with the RFC indicator, so the generated options-ipv4.pcap changes. A TS integer is now the raw 32-bit field, so a set high bit is kept and no longer warns.

Tests (test_ipv4_roundtrip_unit.py is new; test_ipv4_unit.py pins updated)

  • Without the fix: 34 failed, 4 passed; the edited-options test fails on the previous head with invalid header length: 24. With it: 11 passed, 27 subtests.
  • test_ipv4_unit 26 passed, test_option_roundtrip_unit 6 passed, tests/project 389 passed / 1 skipped, run_unittest_leg.py protocols/internet 514 tests OK. Also test_quick_start_func_unit, test_option_coverage_runtime, test_generated_pcap_runtime, test_declared_length_roundtrip_runtime and 7 more IPv4-adjacent modules all pass.

Left out of scope: QSFunction admits only 0–8, so the selector catches EnumValueError for 9–15. TS still zero-fills slots past its pointer, the same shape as #1319.

@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 94c503fda: NEEDS CHANGES. The Sonnet reviewer (author Opus) returned GOOD TO GO with two non-blocking findings. I re-derived both, and one of them blocks this PR under the every-round-trip-matches ruling.

1. Octets after EOOL are still lost when they are non-zero. This is blocking: #1317 asks for exactly these octets to be kept. Measured in the PR worktree:

options area rebuilt exact
01000000 (NOP, EOOL, zeros) 01000000 yes
00ff0102 (EOOL, then non-zero) 00000000 no
00070304 00000000 no

The fix keeps trailing octets as EOOL entries, so it can only express zeros. Carry the raw octets after EOOL instead.

2. from_data now always passes header_length=data.hdr_len. I confirmed this: _make_data returns header_length=24, where main returned none. The reviewer measured that editing a parsed Data_IPv4 to add an option now raises ProtocolError: invalid header length: 24, and that removing one silently keeps the old IHL. main recomputed the length in both cases. This is a regression on edited data, the same shape as #1155's stale total_length. At minimum it needs a docstring line; recomputing whenever the options no longer fit is better.

Verified and accepted:

  • SEC indicator. It now matches RFC 1108 §2.4(a) and Wireshark's packet-ip.c (mask 0x01, loop while (val & 0x01)).
  • Option length. The minimum is 2, per RFC 791.
  • Reserved bits. The RFC 791 flag bit and the RFC 4782 (30, 2) layout both check out.
  • Route options. RR with pointers 4, 8, 12, 16, 255, 5 and 7 all rebuild.
  • Fixtures. All 1361 IPv4 fixture layers rebuild. info differs from main only by the new keys.
  • Tests. The new module gives 28 failures on the base tree. The protocols/internet leg runs 512 tests with 0 failures. 38 other modules, including reassembly, traceflow and every toolkit adapter, also pass.

For follow-up issues, all confirmed:

  • TS still zero-fills the slots after its pointer.
  • const/ipv4/qs_function.py:35 accepts only 0–8.
  • IPv6 hopopt.py:250 still raises on an unassigned Quick-Start function, which IPv4 now keeps.
  • A QS function in 9–15 with nonce ≥ 2³¹ raises a raw OverflowError.

@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 7, 2026
… bits byte-exactly

- make(): concatenate the options and pad once at the end, so an EOOL after
  an unaligned option no longer hides the rest (#1316).
- from_data: write parsed NOP/EOOL in place, and carry the octets after EOOL
  verbatim as the new `padding` data field and make() keyword (#1317).
- Unassigned options: data defaults to b'', length is len(data) + 2, and a
  length of 2 is accepted (#1318).
- LSR/SSR/RR: model every route slot; slots at or past the pointer are the
  new data field `remaining` and a make() keyword (#1319).
- TS: keep a timestamp with the high-order bit set as the raw value (#1320).
- SEC: the field termination indicator follows RFC 1108 section 2.4(a),
  0 on the final octet and 1 when more follow (#1321).
- QS: an unassigned function parses and rebuilds as an unassigned option,
  and make() builds one (#1322).
- Name the reserved flag bit, the QS nonce reserved bits and the QS report
  unused octet (#1326).
@JarryShaw

Copy link
Copy Markdown
Owner Author

Round-2 verdict on 816b8b6cc: GOOD TO GO. I verified the delta from 94c503fda myself: 4 files, +65/−36. Both blockers are fixed.

1. Octets after EOOL are kept. A new padding data field holds the octets between the end of the options and hdr_len, and make(padding=…) writes them back after an EOOL. Measured:

options area padding exact
00ff0102 ff0102 yes
00070304 070304 yes
0000ffff 00ffff yes
9404000000abcdef abcdef yes
01000000, 00000000, 01010100 zeros / empty yes

A fresh option list still pads exactly once (padding is ignored there, and a test pins it).

2. header_length is gone, so edited data recomputes the IHL as on main:

  • adding an option gives IHL 7;
  • removing one gives IHL 6;
  • removing all of them from a parsed header with trailing octets gives IHL 5.

total_length still passes through per #1155.

Revert proof: 34 failed / 4 passed against origin/main. protocols/internet leg: 514 tests, 0 failures. All 1361 fixture IPv4 layers still rebuild.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Coverage: 88.87% (unit tier, Python 3.14, 816b8b6cc, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1034 2342 845 90.24%
pcapkit/corekit 1878 89 580 22 94.43%
pcapkit/dumpkit 142 0 42 0 100.00%
pcapkit/foundation 2442 102 852 39 94.38%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15945 214 4134 182 98.00%
pcapkit/toolkit 495 73 150 1 83.57%
pcapkit/utilities 429 4 122 4 98.55%
pcapkit/vendor 4409 2359 1006 158 42.84%

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

@JarryShaw
JarryShaw merged commit 8ed5bd6 into main Oct 7, 2026
38 checks passed
@JarryShaw
JarryShaw deleted the fix/ipv4-roundtrip branch October 7, 2026 22:56
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 7, 2026
JarryShaw added a commit that referenced this pull request Oct 8, 2026
…o longer raises the wrapped error (#1395)

#1344 pads the IPv4 option area once at the end, so the schema now
accepts octets read() then rejects (an option whose length runs past the
area), and the reader's bare ProtocolError escaped the wrap #1352 put
around the schema re-parse only. unpack() now wraps the read as well when
the made header holds raw items, and the wrapped message carries the
original one. The same escape is fixed for TCP, HOPOPT, IPv6-Opts, MH,
SCTP and HIP.

Closes #1394
@JarryShaw JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment