From 1fb6c7e1153639717a7a097327f976ef8da3a255 Mon Sep 17 00:00:00 2001
From: Hugues Bruant <hugues@aerofs.com>
Date: Fri, 3 Apr 2015 17:45:36 -0400
Subject: [PATCH] Fix overzealous assertion in SslHandler.decode

Motivation:

This AE was seen in the wild at a non-negligible rate among AeroFS
clients (JDK 8, TLS 1.2, mutual auth with RSA certs).

Upon examination of SslHandler's code a few things became apparent:

- the AE is unnecessary given the contract of decode()
- the AE was introduced between 3.8 and 3.9
- the AE is no longer present in in 4.x and master
- branches that do not have the AE skip all the bytes being fed to
  unwrap()

It is not entirely clear what sequence of SSL records can trip the
assert but it seems to happen before the handshake is completed. The
little detailed data we've been able to gather shows the assert being
triggered when

- SSLEngine.unwrap returns NEED_WRAP
- the remaining buffer is a TLS heartbeat record

Likewise, it is not entirely clear if skipping the remaining bytes is
the right thing to do or if they should be fed back to unwrap.

Modifications:

Mirror behavior in newer versions by removing the assert and skipping
bytes fed to unwrap()

Add logging in an effort to get a better understanding of this corner
case.

Result:

Avoid crashes
---
 .../jboss/netty/handler/ssl/SslHandler.java   | 29 ++++++++++++-------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/src/main/java/org/jboss/netty/handler/ssl/SslHandler.java b/src/main/java/org/jboss/netty/handler/ssl/SslHandler.java
index 05b3e81b83..39761d9ed5 100644
--- a/src/main/java/org/jboss/netty/handler/ssl/SslHandler.java
+++ b/src/main/java/org/jboss/netty/handler/ssl/SslHandler.java
@@ -913,9 +913,9 @@ public class SslHandler extends FrameDecoder
             //
             // See https://github.com/netty/netty/issues/1534
 
-            final ByteBuffer inNetBuf = in.toByteBuffer(in.readerIndex(), totalLength);
-            unwrapped = unwrap(ctx, channel, in, inNetBuf, totalLength, true);
-            assert !inNetBuf.hasRemaining() || engine.isInboundDone();
+            in.skipBytes(totalLength);
+            final ByteBuffer inNetBuf = in.toByteBuffer(startOffset, totalLength);
+            unwrapped = unwrap(ctx, channel, inNetBuf, totalLength, true);
         }
 
         if (nonSslRecord) {
@@ -1229,7 +1229,7 @@ public class SslHandler extends FrameDecoder
      */
     private void unwrapNonAppData(
             ChannelHandlerContext ctx, Channel channel, boolean mightNeedHandshake) throws SSLException {
-        unwrap(ctx, channel, ChannelBuffers.EMPTY_BUFFER, EMPTY_BUFFER, -1, mightNeedHandshake);
+        unwrap(ctx, channel, EMPTY_BUFFER, -1, mightNeedHandshake);
     }
 
     /**
@@ -1237,10 +1237,9 @@ public class SslHandler extends FrameDecoder
      */
     private ChannelBuffer unwrap(
             ChannelHandlerContext ctx, Channel channel,
-            ChannelBuffer nettyInNetBuf, ByteBuffer nioInNetBuf,
+            ByteBuffer nioInNetBuf,
             int initialNettyOutAppBufCapacity, boolean mightNeedHandshake) throws SSLException {
 
-        final int nettyInNetBufStartOffset = nettyInNetBuf.readerIndex();
         final int nioInNetBufStartOffset = nioInNetBuf.position();
         final ByteBuffer nioOutAppBuf = bufferPool.acquireBuffer();
 
@@ -1298,10 +1297,6 @@ public class SslHandler extends FrameDecoder
                         } finally {
                             outAppBuf.flip();
 
-                            // Sync the offset of the inbound buffer.
-                            nettyInNetBuf.readerIndex(
-                                    nettyInNetBufStartOffset + nioInNetBuf.position() - nioInNetBufStartOffset);
-
                             // Copy the unwrapped data into a smaller buffer.
                             if (outAppBuf.hasRemaining()) {
                                 if (nettyOutAppBuf == null) {
@@ -1349,6 +1344,20 @@ public class SslHandler extends FrameDecoder
 
                     if (result.getStatus() == Status.BUFFER_UNDERFLOW ||
                         result.bytesConsumed() == 0 && result.bytesProduced() == 0) {
+                        if (nioInNetBuf.hasRemaining() && !engine.isInboundDone()) {
+                            // We expect SSLEngine to consume all the bytes we feed it, but
+                            // empirical evidence indicates that we sometimes end up with leftovers
+                            // Log when this happens to get a better understanding of this corner
+                            // case.
+                            // See https://github.com/netty/netty/pull/3584
+                            logger.warn("Unexpected leftover data after SSLEngine.unwrap():"
+                                    + " status=" + result.getStatus()
+                                    + " handshakeStatus=" + result.getHandshakeStatus()
+                                    + " consumed=" + result.bytesConsumed()
+                                    + " produced=" + result.bytesProduced()
+                                    + " remaining=" + nioInNetBuf.remaining()
+                                    + " data=" + ChannelBuffers.hexDump(ChannelBuffers.wrappedBuffer(nioInNetBuf)));
+                        }
                         break;
                     }
                 }