8346017: Socket.connect specified to throw UHE for unresolved address is problematic for SOCKS V5 proxy

vy · dfuch · commit 9e8aa855fe34 · 2024-12-18T09:43:06.000Z
Reviewed-by: dfuchs, alanb
diff --git a/src/java.base/share/classes/java/net/Socket.java b/src/java.base/share/classes/java/net/Socket.java
@@ -569,9 +569,8 @@ void setConnected() {
     /**
      * Connects this socket to the server.
      *
-     * <p> If the endpoint is an unresolved {@link InetSocketAddress}, or the
-     * connection cannot be established, then the socket is closed, and an
-     * {@link IOException} is thrown.
+     * <p> If the connection cannot be established, then the socket is closed,
+     * and an {@link IOException} is thrown.
      *
      * <p> This method is {@linkplain Thread#interrupt() interruptible} in the
      * following circumstances:
@@ -591,8 +590,8 @@ void setConnected() {
      * @param   endpoint the {@code SocketAddress}
      * @throws  IOException if an error occurs during the connection, the socket
      *          is already connected or the socket is closed
-     * @throws  UnknownHostException if the endpoint is an unresolved
-     *          {@link InetSocketAddress}
+     * @throws  UnknownHostException if the connection could not be established
+     *          because the endpoint is an unresolved {@link InetSocketAddress}
      * @throws  java.nio.channels.IllegalBlockingModeException
      *          if this socket has an associated channel,
      *          and the channel is in non-blocking mode
@@ -609,9 +608,8 @@ public void connect(SocketAddress endpoint) throws IOException {
      * A timeout of zero is interpreted as an infinite timeout. The connection
      * will then block until established or an error occurs.
      *
-     * <p> If the endpoint is an unresolved {@link InetSocketAddress}, the
-     * connection cannot be established, or the timeout expires before the
-     * connection is established, then the socket is closed, and an
+     * <p> If the connection cannot be established, or the timeout expires
+     * before the connection is established, then the socket is closed, and an
      * {@link IOException} is thrown.
      *
      * <p> This method is {@linkplain Thread#interrupt() interruptible} in the
@@ -634,8 +632,8 @@ public void connect(SocketAddress endpoint) throws IOException {
      * @throws  IOException if an error occurs during the connection, the socket
      *          is already connected or the socket is closed
      * @throws  SocketTimeoutException if timeout expires before connecting
-     * @throws  UnknownHostException if the endpoint is an unresolved
-     *          {@link InetSocketAddress}
+     * @throws  UnknownHostException if the connection could not be established
+     *          because the endpoint is an unresolved {@link InetSocketAddress}
      * @throws  java.nio.channels.IllegalBlockingModeException
      *          if this socket has an associated channel,
      *          and the channel is in non-blocking mode
@@ -660,12 +658,6 @@ public void connect(SocketAddress endpoint, int timeout) throws IOException {
         if (!(endpoint instanceof InetSocketAddress epoint))
             throw new IllegalArgumentException("Unsupported address type");
 
-        if (epoint.isUnresolved()) {
-            var uhe = new UnknownHostException(epoint.getHostName());
-            closeSuppressingExceptions(uhe);
-            throw uhe;
-        }
-
         InetAddress addr = epoint.getAddress();
         checkAddress(addr, "connect");
 
diff --git a/test/jdk/java/net/Socket/ConnectFailTest.java b/test/jdk/java/net/Socket/ConnectFailTest.java
@@ -29,13 +29,15 @@
 import java.io.IOException;
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
+import java.net.Proxy;
 import java.net.ServerSocket;
 import java.net.Socket;
 import java.net.SocketException;
 import java.net.UnknownHostException;
 import java.nio.channels.SocketChannel;
 import java.util.List;
 
+import static java.net.InetAddress.getLoopbackAddress;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -50,6 +52,8 @@
  */
 class ConnectFailTest {
 
+    // Implementation Note: Explicitly binding on the loopback address to avoid potential unstabilities.
+
     private static final int DEAD_SERVER_PORT = 0xDEAD;
 
     private static final InetSocketAddress REFUSING_SOCKET_ADDRESS = Utils.refusingEndpoint();
@@ -83,7 +87,7 @@ void testUnboundSocket(Socket socket) throws IOException {
     @MethodSource("sockets")
     void testBoundSocket(Socket socket) throws IOException {
         try (socket) {
-            socket.bind(new InetSocketAddress(0));
+            socket.bind(new InetSocketAddress(getLoopbackAddress(), 0));
             assertTrue(socket.isBound());
             assertFalse(socket.isConnected());
             assertThrows(IOException.class, () -> socket.connect(REFUSING_SOCKET_ADDRESS));
@@ -132,7 +136,7 @@ void testUnboundSocketWithUnresolvedAddress(Socket socket) throws IOException {
     @MethodSource("sockets")
     void testBoundSocketWithUnresolvedAddress(Socket socket) throws IOException {
         try (socket) {
-            socket.bind(new InetSocketAddress(0));
+            socket.bind(new InetSocketAddress(getLoopbackAddress(), 0));
             assertTrue(socket.isBound());
             assertFalse(socket.isConnected());
             assertThrows(UnknownHostException.class, () -> socket.connect(UNRESOLVED_ADDRESS));
@@ -161,7 +165,8 @@ static List<Socket> sockets() throws Exception {
         Socket socket = new Socket();
         @SuppressWarnings("resource")
         Socket channelSocket = SocketChannel.open().socket();
-        return List.of(socket, channelSocket);
+        Socket noProxySocket = new Socket(Proxy.NO_PROXY);
+        return List.of(socket, channelSocket, noProxySocket);
     }
 
     private static ServerSocket createEphemeralServerSocket() throws IOException {
diff --git a/test/jdk/java/net/Socket/ConnectSocksProxyTest.java b/test/jdk/java/net/Socket/ConnectSocksProxyTest.java
@@ -0,0 +1,218 @@
+/*
+ * Copyright (c) 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+import jdk.test.lib.Utils;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.net.Authenticator;
+import java.net.InetSocketAddress;
+import java.net.PasswordAuthentication;
+import java.net.Proxy;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.net.SocketException;
+
+import static java.net.InetAddress.getLoopbackAddress;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/*
+ * @test
+ * @bug 8346017
+ * @summary Verifies the `connect()` behaviour of a SOCKS proxy socket. In particular, that passing a resolvable
+ *          unresolved address doesn't throw an exception.
+ * @library /test/lib /java/net/Socks
+ * @build SocksServer
+ * @run junit ConnectSocksProxyTest
+ */
+class ConnectSocksProxyTest {
+
+    // Implementation Note: Explicitly binding on the loopback address to avoid potential unstabilities.
+
+    private static final int DEAD_SERVER_PORT = 0xDEAD;
+
+    private static final InetSocketAddress REFUSING_SOCKET_ADDRESS = Utils.refusingEndpoint();
+
+    private static final InetSocketAddress UNRESOLVED_ADDRESS =
+            InetSocketAddress.createUnresolved("no.such.host", DEAD_SERVER_PORT);
+
+    private static final String PROXY_AUTH_USERNAME = "foo";
+
+    private static final String PROXY_AUTH_PASSWORD = "bar";
+
+    private static SocksServer PROXY_SERVER;
+
+    private static Proxy PROXY;
+
+    @BeforeAll
+    static void initAuthenticator() {
+        Authenticator.setDefault(new Authenticator() {
+            @Override
+            protected PasswordAuthentication getPasswordAuthentication() {
+                return new PasswordAuthentication(PROXY_AUTH_USERNAME, PROXY_AUTH_PASSWORD.toCharArray());
+            }
+        });
+    }
+
+    @BeforeAll
+    static void initProxyServer() throws IOException {
+        PROXY_SERVER = new SocksServer(getLoopbackAddress(), 0, false);
+        PROXY_SERVER.addUser(PROXY_AUTH_USERNAME, PROXY_AUTH_PASSWORD);
+        PROXY_SERVER.start();
+        InetSocketAddress proxyAddress = new InetSocketAddress(getLoopbackAddress(), PROXY_SERVER.getPort());
+        PROXY = new Proxy(Proxy.Type.SOCKS, proxyAddress);
+    }
+
+    @AfterAll
+    static void stopProxyServer() {
+        PROXY_SERVER.close();
+    }
+
+    @Test
+    void testUnresolvedAddress() {
+        assertTrue(UNRESOLVED_ADDRESS.isUnresolved());
+    }
+
+    /**
+     * Verifies that an unbound socket is closed when {@code connect()} fails.
+     */
+    @Test
+    void testUnboundSocket() throws IOException {
+        try (Socket socket = createProxiedSocket()) {
+            assertFalse(socket.isBound());
+            assertFalse(socket.isConnected());
+            assertThrows(IOException.class, () -> socket.connect(REFUSING_SOCKET_ADDRESS));
+            assertTrue(socket.isClosed());
+        }
+    }
+
+    /**
+     * Verifies that a bound socket is closed when {@code connect()} fails.
+     */
+    @Test
+    void testBoundSocket() throws IOException {
+        try (Socket socket = createProxiedSocket()) {
+            socket.bind(new InetSocketAddress(getLoopbackAddress(), 0));
+            assertTrue(socket.isBound());
+            assertFalse(socket.isConnected());
+            assertThrows(IOException.class, () -> socket.connect(REFUSING_SOCKET_ADDRESS));
+            assertTrue(socket.isClosed());
+        }
+    }
+
+    /**
+     * Verifies that a connected socket is not closed when {@code connect()} fails.
+     */
+    @Test
+    void testConnectedSocket() throws Throwable {
+        try (Socket socket = createProxiedSocket();
+             ServerSocket serverSocket = createEphemeralServerSocket()) {
+            socket.connect(serverSocket.getLocalSocketAddress());
+            try (Socket _ = serverSocket.accept()) {
+                assertTrue(socket.isBound());
+                assertTrue(socket.isConnected());
+                SocketException exception = assertThrows(
+                        SocketException.class,
+                        () -> socket.connect(REFUSING_SOCKET_ADDRESS));
+                assertEquals("Already connected", exception.getMessage());
+                assertFalse(socket.isClosed());
+            }
+        }
+    }
+
+    /**
+     * Delegates to {@link #testUnconnectedSocketWithUnresolvedAddress(boolean, Socket)} using an unbound socket.
+     */
+    @Test
+    void testUnboundSocketWithUnresolvedAddress() throws IOException {
+        try (Socket socket = createProxiedSocket()) {
+            assertFalse(socket.isBound());
+            assertFalse(socket.isConnected());
+            testUnconnectedSocketWithUnresolvedAddress(false, socket);
+        }
+    }
+
+    /**
+     * Delegates to {@link #testUnconnectedSocketWithUnresolvedAddress(boolean, Socket)} using a bound socket.
+     */
+    @Test
+    void testBoundSocketWithUnresolvedAddress() throws IOException {
+        try (Socket socket = createProxiedSocket()) {
+            socket.bind(new InetSocketAddress(getLoopbackAddress(), 0));
+            testUnconnectedSocketWithUnresolvedAddress(true, socket);
+        }
+    }
+
+    /**
+     * Verifies the behaviour of an unconnected socket when {@code connect()} is invoked using an unresolved address.
+     */
+    private static void testUnconnectedSocketWithUnresolvedAddress(boolean bound, Socket socket) throws IOException {
+        assertEquals(bound, socket.isBound());
+        assertFalse(socket.isConnected());
+        try (ServerSocket serverSocket = createEphemeralServerSocket()) {
+            InetSocketAddress unresolvedAddress = InetSocketAddress.createUnresolved(
+                    getLoopbackAddress().getHostAddress(),
+                    serverSocket.getLocalPort());
+            socket.connect(unresolvedAddress);
+            try (Socket _ = serverSocket.accept()) {
+                assertTrue(socket.isBound());
+                assertTrue(socket.isConnected());
+                assertFalse(socket.isClosed());
+            }
+        }
+    }
+
+    /**
+     * Verifies that a connected socket is not closed when {@code connect()} is invoked using an unresolved address.
+     */
+    @Test
+    void testConnectedSocketWithUnresolvedAddress() throws Throwable {
+        try (Socket socket = createProxiedSocket();
+             ServerSocket serverSocket = createEphemeralServerSocket()) {
+            socket.connect(serverSocket.getLocalSocketAddress());
+            try (Socket _ = serverSocket.accept()) {
+                assertTrue(socket.isBound());
+                assertTrue(socket.isConnected());
+                SocketException exception = assertThrows(
+                        SocketException.class,
+                        () -> socket.connect(UNRESOLVED_ADDRESS));
+                assertEquals("Already connected", exception.getMessage());
+                assertFalse(socket.isClosed());
+            }
+        }
+    }
+
+    private static Socket createProxiedSocket() {
+        return new Socket(PROXY);
+    }
+
+    private static ServerSocket createEphemeralServerSocket() throws IOException {
+        return new ServerSocket(0, 0, getLoopbackAddress());
+    }
+
+}
