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

[Aleksei Efimov]

On Tue, 22 Aug 2023 18:21:17 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.
>
> Weibing Xiao has updated the pull request incrementally with one additional commit since the last revision:
> 
>   format the code

May I suggest a slightly different approach for refactoring of the Connection class code here:
Instead of having two methods for creating a `Socket` with OR without socket factory the method to select a socket factory can be introduced: 
If a socket factory class name is not set - the factory returned by `SocketFactory.getDefault()` will be used (which is identical to `s = new Socket()` for calls with timeout set; and `s = new Socket(host, port)` for calls with no timeout set):

```    private SocketFactory getSocketFactory(String socketFactoryName) throws Exception {
    if (socketFactoryName == null) {
        if (debug) {
            System.err.println("Connection: using default SocketFactory");
        }
        return SocketFactory.getDefault();
    } else {
        if (debug) {
            System.err.println("Connection: loading supplied SocketFactory: " + socketFactoryName);
        }
        @SuppressWarnings("unchecked")
        Class<? extends SocketFactory> socketFactoryClass =
                (Class<? extends SocketFactory>) Obj.helper.loadClass(socketFactoryName);
        Method getDefault =
                socketFactoryClass.getMethod("getDefault");
        SocketFactory factory = (SocketFactory) getDefault.invoke(null, new Object[]{});
        return factory;
    }
}


It then can be used to create a connection socket:

    private Socket createConnectionSocket(String host, int port, SocketFactory factory, int connectTimeout) throws Exception {
        Socket socket = null;

        if (connectTimeout > 0) {
            // create unconnected socket and then connect it if timeout
            // is supplied
            InetSocketAddress endpoint =
                    createInetSocketAddress(host, port);
            // unconnected socket
            socket = factory.createSocket();
            // connect socket with a timeout
            socket.connect(endpoint, connectTimeout);
            if (debug) {
                System.err.println("Connection: creating socket with " +
                        "a connect timeout");
            }
        }
        if (socket == null) {
            // create connected socket
            socket = factory.createSocket(host, port);
            if (debug) {
                System.err.println("Connection: creating connected socket with" +
                        " no connect timeout");
            }
        }
        return socket;
    }


And then `createSocket` can be changed to:

    private Socket createSocket(String host, int port, String socketFactory,
                                int connectTimeout) throws Exception {

        SocketFactory factory = getSocketFactory(socketFactory);
        assert factory != null;
        Socket socket = createConnectionSocket(host, port, factory, connectTimeout);

        // the handshake for SSL connection with server and reset timeout for the socket
        try {
            initialSSLHandshake(socket, connectTimeout);
        } catch (Exception e) {
            // 8314063 the socket is not closed after the failure of handshake
            // close the socket while the error happened
            closeOpenedSocket(socket);
            throw e;
        }
        return socket;
    }


Also, the new try catch block can be put around initialSSLHandshake method only - it is the only case when socket can be set to a non-null value.

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

PR Comment: https://git.openjdk.org/jdk/pull/15294#issuecomment-1690278791