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 d1d4d203649767289068ea4d0f713ac6dd2b7040 Author: opencode <[email protected]> AuthorDate: Fri Oct 30 18:31:20 2026 +0100 Let ProxyConnection's Wrapper answers agree with each other ProxyConnection.isWrapperFor() answered false for org.apache.tomcat.jdbc.pool.PooledConnection while ProxyConnection.unwrap() handed out that very object, so the two halves of the java.sql.Wrapper contract contradicted each other for the same receiver: isWrapperFor is what a caller is told to consult before unwrapping, and it predicted a failure the call would not have. The same pair disagreed in the corner where the interceptor has been released and holds no pooled connection at all, where unwrap() returned a silent null that the caller then used as if it were a connection. isWrapperFor() now reports whether a pooled connection is actually held, and unwrap() answers the same not-a-wrapper SQLException for that class when none is held, the shape the XA connection case has always used. Held and unheld now each give a consistent pair, and the released state no longer hands back a null that surfaces as an unrelated null pointer at the caller's first use. The pooled connection that unwrap() does return for that class is the pool's own internal object, able to close and discard the connection behind the pool's accounting, so it is now documented as meant for trusted callers such as interceptors rather than for application code. Two claims in the report this item came from did not survive verification and are worth recording. The public unwrap() and the invoke handler's unwrap branch were described as giving different answers for the internal class, but the handler's short-circuit tests iface.isInstance(proxy), which is false for that concrete class, so both entry points already answered identically; the report had conflated the pool's PooledConnection with the JDBC one. That JDBC interface is the one argument the two receivers still answer differently, and they must: the connection handle is a proxy for it and the interceptor is not, so the contract asks each object about its own wrappers. Making the handler's branch answer the internal object as well, which the report also raised as the alignment to perform, would have undone the decision recorded in abfc0bce98, which deliberately kept that idiom with a control test, and would have broken the twenty-two places in this module that use it while adding no protection, since the handle already hands out the raw physical session and in-process application code is trusted per the project's security model. Extend TestWrapperContract to pin the identity of the internal answer across both entry points, the XA answer across both, the plain pool's refusal of XA across both, the answers that legitimately differ per receiver, and the refusal pair once the interceptor holds nothing. No changelog entry, following abfc0bce98, which changed these answers in the same direction without one; if the project ever documents this series it should be a single entry covering the whole evolution of these answers rather than one per step. --- .../apache/tomcat/jdbc/pool/ProxyConnection.java | 22 ++- .../tomcat/jdbc/pool/TestWrapperContract.java | 167 +++++++++++++++++++-- 2 files changed, 177 insertions(+), 12 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ProxyConnection.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ProxyConnection.java index de8f26cc13..c58ad67c6d 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ProxyConnection.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ProxyConnection.java @@ -111,6 +111,13 @@ public class ProxyConnection extends JdbcInterceptor { * @return true if this interceptor wraps the given interface */ public boolean isWrapperFor(Class<?> iface) { + if (iface == PooledConnection.class) { + // The Wrapper answer must agree with what unwrap() hands out: + // for this class unwrap() returns the pooled connection this + // interceptor sits on, so the honest answer is whether one is + // still held. That object is pool-internal - see unwrap(). + return connection != null; + } if (iface == XAConnection.class && connection.getXAConnection() != null) { return true; } @@ -147,12 +154,25 @@ public class ProxyConnection extends JdbcInterceptor { * * @param iface the interface to unwrap to * - * @return the unwrapped connection + * @return the unwrapped connection. For {@link PooledConnection} this is + * the pool's own pooled connection - a pool-internal object meant + * for trusted callers such as interceptors, not for application + * code, as it can close and discard the connection behind the + * pool's own accounting. * * @throws SQLException if the connection does not wrap the given interface */ public Object unwrap(Class<?> iface) throws SQLException { if (iface == PooledConnection.class) { + if (connection == null) { + // The same not-a-wrapper failure the XA branch below gives + // for a missing XA connection: with none held there is + // nothing to hand out, and the silent null would contradict + // what isWrapperFor() says and turn into an unrelated + // NullPointerException at the caller's first use of the + // "unwrapped" value. + throw new SQLException("Not a wrapper of " + iface.getName()); + } return connection; } else if (iface == XAConnection.class) { javax.sql.XAConnection xa = connection.getXAConnection(); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestWrapperContract.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestWrapperContract.java index 3faacbc5eb..560a96a8f0 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestWrapperContract.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestWrapperContract.java @@ -36,9 +36,35 @@ import org.apache.tomcat.jdbc.JdbcPoolBaseTest; * {@link javax.sql.XAConnection} on XA pools), while unwrapping interfaces * the physical driver connection provides keeps reaching the physical * connection. + * + * <p>The pool's own {@link org.apache.tomcat.jdbc.pool.PooledConnection} is + * answered the same way through both entry points of the chain - the handle's + * dispatch and the bottom interceptor's own public methods - and there + * {@code isWrapperFor} must agree with what {@code unwrap} hands out. Once the + * interceptor holds no pooled connection at all, the pair for that class is + * the refusal: {@code isWrapperFor} says false and {@code unwrap} answers the + * not-a-wrapper failure it predicts, rather than handing back a silent null. + * The one answer the two entry points legitimately differ on is + * {@link javax.sql.PooledConnection}, which the handle implements and the + * interceptor does not: the contract asks each object about its own wrappers, + * so both answers are pinned as the correct per-object pair.</p> */ class TestWrapperContract extends JdbcPoolBaseTest { + private XADataSource newXaDataSource() { + org.h2.jdbcx.JdbcDataSource inner = new org.h2.jdbcx.JdbcDataSource(); + inner.setUrl(getUrl()); + inner.setUser("sa"); + inner.setPassword(""); + XADataSource ds = new XADataSource(); + ds.setDriverClassName("org.h2.Driver"); + ds.getPoolProperties().setJmxEnabled(false); + ds.getPoolProperties().setInitialSize(1); + ds.getPoolProperties().setMinIdle(0); + ds.getPoolProperties().setDataSource(inner); + return manage(ds); + } + @Test void unwrapToXaConnectionOnAPlainPoolFailsAsNotAWrapperNotSilentlyReturnsNull() throws Exception { @@ -117,17 +143,7 @@ class TestWrapperContract extends JdbcPoolBaseTest { @Test void anXaPoolStillUnwrapsToTheDriversXaConnection() throws Exception { - org.h2.jdbcx.JdbcDataSource inner = new org.h2.jdbcx.JdbcDataSource(); - inner.setUrl(getUrl()); - inner.setUser("sa"); - inner.setPassword(""); - XADataSource ds = new XADataSource(); - ds.setDriverClassName("org.h2.Driver"); - ds.getPoolProperties().setJmxEnabled(false); - ds.getPoolProperties().setInitialSize(1); - ds.getPoolProperties().setMinIdle(0); - ds.getPoolProperties().setDataSource(inner); - manage(ds); + XADataSource ds = newXaDataSource(); ds.createPool(); try (Connection pooled = ds.getConnection()) { @@ -141,4 +157,133 @@ class TestWrapperContract extends JdbcPoolBaseTest { Assertions.assertNotSame(pooled, unwrapped); } } + + @Test + void theInternalPooledConnectionIsAnsweredIdenticallyByBothEntryPoints() + throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + ProxyConnection interceptor = interceptorOf(pooled, ProxyConnection.class); + // What the handle's dispatch answers for the pool's own pooled + // connection, the bottom interceptor's public method answers + // too - one object on both entry points, pinning the + // consistency the two entry points of the chain must keep. + Object viaHandle = pooled.unwrap( + org.apache.tomcat.jdbc.pool.PooledConnection.class); + Object viaInterceptor = interceptor.unwrap( + org.apache.tomcat.jdbc.pool.PooledConnection.class); + Assertions.assertSame(viaInterceptor, viaHandle, + "the handle and the interceptor must answer the same object"); + Assertions.assertSame(interceptor.getConnection(), viaHandle, + "the answer both entry points give is the pool's own pooled connection"); + // The Wrapper contract: isWrapperFor must tell the truth about + // what unwrap hands out. Both objects sit on - are indirectly a + // wrapper of - the pooled connection their unwrap returns, so + // both must answer true, not the false that contradicted unwrap. + Assertions.assertTrue( + pooled.isWrapperFor(org.apache.tomcat.jdbc.pool.PooledConnection.class), + "isWrapperFor must agree with the unwrap the handle answers"); + Assertions.assertTrue( + interceptor.isWrapperFor(org.apache.tomcat.jdbc.pool.PooledConnection.class), + "isWrapperFor must agree with the unwrap the interceptor answers"); + } + } + + @Test + void theUnheldPooledConnectionIsRefusedAsNotAWrapperByTheInterceptor() + throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + Connection pooled = ds.getConnection(); + // Taken while the chain is intact: after the close the disposable + // facade seals and no longer forwards to the interceptor. + ProxyConnection interceptor = interceptorOf(pooled, ProxyConnection.class); + pooled.close(); + // The released interceptor holds no pooled connection, so + // isWrapperFor for that class says false - and the same query pair + // must hold in this corner as while it is held: unwrap answers the + // not-a-wrapper failure isWrapperFor predicts, the way the + // XAConnection case has always done, not the silent null that + // contradicts the refusal and surfaces as an unrelated + // NullPointerException at the caller's first use. + Assertions.assertFalse( + interceptor.isWrapperFor(org.apache.tomcat.jdbc.pool.PooledConnection.class), + "an interceptor holding nothing is not a wrapper of it"); + SQLException thrown = Assertions.assertThrows(SQLException.class, + () -> interceptor.unwrap(org.apache.tomcat.jdbc.pool.PooledConnection.class), + "unwrap must answer the not-a-wrapper failure isWrapperFor " + + "predicts, not hand back a silent null"); + Assertions.assertTrue(thrown.getMessage().contains("Not a wrapper"), + "the failure must say it is a not-a-wrapper case: " + + thrown.getMessage()); + } + + @Test + void theJavaxPooledConnectionAnswerFollowsEachObjectsOwnInterfaces() + throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + ProxyConnection interceptor = interceptorOf(pooled, ProxyConnection.class); + // The handle implements javax.sql.PooledConnection, so per the + // contract it is a wrapper of it and unwraps to itself. + Assertions.assertTrue(pooled.isWrapperFor(javax.sql.PooledConnection.class)); + Assertions.assertSame(pooled, pooled.unwrap(javax.sql.PooledConnection.class)); + // The interceptor is no such interface, and nothing it can reach + // implements one - neither the pool's own pooled connection nor + // the physical driver connection does - so its honest answer is + // the not-a-wrapper pair. The entry points differ here because + // they are different objects and the contract asks each object + // about its own wrappers; pin that no realignment turns either + // answer into a lie. + Assertions.assertFalse( + interceptor.isWrapperFor(javax.sql.PooledConnection.class), + "the interceptor implements no javax.sql.PooledConnection"); + Assertions.assertThrows(SQLException.class, + () -> interceptor.unwrap(javax.sql.PooledConnection.class), + "unwrapping to an interface nothing behind the interceptor " + + "provides must fail, not hand back a foreign object"); + } + } + + @Test + void theXaConnectionIsAnsweredIdenticallyByBothEntryPoints() throws Exception { + XADataSource ds = newXaDataSource(); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + ProxyConnection interceptor = interceptorOf(pooled, ProxyConnection.class); + Assertions.assertTrue(pooled.isWrapperFor(XAConnection.class)); + Assertions.assertTrue(interceptor.isWrapperFor(XAConnection.class)); + Object viaHandle = pooled.unwrap(XAConnection.class); + Object viaInterceptor = interceptor.unwrap(XAConnection.class); + Assertions.assertSame(viaInterceptor, viaHandle, + "both entry points must answer with the same XA connection"); + Assertions.assertNotSame(pooled, viaHandle, + "the XA answer is the driver's XA connection object, not the handle"); + } + } + + @Test + void thePlainPoolRefusalOfXaIsIdenticalThroughBothEntryPoints() throws Exception { + DataSource ds = newDataSource(); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + ProxyConnection interceptor = interceptorOf(pooled, ProxyConnection.class); + // A plain pool refuses this class the same way through both + // entry points, with isWrapperFor agreeing - pinning that the + // pooled-connection answer above did not blur the XA boundary. + Assertions.assertFalse(pooled.isWrapperFor(XAConnection.class)); + Assertions.assertFalse(interceptor.isWrapperFor(XAConnection.class)); + Assertions.assertThrows(SQLException.class, + () -> pooled.unwrap(XAConnection.class)); + Assertions.assertThrows(SQLException.class, + () -> interceptor.unwrap(XAConnection.class)); + } + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
