ReferenceCountedOpenSslEngineTest cleanup sequencing bug

Motivation:
ReferenceCountedOpenSslEngine depends upon the the SslContext to cleanup JNI resources. If we don't wait until the ReferenceCountedOpenSslEngine is done with cleanup before cleaning up the SslContext we may crash the JVM.

Modifications:
- Wait for the channels to close (and thus the ReferenceCountedOpenSslEngine to be cleaned up) before cleaning up the associated SslContext.

Result:
Cleanup sequencing is correct and no more JVM crash.
Fixes https://github.com/netty/netty/issues/5692
This commit is contained in:
Scott Mitchell 2016-08-17 13:17:18 -07:00 committed by Norman Maurer
parent 5fd239c29c
commit 9bc3e56647
5 changed files with 36 additions and 3 deletions

View File

@ -40,6 +40,10 @@ import javax.security.auth.x500.X500Principal;
/** /**
* A client-side {@link SslContext} which uses OpenSSL's SSL/TLS implementation. * A client-side {@link SslContext} which uses OpenSSL's SSL/TLS implementation.
* <p>Instances of this class must be {@link #release() released} or else native memory will leak! * <p>Instances of this class must be {@link #release() released} or else native memory will leak!
*
* <p>Instances of this class <strong>must not</strong> be released before any {@link ReferenceCountedOpenSslEngine}
* which depends upon the instance of this class is released. Otherwise if any method of
* {@link ReferenceCountedOpenSslEngine} is called which uses this class's JNI resources the JVM may crash.
*/ */
public final class ReferenceCountedOpenSslClientContext extends ReferenceCountedOpenSslContext { public final class ReferenceCountedOpenSslClientContext extends ReferenceCountedOpenSslContext {
private static final InternalLogger logger = private static final InternalLogger logger =

View File

@ -62,6 +62,10 @@ import static io.netty.util.internal.ObjectUtil.checkNotNull;
* An implementation of {@link SslContext} which works with libraries that support the * An implementation of {@link SslContext} which works with libraries that support the
* <a href="https://www.openssl.org/">OpenSsl</a> C library API. * <a href="https://www.openssl.org/">OpenSsl</a> C library API.
* <p>Instances of this class must be {@link #release() released} or else native memory will leak! * <p>Instances of this class must be {@link #release() released} or else native memory will leak!
*
* <p>Instances of this class <strong>must not</strong> be released before any {@link ReferenceCountedOpenSslEngine}
* which depends upon the instance of this class is released. Otherwise if any method of
* {@link ReferenceCountedOpenSslEngine} is called which uses this class's JNI resources the JVM may crash.
*/ */
public abstract class ReferenceCountedOpenSslContext extends SslContext implements ReferenceCounted { public abstract class ReferenceCountedOpenSslContext extends SslContext implements ReferenceCounted {
private static final InternalLogger logger = private static final InternalLogger logger =

View File

@ -72,6 +72,10 @@ import static javax.net.ssl.SSLEngineResult.Status.OK;
* Implements a {@link SSLEngine} using * Implements a {@link SSLEngine} using
* <a href="https://www.openssl.org/docs/crypto/BIO_s_bio.html#EXAMPLE">OpenSSL BIO abstractions</a>. * <a href="https://www.openssl.org/docs/crypto/BIO_s_bio.html#EXAMPLE">OpenSSL BIO abstractions</a>.
* <p>Instances of this class must be {@link #release() released} or else native memory will leak! * <p>Instances of this class must be {@link #release() released} or else native memory will leak!
*
* <p>Instances of this class <strong>must</strong> be released before the {@link ReferenceCountedOpenSslContext}
* the instance depends upon are released. Otherwise if any method of this class is called which uses the
* the {@link ReferenceCountedOpenSslContext} JNI resources the JVM may crash.
*/ */
public class ReferenceCountedOpenSslEngine extends SSLEngine implements ReferenceCounted { public class ReferenceCountedOpenSslEngine extends SSLEngine implements ReferenceCounted {

View File

@ -35,6 +35,10 @@ import static io.netty.util.internal.ObjectUtil.checkNotNull;
/** /**
* A server-side {@link SslContext} which uses OpenSSL's SSL/TLS implementation. * A server-side {@link SslContext} which uses OpenSSL's SSL/TLS implementation.
* <p>Instances of this class must be {@link #release() released} or else native memory will leak! * <p>Instances of this class must be {@link #release() released} or else native memory will leak!
*
* <p>Instances of this class <strong>must not</strong> be released before any {@link ReferenceCountedOpenSslEngine}
* which depends upon the instance of this class is released. Otherwise if any method of
* {@link ReferenceCountedOpenSslEngine} is called which uses this class's JNI resources the JVM may crash.
*/ */
public final class ReferenceCountedOpenSslServerContext extends ReferenceCountedOpenSslContext { public final class ReferenceCountedOpenSslServerContext extends ReferenceCountedOpenSslContext {
private static final byte[] ID = {'n', 'e', 't', 't', 'y'}; private static final byte[] ID = {'n', 'e', 't', 't', 'y'};

View File

@ -118,18 +118,35 @@ public abstract class SSLEngineTest {
@After @After
public void tearDown() throws InterruptedException { public void tearDown() throws InterruptedException {
ChannelFuture clientCloseFuture = null;
ChannelFuture serverConnectedCloseFuture = null;
ChannelFuture serverCloseFuture = null;
if (clientChannel != null) { if (clientChannel != null) {
clientChannel.close(); clientCloseFuture = clientChannel.close();
clientChannel = null; clientChannel = null;
} }
if (serverConnectedChannel != null) { if (serverConnectedChannel != null) {
serverConnectedChannel.close(); serverConnectedCloseFuture = serverConnectedChannel.close();
serverConnectedChannel = null; serverConnectedChannel = null;
} }
if (serverChannel != null) { if (serverChannel != null) {
serverChannel.close().sync(); serverCloseFuture = serverChannel.close();
serverChannel = null; serverChannel = null;
} }
// We must wait for the Channel cleanup to finish. In the case if the ReferenceCountedOpenSslEngineTest
// the ReferenceCountedOpenSslEngine depends upon the SslContext and so we must wait the cleanup the
// SslContext to avoid JVM core dumps!
//
// See https://github.com/netty/netty/issues/5692
if (clientCloseFuture != null) {
clientCloseFuture.sync();
}
if (serverConnectedCloseFuture != null) {
serverConnectedCloseFuture.sync();
}
if (serverCloseFuture != null) {
serverCloseFuture.sync();
}
if (serverSslCtx != null) { if (serverSslCtx != null) {
cleanupSslContext(serverSslCtx); cleanupSslContext(serverSslCtx);
serverSslCtx = null; serverSslCtx = null;