changeset 49315:ac95c7a76132

8198928: (so) SocketChannel connect may deadlock if closed at around same time that connect fails Reviewed-by: bpb, mli
author alanb
date Wed, 07 Mar 2018 07:13:55 +0000
parents 9ffbe8258541
children 4affaea00c05
files src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java src/java.base/unix/native/libnio/ch/Net.c src/java.base/unix/native/libnio/ch/SocketChannelImpl.c src/java.base/windows/native/libnio/ch/SocketChannelImpl.c test/jdk/java/nio/channels/AsyncCloseAndInterrupt.java test/jdk/java/nio/channels/SocketChannel/CloseDuringConnect.java
diffstat 6 files changed, 262 insertions(+), 156 deletions(-) [+]
line wrap: on
line diff
--- a/src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java	Tue Mar 06 10:51:26 2018 -0800
+++ b/src/java.base/share/classes/sun/nio/ch/SocketChannelImpl.java	Wed Mar 07 07:13:55 2018 +0000
@@ -588,12 +588,16 @@
 
     /**
      * Marks the beginning of a connect operation that might block.
-     *
+     * @param blocking true if configured blocking
+     * @param isa the remote address
      * @throws ClosedChannelException if the channel is closed
      * @throws AlreadyConnectedException if already connected
      * @throws ConnectionPendingException is a connection is pending
+     * @throws IOException if the pre-connect hook fails
      */
