Skip to content

fix(tcp): from_data reorders and re-pads TCP options - #1163

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1151-tcp-options-roundtrip
Oct 7, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1151-tcp-options-roundtrip

Conversation

@JarryShaw

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort) — ran the tools on tcp.py only: mypy 0 errors, pylint no new messages (5 fewer); isort's complaint on tcp.py/test_tcp_udp_unit.py is already on main
  • make test passes, and a test case covers the change — the 17 tests/protocols/transport/test_tcp*/test_transport_unit modules, test_option_roundtrip_unit, and tests/project, each run on its own
  • Added a changelog entry — N/A, added centrally after the wave

What is the purpose of your pull request?

  • fix — corrects a defect

Description of your pull request and other information

Closes #1151. _make_tcp_options dropped the NOP/EOOL options it was given and padded each option to 4 octets separately. Now it emits options as given and pads once at the end: zero octets after a trailing EOOL, otherwise NOPs then an EOOL.

Probe: parse each TCP segment, TCP.from_data(info), compare header octets. The payload maker is stubbed so the HTTP rebuild (#1154) does not interfere.

capture before after
http.pcap 0/1117 1117/1117
http6.cap / test.pcap 0/26, 0/32 26/26, 32/32
many_interfaces.pcapng / tcp.pcap / stream.pcap 0/32, 0/7, 0/5 all

http.pcap before: a002…0402080a…01030306 → b002…040201 00 080a…0100 030306 00. options-*.pcap regenerate byte-identical.

New test file tests/protocols/transport/test_tcp_options_roundtrip_unit.py: 4 passed with the fix, 4 failed without it. test_tcp_udp_unit.py pinned the old NOP-dropping behaviour and is updated. EXPECTED_FAILURES had no TCP entries, so it is unchanged.

_make_tcp_options dropped every NOP/EOOL it was given as "padding" and
then padded each option to 4 octets on its own, inserting an EOOL after
any misaligned option. A parsed segment therefore rebuilt with its NOPs
moved, mid-list EOOLs, and a larger data offset (a0 -> b0 on http.pcap).

Options are now emitted as given, and the list is padded once at the end:
zero octets after a trailing EOOL, otherwise NOPs followed by an EOOL.

test_tcp_udp_unit pinned the old drop-the-NOPs behaviour and is updated.

Closes #1151
@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: pending No verdict for the current head - never reviewed, or the head moved since the last one review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on 5b3c5ad9a: GOOD TO GO (ran on Sonnet; author Opus). Also labelled breaking: this changes the pack path.

  • Root cause confirmed: on main, TCP(options=[WindowScale(7), MSS(1460)]) packs 03030700 020405b4. The EOOL after WindowScale ends option parsing, so the MSS is hidden. I re-ran this myself. The PR packs 030307020405b400. On main, rebuilt headers carry a mid-list EOOL 222 times in http.pcap and 4 times each in http6.cap, test.pcap and many_interfaces.pcapng; with the PR there are none.
  • Byte-exact header rebuilds: 1117/1117 http.pcap, 26/26 http6.cap, 32/32 test.pcap, 32/32 many_interfaces.pcapng, 7/7 tcp.pcap and 5/5 stream.pcap, against 0 on main. options-*.pcap regenerate byte-identical.
  • Padding: follows RFC 9293 §3.1. Across 600 fuzzed option lists, data_offset*4 == len(header) held and every list re-parsed cleanly.
  • Edited assertions in test_tcp_udp_unit.py: they pinned a defect recorded by the bulk commit 78e28d811.
  • Fails without the fix: 8 failed. With fix(http): HTTP.from_data raises 'invalid HTTP version: 0' #1162 merged alongside, a full TCP.from_data round trip with payload gives 1117/1117 byte-exact http.pcap segments.
  • Follow-up, not in this diff: examples/generators/options.py:1606-1617 still says NOP/EOOL are dropped by _make_tcp_options. That is now false, though the SKIP entries still work.

@JarryShaw JarryShaw added breaking Breaks public-facing behaviour or API (apply alongside the type label) 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 6, 2026
@github-actions

github-actions Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Coverage: 88.86% (unit tier, Python 3.14, 5b3c5ad9a, 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 91 580 22 94.34%
pcapkit/dumpkit 142 0 42 0 100.00%
pcapkit/foundation 2422 104 842 40 94.24%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15674 188 3950 163 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
JarryShaw merged commit c504804 into main Oct 7, 2026
44 checks passed
@JarryShaw
JarryShaw deleted the fix/1151-tcp-options-roundtrip branch October 7, 2026 01:44
@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 7, 2026
- 4 new 1.5.0 entries: HTTP/2 sample stream identifiers (#1161),
  TCP option re-padding (#1163, breaking), format='pcap' output
  (#1164), declared lengths on truncated captures (#1166, breaking)
- extend the PCAP Header (#1159), HIP (#1160) and from_data
  round-trip (#1162) entries rather than duplicating them
- regenerate CHANGELOG.md with util/changelog_md.py
- process.rst: entry-count pin re-measured, 189 -> 193

tests/project: conventions_doc_claims, changelog_md and
documentation_claims pass; changelog_md.py --check is in step.
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(tcp): from_data reorders and re-pads TCP options

1 participant