From 907e30ff00abd6cd4935987810d282f46ec07704 Mon Sep 17 00:00:00 2001
From: Christoph Langer <clanger@openjdk.org>
Date: Mon, 25 Mar 2024 16:31:52 +0000
Subject: [PATCH] 8325579: Inconsistent behavior in
 com.sun.jndi.ldap.Connection::createSocket

Reviewed-by: dfuchs, aefimov
---
 .../classes/com/sun/jndi/ldap/Connection.java |  47 +--
 .../share/classes/module-info.java            |  24 +-
 .../ldap/LdapSSLHandshakeFailureTest.java     | 272 +++++++++++-------
 3 files changed, 219 insertions(+), 124 deletions(-)

diff --git a/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java b/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java
index deccd70f878..f270a34d5b7 100644
--- a/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java
+++ b/src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1999, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1999, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -122,17 +122,15 @@ import javax.security.sasl.SaslException;
 public final class Connection implements Runnable {
 
     private static final boolean debug = false;
-    private static final int dump = 0; // > 0 r, > 1 rw
-
 
     private final Thread worker;    // Initialized in constructor
 
-    private boolean v3 = true;       // Set in setV3()
+    private boolean v3 = true;     // Set in setV3()
 
     public final String host;  // used by LdapClient for generating exception messages
-                         // used by StartTlsResponse when creating an SSL socket
+                               // used by StartTlsResponse when creating an SSL socket
     public final int port;     // used by LdapClient for generating exception messages
-                         // used by StartTlsResponse when creating an SSL socket
+                               // used by StartTlsResponse when creating an SSL socket
 
     private boolean bound = false;   // Set in setBound()
 
@@ -324,30 +322,37 @@ public final class Connection implements Runnable {
     }
 
     private Socket createConnectionSocket(String host, int port, SocketFactory factory,
-                                          int connectTimeout) throws Exception {
+                                          int connectTimeout) throws IOException {
         Socket socket = null;
 
+        // if timeout is supplied, try to use unconnected socket for connecting with timeout
         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");
+                System.err.println("Connection: creating socket with a connect timeout");
+            }
+            try {
+                // unconnected socket
+                socket = factory.createSocket();
+            } catch (IOException e) {
+                // unconnected socket is likely not supported by the SocketFactory
+                if (debug) {
+                    System.err.println("Connection: unconnected socket not supported by SocketFactory");
+                }
+            }
+            if (socket != null) {
+                InetSocketAddress endpoint = createInetSocketAddress(host, port);
+                // connect socket with a timeout
+                socket.connect(endpoint, connectTimeout);
             }
         }
+
+        // either no timeout was supplied or unconnected socket did not work
         if (socket == null) {
             // create connected socket
-            socket = factory.createSocket(host, port);
             if (debug) {
-                System.err.println("Connection: creating connected socket with" +
-                        " no connect timeout");
+                System.err.println("Connection: creating connected socket with no connect timeout");
             }
+            socket = factory.createSocket(host, port);
         }
         return socket;
     }
