From 031c2e2e8899d037228a492a458ccd194eb8df9c Mon Sep 17 00:00:00 2001 From: Bryce Anderson <banderson@twitter.com> Date: Mon, 7 Oct 2019 00:12:54 -0600 Subject: [PATCH] Reference-counted SslEngines retain a reference to their parent SslContext (#9626) Motivation: With the Netty ref-counted OpenSSL implementation the parent SslContext maintains state necessary for the SslEngine's it produces. However, it's possible for the parent context to be closed and release those resources before the child engines are finished which causes problems. Modification: Spawned ReferenceCountedOpenSslEngine's retain a reference to their parent ReferenceCountedOpenSslContext. Result: The lifetime of the shared data is extended to include the lifetime of the dependents. --- .../ssl/ReferenceCountedOpenSslEngine.java | 7 ++++++ .../ReferenceCountedOpenSslEngineTest.java | 22 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java index d2bc88254f..ffae7306e2 100644 --- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java +++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java @@ -181,6 +181,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc boolean closed = leak.close(ReferenceCountedOpenSslEngine.this); assert closed; } + parentContext.release(); } }; @@ -208,6 +209,7 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc final ByteBufAllocator alloc; private final OpenSslEngineMap engineMap; private final OpenSslApplicationProtocolNegotiator apn; + private final ReferenceCountedOpenSslContext parentContext; private final OpenSslSession session; private final ByteBuffer[] singleSrcBuffer = new ByteBuffer[1]; private final ByteBuffer[] singleDstBuffer = new ByteBuffer[1]; @@ -366,6 +368,11 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc } } + // Now that everything looks good and we're going to successfully return the + // object so we need to retain a reference to the parent context. + parentContext = context; + parentContext.retain(); + // Only create the leak after everything else was executed and so ensure we don't produce a false-positive for // the ResourceLeakDetector. leak = leakDetection ? leakDetector.track(this) : null; diff --git a/handler/src/test/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngineTest.java index 9137dc3d98..2e7c26075a 100644 --- a/handler/src/test/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngineTest.java @@ -15,12 +15,15 @@ */ package io.netty.handler.ssl; +import io.netty.buffer.UnpooledByteBufAllocator; import io.netty.handler.ssl.util.InsecureTrustManagerFactory; import io.netty.util.ReferenceCountUtil; import org.junit.Test; import javax.net.ssl.SSLEngine; +import static junit.framework.TestCase.*; + public class ReferenceCountedOpenSslEngineTest extends OpenSslEngineTest { public ReferenceCountedOpenSslEngineTest(BufferType type, ProtocolCipherCombo combo, boolean delegate, @@ -77,4 +80,23 @@ public class ReferenceCountedOpenSslEngineTest extends OpenSslEngineTest { } return context; } + + @Test + public void parentContextIsRetainedByChildEngines() throws Exception { + SslContext clientSslCtx = SslContextBuilder.forClient() + .trustManager(InsecureTrustManagerFactory.INSTANCE) + .sslProvider(sslClientProvider()) + .protocols(protocols()) + .ciphers(ciphers()) + .build(); + + SSLEngine engine = clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT); + assertEquals(ReferenceCountUtil.refCnt(clientSslCtx), 2); + + cleanupClientSslContext(clientSslCtx); + assertEquals(ReferenceCountUtil.refCnt(clientSslCtx), 1); + + cleanupClientSslEngine(engine); + assertEquals(ReferenceCountUtil.refCnt(clientSslCtx), 0); + } }