Skip to content

fix(tcp)!: reserved bits, post-EOOL octets and the UTO granularity round-trip (#1211, #1218, #1219) - #1348

Merged
JarryShaw merged 1 commit into
mainfrom
fix/tcp-roundtrip-reserved-eool-uto
Oct 7, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/tcp-roundtrip-reserved-eool-uto

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style — mypy and isort --check-only are clean on the three TCP modules; pylint adds no new messages on changed lines (its existing W0613/C0301 baseline remains)
  • make test passes, and a test case covers the change. I ran the new module, the TCP/MPTCP modules, test_option_roundtrip_unit, tests/project, and run_unittest_leg.py protocols --exclude protocols/internet. I did not run the full suite.
  • Changelog entry — N/A, added centrally

What is the purpose of your pull request?

  • fix — corrects a defect

Closes #1211
Closes #1218
Closes #1219
Refs #1223. The TCP repro there, 020405b4 00aabbcc, now round-trips because the post-EOOL octets are kept. PaddingField is unchanged.

Before → after, on c9676c563, comparing from_data(TCP(raw).info) with raw: header octet 12 5e, POC-SP 0a033f00, QS 1b08834012345679, EOOL 00010101 020405b4 at DO 7, and UTO 1c048005 all used to come back mismatched. Now all are byte-exact.

Breaking: these data-class fields are now required: Flags.reserved, PartialOrderServiceProfile.reserved, QuickStartResponse.reserved and .nonce_reserved, and UserTimeout.granularity. The TCP offset, POC-SP profile, QS flags and nonce BitField dicts also need a reserved key. Data_TCP gains padding whenever options are present. make() gains padding and reserved. padding is re-emitted only while the options still end with EOOL. If the EOOL is edited out, the padding is dropped and the Data Offset is recomputed. A User Timeout keeps its parsed unit only while the value still fits in it, and switches unit otherwise. Six existing test modules built the old shapes by hand, so I updated them. One of them, test_tcp_mptcp_join_flag_ordering_unit, greps make()'s source, and its substring is now _make_tcp_options(options.

Tests: the new tests/protocols/transport/test_tcp_roundtrip_unit.py has 11 passed and 25 subtests passed. Against origin/main it gives 26 failed and 5 passed. The new edited-data tests (EOOL removed, NOP and EOOL removed, every option removed, UTO unit switch) give 8 failed against the first revision, 1fd285e7a. All 1233 TCP frames across the 22 captures still rebuild byte-exactly. test_option_roundtrip_unit gives 6 passed and 362 subtests passed, with no tcp entry in EXPECTED_FAILURES. The ordering leg ran 909 tests with 0 failures, and tests/project gives 389 passed and 1 skipped.

@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 1fd285e7a: NEEDS CHANGES (Sonnet reviewed; author Opus). One edited-data regression, which I re-derived myself.

padding goes stale when the EOOL is removed. I parsed a header with MSS, NOP, EOOL plus 6 padding octets. It rebuilds byte-exactly. Then I removed the options and called make() from _make_data's output, which raises ProtocolError: TCP: padding given without an End of Option List option. main built it. This is the same edited-data shape that IPv4 #1344 had to fix today. Drop the carried padding when the option list no longer ends in EOOL, and add tests for removing the EOOL and for removing every option.

Minor, same class: editing a parsed UTO with G clear to 40000 s raises timeout value too large: 0, where main auto-switched to minutes. The message also prints the timeout argument rather than the value.

Held up:

@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
…und-trip (#1211, #1218, #1219)

- #1211: name the reserved bits of the header (octet 12, bits 4-6), the
  POC-SP Filler and the Quick-Start Resv./R bits in the schema, carry them
  in Flags.reserved, PartialOrderServiceProfile.reserved and
  QuickStartResponse.reserved/nonce_reserved, and write them back.
- #1218: keep the octets after EOOL as TCP.padding and re-emit them while
  the options still end with EOOL, so the rebuilt Data Offset matches the
  parsed one; once the EOOL is edited out the padding is dropped and the
  Data Offset is recomputed.
- #1219: store the G bit as UserTimeout.granularity and honour it in the
  maker while the value still fits in that unit, switching unit otherwise;
  the maker also takes an explicit granularity keyword, and the too-large
  error reports the actual value.

Breaking: new required data-class fields, and the offset/profile/flags/nonce
BitField dicts need a 'reserved' key. Existing tests that build those
schemas and data by hand are updated to the new shape.
@JarryShaw
JarryShaw force-pushed the fix/tcp-roundtrip-reserved-eool-uto branch from 1fd285e to 72615ea Compare October 7, 2026 20:23
@JarryShaw

Copy link
Copy Markdown
Owner Author

Round-2 verdict on 72615ea54: GOOD TO GO. I verified the delta from 1fd285e7a myself: 2 files, +100/−36.

The blocker is fixed: padding is written back only while the options still end in EOOL, and Data Offset recomputes otherwise. I parsed MSS, NOP, EOOL plus 6 padding octets (which rebuilds byte-exactly), then rebuilt it with options removed:

removed Data Offset options written
EOOL 7 020405b401010100 (realigned)
NOP + EOOL 6 020405b4
everything 5 none

UTO units: seconds stay seconds while they fit in 15 bits and minutes stay minutes only for whole minutes, so 40000 s with G clear becomes 666 min (1c04829a). The error now prints the real value. The parsed 1c048001, 1c040001, 1c047fff and 1c04ffff all still rebuild exactly.

Tests: the new module fails 8 tests against the previous head and 26 against origin/main. The protocols --exclude protocols/internet leg runs 909 tests with 0 failures.

@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 7, 2026
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

Coverage: 88.94% (unit tier, Python 3.14, 72615ea54, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18777 1032 2348 843 90.27%
pcapkit/corekit 1930 89 602 22 94.59%
pcapkit/dumpkit 189 1 68 1 99.22%
pcapkit/foundation 2442 102 852 39 94.38%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15993 207 4168 178 98.05%
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 51eb712 into main Oct 7, 2026
36 checks passed
@JarryShaw
JarryShaw deleted the fix/tcp-roundtrip-reserved-eool-uto branch October 7, 2026 23:01
@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 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

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