Race condition between adding and removing SslHandler causing NPE

[mingyu89]

We are seeing the same issue as in ticket #6536 on version 4.1.28.
The variable pendingUnencryptedWrites in SslHandler is initialized in method "handlerAdded" if the following sequence of event happens, we will hit this NPE :

1. connect channel
2. add handler to channel
3. disconnect channel
4. call SslHandler.handlerRemoved0 from disconnecting channel. This will access pendingUnencryptedWrites and throw NPE
5. call SslHandler.handlerAdded and initialize pendingUnencryptedWrites. But it's too late.

2018-12-06 15:50:20.473 WARN [nioEventLoopGroup-3-15] io.netty.channel.DefaultChannelPipeline - An exceptionCaught() event was fired, and it reached at the tail of the pipeline. It usually means the last handler in the pipeline did not handle the exception.
io.netty.channel.ChannelPipelineException: io.netty.handler.ssl.SslHandler.handlerRemoved() has thrown an exception.
 at io.netty.channel.DefaultChannelPipeline.callHandlerRemoved0(DefaultChannelPipeline.java:676) [netty-all-4.1.28.Final.jar:4.1.28.Final]
 at io.netty.channel.DefaultChannelPipeline.destroyDown(DefaultChannelPipeline.java:922) [netty-all-4.1.28.Final.jar:4.1.28.Final]
 at io.netty.channel.DefaultChannelPipeline.destroyUp(DefaultChannelPipeline.java:888) [netty-all-4.1.28.Final.jar:4.1.28.Final]
 at io.netty.channel.DefaultChannelPipeline.destroy(DefaultChannelPipeline.java:880) [netty-all-4.1.28.Final.jar:4.1.28.Final]
 at io.netty.channel.DefaultChannelPipeline.access$700(DefaultChannelPipeline.java:46) [netty-all-4.1.28.Final.jar:4.1.28.Final]
 at io.netty.channel.DefaultChannelPipeline$HeadContext.channelUnregistered(DefaultChannelPipeline.java:1416) [netty-all-4.1.28.Final.jar:4.1.28.Final]
 at io.netty.channel.AbstractChannelHandlerContext.invokeChannelUnregistered(AbstractChannelHandlerContext.java:181) [netty-all-4.1.28.Final.jar:4.1.28.Final]
 at io.netty.channel.AbstractChannelHandlerContext.invokeChannelUnregistered(AbstractChannelHandlerContext.java:167) [netty-all-4.1.28.Final.jar:4.1.28.Final]
 at io.netty.channel.DefaultChannelPipeline.fireChannelUnregistered(DefaultChannelPipeline.java:865) [netty-all-4.1.28.Final.jar:4.1.28.Final]
 at io.netty.channel.AbstractChannel$AbstractUnsafe$8.run(AbstractChannel.java:830) [netty-all-4.1.28.Final.jar:4.1.28.Final]
 at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:163) [netty-all-4.1.28.Final.jar:4.1.28.Final]
 at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:404) [netty-all-4.1.28.Final.jar:4.1.28.Final]
 at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:464) [netty-all-4.1.28.Final.jar:4.1.28.Final]
 at io.netty.util.concurrent.SingleThreadEventExecutor$5.run(SingleThreadEventExecutor.java:884) [netty-all-4.1.28.Final.jar:4.1.28.Final]
 at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [netty-all-4.1.28.Final.jar:4.1.28.Final]
 at java.lang.Thread.run(Thread.java:748) [?:1.8.0_171]
Caused by: java.lang.NullPointerException

[normanmaurer]

@mingyu89 I wonder how this can happen... handlerRemoved0 should never be called until handlerAdded was called. Do you call these methods by yourself ? how do you add the SslHandler to the pipeline ? Can you show the code how you bootstrap it ?

[mingyu89]

@normanmaurer Thank you for looking at it.
The SslHandler isn't added to the pipeline during server bootstrap. After the connection is established, client will start to send requests to server. At some point the client may decide to start to use TLS and (stop sending any other request and) send a certain request telling the server to add SslHandler.
After receiving the request, server will :

  public void enableTLS(final SSLEngine sslEngine)
      throws GeneralSecurityException {
    if (this.attemptedToEnableTLS.compareAndSet(false, true)) {
      _logger.debug("Enabling TLS for transport {}", this.channel);
      final SslHandler handler = new SslHandler(sslEngine, true);
      this.channel.pipeline().addFirst(SSL_HANDLER_STRING, handler);
    } else {
      throw new GeneralSecurityException("TLS can not be enabled on the channel " + this.channel
          + " more than once");
    }
  }

which add the handler to pipleline.
Wouldn't it happen if there's a racing disconnect?

[normanmaurer]

It should not... is this called from outside the EventLoop ?

[normanmaurer]

@mingyu89 actually I think I know what happens here... Let me come up with a fix.

[normanmaurer]

@mingyu89 #8684 PTAL

[mingyu89]

@normanmaurer Thank you!