GUACAMOLE-2293: Release active connection when guacd socket teardown throws on close#1223
Open
nabaco wants to merge 1 commit into
Open
GUACAMOLE-2293: Release active connection when guacd socket teardown throws on close#1223nabaco wants to merge 1 commit into
nabaco wants to merge 1 commit into
Conversation
…throws on close. ActiveConnectionService.deleteObject() only closed the tunnel when tunnel.isOpen() was true. AbstractGuacamoleTunnel.isOpen() delegates to the underlying socket, so once the remote peer (e.g. a WebSocket client that disconnected abruptly behind a proxy/tunnel) is gone, isOpen() returns false and the kill request becomes a no-op: the ConnectionCleanupTask never runs, so the in-memory active connection record and its concurrency seat are never released. The 'Kill session' UI/REST action then appears to succeed while the ghost session persists. More fundamentally, ManagedInetGuacamoleSocket.close() and ManagedSSLGuacamoleSocket.close() ran the socket-closed cleanup task after super.close() with no finally. When the underlying guacd socket is being torn down concurrently (the read thread and @onclose both close it), super.close() can throw (JDK's NIO layer surfaces a spurious IOException on the second close) and the cleanup task is skipped, leaking the active connection record even on a normal disconnect. Always call tunnel.close() on delete (the cleanup task is idempotent), and run the socket-closed cleanup task in a finally block so cleanup happens even when closing an already-dead socket throws.
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
1 participant
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.
ManagedInetGuacamoleSocket/ManagedSSLGuacamoleSocketrun their cleanup task (socketClosedTask, which removes the in-memoryActiveConnectionRecordand releases its concurrency seat) aftersuper.close(), not in afinally. At disconnect the guacd socket is closed nearly simultaneously by the read thread and by@OnClose. When the two race, the JDK's NIO layer throwsIOExceptionon the secondclose()of the same fd (theclose()syscall succeeds - it is a NIO double-close artifact).super.close()then throws, the cleanup task is skipped, and the active connection leaks in memory until guacamole-app restarts.Run
socketClosedTaskin afinallyso cleanup always happens, regardless of whethersuper.close()throws (ConnectionCleanupTaskalready guards itself to run once, so this is safe).Also drop the
tunnel.isOpen()guard inActiveConnectionService.deleteObject()and always calltunnel.close(). Once the peer has vanishedisOpen()returnsfalse, which made "Kill session" a no-op and left admins unable to reap a leaked connection.