Skip to content

Commit a660bf3

Browse files
authored
fix(tcp): correct MPTCP unknown-subtype length and drop an undeclared parse-only attribute (#2396)
#2293: _MPTCP.post_process set ret.option = Enum_Option.Multipath_TCP on the subtype schema. MPTCP declares no option field, the declared kind field already holds Multipath_TCP, and no _make_mptcp_* maker mirrors it -- so a parsed schema carried a key a built one did not, though both packed identical octets. Drop the assignment; length and subtype stay, since those really are set here and nowhere else on the unpacking path. #2272: an unknown MPTCP subtype with data=b'' declared Length 2 but MPTCPUnknown always packs 3 header octets (kind, length, and the subtype octet whose low nibble takes data[0]) plus data[1:]. The two agree for any non-empty data and disagree only at data=b''. Size the length from what the schema actually writes: 3 + len(data[1:]). Tests: option-attribute parity across MP_CAPABLE, MP_FASTCLOSE and the MPTCPUnknown fallback; declared-vs-wire length across payload widths, the 3-octet minimum, and a real parse-back through the public TCP() constructor. test_tcp_udp_unit.py's post_process assertion is updated to require option's absence rather than its value.
1 parent fb66c60 commit a660bf3

5 files changed

Lines changed: 495 additions & 3 deletions

File tree

‎pcapkit/protocols/schema/transport/tcp.py‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -730,7 +730,16 @@ def post_process(self, packet: 'dict[str, Any]') -> 'MPTCP':
730730
"""
731731
ret = self.data
732732

733-
ret.option = Enum_Option.Multipath_TCP
733+
# NOTE: Only attributes that the construction path sets too are assigned
734+
# here, so that a schema parsed off the wire carries exactly the same
735+
# attributes as one built in memory by ``TCP._make_mode_mp`` (#2293). An
736+
# earlier revision also set ``ret.option = Enum_Option.Multipath_TCP``,
737+
# which no maker mirrored and nothing ever read: ``MPTCP`` declares no
738+
# ``option`` field, and its real ``kind`` field already carries
739+
# ``Multipath_TCP``. ``subtype`` is the one attribute that genuinely has
740+
# to be assigned on both paths -- see the NOTE on :class:`MPTCP` for why
741+
# it cannot be a field -- and ``TCP._make_mode_mp`` makes the matching
742+
# assignment for construction.
734743
ret.length = self.test['length']
735744
ret.subtype = Enum_MPTCPOption.get(packet['test']['subtype'])
736745

‎pcapkit/protocols/transport/tcp.py‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2895,9 +2895,21 @@ def _make_mptcp_unknown(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_M
28952895
if opt is not None:
28962896
data = opt.data
28972897

2898+
# NOTE: The length is sized from what the schema actually writes, not
2899+
# from ``data``'s own width (#2272). ``Schema_MPTCPUnknown`` always packs
2900+
# three octets of header -- ``kind``, ``length`` and the subtype/flags
2901+
# octet whose low nibble takes ``data[0]`` -- and then ``data[1:]`` as
2902+
# its body, so the option is never shorter than 3 octets -- the smallest
2903+
# Multipath TCP option :rfc:`8684` section 3 permits. The former
2904+
# ``2 + len(data)`` declared 2 for ``data=b''`` while still packing those
2905+
# three octets, and the segment could not be read back:
2906+
# ``MPTCPUnknown.data`` sizes itself ``pkt['length'] - 3``, which went
2907+
# negative and raised ``ProtocolError: Field data resolved to a negative
2908+
# length``. For every non-empty ``data`` the two expressions agree, so
2909+
# this only changes the empty case.
28982910
return Schema_MPTCPUnknown(
28992911
kind=cast('Enum_Option', Enum_Option.Multipath_TCP),
2900-
length=2 + len(data),
2912+
length=3 + len(data[1:]),
29012913
test={
29022914
'subtype': subtype.value,
29032915
'data': data[0] & 0x0F if data else 0,
Lines changed: 261 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,261 @@
1+
# -*- coding: utf-8 -*-
2+
"""A parsed Multipath TCP schema carries the same attributes as a built one, ``option`` included.
3+
4+
GitHub issue #2293. :meth:`_MPTCP.post_process
5+
<pcapkit.protocols.schema.transport.tcp._MPTCP.post_process>` set ``ret.option =
6+
Enum_Option.Multipath_TCP`` on the subtype schema it returns. No ``_make_mptcp_*`` maker, and
7+
not :meth:`TCP._make_mode_mp <pcapkit.protocols.transport.tcp.TCP._make_mode_mp>` either, made
8+
the matching assignment -- so the attribute existed on a schema unpacked from wire bytes and
9+
not on the same schema built in memory.
10+
11+
Why the assignment had nothing to be symmetric about
12+
----------------------------------------------------
13+
14+
``option`` was not a field. :class:`~pcapkit.protocols.schema.transport.tcp.MPTCP` declares
15+
exactly ``kind`` and ``length`` (#541) and every concrete subtype adds its own on top; none of
16+
them declares ``option``, under :data:`~typing.TYPE_CHECKING` or otherwise. So the assignment
17+
created an attribute out of nothing at unpack time, and nothing in ``pcapkit`` read it back:
18+
``kind``, a real :class:`~pcapkit.corekit.fields.enums.EnumField`, already carries
19+
:attr:`~pcapkit.const.tcp.option.Option.Multipath_TCP`, and it is ``kind`` that the schema
20+
packs.
21+
22+
That is the difference from ``subtype``, the other attribute ``post_process`` assigns, which
23+
*is* needed on both paths and stays. ``subtype`` cannot be a field -- every subtype schema
24+
already packs those 4 bits inside its own ``test``
25+
:class:`~pcapkit.corekit.fields.strings.BitField`, so a second field for them would either
26+
double-pack or need a derive-don't-pack field this library has no equivalent of -- and every
27+
``_read_mptcp_*`` reader reads it, which is why ``_make_mode_mp`` sets it for construction
28+
(#566, #579, pinned in
29+
:mod:`tests.protocols.transport.test_tcp_mptcp_subtype_unit`). ``option`` had neither
30+
property: no reader wanted it and no maker set it. Dropping it is therefore the symmetric
31+
fix, where adding a mirror assignment to ``_make_mode_mp`` would have propagated a value
32+
with no purpose to the construction path as well.
33+
34+
Why the defect is invisible to a round trip
35+
-------------------------------------------
36+
37+
Both schemas pack to the same octets, because ``option`` is not among the fields ``pack``
38+
walks. Only the attribute *sets* differ, so a byte-level round trip stays closed over the
39+
defect -- the blind spot
40+
:mod:`tests.protocols.test_option_roundtrip_unit`'s docstring names. The assertion here is
41+
accordingly on ``set(schema.to_dict())``, the attributes each path leaves behind, rather than
42+
on packed bytes; the packed bytes are asserted equal as well, to pin that dropping the
43+
assignment moved nothing on the wire.
44+
45+
Coverage
46+
--------
47+
48+
Three subtypes, each reached by parsing hand-built spec-correct octets and then rebuilt from
49+
the resulting data model through ``_make_mode_mp``: MP_CAPABLE and MP_FASTCLOSE for the
50+
registered makers, and an unknown subtype for the ``MPTCPUnknown`` fallback, since
51+
``post_process`` runs once per option regardless of which subtype schema it dispatched to.
52+
53+
MP_JOIN is deliberately absent. ``_make_mptcp_join`` picks between the three layouts of
54+
:rfc:`8684` section 3.2 by reading ``self._flags``, which a bare ``TCP.__new__(TCP)`` has
55+
not got, so reaching it would mean constructing a whole protocol instance and would test the
56+
flag dispatch rather than the attribute symmetry this module is about -- that dispatch is
57+
covered in :mod:`tests.protocols.transport.test_tcp_mptcp_join_flag_ordering_unit`.
58+
59+
:meth:`~TCPMPTCPOptionAttributeUnitTests.test_option_is_not_a_declared_field` is the positive
60+
half: it derives from the schema classes themselves that ``option`` never was a field, so the
61+
case for deleting the assignment rather than mirroring it does not rest on this docstring.
62+
63+
Every case builds its own octets in memory and reads no capture under
64+
:file:`examples/captures/`, so this belongs to the unit tier.
65+
66+
"""
67+
from __future__ import annotations
68+
69+
import unittest
70+
71+
#: Hand-built, spec-correct options, each starting at its ``kind`` octet and already a
72+
#: multiple of 4 octets long, keyed by the subtype they carry.
73+
#:
74+
#: * ``MP_CAPABLE`` -- :rfc:`8684` section 3.1 figure 4, the 12-octet form: ``1e`` kind, ``0c``
75+
#: length, ``01`` the subtype nibble ``0`` beside version ``1``, ``81`` the ``A``..``H``
76+
#: flags, then the sender's key.
77+
#: * ``MP_FASTCLOSE`` -- :rfc:`8684` section 3.5 figure 14: subtype nibble ``7`` with its 12
78+
#: reserved bits as ``0x70 0x00``, then the receiver's key.
79+
#: * ``unknown`` -- the 3-octet minimum of section 3 with subtype nibble ``0xf``
80+
#: (``Reserved_for_Private_Use``) and a data nibble of ``5``, padded with one
81+
#: End-of-Option-List octet.
82+
SPEC_OPTIONS = {
83+
'MP_CAPABLE': bytes([0x1E, 0x0C, 0x01, 0x81]) + b'\xAA' * 8,
84+
'MP_FASTCLOSE': bytes([0x1E, 0x0C, 0x70, 0x00]) + b'\xCC' * 8,
85+
'unknown': bytes([0x1E, 0x03, 0xF5, 0x00]),
86+
}
87+
88+
89+
def build_tcp_segment(option_octets: 'bytes') -> 'bytes':
90+
"""Pack a whole SYN segment carrying ``option_octets`` verbatim as its only option.
91+
92+
Goes through :class:`~pcapkit.protocols.schema.transport.tcp.TCP` (the schema, not the
93+
protocol) directly, so no ``_make_mptcp_*`` maker runs and the option bytes reach the
94+
wire exactly as given -- which is what makes the parsed side of each comparison below an
95+
independent starting point rather than something a maker produced.
96+
97+
Args:
98+
option_octets: The whole option, header octets included, already padded to a multiple
99+
of 4 octets.
100+
101+
Returns:
102+
The packed TCP segment.
103+
104+
Note:
105+
``pcapkit`` is imported here, inside the function, rather than at module level, for
106+
the reason
107+
:func:`tests.protocols.transport.test_tcp_mptcp_subtype_unit.build_mptcp_option`'s own
108+
docstring records: another module in this suite pops ``pcapkit``'s submodules out of
109+
:data:`sys.modules`, so a name bound at collection time can end up pointing at a
110+
schema class built from a stale ``Schema`` base.
111+
112+
"""
113+
from pcapkit.protocols.schema.transport.tcp import TCP as Schema_TCP
114+
115+
if len(option_octets) % 4:
116+
raise ValueError('option_octets must already be a multiple of 4 octets long')
117+
118+
schema = Schema_TCP(
119+
srcport=50000, dstport=80, seq=1, ack=0,
120+
offset={'offset': 5 + len(option_octets) // 4, 'reserved': 0, 'ns': 0},
121+
flags={'cwr': 0, 'ece': 0, 'urg': 0, 'ack': 0, 'psh': 0, 'rst': 0, 'syn': 1, 'fin': 0},
122+
window=8192, checksum=b'\x00\x00', urgent=0,
123+
options=option_octets,
124+
payload=b'',
125+
)
126+
return schema.pack()
127+
128+
129+
def parse_and_rebuild(option_octets: 'bytes') -> 'tuple[object, object]':
130+
"""Parse one MPTCP option off the wire, then rebuild it from the data model that produced.
131+
132+
Args:
133+
option_octets: The whole option, as :func:`build_tcp_segment` wants it.
134+
135+
Returns:
136+
The schema :meth:`_MPTCP.post_process
137+
<pcapkit.protocols.schema.transport.tcp._MPTCP.post_process>` returned for the parsed
138+
option, and the schema :meth:`TCP._make_mode_mp
139+
<pcapkit.protocols.transport.tcp.TCP._make_mode_mp>` builds from the parsed option's
140+
data model -- the two paths #2293 is about, starting from the same option.
141+
142+
"""
143+
from pcapkit.const.tcp.option import Option as Enum_Option
144+
from pcapkit.protocols.transport.tcp import TCP
145+
146+
raw = build_tcp_segment(option_octets)
147+
protocol = TCP(raw, len(raw))
148+
149+
parsed = protocol.schema.options[0]
150+
built = TCP.__new__(TCP)._make_mode_mp( # pylint: disable=protected-access
151+
Enum_Option.Multipath_TCP, protocol.info.options[Enum_Option.Multipath_TCP])
152+
return parsed, built
153+
154+
155+
class TCPMPTCPOptionAttributeUnitTests(unittest.TestCase):
156+
"""Parsing and building an MPTCP option leave the same attributes on the schema."""
157+
158+
def test_parsed_and_built_schemas_carry_the_same_attributes(self) -> None:
159+
"""``set(schema.to_dict())`` agrees between the parse path and the build path.
160+
161+
The assertion #2293 reports: on ``main`` the parsed side carries a surplus
162+
``option``, so this reads ``{'option'}`` rather than the empty set.
163+
164+
"""
165+
for name, octets in SPEC_OPTIONS.items():
166+
with self.subTest(subtype=name):
167+
parsed, built = parse_and_rebuild(octets)
168+
169+
self.assertEqual(set(parsed.to_dict()), set(built.to_dict()),
170+
'a parsed schema and a built schema must have the same keys')
171+
172+
def test_parsed_schema_has_no_option_attribute(self) -> None:
173+
"""The surplus attribute itself is gone, named rather than inferred from a set diff.
174+
175+
Pinned separately so a regression says *which* attribute came back, and so that
176+
making ``option`` symmetric by also setting it on the construction path -- which
177+
would satisfy the test above -- does not pass here.
178+
179+
"""
180+
for name, octets in SPEC_OPTIONS.items():
181+
with self.subTest(subtype=name):
182+
parsed, built = parse_and_rebuild(octets)
183+
184+
self.assertFalse(hasattr(parsed, 'option'))
185+
self.assertFalse(hasattr(built, 'option'))
186+
187+
def test_kind_still_carries_multipath_tcp(self) -> None:
188+
"""Dropping ``option`` loses no value: ``kind`` is where the option number lives.
189+
190+
``kind`` is a real field on :class:`~pcapkit.protocols.schema.transport.tcp.MPTCP`, so
191+
it is both set on every path and actually packed, which is the whole reason ``option``
192+
was redundant rather than merely asymmetric.
193+
194+
"""
195+
from pcapkit.const.tcp.option import Option as Enum_Option
196+
197+
for name, octets in SPEC_OPTIONS.items():
198+
with self.subTest(subtype=name):
199+
parsed, built = parse_and_rebuild(octets)
200+
201+
self.assertEqual(parsed.kind, Enum_Option.Multipath_TCP)
202+
self.assertEqual(built.kind, Enum_Option.Multipath_TCP)
203+
204+
def test_subtype_is_still_set_on_both_paths(self) -> None:
205+
"""``subtype``, the assignment that stays, is unaffected.
206+
207+
``post_process`` assigns both ``option`` and ``subtype``; only the first was surplus
208+
(#566, #579). Guards against a fix that reached for the whole block.
209+
210+
"""
211+
from pcapkit.const.tcp.mp_tcp_option import MPTCPOption as Enum_MPTCPOption
212+
213+
expected = {
214+
'MP_CAPABLE': Enum_MPTCPOption.MP_CAPABLE,
215+
'MP_FASTCLOSE': Enum_MPTCPOption.MP_FASTCLOSE,
216+
'unknown': Enum_MPTCPOption.Reserved_for_Private_Use,
217+
}
218+
for name, octets in SPEC_OPTIONS.items():
219+
with self.subTest(subtype=name):
220+
parsed, built = parse_and_rebuild(octets)
221+
222+
self.assertEqual(parsed.subtype, expected[name])
223+
self.assertEqual(built.subtype, expected[name])
224+
225+
def test_packed_octets_are_unchanged(self) -> None:
226+
"""Both schemas still pack the octets they were parsed from.
227+
228+
``option`` was never packed, so removing it must move nothing on the wire. This is
229+
also what makes the attribute-set assertion above necessary: a byte-level round trip
230+
passes either way.
231+
232+
"""
233+
for name, octets in SPEC_OPTIONS.items():
234+
with self.subTest(subtype=name):
235+
parsed, built = parse_and_rebuild(octets)
236+
expected = octets.rstrip(b'\x00') if name == 'unknown' else octets
237+
238+
self.assertEqual(parsed.pack(), expected)
239+
self.assertEqual(built.pack(), expected)
240+
241+
def test_option_is_not_a_declared_field(self) -> None:
242+
"""No MPTCP schema declares an ``option`` field, so nothing could have depended on it.
243+
244+
Derived from the schema classes the library ships rather than from a list written
245+
here, so the claim is rechecked whenever a subtype gains a field.
246+
247+
"""
248+
from pcapkit.protocols.schema.transport.tcp import MPTCP
249+
250+
schemas = [MPTCP, *MPTCP.__enum__.values(),
251+
MPTCP.__enum__.default_factory()] # the ``MPTCPUnknown`` fallback
252+
self.assertGreater(len(schemas), 1, 'the subtype registry must not be empty')
253+
254+
for schema in schemas:
255+
with self.subTest(schema=schema.__name__):
256+
self.assertNotIn('option', schema.__fields__)
257+
self.assertIn('kind', schema.__fields__)
258+
259+
260+
if __name__ == '__main__':
261+
unittest.main()

0 commit comments

Comments
 (0)