This is an automated email from the ASF dual-hosted git repository. rmaucher pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit fb8c35c1c0eaf34040d63886efaf52c4e682e30b Author: opencode <[email protected]> AuthorDate: Fri Oct 30 15:59:37 2026 +0100 Reject a null connection returned by a configured DataSource The javax.sql.DataSource contract allows getConnection() to return null when no connection can be provided. PooledConnection.connectUsingDriver() has always rejected that, and the connection pool data source branch rejects it since 1f0836f371, but the plain DataSource branch of connectUsingDataSource() assigned whatever the provider returned and returned normally, so connect() treated a connection that was never created as a successful physical connect. What the application then saw depended on its configuration. With defaultAutoCommit, defaultReadOnly, defaultTransactionIsolation or defaultCatalog set, the block that applies the default connection state dereferenced the null field and the borrow failed with an error whose cause was a NullPointerException. Otherwise the connection was handed to validate(), where the guard added by c4274f6603 at least kept it from being given out, but ConnectionPool.createConnection() reported "Validation Query Failed" for a connection that had never existed, and a pool with initialSize set failed at startup the same way. Neither message mentioned the provider that had refused to supply a connection. That branch now rejects a null return with an SQLException naming the provider, in the shape the driver, XA data source and connection pool data source branches already use. The guard sits at the end of the branch that can produce a null, so every route out of connectUsingDataSource() leaves a non-null connection, and the @SuppressWarnings("null") that dismissed this hazard as an Eclipse false positive is gone now that the hazard is not there to dismiss. Add TestNullConnectionFromDataSource using a provider that returns null, asserting that the failure names the provider rather than reporting a validation failure, that applying a default connection state no longer produces a NullPointerException, that the overload used for borrowed credentials is guarded in the same way, and that the pool works normally once the provider starts handing out connections. --- .../apache/tomcat/jdbc/pool/PooledConnection.java | 8 +- .../pool/TestNullConnectionFromDataSource.java | 231 +++++++++++++++++++++ 2 files changed, 238 insertions(+), 1 deletion(-) 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 1977f05aa1..96a900e65c 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 @@ -297,7 +297,6 @@ public class PooledConnection implements PooledConnectionMBean { * * @throws SQLException if unable to obtain a connection from the DataSource */ - @SuppressWarnings("null") // Suppress false positive warnings protected void connectUsingDataSource() throws SQLException { String usr = null; String pwd = null; @@ -361,6 +360,13 @@ public class PooledConnection implements PooledConnectionMBean { } else { connection = ds.getConnection(); } + if (connection == null) { + // The DataSource contract allows getConnection to return null + // when no connection can be provided; keeping that null would + // surface later as a misleading validation failure, or as a + // null pointer when the default connection state is applied. + throw new SQLException("DataSource " + ds + " returned a null connection"); + } } else if (poolProperties.getDataSource() instanceof javax.sql.ConnectionPoolDataSource) { javax.sql.ConnectionPoolDataSource ds = (javax.sql.ConnectionPoolDataSource) poolProperties.getDataSource(); javax.sql.PooledConnection pooled = (usr != null && pwd != null) ? ds.getPooledConnection(usr, pwd) diff --git a/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestNullConnectionFromDataSource.java b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestNullConnectionFromDataSource.java new file mode 100644 index 0000000000..d4fea0aa98 --- /dev/null +++ b/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/pool/TestNullConnectionFromDataSource.java @@ -0,0 +1,231 @@ +/* + * 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.DriverManager; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.logging.Logger; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import org.apache.tomcat.jdbc.JdbcPoolBaseTest; + +/** + * The JDBC specification lets {@link javax.sql.DataSource#getConnection()} + * return null when the provider cannot hand out a connection - unlike + * {@link java.sql.Driver#connect(String, java.util.Properties)}, which must + * throw instead. A pool drawing its connections from a plain DataSource must + * therefore reject a null connection at once, the way the driver path and the + * XA and ConnectionPoolDataSource handle paths already do: accepting it let + * the connect "succeed" with no connection behind it, which the pool later + * reported as a failed validation query for a connection that was never + * created - or, once a default connection state (auto-commit, read-only, + * isolation, catalog) was configured, as a raw NullPointerException from the + * state setup. The null handling of the two handle-based providers is pinned + * by {@code TestXaConnectionLeak} and {@code TestConnectionPoolDataSourceLeak}. + */ +class TestNullConnectionFromDataSource extends JdbcPoolBaseTest { + + /** + * A spec-faithful DataSource entitled to hand back no connection: while + * refusal is armed, both overloads return null; once disarmed they hand + * out real H2 connections, so a test can also verify that a refusal is + * not fatal to the pool. The call counters pin which overload the pool + * used for the configured credentials. + */ + private static final class NullReturningDataSource implements javax.sql.DataSource { + + private final String url; + private final AtomicInteger noArgCalls = new AtomicInteger(); + private final AtomicInteger credentialedCalls = new AtomicInteger(); + private volatile boolean returnsNull = true; + + NullReturningDataSource(String url) { + this.url = url; + } + + @Override + public Connection getConnection() throws SQLException { + noArgCalls.incrementAndGet(); + return returnsNull ? null : DriverManager.getConnection(url, "sa", ""); + } + + @Override + public Connection getConnection(String username, String password) throws SQLException { + credentialedCalls.incrementAndGet(); + return returnsNull ? null : DriverManager.getConnection(url, username, password); + } + + @Override + public java.io.PrintWriter getLogWriter() { + return null; + } + + @Override + public void setLogWriter(java.io.PrintWriter out) { + // no logging + } + + @Override + public void setLoginTimeout(int seconds) { + // no timeout + } + + @Override + public int getLoginTimeout() { + return 0; + } + + @Override + public Logger getParentLogger() { + return Logger.getLogger(NullReturningDataSource.class.getName()); + } + + @Override + public <T> T unwrap(Class<T> iface) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isWrapperFor(Class<?> iface) { + return false; + } + } + + private DataSource nullProviderPool(NullReturningDataSource provider) { + DataSource ds = newDataSource(); + ds.getPoolProperties().setDataSource(provider); + ds.getPoolProperties().setInitialSize(0); + return ds; + } + + private static void assertNamesTheProvider(SQLException failure) { + Assertions.assertTrue( + failure.getMessage().contains(NullReturningDataSource.class.getName()), + "the failure must name the offending provider, was: " + failure.getMessage()); + Assertions.assertTrue(failure.getMessage().contains("null connection"), + "the failure must report that the provider returned no connection, was: " + + failure.getMessage()); + } + + @Test + void aNullConnectionFailsTheBorrowAndNamesTheProvider() throws Exception { + NullReturningDataSource provider = new NullReturningDataSource(getUrl()); + DataSource ds = nullProviderPool(provider); + + // Without any default connection state configured, a null connection + // used to survive connect() and surface as a failed connect-time + // validation of a connection that was never created. + SQLException failure = Assertions.assertThrows(SQLException.class, ds::getConnection, + "a provider that hands back no connection must fail the borrow"); + assertNamesTheProvider(failure); + Assertions.assertFalse(failure.getMessage().contains("Validation Query"), + "the failure must not blame validation for a connection that " + + "was never created, was: " + failure.getMessage()); + Assertions.assertEquals(1, provider.credentialedCalls.get(), + "the configured credentials must be offered to the provider"); + Assertions.assertEquals(0, provider.noArgCalls.get()); + awaitSessions(0); + } + + @Test + void aNullConnectionWithAConfiguredDefaultFailsWithoutANullPointerException() + throws Exception { + NullReturningDataSource provider = new NullReturningDataSource(getUrl()); + DataSource ds = nullProviderPool(provider); + ds.getPoolProperties().setDefaultAutoCommit(Boolean.TRUE); + + // Applying the configured default state used to dereference the null + // connection: the borrow failed with the message of a bare + // NullPointerException, wrapped only by the pool's generic error + // translation. + SQLException failure = Assertions.assertThrows(SQLException.class, ds::getConnection, + "a provider that hands back no connection must fail the borrow"); + Assertions.assertFalse(failure.getCause() instanceof NullPointerException, + "the borrow must not fail through the null-connection " + + "dereference of the default-state setup, cause was: " + + failure.getCause()); + assertNamesTheProvider(failure); + awaitSessions(0); + } + + @Test + void aDirectConnectWithAConfiguredDefaultRejectsTheNullConnection() throws Exception { + NullReturningDataSource provider = new NullReturningDataSource(getUrl()); + PoolProperties properties = new PoolProperties(); + properties.setUrl(getUrl()); + properties.setDriverClassName("org.h2.Driver"); + properties.setUsername("sa"); + properties.setPassword(""); + properties.setJmxEnabled(false); + properties.setInitialSize(0); + properties.setRemoveAbandoned(false); + properties.setTimeBetweenEvictionRunsMillis(-1); + properties.setDefaultReadOnly(Boolean.TRUE); + properties.setDataSource(provider); + + ConnectionPool pool = new ConnectionPool(properties); + PooledConnection con = new PooledConnection(properties, pool); + // The contract of connect() is an SQLException on any failure to + // establish the connection - never a NullPointerException from the + // default-state setup that follows it. + Assertions.assertThrows(SQLException.class, con::connect, + "a null connection must be rejected by the connect itself"); + Assertions.assertFalse(con.isInitialized()); + } + + @Test + void theNoArgumentOverloadIsGuardedAsTightly() throws Exception { + NullReturningDataSource provider = new NullReturningDataSource(getUrl()); + DataSource ds = manage(new DataSource()); + ds.setUrl(getUrl()); + ds.getPoolProperties().setJmxEnabled(false); + ds.getPoolProperties().setInitialSize(0); + ds.getPoolProperties().setMinIdle(0); + ds.getPoolProperties().setDataSource(provider); + // No credentials configured: the pool must take the no-argument + // overload, which is just as entitled to return null. + SQLException failure = Assertions.assertThrows(SQLException.class, ds::getConnection, + "a provider that hands back no connection must fail the borrow"); + assertNamesTheProvider(failure); + Assertions.assertEquals(1, provider.noArgCalls.get(), + "without configured credentials the no-argument overload must be used"); + Assertions.assertEquals(0, provider.credentialedCalls.get()); + awaitSessions(0); + } + + @Test + void thePoolStillServesBorrowersAfterTheProviderStopsReturningNull() throws Exception { + NullReturningDataSource provider = new NullReturningDataSource(getUrl()); + DataSource ds = nullProviderPool(provider); + + Assertions.assertThrows(SQLException.class, ds::getConnection, + "the refusal must fail the borrow"); + awaitSessions(0); + + provider.returnsNull = false; + try (Connection con = ds.getConnection(); + Statement st = con.createStatement()) { + st.execute("SELECT 1"); + } + awaitSessions(1); + } +} --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
