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 208893aac9
commit acb40a87c3
2 changed files with 16 additions and 3 deletions

View File

@ -26,6 +26,7 @@ import io.netty.util.AsyncMapping;
import io.netty.util.CharsetUtil;
import io.netty.util.DomainNameMapping;
import io.netty.util.Mapping;
import io.netty.util.ReferenceCountUtil;
import io.netty.util.concurrent.Future;
import io.netty.util.concurrent.FutureListener;
import io.netty.util.concurrent.Promise;
@ -38,6 +39,8 @@ import java.net.SocketAddress;
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
@ -303,9 +306,19 @@ public class SniHandler extends ByteToMessageDecoder implements ChannelOutboundH
}
private void replaceHandler(ChannelHandlerContext ctx, Selection selection) {
SSLEngine sslEngine = null;
this.selection = selection;
SslHandler sslHandler = selection.context.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) {
this.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);
}
}
@Override

View File

@ -869,7 +869,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);
}