Skip to content

fix(tcp): Quick-Start Response reader reads a stray octet; maker fails below 40 kbps - #1135

Merged
JarryShaw merged 1 commit into
mainfrom
fix/1119-tcp-qs-response
Oct 6, 2026
Merged

JarryShaw merged 1 commit into
mainfrom
fix/1119-tcp-qs-response

Conversation

@JarryShaw

@JarryShaw JarryShaw commented Oct 6, 2026 •

Copy link
Copy Markdown
Owner
  • Searched for similar pull requests
  • Followed the coding style (make pylint, make mypy, make isort). Ran with the Makefile flags on tcp.py only: no new findings. pylint's W0612 unused-variable 'size' is gone.
  • make test passes, and a test case covers the change. Not the full suite; the modules run are listed below.
  • Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible — N/A, added centrally after the wave

What is the purpose of your pull request?

  • fix — corrects a defect
  • feat — adds a feature
  • perf — changes performance, not behaviour
  • refactor — changes neither behaviour nor performance
  • test — tests only
  • docs — documentation only
  • ci — workflows or build tooling
  • chore — anything else

Description of your pull request and other information

Closes #1119

  • Reader. _read_mode_qs called self._read_unpack(1) past the option before its length check. That line is removed.
  • Maker. _make_mode_qs produced a negative rate code below 40 kbps. It now clamps to N = 0. RFC 4782 §3.1 defines the rate as 40 kbps * 2**N, with N=0 meaning a rate of zero. §4.3 allows a response to report a lower rate than requested, so rounding down is valid and nothing is refused.
  • Removes the tcp-option/Quick_Start_Response EXPECTED_FAILURES entry. make_samples now captures the option (257 cases, 7 left out).

Probe (rates 0 / 80 / 640 + payload / 20 / 60 kbps):

  • before: StructError: unpack: empty buffer ×4; at 20 kbps ValueError … b'0000-001'
  • after: 0, 80.0, 640.0, 0, 0, each with ttl_diff=3, nonce=0x1234

Tests: new tests/protocols/transport/test_tcp_quick_start_response_unit.py covers wire parsing and round trips, with and without payload, and rate flooring. Without the fix it gives 18 failed. With only the reader fixed, the 3 sub-40 kbps subtests fail. Each module was run in its own process:

  • new module: 3 passed, 18 subtests passed
  • test_option_roundtrip_unit: 6 passed, 360 subtests passed
  • test_tcp_udp_unit: 18 passed; test_tcp_runtime: 5 passed
  • tests/project: 379 passed, 1 skipped

@JarryShaw JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 6, 2026
@JarryShaw

Copy link
Copy Markdown
Owner Author

Cross-review verdict on fe0471fff: GOOD TO GO (ran on Sonnet; author Opus)

  • Reader: the stray _read_unpack(1) is removed. On main, make_samples.py leaves tcp-option/Quick_Start_Response out with StructError: unpack: empty buffer. With this PR it parses.
  • Maker: rates under 80 kbps now clamp to N=0. This fits RFC 4782 §3.1 and §4.3: N=0 means zero, and a response may report a lower rate. Probed across 20–1,310,720 kbps; each rate packs to the expected code and parses back.
  • Tests:
    • with tcp.py reverted, the new module fails 18;
    • with only the clamp reverted, 3 subtests fail;
    • test_option_roundtrip_unit passes 360.
  • Known limits, not regressions:
    • Rates of 2.6 Gbps or more raise FieldValueError, because N=16 does not fit in 4 bits.
    • req_rate comes back as a float, and main does the same.

Merge order: this PR and #1131 both delete neighbouring EXPECTED_FAILURES entries in test_option_roundtrip_unit.py, so the two conflict textually there. Whichever merges second needs a trivial rebase. The merge with #1114 in tcp.py is clean.

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

github-actions Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Coverage: 88.75% (unit tier, Python 3.14, 6011af7d7, Unit Tests run success)

Package Statements Missed Branches Partial Cover
pcapkit (top level) 104 4 20 4 93.55%
pcapkit/const 18757 1035 2342 846 90.23%
pcapkit/corekit 1873 91 578 22 94.33%
pcapkit/dumpkit 136 0 40 0 100.00%
pcapkit/foundation 2425 145 842 34 92.56%
pcapkit/interface 112 7 40 5 92.11%
pcapkit/protocols 15658 188 3944 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 added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 6, 2026
…s below 40 kbps

- _read_mode_qs read one octet past the option before its length check, so
  every Quick-Start Response failed with "StructError: unpack: empty buffer".
- _make_mode_qs took floor(log2(rate / 40)), negative for rates under 40 kbps,
  which cannot pack into the 4-bit field. Clamp to N = 0: RFC 4782 Section 3.1
  defines N = 0 as a rate of zero, and Section 4.3 lets a response report a
  lower rate than requested.
- Drop the tcp-option/Quick_Start_Response EXPECTED_FAILURES entry, now closed.
- Add tests/protocols/transport/test_tcp_quick_start_response_unit.py.

Closes #1119
@JarryShaw
JarryShaw force-pushed the fix/1119-tcp-qs-response branch from fe0471f to 6011af7 Compare October 6, 2026 20:41
@JarryShaw JarryShaw added 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 6011af7d7: GOOD TO GO (ran on Sonnet; author Opus, round 2)

This round reviewed the rebase onto main at dbb30c4a3, after #1131 merged.

  • The tcp.py change is the same as in round 1: the stray _read_unpack(1) is removed, and the rate is clamped with max(..., 0).
  • EXPECTED_FAILURES has no tcp-option/ entries left; the import was checked against the worktree.
  • Nothing from fix(tcp): User Timeout maker writes length=3; RFC 5482 requires 4 #1131 was lost: _make_mode_timeout still writes length=4, and its test passes.
  • test_tcp_quick_start_response_unit passes 3 and test_option_roundtrip_unit passes 360 subtests.

The round-1 findings still hold.

@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 6, 2026
@JarryShaw
JarryShaw merged commit 7cb1c25 into main Oct 6, 2026
62 of 64 checks passed
@JarryShaw
JarryShaw deleted the fix/1119-tcp-qs-response branch October 6, 2026 22:31
@JarryShaw JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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): Quick-Start Response reader reads a stray octet; maker fails below 40 kbps

1 participant