This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 11.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit e60a7a4360141e206471908ca164c31e616168d9 Author: opencode <[email protected]> AuthorDate: Thu Nov 5 23:15:18 2026 +0100 Reset validation on a new session and make PooledConnection logging safe A successful connect() kept the previous session's lastValidated timestamp, so a reconnect could skip testOnBorrow/testOnReturn validation for the interval remaining from the replaced session. Reset the timestamp when a new physical session is established, restore it if a credential reconnect fails, and document the attribute. PooledConnection.toString() interpolated the driver's own toString() into abandon, suspect, reconnect and return warnings. That text is unmasked and may contain credentials or newlines. Render the pooled connection's identity and connected state instead. --- .../apache/tomcat/jdbc/pool/PooledConnection.java | 35 +++++++++-- .../tomcat/jdbc/pool/PooledConnectionMBean.java | 15 ++--- .../jdbc/pool/TestPooledConnectionToString.java | 71 ++++++++++++++++++++++ .../apache/tomcat/jdbc/pool/TestValidation.java | 41 +++++++++++-- 4 files changed, 144 insertions(+), 18 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java index 1690d8de95..4cf1162252 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnection.java @@ -123,8 +123,8 @@ public class PooledConnection implements PooledConnectionMBean { private volatile long lastConnected = -1; /** * Timestamp of the last completed successful validation, used to keep track of validation intervals. Seeded with - * the time this pooled connection was created, so on a pool that runs no validation it never moves and cannot be - * told apart from a real validation at that instant. + * the time this pooled connection was created, and reset to 0 when a new physical session is established: a + * validation that completed on the session just replaced says nothing about the new one. */ private volatile long lastValidated = System.currentTimeMillis(); /** @@ -298,6 +298,11 @@ public class PooledConnection implements PooledConnectionMBean { } this.discarded = false; this.lastConnected = System.currentTimeMillis(); + // The validation timestamp belongs to the session that was just given + // up: a new physical session must be due for its own validation, or a + // recently validated predecessor lets the configured testOnBorrow / + // testOnReturn policy skip the check on this one. + this.lastValidated = 0; // The physical session is up: this is the point at which the // connection can publish itself below the pool's MBean. Tying that to @@ -661,7 +666,8 @@ public class PooledConnection implements PooledConnectionMBean { javax.sql.XAConnection servedXa = xaConnection; javax.sql.PooledConnection servedCpds = cpdsConnection; long servedLastConnected = lastConnected; - HashMap<Object,Object> sessionAttributes = getAttributes(); + long servedLastValidated = lastValidated; + HashMap<Object, Object> sessionAttributes = getAttributes(); boolean hadUser = sessionAttributes.containsKey(PROP_USER); Object servedUser = sessionAttributes.get(PROP_USER); boolean hadPassword = sessionAttributes.containsKey(PROP_PASSWORD); @@ -716,6 +722,7 @@ public class PooledConnection implements PooledConnectionMBean { xaConnection = servedXa; cpdsConnection = servedCpds; lastConnected = servedLastConnected; + lastValidated = servedLastValidated; if (hadUser) { sessionAttributes.put(PROP_USER, servedUser); } else { @@ -1474,12 +1481,19 @@ public class PooledConnection implements PooledConnectionMBean { } /** +<<<<<<< HEAD * Returns the timestamp at which the last successful validation completed. Until a validation has completed for the * first time, this is the time this pooled connection was created - which on a pool with no validation configured * is the value the attribute keeps. +======= + * Returns the timestamp at which the last successful validation completed on the current physical session. While + * the session has not been validated it reports 0, so a validation inherited from the session a reconnect replaced + * cannot be mistaken for one of this session. Before any session has been established, it reports the time this + * pooled connection was created. +>>>>>>> 52871c512d (Reset validation on a new session and make PooledConnection logging safe) * * @return the timestamp at which the last successful validation completed as defined by - * {@link System#currentTimeMillis()} + * {@link System#currentTimeMillis()}, or 0 while the current session has not been validated */ @Override public long getLastValidated() { @@ -1615,16 +1629,25 @@ public class PooledConnection implements PooledConnectionMBean { } /** +<<<<<<< HEAD * Returns a diagnostic rendering of this pooled connection: the driver's own {@link java.sql.Connection#toString()} * when a physical session is present, or {@code null}. The driver's text is not masked and may include the URL, the * user or other session details, so pool warnings that pass a pooled connection should treat it as best-effort * diagnostic output rather than as a credential-safe view. +======= + * Returns a diagnostic rendering of this pooled connection that is safe to interpolate into a pool log message: + * the connection's identity and whether it currently holds a physical session, never the driver's own + * {@link Connection#toString()}. That text is unmasked and may carry the URL, the user or other session details, + * and it may contain newlines, so passing it into a pool warning could expose a credential or forge log lines. A + * caller that wants the driver's text for an explicit dump can read the physical connection itself. +>>>>>>> 52871c512d (Reset validation on a new session and make PooledConnection logging safe) * - * @return the diagnostic rendering + * @return the credential-safe diagnostic rendering */ @Override public String toString() { - return "PooledConnection[" + (connection != null ? connection.toString() : "null") + "]"; + return "PooledConnection@" + Integer.toHexString(System.identityHashCode(this)) + + (connection != null ? "[connected]" : "[null]"); } /** diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnectionMBean.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnectionMBean.java index 78df8ad1d6..6f1882e545 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnectionMBean.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PooledConnectionMBean.java @@ -82,15 +82,16 @@ public interface PooledConnectionMBean { boolean isDiscarded(); /** - * Returns the timestamp at which the last successful validation - * completed. Until a validation has completed for the first time, this is - * the time this pooled connection was created - which on a pool with no - * validation configured is the value the attribute keeps, so a - * never-validated connection cannot be told apart from one validated at - * its creation instant. + * Returns the timestamp at which the last successful validation completed + * on the current physical session. While that session has not been + * validated it reports 0, so a validation inherited from the session a + * reconnect replaced cannot be mistaken for one of this session. Before + * any session has been established, it reports the time this pooled + * connection was created. * * @return the last validated timestamp as defined by - * {@link System#currentTimeMillis()} + * {@link System#currentTimeMillis()}, or 0 while the current + * session has not been validated */ long getLastValidated(); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPooledConnectionToString.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPooledConnectionToString.java new file mode 100644 index 0000000000..33520e0254 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestPooledConnectionToString.java @@ -0,0 +1,71 @@ +/* + * 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.lang.reflect.Field; +import java.lang.reflect.Proxy; +import java.sql.Connection; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +/** + * A pooled connection is interpolated into pool warnings on the abandon, + * suspect, reconnect and return paths. Its rendering must not use the driver's + * own {@code toString()}, whose text can carry a credential or a newline. + */ +class TestPooledConnectionToString { + + @Test + void theRenderingDoesNotUseTheDriverText() throws Exception { + PoolProperties properties = new PoolProperties(); + properties.setInitialSize(0); + properties.setJmxEnabled(false); + ConnectionPool pool = new ConnectionPool(properties); + try { + PooledConnection pooled = new PooledConnection(properties, pool); + Connection driverText = (Connection) Proxy.newProxyInstance( + Connection.class.getClassLoader(), new Class[] { Connection.class }, + (proxy, method, args) -> { + switch (method.getName()) { + case "toString": + return "jdbc:h2:mem:x;password=s3cret\nforged log line"; + case "hashCode": + return Integer.valueOf(System.identityHashCode(proxy)); + case "equals": + return Boolean.valueOf(proxy == args[0]); + default: + return null; + } + }); + Field connection = PooledConnection.class.getDeclaredField("connection"); + connection.setAccessible(true); + connection.set(pooled, driverText); + + String rendering = pooled.toString(); + + Assertions.assertFalse(rendering.contains("s3cret"), + "the rendering must not carry a credential from the driver's toString()"); + Assertions.assertFalse(rendering.contains("\n"), + "the rendering must not carry a newline that could forge a log line"); + Assertions.assertTrue(rendering.startsWith("PooledConnection@"), + "the rendering must identify the pooled connection itself, was: " + rendering); + } finally { + pool.close(true); + } + } +} diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestValidation.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestValidation.java index 209c052e45..e53ea9c160 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestValidation.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestValidation.java @@ -61,6 +61,35 @@ class TestValidation extends JdbcPoolBaseTest { return ((javax.sql.PooledConnection) pooled).getConnection(); } + @Test + void aReconnectedSessionIsDueForItsOwnValidation() throws Exception { + DataSource ds = newDataSource(); + ds.getPoolProperties().setTestOnBorrow(true); + ds.getPoolProperties().setValidationInterval(60000); + ds.getPoolProperties().setInitialSize(0); + CountingValidator validator = new CountingValidator(); + ds.getPoolProperties().setValidator(validator); + ds.createPool(); + + try (Connection pooled = ds.getConnection()) { + PooledConnection pc = pooled.unwrap(PooledConnection.class); + pc.setLastValidated(System.currentTimeMillis()); + int before = validator.actions.size(); + Assertions.assertTrue(pc.validate(PooledConnection.VALIDATE_BORROW, null), + "precondition: a recent validation must gate the check"); + Assertions.assertEquals(before, validator.actions.size(), + "precondition: the validation interval must skip the check"); + + pc.reconnect(); + + Assertions.assertTrue(pc.validate(PooledConnection.VALIDATE_BORROW, null), + "the new physical session must be validated"); + Assertions.assertTrue(validator.actions.size() > before, + "a validation recorded on the replaced session must not be inherited by" + + " the session the reconnect established"); + } + } + @Test void aFailingValidationQueryFailsTheBorrowAndLeaksNothing() throws Exception { DataSource ds = validatingDataSource(BROKEN_SQL); @@ -291,16 +320,18 @@ class TestValidation extends JdbcPoolBaseTest { c.createStatement().execute("SELECT 1"); } } - // The interval is measured from the last successful validation, - // which for a fresh physical connection is its creation. - Assertions.assertEquals(0, validator.actions.size(), - "within the validation interval no borrow may validate"); + // A new physical session is due for its first validation; the + // interval is measured from that successful check, so the two + // borrows that follow are suppressed. + Assertions.assertEquals(1, validator.actions.size(), + "the first borrow of a new session must validate, and the interval must" + + " suppress the repeated checks that follow"); ds.getPoolProperties().setValidationInterval(0); try (Connection c = ds.getConnection()) { c.createStatement().execute("SELECT 1"); } - Assertions.assertEquals(1, validator.actions.size(), + Assertions.assertEquals(2, validator.actions.size(), "with the interval switched off the borrow must validate"); } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
