This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/9.0.x by this push:
new 9c1daf4505 Fix BZ 70110 and various related issues identified by
CoPilot review
9c1daf4505 is described below
commit 9c1daf4505de7d2111609ed9e663a34a1f67ab1a
Author: Mark Thomas <[email protected]>
AuthorDate: Tue Jun 16 11:25:05 2026 +0100
Fix BZ 70110 and various related issues identified by CoPilot review
---
java/org/apache/coyote/AbstractProtocol.java | 51 +++---
.../tomcat/websocket/LocalStrings.properties | 1 +
.../tomcat/websocket/WsWebSocketContainer.java | 28 ++-
.../tomcat/websocket/pojo/PojoEndpointBase.java | 4 +-
.../websocket/server/LocalStrings.properties | 1 +
.../websocket/server/WsHttpUpgradeHandler.java | 24 ++-
test/org/apache/coyote/TestAbstractProtocol.java | 197 +++++++++++++++++++++
webapps/docs/changelog.xml | 10 ++
8 files changed, 285 insertions(+), 31 deletions(-)
diff --git a/java/org/apache/coyote/AbstractProtocol.java
b/java/org/apache/coyote/AbstractProtocol.java
index f50414f9fc..496c2263db 100644
--- a/java/org/apache/coyote/AbstractProtocol.java
+++ b/java/org/apache/coyote/AbstractProtocol.java
@@ -1445,27 +1445,7 @@ public abstract class AbstractProtocol<S> implements
ProtocolHandler, MBeanRegis
// Connection closed. OK to recycle the processor.
// Processors handling upgrades require additional clean-up
// before release.
- if (processor.isUpgrade()) {
- UpgradeToken upgradeToken =
processor.getUpgradeToken();
- HttpUpgradeHandler httpUpgradeHandler =
upgradeToken.getHttpUpgradeHandler();
- InstanceManager instanceManager =
upgradeToken.getInstanceManager();
- if (instanceManager == null) {
- httpUpgradeHandler.destroy();
- } else {
- ClassLoader oldCL =
upgradeToken.getContextBind().bind(false, null);
- try {
- httpUpgradeHandler.destroy();
- } finally {
- try {
-
instanceManager.destroyInstance(httpUpgradeHandler);
- } catch (Throwable t) {
- ExceptionUtils.handleThrowable(t);
-
getLog().error(sm.getString("abstractConnectionHandler.error"), t);
- }
- upgradeToken.getContextBind().unbind(false,
oldCL);
- }
- }
- }
+ cleanUpIfUpgrade(processor);
release(processor);
processor = null;
@@ -1508,6 +1488,9 @@ public abstract class AbstractProtocol<S> implements
ProtocolHandler, MBeanRegis
getLog().error(sm.getString("abstractConnectionHandler.error"), t);
}
+ // Processors handling upgrades require additional clean-up
+ // before release.
+ cleanUpIfUpgrade(processor);
// Make sure socket/processor is removed from the list of current
// connections
release(processor);
@@ -1515,6 +1498,31 @@ public abstract class AbstractProtocol<S> implements
ProtocolHandler, MBeanRegis
}
+ private void cleanUpIfUpgrade(Processor processor) {
+ if (processor != null && processor.isUpgrade()) {
+ UpgradeToken upgradeToken = processor.getUpgradeToken();
+ HttpUpgradeHandler httpUpgradeHandler =
upgradeToken.getHttpUpgradeHandler();
+ InstanceManager instanceManager =
upgradeToken.getInstanceManager();
+ if (instanceManager == null) {
+ httpUpgradeHandler.destroy();
+ } else {
+ ClassLoader oldCL =
upgradeToken.getContextBind().bind(false, null);
+ try {
+ httpUpgradeHandler.destroy();
+ } finally {
+ try {
+
instanceManager.destroyInstance(httpUpgradeHandler);
+ } catch (Throwable t) {
+ ExceptionUtils.handleThrowable(t);
+
getLog().error(sm.getString("abstractConnectionHandler.error"), t);
+ }
+ upgradeToken.getContextBind().unbind(false, oldCL);
+ }
+ }
+ }
+ }
+
+
/**
* Performs a long poll on the socket.
*
@@ -1588,6 +1596,7 @@ public abstract class AbstractProtocol<S> implements
ProtocolHandler, MBeanRegis
@Override
public void release(SocketWrapperBase<S> socketWrapper) {
Processor processor = (Processor)
socketWrapper.takeCurrentProcessor();
+ cleanUpIfUpgrade(processor);
release(processor);
}
diff --git a/java/org/apache/tomcat/websocket/LocalStrings.properties
b/java/org/apache/tomcat/websocket/LocalStrings.properties
index 22968a6340..ff911a8fbf 100644
--- a/java/org/apache/tomcat/websocket/LocalStrings.properties
+++ b/java/org/apache/tomcat/websocket/LocalStrings.properties
@@ -132,6 +132,7 @@ wsSession.unknownHandler=Unable to add the message handler
[{0}] as it was for t
wsSession.unknownHandlerType=Unable to add the message handler [{0}] as it was
wrapped as the unrecognised type [{1}]
wsWebSocketContainer.asynchronousSocketChannelFail=Unable to open a connection
to the server
+wsWebSocketContainer.closeSessionFail=Failed to close WebSocket session during
error handling
wsWebSocketContainer.connect.entry=Connecting endpoint instance of type [{0}]
to [{1}]
wsWebSocketContainer.connect.write=Writing the HTTP upgrade request from
buffer starting at [{0}] with a limit of [{1}] from local address [{2}]
wsWebSocketContainer.defaultConfiguratorFail=Failed to create the default
configurator
diff --git a/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
b/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
index 5bfdbe833e..881934dd39 100644
--- a/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
+++ b/java/org/apache/tomcat/websocket/WsWebSocketContainer.java
@@ -70,6 +70,7 @@ import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.InstanceManager;
import org.apache.tomcat.InstanceManagerBindings;
+import org.apache.tomcat.util.ExceptionUtils;
import org.apache.tomcat.util.buf.StringUtils;
import org.apache.tomcat.util.collections.CaseInsensitiveKeyMap;
import org.apache.tomcat.util.http.Method;
@@ -484,8 +485,22 @@ public class WsWebSocketContainer implements
WebSocketContainer, BackgroundProce
// completed transformation chain to the remote end point.
wsRemoteEndpointClient.setTransformation(wsFrameClient.getTransformation());
- wsSession.getLocal().onOpen(wsSession, clientEndpointConfiguration);
- registerSession(wsSession.getLocal(), wsSession);
+ try {
+ wsSession.getLocal().onOpen(wsSession,
clientEndpointConfiguration);
+ } catch (Throwable t) {
+ ExceptionUtils.handleThrowable(t);
+ wsSession.getLocal().onError(wsSession, t);
+ try {
+ CloseReason cr = new CloseReason(CloseCodes.CLOSED_ABNORMALLY,
t.getMessage());
+ wsSession.close(cr);
+ } catch (IOException ioe) {
+
log.warn(sm.getString("wsWebSocketContainer.closeSessionFail"), ioe);
+ }
+ throw new IllegalArgumentException(t);
+ } finally {
+ // Always register the session to ensure the connection is closed
in both normal and error cases.
+ registerSession(wsSession.getLocal(), wsSession);
+ }
/*
* It is possible that the server sent one or more messages as soon as
the WebSocket connection was established.
@@ -610,8 +625,13 @@ public class WsWebSocketContainer implements
WebSocketContainer, BackgroundProce
*/
protected void registerSession(Object key, WsSession wsSession) {
- if (!wsSession.isOpen()) {
- // The session was closed during onOpen. No need to register it.
+ if (wsSession.isClosed()) {
+ /*
+ * The session was fully closed during onOpen. No need to register
it. For all other states, including the
+ * partially closed ones, register the session to ensure that the
connection is closed if the other endpoint
+ * doesn't complete the closing handshake in a timely manner.
+ */
+
return;
}
synchronized (endPointSessionMapLock) {
diff --git a/java/org/apache/tomcat/websocket/pojo/PojoEndpointBase.java
b/java/org/apache/tomcat/websocket/pojo/PojoEndpointBase.java
index 8e8e38a121..a18d0c12ee 100644
--- a/java/org/apache/tomcat/websocket/pojo/PojoEndpointBase.java
+++ b/java/org/apache/tomcat/websocket/pojo/PojoEndpointBase.java
@@ -22,6 +22,7 @@ import java.util.Map;
import java.util.Set;
import javax.websocket.CloseReason;
+import javax.websocket.CloseReason.CloseCodes;
import javax.websocket.Endpoint;
import javax.websocket.EndpointConfig;
import javax.websocket.MessageHandler;
@@ -98,7 +99,8 @@ public abstract class PojoEndpointBase extends Endpoint {
// Trigger the error handler and close the session
onError(session, t);
try {
- session.close();
+ CloseReason cr = new CloseReason(CloseCodes.CLOSED_ABNORMALLY,
t.getMessage());
+ session.close(cr);
} catch (IOException ioe) {
log.warn(sm.getString("pojoEndpointBase.closeSessionFail"), ioe);
}
diff --git a/java/org/apache/tomcat/websocket/server/LocalStrings.properties
b/java/org/apache/tomcat/websocket/server/LocalStrings.properties
index 5a2359dcc6..f6d8e68994 100644
--- a/java/org/apache/tomcat/websocket/server/LocalStrings.properties
+++ b/java/org/apache/tomcat/websocket/server/LocalStrings.properties
@@ -37,6 +37,7 @@ wsFrameServer.illegalReadState=Unexpected read state [{0}]
wsFrameServer.onDataAvailable=Method entry
wsHttpUpgradeHandler.closeOnError=Closing WebSocket connection due to an error
+wsHttpUpgradeHandler.closeSessionFail=Failed to close WebSocket session during
error handling
wsHttpUpgradeHandler.destroyFailed=Failed to close WebConnection while
destroying the WebSocket HttpUpgradeHandler
wsHttpUpgradeHandler.noPreInit=The preInit() method must be called to
configure the WebSocket HttpUpgradeHandler before the container calls init().
Usually, this means the Servlet that created the WsHttpUpgradeHandler instance
should also call preInit()
wsHttpUpgradeHandler.serverStop=The server is stopping
diff --git a/java/org/apache/tomcat/websocket/server/WsHttpUpgradeHandler.java
b/java/org/apache/tomcat/websocket/server/WsHttpUpgradeHandler.java
index 30937134ec..f3a9326426 100644
--- a/java/org/apache/tomcat/websocket/server/WsHttpUpgradeHandler.java
+++ b/java/org/apache/tomcat/websocket/server/WsHttpUpgradeHandler.java
@@ -34,6 +34,7 @@ import
org.apache.coyote.http11.upgrade.InternalHttpUpgradeHandler;
import org.apache.coyote.http11.upgrade.UpgradeInfo;
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
+import org.apache.tomcat.util.ExceptionUtils;
import org.apache.tomcat.util.net.AbstractEndpoint.Handler.SocketState;
import org.apache.tomcat.util.net.SSLSupport;
import org.apache.tomcat.util.net.SocketEvent;
@@ -128,9 +129,9 @@ public class WsHttpUpgradeHandler implements
InternalHttpUpgradeHandler {
// Need to call onOpen using the web application's class loader
// Create the frame using the application's class loader so it can pick
// up application specific config from the ServerContainerImpl
- Thread t = Thread.currentThread();
- ClassLoader cl = t.getContextClassLoader();
- t.setContextClassLoader(applicationClassLoader);
+ Thread currentThread = Thread.currentThread();
+ ClassLoader cl = currentThread.getContextClassLoader();
+ currentThread.setContextClassLoader(applicationClassLoader);
try {
wsRemoteEndpointServer =
new WsRemoteEndpointImplServer(socketWrapper, upgradeInfo,
webSocketContainer, connection);
@@ -143,12 +144,25 @@ public class WsHttpUpgradeHandler implements
InternalHttpUpgradeHandler {
// WsFrame adds the necessary final transformations. Copy the
// completed transformation chain to the remote end point.
wsRemoteEndpointServer.setTransformation(wsFrame.getTransformation());
- ep.onOpen(wsSession, serverEndpointConfig);
+ try {
+ ep.onOpen(wsSession, serverEndpointConfig);
+ } catch (Throwable t) {
+ // If this really is fatal re-thrown
+ ExceptionUtils.handleThrowable(t);
+ ep.onError(wsSession, t);
+ try {
+ CloseReason cr = new
CloseReason(CloseCodes.CLOSED_ABNORMALLY, t.getMessage());
+ wsSession.close(cr);
+ } catch (IOException ioe) {
+
log.warn(sm.getString("wsHttpUpgradeHandler.closeSessionFail"), ioe);
+ }
+ throw new IllegalArgumentException(t);
+ }
webSocketContainer.registerSession(serverEndpointConfig.getPath(),
wsSession);
} catch (DeploymentException e) {
throw new IllegalArgumentException(e);
} finally {
- t.setContextClassLoader(cl);
+ currentThread.setContextClassLoader(cl);
}
}
diff --git a/test/org/apache/coyote/TestAbstractProtocol.java
b/test/org/apache/coyote/TestAbstractProtocol.java
new file mode 100644
index 0000000000..81367838a6
--- /dev/null
+++ b/test/org/apache/coyote/TestAbstractProtocol.java
@@ -0,0 +1,197 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.coyote;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
+import java.lang.reflect.Field;
+import java.net.Socket;
+import java.nio.charset.StandardCharsets;
+import java.util.Base64;
+import java.util.Set;
+import java.util.function.BooleanSupplier;
+
+import javax.servlet.ServletContextEvent;
+import javax.websocket.DeploymentException;
+import javax.websocket.Endpoint;
+import javax.websocket.EndpointConfig;
+import javax.websocket.Session;
+import javax.websocket.server.ServerContainer;
+import javax.websocket.server.ServerEndpointConfig;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.catalina.Context;
+import org.apache.catalina.servlets.DefaultServlet;
+import org.apache.catalina.startup.Tomcat;
+import org.apache.coyote.http11.AbstractHttp11Protocol;
+import org.apache.coyote.http11.upgrade.UpgradeGroupInfo;
+import org.apache.coyote.http11.upgrade.UpgradeInfo;
+import org.apache.tomcat.websocket.WebSocketBaseTest;
+import org.apache.tomcat.websocket.server.Constants;
+import org.apache.tomcat.websocket.server.WsContextListener;
+
+/**
+ * Reproducer for UpgradeInfo leak in AbstractProtocol.ConnectionHandler.
+ * <p>
+ * When a WebSocket endpoint's onOpen() throws a RuntimeException, the
exception propagates out of
+ * WsHttpUpgradeHandler.init(), which is called at
AbstractProtocol.ConnectionHandler.process() line 945 — after the
+ * UpgradeProcessorInternal (with its UpgradeInfo) was already created at line
933. The exception is caught by the catch
+ * blocks (lines 1037-1068), which fall through to release(processor) without
calling httpUpgradeHandler.destroy(). The
+ * UpgradeInfo is never removed from the UpgradeGroupInfo set.
+ * <p>
+ * The same issue exists in ConnectionHandler.release(SocketWrapperBase),
which is called when the NIO poller closes a
+ * socket directly (e.g., on thread pool rejection, invalid key, or
CancelledKeyException).
+ */
+public class TestAbstractProtocol extends WebSocketBaseTest {
+
+ private static final String PATH = "/throwOnOpen";
+
+ @Test
+ public void testUpgradeInfoLeakOnOpenException() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+ Context ctx = getProgrammaticRootContext();
+ ctx.addApplicationListener(Config.class.getName());
+ Tomcat.addServlet(ctx, "default", new DefaultServlet());
+ ctx.addServletMappingDecoded("/", "default");
+
+ tomcat.start();
+
+ AbstractHttp11Protocol<?> protocol = (AbstractHttp11Protocol<?>)
tomcat.getConnector().getProtocolHandler();
+
+ UpgradeGroupInfo groupInfo = protocol.getUpgradeGroupInfo("websocket");
+ Set<UpgradeInfo> upgradeInfos = getUpgradeInfoSet(groupInfo);
+
+ int sizeBeforeTest = upgradeInfos.size();
+ int numConnections = 5;
+
+ for (int i = 0; i < numConnections; i++) {
+ connectWebSocket(getPort(), PATH);
+ }
+
+ // Wait for server to process the connections
+ Thread.sleep(1000);
+ waitForCondition(() -> protocol.getWaitingProcessorCount() == 0,
10000);
+
+ int leakedCount = upgradeInfos.size() - sizeBeforeTest;
+ Assert.assertEquals("UpgradeInfo leaked! Found " + leakedCount + "
leaked objects. " +
+ "When onOpen() throws, the exception path in " +
+ "ConnectionHandler.process() calls release(processor) without
" +
+ "calling httpUpgradeHandler.destroy().", sizeBeforeTest,
upgradeInfos.size());
+ }
+
+
+ /**
+ * Performs a WebSocket handshake and then closes the socket.
+ */
+ private void connectWebSocket(int port, String path) throws Exception {
+ Socket socket = new Socket("localhost", port);
+ socket.setSoLinger(true, 0);
+ socket.setSoTimeout(5000);
+
+ OutputStream out = socket.getOutputStream();
+ InputStream in = socket.getInputStream();
+
+ byte[] keyBytes = new byte[16];
+ for (int i = 0; i < keyBytes.length; i++) {
+ keyBytes[i] = (byte) (Math.random() * 256);
+ }
+ String wsKey = Base64.getEncoder().encodeToString(keyBytes);
+
+ //@formatter:off
+ String request =
+ "GET " + path + " HTTP/1.1\r\n" +
+ "Host: localhost:" + port + "\r\n" +
+ "Upgrade: websocket\r\n" +
+ "Connection: Upgrade\r\n" +
+ "Sec-WebSocket-Key: " + wsKey + "\r\n" +
+ "Sec-WebSocket-Version: 13\r\n" + "\r\n";
+ //@formatter:on
+
+ out.write(request.getBytes(StandardCharsets.ISO_8859_1));
+ out.flush();
+
+ // Read response (may be 101 before the exception, or an error)
+ StringBuilder response = new StringBuilder();
+ try {
+ int b;
+ while ((b = in.read()) != -1) {
+ response.append((char) b);
+ if (response.toString().endsWith("\r\n\r\n")) {
+ break;
+ }
+ }
+ } catch (IOException e) {
+ // Server may close the connection
+ }
+
+ try {
+ socket.close();
+ } catch (IOException e) {
+ // Ignore
+ }
+ }
+
+
+ /**
+ * WebSocket endpoint that throws RuntimeException from onOpen(). This is
a programmatic endpoint — not wrapped by
+ * PojoEndpointBase, so the exception propagates directly out of
WsHttpUpgradeHandler.init().
+ */
+ public static class ThrowingEndpoint extends Endpoint {
+ @Override
+ public void onOpen(Session session, EndpointConfig config) {
+ throw new RuntimeException("Simulated onOpen failure");
+ }
+ }
+
+
+ /**
+ * Registers the ThrowingEndpoint as a programmatic WebSocket endpoint.
+ */
+ public static class Config extends WsContextListener {
+ @Override
+ public void contextInitialized(ServletContextEvent sce) {
+ super.contextInitialized(sce);
+ ServerContainer sc = (ServerContainer) sce.getServletContext()
+
.getAttribute(Constants.SERVER_CONTAINER_SERVLET_CONTEXT_ATTRIBUTE);
+ try {
+ ServerEndpointConfig sec =
ServerEndpointConfig.Builder.create(ThrowingEndpoint.class, PATH).build();
+ sc.addEndpoint(sec);
+ } catch (DeploymentException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+ }
+
+
+ @SuppressWarnings("unchecked")
+ private static Set<UpgradeInfo> getUpgradeInfoSet(UpgradeGroupInfo
groupInfo) throws Exception {
+ Field upgradeInfosField =
UpgradeGroupInfo.class.getDeclaredField("upgradeInfos");
+ upgradeInfosField.setAccessible(true);
+ return (Set<UpgradeInfo>) upgradeInfosField.get(groupInfo);
+ }
+
+
+ private static void waitForCondition(BooleanSupplier condition, long
timeoutMs) throws InterruptedException {
+ long deadline = System.currentTimeMillis() + timeoutMs;
+ while (!condition.getAsBoolean() && System.currentTimeMillis() <
deadline) {
+ Thread.sleep(50);
+ }
+ }
+}
\ No newline at end of file
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 72c735d6f7..e1f47ccc4c 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -414,6 +414,16 @@
Incorrect <code>Future.isDone()</code> return by
<code>AsyncChannelWrapperSecure</code>. (remm)
</fix>
+ <fix>
+ Trigger standard WebSocket error handling if a call to
+ <code>Endpoint.onOpen()</code> fails for a programmatic endpoint.
+ (markt)
+ </fix>
+ <fix>
+ <bug>70110</bug>: Fix memory leak if a call to
+ <code>Endpoint.onOpen()</code> fails for a programmatic endpoint. Test
+ case provided by uabdur. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Web applications">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
