From 3f7a7949dbeb8d046e19ed8a1cd0fd78cb1b1fb2 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 639713b8b4..87dac04d22 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];
@@ -362,6 +364,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);
+    }
 }