fix(tcp): resolve the connection flags before building the options (#587) - #597
Conversation
|
✅ GOOD TO MERGE — head |
Cross-review appendix — PR #597Reviewer: Sonnet; PR authored on Opus 5. Reviewed at head
|
) Building any MP_JOIN option raised `AttributeError: 'TCP' object has no attribute '_flags'`. `TCP.make` built the options at tcp.py:547 and assigned `self._flags` only at tcp.py:567, but `_make_mptcp_join` (tcp.py:2780-2786) branches on that attribute to choose between RFC 8684 s3.2's three MP_JOIN layouts: figure 5 for SYN at 12 octets, figure 6 for SYN/ACK at 16, figure 7 for ACK at 24. The parse path was always fine -- `read` assigns at tcp.py:485, before parsing options at tcp.py:494, so the identical branches at tcp.py:1526-1532 always had them. - Hoist the `flags` dict and the `_flag` accumulation above the `_make_tcp_options` call. Only the data-offset computation stays below it, needing the options' `total_length`. Not a two-line move: `_flag` derives from the `flags` dict, so the dict has to travel with it. - Not fixed with a zero default. `Protocol.pack` is public and calls `make`, so an instance that had parsed already had `_flags` set and silently built the option for the segment it had *read*. Measured: a parsed MP_JOIN-SYN instance asked to pack an MP_JOIN-ACK segment emitted an ACK header carrying figure 5's 12-octet SYN option, the caller's 20-octet HMAC replaced by an all-zero token and nonce, with nothing raised. A default fixes only the crash. - Seed the accumulator with `Enum_Flags(0)` rather than `cast('Enum_Flags', 0)`. The hoist made the no-SYN-no-ACK branch reachable, and `typing.cast` being a runtime no-op left `_flags` a plain `int` on which the first membership test raised `TypeError` instead of the documented `ProtocolError`. The read path's seed is left alone: `mptcp_data_selector` rejects a flagless MP_JOIN before `_read_mptcp_join` runs. - New test_tcp_mptcp_join_flag_ordering_unit.py, 17 cases: all three layouts through the public constructor, the pack/parse cycle per layout, the stale-flags case, the statement order, and parse-path and other-option controls. On the unfixed tree it reports `12 failed, 9 passed, 9 subtests passed` (exit 1); with the fix, `17 passed, 16 subtests passed` (exit 0). - Delete the now-stale `tcp-mptcp/MP_JOIN` EXPECTED_FAILURES entry and lift the MP_JOIN exclusion in test_tcp_mptcp_subtype_unit.py. tests/protocols/ 615 passed / 1469 subtests, exit 0. Coverage of the two TCP modules was already 100% pre-fix while this path was wholly broken, because the existing cases assign a Python `set` to `_flags` on a bare `TCP.__new__(TCP)`.
af5d723 to
14c400e
Compare
|
Cross-review (Sonnet, authored on Opus 5): GOOD TO GO as of |
|
Re-pointing my verdict — PR #597 My 23:24:12Z comment (
On the other comment on this PR (23:26:29Z, "Cross-review (Sonnet, authored on Opus 5): GOOD TO GO as of ✅ GOOD TO MERGE — head |
…t a no-op cast
* `TCP.read` seeded `_flag` with `cast('Enum_Flags', 0)`. `typing.cast` is a
runtime no-op, so a segment whose flags octet is all zero left `self._flags`
a plain `int` -- the `|=` below is the only thing that promotes it, and it
never runs. A membership test then raised `TypeError: argument of type 'int'
is not a container or iterable`, and `TCP.connection` returned an `int` where
it and `Data_TCP.connection` both annotate `Flags`. Seed `Enum_Flags(0)`,
matching what #597 did to the sibling accumulator in `make`.
* Correct the prose that recorded the read path as deliberately keeping its
`cast`: the NOTE in `make`, one NOTE in `test_tcp_udp_unit.py`, and two
docstrings in `test_tcp_mptcp_join_flag_ordering_unit.py`.
* Add `test_a_flagless_segment_seeds_its_connection_flags_as_an_enum`, four
subtests over the flags octet, asserting the *type* -- `0 == Enum_Flags(0)`,
so equality cannot discriminate -- plus that `_read_mptcp_join` now reaches
its own `ProtocolError` rather than a bare `TypeError`, plus the one
observable difference: a flagless segment's `connection` dumps as
`'Flags::None [0]'` where it dumped as `0`. That removes a type inconsistency
(number when flagless, string otherwise); the literal `None` is a separate
rendering defect in `pcapkit.dumpkit.common` and is left to its own change.
Coverage cannot see the fix: the line already executed, and `tcp.py` reads
100%/100% either side. Subtests over `tests/protocols/transport/` go 99 -> 103.
Verified against the unfixed snapshot: the new test fails `AssertionError:
<class 'int'> is not <aenum 'Flags'>`, exit 1; with the fix, exit 0. No
caller-visible parse change -- a flagless MP_JOIN still stops at
`mptcp_data_selector`'s `FieldError`, measured identical both sides.
Fixes #616
…t a no-op cast
* `TCP.read` seeded `_flag` with `cast('Enum_Flags', 0)`. `typing.cast` is a
runtime no-op, so a segment whose flags octet is all zero left `self._flags`
a plain `int` -- the `|=` below is the only thing that promotes it, and it
never runs. A membership test then raised `TypeError: argument of type 'int'
is not a container or iterable`, and `TCP.connection` returned an `int` where
it and `Data_TCP.connection` both annotate `Flags`. Seed `Enum_Flags(0)`,
matching what #597 did to the sibling accumulator in `make`.
* Correct the prose that recorded the read path as deliberately keeping its
`cast`: the NOTE in `make`, one NOTE in `test_tcp_udp_unit.py`, and two
docstrings in `test_tcp_mptcp_join_flag_ordering_unit.py`.
* Add `test_a_flagless_segment_seeds_its_connection_flags_as_an_enum`, four
subtests over the flags octet, asserting the *type* -- `0 == Enum_Flags(0)`,
so equality cannot discriminate -- plus that `_read_mptcp_join` now reaches
its own `ProtocolError` rather than a bare `TypeError`, plus the one
observable difference: a flagless segment's `connection` dumps as
`'Flags::None [0]'` where it dumped as `0`. That removes a type inconsistency
(number when flagless, string otherwise); the literal `None` is a separate
rendering defect in `pcapkit.dumpkit.common` and is left to its own change.
Coverage cannot see the fix: the line already executed, and `tcp.py` reads
100%/100% either side. Subtests over `tests/protocols/transport/` go 99 -> 103.
Verified against the unfixed snapshot: the new test fails `AssertionError:
<class 'int'> is not <aenum 'Flags'>`, exit 1; with the fix, exit 0. No
caller-visible parse change -- a flagless MP_JOIN still stops at
`mptcp_data_selector`'s `FieldError`, measured identical both sides.
Fixes #616
…t a no-op cast (#616) (#634) * `TCP.read` seeded `_flag` with `cast('Enum_Flags', 0)`. `typing.cast` is a runtime no-op, so a segment whose flags octet is all zero left `self._flags` a plain `int` -- the `|=` below is the only thing that promotes it, and it never runs. A membership test then raised `TypeError: argument of type 'int' is not a container or iterable`, and `TCP.connection` returned an `int` where it and `Data_TCP.connection` both annotate `Flags`. Seed `Enum_Flags(0)`, matching what #597 did to the sibling accumulator in `make`. * Correct the prose that recorded the read path as deliberately keeping its `cast`: the NOTE in `make`, one NOTE in `test_tcp_udp_unit.py`, and two docstrings in `test_tcp_mptcp_join_flag_ordering_unit.py`. * Add `test_a_flagless_segment_seeds_its_connection_flags_as_an_enum`, four subtests over the flags octet, asserting the *type* -- `0 == Enum_Flags(0)`, so equality cannot discriminate -- plus that `_read_mptcp_join` now reaches its own `ProtocolError` rather than a bare `TypeError`, plus the one observable difference: a flagless segment's `connection` dumps as `'Flags::None [0]'` where it dumped as `0`. That removes a type inconsistency (number when flagless, string otherwise); the literal `None` is a separate rendering defect in `pcapkit.dumpkit.common` and is left to its own change. Coverage cannot see the fix: the line already executed, and `tcp.py` reads 100%/100% either side. Subtests over `tests/protocols/transport/` go 99 -> 103. Verified against the unfixed snapshot: the new test fails `AssertionError: <class 'int'> is not <aenum 'Flags'>`, exit 1; with the fix, exit 0. No caller-visible parse change -- a flagless MP_JOIN still stops at `mptcp_data_selector`'s `FieldError`, measured identical both sides. Fixes #616
1 participant
Fixes #587
The defect
Constructing any MP_JOIN option raised
AttributeError: 'TCP' object has no attribute '_flags'.TCP.makebuilt the options first and assigned the flags afterwards. Line numbers re-located onorigin/main(fa6d18e31) — the issue's own citations for_make_mptcp_joinand_read_mptcp_joinpredate #585 and have since moved:origin/mainmakebuilds optionstcp.py:547tcp.py:547✓makeassignsself._flagstcp.py:567tcp.py:567✓_make_mptcp_joinbranchestcp.py:2780-2786tcp.py:2695-2699— movedreadassignsself._flagstcp.py:485tcp.py:485✓readparses optionstcp.py:494_read_mptcp_joinbranchestcp.py:1526-1532tcp.py:1521-1525— movedThe read path was never affected:
readassigns the flags attcp.py:485, before it parses the options attcp.py:494, so the identical branches attcp.py:1526-1532always had them. Onlymakewas inverted.Why the branch is right and the flags have to move
MP_JOIN is genuinely flag-dependent. RFC 8684 section 3.2 gives it three layouts, and they are three different lengths, so no single form can serve:
Kind/LengthB1,Address ID1, token 4, random number 4B1,Address ID1, truncated HMAC 8, random number 4The fix
Hoist the
flagsdict and the_flagaccumulation above the_make_tcp_optionscall. Theoffsetcomputation stays below it, being the one statement in that block that genuinely needs the options'total_length.#585's author called this "a two-line hoist". Checked, and it is not:
_flagis accumulated from theflagsdict, so the dict has to travel with it — a 16-line block move, plus one changed line. Hoisting onlyself._flags = _flagwould not compile. Nothing between the old and new sites readsself._flagsor depends on the option build, so the move is behaviour-preserving for every other option;TCPMakeOtherOptionsUnitTestsis the witness.It is deliberately not a zero initialisation
Protocol.packis public and callsmake, so an instance that has already parsed a segment can be asked to build a different one — and it already has_flagsset, so no default value would ever come into play for it. Pre-fix it silently built the option for the segment it had read. Measured onorigin/main: a parsed MP_JOIN-SYN instance asked to pack an MP_JOIN-ACK segment produced— header flag octet
0x10(ACK, as requested), option1e0c…, figure 5's 12-octet SYN form, with the caller's 20-octet HMAC (the entire authentication payload of figure 7, and the only reason to send that form) replaced by an all-zero phantom token and nonce. Nothing raised. Post-fix the same call emits1e18 10 00+ the 20 HMAC octets: figure 7, length 24.So the
AttributeErrorwas the benign symptom. A zero default fixes only the fresh-instance crash, leaves the silent corruption exactly as it was, and converts the flagless case into a spurious error.TCPMPTCPJoinStaleFlagsUnitTestspins this.A second, smaller fix in the same block
The hoist made one branch reachable for the first time: an MP_JOIN asked for with neither SYN nor ACK set.
_make_mptcp_joinends in aProtocolErrorfor exactly that case, but it could not be reached — the accumulator was seeded withcast('Enum_Flags', 0), andtyping.castis a runtime no-op, so with no flag setself._flagsstayed the plainint0and the first membership test raised instead:Seeding with
Enum_Flags(0)— a real flaglessaenum.IntFlagmember, which still compares equal to0and still ORs identically — lets the library's own documented error surface. A bareTypeErrorescaping the library would break the in-library exception contract, and it is newly reachable only because of this change, so it is fixed here rather than deferred.The read path was checked for the same shape of problem and does not have it:
mptcp_data_selectorguards the flagless case atschema/transport/tcp.py:204-212and raisesFieldErrorbefore_read_mptcp_joinis reached, so its own closingProtocolErrorstays unreachable. Itscastis deliberately left alone — changing it would alter theconnectionvalue reported for every flagless parsed segment, which is not what #587 is about. Both halves of that reasoning are asserted, so the decision cannot rot silently.Failing-then-passing evidence
New module, on the unfixed tree:
exit code 1. The representative failures, verbatim:
With the fix:
exit code 0.
The round-trip suite
EXPECTED_FAILUREScarried atcp-mptcp/MP_JOINentry that existed because of this defect, so it goes stale here. All three states measured (tests/protocols/test_option_roundtrip_unit.py):6 passed, 358 subtests passed1 failed, 6 passed, 357 subtests passed6 passed, 358 subtests passedThe middle row is the suite's own contract working as designed — it failed with
'OK' != 'CONSTRUCT'and the message "If the defect is fixed, delete its EXPECTED_FAILURES entry." That one entry is deleted and no other is touched; the table goes from 46 entries to 45, and no MPTCP entry remains.A finding worth recording
Statement and branch coverage of both TCP modules was already 100% on
origin/mainwith the pristine tests, while the public MP_JOIN constructor was broken for every caller. Measured, pristine tree and pristine tests:The reason is that the pre-existing cases reach
_make_mptcp_joinby assigning a Pythonsetto_flagson a bareTCP.__new__(TCP)(test_tcp_udp_unit.py:828-862) —Flags.SYN in {Flags.SYN}is true, so every branch executes, while both the statement ordering and the accumulator's real type are bypassed. 100% coverage of a path no caller can reach the way a caller reaches it. That is what the new module covers: everything in it goes through the public constructor or throughpackon a parsed instance.Testing
tests/protocols/transport/test_tcp_mptcp_join_flag_ordering_unit.py— new, 17 cases in six classes: the three layouts through the public constructor with their RFC octets, the construct/pack/parse cycle per layout, the stale-flags case, the statement order itself (so a future refactor cannot silently re-invert it), and controls for the parse path and for flag-independent options.test_tcp_mptcp_subtype_unit.py— the MP_JOIN exclusion its docstring recorded is lifted, and thesubtypeassertion it could not make for MP_JOIN is added, once per layout.build_mptcp_optiongains an optionalheader=override, defaulting to the previous behaviour.test_tcp_mptcp_length_arithmetic_unit.py(_make_mptcp_*/_read_mptcp_* length arithmetic is wrong at six more sites beyond MP_CAPABLE #576/fix(tcp): correct MPTCP option length arithmetic at all six #576 sites #585) — untouched and still passing.tests/protocols/—615 passed, 1469 subtests passed, exit 0.tests/project/,tests/test_docstring_contract.py,tests/test_tier_guard.py—138 passed, 882 subtests passed, exit 0.tests/integration/—90 passed, 2 skipped, 115 subtests passed, exit 0, after regenerating the captures withexamples/generators/make_samples.pyagainst the fixed tree.Every run was scoped with
coverage run --include='pcapkit/protocols/transport/tcp.py,pcapkit/protocols/schema/transport/tcp.py'and a narrowed test path, and exit codes were read from a file rather than from the summary line, since pytest 9.1.1 prints PASSED for the parent of a failing subtest.Changelog
A
**Fixed**and an**Added**bullet indocs/source/changelog/1.5.0.rst;CHANGELOG.mdregenerated withpython util/changelog_md.py, andpython util/changelog_md.py --checkexits 0.Found but deliberately not fixed
test_tcp_udp_unit.pyassigns asetto_flagsat lines 828, 834, 840, 845, 937, 961, 977, 990 (and 490-526), where production code produces anaenum.IntFlag. The assertions pass either way, but they are asserting against a value shape the library never emits, which is exactly why neither defect here was caught despite 100% branch coverage.proto._flags = Flags.SYNandFlags(0)would be behaviour-identical; that is a test-fidelity change with its own blast radius and belongs in its own PR.examples/generators/options.py'sTCP_BASEpasses keywordsTCP.makedoes not declare.'seq': 1and'ack_flag': Falseare both swallowed by**kwargs, while'ack': 0binds toack, the acknowledgement flag, not toack_no. Measured:TCP(**TCP_BASE).info.seqis0, not the1the mapping reads as. Harmless to what those cases assert, and changing it would churn every generated fixture, so it is only documented — the new module defines its ownTCP_HEADERwithmake's real parameter names and says why._read_mptcp_join's closingProtocolError(tcp.py:1532) is unreachable, becausemptcp_data_selectorrejects a flagless MP_JOIN first. Left as it is; the reasoning is now recorded in code and pinned by a test rather than being rediscovered.Cross-review
Reviewed independently on a different model (Sonnet; this change was authored on Opus 5), briefed to falsify rather than confirm. Verdict: NEEDS CHANGES, one substantive finding, now addressed — so GOOD TO GO as of
14c400e65.It reproduced C1, C3 and C10 independently rather than taking them on trust, by extracting a pristine
origin/mainwithgit archiveinto a scratch directory instead of mutating the worktree. Its pre-fix stale-flags measurement came back as option bytes1e0c10000000000000000000— figure 5, 12 octets, zeroed token and nonce, HMAC dropped — matching the byte string quoted above, and its independently re-measured pre-fix coverage came back as exactly594 0 198 0 100%and219 0 20 0 100%, matching the numbers quoted above.The finding it raised, and what changed.
test_packing_replaces_the_parsed_flagspassed on the unfixed tree as well, so it was not a discriminator, while its docstring — "the mechanism behind the case above, asserted directly" — implied it was. That is a fair hit and the docstring was overclaiming.It offered two remedies; the first does not work, and that is worth recording. Passing a real
options=argument would not make the test discriminate: pre-fix,makeassignedself._flagsafter building the options, so by the timepackreturned the attribute held the new call's flags on either tree, and only the option octets built in between were wrong. The assertion that separates the trees is therefore on those octets, whichtest_parsed_instance_packs_the_requested_layoutalready owns. So the second remedy was taken — the case is renamedtest_make_re_derives_the_flags_on_every_call, its docstring now states in bold that it is not a regression guard for #587 and passed pre-fix, explains the property the fix leans on that it does pin, and records why addingoptions=would not change that. A third flag combination was added so the assertion cannot be satisfied by a one-off rather than by a per-call derivation.Also fixed from the same review: a prose miscount, "the other seven flags" where the
flagsdict has eight keys and excluding SYN/ACK leaves six. Now named explicitly (cwr,ece,urg,psh,rst,fin).Its remaining verdicts — C2, C4, C5, C7, C8, C9 — all held, including every one of the line citations above and that the schema module's diff is empty. Two things it could not establish, stated as such rather than glossed: it derived the RFC 8684 figure/length map from the in-repo docstrings rather than from the RFC text, having no verified network access; and it did not exhaustively audit every other protocol's
make()for an analogously-shaped ordering defect under a different attribute name, noting that as a separate, larger investigation.Rebased
Rebased onto
0bd517a1c(after #595 merged). One commit per package, on top of the current remote. The only conflicts were in the two changelog files — no source conflict, so thetcp.pychange and the tests are untouched by the rebase.docs/source/changelog/1.5.0.rstwas resolved by keeping #595's bullet and these, not by choosing;CHANGELOG.mdwas not hand-merged but discarded and regenerated withpython util/changelog_md.py, andpython util/changelog_md.py --checkexits 0.Re-verified after the rebase:
tests/protocols/transport/,tests/protocols/test_option_roundtrip_unit.py,tests/project/,tests/test_docstring_contract.pyandtests/test_tier_guard.pytogether give244 passed, 957 subtests passed, exit 0.One thing deliberately left out
The
mptcp_dss_ack_selectordocstring atpcapkit/protocols/schema/transport/tcp.py:270-280, and the matching paragraphs intest_tcp_mptcp_length_arithmetic_unit.py, say that a corrected callable-lengthNumberField"would not have worked" and that fixing it "belongs topcapkit.corekit.fields.numbers". #598 fixes exactly that, which will make those paragraphs stale.Not corrected here, on purpose: #598 is still open and unmerged, so as of
0bd517a1cthat prose still describesmainaccurately. Rewriting it now would make this PR assert something untrue of the tree it targets, and would be wrong again if #598 changes shape or is abandoned. The correction belongs in #598 itself — the change that makes the old wording false should be the change that replaces it — or in a follow-up once it has landed. Flagging it here so it is not lost. (Per #591's worker, theSwitchFieldworkaround itself stays regardless: itsNoValueField()branch handles the field being absent from the wire when the DSS flag is clear, which a callable-lengthNumberFieldcannot express. Only the justification goes stale, not the code.)