Repository navigation
fix(shm): enable transport SHM only after a completed negotiation - #2883
Open
YuanYuYuan wants to merge 6 commits into
Open
YuanYuYuan wants to merge 6 commits into
YuanYuYuan wants to merge 6 commits into
Conversation
Reject unusable acknowledgements and withhold SHM OpenAck when the peer did not establish an SHM receive lease. Preserve SHM on best-effort links with disabled handoff counters. Add seven negative handshake regressions and a reliable/best-effort control. All seven fail on unmodified upstream and pass with the fix. Related to eclipse-zenoh#2863. Assisted-by: OpenAI Codex Signed-off-by: Richard Williams <richardw347@gmail.com>
An unpatched acceptor puts an SHM extension in every OpenAck, even when it mapped no initiator segment and runs without SHM. The initiator treated that extension as an error whenever it had no mapped acceptor segment, so it refused a peer that would have run plain. Record in the initiator whether the InitAck carried an SHM extension, before any check can decline it. Without an offer, ignore an OpenAck extension and run without SHM. After an offer, keep the existing failures.
An unpatched initiator turns SHM on from the acceptor segment it mapped and does not wait for an OpenAck. When the acceptor declined the proposal (challenge mismatch or unusable counters) it returned Ok and sent no OpenAck extension, so it ran plain while the initiator sent SHM descriptors. Fail the handshake instead, so the link is refused and the operator sees it. An absent OpenSyn extension still keeps the acceptor plain.
Three handshake tests accepted any error. They now check the message of the rejecting step, so a rejection by a different check no longer passes.
An acceptor that declines an SHM proposal must not keep a TX counter. The test counts the counters left in the pool and has a positive control that shows a lease.
A 1.9.0 initiator puts its segment in the InitSyn, so the acceptor maps it. The OpenSyn extension of 1.9.0 has another wire encoding than the one of 1.10, so the acceptor gets no usable SHM proposal. A 1.10.0 or 1.10.1 acceptor enabled SHM from the mapped segment alone and ended on SHM while the initiator ran plain. The test runs the real InitSyn and InitAck exchange, then gives the acceptor no OpenSyn extension. It expects no OpenAck extension, no TX counter and no transport SHM config. A control with a current OpenSyn shows that SHM can be enabled. The test models the state of the acceptor after such a handshake. It does not run the 1.9.0 initiator, and it does not decode the 1.9.0 extension bytes in the transport codec. Only the interop harness covers the real 1.9.0 binary.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2883 +/- ##
==========================================
+ Coverage 74.95% 74.99% +0.03%
==========================================
Files 419 419
Lines 64143 64170 +27
==========================================
+ Hits 48080 48123 +43
+ Misses 16063 16047 -16 ☔ View full report in Codecov by Harness. |
6 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
With an unpatched 1.10.x peer, SHM negotiation can end with one peer on SHM and the other plain. The SHM peer sends descriptors. The plain peer delivers them as payloads, so the application receives corrupt data. Addresses #2863.
The cause is the SHM handoff change in #2646, released in 1.10.0. Before it, 1.9.0 enabled SHM from a confirmed flag. Since it, each peer enables transport SHM when it opened the segment of the other peer. One failed open on one side leaves one peer on SHM and the other plain.
The diagrams show the same case: the acceptor cannot validate the initiator segment and declines.
Before #2646 (1.9.0): SHM needs a confirmation
sequenceDiagram participant I as Initiator participant A as Acceptor I->>A: InitSyn (SHM offer) A->>I: InitAck (acceptor segment) I->>A: OpenSyn (challenge) Note over A: challenge mismatch: decline A->>I: OpenAck (SHM not confirmed) Note over I,A: SHM needs the confirmation on both ends: both run plainSince #2646 (1.10.x): SHM follows the open
sequenceDiagram participant I as Initiator participant A as Acceptor I->>A: InitSyn (SHM offer) A->>I: InitAck (acceptor segment) Note over I: opened the acceptor segment: SHM on I->>A: OpenSyn (challenge) Note over A: challenge mismatch: decline, run plain A->>I: OpenAck (no SHM extension) I->>A: put (SHM descriptor) Note over A: plain: delivers the descriptor bytes as the payloadIn a two-process test, this case produced 695 to 697 corrupt samples per run.
With this PR: a decline fails the handshake
sequenceDiagram participant I as Initiator participant A as Acceptor I->>A: InitSyn (SHM offer) A->>I: InitAck (acceptor segment) I->>A: OpenSyn (challenge) Note over A: challenge mismatch: decline, fail the handshake A->>I: Close Note over I,A: no link, so no SHM descriptor reaches a plain peerFix: before and after
This PR makes a decline fail the handshake, so both peers agree or the acceptor refuses the link.
Commits
Require completed SHM negotiation before enabling transport SHMfix(shm): ignore OpenAck extension without offerfix(shm): fail handshake on declined proposaltest(shm): ...Each fix has a test that fails without it.
Relation to #2873
This PR carries the handshake half of #2873. The lifecycle commit of #2873 merges on its own. The two commits touch different files. If #2873 lands first with its handshake commit, commit 1 drops out on rebase.
Breaking changes
None to the wire format or the public API.
/dev/shmnamespaces