@@ -356,7 +361,7 @@ public final class Connection implements Runnable {
     // the SSL handshake following socket connection as part of the timeout.
     // So explicitly set a socket read timeout, trigger the SSL handshake,
     // then reset the timeout.
-    private void initialSSLHandshake(SSLSocket sslSocket , int connectTimeout) throws Exception {
+    private void initialSSLHandshake(SSLSocket sslSocket, int connectTimeout) throws Exception {
 
             if (!IS_HOSTNAME_VERIFICATION_DISABLED) {
                 SSLParameters param = sslSocket.getSSLParameters();
diff --git a/src/java.naming/share/classes/module-info.java b/src/java.naming/share/classes/module-info.java
index c4c7a606c6c..5a731000194 100644
--- a/src/java.naming/share/classes/module-info.java
+++ b/src/java.naming/share/classes/module-info.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2014, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -36,21 +36,33 @@
  * The following implementation specific environment properties are supported by the
  * default LDAP Naming Service Provider implementation in the JDK:
  * <ul>
+ *     <li>{@code java.naming.ldap.factory.socket}:
+ *         <br>The value of this environment property specifies the fully
+ *         qualified class name of the socket factory used by the LDAP provider.
+ *         This class must implement the {@link javax.net.SocketFactory} abstract class
+ *         and provide an implementation of the static "getDefault()" method that
+ *         returns an instance of the socket factory. By default the environment
+ *         property is not set.
+ *     </li>
  *     <li>{@code com.sun.jndi.ldap.connect.timeout}:
- *         <br>The value of this property is the string representation
- *         of an integer representing the connection timeout in
- *         milliseconds. If the LDAP provider cannot establish a
- *         connection within that period, it aborts the connection attempt.
+ *         <br>The value of this environment property is the string representation
+ *         of an integer specifying the connection timeout in milliseconds.
+ *         If the LDAP provider cannot establish a connection within that period,
+ *         it aborts the connection attempt.
  *         The integer should be greater than zero. An integer less than
  *         or equal to zero means to use the network protocol's (i.e., TCP's)
  *         timeout value.
  *         <br> If this property is not specified, the default is to wait
  *         for the connection to be established or until the underlying
  *         network times out.
+ *         <br> If a custom socket factory is provided via environment property
+ *         {@code java.naming.ldap.factory.socket} and unconnected sockets
+ *         are not supported, the specified timeout is ignored
+ *         and the provider behaves as if no connection timeout was set.
  *     </li>
  *     <li>{@code com.sun.jndi.ldap.read.timeout}:
  *         <br>The value of this property is the string representation
- *         of an integer representing the read timeout in milliseconds
+ *         of an integer specifying the read timeout in milliseconds
  *         for LDAP operations. If the LDAP provider cannot get a LDAP
  *         response within that period, it aborts the read attempt. The
  *         integer should be greater than zero. An integer less than or
diff --git a/test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java b/test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java
index 29f74d250f7..15d8f8b074c 100644
--- a/test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java
+++ b/test/jdk/com/sun/jndi/ldap/LdapSSLHandshakeFailureTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -21,68 +21,117 @@
  * questions.
  */
 
-import jdk.test.lib.net.URIBuilder;
-
-import javax.naming.Context;
-import javax.naming.ldap.InitialLdapContext;
-import javax.naming.ldap.LdapContext;
-import javax.net.SocketFactory;
-import javax.net.ssl.SSLServerSocketFactory;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.lang.reflect.Field;
 import java.net.InetAddress;
 import java.net.ServerSocket;
 import java.net.Socket;
 import java.net.SocketException;
+import java.net.UnknownHostException;
 import java.util.Hashtable;
 
+import javax.naming.Context;
+import javax.naming.NamingException;
+import javax.naming.ldap.InitialLdapContext;
+import javax.naming.ldap.LdapContext;
+import javax.net.SocketFactory;
+import javax.net.ssl.SSLServerSocketFactory;
+
+import jdk.test.lib.net.URIBuilder;
+
 /*
  * @test
- * @bug 8314063
+ * @bug 8314063 8325579
  * @library /test/lib
- * @summary For LDAPs connection, if the value of com.sun.jndi.ldap.connect.timeout is
- * set too small or not an optimal value for the system, after the socket is created and
- * connected to the server, but the handshake between the client and server fails due to
- * socket time out, the opened socket is not closed properly. In this test case, the server
- * is forced to sleep ten seconds and connection time out for client is one second. This
- * will allow the socket opened and connected, and give the chance for the handshake to be
- * timed out. Before this fix, the socket is kept opened. Right now the exception will be
- * caught and the socket will be closed.
+ * @summary Several scenarios for LDAP connection handshaking are tested here.
+ * We test different combinations of com.sun.jndi.ldap.connect.timeout values
+ * and server behavior, e.g. a server that replies immediately vs a server that
+ * delays the initial answer. We also try to check whether the underlying Socket
+ * object will be closed correctly.
+ * We expect exceptions when using a custom SocketFactory that does not supply
+ * SSL Sockets. In that case we instrument the supplied Socket object and check
+ * if it was properly closed after the handshake failure.
+ * When the value of com.sun.jndi.ldap.connect.timeout is set lower than the
+ * server delay, we also expect an exception.
+ * In all other cases a valid Context object shall be returned and we check
+ * whether the socket is closed after closing the Context.
  *
- * @run main/othervm LdapSSLHandshakeFailureTest LdapSSLHandshakeFailureTest$CustomSocketFactory true 6000
- * @run main/othervm LdapSSLHandshakeFailureTest -1000 true 6000
- * @run main/othervm LdapSSLHandshakeFailureTest -1000 false 6000
- * @run main/othervm LdapSSLHandshakeFailureTest 2000 false 6000
- * @run main/othervm LdapSSLHandshakeFailureTest 0 true 6000
- * @run main/othervm LdapSSLHandshakeFailureTest 0 false 6000
+ * @modules java.naming/javax.naming:+open java.naming/com.sun.jndi.ldap:+open
+ * @run main/othervm LdapSSLHandshakeFailureTest
  * @run main/othervm LdapSSLHandshakeFailureTest true
- * @run main/othervm LdapSSLHandshakeFailureTest false
+ * @run main/othervm LdapSSLHandshakeFailureTest 0
+ * @run main/othervm LdapSSLHandshakeFailureTest 0 true
+ * @run main/othervm LdapSSLHandshakeFailureTest 2000
+ * @run main/othervm LdapSSLHandshakeFailureTest 2000 true
+ * @run main/othervm LdapSSLHandshakeFailureTest -1000
+ * @run main/othervm LdapSSLHandshakeFailureTest LdapSSLHandshakeFailureTest$CustomSocketFactoryNoUnconnected
+ * @run main/othervm LdapSSLHandshakeFailureTest LdapSSLHandshakeFailureTest$CustomSocketFactoryNoUnconnected 1000
+ * @run main/othervm LdapSSLHandshakeFailureTest LdapSSLHandshakeFailureTest$CustomSocketFactoryNoUnconnected true
+ * @run main/othervm LdapSSLHandshakeFailureTest LdapSSLHandshakeFailureTest$CustomSocketFactoryNoUnconnected 1000 true
+ * @run main/othervm LdapSSLHandshakeFailureTest LdapSSLHandshakeFailureTest$CustomSocketFactory
+ * @run main/othervm LdapSSLHandshakeFailureTest LdapSSLHandshakeFailureTest$CustomSocketFactory 1000
+ * @run main/othervm LdapSSLHandshakeFailureTest LdapSSLHandshakeFailureTest$CustomSocketFactory true
+ * @run main/othervm LdapSSLHandshakeFailureTest LdapSSLHandshakeFailureTest$CustomSocketFactory 1000 true
  */
 
 public class LdapSSLHandshakeFailureTest {
-    private static String SOCKET_CLOSED_MSG = "The socket has been closed.";
+    private static int SERVER_SLEEPING_TIME = 4000;
+    private static String progArgs[];
+    private static int curArg;
+    private static String customSocketFactory;
+    private static Integer connectTimeout;
+    private static boolean serverSlowDown;
 
-    private static int serverSleepingTime = 5000;
+    private static String popArg() {
+        if (curArg >= progArgs.length) {
+            return null;
+        }
+        return progArgs[curArg++];
+    }
 
-    public static void main(String args[]) throws Exception {
+    private static void parseArgs(String args[]) {
+        progArgs = args;
+        curArg = 0;
+
+        String arg = popArg();
+        if (arg == null)
+            return;
+
+        if (arg.startsWith("LdapSSLHandshakeFailureTest$CustomSocketFactory")) {
+            customSocketFactory = arg;
+            arg = popArg();
+            if (arg == null)
+                return;
+        }
+
+        try {
+            connectTimeout = Integer.valueOf(arg);
+            arg = popArg();
+            if (arg == null)
+                return;
+        } catch (NumberFormatException e) {
+            // then it must be the boolean arg for serverSlowDown
+        }
+
+        serverSlowDown = Boolean.valueOf(arg);
+    }
+
+    public static void main(String args[]) {
+        parseArgs(args);
+
+        System.out.println("Testing " +
+            (customSocketFactory == null ? "without custom SocketFactory" : "with custom SocketFactory \"" + customSocketFactory + "\"") +
+            ", " + (connectTimeout == null ? "no connectTimeout" : "connectTimeout=" + connectTimeout + "") +
+            ", serverSlowDown=" + serverSlowDown);
 
         // Set the keystores
         setKeyStore();
-        boolean serverSlowDown = Boolean.valueOf(args[0]);
-        if (args.length == 2) {
-            serverSlowDown = Boolean.valueOf(args[1]);
-        }
 
-        if (args.length == 3) {
-            serverSleepingTime = Integer.valueOf(args[2]);
-        }
-
-        boolean hasCustomSocketFactory = args[0]
-                .equals("LdapSSLHandshakeFailureTest$CustomSocketFactory");
         // start the test server first.
-        try (TestServer server = new TestServer(serverSlowDown, serverSleepingTime)) {
+        try (TestServer server = new TestServer(serverSlowDown)) {
             server.start();
             Hashtable<String, Object> env = new Hashtable<>();
             env.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory");
@@ -93,15 +142,13 @@ public class LdapSSLHandshakeFailureTest {
                     .port(server.getPortNumber())
                     .buildUnchecked().toString());
 
-            if (hasCustomSocketFactory) {
-                env.put("java.naming.ldap.factory.socket", args[0]);
-                env.put("com.sun.jndi.ldap.connect.timeout", "1000");
+            if (customSocketFactory != null) {
+                env.put("java.naming.ldap.factory.socket", customSocketFactory);
             }
 
-            if (args.length == 2 && !hasCustomSocketFactory) {
-                env.put("com.sun.jndi.ldap.connect.timeout", args[0]);
+            if (connectTimeout != null) {
+                env.put("com.sun.jndi.ldap.connect.timeout", connectTimeout.toString());
             }
-
             env.put(Context.SECURITY_PROTOCOL, "ssl");
             env.put(Context.SECURITY_AUTHENTICATION, "Simple");
             env.put(Context.SECURITY_PRINCIPAL, "cn=principal");
@@ -109,62 +156,55 @@ public class LdapSSLHandshakeFailureTest {
             LdapContext ctx = null;
             try {
                 ctx = new InitialLdapContext(env, null);
-            } catch (Exception e) {
-                if (CustomSocketFactory.customSocket.closeMethodCalledCount() > 0
-                        && hasCustomSocketFactory
-                        && Boolean.valueOf(args[1])) {
-                    System.out.println(SOCKET_CLOSED_MSG);
+            } catch (NamingException e) {
+                if (customSocketFactory != null) {
+                    System.out.println("Caught expected Exception with custom SocketFactory (no SSL Socket).");
+                    if (CustomSocketFactory.customSocket.closeMethodCalledCount() <= 0) {
+                        throw new RuntimeException("Custom Socket was not closed.");
+                    }
+                } else if (connectTimeout > 0) {
+                    System.out.println("Caught expected Exception with connectTimeout > 0.");
                 } else {
                     throw e;
                 }
             } finally {
-                if (ctx != null)
+                if (ctx != null) {
+                    System.out.println("Context was created, closing it.");
+                    Socket sock = getSocket(ctx);
                     ctx.close();
+                    if (!sock.isClosed()) {
+                        throw new RuntimeException("Socket isn't closed");
+                    }
+                }
             }
+        } catch (Exception e) {
+            e.printStackTrace();
+            throw new RuntimeException(e);
         }
     }
 
-    public static class CustomSocketFactory extends SocketFactory {
-        private static CustomSocket customSocket;
-
-        public static CustomSocketFactory getDefault() {
-            return new CustomSocketFactory();
-        }
-
-        @Override
-        public Socket createSocket() throws SocketException {
-            customSocket = new CustomSocket();
-            return customSocket;
-        }
-
-        @Override
-        public Socket createSocket(String s, int timeout) {
-            return customSocket;
-        }
-
-        @Override
-        public Socket createSocket(String host, int port, InetAddress localHost,
-                                   int localPort) {
-            return customSocket;
-        }
-
-        @Override
-        public Socket createSocket(InetAddress host, int port) {
-            return customSocket;
-        }
-
-        @Override
-        public Socket createSocket(InetAddress address, int port,
-                                   InetAddress localAddress, int localPort) {
-            return customSocket;
-        }
+    private static Socket getSocket(LdapContext ctx) throws Exception {
+        Field defaultInitCtxField = ctx.getClass().getSuperclass().getSuperclass().getDeclaredField("defaultInitCtx");
+        defaultInitCtxField.setAccessible(true);
+        Object defaultInitCtx = defaultInitCtxField.get(ctx);
+        Field clntField = defaultInitCtx.getClass().getDeclaredField("clnt");
+        clntField.setAccessible(true);
+        Object clnt = clntField.get(defaultInitCtx);
+        Field connField = clnt.getClass().getDeclaredField("conn");
+        connField.setAccessible(true);
+        Object conn = connField.get(clnt);
+        return (Socket)conn.getClass().getDeclaredField("sock").get(conn);
     }
 
     private static class CustomSocket extends Socket {
-        private int closeMethodCalled = 0;
+        private int closeMethodCalled;
 
         public CustomSocket() {
-            closeMethodCalled = 0;
+            super();
+        }
+
+        public CustomSocket(String s, int port) throws IOException {
+            super(s, port);
         }
 
         public int closeMethodCalledCount() {
@@ -178,25 +218,65 @@ public class LdapSSLHandshakeFailureTest {
         }
     }
 
+    public static class CustomSocketFactoryNoUnconnected extends SocketFactory {
+        static CustomSocket customSocket;
+
+        public static SocketFactory getDefault() {
+            return new CustomSocketFactoryNoUnconnected();
+        }
+
+        @Override
+        public Socket createSocket(String s, int port) throws IOException {
+            customSocket = new CustomSocket(s, port);
+            return customSocket;
+        }
+
+        @Override
+        public Socket createSocket(String host, int port, InetAddress localHost, int localPort)
+                throws IOException, UnknownHostException {
+            return null;
+        }
+
+        @Override
+        public Socket createSocket(InetAddress host, int port) throws IOException {
+            return null;
+        }
+
+        @Override
+        public Socket createSocket(InetAddress address, int port, InetAddress localAddress, int localPort)
+                throws IOException {
+            return null;
+        }
+    }
+
+    public static class CustomSocketFactory extends CustomSocketFactoryNoUnconnected {
+        public static SocketFactory getDefault() {
+            return new CustomSocketFactory();
+        }
+
+        @Override
+        public Socket createSocket() throws SocketException {
+            customSocket = new CustomSocket();
+            return customSocket;
+        }
+    }
+
     private static void setKeyStore() {
+        String keystore = System.getProperty("test.src", ".") + File.separator + "ksWithSAN";
 
-        String fileName = "ksWithSAN", dir = System.getProperty("test.src", ".") + File.separator;
-
-        System.setProperty("javax.net.ssl.keyStore", dir + fileName);
+        System.setProperty("javax.net.ssl.keyStore", keystore);
         System.setProperty("javax.net.ssl.keyStorePassword", "welcome1");
-        System.setProperty("javax.net.ssl.trustStore", dir + fileName);
+        System.setProperty("javax.net.ssl.trustStore", keystore);
         System.setProperty("javax.net.ssl.trustStorePassword", "welcome1");
     }
 
     static class TestServer extends Thread implements AutoCloseable {
         private boolean isForceToSleep;
-        private int sleepingTime;
         private final ServerSocket serverSocket;
         private final int PORT;
 
-        private TestServer(boolean isForceToSleep, int sleepingTime) {
+        private TestServer(boolean isForceToSleep) {
             this.isForceToSleep = isForceToSleep;
-            this.sleepingTime = sleepingTime;
             try {
                 SSLServerSocketFactory socketFactory = (SSLServerSocketFactory) SSLServerSocketFactory.getDefault();
                 serverSocket = socketFactory.createServerSocket(0, 0, InetAddress.getLoopbackAddress());
@@ -217,7 +297,7 @@ public class LdapSSLHandshakeFailureTest {
                  InputStream in = socket.getInputStream();
                  OutputStream out = socket.getOutputStream()) {
                 if (isForceToSleep) {
-                    Thread.sleep(sleepingTime);
+                    Thread.sleep(SERVER_SLEEPING_TIME);
                 }
                 byte[] bindResponse = {0x30, 0x0C, 0x02, 0x01, 0x01, 0x61, 0x07, 0x0A,
                         0x01, 0x00, 0x04, 0x00, 0x04, 0x00};
@@ -233,7 +313,7 @@ public class LdapSSLHandshakeFailureTest {
                     in.skip(in.available());
                 }
             } catch (Exception e) {
-                e.printStackTrace();
+                // e.printStackTrace();
             }
         }
 
@@ -245,5 +325,3 @@ public class LdapSSLHandshakeFailureTest {
         }
     }
 }
-
-