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

[Aleksei Efimov]

On Thu, 24 Aug 2023 00:10: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.
>
> Weibing Xiao has updated the pull request incrementally with one additional commit since the last revision:
> 
>   refactoring the code according to the comment

The last change reformatted several lines unrelated to this change - I've commented on a few occurrences. Could you please keep only changes related to the scope of this PR?

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

> 60: 
> 61: /**
> 62:   * A thread that creates a connection to an LDAP server.

The formatting changes here is not related to the code changes under review in this PR. Could you please revert them?

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

> 126: 
> 127:     public final String host;  // used by LdapClient for generating exception messages
> 128:     // used by StartTlsResponse when creating an SSL socket

Same - reformatting of unrelated code

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

> 131:                          // used by StartTlsResponse when creating an SSL socket
> 132:     public final int port;     // used by LdapClient for generating exception messages
> 133:                          // used by StartTlsResponse when creating an SSL socket

Same - reformatting of unrelated code

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

> 244: 
> 245:             CommunicationException ce =
> 246:                 new CommunicationException(host + ":" + port);

Same - reformatting of unrelated code

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

> 252:             // Also catches all IO errors generated by socket creation.
> 253:             CommunicationException ce =
> 254:                 new CommunicationException(host + ":" + port);

Same - reformatting of unrelated code

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

> 352:     private void initialSSLHandshake(Socket socket, int connectTimeout) throws Exception {
> 353: 
> 354:         if (socket instanceof SSLSocket) {

instanceof pattern matching can be used here:

        if (socket instanceof SSLSocket sslSocket) {

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

> 385: 
> 386:     LdapRequest writeRequest(BerEncoder ber, int msgId,
> 387:         boolean pauseAfterReceipt) throws IOException {

Reformatting of unrelated code

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

> 389: 
> 390:     LdapRequest writeRequest(BerEncoder ber, int msgId,
> 391:                              boolean pauseAfterReceipt, int replyQueueCapacity) throws IOException {

Reformatting of unrelated code

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

> 392: 
> 393:         LdapRequest req =
> 394:                 new LdapRequest(msgId, pauseAfterReceipt, replyQueueCapacity);

Reformatting of unrelated code

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

> 433:             if (sock == null) {
> 434:                 throw new ServiceUnavailableException(host + ":" + port +
> 435:                     "; socket closed");

Reformatting of unrelated code

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

> 617: 
> 618:     /**
> 619:      * @param reqCtls Possibly null request controls that accompanies the

Reformatting of unrelated code

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

PR Review: https://git.openjdk.org/jdk/pull/15294#pullrequestreview-1593589706
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304321768
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304322767
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304322965
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304324031
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304324165
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304326820
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304329352
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304330163
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304330374
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304330800
PR Review Comment: https://git.openjdk.org/jdk/pull/15294#discussion_r1304331645