Skip to content

Commit

Permalink
Fix reentrancy bug in SslHandler which can cause IllegalReferenceCoun…
Browse files Browse the repository at this point in the history 
…tException (netty#11854)

Motivation:

These days our native SSL implementation makes use of "tasks" to allow off-loading operations. Due a bug this could lead to an re-entrancy issue when the Executor used is directly executing the task by the same thread that submits it. As an end-result it was possible to see IllegalReferenceCountException when the tryDecode(...) method will cause the original buffer to be released.

Modifications:

- Always submit the resume to the eventloop to ensure we never have an re-entrancy problem
- Add unit tests that would fail before the fix.

Result:

Fixes netty/netty-tcnative#680
  • Loading branch information
@normanmaurer
normanmaurer committed Nov 24, 2021
1 parent 167bd2b commit 2b4ac36
Show file tree
Hide file tree
Showing 2 changed files with 158 additions and 34 deletions.
24 changes: 13 additions & 11 deletions handler/src/main/java/io/netty/handler/ssl/SslHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1756,17 +1756,19 @@ private void resumeOnEventExecutor() {

void runComplete() {
EventExecutor executor = ctx.executor();
if (executor.inEventLoop()) {
resumeOnEventExecutor();
} else {
// Jump back on the EventExecutor.
executor.execute(new Runnable() {
@Override
public void run() {
resumeOnEventExecutor();
}
});
}
// Jump back on the EventExecutor. We do this even if we are already on the EventLoop to guard against
// reentrancy issues. Failing to do so could lead to the situation of tryDecode(...) be called and so
// channelRead(...) while still in the decode loop. In this case channelRead(...) might release the input
// buffer if its empty which would then result in an IllegalReferenceCountException when we try to continue
// decoding.
//
// See https://github.com/netty/netty-tcnative/issues/68
executor.execute(new Runnable() {
@Override
public void run() {
resumeOnEventExecutor();
}
});
}

@Override
Expand Down
168 changes: 145 additions & 23 deletions handler/src/test/java/io/netty/handler/ssl/SslHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,13 @@

public class SslHandlerTest {

private static final Executor DIRECT_EXECUTOR = new Executor() {
@Override
public void execute(Runnable command) {
command.run();
}
};

@Test
@Timeout(value = 5000, unit = TimeUnit.MILLISECONDS)
public void testNonApplicationDataFailureFailsQueuedWrites() throws NoSuchAlgorithmException, InterruptedException {
Expand Down Expand Up @@ -980,55 +987,160 @@ public void operationComplete(ChannelFuture future) throws Exception {
}

@Test
public void testHandshakeWithExecutorThatExecuteDirecty() throws Exception {
testHandshakeWithExecutor(new Executor() {
@Override
public void execute(Runnable command) {
command.run();
}
});
public void testHandshakeWithExecutorThatExecuteDirectlyJDK() throws Throwable {
testHandshakeWithExecutor(DIRECT_EXECUTOR, SslProvider.JDK, false);
}

@Test
public void testHandshakeWithImmediateExecutor() throws Exception {
testHandshakeWithExecutor(ImmediateExecutor.INSTANCE);
public void testHandshakeWithImmediateExecutorJDK() throws Throwable {
testHandshakeWithExecutor(ImmediateExecutor.INSTANCE, SslProvider.JDK, false);
}

@Test
public void testHandshakeWithImmediateEventExecutor() throws Exception {
testHandshakeWithExecutor(ImmediateEventExecutor.INSTANCE);
public void testHandshakeWithImmediateEventExecutorJDK() throws Throwable {
testHandshakeWithExecutor(ImmediateEventExecutor.INSTANCE, SslProvider.JDK, false);
}

@Test
public void testHandshakeWithExecutor() throws Exception {
public void testHandshakeWithExecutorJDK() throws Throwable {
ExecutorService executorService = Executors.newCachedThreadPool();
try {
testHandshakeWithExecutor(executorService);
testHandshakeWithExecutor(executorService, SslProvider.JDK, false);
} finally {
executorService.shutdown();
}
}

private static void testHandshakeWithExecutor(Executor executor) throws Exception {
final SslContext sslClientCtx = SslContextBuilder.forClient()
.trustManager(InsecureTrustManagerFactory.INSTANCE)
.sslProvider(SslProvider.JDK).build();
@Test
public void testHandshakeWithExecutorThatExecuteDirectlyOpenSsl() throws Throwable {
OpenSsl.ensureAvailability();
testHandshakeWithExecutor(DIRECT_EXECUTOR, SslProvider.OPENSSL, false);
}

@Test
public void testHandshakeWithImmediateExecutorOpenSsl() throws Throwable {
OpenSsl.ensureAvailability();
testHandshakeWithExecutor(ImmediateExecutor.INSTANCE, SslProvider.OPENSSL, false);
}

@Test
public void testHandshakeWithImmediateEventExecutorOpenSsl() throws Throwable {
OpenSsl.ensureAvailability();
testHandshakeWithExecutor(ImmediateEventExecutor.INSTANCE, SslProvider.OPENSSL, false);
}

@Test
public void testHandshakeWithExecutorOpenSsl() throws Throwable {
OpenSsl.ensureAvailability();
ExecutorService executorService = Executors.newCachedThreadPool();
try {
testHandshakeWithExecutor(executorService, SslProvider.OPENSSL, false);
} finally {
executorService.shutdown();
}
}

@Test
public void testHandshakeMTLSWithExecutorThatExecuteDirectlyJDK() throws Throwable {
testHandshakeWithExecutor(DIRECT_EXECUTOR, SslProvider.JDK, true);
}

@Test
public void testHandshakeMTLSWithImmediateExecutorJDK() throws Throwable {
testHandshakeWithExecutor(ImmediateExecutor.INSTANCE, SslProvider.JDK, true);
}

@Test
public void testHandshakeMTLSWithImmediateEventExecutorJDK() throws Throwable {
testHandshakeWithExecutor(ImmediateEventExecutor.INSTANCE, SslProvider.JDK, true);
}

@Test
public void testHandshakeMTLSWithExecutorJDK() throws Throwable {
ExecutorService executorService = Executors.newCachedThreadPool();
try {
testHandshakeWithExecutor(executorService, SslProvider.JDK, true);
} finally {
executorService.shutdown();
}
}

@Test
public void testHandshakeMTLSWithExecutorThatExecuteDirectlyOpenSsl() throws Throwable {
OpenSsl.ensureAvailability();
testHandshakeWithExecutor(DIRECT_EXECUTOR, SslProvider.OPENSSL, true);
}

@Test
public void testHandshakeMTLSWithImmediateExecutorOpenSsl() throws Throwable {
OpenSsl.ensureAvailability();
testHandshakeWithExecutor(ImmediateExecutor.INSTANCE, SslProvider.OPENSSL, true);
}

@Test
public void testHandshakeMTLSWithImmediateEventExecutorOpenSsl() throws Throwable {
OpenSsl.ensureAvailability();
testHandshakeWithExecutor(ImmediateEventExecutor.INSTANCE, SslProvider.OPENSSL, true);
}

@Test
public void testHandshakeMTLSWithExecutorOpenSsl() throws Throwable {
OpenSsl.ensureAvailability();
ExecutorService executorService = Executors.newCachedThreadPool();
try {
testHandshakeWithExecutor(executorService, SslProvider.OPENSSL, true);
} finally {
executorService.shutdown();
}
}

private static void testHandshakeWithExecutor(Executor executor, SslProvider provider, boolean mtls)
throws Throwable {
final SelfSignedCertificate cert = new SelfSignedCertificate();
final SslContext sslServerCtx = SslContextBuilder.forServer(cert.key(), cert.cert())
.sslProvider(SslProvider.JDK).build();
final SslContext sslClientCtx;
final SslContext sslServerCtx;
if (mtls) {
sslClientCtx = SslContextBuilder.forClient().protocols(SslProtocols.TLS_v1_2)
.trustManager(InsecureTrustManagerFactory.INSTANCE).keyManager(cert.key(), cert.cert())
.sslProvider(provider).build();

sslServerCtx = SslContextBuilder.forServer(cert.key(), cert.cert()).protocols(SslProtocols.TLS_v1_2)
.trustManager(InsecureTrustManagerFactory.INSTANCE)
.clientAuth(ClientAuth.REQUIRE)
.sslProvider(provider).build();
} else {
sslClientCtx = SslContextBuilder.forClient()
.trustManager(InsecureTrustManagerFactory.INSTANCE)
.sslProvider(provider).build();

sslServerCtx = SslContextBuilder.forServer(cert.key(), cert.cert())
.sslProvider(provider).build();
}

EventLoopGroup group = new NioEventLoopGroup();
Channel sc = null;
Channel cc = null;
final SslHandler clientSslHandler = sslClientCtx.newHandler(UnpooledByteBufAllocator.DEFAULT, executor);
final SslHandler serverSslHandler = sslServerCtx.newHandler(UnpooledByteBufAllocator.DEFAULT, executor);

final SslHandler clientSslHandler = new SslHandler(
sslClientCtx.newEngine(UnpooledByteBufAllocator.DEFAULT), executor);
final SslHandler serverSslHandler = new SslHandler(
sslServerCtx.newEngine(UnpooledByteBufAllocator.DEFAULT), executor);
final AtomicReference<Throwable> causeRef = new AtomicReference<Throwable>();
try {
sc = new ServerBootstrap()
.group(group)
.channel(NioServerSocketChannel.class)
.childHandler(serverSslHandler)
.childHandler(new ChannelInitializer<Channel>() {
@Override
protected void initChannel(Channel ch) {
ch.pipeline().addLast(serverSslHandler);
ch.pipeline().addLast(new ChannelInboundHandlerAdapter() {
@Override
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
causeRef.compareAndSet(null, cause);
}
});
}
})
.bind(new InetSocketAddress(0)).syncUninterruptibly().channel();

ChannelFuture future = new Bootstrap()
Expand All @@ -1038,12 +1150,22 @@ private static void testHandshakeWithExecutor(Executor executor) throws Exceptio
@Override
protected void initChannel(Channel ch) {
ch.pipeline().addLast(clientSslHandler);
ch.pipeline().addLast(new ChannelInboundHandlerAdapter() {
@Override
public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
causeRef.compareAndSet(null, cause);
}
});
}
}).connect(sc.localAddress());
cc = future.syncUninterruptibly().channel();

assertTrue(clientSslHandler.handshakeFuture().await().isSuccess());
assertTrue(serverSslHandler.handshakeFuture().await().isSuccess());
Throwable cause = causeRef.get();
if (cause != null) {
throw cause;
}
} finally {
if (cc != null) {
cc.close().syncUninterruptibly();
Expand Down

0 comments on commit 2b4ac36

Please sign in to comment.