fix(tcp): correct MPTCP option length arithmetic at all six #576 sites - #585
Conversation
|
Flagging the one red in CI so it is not misread as a broken fix. The only failing test in the whole suite is a stale expected-failure record, not a defect in this change: That is the suite working as designed — its own docstring says "a case present in it must still fail ... so that fixing the defect turns this red and the entry gets deleted rather than left behind". MP_FASTCLOSE was in The required change is deleting the Two incidental notes on that entry's recorded text, for whoever removes it: it cites RFC 8684 §3.7 for MP_FASTCLOSE, but §3.7 is Fallback (MP_FAIL) — Fast Close is §3.5, figure 14. The same mis-citation is in #576's body. And its line references ( |
Re-derived every length from RFC 8684 rather than from the issue body, which mis-cites one section and files one site against the wrong line. - MP_FASTCLOSE (s3.5 fig 14, 12 octets): the schema had no reserved field, so it packed 11 against a declared 12, and `_read_mptcp_fastclose` required 16. Added the reserved octet and corrected the guard to 12; the maker was already right. - MP_JOIN-SYN/ACK (s3.2 fig 6, 16 octets): `_make_join_synack` wrote 12, the SYN form's length, and `_read_join_synack` required 20, contradicting its own docstring. Both now 16. The `opt` branch also never assigned `hmac`, so reconstruction substituted `bytes(8)` for the parsed HMAC. - MP_JOIN-ACK (s3.2 fig 7, 24 octets): `_make_join_ack` wrote 8. Its reader already required 24, so nothing it produced could be parsed back. - REMOVE_ADDR (s3.4.2 fig 13, `3 + n`): length was a constant 4, right for exactly the one-ID case the fixture happens to use. - MP_PRIO (s3.3.8 fig 11, 3 octets): length was a constant 4, which satisfied `MPTCPPriority.addr_id`'s own `length == 4` predicate and so packed a phantom all-zero Address ID. The `opt` branch also dropped `backup`. - DSS (s3.3 fig 9): the maker's length expression is RFC-correct and unchanged; the defect is the schema field widths #576's own split names, `MPTCPDSS.ack` and `.dsn` packing 0 octets rather than 4 when unextended. Both now select `UInt32Field`/`UInt64Field` through a `SwitchField`, because `NumberField(length=<callable>)` cannot pack at all. MP_JOIN-SYN is not a defect: figure 5 gives 12 and all three sites agree. Deletes the now-stale `tcp-mptcp/MP_FASTCLOSE` entry from EXPECTED_FAILURES, which this fix makes pass; `tcp-mptcp/MP_JOIN` stays, failing for an unrelated `self._flags` ordering defect. Adds the two new selectors to the docs. New per-site tests in test_tcp_mptcp_length_arithmetic_unit.py, each failing on the unfixed tree. Coverage of the two changed modules 99% -> 100%.
708d98d to
86d225c
Compare
JarryShaw
changed the title
|
Superseding my earlier comment: the
Rebased onto current |
|
✅ GOOD TO GO at head sha |
|
Reviewer: Sonnet; PR authored on Opus 5. Falsify-not-bless pass on PR #585 ( 1. RFC 8684 re-derivation, from
|
…ce from the placeholder (#591) A `NumberField` whose `length` was a callable could not pack or parse at any width `struct` has a native integer code for. `length` is a placeholder of `-1` until `__call__` resolves the callable, `-1` has no native code, and `build_template` raised `_need_process` for it and never put it back -- so the flag was a latch. Resolving the real width rebuilt the template and left the latch set, and `pre_process` then handed bytes to a template that had become `>Q`, raising `struct.error: required argument is not an integer`. - `build_template` now *assigns* `_need_process` rather than only ever raising it, so the flag always describes the length that template was built for. That is what tells a placeholder apart from a width that genuinely needs byte packing without tracking that a placeholder was ever in play: the answer for `-1` is True, the answer for `8` is False, and the width in force decides. A callable resolving to 3 still takes the fall-through branch and still gets True, so clearing the flag unconditionally -- which would have been the one-line fix -- is not what happens here. - All four native widths were affected, not only the 8 that #591 reproduces. The latch has nothing to do with the width it latches into, so 1, 2 and 4 failed identically. Measured on `NumberField` and on `EnumField`, both of which leave `__template__` unset; the eight subclasses that fix `__template__` never latched anything and are unchanged. - Parsing was broken in the mirror direction and is fixed with it: `post_process` called `int.from_bytes` on the integer `struct.unpack` had already produced from a `>Q` template. - `pre_process` consults the flag *after* the `_length < 0` repair rebuilds the template rather than before it. That repair can land on a native width, and deciding first and rebuilding second is how the template and the returned value came to disagree in the first place. This is what made every extended 8-octet MPTCP DSS form unbuildable, since those widths are chosen at runtime from the DSS flags and so must come from a callable. #585 worked around it in the TCP schema alone, leaving every other caller exposed; that workaround is left in place, because its `NoValueField` branch for an absent field is load-bearing independently of this defect. New tests in tests/corekit/test_fields_numbers_callable_length.py proven to fail without the fix: 21 failures and 8 errors across 7 of 10 tests before, all 10 passing after. The 3 that pass either way are the guards against over-correcting -- the byte-packed widths, the unresolved placeholder, and the `__template__` subclasses. tests/corekit/ 134 passed, 195 subtests, and the MPTCP length-arithmetic suite 24 passed, 5 subtests. No new mypy finding. Fixes #591
…ce from the placeholder (#591) (#598) A `NumberField` whose `length` was a callable could not pack or parse at any width `struct` has a native integer code for. `length` is a placeholder of `-1` until `__call__` resolves the callable, `-1` has no native code, and `build_template` raised `_need_process` for it and never put it back -- so the flag was a latch. Resolving the real width rebuilt the template and left the latch set, and `pre_process` then handed bytes to a template that had become `>Q`, raising `struct.error: required argument is not an integer`. - `build_template` now *assigns* `_need_process` rather than only ever raising it, so the flag always describes the length that template was built for. That is what tells a placeholder apart from a width that genuinely needs byte packing without tracking that a placeholder was ever in play: the answer for `-1` is True, the answer for `8` is False, and the width in force decides. A callable resolving to 3 still takes the fall-through branch and still gets True, so clearing the flag unconditionally -- which would have been the one-line fix -- is not what happens here. - All four native widths were affected, not only the 8 that #591 reproduces. The latch has nothing to do with the width it latches into, so 1, 2 and 4 failed identically. Measured on `NumberField` and on `EnumField`, both of which leave `__template__` unset; the eight subclasses that fix `__template__` never latched anything and are unchanged. - Parsing was broken in the mirror direction and is fixed with it: `post_process` called `int.from_bytes` on the integer `struct.unpack` had already produced from a `>Q` template. - `pre_process` consults the flag *after* the `_length < 0` repair rebuilds the template rather than before it. That repair can land on a native width, and deciding first and rebuilding second is how the template and the returned value came to disagree in the first place. This is what made every extended 8-octet MPTCP DSS form unbuildable, since those widths are chosen at runtime from the DSS flags and so must come from a callable. #585 worked around it in the TCP schema alone, leaving every other caller exposed; that workaround is left in place, because its `NoValueField` branch for an absent field is load-bearing independently of this defect. New tests in tests/corekit/test_fields_numbers_callable_length.py proven to fail without the fix: 21 failures and 8 errors across 7 of 10 tests before, all 10 passing after. The 3 that pass either way are the guards against over-correcting -- the byte-packed widths, the unresolved placeholder, and the `__template__` subclasses. tests/corekit/ 134 passed, 195 subtests, and the MPTCP length-arithmetic suite 24 passed, 5 subtests. No new mypy finding. Fixes #591
1 participant
Fixes #576
Re-derived all six lengths from RFC 8684 directly, rather than from the issue body or from
the correcting comment. Two of the issue's own claims did not survive that: one site is not a
defect where the issue puts it, and one section citation is wrong. Both are called out below.
The six sites
ack/dsnpacked 0 octets, not 4All six are genuine defects. One — DSS — is not a defect at the line the issue's item 6 names;
see below. Separately, MP_JOIN-SYN, which the issue's body groups with the other two MP_JOIN
forms and its comment exonerates, is confirmed correct and is not changed.
The arithmetic
Every length is the sum of the octets the named figure draws. The fixed head is 2 octets of
Kind/Lengthplus the subtype row — 1 octet where the subtype's 4 bits are followed by 4bits of flags or reserved, 2 octets where they are followed by 12 reserved bits.
Kind1 +Length1 + subtype-and-12-reserved-bits 2 +receiver's key 8 = 12. Three sites disagreed: the maker declared the correct 12, but
MPTCPFastclosehad no reserved field at all and so packed 11, and_read_mptcp_fastcloserequired 16 — a number the RFC never produces for this option.Net effect: constructing an MP_FASTCLOSE raised
ProtocolError: TCP: [OptNo 30] invalid format, the maker's correct length failing the parser's wrong check.Note: §3.5, not §3.7. §3.7 is Fallback (MP_FAIL). The issue body says 3.7, and so does
the
EXPECTED_FAILURESentry it left behind.Kind1 +Length1 + subtype/rsv/B1 +Address ID1 + truncated HMAC 8 + random number 4 = 16. The maker wrote 12, which is
_make_join_syn's own correct length for figure 5's SYN form — that one carries a4-octet token where this carries an 8-octet HMAC.
_read_join_synackindependentlyrequired 20, contradicting its own docstring figure, which the issue does not mention:
fixing only the maker would have left the form unusable.
The
if opt is not None:branch also setnoncetwice and never sethmac, soreconstructing a parsed option substituted the
bytes(8)default for the HMAC that was onthe wire.
Kind1 +Length1 + subtype-and-12-reserved-bits 2 + thefull 160-bit HMAC 20 = 24. The maker wrote 8.
_read_join_ackalready required 24, sonothing the maker produced could be parsed back at all.
Length = 3 + noutright — head 3 + one octetper Address ID. The constant 4 is right for exactly one list length, and
examples/generators/options.pypassesaddr_id=[1], which is why the round-trip suitenever saw it.
MPTCPRemoveAddress.addr_idsizes its list aspkt['length'] - 3, so theconstant mis-sized the parse as well as the pack.
document "specifies the removal of the AddrID field [RFC6824] in the MP_PRIO option",
closing a theoretical attack in which a subflow could be forced into backup mode. So
3, with RFC 6824's 4-octet form still accepted as legacy, hence
length=3 if addr_id is None else 4rather than a flat 3. The old constant 4 satisfiedMPTCPPriority.addr_id's ownpkt['length'] == 4predicate, so declaring the legacylength created the legacy field:
addr_id=Nonepacked1e045000, a phantom all-zeroAddress ID. The
optbranch also droppedbackup, which is the option's entire payload.Site 6 (DSS) is a real defect, but not at the line the issue points to
To be explicit about scope, since DSS is the one site where the fix is not a changed
length=:the field widths are what #576 asks for. Its own "Suggested split" says "DSS needs its
schema's
ack/dsnfield-length lambdas corrected to 4/8 (not 0/8)", and its item 6 is titled"declared length does not match what the schema actually packs". That is exactly and only what
is changed here. Nothing outside #576 is touched.
What did not survive re-derivation is the rest of that sentence — "which will also change what
its maker needs to compute" — and item 6's primary target,
_make_mptcp_dss's length expressionat
tcp.py:2873. Re-derived from figure 9, that expression is already correct and is leftuntouched (with a comment saying so, so a later pass does not "fix" it). Read as a base plus
widening increments rather than one term per field:
4 is the head;
Acontributes the 4-octet Data ACK andaa further 4 to widen it to 8;Mcontributes 12 (DSN 4 + SSN 4 + Data-Level Length 2 + Checksum 2) and
ma further 4 to widenthe DSN to 8. All flags set gives 4 + 4 + 4 + 12 + 4 = 28, which is the maximum §3.3 states
independently in prose.
What was wrong is the schema that expression describes.
MPTCPDSS.ackand.dsnreadNumberField(length=lambda pkt: 8 if pkt['flags']['a'] else 0)— 0 octets where the figuresays 4 — so the option went onto the wire 4 or 8 octets shorter than it declared and the
ack/dsnvalues were not present at all.A second defect sat behind the first. Correcting the lambda to
8 if ... else 4wouldstill not pack:
NumberFieldcallsbuild_templateonce at__init__with the placeholderlength
-1, which latches_need_process = True, and nothing clears it when__call__laterresolves the real length and rebuilds the template as
>I/>Q.pre_processthen handsstruct.packbytes for an integer template. Measured on the 8-octet form the old lambda didreach:
So every extended (8-octet) DSS form was unpackable outright, not merely mislabelled. That
belongs to
pcapkit/corekit/fields/numbers.pyand is not fixed here — the schema insteadselects between
UInt32FieldandUInt64Fieldthrough aSwitchField, both of which fix__template__at class level, following themptcp_add_address_selectorpattern already inthat module.
MP_JOIN-SYN is not a defect either
The issue body groups the three MP_JOIN forms and the comment on the issue corrects the
attribution. Confirmed independently: §3.2 fig 5 gives
Length = 12— head 4 + token 4 +random number 4 —
_make_join_synwrites 12,_read_join_synguards!= 12. All three agree.TCPMPTCPJoinSYNIsCorrectUnitTestspins that so a later pass reading the body alone does not"correct" it.
Tests
New module
tests/protocols/transport/test_tcp_mptcp_length_arithmetic_unit.py, one class persite so a regression names the option it broke. Each asserts the byte-exact packed option,
not only its length octet — declared and packed length disagreeing is the whole defect, so both
are checked. Where the reader guard was also wrong (sites 1 and 2) a second test splices
hand-built, spec-correct octets into a real TCP segment and parses them back through
TCPproper, so a pack-side and a parse-side bug cannot cancel out behind a closed round trip.
Every test was run against the unfixed tree first. Verbatim,
coverage run -m pytest -v:The four pre-fix passes are deliberate: the two
JoinSYNIsCorrecttests pin a non-defect, andtest_explicit_address_id_keeps_the_legacy_four_octet_formguards against over-correctingMP_PRIO to a flat 3.
test_length_tracks_the_number_of_address_idsprintsPASSEDwhile fourof its subtests fail — pytest 9.1.1 here has no
pytest-subtests, so a failing subtest does notfail its parent;
test_two_address_ids_pack_five_octetsexists so that site has a test thatfails outright, and the run's exit code is 1 either way.
Post-fix, same module:
Whole transport suite:
Coverage of the two changed files,
coverage run -m pytestovertests/protocols/transport/(neverpytest-cov):pcapkit/protocols/schema/transport/tcp.pypcapkit/protocols/transport/tcp.pypylintwith the project's ownMakefileflags, message set diffed againstmain: onewarning removed (
W0109: Duplicate key 'A' in dictionary, the duplicate'A': flag_Atheissue notes in the DSS flags literal), none added.
Two tests that pinned the old constants, now updated
tests/protocols/transport/test_tcp_udp_unit.pyasserted_read_join_synackaccepts 20 and_read_mptcp_fastcloseaccepts 16 — the wrong guards themselves. Both now assert the RFCvalue and reject the old one, the shape TCP._make_mptcp_capable writes length 20/32 where RFC 8684 gives 12/20, and now packs wrong bytes rather than crashing #567's note in the same test already uses. Its
REMOVE_ADDR case also carried
length=4with two Address IDs, corrected to 5.tests/protocols/transport/test_tcp_mptcp_subtype_unit.py'stest_mp_fastclose_still_fails_but_no_longer_on_subtypewas fix(tcp): set MPTCP.subtype on construction, fix MP_CAPABLE's length/rkey (#566, #567) #579's explicit placeholder forthis fix ("it still cannot be built through
TCP()-- _make_mptcp_*/_read_mptcp_* length arithmetic is wrong at six more sites beyond MP_CAPABLE #576, not this issue"). It now assertsthe construction succeeds and reports its subtype.
Also in this PR: the
EXPECTED_FAILURESentry this fix makes staleEXPECTED_FAILURES['tcp-mptcp/MP_FASTCLOSE']is deleted, intests/protocols/test_option_roundtrip_unit.py— a file outside the six sites, hence thisnote. The entry existed because of #576, so it is stale precisely as a consequence of this
fix, and the suite is built to say so: "a case present in it must still fail ... so that fixing
the defect turns this red and the entry gets deleted rather than left behind". Removing it on
mainseparately would turnmainred until this merged, so it belongs here.tests/protocols/test_option_roundtrip_unit.pyalone:1 failed, 6 passed, 357 subtests passed6 passed, 358 subtests passedThe subtest count rises by one because
tcp-mptcp/MP_FASTCLOSEmoves from a recorded gap to apassing round-trip case.
tcp-mptcp/MP_JOINis kept: it fails for an unrelated reason (below), not for anything#576 describes. The narrative comment above both entries is updated to record why MP_FASTCLOSE
left the table, and to note that REMOVE_ADDR, MP_PRIO and DSS read
'OK'in this suitethroughout without being correct — it only checks the cycle is self-consistent, which a wrong
length can be, which is why #576's coverage is per option against RFC 8684 instead.
Deliberately not fixed
NumberField(length=<callable>)cannot pack — latched_need_process, detailed above.pcapkit/corekit/fields/numbers.py, out of scope here; worked around in the schema.tcp-mptcp/MP_JOINstill cannot be constructed throughTCP()._make_mptcp_joindispatches on
self._flags, whichTCP._makeassigns after it has already built theoptions (
pcapkit/protocols/transport/tcp.py: options at :547,_flagsat :567), soconstruction raises
AttributeError: 'TCP' object has no attribute '_flags'. Already recordedin
EXPECTED_FAILURESwith that exact diagnosis, unchanged by this work; it is why theMP_JOIN tests here drive the makers directly and reach the readers by parsing bytes.
required", so
n = 0is not a described form, but_read_mptcp_removepermitslength >= 3.Tightening that rejects input the library previously accepted, which is beyond this change;
noted in the reader's docstring.