Message-ID: From: "vlsi (@vlsi)" To: "pgjdbc/pgjdbc" Date: Mon, 08 Jun 2026 13:14:24 +0000 Subject: [pgjdbc/pgjdbc] PR #4161: fix: close socket when PgConnection setup fails after connect List-Id: X-GitHub-Additions: 210 X-GitHub-Author-Id: 213894 X-GitHub-Author-Login: vlsi X-GitHub-Base: master X-GitHub-Changed-Files: 2 X-GitHub-Commits: 1 X-GitHub-Deletions: 93 X-GitHub-Head-Branch: fix/pgconnection-socket-leak-on-setup-failure X-GitHub-Head-SHA: 2ee0208f4464d9e1e8e79a6c15bc6ec25aa13f24 X-GitHub-Issue: 4161 X-GitHub-Labels: bug X-GitHub-Merge-SHA: 05b134ab128a095359574c2f2b516fe3d1d2dc39 X-GitHub-Merged-By: vlsi X-GitHub-Repo: pgjdbc/pgjdbc X-GitHub-State: merged X-GitHub-Type: pull_request X-GitHub-Url: https://github.com/pgjdbc/pgjdbc/pull/4161 Content-Type: text/plain; charset=utf-8 ### Why `PgConnection`'s constructor opens the network connection with `ConnectionFactory.openConnection(...)`, then runs a long sequence of setup steps — `SET SESSION CHARACTERISTICS`, `COMMIT`/`ROLLBACK` query preparation, `initObjectTypes`, type-cache setup, and so on — before registering the connection with the `Cleaner` on the constructor's last line. If any of those steps fails (a server error on a `SET`, a mid-setup network drop surfacing as `IOException`/`SQLException`, a `RuntimeException`, or a thread interrupt at an interruptible point), the constructor throws without ever closing `queryExecutor`. The half-built `PgConnection` is never assigned to a variable (`Driver.makeConnection` just does `return new PgConnection(...)`) and the `Cleaner` has not been registered yet, so nothing closes the open socket and it leaks. The cleanup inside `ConnectionFactoryImpl` (`tryConnect` / `openConnectionImpl`, which call `closeStream(...)` on failure) only covers failures *inside* the factory. The gap is the `PgConnection`-level setup that runs after the factory returns. This is a pre-existing issue, independent of the recent loginTimeout / `connectThreadFactory` work. ### What - Wrap the constructor body that runs after `ConnectionFactory.openConnection(...)` in a `try`/`catch (SQLException | RuntimeException | Error e)` that closes `queryExecutor` and rethrows the original failure, attaching any close error as a suppressed exception (mirroring the `closeStream(stream, e)` pattern in `ConnectionFactoryImpl`). - `QueryExecutorBase.close()` checks `isClosed()`, so closing again on a path that already closed the executor is a safe no-op — there is no double-close hazard. - Add `ConnectionSetupFailureTest`. It forces a failure *after* connect with an invalid `stringtype` value (validated only inside the constructor, after the socket is open) and asserts, through a small nested recording `SocketFactory`, that every socket handed out is closed. ### How to verify ``` ./gradlew --quiet :postgresql:compileJava ./gradlew --quiet :postgresql:checkstyleMain :postgresql:autostyleJavaCheck ./gradlew --quiet :postgresql:test --tests "org.postgresql.test.jdbc2.ConnectionSetupFailureTest" ``` The new test fails on `master` (the socket to the server stays open) and passes with this change. The existing `ConnectionTest` suite continues to pass. ### Changes to Existing Features * This does not break existing behaviour: on the happy path the `try` completes normally and the constructor is unchanged. Only the failure path is affected — it now closes the socket instead of leaking it. ### New Feature Submissions 1. [x] Does your submission pass tests? 2. [x] Does `./gradlew styleCheck` pass? 3. [ ] N/A — tests are discovered by JUnit 5; this module no longer uses hand-maintained test suites. 🤖 Generated with [Claude Code](https://claude.com/claude-code) diff --git a/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java b/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java index 1e03d1b4bd..c6351adacc 100644 --- a/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java +++ b/pgjdbc/src/main/java/org/postgresql/jdbc/PgConnection.java @@ -289,123 +289,137 @@ public PgConnection(HostSpec[] hostSpecs, // Now make the initial connection and set up local state this.queryExecutor = ConnectionFactory.openConnection(hostSpecs, info); - // WARNING for unsupported servers (9.0 and lower are not supported) - if (LOGGER.isLoggable(Level.WARNING) && !haveMinimumServerVersion(ServerVersion.v9_1)) { - LOGGER.log(Level.WARNING, "Unsupported Server Version: {0}", queryExecutor.getServerVersion()); - } + // The socket is open now, but the connection is only registered with the Cleaner at the very + // end of this constructor. If any setup step below fails, close queryExecutor explicitly so + // the socket is not leaked, then rethrow the original failure. + try { + // WARNING for unsupported servers (9.0 and lower are not supported) + if (LOGGER.isLoggable(Level.WARNING) && !haveMinimumServerVersion(ServerVersion.v9_1)) { + LOGGER.log(Level.WARNING, "Unsupported Server Version: {0}", queryExecutor.getServerVersion()); + } - setSessionReadOnly = createQuery("SET SESSION CHARACTERISTICS AS TRANSACTION READ ONLY", false, true); - setSessionNotReadOnly = createQuery("SET SESSION CHARACTERISTICS AS TRANSACTION READ WRITE", false, true); + setSessionReadOnly = createQuery("SET SESSION CHARACTERISTICS AS TRANSACTION READ ONLY", false, true); + setSessionNotReadOnly = createQuery("SET SESSION CHARACTERISTICS AS TRANSACTION READ WRITE", false, true); - // Set read-only early if requested - if (PGProperty.READ_ONLY.getBoolean(info)) { - setReadOnly(true); - } + // Set read-only early if requested + if (PGProperty.READ_ONLY.getBoolean(info)) { + setReadOnly(true); + } - this.hideUnprivilegedObjects = PGProperty.HIDE_UNPRIVILEGED_OBJECTS.getBoolean(info); + this.hideUnprivilegedObjects = PGProperty.HIDE_UNPRIVILEGED_OBJECTS.getBoolean(info); - // Default is true: DDL transparently invalidates the prepared-statement - // cache so the driver re-prepares server plans rather than surfacing - // "cached plan must not change result type" to callers. - queryExecutor.setFlushCacheOnDdl(PGProperty.FLUSH_CACHE_ON_DDL.getBoolean(info)); + // Default is true: DDL transparently invalidates the prepared-statement + // cache so the driver re-prepares server plans rather than surfacing + // "cached plan must not change result type" to callers. + queryExecutor.setFlushCacheOnDdl(PGProperty.FLUSH_CACHE_ON_DDL.getBoolean(info)); - // get oids that support binary transfer - Set binaryOids = getBinaryEnabledOids(info); - // get oids that should be disabled from transfer - binaryDisabledOids = getBinaryDisabledOids(info); - // if there are any, remove them from the enabled ones - if (!binaryDisabledOids.isEmpty()) { - binaryOids.removeAll(binaryDisabledOids); - } + // get oids that support binary transfer + Set binaryOids = getBinaryEnabledOids(info); + // get oids that should be disabled from transfer + binaryDisabledOids = getBinaryDisabledOids(info); + // if there are any, remove them from the enabled ones + if (!binaryDisabledOids.isEmpty()) { + binaryOids.removeAll(binaryDisabledOids); + } - // split for receive and send for better control - Set useBinarySendForOids = new HashSet<>(binaryOids); + // split for receive and send for better control + Set useBinarySendForOids = new HashSet<>(binaryOids); - Set useBinaryReceiveForOids = new HashSet<>(binaryOids); + Set useBinaryReceiveForOids = new HashSet<>(binaryOids); - /* - * Does not pass unit tests because unit tests expect setDate to have millisecond accuracy - * whereas the binary transfer only supports date accuracy. - */ - useBinarySendForOids.remove(Oid.DATE); + /* + * Does not pass unit tests because unit tests expect setDate to have millisecond accuracy + * whereas the binary transfer only supports date accuracy. + */ + useBinarySendForOids.remove(Oid.DATE); - queryExecutor.setBinaryReceiveOids(useBinaryReceiveForOids); - queryExecutor.setBinarySendOids(useBinarySendForOids); + queryExecutor.setBinaryReceiveOids(useBinaryReceiveForOids); + queryExecutor.setBinarySendOids(useBinarySendForOids); - if (LOGGER.isLoggable(Level.FINEST)) { - LOGGER.log(Level.FINEST, " types using binary send = {0}", oidsToString(useBinarySendForOids)); - LOGGER.log(Level.FINEST, " types using binary receive = {0}", oidsToString(useBinaryReceiveForOids)); - LOGGER.log(Level.FINEST, " integer date/time = {0}", queryExecutor.getIntegerDateTimes()); - } + if (LOGGER.isLoggable(Level.FINEST)) { + LOGGER.log(Level.FINEST, " types using binary send = {0}", oidsToString(useBinarySendForOids)); + LOGGER.log(Level.FINEST, " types using binary receive = {0}", oidsToString(useBinaryReceiveForOids)); + LOGGER.log(Level.FINEST, " integer date/time = {0}", queryExecutor.getIntegerDateTimes()); + } - // - // String -> text or unknown? - // + // + // String -> text or unknown? + // - String stringType = PGProperty.STRING_TYPE.getOrDefault(info); - if (stringType != null) { - if ("unspecified".equalsIgnoreCase(stringType)) { - bindStringAsVarchar = false; - } else if ("varchar".equalsIgnoreCase(stringType)) { - bindStringAsVarchar = true; + String stringType = PGProperty.STRING_TYPE.getOrDefault(info); + if (stringType != null) { + if ("unspecified".equalsIgnoreCase(stringType)) { + bindStringAsVarchar = false; + } else if ("varchar".equalsIgnoreCase(stringType)) { + bindStringAsVarchar = true; + } else { + throw new PSQLException( + GT.tr("Unsupported value for stringtype parameter: {0}", stringType), + PSQLState.INVALID_PARAMETER_VALUE); + } } else { - throw new PSQLException( - GT.tr("Unsupported value for stringtype parameter: {0}", stringType), - PSQLState.INVALID_PARAMETER_VALUE); + bindStringAsVarchar = true; } - } else { - bindStringAsVarchar = true; - } - - // Initialize timestamp stuff - timestampUtils = new TimestampUtils(!queryExecutor.getIntegerDateTimes(), - new QueryExecutorTimeZoneProvider(queryExecutor)); - // Initialize common queries. - // isParameterized==true so full parse is performed and the engine knows the query - // is not a compound query with ; inside, so it could use parse/bind/exec messages - commitQuery = createQuery("COMMIT", false, true).query; - rollbackQuery = createQuery("ROLLBACK", false, true).query; + // Initialize timestamp stuff + timestampUtils = new TimestampUtils(!queryExecutor.getIntegerDateTimes(), + new QueryExecutorTimeZoneProvider(queryExecutor)); - int unknownLength = PGProperty.UNKNOWN_LENGTH.getInt(info); + // Initialize common queries. + // isParameterized==true so full parse is performed and the engine knows the query + // is not a compound query with ; inside, so it could use parse/bind/exec messages + commitQuery = createQuery("COMMIT", false, true).query; + rollbackQuery = createQuery("ROLLBACK", false, true).query; - // Initialize object handling - @SuppressWarnings("argument") - TypeInfo typeCache = createTypeInfo(this, unknownLength); - this.typeCache = typeCache; - initObjectTypes(info); + int unknownLength = PGProperty.UNKNOWN_LENGTH.getInt(info); - if (PGProperty.LOG_UNCLOSED_CONNECTIONS.getBoolean(info)) { - openStackTrace = new Throwable("Connection was created at this point:"); - } - finalizeAction = new PgConnectionCleaningAction(lock, openStackTrace, queryExecutor.getCloseAction()); - this.logServerErrorDetail = PGProperty.LOG_SERVER_ERROR_DETAIL.getBoolean(info); - this.disableColumnSanitiser = PGProperty.DISABLE_COLUMN_SANITISER.getBoolean(info); - this.convertBooleanToNumeric = PGProperty.CONVERT_BOOLEAN_TO_NUMERIC.getBoolean(info); + // Initialize object handling + @SuppressWarnings("argument") + TypeInfo typeCache = createTypeInfo(this, unknownLength); + this.typeCache = typeCache; + initObjectTypes(info); - if (haveMinimumServerVersion(ServerVersion.v8_3)) { - typeCache.addCoreType("uuid", Oid.UUID, Types.OTHER, "java.util.UUID", Oid.UUID_ARRAY); - typeCache.addCoreType("xml", Oid.XML, Types.SQLXML, "java.sql.SQLXML", Oid.XML_ARRAY); - } + if (PGProperty.LOG_UNCLOSED_CONNECTIONS.getBoolean(info)) { + openStackTrace = new Throwable("Connection was created at this point:"); + } + finalizeAction = new PgConnectionCleaningAction(lock, openStackTrace, queryExecutor.getCloseAction()); + this.logServerErrorDetail = PGProperty.LOG_SERVER_ERROR_DETAIL.getBoolean(info); + this.disableColumnSanitiser = PGProperty.DISABLE_COLUMN_SANITISER.getBoolean(info); + this.convertBooleanToNumeric = PGProperty.CONVERT_BOOLEAN_TO_NUMERIC.getBoolean(info); + + if (haveMinimumServerVersion(ServerVersion.v8_3)) { + typeCache.addCoreType("uuid", Oid.UUID, Types.OTHER, "java.util.UUID", Oid.UUID_ARRAY); + typeCache.addCoreType("xml", Oid.XML, Types.SQLXML, "java.sql.SQLXML", Oid.XML_ARRAY); + } - this.clientInfo = new Properties(); - if (haveMinimumServerVersion(ServerVersion.v9_0)) { - String appName = PGProperty.APPLICATION_NAME.getOrDefault(info); - if (appName == null) { - appName = ""; + this.clientInfo = new Properties(); + if (haveMinimumServerVersion(ServerVersion.v9_0)) { + String appName = PGProperty.APPLICATION_NAME.getOrDefault(info); + if (appName == null) { + appName = ""; + } + this.clientInfo.put("ApplicationName", appName); } - this.clientInfo.put("ApplicationName", appName); - } - fieldMetadataCache = new LruCache<>( - Math.max(0, PGProperty.DATABASE_METADATA_CACHE_FIELDS.getInt(info)), - Math.max(0, PGProperty.DATABASE_METADATA_CACHE_FIELDS_MIB.getInt(info) * 1024L * 1024L), - false); + fieldMetadataCache = new LruCache<>( + Math.max(0, PGProperty.DATABASE_METADATA_CACHE_FIELDS.getInt(info)), + Math.max(0, PGProperty.DATABASE_METADATA_CACHE_FIELDS_MIB.getInt(info) * 1024L * 1024L), + false); - replicationConnection = PGProperty.REPLICATION.getOrDefault(info) != null; + replicationConnection = PGProperty.REPLICATION.getOrDefault(info) != null; - xmlFactoryFactoryClass = PGProperty.XML_FACTORY_FACTORY.getOrDefault(info); - cleanable = LazyCleanerImpl.getInstance().register(leakHandle, finalizeAction); + xmlFactoryFactoryClass = PGProperty.XML_FACTORY_FACTORY.getOrDefault(info); + cleanable = LazyCleanerImpl.getInstance().register(leakHandle, finalizeAction); + } catch (SQLException | RuntimeException | Error e) { + // close() is idempotent (QueryExecutorBase.close checks isClosed), so this is a safe no-op + // if a setup step already closed the executor. + try { + queryExecutor.close(); + } catch (Exception suppressed) { + e.addSuppressed(suppressed); + } + throw e; + } } private static ReadOnlyBehavior getReadOnlyBehavior(@Nullable String property) { diff --git a/pgjdbc/src/test/java/org/postgresql/test/jdbc2/ConnectionSetupFailureTest.java b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/ConnectionSetupFailureTest.java new file mode 100644 index 0000000000..0d3e0f78f7 --- /dev/null +++ b/pgjdbc/src/test/java/org/postgresql/test/jdbc2/ConnectionSetupFailureTest.java @@ -0,0 +1,103 @@ +/* + * Copyright (c) 2026, PostgreSQL Global Development Group + * See the LICENSE file in the project root for more information. + */ + +package org.postgresql.test.jdbc2; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.postgresql.PGProperty; +import org.postgresql.test.TestUtil; + +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.net.InetAddress; +import java.net.Socket; +import java.sql.SQLException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Properties; + +import javax.net.SocketFactory; + +/** + * Verifies that the underlying socket is closed when {@code PgConnection} setup fails after the + * network connection has already been established. The connection is opened successfully by + * {@code ConnectionFactory.openConnection}, then a later setup step throws, so the socket must be + * closed by the constructor's cleanup path rather than leaked. + */ +class ConnectionSetupFailureTest { + + /** + * A {@link SocketFactory} that records every socket it hands out, so the test can assert the + * sockets were closed. It is referenced by class name through the {@code socketFactory} + * connection property and instantiated by {@code ObjectFactory} via its implicit no-argument + * constructor. + */ + public static class RecordingSocketFactory extends SocketFactory { + static final List SOCKETS = Collections.synchronizedList(new ArrayList<>()); + + static void reset() { + SOCKETS.clear(); + } + + private static Socket record(Socket socket) { + SOCKETS.add(socket); + return socket; + } + + @Override + public Socket createSocket() { + return record(new Socket()); + } + + @Override + public Socket createSocket(String host, int port) throws IOException { + return record(new Socket(host, port)); + } + + @Override + public Socket createSocket(String host, int port, InetAddress localHost, int localPort) + throws IOException { + return record(new Socket(host, port, localHost, localPort)); + } + + @Override + public Socket createSocket(InetAddress host, int port) throws IOException { + return record(new Socket(host, port)); + } + + @Override + public Socket createSocket(InetAddress address, int port, InetAddress localAddress, + int localPort) throws IOException { + return record(new Socket(address, port, localAddress, localPort)); + } + } + + @Test + void socketClosedWhenSetupFailsAfterConnect() throws Exception { + RecordingSocketFactory.reset(); + + Properties props = new Properties(); + PGProperty.SOCKET_FACTORY.set(props, RecordingSocketFactory.class.getName()); + // An invalid stringtype value is validated only inside the PgConnection constructor, after the + // socket has already been opened, so it triggers the post-openConnection cleanup path. + PGProperty.STRING_TYPE.set(props, "this-is-not-a-valid-stringtype"); + + SQLException ex = assertThrows(SQLException.class, () -> TestUtil.openDB(props)); + assertTrue(ex.getMessage().contains("stringtype"), + "Expected a stringtype validation failure, but got: " + ex.getMessage()); + + assertFalse(RecordingSocketFactory.SOCKETS.isEmpty(), + "The connection attempt should have created at least one socket"); + for (Socket socket : RecordingSocketFactory.SOCKETS) { + assertTrue(socket.isClosed(), + "Socket should be closed after the failed connection setup: " + socket); + } + } +}