Skip to content

fix(tcp): MPTCP options round-trip byte-exactly (#1212-#1217, #1220) - #1254

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

JarryShaw merged 1 commit into
mainfrom
fix/mptcp-roundtrip

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner

Please follow the guide below

  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort) — run on the three changed modules with the Makefile flags; no new message kinds
  • make test passes, and a test case covers the change — the modules listed below only, not the full suite
  • Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible — 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 #1212
Closes #1213
Closes #1214
Closes #1215
Closes #1216
Closes #1217
Closes #1220

Every MPTCP subtype now rebuilds byte-exactly. Reserved bits are mapped and kept. MP_FAIL reads its 12 reserved bits, and unknown subtypes read length - 3. DSS widths follow a/m, and its checksum follows the length. ADD_ADDR takes its family, port and HMAC from the length (RFC 8684), and RFC 6824 options still round-trip. MP_CAPABLE keeps C and D–G.

Breaking: these are new required data-model fields:

  • reserved on MPTCPJoinSYN/SYNACK/ACK, MPTCPDSS, MPTCPRemoveAddress, MPTCPPriority, MPTCPFallback and MPTCPFastclose
  • ack_wide and dsn_wide on MPTCPDSS
  • reserved, echo and hmac on MPTCPAddAddress
  • deny_join and reserved on MPTCPCapableFlag

MPTCPAddAddress.version now comes from the address family. The schema test dicts gained reserved/echo keys, and the ADD_ADDR one lost version. mptcp_add_address_selector reads length. make() output is unchanged by default. The ADD_ADDR maker now rejects an hmac that is not 8 octets, or one given with echo=True.

Probe (24 option repros, TCP.from_data(TCP(raw,len(raw)).info).data): 19 DIFF/ERR before (the 3 controls already OK) → 24 OK after. Fixtures: 1259 TCP frames round-trip, 0 bad.

Tests:

  • New test_mptcp_roundtrip_unit.py: 12 passed, 36 subtests. With the three source files reverted to main, 38 failed.
  • The test_tcp_udp_unit, test_tcp_mptcp_addaddr_unit and test_tcp_mptcp_error_message_unit tests now build the new fields. One rejection case moved from version 5 to length 9, and the OptNo count went 33→35. The tests/corekit/test_fields_misc.py ADD_ADDR rows now pass lengths 8 and 20.
  • Other MPTCP/TCP modules, test_option_roundtrip_unit and tests/project (389 passed) all pass. The protocols --exclude protocols/internet unittest leg ran 862 tests OK. test_fields_misc 10 passed, test_fields_ipaddress 23 passed.

@JarryShaw JarryShaw moved this to In review in PyPCAPKit Oct 7, 2026
@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
@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 88.92% (unit tier, Python 3.14, 7eb1b2110, Unit Tests run failure)

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 15806 190 4048 165 98.18%
pcapkit/toolkit 487 65 144 1 85.42%
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

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 634ac75cf: NEEDS CHANGES (Sonnet; author Opus).

  • Must fix: tests/corekit/test_fields_misc.py::SwitchFieldBoolDispatchTests::test_address_typed_switch_branches_still_take_a_real_address now fails: 5 subtests with KeyError: 'length'. Its hand-built table at :57-60 still passes {'test': {'version': 4/6}}, but mptcp_add_address_selector now reads pkt['length']. The module passes on main (10 passed). The leg excluding protocols/internet does not cover tests/corekit, which is why the PR's run missed it. The docstring at tests/corekit/test_fields_ipaddress.py:522 is stale too.
  • Confirmed: every issue repro rebuilds byte-exactly, and a 114-case MPTCP probe gives 114 OK. Every layout matches the RFC 8684 text. ADD_ADDR lengths 8/10/16/18 (IPv4) and 20/22/28/30 (IPv6) are unambiguous, and RFC 6824 options still round-trip. 1256 TCP fixture frames rebuild identically.
  • Nits:
    • The default ADD_ADDR make() emits E=0 without an HMAC (the RFC 6824 layout).
    • echo=True is accepted together with an HMAC.
    • A short hmac is zero-padded silently.

@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
- Map the reserved bits of MP_JOIN, DSS, REMOVE_ADDR, MP_PRIO, MP_FAIL
  and MP_FASTCLOSE into the schemas and keep them in a new `reserved`
  data-model field.
- MP_FAIL reads its 12 reserved bits, so the DSN starts at offset 4.
- Unknown subtypes size their data as `length - 3`.
- DSS keeps the `a`/`m` flags (`ack_wide`/`dsn_wide`) and they decide
  the field widths; the checksum is present only when the length has
  room for it.
- ADD_ADDR follows RFC 8684: family, port and HMAC from the length,
  `reserved`/`echo` bits and an optional `hmac`; RFC 6824 options still
  parse and round-trip.
- MP_CAPABLE keeps the C flag (`deny_join`) and bits D-G (`reserved`).

New data-model fields are required arguments, so existing tests that
build these models directly are updated.
@JarryShaw
JarryShaw force-pushed the fix/mptcp-roundtrip branch from 634ac75 to 7eb1b21 Compare October 7, 2026 18:25
@JarryShaw JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: needs-changes Cross-review at the current head says changes are required; see the verdict comment labels Oct 7, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 7eb1b2110: GOOD TO GO (Sonnet, round 2; author Opus).

  • Round-1 blocker fixed: tests/corekit/test_fields_misc.py gives 10 passed, and test_fields_ipaddress.py gives 23 passed. No other MPTCP schema in tests/ uses version. All 13 MPTCP-related modules pass.
  • Repros: all 13 issue repros rebuild byte-exactly, and fail on main. I re-ran the MP_FAIL repro myself.
  • Fixtures: every TCP fixture layer rebuilds exactly.
  • ADD_ADDR checks: these match RFC 8684 §3.4.1. An HMAC must be 8 octets, an echo carries no HMAC, and parsed options rebuild as read.
  • Error-message pin: the 34→35 change matches exactly the one new invalid HMAC message.
  • Legs:
    • protocols --exclude protocols/internet: 862 tests, 0 failures.
    • The corekit leg's 5 failures in test_sentinel_exports_unit.py also occur on clean main. They are pre-existing and will be filed separately.
  • Nit: with no HMAC and echo=False, ADD_ADDR still builds the legacy RFC 6824 layout.

@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
JarryShaw merged commit 1795393 into main Oct 7, 2026
37 of 39 checks passed
@JarryShaw
JarryShaw deleted the fix/mptcp-roundtrip branch October 7, 2026 19:08
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment