This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 069d24f848a7fba8aebc748171355586d721f3c4 Author: opencode <[email protected]> AuthorDate: Tue Oct 27 10:29:12 2026 +0100 Report the pooled connection from a statement the pool proxied The statement facade that is on by default named only the object methods, close/isClosed and the result-set accessors, and forwarded everything else - including java.sql.Statement.getConnection() - reflectively to its delegate. With no decorator interceptor configured that delegate is the raw driver statement, so the application was handed the driver's own connection for the borrow the statement came from. That handle was worse than an unlabelled one. It stayed usable after the connection had been returned and re-borrowed by someone else, so its holder read and wrote another component's transaction and could close a session the pool believed it owned, and every statement created through it arrived at none of the interceptors that were configured for the pool it came from - not StatementFinalizer's documented close-on-return, not a statement cache. The pool already had a component that answered this call correctly, the StatementDecoratorInterceptor whose documented purpose since BZ 48392 is to prevent exactly this access; the facade received the very object it needed and discarded it. The facade now answers getConnection() with the connection proxy it is handed when the statement is created, which is the handle the application borrowed and what the JDBC specification says the call returns. The answer is placed after the closed-statement guard, so a closed statement still throws as java.sql.Statement specifies, matching the order the decorator uses. unwrap() deliberately keeps delegating, so the driver statement remains reachable. A statement kept past its borrow now reports the inert handle it came from instead of the live session. --- modules/jdbc-pool/doc/jdbc-pool.xml | 3 + .../apache/tomcat/jdbc/pool/PoolConfiguration.java | 7 +- .../apache/tomcat/jdbc/pool/StatementFacade.java | 24 ++- .../jdbc/pool/TestStatementConnectionHandle.java | 214 +++++++++++++++++++++ 4 files changed, 244 insertions(+), 4 deletions(-) diff --git a/modules/jdbc-pool/doc/jdbc-pool.xml b/modules/jdbc-pool/doc/jdbc-pool.xml index 228ed67456..68e2e9be28 100644 --- a/modules/jdbc-pool/doc/jdbc-pool.xml +++ b/modules/jdbc-pool/doc/jdbc-pool.xml @@ -575,6 +575,9 @@ <p>(boolean) Set this to true if you wish to wrap statements in order to enable <code>equals()</code> and <code>hashCode()</code> methods to be called on the closed statements if any statement proxy is set. + A wrapped statement also reports the pooled connection it was created + from when <code>Statement.getConnection()</code> is called, rather than + the driver's own connection. Default value is <code>true</code>. </p> </attribute> diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java index 3ca87d9831..3926e13fd0 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolConfiguration.java @@ -924,7 +924,9 @@ public interface PoolConfiguration { /** * Set this to true if you wish to wrap statements in order to enable equals() and hashCode() - * methods to be called on the closed statements if any statement proxy is set. + * methods to be called on the closed statements if any statement proxy is set. A wrapped + * statement also reports the pooled connection it was created from rather than the driver's + * own connection. * @param useStatementFacade set to <code>true</code> to wrap statements */ void setUseStatementFacade(boolean useStatementFacade); @@ -932,7 +934,8 @@ public interface PoolConfiguration { /** * Returns <code>true</code> if this connection pool is configured to wrap statements in order * to enable equals() and hashCode() methods to be called on the closed statements if any - * statement proxy is set. + * statement proxy is set. A wrapped statement also reports the pooled connection it was + * created from rather than the driver's own connection. * @return <code>true</code> if the statements are wrapped */ boolean getUseStatementFacade(); diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/StatementFacade.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/StatementFacade.java index 2ea918d2f8..1ff14b4dc5 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/StatementFacade.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/StatementFacade.java @@ -76,7 +76,7 @@ public class StatementFacade extends AbstractCreateStatementInterceptor { // do nothing return statement; } - return constructor.newInstance(new Object[] { new StatementProxy(statement, sql) }); + return constructor.newInstance(new Object[] { new StatementProxy(statement, sql, proxy) }); } catch (Exception x) { logger.warn("Unable to create statement proxy.", x); } @@ -99,15 +99,26 @@ public class StatementFacade extends AbstractCreateStatementInterceptor { * The SQL query text. */ protected final String query; + /** + * The pooled connection the statement was created from, as reported + * by {@link java.sql.Statement#getConnection()}. It is the handle the + * application borrowed, so it stops working once that borrow has been + * returned, while the driver's own connection would have gone on + * serving whoever borrowed the connection next. + */ + protected final Object connection; /** * Constructs a StatementProxy. * @param parent the parent statement * @param query the SQL query text + * @param connection the pooled connection the statement was created + * from */ - public StatementProxy(Object parent, String query) { + public StatementProxy(Object parent, String query, Object connection) { this.delegate = parent; this.query = query; + this.connection = connection; } @Override @@ -137,6 +148,15 @@ public class StatementFacade extends AbstractCreateStatementInterceptor { if (delegate == null) { throw new SQLException("Statement closed."); } + // The connection a statement reports is the pooled handle the + // application created it from, not the driver's own connection. + // Forwarding the call instead hands the application a handle on + // the physical session: it survives the borrow it came from and + // bypasses every interceptor, so statements made through it + // escape the close-on-return tracking and any statement cache. + if (compare(GETCONNECTION_VAL, method)) { + return connection; + } Object result; try { diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestStatementConnectionHandle.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestStatementConnectionHandle.java new file mode 100644 index 0000000000..fed7ca4736 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestStatementConnectionHandle.java @@ -0,0 +1,214 @@ +/* + * 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.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; +import org.apache.tomcat.jdbc.pool.interceptor.StatementDecoratorInterceptor; +import org.apache.tomcat.jdbc.pool.interceptor.StatementFinalizer; + +/** + * What a pooled statement reports as its connection. + * + * <p>{@link java.sql.Statement#getConnection()} is specified to return the + * {@link Connection} that produced the statement, which for a pooled + * statement is the handle the application borrowed - not the driver's own + * connection. Handing out the driver object instead lets an application + * reach a physical session the pool no longer owns: it can close it, or + * keep creating statements on it after the handle was returned, and those + * statements then escape the interceptors that were configured for them + * ({@link StatementFinalizer}'s close-on-return, a statement cache).</p> + * + * <p>{@link StatementDecoratorInterceptor} has always answered this call + * with the pooled handle; {@link StatementFacade}, which is on by default, + * did not.</p> + */ +class TestStatementConnectionHandle extends JdbcPoolBaseTest { + + @Test + void aStatementReportsThePooledHandleThatCreatedIt() throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + try (Statement plain = pooled.createStatement()) { + Assertions.assertSame(pooled, plain.getConnection(), + "createStatement() must yield a statement that reports the borrowed handle"); + } + try (Statement prepared = pooled.prepareStatement("SELECT 1")) { + Assertions.assertSame(pooled, prepared.getConnection(), + "prepareStatement() must yield a statement that reports the borrowed handle"); + } + try (Statement callable = pooled.prepareCall("CALL 1")) { + Assertions.assertSame(pooled, callable.getConnection(), + "prepareCall() must yield a statement that reports the borrowed handle"); + } + // The route BZ 48392 was about: a handle reached through a result + // set rather than held directly. + try (Statement st = pooled.createStatement(); + ResultSet rs = st.executeQuery("SELECT 1")) { + Assertions.assertSame(pooled, rs.getStatement().getConnection(), + "a connection reached through a result set's statement must be the " + + "borrowed handle as well"); + } + } + } + + @Test + void aReportedConnectionIsThePoolHandleAndNotTheDriverConnection() throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + try (Connection pooled = ds.getConnection(); + Statement st = pooled.createStatement()) { + Connection reported = st.getConnection(); + Assertions.assertFalse(reported.getClass().getName().startsWith("org.h2."), + "the physical driver connection must not reach the application: " + + reported.getClass().getName()); + Assertions.assertSame(pooled, reported, + "the reported connection must be the very handle that was borrowed"); + } + } + + /** + * The consequence that makes the leak an availability problem rather + * than a cosmetic one: statements created through the handle a statement + * reports are the ones {@code StatementFinalizer} is documented to close + * when the connection goes back. + */ + @Test + void statementsCreatedThroughAReportedConnectionAreClosedOnReturn() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors(StatementFinalizer.class.getName()); + ds.createPool(); + + Statement escaped; + Connection pooled = ds.getConnection(); + try (Statement st = pooled.createStatement()) { + escaped = st.getConnection().createStatement(); + } + pooled.close(); + + Assertions.assertTrue(escaped.isClosed(), + "a statement created through the connection a pooled statement reports " + + "must be closed when that connection is returned"); + } + + /** + * What the change is for: the handle a statement reports belongs to the + * borrow it came from, so it stops working when that borrow ends. The + * driver connection the facade used to hand out kept serving whoever + * held the physical session next. + */ + @Test + void aReportedConnectionStopsWorkingWhenItsBorrowEnds() throws Exception { + DataSource ds = newDataSource(); + // force the next borrow onto the same physical session + ds.getPoolProperties().setMaxActive(1); + ds.getPoolProperties().setMaxIdle(1); + ds.createPool(); + + Connection first = ds.getConnection(); + setSessionMarker(first, 4242); + Statement survivor = first.createStatement(); + Connection reported = survivor.getConnection(); + Assertions.assertSame(first, reported); + first.close(); + + Connection second = ds.getConnection(); + Assertions.assertEquals(Integer.valueOf(4242), sessionMarker(second), + "the next borrower holds the very session the reported connection came from"); + + Assertions.assertTrue(reported.isClosed(), + "the reported handle must report itself closed once its borrow ended"); + Assertions.assertThrows(SQLException.class, reported::getAutoCommit, + "the reported connection must not serve the next borrower"); + survivor.close(); + second.close(); + } + + /** + * The facade answers {@code getConnection()} from the handle it was + * given, so the answer survives the statement being closed only as far + * as the JDBC specification allows: a closed statement still throws. + */ + @Test + void aClosedStatementStillReportsNoConnection() throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + Statement st = pooled.createStatement(); + st.close(); + Assertions.assertThrows(SQLException.class, st::getConnection, + "getConnection() on a closed statement must throw, as java.sql.Statement specifies"); + } + } + + /** + * {@code unwrap()} is the documented way to reach the driver's object + * and is deliberately left delegating to the driver. + */ + @Test + void unwrapStillReachesTheDriverStatement() throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + try (Connection pooled = ds.getConnection(); + Statement st = pooled.createStatement()) { + Assertions.assertTrue(st.unwrap(Statement.class).getClass().getName().startsWith("org.h2."), + "unwrap() must still reach the driver statement: " + + st.unwrap(Statement.class).getClass().getName()); + } + } + + /** + * The same contract with either statement wrapper in the chain, so the + * answer does not depend on an unrelated interceptor being configured. + */ + @Test + void theDecoratorAndTheFacadeAnswerTheSameWay() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setJdbcInterceptors( + StatementDecoratorInterceptor.class.getSimpleName()); + ds.createPool(); + + try (Connection pooled = ds.getConnection(); + Statement st = pooled.createStatement()) { + Assertions.assertSame(pooled, st.getConnection(), + "with the decorator configured the reported connection must stay the pooled handle"); + } + + DataSource unfacaded = newDataSource(); + unfacaded.getPoolProperties().setUseStatementFacade(false); + unfacaded.getPoolProperties().setJdbcInterceptors( + StatementDecoratorInterceptor.class.getSimpleName()); + unfacaded.createPool(); + try (Connection pooled = unfacaded.getConnection(); + Statement st = pooled.createStatement()) { + Assertions.assertSame(pooled, st.getConnection(), + "the decorator alone must keep answering with the pooled handle"); + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
