RFR: 8314063 : The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection

[Aleksei Efimov]

On Tue, 15 Aug 2023 17:30:54 GMT, Weibing Xiao <duke at openjdk.org> wrote:

> Please refer to JDK-8314063.
> 
> The failure scenario is due to the setting of connection timeout. It is either too small or not an optimal value for the system. When the client tries to connect to the server with LDAPs protocol. It requires the handshake after the socket is created and connected, but it fails due to connection timeout and leaves the socket open. It is not closed properly due to the exception handling in the JDK code.
> 
> The change is adding a try/catch block and closing the socket in the catch block,  and the format of the code got changed consequently.

Hello Weibing,
The approach taken here seems resonable to me. Please find my comments on the fix and the test below:

src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 284:

> 282: 
> 283:         Socket socket = null;
> 284:         try {

This method is hard to read. Readability could be improved by adding/splitting it into two new methods:
 - one for creating a socket with a use of specified socket factory
 - another one for creating a socket with `new Socket`

src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 369:

> 367:                 }
> 368:             }
> 369:         } catch (Exception e) {

The code wrapped in this try-catch block can throw unchecked exceptions, for example `SecurityException` thrown by `Socket.connect`. For such cases the newly created socket remain open.

src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 372:

> 370:             // 8314063 the socket is not closed after the failure of handshake
> 371:             if (socket != null && !socket.isClosed()) {
> 372:                 socket.close();

The original exception can be lost if `socket.close()` fails with `IOException` here

test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 64:

> 62:         setKeyStore();
> 63:         // start the test server first.
> 64:         TestServer server = new TestServer();

Since the `TestServer` implements `AutoClosable`, can we take advantage of that and of try-with-resources to close the server socket. One possibility is to move test server, url and environment table creation into try-catch block, and convert it to try-with-resources (`TestServer`).

test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 66:

> 64:         TestServer server = new TestServer();
> 65:         server.start();
> 66:         url = "ldaps://localhost:" + server.getPortNumber();

The `URIBuilder` utility class from a test library  can be used to construct a test server url here:

URI uri = URIBuilder.newBuilder()
.scheme("ldaps")
.loopback()
.port(server.getPortNumber())
.build();

test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 78:

> 76:         try {
> 77:             LdapContext ctx = new InitialLdapContext(env, null);
> 78:             ctx.close();

The context can be closed in finally block

test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 151:

> 149:         private volatile boolean exceptionThrown;
> 150: 
> 151:         TestServer() throws IOException {

`IOException` is not thrown by this constructor - can be removed

test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 154:

> 152:             try {
> 153:                 SSLServerSocketFactory socketFactory = (SSLServerSocketFactory) SSLServerSocketFactory.getDefault();
> 154:                 serverSocket = socketFactory.createServerSocket(0);

The test server can be bound to the loopback address here and `URIBuilder` test library class can be used to construct the `URL` as mentioned above:
`socketFactory.createServerSocket(0, 0, InetAddress.getLoopbackAddress());`

test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 166:

> 164:         }
> 165: 
> 166:         public boolean isExceptionThrown() {

`isExceptionThrown` method is not used by the test

test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java line 172:

> 170:         @Override
> 171:         public void run() {
> 172:             try (Socket socket = serverSocket.accept()) {

Can be beautified a bit by putting socket, IS and OS into one try-with-resources statement. Something like:

            try (Socket socket = serverSocket.accept();
                 InputStream in = socket.getInputStream();
                 OutputStream out = socket.getOutputStream()) {

-------------

PR Review: https://git.openjdk.org/jdk/pull/15294#pullrequestreview-1581104126
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296223224
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296269582
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296224057
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296226004
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296228008
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296229176
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296230459
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296232557
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296235296
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1296236949