8278326: Socket close is not thread safe and other cleanup

Reviewed-by: jpai

src/java.base/share/classes/java/net/HttpConnectSocketImpl.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2023, 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
@@ -124,8 +124,8 @@ protected void connect(SocketAddress endpoint, int timeout)
         // close the original socket impl and release its descriptor
         close();
 
-        // update the Sockets impl to the impl from the http Socket
-        SocketImpl si = httpSocket.impl;
+        // change Socket to use httpSocket's SocketImpl
+        SocketImpl si = httpSocket.impl();
         socket.setImpl(si);
 
         // best effort is made to try and reset options previously set

src/java.base/share/classes/java/net/ServerSocket.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1995, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1995, 2023, 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
@@ -78,18 +78,23 @@
  * @since   1.0
  */
 public class ServerSocket implements java.io.Closeable {
-    /**
-     * The underlying SocketImpl
-     */
+    // the underlying SocketImpl
     private final SocketImpl impl;
 
-    /**
-     * Various states of this socket, need stateLock to change.
-     */
+    // various states
     private volatile boolean created;   // impl.create(boolean) called
     private volatile boolean bound;
     private volatile boolean closed;
-    private final Object stateLock = new Object();
+
+    // used to coordinate creating and closing underlying socket
+    private final Object socketLock = new Object();
+
+    /**
+     * Creates a server socket with the given {@code SocketImpl}.
+     */
+    private ServerSocket(Void unused, SocketImpl impl) {
+        this.impl = Objects.requireNonNull(impl);
+    }
 
     /**
      * Creates a server socket with a user-specified {@code SocketImpl}.
@@ -104,9 +109,7 @@ public class ServerSocket implements java.io.Closeable {
      * @since 12
      */
     protected ServerSocket(SocketImpl impl) {
-        Objects.requireNonNull(impl);
-        checkPermission();
-        this.impl = impl;
+        this(checkPermission(), impl);
     }
 
     private static Void checkPermission() {
@@ -280,16 +283,26 @@ public ServerSocket(int port, int backlog, InetAddress bindAddr) throws IOExcept
     }
 
     /**
-     * Get the {@code SocketImpl} attached to this socket, creating
-     * it if necessary.
-     *
-     * @return  the {@code SocketImpl} attached to that ServerSocket.
-     * @throws SocketException if creation fails.
-     * @since 1.4
+     * Create a SocketImpl for a server socket. The SocketImpl is created
+     * without an underlying socket.
+     */
+    private static SocketImpl createImpl() {
+        SocketImplFactory factory = ServerSocket.factory;
+        if (factory != null) {
+            return factory.createSocketImpl();
+        } else {
+            return SocketImpl.createPlatformSocketImpl(true);
+        }
+    }
+
+    /**
+     * Returns the {@code SocketImpl} for this ServerSocket, creating the
+     * underlying socket if required.
+     * @throws SocketException if creating the underlying socket fails
      */
     private SocketImpl getImpl() throws SocketException {
         if (!created) {
-            synchronized (stateLock) {
+            synchronized (socketLock) {
                 if (!created) {
                     if (closed) {
                         throw new SocketException("Socket is closed");
@@ -308,18 +321,6 @@ private SocketImpl getImpl() throws SocketException {
         return impl;
     }
 
-    /**
-     * Create a SocketImpl for a server socket.
-     */
-    private static SocketImpl createImpl() {
-        SocketImplFactory factory = ServerSocket.factory;
-        if (factory != null) {
-            return factory.createSocketImpl();
-        } else {
-            return SocketImpl.createPlatformSocketImpl(true);
-        }
-    }
-
     /**
      *
      * Binds the {@code ServerSocket} to a specific address
@@ -385,15 +386,11 @@ public void bind(SocketAddress endpoint, int backlog) throws IOException {
         if (security != null)
             security.checkListen(epoint.getPort());
 
-        synchronized (stateLock) {
-            if (closed)
-                throw new SocketException("Socket is closed");
-            if (bound)
-                throw new SocketException("Already bound");
-            getImpl().bind(epoint.getAddress(), epoint.getPort());
-            getImpl().listen(backlog);
-            bound = true;
-        }
+        // SocketImpl bind+listen throw if already bound or closed
+        SocketImpl impl = getImpl();
+        impl.bind(epoint.getAddress(), epoint.getPort());
+        impl.listen(backlog);
+        bound = true;
     }
 
     /**
@@ -581,19 +578,24 @@ public Socket accept() throws IOException {
      * @revised 1.4
      */
     protected final void implAccept(Socket s) throws IOException {
-        SocketImpl si = s.impl;
+        SocketImpl si = s.impl();
 
         // Socket has no SocketImpl
         if (si == null) {
             si = implAccept();
-            s.setImpl(si);
-            s.postAccept();
+            try {
+                s.setConnectedImpl(si);
+            } catch (SocketException e) {
+                // s has been closed so newly accepted connection needs to be closed
+                si.closeQuietly();
+                throw e;
+            }
             return;
         }
 
         // Socket has a SOCKS or HTTP SocketImpl, need delegate
-        if (si instanceof DelegatingSocketImpl) {
-            si = ((DelegatingSocketImpl) si).delegate();
+        if (si instanceof DelegatingSocketImpl dsi) {
+            si = dsi.delegate();
             assert si instanceof PlatformSocketImpl;
         }
 
@@ -609,17 +611,23 @@ protected final void implAccept(Socket s) throws IOException {
         if (impl instanceof PlatformSocketImpl) {
             SocketImpl psi = platformImplAccept();
             si.copyOptionsTo(psi);
-            s.setImpl(psi);
-            si.closeQuietly();
+            try {
+                s.setConnectedImpl(psi);
+            } catch (SocketException e) {
+                // s has been closed so newly accepted connection needs to be closed
+                psi.closeQuietly();
+                throw e;
+            }
         } else {
-            s.impl = null; // temporarily break connection to impl
+            s.setImpl(null);    // temporarily break connection to impl
             try {
                 customImplAccept(si);
             } finally {
-                s.impl = si;  // restore connection to impl
+                s.setImpl(si);  // restore connection to impl
             }
+            s.setConnected();
         }
-        s.postAccept();
+
     }
 
     /**
@@ -736,7 +744,7 @@ private void ensureCompatible(SocketImpl si) throws IOException {
      * @revised 1.4
      */
     public void close() throws IOException {
-        synchronized (stateLock) {
+        synchronized (socketLock) {
             if (!closed) {
                 closed = true;
 
@@ -830,8 +838,8 @@ public int getSoTimeout() throws IOException {
             throw new SocketException("Socket is closed");
         Object o = getImpl().getOption(SocketOptions.SO_TIMEOUT);
         /* extra type safety */
-        if (o instanceof Integer) {
-            return ((Integer) o).intValue();
+        if (o instanceof Integer i) {
+            return i.intValue();
         } else {
             return 0;
         }
@@ -1010,10 +1018,9 @@ public static synchronized void setSocketFactory(SocketImplFactory fac) throws I
      * @since 1.4
      * @see #getReceiveBufferSize
      */
-     public void setReceiveBufferSize (int size) throws SocketException {
-        if (!(size > 0)) {
+    public void setReceiveBufferSize(int size) throws SocketException {
+        if (size <= 0)
             throw new IllegalArgumentException("negative receive size");
-        }
         if (isClosed())
             throw new SocketException("Socket is closed");
         getImpl().setOption(SocketOptions.SO_RCVBUF, size);