From cf2e829a0e411be73307aa19977f19ffca857664 Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Wed, 20 Jan 2016 11:09:29 -0800 Subject: [PATCH] SslHandler should call beginHanshake once for the initial handshake Motivation: Not all SSLEngine implementations permit beginHandshake being called while a handshake is in progress during the initial handshake. We should ensure we only go through the initial handshake code once to prevent unexpected exceptions from being thrown. Modifications: - Only call beginHandshake if there is not currently a handshake in progress Result: SslHandler's handshake method is compatible with OpenSSLEngineImpl in Android 5.0+ and 6.0+. Fixes https://github.com/netty/netty/issues/4718 --- .../io/netty/handler/ssl/OpenSslEngine.java | 26 +++++++++---------- .../java/io/netty/handler/ssl/SslHandler.java | 4 +++ 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslEngine.java index 1190030dc6..22bb1f6a6c 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslEngine.java @@ -148,7 +148,6 @@ public final class OpenSslEngine extends SSLEngine { private HandshakeState handshakeState = HandshakeState.NOT_STARTED; private boolean receivedShutdown; - @SuppressWarnings("UnusedDeclaration") private volatile int destroyed; private volatile ClientAuth clientAuth = ClientAuth.NONE; @@ -226,13 +225,18 @@ public final class OpenSslEngine extends SSLEngine { } @Override - public SSLSession getHandshakeSession() { - if (handshakeState != HandshakeState.NOT_STARTED) { - // handshake started we are able to return the session. + public synchronized SSLSession getHandshakeSession() { + // Javadocs state return value should be: + // null if this instance is not currently handshaking, or if the current handshake has not + // progressed far enough to create a basic SSLSession. Otherwise, this method returns the + // SSLSession currently being negotiated. + switch(handshakeState) { + case NOT_STARTED: + case FINISHED: + return null; + default: return session; } - // As stated by the javadocs of getHandshakeSession() we should return null if the handshake not started yet. - return null; } /** @@ -1250,18 +1254,12 @@ public final class OpenSslEngine extends SSLEngine { @Override public synchronized SSLEngineResult.HandshakeStatus getHandshakeStatus() { // Check if we are in the initial handshake phase or shutdown phase - if (needPendingStatus()) { - return pendingStatus(SSL.pendingWrittenBytesInBIO(networkBIO)); - } - return NOT_HANDSHAKING; + return needPendingStatus() ? pendingStatus(SSL.pendingWrittenBytesInBIO(networkBIO)) : NOT_HANDSHAKING; } private SSLEngineResult.HandshakeStatus getHandshakeStatus(int pending) { // Check if we are in the initial handshake phase or shutdown phase - if (needPendingStatus()) { - return pendingStatus(pending); - } - return NOT_HANDSHAKING; + return needPendingStatus() ? pendingStatus(pending) : NOT_HANDSHAKING; } private boolean needPendingStatus() { diff --git a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java index b581749ae2..327930a688 100644 --- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java +++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java @@ -1351,6 +1351,10 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH } handshakePromise = p = newHandshakePromise; + } else if (engine.getHandshakeStatus() != HandshakeStatus.NOT_HANDSHAKING) { + // Not all SSLEngine implementations support calling beginHandshake multiple times while a handshake + // is in progress. See https://github.com/netty/netty/issues/4718. + return; } else { // Forced to reuse the old handshake. p = handshakePromise;