-    private void beginConnect(boolean blocking) throws ClosedChannelException {
+    private void beginConnect(boolean blocking, InetSocketAddress isa)
+        throws IOException
+    {
         if (blocking) {
             // set hook for Thread.interrupt
             begin();
@@ -604,6 +608,13 @@
                 throw new AlreadyConnectedException();
             if (state == ST_CONNECTIONPENDING)
                 throw new ConnectionPendingException();
+            assert state == ST_UNCONNECTED;
+            state = ST_CONNECTIONPENDING;
+
+            if (localAddress == null)
+                NetHooks.beforeTcpConnect(fd, isa.getAddress(), isa.getPort());
+            remoteAddress = isa;
+
             if (blocking)
                 readerThread = NativeThread.current();
         }
@@ -614,11 +625,21 @@
      *
      * @throws AsynchronousCloseException if the channel was closed due to this
      * thread being interrupted on a blocking connect operation.
+     * @throws IOException if completed and unable to obtain the local address
      */
     private void endConnect(boolean blocking, boolean completed)
-        throws AsynchronousCloseException
+        throws IOException
     {
         endRead(blocking, completed);
+
+        if (completed) {
+            synchronized (stateLock) {
+                if (state == ST_CONNECTIONPENDING) {
+                    localAddress = Net.localAddress(fd);
+                    state = ST_CONNECTED;
+                }
+            }
+        }
     }
 
     @Override
@@ -628,64 +649,37 @@
         if (sm != null)
             sm.checkConnect(isa.getAddress().getHostAddress(), isa.getPort());
 
-        readLock.lock();
+        InetAddress ia = isa.getAddress();
+        if (ia.isAnyLocalAddress())
+            ia = InetAddress.getLocalHost();
+
         try {
-            writeLock.lock();
+            readLock.lock();
             try {
-                // notify before-connect hook
-                synchronized (stateLock) {
-                    if (state == ST_UNCONNECTED && localAddress == null) {
-                        NetHooks.beforeTcpConnect(fd, isa.getAddress(), isa.getPort());
+                writeLock.lock();
+                try {
+                    int n = 0;
+                    boolean blocking = isBlocking();
+                    try {
+                        beginConnect(blocking, isa);
+                        do {
+                            n = Net.connect(fd, ia, isa.getPort());
+                        } while (n == IOStatus.INTERRUPTED && isOpen());
+                    } finally {
+                        endConnect(blocking, (n > 0));
                     }
-                }
-
-                InetAddress ia = isa.getAddress();
-                if (ia.isAnyLocalAddress())
-                    ia = InetAddress.getLocalHost();
-
-                int n = 0;
-                boolean blocking = isBlocking();
-                try {
-                    try {
-                        beginConnect(blocking);
-                        if (blocking) {
-                            do {
-                                n = Net.connect(fd, ia, isa.getPort());
-                            } while (n == IOStatus.INTERRUPTED && isOpen());
-                        } else {
-                            n = Net.connect(fd, ia, isa.getPort());
-                        }
-                    } finally {
-                        endConnect(blocking, n > 0);
-                    }
-                } catch (IOException x) {
-                    // connect failed, close socket
-                    close();
-                    throw x;
-                }
-
-                // connection may be established
-                synchronized (stateLock) {
-                    if (!isOpen())
-                        throw new AsynchronousCloseException();
-                    remoteAddress = isa;
-                    if (n > 0) {
-                        // connected established
-                        localAddress = Net.localAddress(fd);
-                        state = ST_CONNECTED;
-                        return true;
-                    } else {
-                        // connection pending
-                        assert !blocking;
-                        state = ST_CONNECTIONPENDING;
-                        return false;
-                    }
+                    assert IOStatus.check(n);
+                    return n > 0;
+                } finally {
+                    writeLock.unlock();
                 }
             } finally {
-                writeLock.unlock();
+                readLock.unlock();
             }
-        } finally {
-            readLock.unlock();
+        } catch (IOException ioe) {
+            // connect failed, close the channel
+            close();
+            throw ioe;
         }
     }
 
@@ -714,65 +708,62 @@
      *
      * @throws AsynchronousCloseException if the channel was closed due to this
      * thread being interrupted on a blocking connect operation.
+     * @throws IOException if completed and unable to obtain the local address
      */
     private void endFinishConnect(boolean blocking, boolean completed)
-        throws AsynchronousCloseException
+        throws IOException
     {
         endRead(blocking, completed);
+
+        if (completed) {
+            synchronized (stateLock) {
+                if (state == ST_CONNECTIONPENDING) {
+                    localAddress = Net.localAddress(fd);
+                    state = ST_CONNECTED;
+                }
+            }
+        }
     }
 
     @Override
     public boolean finishConnect() throws IOException {
-        readLock.lock();
         try {
-            writeLock.lock();
+            readLock.lock();
             try {
-                // already connected?
-                synchronized (stateLock) {
-                    if (state == ST_CONNECTED)
+                writeLock.lock();
+                try {
+                    // no-op if already connected
+                    if (isConnected())
                         return true;
-                }
 
-                int n = 0;
-                boolean blocking = isBlocking();
-                try {
+                    boolean blocking = isBlocking();
+                    boolean connected = false;
                     try {
                         beginFinishConnect(blocking);
+                        int n = 0;
                         if (blocking) {
                             do {
                                 n = checkConnect(fd, true);
-                            } while (n == 0 || (n == IOStatus.INTERRUPTED) && isOpen());
+                            } while ((n == 0 || n == IOStatus.INTERRUPTED) && isOpen());
                         } else {
                             n = checkConnect(fd, false);
                         }
+                        connected = (n > 0);
                     } finally {
-                        endFinishConnect(blocking, n > 0);
+                        endFinishConnect(blocking, connected);
                     }
-                } catch (IOException x) {
-                    close();
-                    throw x;
-                }
-
-                // post finishConnect, connection may be established
-                synchronized (stateLock) {
-                    if (!isOpen())
-                        throw new AsynchronousCloseException();
-                    if (n > 0) {
-                        // connection established
-                        localAddress = Net.localAddress(fd);
-                        state = ST_CONNECTED;
-                        return true;
-                    } else {
-                        // connection still pending
-                        assert !blocking;
-                        return false;
-                    }
+                    assert (blocking && connected) ^ !blocking;
+                    return connected;
+                } finally {
+                    writeLock.unlock();
                 }
             } finally {
-                writeLock.unlock();
+                readLock.unlock();
             }
-        } finally {
-            readLock.unlock();
+        } catch (IOException ioe) {
+            // connect failed, close the channel
+            close();
+            throw ioe;
         }
     }
 
--- a/src/java.base/unix/native/libnio/ch/Net.c	Tue Mar 06 10:51:26 2018 -0800
+++ b/src/java.base/unix/native/libnio/ch/Net.c	Wed Mar 07 07:13:55 2018 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2001, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2001, 2018, 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
@@ -293,8 +293,7 @@
     int sa_len = 0;
     int rv;
 
-    if (NET_InetAddressToSockaddr(env, iao, port, &sa, &sa_len,
-                                  preferIPv6) != 0) {
+    if (NET_InetAddressToSockaddr(env, iao, port, &sa, &sa_len, preferIPv6) != 0) {
         return IOS_THROWN;
     }
 
@@ -761,11 +760,11 @@
             break;
 #endif
         case ECONNREFUSED:
+        case ETIMEDOUT:
+        case ENOTCONN:
             xn = JNU_JAVANETPKG "ConnectException";
             break;
-        case ETIMEDOUT:
-            xn = JNU_JAVANETPKG "ConnectException";
-            break;
+
         case EHOSTUNREACH:
             xn = JNU_JAVANETPKG "NoRouteToHostException";
             break;
--- a/src/java.base/unix/native/libnio/ch/SocketChannelImpl.c	Tue Mar 06 10:51:26 2018 -0800
+++ b/src/java.base/unix/native/libnio/ch/SocketChannelImpl.c	Wed Mar 07 07:13:55 2018 +0000
@@ -59,23 +59,29 @@
     poller.events = POLLOUT;
     poller.revents = 0;
     result = poll(&poller, 1, block ? -1 : 0);
+
     if (result < 0) {
-        JNU_ThrowIOExceptionWithLastError(env, "Poll failed");
-        return IOS_THROWN;
+        if (errno == EINTR) {
+            return IOS_INTERRUPTED;
+        } else {
+            JNU_ThrowIOExceptionWithLastError(env, "poll failed");
+            return IOS_THROWN;
+        }
     }
     if (!block && (result == 0))
-       return IOS_UNAVAILABLE;
+        return IOS_UNAVAILABLE;
 
-    if (poller.revents) {
+    if (result > 0) {
         errno = 0;
         result = getsockopt(fd, SOL_SOCKET, SO_ERROR, &error, &n);
         if (result < 0) {
-            handleSocketError(env, errno);
-            return JNI_FALSE;
+            return handleSocketError(env, errno);
         } else if (error) {
-            handleSocketError(env, error);
-            return JNI_FALSE;
+            return handleSocketError(env, error);
+        } else if ((poller.revents & POLLHUP) != 0) {
+            return handleSocketError(env, ENOTCONN);
         }
+        // connected
         return 1;
     }
     return 0;
--- a/src/java.base/windows/native/libnio/ch/SocketChannelImpl.c	Tue Mar 06 10:51:26 2018 -0800
+++ b/src/java.base/windows/native/libnio/ch/SocketChannelImpl.c	Wed Mar 07 07:13:55 2018 +0000
@@ -58,9 +58,7 @@
                                                jobject fdo, jboolean block)
 {
     int optError = 0;
-    int lastError = 0;
-    int result = 0;
-    int retry = 0;
+    int result;
     int n = sizeof(int);
     jint fd = fdval(env, fdo);
     fd_set wr, ex;
@@ -73,64 +71,33 @@
 
     result = select(fd+1, 0, &wr, &ex, block ? NULL : &t);
 
-    /* save last winsock error */
-    if (result == SOCKET_ERROR) {
-        lastError = WSAGetLastError();
-    }
-
-    if (block) { /* must configure socket back to blocking state */
-        u_long argp = 0;
-        int r = ioctlsocket(fd, FIONBIO, &argp);
-        if (r == SOCKET_ERROR) {
-            handleSocketError(env, WSAGetLastError());
-        }
-    }
-
     if (result == 0) {  /* timeout */
         return block ? 0 : IOS_UNAVAILABLE;
     } else {
-        if (result == SOCKET_ERROR)     { /* select failed */
-            handleSocketError(env, lastError);
+        if (result == SOCKET_ERROR) { /* select failed */
+            handleSocketError(env, WSAGetLastError());
             return IOS_THROWN;
         }
     }
 
-    /*
-     * Socket is writable or error occurred. On some Windows editions
-     * the socket will appear writable when the connect fails so we
-     * check for error rather than writable.
-     */
-    if (!FD_ISSET(fd, &ex)) {
-        return 1;               /* connection established */
+    // connection established if writable and no error to check
+    if (FD_ISSET(fd, &wr) && !FD_ISSET(fd, &ex)) {
+        return 1;
     }
 
-    /*
-     * A getsockopt( SO_ERROR ) may indicate success on NT4 even
-     * though the connection has failed. The workaround is to allow
-     * winsock to be scheduled and this is done via by yielding.
-     * As the yield approach is problematic in heavy load situations
-     * we attempt up to 3 times to get the failure reason.
-     */
-    for (retry=0; retry<3; retry++) {
-        result = getsockopt((SOCKET)fd,
-                            SOL_SOCKET,
-                            SO_ERROR,
-                            (char *)&optError,
-                            &n);
-        if (result == SOCKET_ERROR) {
-            int lastError = WSAGetLastError();
-            if (lastError == WSAEINPROGRESS) {
-                return IOS_UNAVAILABLE;
-            }
-            NET_ThrowNew(env, lastError, "getsockopt");
-            return IOS_THROWN;
+    result = getsockopt((SOCKET)fd,
+                        SOL_SOCKET,
+                        SO_ERROR,
+                        (char *)&optError,
+                        &n);
+    if (result == SOCKET_ERROR) {
+        int lastError = WSAGetLastError();
+        if (lastError == WSAEINPROGRESS) {
+            return IOS_UNAVAILABLE;
         }
-        if (optError) {
-            break;
-        }
-        Sleep(0);
+        NET_ThrowNew(env, lastError, "getsockopt");
+        return IOS_THROWN;
     }
-
     if (optError != NO_ERROR) {
         handleSocketError(env, optError);
         return IOS_THROWN;
--- a/test/jdk/java/nio/channels/AsyncCloseAndInterrupt.java	Tue Mar 06 10:51:26 2018 -0800
+++ b/test/jdk/java/nio/channels/AsyncCloseAndInterrupt.java	Wed Mar 07 07:13:55 2018 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2002, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2002, 2018, 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
@@ -708,9 +708,9 @@
 
         test(connectedSocketChannelFactory);
 
-        if (TestUtil.onWindows()) {
+        if (TestUtil.onWindows() || TestUtil.onSolaris()) {
             log.println("WARNING Cannot reliably test connect/finishConnect"
-                + " operations on Windows");
+                + " operations on this platform");
         } else {
             // Only the following tests need refuser's connection backlog
             // to be saturated
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/java/nio/channels/SocketChannel/CloseDuringConnect.java	Wed Mar 07 07:13:55 2018 +0000
@@ -0,0 +1,143 @@
+/*
+ * Copyright (c) 2018, 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.
+ */
+
+/* @test
+ * @bug 8198928
+ * @run main CloseDuringConnect
+ * @summary Attempt to cause a deadlock by closing a SocketChannel in one thread
+ *     where another thread is closing the channel after a connect fail
+ */
+
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.net.SocketAddress;
+import java.nio.channels.SocketChannel;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.stream.IntStream;
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+
+public class CloseDuringConnect {
+
+    // number of test iterations, needs to be 5-10 at least
+    static final int ITERATIONS = 50;
+
+    // maximum delay before closing SocketChannel, in milliseconds
+    static final int MAX_DELAY_BEFORE_CLOSE = 20;
+
+    /**
+     * Returns the socket address of an endpoint that refuses connections. The
+     * endpoint is an InetSocketAddress where the address is the loopback address
+     * and the port is a system port (1-1023 range).
+     */
+    static SocketAddress refusingEndpoint() {
+        InetAddress lb = InetAddress.getLoopbackAddress();
+        int port = 1;
+        while (port < 1024) {
+            SocketAddress sa = new InetSocketAddress(lb, port);
+            try {
+                SocketChannel.open(sa).close();
+            } catch (IOException ioe) {
+                return sa;
+            }
+            port++;
+        }
+        throw new RuntimeException("Unable to find system port that is refusing connections");
+    }
+
+    /**
+     * Invoked by a task in the thread pool to connect to a remote address.
+     * The connection should never be established.
+     */
+    static Void connect(SocketChannel sc, SocketAddress remote) {
+        try {
+            if (!sc.connect(remote)) {
+                while (!sc.finishConnect()) {
+                    Thread.yield();
+                }
+            }
+            throw new RuntimeException("Connected, should not happen");
+        } catch (IOException expected) { }
+        if (sc.isConnected())
+            throw new RuntimeException("isConnected return true, should not happen");
+        return null;
+    }
+
+    /**
+     * Invoked by a task in the thread pool to close a socket channel.
+     */
+    static Void close(SocketChannel sc) {
+        try {
+            sc.close();
+        } catch (IOException e) {
+            throw new UncheckedIOException("close failed", e);
+        }
+        return null;
+    }
+
+    /**
+     * Test for deadlock by submitting a task to connect to the given address
+     * while another task closes the socket channel.
+     * @param pool the thread pool to submit or schedule tasks
+     * @param remote the remote address, does not accept connections
+     * @param blocking socket channel blocking mode
+     * @param delay the delay, in millis, before closing the channel
+     */
+    static void test(ScheduledExecutorService pool,
+                     SocketAddress remote,
+                     boolean blocking,
+                     long delay) {
+        try {
+            SocketChannel sc = SocketChannel.open();
+            sc.configureBlocking(blocking);
+            Future<Void> r1 = pool.submit(() -> connect(sc, remote));
+            Future<Void> r2 = pool.schedule(() -> close(sc), delay, MILLISECONDS);
+            r1.get();
+            r2.get();
+        } catch (Throwable t) {
+            throw new RuntimeException("Test failed", t);
+        }
+    }
+
+    public static void main(String[] args) throws Exception {
+        SocketAddress refusing = refusingEndpoint();
+        ScheduledExecutorService pool = Executors.newScheduledThreadPool(2);
+        try {
+            IntStream.range(0, ITERATIONS).forEach(i -> {
+                System.out.format("Iteration %d ...%n", (i + 1));
+
+                // Execute the test for varying delays up to MAX_DELAY_BEFORE_CLOSE,
+                // for socket channels configured both blocking and non-blocking
+                IntStream.range(0, MAX_DELAY_BEFORE_CLOSE).forEach(delay -> {
+                    test(pool, refusing, /*blocking mode*/true, delay);
+                    test(pool, refusing, /*blocking mode*/false, delay);
+                });
+            });
+        } finally {
+            pool.shutdown();
+        }
+    }
+}