Repository navigation
Fix listening socket leak in AgentLocalProxy.get_connection - #2694
Open
kratos0718 wants to merge 1 commit into
Open
kratos0718 wants to merge 1 commit into
kratos0718 wants to merge 1 commit into
Conversation
get_connection() opens a unix socket, binds and listens on it, accepts exactly one connection, and returns that accepted socket - but the listening socket itself was never closed on either the success or the exception path (the bare except: raise was a no-op for control flow, so it didn't help either). Every call leaked the listening socket's file descriptor. Replaced the bare except:/raise with a finally: conn.close(), which closes the listening socket unconditionally while leaving the already-accepted connection socket untouched (accept() returns an independent fd, closing the listener doesn't affect it). Added a regression test using a real unix socket end to end: a background thread connects while the main thread calls get_connection(), and the test asserts the listening socket (tagged via a bind()-intercepting socket subclass, identified unambiguously regardless of how the connector thread's own retrying client sockets interleave with it) is closed afterward, while the accepted socket is not. Verified this test fails against the pre-fix code (fileno() is a live fd, not -1) and passes against the fix.
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.
What
AgentLocalProxy.get_connection()opens a unix socket, binds and listens on it, accepts exactly one connection, and returns that accepted socket:The listening socket (
conn) itself is never closed on either the success path or the exception path - theexcept: raisedoesn't help, it's a no-op for control flow (catches and immediately re-raises the same exception, identical behavior to notry/exceptat all). Every call leaks the listening socket's file descriptor.Fix
Closes the listening socket unconditionally. The already-accepted connection socket (
r) is untouched -accept()returns an independent fd, closing the listener doesn't affect it.Testing
Added a regression test in
tests/agent.pythat drives this through a real unix socket end to end: a background thread connects while the main thread callsget_connection(), and the test tags whichever socket instanceget_connection()callsbind()on (via a smallsocket.socketsubclass swapped in for the duration of the call - unambiguous regardless of how the connector thread's own retrying client sockets interleave with it, since only the listening socket ever callsbind()), then asserts that socket is closed (fileno() == -1) while the accepted one isn't.Verified this test fails against the pre-fix code (
fileno()returns a live fd, not-1) and passes against the fix. Fulltests/agent.pysuite: 33 passed, 1 skipped, unchanged from before this PR.flake8/black --checkclean on both changed files.Note
#2644 is an open PR touching the same file's other bare-
except:clauses (a separate, broader cleanup, unrelated to this specific leak) - flagging in case of overlap, though this PR only touches the oneget_connectionmethod.