This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 695c33c36aac5bb148865dcfd010e07c6dab2d92 Author: opencode <[email protected]> AuthorDate: Fri Nov 6 08:08:35 2026 +0100 Fix initialization and interceptor failure cleanup in the pool A malformed jdbcInterceptors value installed on a live pool could leak a borrowed connection when a pre-loaded connection built its interceptor chain on borrow. The parse failure now returns the connection to the pool and is reported as an SQLException. A parse failure during pool close is logged and no longer aborts the interceptor close notifications. Pool initialization now also cleans up when a non-fatal Error escapes an interceptor start, and when arming the cleaner fails after the initial pre-load. Both failures close the partially built pool and are reported as an SQLException. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 47 +++++++- .../tomcat/jdbc/pool/util/LocalStrings.properties | 2 + .../TestMalformedInterceptorConfiguration.java | 110 ++++++++++++++++++ .../jdbc/pool/TestPoolInitializationCleanup.java | 123 +++++++++++++++++++++ 4 files changed, 277 insertions(+), 5 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java index 11204c4483..c679d05d59 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java @@ -454,8 +454,21 @@ public class ConnectionPool { // // Build the proxy handler handler = new ProxyConnection(this, con, getPoolProperties().isUseEquals()); - // Set up the interceptor chain - PoolProperties.InterceptorDefinition[] proxies = getPoolProperties().getJdbcInterceptorsAsArray(); + // Set up the interceptor chain. The property is mutable on a live + // pool and its parse can fail, so a malformed value must fail this + // borrow with the connection returned to the pool rather than + // leave it counted as busy with no handle a caller can return. + PoolProperties.InterceptorDefinition[] proxies; + try { + proxies = getPoolProperties().getJdbcInterceptorsAsArray(); + } catch (Throwable x) { + ExceptionUtils.handleThrowable(x); + SQLException sx = new SQLException(MessageFactory.get("connectionPool.unableToParseInterceptorChain")); + sx.initCause(x); + // Nothing has been done to the connection, it can go back to the pool + abandonSetup(con, sx, false); + throw sx; + } for (int i = proxies.length - 1; i >= 0; i--) { // Record what this chain was built from: the definitions the // close has to pair with are the ones a chain ran, not only @@ -778,7 +791,17 @@ public class ConnectionPool { informInterceptorOfPoolClosure(started[i]); } } - PoolProperties.InterceptorDefinition[] proxies = getPoolProperties().getJdbcInterceptorsAsArray(); + PoolProperties.InterceptorDefinition[] proxies; + try { + proxies = getPoolProperties().getJdbcInterceptorsAsArray(); + } catch (Throwable x) { + // A malformed list that was installed on the live pool must not + // abort the close: the definitions already recorded still have + // to receive their notification. + ExceptionUtils.handleThrowable(x); + log.warn(MessageFactory.get("connectionPool.unableToParseInterceptorChain"), x); + proxies = new PoolProperties.InterceptorDefinition[0]; + } for (int i = 0; i < proxies.length; i++) { if (!isToldAlready(proxies[i], started)) { informInterceptorOfPoolClosure(proxies[i]); @@ -921,7 +944,8 @@ public class ConnectionPool { JdbcInterceptor interceptor = proxies[i].getInterceptorClass().getConstructor().newInstance(); interceptor.setProperties(proxies[i].getProperties()); interceptor.poolStarted(this); - } catch (Exception x) { + } catch (Throwable x) { + ExceptionUtils.handleThrowable(x); log.error(MessageFactory.get("connectionPool.unableToInformInterceptorOfPoolStart"), x); org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = jmxPool; if (jmx != null) { @@ -990,7 +1014,20 @@ public class ConnectionPool { // earlier configuration called for, and a trigger turned on during // startup then leaves the getters promising maintenance that no thread // ever performs for the life of the pool. - initializePoolCleaner(properties); + try { + initializePoolCleaner(properties); + } catch (Throwable x) { + // The construction has already acquired resources - the MBean, + // the interceptors told about the start, and the pre-loaded + // sessions - that no caller can reach once this constructor + // fails, so they are torn down here. + ExceptionUtils.handleThrowable(x); + log.error(MessageFactory.get("connectionPool.unableToInitializePoolCleaner"), x); + close(true); + SQLException ex = new SQLException(x.getMessage()); + ex.initCause(x); + throw ex; + } closed = false; } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties index 631c562cf3..6a6080c7f3 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties @@ -75,7 +75,9 @@ connectionPool.unableToCheckClosed=Unable to check if connection is closed connectionPool.unableToClosePooledConnection=Unable to close pooled connection. connectionPool.unableToCreateInitialConnections=Unable to create initial connections of pool. connectionPool.unableToInformInterceptorOfPoolStart=Unable to inform interceptor of pool start. +connectionPool.unableToInitializePoolCleaner=Unable to initialize the pool cleaner. connectionPool.unableToInstantiateInterceptorChain=Unable to instantiate interceptor chain. +connectionPool.unableToParseInterceptorChain=Unable to parse the interceptor configuration. connectionPool.unableToRegisterMBean=Unable to register the pooled connection MBean. connectionPool.unableToResetInterceptor=Unable to reset interceptor [{0}]. connectionPool.unableToReturnConnection=Unable to return connection [{0}] to the pool, closing it. diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestMalformedInterceptorConfiguration.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestMalformedInterceptorConfiguration.java new file mode 100644 index 0000000000..c33318e2cd --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestMalformedInterceptorConfiguration.java @@ -0,0 +1,110 @@ +/* + * 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.tomcat.jdbc.pool; + +import java.sql.Connection; +import java.sql.SQLException; +import java.util.concurrent.atomic.AtomicInteger; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * A malformed {@code jdbcInterceptors} value that is installed on a live pool + * must fail the borrow or the close cleanly. It must not leave the borrowed + * connection counted as busy, and it must not stop the pool from delivering + * the interceptor close notifications. + */ +class TestMalformedInterceptorConfiguration extends JdbcPoolBaseTest { + + private static final String MALFORMED = "StatementCache(password=secret,bad)"; + + public static class CloseCountingInterceptor extends JdbcInterceptor { + + static final AtomicInteger CLOSED = new AtomicInteger(); + + @Override + public void reset(ConnectionPool parent, PooledConnection con) { + // NO-OP + } + + @Override + public void poolClosed(ConnectionPool parent) { + CLOSED.incrementAndGet(); + } + } + + @Test + void malformedRuntimeInterceptorListDoesNotLeakTheBorrowedConnection() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitialSize(1); + ds.getPoolProperties().setMinIdle(0); + ds.getPoolProperties().setMaxActive(2); + ds.getPoolProperties().setMaxIdle(2); + ds.getPoolProperties().setTimeBetweenEvictionRunsMillis(-1); + ds.createPool(); + ConnectionPool pool = ds.getPool(); + + // The pre-loaded connection has no interceptor chain yet, so the next + // borrow builds one and parses the (now malformed) property. + ds.getPoolProperties().setJdbcInterceptors(MALFORMED); + + Assertions.assertThrows(SQLException.class, ds::getConnection, + "a malformed interceptor list must fail the borrow"); + Assertions.assertEquals(0, pool.getActive(), + "the failed setup must not leave the connection in busy"); + Assertions.assertEquals(1, pool.getIdle(), + "the connection must be returned to idle"); + Assertions.assertEquals(1, pool.getSize(), + "the failed setup must not leak the pool slot"); + + // The same connection is still usable once the configuration is fixed. + ds.getPoolProperties().setJdbcInterceptors(null); + try (Connection second = ds.getConnection()) { + Assertions.assertNotNull(second); + } + } + + @Test + void malformedRuntimeInterceptorListDoesNotAbortCloseNotifications() throws Exception { + CloseCountingInterceptor.CLOSED.set(0); + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMinIdle(0); + ds.getPoolProperties().setTimeBetweenEvictionRunsMillis(-1); + ds.createPool(); + ConnectionPool pool = ds.getPool(); + + // Install an interceptor after the pool started and use it once, so + // the definition is recorded as part of a connection chain. + ds.getPoolProperties().setJdbcInterceptors(CloseCountingInterceptor.class.getName()); + try (Connection connection = ds.getConnection()) { + Assertions.assertNotNull(connection); + } + + // Break the property before the close. The close still has to deliver + // the notification to the definitions the chain was built from. + ds.getPoolProperties().setJdbcInterceptors(MALFORMED); + + Assertions.assertDoesNotThrow(() -> pool.close(true), + "a malformed interceptor list must not abort the pool close"); + Assertions.assertEquals(1, CloseCountingInterceptor.CLOSED.get(), + "the close must still notify the interceptor chain's definitions"); + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolInitializationCleanup.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolInitializationCleanup.java new file mode 100644 index 0000000000..524ff0eb5c --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPoolInitializationCleanup.java @@ -0,0 +1,123 @@ +/* + * 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.tomcat.jdbc.pool; + +import java.sql.SQLException; +import java.util.concurrent.atomic.AtomicInteger; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * A pool whose initialization fails after it has acquired resources - a + * started interceptor or the pre-loaded physical sessions - must tear those + * resources down before the constructor fails. + */ +class TestPoolInitializationCleanup extends JdbcPoolBaseTest { + + public static class StartCountingInterceptor extends JdbcInterceptor { + + static final AtomicInteger STARTED = new AtomicInteger(); + static final AtomicInteger CLOSED = new AtomicInteger(); + + @Override + public void reset(ConnectionPool parent, PooledConnection con) { + // NO-OP + } + + @Override + public void poolStarted(ConnectionPool parent) { + STARTED.incrementAndGet(); + } + + @Override + public void poolClosed(ConnectionPool parent) { + CLOSED.incrementAndGet(); + } + } + + public static class ErrorOnStartInterceptor extends JdbcInterceptor { + + @Override + public void reset(ConnectionPool parent, PooledConnection con) { + // NO-OP + } + + @Override + public void poolStarted(ConnectionPool parent) { + throw new LinkageError("boom from poolStarted()"); + } + } + + @Test + void nonFatalErrorFromPoolStartedClosesTheStartedInterceptors() throws Exception { + StartCountingInterceptor.STARTED.set(0); + StartCountingInterceptor.CLOSED.set(0); + DataSource ds = newDataSource(); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMinIdle(0); + ds.getPoolProperties().setJdbcInterceptors( + StartCountingInterceptor.class.getName() + ";" + ErrorOnStartInterceptor.class.getName()); + + Assertions.assertThrows(SQLException.class, ds::createPool, + "a non-fatal Error from an interceptor start must fail pool creation"); + Assertions.assertEquals(1, StartCountingInterceptor.STARTED.get(), + "the interceptor before the failing one must have started"); + Assertions.assertEquals(1, StartCountingInterceptor.CLOSED.get(), + "the failed start must close the interceptors it already started"); + awaitSessions(0); + } + + /** + * A pool whose cleaner cannot be armed after the initial pre-load has + * already established physical sessions. + */ + public static class CleanerFailingPool extends ConnectionPool { + + public CleanerFailingPool(PoolConfiguration properties) throws SQLException { + super(properties); + } + + @Override + public synchronized void initializePoolCleaner(PoolConfiguration properties) { + throw new IllegalStateException("boom from initializePoolCleaner()"); + } + } + + @Test + void failureToArmTheCleanerAfterThePreLoadClosesTheConnections() throws Exception { + PoolProperties properties = new PoolProperties(); + properties.setUrl(getUrl()); + properties.setDriverClassName("org.h2.Driver"); + properties.setUsername("sa"); + properties.setPassword(""); + properties.setJmxEnabled(false); + properties.setInitialSize(1); + properties.setMinIdle(0); + properties.setMaxActive(1); + properties.setTimeBetweenEvictionRunsMillis(-1); + + SQLException thrown = Assertions.assertThrows(SQLException.class, + () -> new CleanerFailingPool(properties), + "a cleaner failure after the pre-load must fail pool creation"); + Assertions.assertInstanceOf(IllegalStateException.class, thrown.getCause(), + "the cleaner failure must remain the cause"); + awaitSessions(0); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
