fix(transport): close inbound link when accept fails or times out - #2736
Closed
eeshsaxena wants to merge 1 commit into
Closed
fix(transport): close inbound link when accept fails or times out#2736eeshsaxena wants to merge 1 commit into
eeshsaxena wants to merge 1 commit into
Conversation
The acceptor task ran accept_link inside tokio::time::timeout and only checked the outer Result with .is_err(). That is true only when the timeout elapses, so the Ok(Err(e)) case, an early handshake error such as a malformed InitSyn, was discarded. Neither the error case nor the timeout case closed the link. accept_link only closes the link on its FSM reject path (the step! macro). Errors that return earlier, and the timeout, leave the socket open. It then sits in CLOSE-WAIT until the process restarts, so a peer that starts a handshake and walks away leaks a file descriptor each time. Match on the full Result and close the link on both failure paths, the same way the accept_pending (DoS) branch just above already does. On success the established transport keeps its own handle, and the close only runs after the timeout returns, so accept_link's future is already gone and there is no concurrent access to the link. Signed-off-by: eeshsaxena <eeshsaxena@gmail.com>
Author
|
Closing this to clear out my older open PRs. Nothing wrong with the change on my side, I am just tidying up a backlog. If it is still useful to you, say so and I will reopen it and rebase it on current main. |
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.
Fixes #2734.
The bug
The acceptor task wraps
accept_linkintokio::time::timeoutand only looks at the outerResultwith.is_err():timeout(...).awaitreturnsResult<ZResult<()>, Elapsed>..is_err()is only true forElapsed, so theOk(Err(e))case (an early handshake error, for example a malformed InitSyn) is silently dropped. More importantly, neither the error case nor the timeout case closes the link.accept_linkitself only closes the link on its FSM reject path (thestep!macro, which sends a Close with a reason). Errors that return earlier, such as a failed InitSyn decode or a?-propagated setup error, and the timeout, never reach that path, so the socket is left open. It then lingers inCLOSE-WAITuntil the process restarts. A peer that begins a handshake and then goes away leaks one descriptor each time, which is the repro in the issue.The fix
Match on the full
Resultand close the link on both failure paths, mirroring what theaccept_pending(DoS) branch a few lines above already does withlink.close():Ok(Ok(())): success, the established transport keeps its own handle, so just drop this one.Ok(Err(e)): log the error and close the link.Err(_)(timeout): log the deadline miss and close the link.accept_linktakes the link by value, so I keep a cheap clone (LinkUnicastisArc-backed) to close afterwards. The close only runs aftertimeout(...).awaithas returned, i.e. afteraccept_link's future has completed or been dropped, so there is no concurrent access to the link. Ifaccept_linkalready closed the link on itsstep!path, the extraclose()is a harmless no-op whose error is ignored.Testing
I don't have a Rust toolchain on this machine to run the suite, and there is no in-tree mock
LinkUnicastTraitto unit test the acceptor task against, so I verified this by reading. The reproduction and CLOSE-WAIT count in #2734 are the behavioral check: with this change the link is closed on the failure paths instead of leaking.Note: I still need to get the Eclipse ECA sorted out for my email, will make sure the DCO/ECA check is green.
🏷️ Label-Based Checklist
No specific label requirements detected.
Current labels: No labels
Add one of these labels to this PR to see relevant checklist items:
api-sync,breaking-change,bug,ci,dependencies,documentation,enhancement,new feature,internalThis section updates automatically when labels change.