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 62e49fed34ff67f6bf076551f7e91f156902540e Author: opencode <[email protected]> AuthorDate: Tue Nov 3 09:39:12 2026 +0100 Refuse a credential change without costing the pool a session A borrow that asked for alternate credentials the database refused disconnected a healthy idle session and then handed that session back to the pool under the identity the database had just refused. shouldForceReconnect() recorded the borrower's credentials on the session as a side effect of merely deciding that a reconnect was needed. When the database then refused those credentials, the borrow failure handler took the surviving session for one already reconnected under the requested identity, discarded the sign-on failure as a stale retry, and returned the session to the idle queue still signed on as the earlier borrower - while the pool lost the session the forced reconnect had disconnected. That decision is now a comparison only; the requested identity is written by reconnectWith() once a session established under it has actually been accepted, and only then does it replace the session in hand. A reconnect that fails before taking on a session returns the original session to the idle queue instead of releasing it. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 56 ++++- .../apache/tomcat/jdbc/pool/PooledConnection.java | 230 +++++++++++++++++---- .../tomcat/jdbc/pool/TestAlternateCredentials.java | 40 ++++ .../tomcat/jdbc/pool/TestShouldForceReconnect.java | 20 +- 4 files changed, 289 insertions(+), 57 deletions(-) diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java index 4c44a6f10d..6cb23f5307 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/ConnectionPool.java @@ -1496,7 +1496,8 @@ public class ConnectionPool { } // Evaluate username/password change as well as max age functionality - boolean forceReconnect = con.shouldForceReconnect(username, password) || con.isMaxAgeExpired(); + boolean credentialsDiffer = con.shouldForceReconnect(username, password); + boolean forceReconnect = credentialsDiffer || con.isMaxAgeExpired(); if (!con.isDiscarded() && !con.isInitialized()) { // Here it states that the connection not discarded, but the connection is null @@ -1557,8 +1558,20 @@ public class ConnectionPool { setToNull = true; return null; } + // Whether the reconnect below took on a new session: only a + // reconnect that never got that far can have left a session worth + // keeping behind it. + boolean reconnected = false; try { - con.reconnect(); + if (credentialsDiffer) { + // A session the database has not refused is not this + // borrower's to spend, so the new identity is put to the + // database before the session it replaces is given up. + con.reconnectWith(username, password); + } else { + con.reconnect(); + } + reconnected = true; reconnectedCount.incrementAndGet(); int validationMode = isInitNewConnections() ? PooledConnection.VALIDATE_INIT : PooledConnection.VALIDATE_BORROW; @@ -1589,7 +1602,17 @@ public class ConnectionPool { throw new SQLException(MessageFactory.get("connectionPool.failedToValidateNewConnection")); } } catch (Exception x) { - release(con); + if (credentialsDiffer && !reconnected && con.isInitialized() && !con.isDiscarded()) { + // The attempt was refused before it gave up the session + // this connection was holding: that session is open, it + // carries its own still-valid credentials, and it belongs + // to the pool and to every borrower the database does not + // turn away. It goes back to the queue it came from + // instead of paying for a borrow that never used it. + returnToIdle(con); + } else { + release(con); + } setToNull = true; if (x instanceof SQLException) { throw (SQLException) x; @@ -1607,6 +1630,33 @@ public class ConnectionPool { } } + /** + * Hands a connection back to the idle queue after a borrow attempt that failed without using the session the + * connection holds. The attempt neither handed the session out nor closed it, so the connection is exactly as + * usable as it was before the attempt and the pool keeps it: a borrower the database refused is not a reason to + * destroy a session, and the slot it stands for. + * + * @param con the connection to hand back to the idle queue + */ + private void returnToIdle(PooledConnection con) { + if (isClosed()) { + // A close has drained the queues once already, so a session + // handed to the idle queue now would sit there out of reach of + // every shutdown path. Retire it instead. + release(con); + } else if (!idle.offer(con)) { + release(con); + } else if (isClosed() && idle.remove(con)) { + // The closed check above and this offer are not one atomic step: + // a close arriving in between has drained the queue this offer + // then fills for the last time. Take the connection back out of a + // queue that has already been drained past; if it is no longer + // there, a taker has taken ownership of it and sees to its + // closure. + release(con); + } + } + /** * Returns whether new connections should be initialized by invoking {@link PooledConnection#validate(int)} with * {@link PooledConnection#VALIDATE_INIT}. 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 3d42c388c5..6ee3233b1f 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 @@ -185,12 +185,16 @@ public class PooledConnection implements PooledConnectionMBean { * {@link PoolConfiguration#isAlternateUsernameAllowed()} method returns false. Returns false if the * username/password has not changed since this connection was connected * + * <p>This is a comparison only. It records nothing: the identity a session is established with is written by the + * reconnect that follows this decision (see {@link #reconnectWith(String, String)}), so that a reconnect the + * database refused cannot leave this connection carrying credentials that were never accepted.</p> + * * @param username the username you wish to connect with, pass in null to accept the default username from * {@link PoolConfiguration#getUsername()} * @param password the password you wish to connect with, pass in null to accept the default username from * {@link org.apache.tomcat.jdbc.pool.PoolConfiguration#getPassword()} * - * @return true is the pool must reconnect + * @return true if the pool must reconnect */ public boolean shouldForceReconnect(String username, String password) { @@ -214,17 +218,6 @@ public class PooledConnection implements PooledConnectionMBean { noChangeInCredentials = noChangeInCredentials && ((password == null && storedPwd == null) || (password != null && password.equals(storedPwd))); - if (username == null) { - getAttributes().remove(PROP_USER); - } else { - getAttributes().put(PROP_USER, username); - } - if (password == null) { - getAttributes().remove(PROP_PASSWORD); - } else { - getAttributes().put(PROP_PASSWORD, password); - } - return !noChangeInCredentials; } @@ -595,6 +588,127 @@ public class PooledConnection implements PooledConnectionMBean { this.connect(); } + /** + * Re-establishes this connection under the credentials handed in, giving up the session it currently holds only + * once a session established under those credentials has been opened. + * + * <p> + * {@link #reconnect()} closes the session in hand before it asks for another one, which is the right order for a + * session the pool is retiring on its own account - because it reached {@link PoolConfiguration#getMaxAge() maxAge} + * or failed a validation. It is the wrong order for a session that is being handed to a borrower under different + * credentials: the database decides whether those credentials are acceptable, and a database that refuses them + * would, with that order, have taken a healthy session out of the pool - a session every other borrower of the + * pool can still use, and one the refused borrower never had a claim on. Here the order is reversed: the + * credentials are put to the database while the session in hand stays in place, and only a session the database + * has accepted replaces it.</p> + * + * <p> + * The credentials themselves are written into {@link #getAttributes()} for the attempt, because that is where the + * connect reads them from, and are put back as they were if the attempt fails. So neither a refusal nor a + * connection that could not be established leaves this connection carrying credentials the database never + * accepted - which matters because those attributes decide the identity of every later reconnect.</p> + * + * @param username the username to re-establish the connection with, pass in null to accept the default username + * from {@link PoolConfiguration#getUsername()} + * @param password the password to re-establish the connection with, pass in null to accept the default password + * from {@link PoolConfiguration#getPassword()} + * + * @throws SQLException if the database refused the credentials or a session under them could not be established. + * This connection then still holds the session, and carries the credentials, it held + * before the call. + */ + public void reconnectWith(String username, String password) throws SQLException { + if (username == null) { + username = poolProperties.getUsername(); + } + if (password == null) { + password = poolProperties.getPassword(); + } + + // The state a refused attempt has to hand back: the session this + // connection is serving and the identity that session was opened + // with. + java.sql.Connection served = connection; + javax.sql.XAConnection servedXa = xaConnection; + javax.sql.PooledConnection servedCpds = cpdsConnection; + HashMap<Object, Object> sessionAttributes = getAttributes(); + boolean hadUser = sessionAttributes.containsKey(PROP_USER); + Object servedUser = sessionAttributes.get(PROP_USER); + boolean hadPassword = sessionAttributes.containsKey(PROP_PASSWORD); + Object servedPassword = sessionAttributes.get(PROP_PASSWORD); + + // Stage the credentials the attempt is made under. A null stands for + // the pool's default, which the connect substitutes the same way it + // does for a plain connect. + if (username == null) { + sessionAttributes.remove(PROP_USER); + } else { + sessionAttributes.put(PROP_USER, username); + } + if (password == null) { + sessionAttributes.remove(PROP_PASSWORD); + } else { + sessionAttributes.put(PROP_PASSWORD, password); + } + + // Set the session being replaced out of reach of the connect below, + // which closes any session it finds before it opens the new one. This + // one has to stay open until the attempt that replaces it is known to + // have succeeded. + connection = null; + xaConnection = null; + cpdsConnection = null; + try { + connect(); + } catch (SQLException | RuntimeException x) { + // A connect can fail after it established the session - the + // default state it applies to a new session can be refused - so + // whatever is in the fields is closed here rather than dropped. + // The interceptor chain is told nothing: this session never + // served anyone, and the state the chain holds belongs to the + // session that is about to be back in service. + closeSession(connection, xaConnection, cpdsConnection); + connection = served; + xaConnection = servedXa; + cpdsConnection = servedCpds; + if (hadUser) { + sessionAttributes.put(PROP_USER, servedUser); + } else { + sessionAttributes.remove(PROP_USER); + } + if (hadPassword) { + sessionAttributes.put(PROP_PASSWORD, servedPassword); + } else { + sessionAttributes.remove(PROP_PASSWORD); + } + throw x; + } + + if (served == null) { + // There was no session to replace: this is a plain connect of a + // connection that had none, and the new session is already in + // place. + return; + } + + // The database accepted the credentials, so the session they replace + // can be given up now. The fields are turned back to it while it is + // retired, so that the chain is told - as it is for every other + // retirement - about the session it has been serving rather than + // about the one that takes its place. + java.sql.Connection accepted = connection; + javax.sql.XAConnection acceptedXa = xaConnection; + javax.sql.PooledConnection acceptedCpds = cpdsConnection; + connection = served; + xaConnection = servedXa; + cpdsConnection = servedCpds; + notifyDisconnected(false); + closeSession(served, servedXa, servedCpds); + connection = accepted; + xaConnection = acceptedXa; + cpdsConnection = acceptedCpds; + } + /** * Disconnects the connection. All exceptions are logged using debug level. * @@ -606,39 +720,8 @@ public class PooledConnection implements PooledConnectionMBean { } setDiscarded(true); if (connection != null) { - try { - parent.disconnectEvent(this, finalize); - } catch (Throwable x) { - // The event itself isolates each link of the chain, so this - // catch only guards against a failure of the walk itself; - // an interceptor must not be able to keep the physical - // connection from being closed - log.warn(MessageFactory.get("pooledConnection.interceptorDisconnectedFailed"), x); - } - javax.sql.XAConnection retired = xaConnection; - if (retired instanceof XABranchTracker) { - // Mark the handle retired before the physical close: branch - // work on a resource captured from this handle then meets the - // pool's clean XA refusal instead of the driver's half-closed - // state. - ((XABranchTracker) retired).markClosed(); - } - try { - if (cpdsConnection != null) { - // Closing the handle closes the physical connection - // and invalidates the logical connection; closing the - // logical connection alone would not. - cpdsConnection.close(); - } else if (xaConnection == null) { - connection.close(); - } else { - xaConnection.close(); - } - } catch (Exception ignore) { - if (log.isDebugEnabled()) { - log.debug("Unable to close underlying SQL connection", ignore); - } - } + notifyDisconnected(finalize); + closeSession(connection, xaConnection, cpdsConnection); } connection = null; xaConnection = null; @@ -649,6 +732,63 @@ public class PooledConnection implements PooledConnectionMBean { } } + /** + * Tells the interceptor chain that the connection is going out of service, so that each link can give up what it + * holds for the session being retired. + * + * @param finalizing whether the retirement is the one that ends the pooled connection + */ + private void notifyDisconnected(boolean finalizing) { + try { + parent.disconnectEvent(this, finalizing); + } catch (Throwable x) { + // The event itself isolates each link of the chain, so this + // catch only guards against a failure of the walk itself; + // an interceptor must not be able to keep the physical + // connection from being closed + log.warn(MessageFactory.get("pooledConnection.interceptorDisconnectedFailed"), x); + } + } + + /** + * Closes a physical session and the handle it came in on, if it came in on one. The session is passed in rather + * than read from the fields, because a caller that has already taken on the session replacing this one - see + * {@link #reconnectWith(String, String)} - still has to close the session it gave up. + * + * @param session the session to close, ignored when null + * @param xaHandle the XA handle the session was obtained through, or null + * @param cpdsHandle the {@link javax.sql.PooledConnection} handle the session was obtained through, or null + */ + private void closeSession(java.sql.Connection session, javax.sql.XAConnection xaHandle, + javax.sql.PooledConnection cpdsHandle) { + if (cpdsHandle == null && xaHandle == null && session == null) { + return; + } + if (xaHandle instanceof XABranchTracker) { + // Mark the handle retired before the physical close: branch + // work on a resource captured from this handle then meets the + // pool's clean XA refusal instead of the driver's half-closed + // state. + ((XABranchTracker) xaHandle).markClosed(); + } + try { + if (cpdsHandle != null) { + // Closing the handle closes the physical connection + // and invalidates the logical connection; closing the + // logical connection alone would not. + cpdsHandle.close(); + } else if (xaHandle != null) { + xaHandle.close(); + } else { + session.close(); + } + } catch (Exception ignore) { + if (log.isDebugEnabled()) { + log.debug("Unable to close underlying SQL connection", ignore); + } + } + } + /** * Returns abandon timeout in milliseconds diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAlternateCredentials.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAlternateCredentials.java index a32cdab6e6..1a73e3876e 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAlternateCredentials.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestAlternateCredentials.java @@ -151,6 +151,46 @@ class TestAlternateCredentials extends JdbcPoolBaseTest { "a refused borrow must not leak a checked out connection"); } + @Test + void aRefusedCredentialChangeCostsThePoolNothing() throws Exception { + DataSource ds = alternatePool(true); + ds.getPoolProperties().setMaxWait(5000); + ds.createPool(); + + // One healthy session of the pool's own user, checked back in: this is + // the session a later alternate-credential borrow picks up and is + // refused with. + try (Connection c = ds.getConnection()) { + setSessionMarker(c, 42); + } + int sizeBefore = ds.getSize(); + int sessionsBefore = poolSessions(); + long reconnectionsBefore = ds.getReconnectedCount(); + + Assertions.assertThrows(SQLException.class, + () -> ds.getConnection(ALT_USER, "wrong-password"), + "the database must refuse a borrow with a wrong password"); + + Assertions.assertEquals(sizeBefore, ds.getSize(), + "an attempt the database refused must not take a session - and with " + + "it a maxActive slot - out of the pool"); + Assertions.assertEquals(sessionsBefore, poolSessions(), + "the session the refused attempt was made on must still be open"); + Assertions.assertEquals(reconnectionsBefore, ds.getReconnectedCount(), + "a refused attempt is not a reconnection"); + + // The session came back where it was, still authenticated as the user + // it was opened as: the refused credentials are not what it will be + // re-established with either. + try (Connection c = ds.getConnection()) { + Assertions.assertEquals(DEFAULT_USER, sessionUser(c)); + Assertions.assertEquals(Integer.valueOf(42), sessionMarker(c), + "the refused attempt must not have replaced the session"); + } + Assertions.assertEquals(sizeBefore, ds.getSize(), + "serving the next borrower must not have needed a new session"); + } + @Test void anUnknownUserIsRefusedByTheDatabase() throws Exception { DataSource ds = alternatePool(true); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestShouldForceReconnect.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestShouldForceReconnect.java index f00f6352b1..9a02edc2b6 100644 --- a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestShouldForceReconnect.java +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestShouldForceReconnect.java @@ -30,8 +30,9 @@ import org.junit.jupiter.params.provider.MethodSource; * re-established for the borrower that asks for it?", checked in * isolation. A pooled session remembers the identity it logged in as, a * {@code null} in a request means the pool's default identity, and the - * operation both decides <em>and</em> records what was asked - so every - * case below starts from an explicitly configured session. + * decision records nothing on its own - the identity is recorded by the + * reconnect that the database accepted - so every case below starts from an + * explicitly configured session. * * <p>Ported from the retired legacy {@code ShouldForceReconnectTest}. The * {@code maxAge} half of the same decision is covered against a real @@ -130,19 +131,20 @@ class TestShouldForceReconnect { } @Test - void theDecisionRecordsTheIdentityItJustVetoed() throws Exception { + void theDecisionAloneRecordsNoIdentity() throws Exception { PooledConnection con = pooledConnection(); loggedInAs(con, DEFAULT_USER, DEFAULT_PASSWD); Assertions.assertTrue(con.shouldForceReconnect(ALT_USER, ALT_PASSWD), "another identity must be refused the pooled session"); - Assertions.assertFalse(con.shouldForceReconnect(ALT_USER, ALT_PASSWD), - "the decision records the new identity, so the next borrower asking " - + "for the same user gets the session once it is re-established"); + Assertions.assertTrue(con.shouldForceReconnect(ALT_USER, ALT_PASSWD), + "the decision records nothing, so the same identity is refused the " + + "session again: only a reconnect the database accepted " + + "establishes a new identity"); + Assertions.assertFalse(con.shouldForceReconnect(DEFAULT_USER, DEFAULT_PASSWD), + "and the session is still the identity it was opened with"); - loggedInAs(con, DEFAULT_USER, DEFAULT_PASSWD); - Assertions.assertFalse(con.shouldForceReconnect(null, null)); - Assertions.assertTrue(con.shouldForceReconnect(ALT_USER, ALT_PASSWD)); + loggedInAs(con, ALT_USER, ALT_PASSWD); Assertions.assertFalse(con.shouldForceReconnect(ALT_USER, ALT_PASSWD)); Assertions.assertTrue(con.shouldForceReconnect(null, null), "and the same holds in the other direction"); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
