Correctly resume wrap / unwrap when SslTask execution completes (#8899)

Motivation:

fa6a8cb09c introduced correct dispatching of delegated tasks for SSLEngine but did not correctly handle some cases for resuming wrap / unwrap after the task was executed. This could lead to stales, which showed up during tests when running with Java11 and BoringSSL.

Modifications:

- Correctly resume wrap / unwrap in all cases.
- Fix timeout value which was changed in previous commit by mistake.

Result:

No more stales after task execution.
This commit is contained in:
Norman Maurer 2019-02-28 20:29:40 +01:00
parent 0d37b06bc8
commit 89139aa3f8
2 changed files with 22 additions and 7 deletions

View File

@ -948,7 +948,8 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH
out = null; out = null;
} }
switch (result.getHandshakeStatus()) { HandshakeStatus status = result.getHandshakeStatus();
switch (status) {
case FINISHED: case FINISHED:
setHandshakeSuccess(); setHandshakeSuccess();
return false; return false;
@ -983,7 +984,7 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH
throw new IllegalStateException("Unknown handshake status: " + result.getHandshakeStatus()); throw new IllegalStateException("Unknown handshake status: " + result.getHandshakeStatus());
} }
if (result.bytesProduced() == 0) { if (result.bytesProduced() == 0 && status != HandshakeStatus.NEED_TASK) {
break; break;
} }
@ -1432,7 +1433,9 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH
throw new IllegalStateException("unknown handshake status: " + handshakeStatus); throw new IllegalStateException("unknown handshake status: " + handshakeStatus);
} }
if (status == Status.BUFFER_UNDERFLOW || consumed == 0 && produced == 0) { if (status == Status.BUFFER_UNDERFLOW ||
// If we processed NEED_TASK we should try again even we did not consume or produce anything.
handshakeStatus != HandshakeStatus.NEED_TASK && consumed == 0 && produced == 0) {
if (handshakeStatus == HandshakeStatus.NEED_UNWRAP) { if (handshakeStatus == HandshakeStatus.NEED_UNWRAP) {
// The underlying engine is starving so we need to feed it with more data. // The underlying engine is starving so we need to feed it with more data.
// See https://github.com/netty/netty/pull/5039 // See https://github.com/netty/netty/pull/5039
@ -1618,6 +1621,11 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH
taskError(e); taskError(e);
return; return;
} }
if (inUnwrap) {
// If we were in the unwrap call when the task was processed we should also try to unwrap
// non app data first as there may not anything left in the inbound buffer to process.
unwrapNonAppData(ctx);
}
// Flush now as we may have written some data as part of the wrap call. // Flush now as we may have written some data as part of the wrap call.
forceFlush(ctx); forceFlush(ctx);
@ -1636,13 +1644,20 @@ public class SslHandler extends ByteToMessageDecoder implements ChannelOutboundH
// that will be written to the Channel. // that will be written to the Channel.
case NEED_WRAP: case NEED_WRAP:
try { try {
wrapNonAppData(ctx, inUnwrap); if (!wrapNonAppData(ctx, false) && inUnwrap) {
// The handshake finished in wrapNonAppData(...), we need to try call
// unwrapNonAppData(...) as we may have some alert that we should read.
//
// This mimics what we would do when we are calling this method while in unwrap(...).
unwrapNonAppData(ctx);
}
// Flush now as we may have written some data as part of the wrap call.
forceFlush(ctx);
} catch (Throwable e) { } catch (Throwable e) {
taskError(e); taskError(e);
return; return;
} }
// Flush now as we may have written some data as part of the wrap call.
forceFlush(ctx);
// Now try to feed in more data that we have buffered. // Now try to feed in more data that we have buffered.
tryDecodeAgain(); tryDecodeAgain();

View File

@ -1163,7 +1163,7 @@ public abstract class SSLEngineTest {
MessageReceiver receiver) throws Exception { MessageReceiver receiver) throws Exception {
List<ByteBuf> dataCapture = null; List<ByteBuf> dataCapture = null;
try { try {
assertTrue(sendChannel.writeAndFlush(message).await(50, TimeUnit.SECONDS)); assertTrue(sendChannel.writeAndFlush(message).await(5, TimeUnit.SECONDS));
receiverLatch.await(5, TimeUnit.SECONDS); receiverLatch.await(5, TimeUnit.SECONDS);
message.readerIndex(0); message.readerIndex(0);
ArgumentCaptor<ByteBuf> captor = ArgumentCaptor.forClass(ByteBuf.class); ArgumentCaptor<ByteBuf> captor = ArgumentCaptor.forClass(ByteBuf.class);