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 da29e2626338f97624e5f527ae4aa7841c4eef4f Author: opencode <[email protected]> AuthorDate: Thu Oct 29 13:57:12 2026 +0100 Fix the JMX pool object hand-over race in the connection pool setup close() nulls the pool's JMX object without synchronization while other threads check and then use the field, so a borrow racing the close could dereference the field twice and observe the null the first read missed. PooledConnection.createMBean() did exactly that via getJmxPool(), and it ran in setupConnection() before any abandonSetup() guard, although the connection is already in the busy queue there: the resulting NullPointerException escaped getConnection() and stranded the connection in busy, losing a physical session and a maxActive slot until restart. Declare the field volatile, read it once into a local at every use site, have the per-connection registration check the pool's JMX object itself and skip when it is gone, and run the registration under abandonSetup() so a failed setup hands the connection back instead of leaking it. --- .../apache/tomcat/jdbc/pool/ConnectionPool.java | 55 +++++-- .../org/apache/tomcat/jdbc/pool/DataSource.java | 9 +- .../apache/tomcat/jdbc/pool/PooledConnection.java | 18 ++- .../jdbc/pool/interceptor/SlowQueryReportJmx.java | 9 +- .../jdbc/pool/TestSetupConnectionMBeanFailure.java | 172 +++++++++++++++++++++ 5 files changed, 238 insertions(+), 25 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 69ab88c413..f67b741a9f 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 @@ -135,9 +135,10 @@ public class ConnectionPool { new LinkedBlockingQueue<>()); /** - * reference to the JMX mbean + * reference to the JMX mbean. close() gives it up without holding any pool lock while other threads still notify + * through it, so the field is volatile and every user reads it at most once, into a local. */ - protected org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmxPool = null; + protected volatile org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmxPool = null; /** * counter to track how many threads are waiting for a connection @@ -346,8 +347,21 @@ public class ConnectionPool { // Fetch previously cached interceptor proxy - one per connection JdbcInterceptor handler = con.getHandler(); if (handler == null) { - if (jmxPool != null) { + // The registration reads the pool's own JMX object itself and does + // nothing once close() has given it up; it is not gated on jmxPool + // here because checking the field and then calling into the + // connection would let close() take it away between the two, and + // the resulting failure must not strand the connection - it is + // already in busy, so a throw that skips abandonSetup() costs a + // physical connection and a maxActive slot until the pool restarts. + try { con.createMBean(); + } catch (Exception x) { + SQLException sx = new SQLException("Unable to register the pooled connection MBean."); + sx.initCause(x); + // Nothing has been done to the connection, it can go back to the pool + abandonSetup(con, sx, false); + throw sx; } // Build the proxy handler handler = new ProxyConnection(this, con, getPoolProperties().isUseEquals()); @@ -654,8 +668,9 @@ public class ConnectionPool { interceptor.poolStarted(this); } catch (Exception x) { log.error("Unable to inform interceptor of pool start.", x); - if (jmxPool != null) { - jmxPool.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.NOTIFY_INIT, getStackTrace(x)); + org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = jmxPool; + if (jmx != null) { + jmx.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.NOTIFY_INIT, getStackTrace(x)); } close(true); SQLException ex = new SQLException(); @@ -674,8 +689,9 @@ public class ConnectionPool { } catch (SQLException x) { log.error("Unable to create initial connections of pool.", x); if (!poolProperties.isIgnoreExceptionOnPreLoad()) { - if (jmxPool != null) { - jmxPool.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.NOTIFY_INIT, getStackTrace(x)); + org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = jmxPool; + if (jmx != null) { + jmx.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.NOTIFY_INIT, getStackTrace(x)); } close(true); throw x; @@ -777,8 +793,9 @@ public class ConnectionPool { if (getPoolProperties().isLogAbandoned()) { log.warn("Connection has been abandoned " + con + ":" + trace); } - if (jmxPool != null) { - jmxPool.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.NOTIFY_ABANDON, trace); + org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = jmxPool; + if (jmx != null) { + jmx.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.NOTIFY_ABANDON, trace); } // Release the connection removeAbandonedCount.incrementAndGet(); @@ -809,8 +826,9 @@ public class ConnectionPool { log.warn("Connection has been marked suspect, possibly abandoned " + con + "[" + (System.currentTimeMillis() - con.getTimestamp()) + " ms.]:" + trace); } - if (jmxPool != null) { - jmxPool.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.SUSPECT_ABANDONED_NOTIFICATION, trace); + org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = jmxPool; + if (jmx != null) { + jmx.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.SUSPECT_ABANDONED_NOTIFICATION, trace); } con.setSuspect(true); } finally { @@ -953,8 +971,9 @@ public class ConnectionPool { waitcount.decrementAndGet(); } if (maxWait == 0 && con == null) { // No wait, return one if we have one - if (jmxPool != null) { - jmxPool.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.POOL_EMPTY, "Pool empty - no wait."); + org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = jmxPool; + if (jmx != null) { + jmx.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.POOL_EMPTY, "Pool empty - no wait."); } throw new PoolExhaustedException("[" + Thread.currentThread().getName() + "] " + "NoWait: Pool empty. Unable to fetch a connection, none available[" + busy.size() + @@ -963,8 +982,9 @@ public class ConnectionPool { // We didn't get a connection, lets see if we timed out if (con == null) { if ((System.currentTimeMillis() - now) >= maxWait) { - if (jmxPool != null) { - jmxPool.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.POOL_EMPTY, + org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = jmxPool; + if (jmx != null) { + jmx.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.POOL_EMPTY, "Pool empty - timeout."); } throw new PoolExhaustedException("[" + Thread.currentThread().getName() + "] " + @@ -1289,8 +1309,9 @@ public class ConnectionPool { " The processing time is " + (System.currentTimeMillis() - con.getTimestamp()) + " ms."); } - if (jmxPool != null) { - jmxPool.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.SUSPECT_RETURNED_NOTIFICATION, + org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = jmxPool; + if (jmx != null) { + jmx.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.SUSPECT_RETURNED_NOTIFICATION, "Connection(" + con + ") that has been marked suspect was returned."); } } diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSource.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSource.java index 70cd1c045b..8868517198 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSource.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/DataSource.java @@ -170,7 +170,12 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, */ protected void registerJmx() { final ConnectionPool p = pool; - if (p == null || p.getJmxPool() == null) { + // Read the pool's JMX object only once: close() gives it up + // concurrently, and a second read can observe the null the first one + // did not - a registration attempt against that null would only fail + // inside the helper, with a spurious error log + final org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = p == null ? null : p.getJmxPool(); + if (jmx == null) { log.warn("JDBC pool MBean [" + oname + "] not registered: the pool " + "has not been created yet or has already been closed."); // oname is the handle postDeregister() unregisters; do not keep @@ -182,7 +187,7 @@ public class DataSource extends DataSourceProxy implements javax.sql.DataSource, // hands back the name of its own publish, or null when the server // refused it, so a name a foreign MBean owns is never released as // if it were this pool's registration - this.oname = JmxUtil.registerJmx(oname, null, p.getJmxPool()); + this.oname = JmxUtil.registerJmx(oname, null, jmx); } /** 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 27e9365999..c366f3fc54 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 @@ -458,8 +458,12 @@ public class PooledConnection implements PooledConnectionMBean { if (log.isDebugEnabled()) { log.debug("Unable to connect to database.", x); } - if (parent.jmxPool != null) { - parent.jmxPool.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.NOTIFY_CONNECT, + // Read the pool's JMX object only once: close() gives it up + // concurrently, and a second read can observe the null the first + // one did not + org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = parent.getJmxPool(); + if (jmx != null) { + jmx.notify(org.apache.tomcat.jdbc.pool.jmx.ConnectionPool.NOTIFY_CONNECT, ConnectionPool.getStackTrace(x)); } if (x instanceof SQLException) { @@ -1090,14 +1094,20 @@ public class PooledConnection implements PooledConnectionMBean { } /** - * Creates and registers an MBean for this pooled connection with JMX. + * Creates and registers an MBean for this pooled connection with JMX. The pool's own MBean is the registration + * base; once close() has given it up - which a borrow can race - there is no base to register under, and the + * registration is skipped. */ public void createMBean() { if (oname != null) { return; } + org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = parent.getJmxPool(); + if (jmx == null) { + return; + } String keyprop = ",connections=PooledConnection[" + connectionIndex.getAndIncrement() + "]"; - oname = JmxUtil.registerJmx(parent.getJmxPool().getObjectName(), keyprop, this); + oname = JmxUtil.registerJmx(jmx.getObjectName(), keyprop, this); } /** diff --git a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmx.java b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmx.java index 7b806d638f..edd7aaffcb 100644 --- a/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmx.java +++ b/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReportJmx.java @@ -311,8 +311,13 @@ public class SlowQueryReportJmx extends SlowQueryReport implements NotificationE long sequence = notifySequence.incrementAndGet(); if (isNotifyPool()) { - if (this.pool != null && this.pool.getJmxPool() != null) { - this.pool.getJmxPool().notify(type, query); + // Read the pool and its JMX object each only once: close() + // gives the JMX object up concurrently, and a second read can + // observe the null the first one did not + ConnectionPool p = this.pool; + org.apache.tomcat.jdbc.pool.jmx.ConnectionPool jmx = p == null ? null : p.getJmxPool(); + if (jmx != null) { + jmx.notify(type, query); } } else { NotificationBroadcasterSupport target = getNotifier(); diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestSetupConnectionMBeanFailure.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestSetupConnectionMBeanFailure.java new file mode 100644 index 0000000000..2875f808bb --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestSetupConnectionMBeanFailure.java @@ -0,0 +1,172 @@ +/* + * 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.SQLException; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * A borrow reads the pool's JMX object when it registers the per-connection + * MBean, but {@code close()} gives that object up without holding any pool + * lock, so a borrow racing the close can find it gone between a null check + * and its use. Read unsafely, that produced a NullPointerException from + * {@code PooledConnection.createMBean()} inside + * {@code ConnectionPool.setupConnection()}. The interesting part is where + * that lands: the connection is already in the busy queue when setup runs, + * and a failure that skips {@code abandonSetup()} strands it there - the + * physical session and a maxActive slot are lost until the pool restarts. + * <p> + * The window between check and use is a few instructions wide and cannot be + * reproduced deterministically. What these tests pin down instead is the + * contract the fix makes total: the registration is skipped when the pool has + * no JMX object to register under - rather than dereferencing it blindly - + * and it runs unconditionally from the setup path, so that whatever it fails + * with, the borrowed connection goes back to the pool. That the remaining + * JMX notifications read the volatile field into a local before using it is + * carried by review, not by a test. + */ +class TestSetupConnectionMBeanFailure extends JdbcPoolBaseTest { + + /* + * A connection whose JMX registration fails while the flag is set, and + * performs the real registration once the test lets it through. + */ + static class FailingRegistrationConnection extends PooledConnection { + + private final AtomicBoolean failRegistration; + + FailingRegistrationConnection(PoolConfiguration prop, ConnectionPool parent, + AtomicBoolean failRegistration) { + super(prop, parent); + this.failRegistration = failRegistration; + } + + @Override + public void createMBean() { + if (failRegistration.get()) { + throw new IllegalStateException("boom from createMBean()"); + } + super.createMBean(); + } + } + + /* + * A pool handing out the connections above. The base create() is still + * invoked for its pool-size bookkeeping; the pooled connection object it + * builds is never connected and simply discarded. + */ + static class FailingRegistrationPool extends ConnectionPool { + + final AtomicBoolean failRegistration = new AtomicBoolean(true); + + FailingRegistrationPool(PoolConfiguration prop) throws SQLException { + super(prop); + } + + @Override + protected PooledConnection create(boolean incrementCounter) { + super.create(incrementCounter); + return new FailingRegistrationConnection(getPoolProperties(), this, failRegistration); + } + } + + /* + * The pool is built directly rather than through a DataSource: the setup + * path and the failure seam are package-protected, and JMX stays disabled + * so nothing reaches the platform MBean server. + */ + private static PoolConfiguration newPoolProperties() { + PoolProperties props = new PoolProperties(); + props.setUrl(getUrl()); + props.setDriverClassName("org.h2.Driver"); + props.setUsername("sa"); + props.setPassword(""); + props.setJmxEnabled(false); + props.setInitialSize(0); + props.setMinIdle(0); + // one slot only: a stranded connection would show up as an + // exhausted pool on the very next borrow + props.setMaxActive(1); + return props; + } + + @Test + void createMBeanSkipsRegistrationWhenPoolHasNoJmxObject() throws Exception { + ConnectionPool pool = new ConnectionPool(newPoolProperties()); + try { + Assertions.assertNull(pool.getJmxPool(), + "a pool with JMX disabled has no MBean to register under"); + PooledConnection con = pool.create(false); + // Before the fix this dereferenced the pool's absent JMX object + // and threw a NullPointerException; the registration must instead + // be skipped, leaving the connection unregistered. + Assertions.assertDoesNotThrow(con::createMBean, + "a missing pool MBean must not fail the connection's registration"); + Assertions.assertNull(con.getObjectName(), + "nothing may be registered while the pool has no JMX object"); + } finally { + pool.close(true); + } + Assertions.assertEquals(0, poolSessions(), + "an unconnected pooled connection may not own a session"); + } + + @Test + void failingMBeanRegistrationReturnsTheConnectionInsteadOfStrandingIt() throws Exception { + FailingRegistrationPool pool = new FailingRegistrationPool(newPoolProperties()); + try { + // The setup path must reach the registration even with JMX + // disabled, so this is the failure the borrow has to survive; a + // gate on the pool's JMX field in front of the call would let the + // check-and-use window back in. + SQLException x = Assertions.assertThrows(SQLException.class, pool::getConnection, + "a failing MBean registration must fail the borrow through the pool's own exception"); + Assertions.assertTrue(x.getCause() instanceof IllegalStateException, + "the borrow must report the registration failure, was: " + x.getCause()); + + // The stranding defect: the connection was already in busy when + // setup failed, so only the abandonSetup() path can hand it back. + Assertions.assertEquals(0, pool.getActive(), + "a setup that must not strand the connection may not leave it in busy"); + Assertions.assertEquals(1, pool.getIdle(), + "the connection must go back to the idle queue, not be discarded"); + Assertions.assertEquals(1, pool.getBorrowedCount(), "the failed borrow is counted"); + Assertions.assertEquals(1, pool.getReturnedCount(), + "the hand-back after the failed setup is counted as a return"); + Assertions.assertEquals(1, poolSessions(), + "the physical connection must still be owned by the pool"); + + // The slot the borrow needed is intact: with maxActive=1, a + // stranded connection would exhaust the pool right here. + pool.failRegistration.set(false); + Connection borrowed = pool.getConnection(); + Assertions.assertNotNull(borrowed, "the pool must still be able to hand out its connection"); + borrowed.close(); + Assertions.assertEquals(0, pool.getActive()); + Assertions.assertEquals(1, pool.getIdle()); + } finally { + pool.close(true); + } + awaitSessions(0); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
