From c5d70ec7dc790d2910130a25984d4db0e56c2480 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Wed, 8 Jun 2016 15:57:47 +0200 Subject: [PATCH] [#5372] Ensure OpenSslClientContext / OpenSslServerContext can be garbage collected Motivation: OpenSslClientContext / OpenSslServerContext can never be garbage collected as both are part of a reference to a callback that is stored as global reference in jni code. Modifications: Ensure the callbacks are static and so not hold the reference. Result: No more leak due not collectable OpenSslClientContext / OpenSslServerContext --- .../handler/ssl/OpenSslClientContext.java | 54 +++++++++++++------ .../io/netty/handler/ssl/OpenSslContext.java | 10 +++- .../handler/ssl/OpenSslServerContext.java | 54 +++++++++++++------ 3 files changed, 86 insertions(+), 32 deletions(-) diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslClientContext.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslClientContext.java index 335dfb1f60..97d200dbf5 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSslClientContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslClientContext.java @@ -246,24 +246,18 @@ public final class OpenSslClientContext extends OpenSslContext { } final X509TrustManager manager = chooseTrustManager(trustManagerFactory.getTrustManagers()); + // IMPORTANT: The callbacks set for verification must be static to prevent memory leak as + // otherwise the context can never be collected. This is because the JNI code holds + // a global reference to the callbacks. + // + // See https://github.com/netty/netty/issues/5372 + // Use this to prevent an error when running on java < 7 if (useExtendedTrustManager(manager)) { - final X509ExtendedTrustManager extendedManager = (X509ExtendedTrustManager) manager; - SSLContext.setCertVerifyCallback(ctx, new AbstractCertificateVerifier() { - @Override - void verify(OpenSslEngine engine, X509Certificate[] peerCerts, String auth) - throws Exception { - extendedManager.checkServerTrusted(peerCerts, auth, engine); - } - }); + SSLContext.setCertVerifyCallback(ctx, + new ExtendedTrustManagerVerifyCallback(engineMap, (X509ExtendedTrustManager) manager)); } else { - SSLContext.setCertVerifyCallback(ctx, new AbstractCertificateVerifier() { - @Override - void verify(OpenSslEngine engine, X509Certificate[] peerCerts, String auth) - throws Exception { - manager.checkServerTrusted(peerCerts, auth); - } - }); + SSLContext.setCertVerifyCallback(ctx, new TrustManagerVerifyCallback(engineMap, manager)); } } catch (Exception e) { throw new SSLException("unable to setup trustmanager", e); @@ -323,4 +317,34 @@ public final class OpenSslClientContext extends OpenSslContext { return false; } } + + private static final class TrustManagerVerifyCallback extends AbstractCertificateVerifier { + private final X509TrustManager manager; + + TrustManagerVerifyCallback(OpenSslEngineMap engineMap, X509TrustManager manager) { + super(engineMap); + this.manager = manager; + } + + @Override + void verify(OpenSslEngine engine, X509Certificate[] peerCerts, String auth) + throws Exception { + manager.checkServerTrusted(peerCerts, auth); + } + } + + private static final class ExtendedTrustManagerVerifyCallback extends AbstractCertificateVerifier { + private final X509ExtendedTrustManager manager; + + ExtendedTrustManagerVerifyCallback(OpenSslEngineMap engineMap, X509ExtendedTrustManager manager) { + super(engineMap); + this.manager = manager; + } + + @Override + void verify(OpenSslEngine engine, X509Certificate[] peerCerts, String auth) + throws Exception { + manager.checkServerTrusted(peerCerts, auth, engine); + } + } } diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java index c8bd54244b..282bb61f98 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java @@ -77,6 +77,7 @@ public abstract class OpenSslContext extends SslContext { /** The OpenSSL SSL_CTX object */ protected volatile long ctx; + final OpenSslEngineMap engineMap = new DefaultOpenSslEngineMap(); long aprPool; @SuppressWarnings({ "unused", "FieldMayBeFinal" }) private volatile int aprPoolDestroyed; @@ -84,7 +85,6 @@ public abstract class OpenSslContext extends SslContext { private final List unmodifiableCiphers; private final long sessionCacheSize; private final long sessionTimeout; - private final OpenSslEngineMap engineMap = new DefaultOpenSslEngineMap(); private final OpenSslApplicationProtocolNegotiator apn; private final int mode; private final Certificate[] keyCertChain; @@ -452,7 +452,13 @@ public abstract class OpenSslContext extends SslContext { return PlatformDependent.javaVersion() >= 7 && trustManager instanceof X509ExtendedTrustManager; } - abstract class AbstractCertificateVerifier implements CertificateVerifier { + abstract static class AbstractCertificateVerifier implements CertificateVerifier { + private final OpenSslEngineMap engineMap; + + AbstractCertificateVerifier(OpenSslEngineMap engineMap) { + this.engineMap = engineMap; + } + @Override public final int verify(long ssl, byte[][] chain, String auth) { X509Certificate[] peerCerts = certificates(chain); diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java index f069eaa492..d5cc4e7354 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java @@ -416,24 +416,18 @@ public final class OpenSslServerContext extends OpenSslContext { final X509TrustManager manager = chooseTrustManager(trustManagerFactory.getTrustManagers()); + // IMPORTANT: The callbacks set for verification must be static to prevent memory leak as + // otherwise the context can never be collected. This is because the JNI code holds + // a global reference to the callbacks. + // + // See https://github.com/netty/netty/issues/5372 + // Use this to prevent an error when running on java < 7 if (useExtendedTrustManager(manager)) { - final X509ExtendedTrustManager extendedManager = (X509ExtendedTrustManager) manager; - SSLContext.setCertVerifyCallback(ctx, new AbstractCertificateVerifier() { - @Override - void verify(OpenSslEngine engine, X509Certificate[] peerCerts, String auth) - throws Exception { - extendedManager.checkClientTrusted(peerCerts, auth, engine); - } - }); + SSLContext.setCertVerifyCallback(ctx, + new ExtendedTrustManagerVerifyCallback(engineMap, (X509ExtendedTrustManager) manager)); } else { - SSLContext.setCertVerifyCallback(ctx, new AbstractCertificateVerifier() { - @Override - void verify(OpenSslEngine engine, X509Certificate[] peerCerts, String auth) - throws Exception { - manager.checkClientTrusted(peerCerts, auth); - } - }); + SSLContext.setCertVerifyCallback(ctx, new TrustManagerVerifyCallback(engineMap, manager)); } } catch (Exception e) { throw new SSLException("unable to setup trustmanager", e); @@ -460,4 +454,34 @@ public final class OpenSslServerContext extends OpenSslContext { "KeyManagerFactory is currently not supported with OpenSslServerContext"); } } + + private static final class TrustManagerVerifyCallback extends AbstractCertificateVerifier { + private final X509TrustManager manager; + + TrustManagerVerifyCallback(OpenSslEngineMap engineMap, X509TrustManager manager) { + super(engineMap); + this.manager = manager; + } + + @Override + void verify(OpenSslEngine engine, X509Certificate[] peerCerts, String auth) + throws Exception { + manager.checkClientTrusted(peerCerts, auth); + } + } + + private static final class ExtendedTrustManagerVerifyCallback extends AbstractCertificateVerifier { + private final X509ExtendedTrustManager manager; + + ExtendedTrustManagerVerifyCallback(OpenSslEngineMap engineMap, X509ExtendedTrustManager manager) { + super(engineMap); + this.manager = manager; + } + + @Override + void verify(OpenSslEngine engine, X509Certificate[] peerCerts, String auth) + throws Exception { + manager.checkClientTrusted(peerCerts, auth, engine); + } + } }