From df385a1a6413bfeab849f5b67c8ae9499fdfa030 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Thu, 10 Dec 2015 18:31:38 -0800 Subject: [PATCH] GUAC-1427: Do not invoke createTunnel() within WebSocket tunnels unless close() is guaranteed to eventually run. --- .../GuacamoleWebSocketTunnelEndpoint.java | 4 ++ ...BasicGuacamoleWebSocketTunnelEndpoint.java | 60 +++++++++---------- .../GuacamoleWebSocketTunnelServlet.java | 36 +++++++---- .../GuacamoleWebSocketTunnelListener.java | 4 ++ .../GuacamoleWebSocketTunnelServlet.java | 35 +++++++---- 5 files changed, 82 insertions(+), 57 deletions(-) diff --git a/guacamole-common/src/main/java/org/glyptodon/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java b/guacamole-common/src/main/java/org/glyptodon/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java index 2b8392bdf..2a85145e0 100644 --- a/guacamole-common/src/main/java/org/glyptodon/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java +++ b/guacamole-common/src/main/java/org/glyptodon/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java @@ -207,6 +207,10 @@ public abstract class GuacamoleWebSocketTunnelEndpoint extends Endpoint { @OnMessage public void onMessage(String message) { + // Ignore inbound messages if there is no associated tunnel + if (tunnel == null) + return; + GuacamoleWriter writer = tunnel.acquireWriter(); try { diff --git a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/websocket/BasicGuacamoleWebSocketTunnelEndpoint.java b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/websocket/BasicGuacamoleWebSocketTunnelEndpoint.java index 0dbc11efc..479d60236 100644 --- a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/websocket/BasicGuacamoleWebSocketTunnelEndpoint.java +++ b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/websocket/BasicGuacamoleWebSocketTunnelEndpoint.java @@ -31,6 +31,7 @@ import javax.websocket.server.HandshakeRequest; import javax.websocket.server.ServerEndpointConfig; import org.glyptodon.guacamole.GuacamoleException; import org.glyptodon.guacamole.net.GuacamoleTunnel; +import org.glyptodon.guacamole.net.basic.TunnelRequest; import org.glyptodon.guacamole.net.basic.TunnelRequestService; import org.glyptodon.guacamole.websocket.GuacamoleWebSocketTunnelEndpoint; @@ -41,16 +42,16 @@ import org.glyptodon.guacamole.websocket.GuacamoleWebSocketTunnelEndpoint; public class BasicGuacamoleWebSocketTunnelEndpoint extends GuacamoleWebSocketTunnelEndpoint { /** - * Unique string which shall be used to store the GuacamoleTunnel + * Unique string which shall be used to store the TunnelRequest * associated with a WebSocket connection. */ - private static final String TUNNEL_USER_PROPERTY = "WS_GUAC_TUNNEL"; + private static final String TUNNEL_REQUEST_PROPERTY = "WS_GUAC_TUNNEL_REQUEST"; /** - * Unique string which shall be used to store any GuacamoleException that - * occurs while retrieving the tunnel during the handshake. + * Unique string which shall be used to store the TunnelRequestService to + * be used for processing TunnelRequests. */ - private static final String ERROR_USER_PROPERTY = "WS_GUAC_TUNNEL_ERROR"; + private static final String TUNNEL_REQUEST_SERVICE_PROPERTY = "WS_GUAC_TUNNEL_REQUEST_SERVICE"; /** * Configurator implementation which stores the requested GuacamoleTunnel @@ -70,52 +71,49 @@ public class BasicGuacamoleWebSocketTunnelEndpoint extends GuacamoleWebSocketTun * service provider to retrieve the necessary service to handle new * connections requests. * - * @param tunnelRequestServiceProvider The tunnel request service - * provider to use for all new - * connections. + * @param tunnelRequestServiceProvider + * The tunnel request service provider to use for all new + * connections. */ public Configurator(Provider tunnelRequestServiceProvider) { this.tunnelRequestServiceProvider = tunnelRequestServiceProvider; } @Override - public void modifyHandshake(ServerEndpointConfig config, HandshakeRequest request, HandshakeResponse response) { + public void modifyHandshake(ServerEndpointConfig config, + HandshakeRequest request, HandshakeResponse response) { super.modifyHandshake(config, request, response); - // Attempt tunnel creation + // Store tunnel request and tunnel request service for retrieval + // upon WebSocket open Map userProperties = config.getUserProperties(); userProperties.clear(); - try { - - // Get tunnel request service - TunnelRequestService tunnelRequestService = tunnelRequestServiceProvider.get(); - - // Store new tunnel within user properties - GuacamoleTunnel tunnel = tunnelRequestService.createTunnel(new WebSocketTunnelRequest(request)); - if (tunnel != null) - userProperties.put(TUNNEL_USER_PROPERTY, tunnel); - - } - catch (GuacamoleException e) { - userProperties.put(ERROR_USER_PROPERTY, e); - } + userProperties.put(TUNNEL_REQUEST_PROPERTY, new WebSocketTunnelRequest(request)); + userProperties.put(TUNNEL_REQUEST_SERVICE_PROPERTY, tunnelRequestServiceProvider.get()); } } @Override - protected GuacamoleTunnel createTunnel(Session session, EndpointConfig config) throws GuacamoleException { + protected GuacamoleTunnel createTunnel(Session session, + EndpointConfig config) throws GuacamoleException { - // Throw any error that occurred during tunnel creation Map userProperties = config.getUserProperties(); - GuacamoleException tunnelError = (GuacamoleException) userProperties.get(ERROR_USER_PROPERTY); - if (tunnelError != null) - throw tunnelError; - // Return created tunnel, if any - return (GuacamoleTunnel) userProperties.get(TUNNEL_USER_PROPERTY); + // Get original tunnel request + TunnelRequest tunnelRequest = (TunnelRequest) userProperties.get(TUNNEL_REQUEST_PROPERTY); + if (tunnelRequest == null) + return null; + + // Get tunnel request service + TunnelRequestService tunnelRequestService = (TunnelRequestService) userProperties.get(TUNNEL_REQUEST_SERVICE_PROPERTY); + if (tunnelRequestService == null) + return null; + + // Create and return tunnel + return tunnelRequestService.createTunnel(tunnelRequest); } diff --git a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/websocket/jetty8/GuacamoleWebSocketTunnelServlet.java b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/websocket/jetty8/GuacamoleWebSocketTunnelServlet.java index e572d05a4..df5806822 100644 --- a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/websocket/jetty8/GuacamoleWebSocketTunnelServlet.java +++ b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/websocket/jetty8/GuacamoleWebSocketTunnelServlet.java @@ -70,25 +70,24 @@ public abstract class GuacamoleWebSocketTunnelServlet extends WebSocketServlet { } @Override - public WebSocket doWebSocketConnect(HttpServletRequest request, String protocol) { - - // Get tunnel - final GuacamoleTunnel tunnel; - - try { - tunnel = doConnect(request); - } - catch (GuacamoleException e) { - logger.error("Creation of WebSocket tunnel to guacd failed: {}", e.getMessage()); - logger.debug("Error connecting WebSocket tunnel.", e); - return null; - } + public WebSocket doWebSocketConnect(final HttpServletRequest request, String protocol) { // Return new WebSocket which communicates through tunnel return new WebSocket.OnTextMessage() { + /** + * The GuacamoleTunnel associated with the connected WebSocket. If + * the WebSocket has not yet been connected, this will be null. + */ + private GuacamoleTunnel tunnel = null; + @Override public void onMessage(String string) { + + // Ignore inbound messages if there is no associated tunnel + if (tunnel == null) + return; + GuacamoleWriter writer = tunnel.acquireWriter(); // Write message received @@ -103,11 +102,22 @@ public abstract class GuacamoleWebSocketTunnelServlet extends WebSocketServlet { } tunnel.releaseWriter(); + } @Override public void onOpen(final Connection connection) { + try { + tunnel = doConnect(request); + } + catch (GuacamoleException e) { + logger.error("Creation of WebSocket tunnel to guacd failed: {}", e.getMessage()); + logger.debug("Error connecting WebSocket tunnel.", e); + closeConnection(connection, e.getStatus()); + return; + } + // Do not start connection if tunnel does not exist if (tunnel == null) { closeConnection(connection, GuacamoleStatus.RESOURCE_NOT_FOUND); diff --git a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/websocket/jetty9/GuacamoleWebSocketTunnelListener.java b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/websocket/jetty9/GuacamoleWebSocketTunnelListener.java index 74fd371c2..802ebbf6f 100644 --- a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/websocket/jetty9/GuacamoleWebSocketTunnelListener.java +++ b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/websocket/jetty9/GuacamoleWebSocketTunnelListener.java @@ -185,6 +185,10 @@ public abstract class GuacamoleWebSocketTunnelListener implements WebSocketListe @Override public void onWebSocketText(String message) { + // Ignore inbound messages if there is no associated tunnel + if (tunnel == null) + return; + GuacamoleWriter writer = tunnel.acquireWriter(); try { diff --git a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/websocket/tomcat/GuacamoleWebSocketTunnelServlet.java b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/websocket/tomcat/GuacamoleWebSocketTunnelServlet.java index 80476c366..ed37a9e76 100644 --- a/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/websocket/tomcat/GuacamoleWebSocketTunnelServlet.java +++ b/guacamole/src/main/java/org/glyptodon/guacamole/net/basic/websocket/tomcat/GuacamoleWebSocketTunnelServlet.java @@ -92,26 +92,25 @@ public abstract class GuacamoleWebSocketTunnelServlet extends WebSocketServlet { } @Override - public StreamInbound createWebSocketInbound(String protocol, HttpServletRequest request) { - - // Get tunnel - final GuacamoleTunnel tunnel; - - try { - tunnel = doConnect(request); - } - catch (GuacamoleException e) { - logger.error("Creation of WebSocket tunnel to guacd failed: {}", e.getMessage()); - logger.debug("Error connecting WebSocket tunnel.", e); - return null; - } + public StreamInbound createWebSocketInbound(String protocol, + final HttpServletRequest request) { // Return new WebSocket which communicates through tunnel return new StreamInbound() { + /** + * The GuacamoleTunnel associated with the connected WebSocket. If + * the WebSocket has not yet been connected, this will be null. + */ + private GuacamoleTunnel tunnel = null; + @Override protected void onTextData(Reader reader) throws IOException { + // Ignore inbound messages if there is no associated tunnel + if (tunnel == null) + return; + GuacamoleWriter writer = tunnel.acquireWriter(); // Write all available data @@ -137,6 +136,16 @@ public abstract class GuacamoleWebSocketTunnelServlet extends WebSocketServlet { @Override public void onOpen(final WsOutbound outbound) { + try { + tunnel = doConnect(request); + } + catch (GuacamoleException e) { + logger.error("Creation of WebSocket tunnel to guacd failed: {}", e.getMessage()); + logger.debug("Error connecting WebSocket tunnel.", e); + closeConnection(outbound, e.getStatus()); + return; + } + // Do not start connection if tunnel does not exist if (tunnel == null) { closeConnection(outbound, GuacamoleStatus.RESOURCE_NOT_FOUND);