Skip to content

Commit 5bad96f

Browse files
author
nachumb
committed
GUACAMOLE-2293: Release active connection when guacd socket teardown 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.
1 parent 4e388e4 commit 5bad96f

3 files changed

Lines changed: 20 additions & 6 deletions

File tree

extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/activeconnection/ActiveConnectionService.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,9 +121,11 @@ public void deleteObject(ModeledAuthenticatedUser user, String identifier)
121121

122122
if (hasObjectPermissions(user, identifier, ObjectPermission.Type.DELETE)) {
123123

124-
// Close connection if not already closed
124+
// Always close, even if the peer already vanished (isOpen() may be
125+
// false). close() releases the active connection record and is
126+
// idempotent.
125127
GuacamoleTunnel tunnel = activeConnection.getTunnel();
126-
if (tunnel != null && tunnel.isOpen())
128+
if (tunnel != null)
127129
tunnel.close();
128130

129131
}

extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/ManagedInetGuacamoleSocket.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,14 @@ public ManagedInetGuacamoleSocket(String hostname, int port,
5959

6060
@Override
6161
public void close() throws GuacamoleException {
62-
super.close();
63-
socketClosedTask.run();
62+
// Run cleanup even if super.close() throws, or the active connection
63+
// record could leak.
64+
try {
65+
super.close();
66+
}
67+
finally {
68+
socketClosedTask.run();
69+
}
6470
}
6571

6672
}

extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/tunnel/ManagedSSLGuacamoleSocket.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,14 @@ public ManagedSSLGuacamoleSocket(String hostname, int port,
5959

6060
@Override
6161
public void close() throws GuacamoleException {
62-
super.close();
63-
socketClosedTask.run();
62+
// Run cleanup even if super.close() throws, or the active connection
63+
// record could leak.
64+
try {
65+
super.close();
66+
}
67+
finally {
68+
socketClosedTask.run();
69+
}
6470
}
6571

6672
}

0 commit comments

Comments
 (0)