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 da4121a74d921aa13148a6da2cde33e75a12fb3e Author: opencode <[email protected]> AuthorDate: Tue Nov 3 09:10:10 2026 +0100 Read the driver properties under the monitor that publishes them The properties handed to the JDBC driver are a copy of the pool's live dbProperties collection, and setConnectionProperties reconfigures that collection in place - it removes the keys the previous string contributed and writes the new ones, as do the credential setters with the user and password entries it mirrors. Those publications are atomic only against readers that hold the configuration's monitor, which the reconfiguring setters themselves take, while the connect path took none: building the copy observed the live collection three times (putAll, the stringPropertyNames snapshot, then a per-key getProperty re-read), so a reconfigure landing between the observations was copied as the union of two published generations. The driver then started with a setting the operator had just removed next to the settings that replaced it - and, when the credential travels in the connection-properties string rather than in the credential fields that are overlaid after the copy, with a user/password pair no generation ever published together, a connection doomed to an authentication failure. The pair is only ever mixed between consecutive generations, never invented, and a rotation concurrent with the rare pool-growing connect is a narrow window, so this is a correctness rather than a security defect. The copy is now taken under the configuration's monitor, for the same reason PoolProperties#clone() synchronizes, and the monitor is a leaf, so taking it cannot order against any other lock. The alternative of calling PoolProperties#clone() was ruled out by the type of the field: the clone is protected, the field is the PoolConfiguration interface the public API accepts any implementation of, and reaching the clone would force a cast plus a second, unsynchronized copy path for implementations other than PoolProperties - a strict superset of the single path now taken. PoolProperties' class javadoc declares the monitor as the snapshot boundary of the configuration, so the convention this relies on is owned by the class rather than observed from outside. Found by a clean-room review of PoolUtilities, which pinned the torn copy deterministically and measured a 5.1% mixed-pair rate under rotation. The new test forces a real setConnectionProperties publication into the middle of the copy through a one-shot hook in the collection: without the fix the copy comes back the union of both generations, with it the driver is handed one generation whole and the publication lands intact right after. --- .../apache/tomcat/jdbc/pool/PoolProperties.java | 7 + .../apache/tomcat/jdbc/pool/PooledConnection.java | 21 +- .../pool/TestReconfigureVsConnectProperties.java | 257 +++++++++++++++++++++ 3 files changed, 282 insertions(+), 3 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java index 9255ace2a9..5765e79f06 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/PoolProperties.java @@ -42,6 +42,13 @@ import org.apache.tomcat.jdbc.pool.util.MessageFactory; /** * Implementation of {@link PoolConfiguration} that holds the configuration properties for a connection pool. + * <p> + * The reconfiguring setters publish their changes under this instance's monitor, which makes that monitor the + * snapshot boundary of the configuration: {@link #clone()} takes its copy under it, and so does the connect path + * when it copies the live {@link #getDbProperties()} collection for the driver. A copy taken under the monitor + * therefore always shows one state some setter published, never a mix of two. Setters that reconfigure in place + * must keep acquiring this monitor, and a reader that needs a consistent snapshot rather than the momentary view + * the unsynchronized getters hand out must take the monitor too. */ public class PoolProperties implements PoolConfiguration, Cloneable, Serializable { 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 9161226119..3d42c388c5 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 @@ -505,9 +505,24 @@ public class PooledConnection implements PooledConnectionMBean { // driver properties. That is not a configuration that cannot connect: // the URL and the credential below are enough for the driver, so the // absent collection becomes an empty one rather than a failed connect. - Properties dbProperties = poolProperties.getDbProperties(); - Properties properties = - dbProperties == null ? new Properties() : PoolUtilities.clone(dbProperties); + // The copy is taken under the configuration's monitor, for the same + // torn-snapshot reason PoolProperties#clone() synchronizes for: + // setConnectionProperties and the credential setters mutate the live + // collection in place under that monitor, while its reads of the + // collection are not atomic against such mutation. A copy taken + // outside the monitor can therefore mix two published generations: + // the driver then sees a key a concurrent reconfigure has just + // removed next to the keys that replaced it, or - when the credential + // travels in the connection-properties string rather than in the + // credential fields that are overlaid below - a user/password pair no + // generation ever published together. The configuration monitor is a + // leaf, so taking it here orders against no other lock. + Properties properties; + synchronized (poolProperties) { + Properties dbProperties = poolProperties.getDbProperties(); + properties = + dbProperties == null ? new Properties() : PoolUtilities.clone(dbProperties); + } if (usr != null) { properties.setProperty(PROP_USER, usr); } diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestReconfigureVsConnectProperties.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestReconfigureVsConnectProperties.java new file mode 100644 index 0000000000..bbf0df3378 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestReconfigureVsConnectProperties.java @@ -0,0 +1,257 @@ +/* + * 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.Driver; +import java.sql.DriverPropertyInfo; +import java.sql.SQLException; +import java.sql.SQLFeatureNotSupportedException; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Properties; +import java.util.Set; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; +import java.util.logging.Logger; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +/** + * The properties the connect path hands the driver are a copy of the live + * {@code dbProperties} collection, and {@code setConnectionProperties} + * reconfigures that collection in place while the pool runs. The copy reads + * the collection more than once and is not atomic against a publication, so + * taken outside the configuration's monitor it can mix two published + * generations: a driver then starts with a key a concurrent reconfigure has + * just removed next to the keys that replaced it, or - when the credential + * travels in the connection-properties string, as here, where the credential + * fields are unset and nothing is overlaid after the copy - with a + * user/password pair no generation ever published together. The connect path + * therefore takes the copy under the same monitor the reconfiguring setters + * hold; this test forces a reconfiguration into the middle of a copy and + * pins that the driver is handed one generation whole. + */ +class TestReconfigureVsConnectProperties { + + private static final String GENERATION_1 = "user=bob;password=P1;sslmode=require"; + private static final String GENERATION_2 = "user=alice;password=P2;newsetting=1"; + + /** + * Records the properties each connect was offered and then declines to + * open a session: the test is about what the copy contained, not about + * any real database voting on it. + */ + public static class RecordingNullDriver implements Driver { + + static final AtomicReference<Properties> lastConnectInfo = new AtomicReference<>(); + + @Override + public Connection connect(String url, Properties info) throws SQLException { + Properties handed = new Properties(); + handed.putAll(info); + lastConnectInfo.set(handed); + return null; + } + + @Override + public boolean acceptsURL(String url) throws SQLException { + return true; + } + + @Override + public DriverPropertyInfo[] getPropertyInfo(String url, Properties info) throws SQLException { + return new DriverPropertyInfo[0]; + } + + @Override + public int getMajorVersion() { + return 1; + } + + @Override + public int getMinorVersion() { + return 0; + } + + @Override + public boolean jdbcCompliant() { + return false; + } + + @Override + public Logger getParentLogger() throws SQLFeatureNotSupportedException { + throw new SQLFeatureNotSupportedException(); + } + } + + /** + * The live driver-properties collection. Its first use as the source of a + * {@code stringPropertyNames()} read - the copy's second observation of + * the collection, after its {@code putAll} - lets the racer attempt the + * second publication and then records which of the two outcomes the race + * produced: the racer parked behind the monitor the copy is holding (the + * copy is taken under the configuration's monitor), or the racer's + * mutation landing while the copy was still reading it (it is not). + */ + private static final class RacingStore extends Properties { + + private static final long serialVersionUID = 1L; + + final CountDownLatch copyObserving = new CountDownLatch(1); + final CountDownLatch racerStarted = new CountDownLatch(1); + final AtomicReference<Thread> racer = new AtomicReference<>(); + volatile String observation = "neither observed"; + + private transient volatile boolean armed = true; + + @Override + public Set<String> stringPropertyNames() { + if (armed) { + armed = false; + copyObserving.countDown(); + awaitQuietly(racerStarted, "the reconfigure racer to start"); + Thread racerThread = racer.get(); + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(10); + while (System.nanoTime() < deadline) { + if (racerThread.getState() == Thread.State.BLOCKED) { + // Parked entering the synchronized publication while + // this read still holds the very same monitor + observation = "racer held off by the copy's monitor"; + break; + } + if (containsKey("newsetting")) { + // The second publication mutated this live collection + // while the copy was still reading it + observation = "racer mutated mid-copy"; + break; + } + pause(); + } + } + return super.stringPropertyNames(); + } + + private static void awaitQuietly(CountDownLatch latch, String what) { + try { + if (!latch.await(10, TimeUnit.SECONDS)) { + throw new IllegalStateException("timed out waiting for " + what); + } + } catch (InterruptedException x) { + Thread.currentThread().interrupt(); + throw new IllegalStateException(x); + } + } + + private static void pause() { + try { + Thread.sleep(1); + } catch (InterruptedException x) { + Thread.currentThread().interrupt(); + } + } + } + + @Test + void aReconfigureRacingTheConnectCopyCannotMixTwoGenerations() throws Exception { + // Run the publication path once on a throwaway configuration, so the + // only monitor the racer can park on during the race is the + // configuration's own + PoolProperties warmUp = new PoolProperties(); + warmUp.setConnectionProperties(GENERATION_1); + warmUp.setConnectionProperties(GENERATION_2); + + PoolProperties props = new PoolProperties(); + props.setJmxEnabled(false); + // No eager connections at pool init: the one-shot hook in the store + // below belongs to the raced connect, not to the pool constructor + props.setInitialSize(0); + props.setUrl("jdbc:tomcat-jdbc-test:reconfiguration"); + props.setDriverClassName(RecordingNullDriver.class.getName()); + RacingStore store = new RacingStore(); + store.setProperty("user", "bob"); + store.setProperty("password", "P1"); + store.setProperty("sslmode", "require"); + props.setDbProperties(store); + // The credential travels in the string, so the credential fields stay + // null and nothing repairs the pair after the copy + props.setConnectionProperties(GENERATION_1); + RecordingNullDriver.lastConnectInfo.set(null); + + ConnectionPool pool = new ConnectionPool(props); + try { + PooledConnection con = new PooledConnection(props, pool); + Thread racer = new Thread(() -> { + store.racer.set(Thread.currentThread()); + store.racerStarted.countDown(); + props.setConnectionProperties(GENERATION_2); + }, "reconfigure-racer"); + AtomicReference<Throwable> connectFailure = new AtomicReference<>(); + Thread connectThread = new Thread(() -> { + try { + con.connectUsingDriver(); + } catch (Throwable x) { + // the recording driver declines every connect + connectFailure.set(x); + } + }, "connect-under-race"); + + connectThread.start(); + Assertions.assertTrue(store.copyObserving.await(10, TimeUnit.SECONDS), + "the connect copy must reach the live collection while it is still taking it"); + racer.start(); + Assertions.assertTrue(store.racerStarted.await(10, TimeUnit.SECONDS), + "the reconfigure racer must attempt its publication"); + connectThread.join(TimeUnit.SECONDS.toMillis(10)); + racer.join(TimeUnit.SECONDS.toMillis(10)); + Assertions.assertFalse(connectThread.isAlive(), "the connect must have ended"); + Assertions.assertFalse(racer.isAlive(), "the reconfigure must have ended"); + + Properties handed = RecordingNullDriver.lastConnectInfo.get(); + Assertions.assertNotNull(handed, "the driver must have been offered the copy"); + Assertions.assertTrue(connectFailure.get() instanceof SQLException, + "the driver declines every connect, after recording the copy"); + + // The copy must be one published generation whole. Without the + // copy under the monitor the racer's mutation lands mid-copy (the + // observation records that) and the copy comes back the union of + // the generations: the removed sslmode still aboard next to the + // newsetting that did not belong to the generation the copy's + // directly copied credentials came from. + Assertions.assertEquals(new HashSet<>(Arrays.asList("user", "password", "sslmode")), + handed.stringPropertyNames(), + "the driver was handed no published generation of the " + + "driver properties (racer: " + store.observation + ")"); + Assertions.assertEquals("bob", handed.getProperty("user"), + "the pair the copy carries must belong together (racer: " + store.observation + ")"); + Assertions.assertEquals("P1", handed.getProperty("password"), + "the pair the copy carries must belong together (racer: " + store.observation + ")"); + Assertions.assertEquals("require", handed.getProperty("sslmode")); + + // And the publication the copy held off did land, whole, after it + Assertions.assertEquals("alice", store.getProperty("user")); + Assertions.assertEquals("P2", store.getProperty("password")); + Assertions.assertEquals("1", store.getProperty("newsetting")); + Assertions.assertNull(store.getProperty("sslmode"), + "the second generation dropped the key the first one carried"); + } finally { + pool.close(true); + } + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
