Repository: netty/netty
Issue: SniHandler#replaceHandler() may leak (reference counted) SslContexts
Labels: defect

The SniHandler#select()/AsyncMapper#map() methods execute asynchronously and by the time the returned Future completes the client is possibly no longer connected.

The #selection field will be set but probably after #channelInactive() (i.e. can't really do anything with it) and the #replace() call on the ChannelPipeline will probably throw a NoSuchElementException.

I have no good suggestion how to fix it. There is an opportunity to cancel the future and it could try to decrement the refCnt if the Future completes successfully but the client has disconnected in the meantime. 

Given SniHandler is a one-shot handler maybe something along the lines of this?!

``` java
class SniHandler {
  private Promsie<SslContext> promise;

  void handlerAdded(ChannelHandlerContext ctx) {
    promsie = ctx.newPromise();
  }

  void channelInactive(ChannelHandlerContext ctx) {
    promsie.cancel(true);
    promsie.addFutureListener(... release() on success ...);
    ctx.fireChannelInactive();
  }

  void select(...) {
    mapping.map(hostname, promsie).addFutureListener(... replaceHandler() ...);
  }
}
```


Issue comments:
thanks for reporting @rkapsi ! let me come up with a PR.

Thanks @Scottmitch and @rkapsi 


Pull request review comments:
Maybe even fire a custom `SniHandlerException(hostname, context, cause)` to give the user every possible opportunity not to lose track of the SslContext.

![MAJOR](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/severity-major.png) Remove this unused method parameter "hostname". [![rule](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/rule.png)](https://garage.netty.io/sonarqube/coding_rules#rule_key=squid%3AS1172)

Firing exceptions with reference counted objects can create confusion in my experience. IMO lets avoid this and push folks toward `replaceHandler`.


Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/ssl/SniHandler.java b/handler/src/main/java/io/netty/handler/ssl/SniHandler.java
index 7fc70d1258a..6b404d3763c 100644
--- a/handler/src/main/java/io/netty/handler/ssl/SniHandler.java
+++ b/handler/src/main/java/io/netty/handler/ssl/SniHandler.java
@@ -303,8 +303,24 @@ public void operationComplete(Future<SslContext> future) throws Exception {
     }
 
     private void replaceHandler(ChannelHandlerContext ctx, Selection selection) {
-        this.selection = selection;
-        SslHandler sslHandler = selection.context.newHandler(ctx.alloc());
+        try {
+            this.selection = selection;
+            replaceHandler(ctx, selection.hostname, selection.context);
+        } catch (Throwable cause) {
+            this.selection = EMPTY_SELECTION;
+            ctx.fireExceptionCaught(cause);
+        }
+    }
+
+    /**
+     * Replaces the {@link SniHandler} with a {@link SslHandler} as provided by the
+     * given {@link SslContext}.
+     *
+     * Users may override this method to implement custom behavior and to get a
+     * hold onto the {@link SslContext}.
+     */
+    protected void replaceHandler(ChannelHandlerContext ctx, String hostname, SslContext context) throws Exception {
+        SslHandler sslHandler = context.newHandler(ctx.alloc());
         ctx.pipeline().replace(this, SslHandler.class.getName(), sslHandler);
     }
 


Test patch: