# HG changeset patch # User alanb # Date 1552333988 0 # Node ID d70fc9bc143061fba97cf081aaada4ff6d6736e0 # Parent b38f280d211404ac5902bbed95de0a65d45cd872 Disallow mixing of platform and custom SocketImpls diff -r b38f280d2114 -r d70fc9bc1430 src/java.base/share/classes/java/net/AbstractPlainSocketImpl.java --- a/src/java.base/share/classes/java/net/AbstractPlainSocketImpl.java Sat Mar 09 12:54:20 2019 +0000 +++ b/src/java.base/share/classes/java/net/AbstractPlainSocketImpl.java Mon Mar 11 19:53:08 2019 +0000 @@ -727,42 +727,6 @@ socketClose0(false); } - @Override - public void copyTo(SocketImpl si) { - // this SocketImpl should be connected - assert fd.valid() && localport != 0 && address != null && port != 0; - - if (si instanceof AbstractPlainSocketImpl) { - AbstractPlainSocketImpl psi = (AbstractPlainSocketImpl) si; - try { - psi.close(); - } catch (IOException ignore) { } - - // copy fields - psi.stream = this.stream; - psi.fd = this.fd; - psi.localport = this.localport; - psi.address = this.address; - psi.port = this.port; - - // reset fields; do not reset timeout - psi.closePending = false; - psi.connectionReset = false; - psi.shut_rd = false; - psi.shut_wr = false; - } else { - // copy fields - si.fd = this.fd; - si.localport = this.localport; - si.address = this.address; - si.port = this.port; - } - - // this SocketImpl is now closed and should be discarded - this.closePending = true; - this.fd = null; - } - abstract void socketCreate(boolean isServer) throws IOException; abstract void socketConnect(InetAddress address, int port, int timeout) throws IOException; diff -r b38f280d2114 -r d70fc9bc1430 src/java.base/share/classes/java/net/ServerSocket.java --- a/src/java.base/share/classes/java/net/ServerSocket.java Sat Mar 09 12:54:20 2019 +0000 +++ b/src/java.base/share/classes/java/net/ServerSocket.java Mon Mar 11 19:53:08 2019 +0000 @@ -546,15 +546,7 @@ // Socket has no SocketImpl if (si == null) { - // create a platform or custom SocketImpl and accept the connection - SocketImplFactory factory = Socket.socketImplFactory(); - if (factory == null) { - si = SocketImpl.createPlatformSocketImpl(false); - } else { - si = factory.createSocketImpl(); - } - implAccept(si); - // bind Socket to the SocketImpl and update socket state + si = implAccept(); s.setImpl(si); s.postAccept(); return; @@ -566,18 +558,61 @@ assert si instanceof PlatformSocketImpl; } - // ServerSocket or Socket (or both) have, or delegate to, a platform SocketImpl - if (impl instanceof PlatformSocketImpl || si instanceof PlatformSocketImpl) { - // create a new platform SocketImpl and accept the connection - var psi = SocketImpl.createPlatformSocketImpl(false); - implAccept(psi); - // copy connection/state to the existing SocketImpl and update socket state - psi.copyTo(si); - s.postAccept(); - return; + // accept connection with a platform or custom SocketImpl + ensureCompatible(si); + if (impl instanceof PlatformSocketImpl) { + SocketImpl psi = platformImplAccept(); + tryCopyOptions(si, psi); + s.setImpl(psi); + close(si); + } else { + customImplAccept(s, si); + assert s.impl == si; } + s.postAccept(); + } - // ServerSocket and Socket bound to custom SocketImpls + /** + * Accepts a connection with a new SocketImpl. + * @return the new SocketImpl + */ + private SocketImpl implAccept() throws IOException { + if (impl instanceof PlatformSocketImpl) { + return platformImplAccept(); + } else { + // custom server SocketImpl, client socket SocketImpl must be set + SocketImplFactory factory = Socket.socketImplFactory(); + if (factory == null) { + throw new IOException("An instance of " + impl.getClass() + + " cannot accept connection with 'null' SocketImpl:" + + " client socket implementation factory not set"); + } + SocketImpl si = factory.createSocketImpl(); + implAccept(si); + return si; + } + } + + /** + * Accepts a connection with a new platform SocketImpl. + * @return the new platform SocketImpl + */ + private SocketImpl platformImplAccept() throws IOException { + assert impl instanceof PlatformSocketImpl; + + // create a new platform SocketImpl and accept the connection + SocketImpl psi = SocketImpl.createPlatformSocketImpl(false); + implAccept(psi); + return psi; + } + + /** + * Accepts a new connection with a custom SocketImpl. + */ + private void customImplAccept(Socket s, SocketImpl si) throws IOException { + assert !(impl instanceof PlatformSocketImpl) + && !(si instanceof PlatformSocketImpl); + s.impl = null; // break connection to impl si.reset(); try { @@ -588,7 +623,6 @@ } finally { s.impl = si; // restore connection to impl } - s.postAccept(); } /** @@ -601,12 +635,6 @@ private void implAccept(SocketImpl si) throws IOException { assert !(si instanceof DelegatingSocketImpl); - // A non-platform SocketImpl cannot accept a connection with a platform SocketImpl - if (!(impl instanceof PlatformSocketImpl) && (si instanceof PlatformSocketImpl)) { - throw new IOException("An instance of " + impl.getClass() + - " cannot accept a connection with an instance of " + si.getClass()); - } - // custom SocketImpl may expect fd/address objects to be created if (!(si instanceof PlatformSocketImpl)) { si.fd = new FileDescriptor(); @@ -616,15 +644,6 @@ // accept a connection impl.accept(si); - // sanity check that the fields defined by SocketImpl have been set - if (si instanceof PlatformSocketImpl) { - var fd = si.fd; - if (fd == null || !fd.valid() || si.localport <= 0 - || si.address == null || si.port <= 0) { - throw new IOException("Invalid accepted state:" + si); - } - } - // check permission, close SocketImpl/connection if denied SecurityManager sm = System.getSecurityManager(); if (sm != null) { @@ -638,6 +657,39 @@ } /** + * Attempts to copy socket options from an old/existing SocketImpl to a new + * SocketImpl. At this time, only the SO_TIMEOUT make sense to copy. + */ + private void tryCopyOptions(SocketImpl oldImpl, SocketImpl newImpl) { + try { + Object timeout = oldImpl.getOption(SocketOptions.SO_TIMEOUT); + if (timeout instanceof Integer) { + newImpl.setOption(SocketOptions.SO_TIMEOUT, timeout); + } + } catch (IOException ignore) { } + } + + /** + * Close the given SocketImpl. + */ + private void close(SocketImpl si) { + try { + si.close(); + } catch (IOException ignore) { } + } + + /** + * Throws IOException if the server SocketImpl and the given client + * SocketImpl are not both platform or custom SocketImpls. + */ + private void ensureCompatible(SocketImpl si) throws IOException { + if ((impl instanceof PlatformSocketImpl) != (si instanceof PlatformSocketImpl)) { + throw new IOException("An instance of " + impl.getClass() + + " cannot accept a connection with an instance of " + si.getClass()); + } + } + + /** * Closes this socket. * * Any thread currently blocked in {@link #accept()} will throw diff -r b38f280d2114 -r d70fc9bc1430 src/java.base/share/classes/sun/net/PlatformSocketImpl.java --- a/src/java.base/share/classes/sun/net/PlatformSocketImpl.java Sat Mar 09 12:54:20 2019 +0000 +++ b/src/java.base/share/classes/sun/net/PlatformSocketImpl.java Mon Mar 11 19:53:08 2019 +0000 @@ -24,20 +24,9 @@ */ package sun.net; -import java.net.SocketImpl; - /** * Implemented by the platform's SocketImpl implementations. */ public interface PlatformSocketImpl { - - /** - * Copy the state from this connected SocketImpl to a target SocketImpl. If - * the target SocketImpl is not a newly created SocketImpl then it is first - * closed to release any resources. The target SocketImpl becomes the owner - * of the file descriptor, this SocketImpl is marked as closed and should - * be discarded. - */ - void copyTo(SocketImpl si); } diff -r b38f280d2114 -r d70fc9bc1430 src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java --- a/src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java Sat Mar 09 12:54:20 2019 +0000 +++ b/src/java.base/share/classes/sun/nio/ch/NioSocketImpl.java Mon Mar 11 19:53:08 2019 +0000 @@ -54,7 +54,6 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.locks.ReentrantLock; -import jdk.internal.access.SharedSecrets; import jdk.internal.ref.CleanerFactory; import sun.net.ConnectionResetException; import sun.net.NetHooks; @@ -470,77 +469,6 @@ } /** - * For use by ServerSocket to copy the state from this connected SocketImpl - * to a target SocketImpl. If the target SocketImpl is not a newly created - * SocketImpl then it is first closed to release any resources. The target - * SocketImpl becomes the owner of the file descriptor, this SocketImpl - * is marked as closed and should be discarded. - */ - @Override - public void copyTo(SocketImpl si) { - if (si instanceof NioSocketImpl) { - NioSocketImpl nsi = (NioSocketImpl) si; - if (nsi.state != ST_NEW) { - try { - nsi.close(); - } catch (IOException ignore) { } - } - // copy/reset fields protected by stateLock - synchronized (nsi.stateLock) { - guarantee(nsi.state == ST_NEW || nsi.state == ST_CLOSED); - synchronized (this.stateLock) { - // this SocketImpl should be connected - guarantee(state == ST_CONNECTED && fd != null && fd.valid() - && localport > 0 && address != null && port > 0); - - // copy fields - nsi.stream = this.stream; - nsi.fd = this.fd; - nsi.localport = this.localport; - nsi.address = this.address; - nsi.port = this.port; - - // reset fields; do not reset timeout - nsi.nonBlocking = false; - nsi.isReuseAddress = false; - nsi.isInputClosed = false; - nsi.isOutputClosed = false; - nsi.state = ST_CONNECTED; - - // GC'ing of this impl should not close the file descriptor - this.closer.disable(); - this.state = ST_CLOSED; - - // create new closer to execute when nsi is GC'ed - nsi.closer = FileDescriptorCloser.create(nsi); - } - } - // reset fields protected by readLock - nsi.readLock.lock(); - try { - nsi.readEOF = false; - nsi.connectionReset = false; - } finally { - nsi.readLock.unlock(); - } - } else { - synchronized (this.stateLock) { - // this SocketImpl should be connected - guarantee(state == ST_CONNECTED && fd != null && fd.valid() - && localport > 0 && address != null && port > 0); - - // set fields in foreign impl - setSocketImplFields(si, fd, localport, address, port); - - // disable closer to prevent GC'ing of this impl from - // closing the file descriptor - this.closer.disable(); - this.state = ST_CLOSED; - } - } - } - - /** * Marks the beginning of a connect operation that might block. * @throws SocketException if the socket is closed or already connected */ @@ -1267,10 +1195,6 @@ } } } - - boolean disable() { - return CLOSED.compareAndSet(this, false, true); - } } /** @@ -1314,11 +1238,4 @@ field.setAccessible(true); field.set(si, value); } - - /** - * Throws InternalError if the given expression is not true. - */ - private static void guarantee(boolean expr) { - if (!expr) throw new InternalError(); - } } diff -r b38f280d2114 -r d70fc9bc1430 test/jdk/java/net/SocketImpl/BadSocketImpls.java --- a/test/jdk/java/net/SocketImpl/BadSocketImpls.java Sat Mar 09 12:54:20 2019 +0000 +++ /dev/null Thu Jan 01 00:00:00 1970 +0000 @@ -1,147 +0,0 @@ -/* - * Copyright (c) 2019, 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 - * @summary Combinations of bad SocketImpls - * @run testng/othervm BadSocketImpls - * @run testng/othervm -Djdk.net.usePlainSocketImpl BadSocketImpls - */ - -import java.io.IOException; -import java.io.InputStream; -import java.io.OutputStream; -import java.net.InetAddress; -import java.net.InetSocketAddress; -import java.net.ServerSocket; -import java.net.Socket; -import java.net.SocketAddress; -import java.net.SocketImpl; -import org.testng.annotations.BeforeMethod; -import org.testng.annotations.Test; -import static java.lang.String.format; -import static java.lang.System.out; -import static org.testng.Assert.fail; - -public class BadSocketImpls { - - Class IOE = IOException.class; - - /** - * Tests that a server-side custom socket impl, whose accept is a no-op, - * does not have any adverse negative effects. Default accept impl. - */ - @Test - public void testNoOpAccept() throws IOException { - ServerSocket ss = new ServerSocket(new NoOpSocketImpl()) { }; - try (ss) { - ss.bind(new InetSocketAddress(0)); - assertThrows(IOE, ss::accept); - } - } - - /** - * Tests that a server-side custom socket impl, whose accept is a no-op, - * does not have any adverse negative effects. Custom accept, client has - * an explicit null impl. - */ - @Test - public void testNoOpAcceptWithNullClientImpl() throws IOException { - ServerSocket ss = new ServerSocket(new NoOpSocketImpl()) { - @Override - public Socket accept() throws IOException { - Socket s = new Socket((SocketImpl)null) { }; - implAccept(s); - return s; - } - }; - try (ss) { - ss.bind(new InetSocketAddress(0)); - assertThrows(IOE, ss::accept); - } - } - - /** - * Tests that a server-side custom socket impl, whose accept is a no-op, - * does not have any adverse negative effects. Custom accept, client has - * a platform impl. - */ - @Test - public void testNoOpAcceptWithPlatformClientImpl() throws IOException { - ServerSocket ss = new ServerSocket(new NoOpSocketImpl()) { - @Override - public Socket accept() throws IOException { - Socket s = new Socket(); - implAccept(s); - return s; - } - }; - try (ss) { - ss.bind(new InetSocketAddress(0)); - assertThrows(IOE, ss::accept); - } - } - - static class NoOpSocketImpl extends SocketImpl { - @Override protected void create(boolean b) { } - @Override protected void connect(String s, int i) { } - @Override protected void connect(InetAddress inetAddress, int i) { } - @Override protected void connect(SocketAddress socketAddress, int i) { } - @Override protected void bind(InetAddress inetAddress, int i) { } - @Override protected void listen(int i) { } - @Override protected void accept(SocketImpl socket) { } - @Override protected InputStream getInputStream() { return null; } - @Override protected OutputStream getOutputStream() { return null; } - @Override protected int available() { return 0; } - @Override protected void close() { } - @Override protected void sendUrgentData(int i) { } - @Override public void setOption(int i, Object o) { } - @Override public Object getOption(int i) { return null; } - } - - static void assertThrows(Class throwableClass, - ThrowingRunnable runnable) { - try { - runnable.run(); - fail(format("Expected %s to be thrown, but nothing was thrown", - throwableClass.getSimpleName())); - } catch (Throwable t) { - if (!throwableClass.isInstance(t)) { - fail(format("Expected %s to be thrown, but %s was thrown", - throwableClass.getSimpleName(), - t.getClass().getSimpleName()), - t); - } - out.println("caught expected exception: " + t); - } - } - - interface ThrowingRunnable { - void run() throws Throwable; - } - - @BeforeMethod - public void breakBetweenTests() { - System.out.println("\n-------\n"); - } -} diff -r b38f280d2114 -r d70fc9bc1430 test/jdk/java/net/SocketImpl/SocketImplCombinations.java --- a/test/jdk/java/net/SocketImpl/SocketImplCombinations.java Sat Mar 09 12:54:20 2019 +0000 +++ b/test/jdk/java/net/SocketImpl/SocketImplCombinations.java Mon Mar 11 19:53:08 2019 +0000 @@ -315,7 +315,7 @@ assertTrue(isPlatformSocketImpl(getSocketImpl(ss))); assertTrue(s == socket); SocketImpl si = getSocketImpl(s); - assertTrue(si instanceof CustomSocketImpl); + assertTrue(isPlatformSocketImpl(si)); checkFields(si); }); } @@ -335,9 +335,9 @@ serverSocketAccept(socket, (ss, s) -> { assertTrue(isPlatformSocketImpl(getSocketImpl(ss))); assertTrue(s == socket); - assertTrue(getSocketImpl(s) == si); - assertTrue(getDelegate(si) == delegate); - checkFields(delegate); + SocketImpl psi = getSocketImpl(socket); + assertTrue(isPlatformSocketImpl(psi)); + checkFields(psi); }); } @@ -350,12 +350,11 @@ Socket socket = new Socket(clientImpl) { }; assertTrue(getSocketImpl(socket) == clientImpl); - serverSocketAccept(socket, (ss, s) -> { - assertTrue(isPlatformSocketImpl(getSocketImpl(ss))); - assertTrue(s == socket); - assertTrue(getSocketImpl(s) == clientImpl); - checkFields(clientImpl); - }); + try (ServerSocket ss = serverSocketToAccept(socket)) { + expectThrows(IOException.class, ss::accept); + } finally { + socket.close(); + } } public void testServerSocketAccept4b() throws IOException { @@ -363,12 +362,13 @@ Socket socket = new Socket(clientImpl) { }; assertTrue(getSocketImpl(socket) == clientImpl); - serverSocketAccept(socket, () -> new CustomSocketImpl(false), (ss, s) -> { - assertTrue(isPlatformSocketImpl(getSocketImpl(ss))); - assertTrue(s == socket); - assertTrue(getSocketImpl(s) == clientImpl); - checkFields(clientImpl); - }); + setSocketSocketImplFactory(() -> new CustomSocketImpl(false)); + try (ServerSocket ss = serverSocketToAccept(socket)) { + expectThrows(IOException.class, ss::accept); + } finally { + setSocketSocketImplFactory(null); + socket.close(); + } } /**