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 b0d4b55121c41f446def10faf1dd5fc6936468c3 Author: opencode <[email protected]> AuthorDate: Fri Oct 30 08:43:47 2026 +0100 Reject constructor cache requests the cached proxy cannot satisfy The statement proxy constructor cache in AbstractCreateStatementInterceptor is a protected static array on a public class, shared by every subclass loaded through the same class loader, but getConstructor(idx, clazz) consulted the requested interface only when populating an empty slot: a subclass that re-requested an already populated index with a different interface silently received the constructor of the proxy implementing the first caller's interface, and the failure surfaced later as a ClassCastException against the application or as a silently degraded, un-proxied statement. Every use of an index must pass an interface the cached proxy implements (in-tree, always the same one); a cache hit that cannot satisfy the request now throws an IllegalStateException naming the index, the cached interface and the requested one, leaving the cache untouched. The class comment, which claimed the constructor was not exposed outside jdbc-pool, now states the real subclass reachability and the pairing contract, and a new TestAbstractCreateStatementInterceptor pins the guard. --- .../AbstractCreateStatementInterceptor.java | 25 +++- .../TestAbstractCreateStatementInterceptor.java | 126 +++++++++++++++++++++ 2 files changed, 147 insertions(+), 4 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractCreateStatementInterceptor.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractCreateStatementInterceptor.java index deef36aa26..7fb5d33906 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractCreateStatementInterceptor.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/AbstractCreateStatementInterceptor.java @@ -22,6 +22,7 @@ import java.lang.reflect.Method; import java.lang.reflect.Proxy; import java.sql.ResultSet; import java.sql.Statement; +import java.util.Arrays; import org.apache.tomcat.jdbc.pool.ConnectionPool; import org.apache.tomcat.jdbc.pool.JdbcInterceptor; @@ -120,7 +121,11 @@ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor EXECUTE_LARGE_UPDATE, EXECUTE_LARGE_BATCH }; /** - * the constructors that are used to create statement proxies + * the constructors that are used to create statement proxies. The cache is + * shared by all subclasses loaded through the same class loader, so every + * use of an index must pass an interface that the cached proxy implements, + * in practice always the same one; {@link #getConstructor(int, Class)} + * enforces this. */ protected static final Constructor<?>[] constructors = new Constructor[STATEMENT_TYPE_COUNT + 1]; @@ -159,11 +164,18 @@ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor * @return - returns a constructor used to create new instances * * @throws NoSuchMethodException Constructor not found + * @throws IllegalStateException The index is already cached for a proxy class that does not implement + * {@code clazz}. Every use of an index in this shared cache must pass + * an interface that the cached proxy implements. */ /* - * Neither the class nor the constructor are exposed outside of jdbc-pool. Given the comments in the jdbc-pool code - * regarding caching for performance, continue to use Proxy.getProxyClass(). This will need to be revisited if that - * method is marked for removal. + * The cache, this method and the class itself are all reachable from subclasses, in-tree or third party. The + * cache is shared by every subclass loaded through the same class loader, so each index must always be + * paired with an interface that the cached proxy implements, in practice always the same one; a cache hit + * whose cached proxy does not implement the requested interface is rejected below rather than silently + * returning a proxy that does not implement what the caller asked for. Given the comments in the jdbc-pool + * code regarding caching for performance, continue to use Proxy.getProxyClass(). This will need to be + * revisited if that method is marked for removal. */ @SuppressWarnings("deprecation") protected Constructor<?> getConstructor(int idx, Class<?> clazz) throws NoSuchMethodException { @@ -171,6 +183,11 @@ public abstract class AbstractCreateStatementInterceptor extends JdbcInterceptor Class<?> proxyClass = Proxy.getProxyClass(AbstractCreateStatementInterceptor.class.getClassLoader(), new Class[] { clazz }); constructors[idx] = proxyClass.getConstructor(new Class[] { InvocationHandler.class }); + } else if (!clazz.isAssignableFrom(constructors[idx].getDeclaringClass())) { + throw new IllegalStateException("Constructor cache index " + idx + " is already populated for " + + Arrays.toString(constructors[idx].getDeclaringClass().getInterfaces()) + ", but " + + clazz.getName() + " was requested. Every use of an index must pass an interface the cached" + + " proxy implements."); } return constructors[idx]; } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestAbstractCreateStatementInterceptor.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestAbstractCreateStatementInterceptor.java new file mode 100644 index 0000000000..d0e51e757d --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/interceptor/TestAbstractCreateStatementInterceptor.java @@ -0,0 +1,126 @@ +/* + * 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.interceptor; + +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationHandler; +import java.lang.reflect.Method; +import java.sql.CallableStatement; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.Statement; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +/** + * The contract of the constructor cache that every subclass of + * {@code AbstractCreateStatementInterceptor} shares in the JVM: an index + * is a slot for one interface, and every use of that index must pass that + * interface. Before the guard was added, a subclass that paired an already + * populated index with a different interface silently got the constructor + * of the proxy implementing the first caller's interface; the resulting + * proxy did not implement what the caller asked for and the failure only + * surfaced later, as a {@code ClassCastException} against the application + * or as a silently degraded, unproxied statement. + * + * <p>The tests below only ever populate an index with the interface every + * in-tree call site pairs with it, and they leave the cache in that state, + * so running them in the same JVM as the rest of the suite - and in any + * method order - is safe.</p> + */ +class TestAbstractCreateStatementInterceptor { + + /* A minimal concrete interceptor; only the cache is exercised. */ + private static class CacheUser extends AbstractCreateStatementInterceptor { + + @Override + public Object createStatement(Object proxy, Method method, Object[] args, Object statement, long time) { + return statement; + } + + @Override + public void closeInvoked() { + // NOOP + } + } + + /* A proxy built by the constructor the cache hands out. */ + private static Object newProxy(CacheUser user, int index, Class<?> iface) throws Exception { + Constructor<?> constructor = user.getConstructor(index, iface); + InvocationHandler handler = (proxy, method, args) -> null; + return constructor.newInstance(new Object[] { handler }); + } + + @Test + void theCanonicalPairingsAreCachedAndSharedAcrossInterceptorInstances() throws Exception { + CacheUser first = new CacheUser(); + CacheUser second = new CacheUser(); + + Class<?>[] canonical = { Statement.class, PreparedStatement.class, CallableStatement.class, ResultSet.class }; + for (int idx = 0; idx < canonical.length; idx++) { + Constructor<?> cached = first.getConstructor(idx, canonical[idx]); + Assertions.assertSame(cached, second.getConstructor(idx, canonical[idx]), + "an interceptor instance must reuse the constructor cached for index " + idx); + Object proxy = newProxy(second, idx, canonical[idx]); + Assertions.assertTrue(canonical[idx].isInstance(proxy), + "the cached constructor must produce a proxy implementing " + canonical[idx].getName()); + } + } + + @Test + void requestingAPopulatedIndexWithADifferentInterfaceFailsLoudly() throws Exception { + CacheUser interceptor = new CacheUser(); + int idx = AbstractCreateStatementInterceptor.CREATE_STATEMENT_IDX; + + // Populate the index the way every in-tree call site does. + Constructor<?> cached = interceptor.getConstructor(idx, Statement.class); + + IllegalStateException failure = Assertions.assertThrows(IllegalStateException.class, + () -> interceptor.getConstructor(idx, PreparedStatement.class), + "a mismatched interface for a populated index must not return the cached constructor"); + + // The message must name the index, the cached interface and the + // requested one, so the offending pairing is identifiable. + Assertions.assertTrue(failure.getMessage().contains("index " + idx), + "the message must name the index: " + failure.getMessage()); + Assertions.assertTrue(failure.getMessage().contains("java.sql.Statement"), + "the message must name the cached interface: " + failure.getMessage()); + Assertions.assertTrue(failure.getMessage().contains("java.sql.PreparedStatement"), + "the message must name the requested interface: " + failure.getMessage()); + + // The rejection must leave the cached entry untouched, so a caller + // with the correct pairing is unaffected. + Assertions.assertSame(cached, AbstractCreateStatementInterceptor.constructors[idx]); + Assertions.assertSame(cached, interceptor.getConstructor(idx, Statement.class)); + } + + @Test + void aRequestForAnInterfaceTheCachedProxyAlsoImplementsIsAccepted() throws Exception { + CacheUser interceptor = new CacheUser(); + int idx = AbstractCreateStatementInterceptor.PREPARE_STATEMENT_IDX; + + // Populate the index with the canonical interface first. + Constructor<?> cached = interceptor.getConstructor(idx, PreparedStatement.class); + + // A PreparedStatement proxy is also a Statement, so widening the + // request to a superinterface of the cached interface is safe and + // must be accepted. + Assertions.assertSame(cached, interceptor.getConstructor(idx, Statement.class)); + Assertions.assertTrue(newProxy(interceptor, idx, Statement.class) instanceof Statement); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
