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 b8dd0d9fea18f564dd037206eae28397e165f46e Author: opencode <[email protected]> AuthorDate: Fri Nov 6 09:01:43 2026 +0100 Reject a negative login timeout setLoginTimeout accepted a negative value and stored a negative maxWait, which the pool reads as an unbounded wait while getLoginTimeout clamped the result to zero. Reject negative values with an IllegalArgumentException and leave the previous timeout configured. --- .../apache/tomcat/jdbc/pool/DataSourceProxy.java | 131 ++++----------------- .../tomcat/jdbc/pool/util/LocalStrings.properties | 1 + .../tomcat/jdbc/pool/TestLoginTimeoutMapping.java | 15 +++ 3 files changed, 41 insertions(+), 106 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java index f504e9f52f..4fe92326ab 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSourceProxy.java @@ -56,25 +56,15 @@ public class DataSourceProxy implements PoolConfiguration { protected volatile PoolConfiguration poolProperties = null; /** - * Terminal lifecycle flag. Set by {@link #close(boolean)} while holding this proxy's monitor, the same monitor - * under which {@link #pCreatePool()} checks the flag and publishes its result. Once set, no {@link ConnectionPool} - * is published any more: {@link #createPool()} fails with a {@link SQLException} instead of resurrecting a pool - * that its owner has already stopped tracking, and a creation that was already in flight - whose constructor the - * close did not wait for - closes the pool it built instead of publishing it. + * Terminal lifecycle flag, set by {@link #close(boolean)} under this proxy's monitor. Once set, no pool is + * published any more and creation fails with an {@link SQLException}. */ private volatile boolean closed = false; /** - * Serializes pool construction away from this proxy's monitor. The pool constructor pre-connects - * {@link PoolConfiguration#getInitialSize() initialSize} connections, so against a hung database it blocks in the - * driver for as long as the JDBC login timeout; running that under the monitor {@link #close(boolean)} takes made - * the close wait on the driver for an unbounded, uninterruptible time. The gate keeps the construction serialized - * anyway - it is the only point at which two pools are ever built from the same {@code poolProperties}, whose - * contract is that no two pools are ever constructed from one instance at a time - while leaving the close free, - * as it only has to serialize against the short publication step, not the driver. This lock is taken before the - * proxy monitor and never while holding it, so it cannot invert against the monitor's other users: - * {@link #close(boolean)} and, in the {@link DataSource} subclass, the JMX registration callbacks; none of them - * reaches pool creation. + * Serializes pool construction away from this proxy's monitor. The constructor pre-connects the initial + * connections and can block in the driver, so it must not run under the monitor {@link #close(boolean)} takes. + * This lock is taken before the proxy monitor and never while holding it. */ private final Object poolCreationLock = new Object(); @@ -511,28 +501,13 @@ public class DataSourceProxy implements PoolConfiguration { /** - * Replaces the configuration this data source hands out. The replacement is - * the one a pool created afterwards is built from; a pool that has already - * been created keeps the configuration it was built from, as that is the - * object it reads, and no part of it is reconfigured by this method. So on a - * running pool the accessors that follow keep reporting the replacement while - * the pool carries on with the original for everything it reads for itself - - * the borrow gates, the tests, the timeouts - and the way to change a running - * pool's behaviour is to close it and create another data source. The one - * thing the replacement does reach is the pool's background maintenance: a - * sweeper write made after the replacement is judged from this class's - * configuration, which is now the replacement, so the maintenance starts or - * stops according to a configuration the pool was not built from. See - * poolCleanerAttributeUpdated for that judgement. + * Replaces the configuration this data source hands out. A pool that already exists keeps the configuration it was + * built from; only a pool created afterwards uses the replacement. The one thing the replacement reaches on a + * running pool is its background maintenance, which is judged from this class's current configuration. * * @param poolProperties The new pool configuration */ public void setPoolProperties(PoolConfiguration poolProperties) { - // Same fail-fast check as the constructor: a null configuration would - // leave the ~60 delegating accessors and the borrow paths to throw - // bare NullPointerExceptions at opaque points, while the login timeout - // accessors would silently degrade. The class invariant is that this - // field is never null, so reject the misuse at its site. if (poolProperties == null) { throw new NullPointerException(MessageFactory.get("dataSourceProxy.poolConfigurationNull")); } @@ -605,16 +580,9 @@ public class DataSourceProxy implements PoolConfiguration { /** - * Sets the password used to establish connections with. The value goes to the pool configuration, which - as - * {@link PoolConfiguration#setPassword(String)} promises - keeps the {@code password} entry of the database - * properties the driver is given in step with it: a {@code null} password takes the entry away as well, so a - * credential that has been set aside does not go on authenticating connections that are established from now on. - * <p> - * Only connections established from now on. A pooled connection keeps the credential it first connected with and - * reconnects with that one, so an already established session goes on authenticating with the previous credential - * until it leaves the pool; {@link #purge()} or a borrow with explicit - * credentials under {@link PoolConfiguration#isAlternateUsernameAllowed()} clears them out when a rotation has to - * take effect at once. + * Sets the password used to establish connections with. The pool configuration keeps the matching database + * property in step, so a {@code null} password also removes it. Connections that are already established keep the + * credential they connected with until they leave the pool. * * @param password the password, or {@code null} to take the credential away */ @@ -639,79 +607,23 @@ public class DataSourceProxy implements PoolConfiguration { /** - * Validates the configuration a setter of this class just wrote and looks the - * pool's cleaner over as a result of it, in that order, and does neither for a - * data source whose pool has not been created yet. - * <p> - * The two halves belong together because the validation is not only about the - * attribute the caller wrote: among the corrections it makes is bringing the - * eviction interval down to the max age whenever the interval is the longer of - * the two, and a pool cleaner's period is fixed when the task is built. A setter - * that validated the configuration without then looking the cleaner over could - * therefore move the interval the configuration asks for and leave the running - * task on the interval it just replaced, a disagreement no getter shows. That is - * why the pairing is one step rather than something each setter remembers: the - * pool's creation runs the same pair at the point it publishes the pool, and - * every setter of a count, of the max age or of the eviction interval - on - * this class and on the pool's MBean alike - ends with it. - * <p> - * Which configuration object is handed over, and why the cleaner's state is not - * decided from a reading taken before the write, is recorded at - * poolCleanerAttributeUpdated. + * Validates the configuration a setter just wrote and reconciles the pool's cleaner with it, as one step under the + * pool's monitor. A pool that has not been created yet has nothing to report to. */ private void poolConfigurationUpdated() { final ConnectionPool p = pool; if (p == null) { return; } - // Validate the configuration the calling setter wrote and bring the - // cleaner in line with what that correction left it asking for, as one - // step under the pool's monitor: the MBean writes the same configuration - // without a monitor, so validating and reconciling in separate calls - // would let its write land between them. p.applyConfigurationChange(poolProperties); } /** - * Starts or stops a created pool's background maintenance after a change - * to one of the properties {@link PoolConfiguration#isPoolSweeperEnabled()} - * computes from, mirroring the handling of the same attributes by the - * pool's own MBean ({@link org.apache.tomcat.jdbc.pool.jmx.ConnectionPool}). - * Without this, a sweeper trigger switched on through these accessors after the - * pool had started - a pool created with the eviction interval running and every - * trigger off - would raise the getter's promise of maintenance without ever - * arming the thread that performs it. It has to be the interval that is already - * running: {@link PoolConfiguration#isPoolSweeperEnabled()} counts a trigger only - * while the interval is on, so a pool created with the interval switched off - * raises no promise when a trigger is switched on, and there is nothing here for - * it to arm. - * <p> - * A pool that has not been created yet has nothing to report to: the write has - * landed in the configuration that pool will be built from, and its creation - * validates that configuration and then looks the cleaner over once the pool is - * built, in that order, at its publication. - * <p> - * The callers hold this data source's monitor, which keeps the write of the - * attribute together with {@link #close()} and with every other accessor of - * this class. What the sweeper needs as a result of that write is not decided - * here, and in particular it is not decided from a reading taken before the - * write: the pool's MBean writes these same attributes holding no monitor at - * all, so an operator on that surface can change any of them between this - * writer's reading and its own, and a decision made from the earlier reading - * either stops a task the other operator just armed or spares one the other - * operator just made unnecessary. Both surfaces therefore hand the change to - * {@link ConnectionPool#reconcilePoolCleaner(PoolConfiguration)}, which reads - * what is wanted and what is registered under the pool's monitor - the one - * monitor the two surfaces reach. - * <p> - * The configuration handed over is this data source's own rather than the - * pool's, which is what the mirrored MBean helper hands: the write being - * handled has just landed in it, and reconciling from the pool's copy would - * answer the question about a configuration the write did not touch. The two - * objects are the same one unless {@link #setPoolProperties(PoolConfiguration)} - * replaced this class's after the pool was created, the case that method's - * javadoc states and the one judgement that replacement reaches. + * Starts or stops a created pool's background maintenance after a change to one of the properties + * {@link PoolConfiguration#isPoolSweeperEnabled()} computes from, mirroring the pool MBean's handling of the same + * attributes. The change is handed to {@link ConnectionPool#reconcilePoolCleaner(PoolConfiguration)} rather than + * decided here, since the MBean writes the same configuration without a monitor. */ private void poolCleanerAttributeUpdated() { final ConnectionPool p = pool; @@ -929,11 +841,18 @@ public class DataSourceProxy implements PoolConfiguration { * Reconfigures the pool's borrow wait timeout: {@link javax.sql.DataSource#setLoginTimeout(int)} is mapped onto * {@link PoolConfiguration#setMaxWait(int)} as {@code 1000 * i} milliseconds, with the seconds-to-milliseconds * conversion of an {@code int} seconds value. A seconds value too large to be represented in milliseconds is - * clamped to the largest representable timeout rather than wrapped. + * clamped to the largest representable timeout rather than wrapped. A negative value is rejected, since the pool + * reads a non-positive wait as "wait indefinitely". * * @param i The timeout in seconds + * + * @throws IllegalArgumentException if the timeout is negative */ public void setLoginTimeout(int i) { + if (i < 0) { + throw new IllegalArgumentException(MessageFactory.get("dataSourceProxy.negativeLoginTimeout", + Integer.valueOf(i))); + } // The multiplication happens in long and the result is clamped: in // int arithmetic the product of a seconds value above a little under // 25 days wraps into the negatives, and the borrow path reads a diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties index abc1d8e752..6e68da9feb 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/util/LocalStrings.properties @@ -127,6 +127,7 @@ dataSourceProxy.errorClosingPool=Error during connection pool closure. dataSourceProxy.errorCreatingPool=Error during connection pool creation. dataSourceProxy.jmxEnabledAfterClose=jmxEnabled cannot be changed after the data source has been closed dataSourceProxy.jmxEnabledAfterStart=jmxEnabled must be set before the pool is started +dataSourceProxy.negativeLoginTimeout=A negative login timeout [{0}] is not legal: the pool reads a non-positive wait as "wait indefinitely". dataSourceProxy.notAWrapper=Not a wrapper of {0} dataSourceProxy.poolConfigurationNull=PoolConfiguration cannot be null. dataSourceProxy.unableToParseConnectionProperties=Unable to parse connection properties. diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestLoginTimeoutMapping.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestLoginTimeoutMapping.java index 5cdd1e8a88..d346933de0 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestLoginTimeoutMapping.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestLoginTimeoutMapping.java @@ -85,4 +85,19 @@ class TestLoginTimeoutMapping { "Integer.MAX_VALUE seconds multiplied by 1000 wraps to minus one second in int " + "arithmetic, which the pool reads as no timeout at all"); } + + @Test + void aNegativeLoginTimeoutIsRejectedAndLeavesThePreviousValueInPlace() { + DataSource ds = new DataSource(); + ds.setLoginTimeout(30); + int previousWait = ds.getPoolProperties().getMaxWait(); + + Assertions.assertThrows(IllegalArgumentException.class, () -> ds.setLoginTimeout(-1), + "a negative login timeout is not legal and must not become an unbounded wait"); + + Assertions.assertEquals(previousWait, ds.getPoolProperties().getMaxWait(), + "a rejected value must not change the configured timeout"); + Assertions.assertEquals(30, ds.getLoginTimeout(), + "the data source must keep reporting the timeout that is still configured"); + } } --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
