Skip to content

Commit 7cb1c25

Browse files
committed
fix(tcp): Quick-Start Response reader reads a stray octet; maker fails 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
1 parent a7ef878 commit 7cb1c25

3 files changed

Lines changed: 88 additions & 13 deletions

File tree

‎pcapkit/protocols/transport/tcp.py‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1348,7 +1348,6 @@ def _read_mode_qs(self, schema: 'Schema_QuickStartResponse', *, options: 'Option
13481348
ProtocolError: If length is **NOT** ``8``.
13491349
13501350
"""
1351-
size = self._read_unpack(1)
13521351
if schema.length != 8:
13531352
raise ProtocolError(f'{self.alias}: [OptNo {schema.kind}] invalid format')
13541353

@@ -2576,7 +2575,10 @@ def _make_mode_qs(self, code: 'Enum_Option', opt: 'Optional[Data_QuickStartRespo
25762575
diff = opt.ttl_diff
25772576
nonce = opt.nonce
25782577

2579-
rate_val = math.floor(math.log2(rate * 1000 / 40000)) if rate > 0 else 0
2578+
# :rfc:`4782` Section 3.1 encodes the rate as 40 kbps * 2 ** N, with
2579+
# N = 0 meaning a rate of zero; a response may report a lower rate than
2580+
# requested (Section 4.3), so a rate under 80 kbps rounds down to N = 0.
2581+
rate_val = max(math.floor(math.log2(rate * 1000 / 40000)), 0) if rate > 0 else 0
25802582
diff_val = diff if isinstance(diff, int) else math.floor(diff.total_seconds())
25812583

25822584
return Schema_QuickStartResponse(

‎tests/protocols/test_option_roundtrip_unit.py‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -137,17 +137,6 @@ class Gap(NamedTuple):
137137

138138
# -- TCP ------------------------------------------------------------------
139139

140-
# ``_make_mode_qs`` computes ``rate_val`` as a floor of a logarithm that is
141-
# negative for any rate under 40 kbps, and a negative value then fails to
142-
# pack into the 4-bit field with ``ValueError: invalid literal for int()
143-
# with base 2: b'0000-110'``. At the default ``rate=0`` it is guarded, and
144-
# the option constructs -- and then will not parse back.
145-
'tcp-option/Quick_Start_Response': Gap(
146-
'PARSE', 'StructError',
147-
'pcapkit/protocols/transport/tcp.py:2465 -- _make_mode_qs; rate_val is '
148-
'negative below 40 kbps, and the option it emits at rate=0 does not '
149-
'parse back'),
150-
151140
# #541 declared real ``kind``/``length`` fields on ``MPTCP``, which got six
152141
# of these seven far enough to construct and pack, and #566/#567 closed
153142
# the rest of the chain: #566 gave ``MPTCP.subtype`` the same treatment
Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
# -*- coding: utf-8 -*-
2+
"""The TCP Quick-Start Response option (kind 27), read and constructed.
3+
4+
GitHub issue #1119 found two defects in this option:
5+
6+
* :meth:`TCP._read_mode_qs <pcapkit.protocols.transport.tcp.TCP._read_mode_qs>`
7+
read one octet past the option before checking its length, so every Quick-Start
8+
Response failed with ``StructError: unpack: empty buffer``, whether or not a
9+
payload followed.
10+
* :meth:`TCP._make_mode_qs <pcapkit.protocols.transport.tcp.TCP._make_mode_qs>`
11+
computed the rate code as ``floor(log2(rate / 40))``. That is negative for
12+
``0 < rate < 40`` kbps, and a negative code cannot pack into the 4-bit field.
13+
14+
:rfc:`4782` Section 4.2 gives the option layout, and Section 3.1 the rate
15+
encoding: ``40 kbps * 2 ** N``, where ``N = 0`` means a rate of zero. Section 4.3
16+
lets a response report a lower rate than the one requested, so a rate below the
17+
smallest non-zero code (80 kbps) is rounded down to ``N = 0`` rather than refused.
18+
19+
Every case builds its own octets in memory, so this belongs to the unit tier.
20+
21+
"""
22+
from __future__ import annotations
23+
24+
import unittest
25+
26+
from pcapkit.const.tcp.option import Option
27+
from pcapkit.protocols.transport.tcp import TCP
28+
29+
#: Quick-Start Response option kind, per :rfc:`4782`.
30+
QS = Option.Quick_Start_Response
31+
32+
33+
def build(rate: 'int', payload: 'bytes' = b'') -> 'bytes':
34+
"""Construct a segment that carries one Quick-Start Response option."""
35+
return TCP(srcport=1, dstport=2, payload=payload,
36+
options=[(QS, {'rate': rate, 'diff': 3, 'nonce': 0x1234})]).data
37+
38+
39+
def parse(raw: 'bytes') -> 'TCP':
40+
"""Parse ``raw`` as a TCP segment."""
41+
return TCP(raw, len(raw))
42+
43+
44+
class QuickStartResponseTests(unittest.TestCase):
45+
"""Quick-Start Response against :rfc:`4782` Sections 3.1 and 4.2."""
46+
47+
def test_wire_octets_parse(self) -> 'None':
48+
"""Hand-built option octets parse, with and without a payload."""
49+
option = bytes([27, 8, 0x04, 3]) + (0x1234 << 2).to_bytes(4, 'big')
50+
header = bytes.fromhex('0001000200000000000000007010ffff00000000')
51+
for payload in (b'', b'xyz'):
52+
with self.subTest(payload=payload):
53+
tcp = parse(header + option + payload)
54+
opt = tcp.info.options[QS]
55+
self.assertEqual(opt.length, 8)
56+
self.assertEqual(opt.req_rate, 640)
57+
self.assertEqual(opt.ttl_diff, 3)
58+
self.assertEqual(opt.nonce, 0x1234)
59+
self.assertEqual(bytes(tcp.payload.data), payload)
60+
61+
def test_round_trip(self) -> 'None':
62+
"""Construct, parse and construct again gives identical octets."""
63+
for rate in (0, 80, 640, 1_310_720):
64+
for payload in (b'', b'xyz'):
65+
with self.subTest(rate=rate, payload=payload):
66+
raw = build(rate, payload)
67+
tcp = parse(raw)
68+
self.assertEqual(tcp.info.options[QS].req_rate, rate)
69+
again = TCP(srcport=1, dstport=2, payload=payload,
70+
options=tcp.info.options).data
71+
self.assertEqual(again, raw)
72+
self.assertEqual(bytes(tcp.payload.data), payload)
73+
74+
def test_rate_rounds_down_to_a_representable_code(self) -> 'None':
75+
"""A rate is floored to ``40 * 2 ** N``, and below 80 kbps to ``N = 0``."""
76+
for rate, code in ((1, 0), (20, 0), (39, 0), (40, 0), (79, 0), (80, 1), (159, 1), (160, 2)):
77+
with self.subTest(rate=rate):
78+
raw = build(rate)
79+
self.assertEqual(raw[20:24], bytes([27, 8, code, 3]))
80+
self.assertEqual(parse(raw).info.options[QS].req_rate, 40 * 2 ** code if code else 0)
81+
82+
83+
if __name__ == '__main__':
84+
unittest.main()

0 commit comments

Comments
 (0)