From a4393831f0c95e4b89812238b9ac26ac4322e451 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Wed, 30 May 2018 22:07:42 +0200 Subject: [PATCH] Fix race in SslHandlerTest that could lead to NPE. (#7989) Motivation: SslHandlerTest tried to get access to the SslHandler in the pipeline via pipeline.get(...) which may return null if the channel was already closed and so the pipeline was teared down. This showed up in a test run as: ``` ------------------------------------------------------------------------------- Test set: io.netty.handler.ssl.SslHandlerTest ------------------------------------------------------------------------------- Tests run: 17, Failures: 0, Errors: 1, Skipped: 1, Time elapsed: 0.802 sec <<< FAILURE! - in io.netty.handler.ssl.SslHandlerTest testCloseOnHandshakeFailure(io.netty.handler.ssl.SslHandlerTest) Time elapsed: 0.188 sec <<< ERROR! java.lang.NullPointerException at io.netty.handler.ssl.SslHandlerTest.testCloseOnHandshakeFailure(SslHandlerTest.java:640) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.base/java.lang.reflect.Method.invoke(Method.java:564) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298) at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292) at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) at java.base/java.lang.Thread.run(Thread.java:844) ``` Modifications: Use an AtomicReference to propagate the SslHandler instance to the outer scope. Result: No more NPE. --- .../java/io/netty/handler/ssl/SslHandlerTest.java | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java index 02cc3ee5b5..e982b6a63c 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java @@ -64,6 +64,7 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import javax.net.ssl.SSLContext; import javax.net.ssl.SSLEngine; @@ -626,17 +627,24 @@ public class SslHandlerTest { }); sc = sb.bind(address).syncUninterruptibly().channel(); + final AtomicReference sslHandlerRef = new AtomicReference(); Bootstrap b = new Bootstrap() .group(group) .channel(LocalChannel.class) .handler(new ChannelInitializer() { @Override protected void initChannel(Channel ch) { - ch.pipeline().addLast(sslClientCtx.newHandler(ch.alloc())); + SslHandler handler = sslClientCtx.newHandler(ch.alloc()); + + // We propagate the SslHandler via an AtomicReference to the outer-scope as using + // pipeline.get(...) may return null if the pipeline was teared down by the time we call it. + // This will happen if the channel was closed in the meantime. + sslHandlerRef.set(handler); + ch.pipeline().addLast(handler); } }); cc = b.connect(sc.localAddress()).syncUninterruptibly().channel(); - SslHandler handler = cc.pipeline().get(SslHandler.class); + SslHandler handler = sslHandlerRef.get(); handler.handshakeFuture().awaitUninterruptibly(); assertFalse(handler.handshakeFuture().isSuccess());