SniHandler reference count leak if pipeline replace fails

Motivation:
The SniHandler attempts to generate a new SslHandler from the selected SslContext in a and insert that SslHandler into the pipeline. However if the underlying channel has been closed or the pipeline has been modified the pipeline.replace(..) operation may fail. Creating the SslHandler may also create a SSLEngine which is of type ReferenceCounted. The SslHandler states that if it is not inserted into a pipeline that it will not take reference count ownership of the SSLEngine. Under these conditions we will leak the SSLEngine if it is reference counted.

Modifications:
- If the pipeline.replace(..) operation fails we should release the SSLEngine object.

Result:
Fixes https://github.com/netty/netty/issues/5678
This commit is contained in:
Scott Mitchell 2016-08-10 18:17:17 -07:00
parent 1e4f5f2cc8
commit aef16549ef
2 changed files with 16 additions and 3 deletions

View File

@ -21,6 +21,7 @@ import io.netty.channel.ChannelHandlerContext;
import io.netty.handler.codec.ByteToMessageDecoder;
import io.netty.util.CharsetUtil;
import io.netty.util.DomainNameMapping;
import io.netty.util.ReferenceCountUtil;
import io.netty.util.internal.logging.InternalLogger;
import io.netty.util.internal.logging.InternalLoggerFactory;
@ -28,6 +29,8 @@ import java.net.IDN;
import java.util.List;
import java.util.Locale;
import javax.net.ssl.SSLEngine;
/**
* <p>Enables <a href="https://tools.ietf.org/html/rfc3546#section-3.1">SNI
* (Server Name Indication)</a> extension for server side SSL. For clients
@ -245,10 +248,20 @@ public class SniHandler extends ByteToMessageDecoder {
}
private void select(ChannelHandlerContext ctx, String hostname) {
SSLEngine sslEngine = null;
SslContext selectedContext = mapping.map(hostname);
selection = new Selection(selectedContext, hostname);
SslHandler sslHandler = selectedContext.newHandler(ctx.alloc());
ctx.pipeline().replace(this, SslHandler.class.getName(), sslHandler);
try {
sslEngine = selection.context.newEngine(ctx.alloc());
ctx.pipeline().replace(this, SslHandler.class.getName(), selection.context.newHandler(sslEngine));
} catch (Throwable cause) {
selection = EMPTY_SELECTION;
// Since the SslHandler was not inserted into the pipeline the ownership of the SSLEngine was not
// transferred to the SslHandler.
// See https://github.com/netty/netty/issues/5678
ReferenceCountUtil.safeRelease(sslEngine);
ctx.fireExceptionCaught(cause);
}
}
private static final class Selection {

View File

@ -866,7 +866,7 @@ public abstract class SslContext {
return newHandler(newEngine(alloc, peerHost, peerPort));
}
private static SslHandler newHandler(SSLEngine engine) {
static SslHandler newHandler(SSLEngine engine) {
return new SslHandler(engine);
}