Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SniHandler#replaceHandler() may leak (reference counted) SslContexts #5678

Closed
rkapsi opened this issue Aug 10, 2016 · 2 comments
Closed

SniHandler#replaceHandler() may leak (reference counted) SslContexts #5678

rkapsi opened this issue Aug 10, 2016 · 2 comments
Assignees
Labels
Milestone

Comments

@rkapsi
Copy link
Member

rkapsi commented Aug 10, 2016

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?!

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() ...);
  }
}
@Scottmitch Scottmitch self-assigned this Aug 11, 2016
@Scottmitch
Copy link
Member

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

@normanmaurer
Copy link
Member

Thanks @Scottmitch and @rkapsi

@normanmaurer normanmaurer added this to the 4.0.41.Final milestone Aug 11, 2016
rkapsi pushed a commit to Squarespace/netty that referenced this issue Aug 25, 2016
The SniHandler attempts to wrap the selected SslContext in a SslHandler 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 and we will leak the SslContext if it's reference counted.

Modifications:

Add a protected replaceHandler(...) method to SniHandler that users can override to implement
their custom behavior.

Result:

Fixes netty#5678 by giving the user the ability to override replaceHandler(...) and handle errors and possibly
release the SslContext.

This change facilitates addtionally reference counting from the user's POV as they now have a way
to get their hands onto the SslContext and maybe insert an another ChannelHandler into the pipeline
(along to SslHandler) that will -- the reference count on channelInactive().

The SslHandler has also a few setter methods such as setHandshakeTimeout() that were previously
inaccessible with the SniHandler. The user can now implement their own replaceHandler() version.
Scottmitch added a commit to Scottmitch/netty that referenced this issue Aug 25, 2016
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 netty#5678
Scottmitch added a commit that referenced this issue Aug 25, 2016
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 #5678
liuzhengyang pushed a commit to liuzhengyang/netty that referenced this issue Sep 10, 2017
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 netty#5678
pulllock pushed a commit to pulllock/netty that referenced this issue Oct 19, 2023
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 netty#5678
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment