Rework Ssl*Stream creation. (#1109)
* Rework Ssl*Stream creation.
PR #1106 introduced a subtle bug by changing Ssl*Stream
creation to use the public get*Stream() methods.
The contract for those methods is to throw if the socket has been
closed, which mean two threads could race such that
startHandshake() called getInputStream() after the socket has
been closed by another (e.g. timeout) thread. The upshot was
that getInputStream() would throw, causing close() to be called
before an output stream is created, causing an NPE when trying to
send a TLS close message.
This change moves the actual creation into a method which doesn't
chack the socket state and so is suitable for calling from
startHandshake().
Also added some additional tests around shutdownInput() and
shutdownOutput() as it became apparent these were missing.
diff --git a/common/src/main/java/org/conscrypt/ConscryptEngineSocket.java b/common/src/main/java/org/conscrypt/ConscryptEngineSocket.java
index 0b8948d..ffa0783 100644
--- a/common/src/main/java/org/conscrypt/ConscryptEngineSocket.java
+++ b/common/src/main/java/org/conscrypt/ConscryptEngineSocket.java
@@ -197,9 +197,8 @@
state = STATE_HANDSHAKE_STARTED;
handshakeStartedMillis = Platform.getMillisSinceBoot();
engine.beginHandshake();
- // Ensure streams are created
- getInputStream();
- getOutputStream();
+ createInputStream();
+ createOutputStream();
} else {
// We've either started the handshake already or have been closed.
// Do nothing in both cases.
@@ -212,9 +211,6 @@
doHandshake();
}
- } catch (SSLException e) {
- close();
- throw e;
} catch (IOException e) {
close();
throw e;
@@ -282,7 +278,10 @@
@Override
public final InputStream getInputStream() throws IOException {
checkOpen();
+ return createInputStream();
+ }
+ private SSLInputStream createInputStream() {
synchronized (stateLock) {
if (in == null) {
in = new SSLInputStream();
@@ -294,7 +293,10 @@
@Override
public final OutputStream getOutputStream() throws IOException {
checkOpen();
+ return createOutputStream();
+ }
+ private SSLOutputStream createOutputStream() {
synchronized (stateLock) {
if (out == null) {
out = new SSLOutputStream();
diff --git a/common/src/test/java/org/conscrypt/javax/net/ssl/SSLSocketVersionCompatibilityTest.java b/common/src/test/java/org/conscrypt/javax/net/ssl/SSLSocketVersionCompatibilityTest.java
index c30fb65..5ce4d5f 100644
--- a/common/src/test/java/org/conscrypt/javax/net/ssl/SSLSocketVersionCompatibilityTest.java
+++ b/common/src/test/java/org/conscrypt/javax/net/ssl/SSLSocketVersionCompatibilityTest.java
@@ -27,6 +27,7 @@
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.junit.Assume.assumeFalse;
@@ -59,7 +60,6 @@
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
-import java.util.Locale;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
@@ -1028,6 +1028,119 @@
}
pair.close();
}
+
+ @Test
+ public void test_SSLSocket_ShutdownInput() throws Exception {
+ // Fdsocket throws SslException rather than returning EOF after input shutdown
+ // on Windows, but we won't be fixing it as that implementation is already deprecated.
+ assumeFalse("Skipping shutdownInput() test on Windows", isWindows());
+
+ final TestSSLContext c = new TestSSLContext.Builder()
+ .clientProtocol(clientVersion)
+ .serverProtocol(serverVersion)
+ .build();
+ byte[] buffer = new byte[1];
+ TestSSLSocketPair pair = TestSSLSocketPair.create(c).connect();
+ SSLSocket server = pair.server;
+ SSLSocket client = pair.client;
+ assertFalse(server.isClosed());
+ assertFalse(client.isClosed());
+ InputStream input = client.getInputStream();
+ client.shutdownInput();
+ assertFalse(client.isClosed());
+ assertFalse(server.isClosed());
+ // Shutdown after shutdown is not OK
+ SocketException exception = assertThrows(SocketException.class, client::shutdownInput);
+ assertTrue(exception.getMessage().contains("already shutdown"));
+
+ // The following operations should succeed, same as after close()
+ HandshakeCompletedListener listener = e -> { };
+ client.addHandshakeCompletedListener(listener);
+ assertNotNull(client.getEnabledCipherSuites());
+ assertNotNull(client.getEnabledProtocols());
+ client.getEnableSessionCreation();
+ client.getNeedClientAuth();
+ assertNotNull(client.getSession());
+ assertNotNull(client.getSSLParameters());
+ assertNotNull(client.getSupportedProtocols());
+ client.getUseClientMode();
+ client.getWantClientAuth();
+ client.removeHandshakeCompletedListener(listener);
+ client.setEnabledCipherSuites(new String[0]);
+ client.setEnabledProtocols(new String[0]);
+ client.setEnableSessionCreation(false);
+ client.setNeedClientAuth(false);
+ client.setSSLParameters(client.getSSLParameters());
+ client.setWantClientAuth(false);
+
+ // The following operations should succeed, unlike after close()
+ client.startHandshake();
+ client.getInputStream();
+ client.getOutputStream();
+ assertEquals(-1, input.read());
+ assertEquals(-1, input.read(buffer));
+ assertEquals(0, input.available());
+
+ pair.close();
+ }
+
+ @Test
+ public void test_SSLSocket_ShutdownOutput() throws Exception {
+ final TestSSLContext c = new TestSSLContext.Builder()
+ .clientProtocol(clientVersion)
+ .serverProtocol(serverVersion)
+ .build();
+ byte[] buffer = new byte[1];
+ TestSSLSocketPair pair = TestSSLSocketPair.create(c).connect();
+ SSLSocket server = pair.server;
+ SSLSocket client = pair.client;
+ assertFalse(server.isClosed());
+ assertFalse(client.isClosed());
+ OutputStream output = client.getOutputStream();
+ client.shutdownOutput();
+ assertFalse(client.isClosed());
+ assertFalse(server.isClosed());
+ // Shutdown after shutdown is not OK
+ SocketException exception = assertThrows(SocketException.class, client::shutdownOutput);
+ assertTrue(exception.getMessage().contains("already shutdown"));
+
+ // The following operations should succeed, same as after close()
+ HandshakeCompletedListener listener = e -> { };
+ client.addHandshakeCompletedListener(listener);
+ assertNotNull(client.getEnabledCipherSuites());
+ assertNotNull(client.getEnabledProtocols());
+ client.getEnableSessionCreation();
+ client.getNeedClientAuth();
+ assertNotNull(client.getSession());
+ assertNotNull(client.getSSLParameters());
+ assertNotNull(client.getSupportedProtocols());
+ client.getUseClientMode();
+ client.getWantClientAuth();
+ client.removeHandshakeCompletedListener(listener);
+ client.setEnabledCipherSuites(new String[0]);
+ client.setEnabledProtocols(new String[0]);
+ client.setEnableSessionCreation(false);
+ client.setNeedClientAuth(false);
+ client.setSSLParameters(client.getSSLParameters());
+ client.setWantClientAuth(false);
+
+ // The following operations should succeed, unlike after close()
+ client.startHandshake();
+ client.getInputStream();
+ client.getOutputStream();
+
+ // Any output should fail
+ try {
+ output.write(buffer);
+ fail();
+ } catch (SocketException | SSLException expected) {
+ // Expected.
+ // SocketException is correct but the old fd-based implementation
+ // throws SSLException, and it's not worth changing it at this late stage.
+ }
+ pair.close();
+ }
+
/**
* b/3350645 Test to confirm that an SSLSocket.close() performing
* an SSL_shutdown does not throw an IOException if the peer
