Correctly fix problem in ByteToMessageDecoder and ReplayingDecoder which could let to have a released buffer passed to the decode methods.

This fixes #1664 and revert also the original commit which was meant to fix it 3b1881b523 . The problem with the original commit was that it could delay handlerRemove(..) calls and so mess up the order or forward bytes to late.

codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java

@@ -229,6 +229,15 @@ public abstract class ByteToMessageDecoder extends ChannelInboundHandlerAdapter
                 int outSize = out.size();
                 int oldInputLength = in.readableBytes();
                 decode(ctx, in, out);
+
+                // Check if this handler was removed before try to continue the loop.
+                // If it was removed it is not safe to continue to operate on the buffer
+                //
+                // See https://github.com/netty/netty/issues/1664
+                if (ctx.isRemoved()) {
+                    break;
+                }
+
                 if (outSize == out.size()) {
                     if (oldInputLength == in.readableBytes()) {
                         break;

codec/src/main/java/io/netty/handler/codec/ReplayingDecoder.java

@@ -358,6 +358,15 @@ public abstract class ReplayingDecoder<S> extends ByteToMessageDecoder {
                 int oldInputLength = in.readableBytes();
                 try {
                     decode(ctx, replayable, out);
+
+                    // Check if this handler was removed before try to continue the loop.
+                    // If it was removed it is not safe to continue to operate on the buffer
+                    //
+                    // See https://github.com/netty/netty/issues/1664
+                    if (ctx.isRemoved()) {
+                        break;
+                    }
+
                     if (outSize == out.size()) {
                         if (oldInputLength == in.readableBytes() && oldState == state) {
                             throw new DecoderException(
@@ -371,6 +380,15 @@ public abstract class ReplayingDecoder<S> extends ByteToMessageDecoder {
                     }
                 } catch (Signal replay) {
                     replay.expect(REPLAY);
+
+                    // Check if this handler was removed before try to continue the loop.
+                    // If it was removed it is not safe to continue to operate on the buffer
+                    //
+                    // See https://github.com/netty/netty/issues/1664
+                    if (ctx.isRemoved()) {
+                        break;
+                    }
+
                     // Return to the checkpoint (or oldPosition) and retry.
                     int checkpoint = this.checkpoint;
                     if (checkpoint >= 0) {

transport/src/main/java/io/netty/channel/DefaultChannelHandlerContext.java

@@ -34,7 +34,6 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
     private final DefaultChannelPipeline pipeline;
     private final String name;
     private final ChannelHandler handler;
-    private int callDepth;
     private boolean removed;
 
     // Will be set to null if no child executor should be used, otherwise it will be set to the
@@ -78,10 +77,6 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
         }
     }
 
-    boolean isRunning() {
-        return callDepth != 0;
-    }
-
     /** Invocation initiated by {@link DefaultChannelPipeline#teardownAll()}}. */
     void teardown() {
         EventExecutor executor = executor();
@@ -160,12 +155,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeChannelRegistered() {
         try {
-            callDepth ++;
+            ((ChannelInboundHandler) handler).channelRegistered(this);
-            try {
-                ((ChannelInboundHandler) handler).channelRegistered(this);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             notifyHandlerException(t);
         }
@@ -190,12 +180,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeChannelUnregistered() {
         try {
-            callDepth ++;
+            ((ChannelInboundHandler) handler).channelUnregistered(this);
-            try {
-                ((ChannelInboundHandler) handler).channelUnregistered(this);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             notifyHandlerException(t);
         }
@@ -220,12 +205,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeChannelActive() {
         try {
-            callDepth ++;
+            ((ChannelInboundHandler) handler).channelActive(this);
-            try {
-                ((ChannelInboundHandler) handler).channelActive(this);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             notifyHandlerException(t);
         }
@@ -250,12 +230,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeChannelInactive() {
         try {
-            callDepth ++;
+            ((ChannelInboundHandler) handler).channelInactive(this);
-            try {
-                ((ChannelInboundHandler) handler).channelInactive(this);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             notifyHandlerException(t);
         }
@@ -293,12 +268,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeExceptionCaught(final Throwable cause) {
         try {
-            callDepth ++;
+            handler.exceptionCaught(this, cause);
-            try {
-                handler.exceptionCaught(this, cause);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             if (logger.isWarnEnabled()) {
                 logger.warn(
@@ -331,12 +301,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeUserEventTriggered(Object event) {
         try {
-            callDepth ++;
+            ((ChannelInboundHandler) handler).userEventTriggered(this, event);
-            try {
-                ((ChannelInboundHandler) handler).userEventTriggered(this, event);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             notifyHandlerException(t);
         }
@@ -365,12 +330,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeChannelRead(Object msg) {
         try {
-            callDepth ++;
+            ((ChannelInboundHandler) handler).channelRead(this, msg);
-            try {
-                ((ChannelInboundHandler) handler).channelRead(this, msg);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             notifyHandlerException(t);
         }
@@ -399,12 +359,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeChannelReadComplete() {
         try {
-            callDepth ++;
+            ((ChannelInboundHandler) handler).channelReadComplete(this);
-            try {
-                ((ChannelInboundHandler) handler).channelReadComplete(this);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             notifyHandlerException(t);
         }
@@ -433,12 +388,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeChannelWritabilityChanged() {
         try {
-            callDepth ++;
+            ((ChannelInboundHandler) handler).channelWritabilityChanged(this);
-            try {
-                ((ChannelInboundHandler) handler).channelWritabilityChanged(this);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             notifyHandlerException(t);
         }
@@ -499,12 +449,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeBind(SocketAddress localAddress, ChannelPromise promise) {
         try {
-            callDepth ++;
+            ((ChannelOutboundHandler) handler).bind(this, localAddress, promise);
-            try {
-                ((ChannelOutboundHandler) handler).bind(this, localAddress, promise);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             notifyOutboundHandlerException(t, promise);
         }
@@ -542,12 +487,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeConnect(SocketAddress remoteAddress, SocketAddress localAddress, ChannelPromise promise) {
         try {
-            callDepth ++;
+            ((ChannelOutboundHandler) handler).connect(this, remoteAddress, localAddress, promise);
-            try {
-                ((ChannelOutboundHandler) handler).connect(this, remoteAddress, localAddress, promise);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             notifyOutboundHandlerException(t, promise);
         }
@@ -582,12 +522,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeDisconnect(ChannelPromise promise) {
         try {
-            callDepth ++;
+            ((ChannelOutboundHandler) handler).disconnect(this, promise);
-            try {
-                ((ChannelOutboundHandler) handler).disconnect(this, promise);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             notifyOutboundHandlerException(t, promise);
         }
@@ -615,12 +550,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeClose(ChannelPromise promise) {
         try {
-            callDepth ++;
+            ((ChannelOutboundHandler) handler).close(this, promise);
-            try {
-                ((ChannelOutboundHandler) handler).close(this, promise);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             notifyOutboundHandlerException(t, promise);
         }
@@ -648,12 +578,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeDeregister(ChannelPromise promise) {
         try {
-            callDepth ++;
+            ((ChannelOutboundHandler) handler).deregister(this, promise);
-            try {
-                ((ChannelOutboundHandler) handler).deregister(this, promise);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             notifyOutboundHandlerException(t, promise);
         }
@@ -683,12 +608,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeRead() {
         try {
-            callDepth ++;
+            ((ChannelOutboundHandler) handler).read(this);
-            try {
-                ((ChannelOutboundHandler) handler).read(this);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             notifyHandlerException(t);
         }
@@ -724,12 +644,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeWrite(Object msg, ChannelPromise promise) {
         try {
-            callDepth ++;
+            ((ChannelOutboundHandler) handler).write(this, msg, promise);
-            try {
-                ((ChannelOutboundHandler) handler).write(this, msg, promise);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             notifyOutboundHandlerException(t, promise);
         }
@@ -759,12 +674,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements
 
     private void invokeFlush() {
         try {
-            callDepth ++;
+            ((ChannelOutboundHandler) handler).flush(this);
-            try {
-                ((ChannelOutboundHandler) handler).flush(this);
-            } finally {
-                callDepth --;
-            }
         } catch (Throwable t) {
             notifyHandlerException(t);
         }

transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java

@@ -506,16 +506,14 @@ final class DefaultChannelPipeline implements ChannelPipeline {
     }
 
     private void callHandlerRemoved(final DefaultChannelHandlerContext ctx) {
-        if (ctx.channel().isRegistered()) {
+        if (ctx.channel().isRegistered() && !ctx.executor().inEventLoop()) {
-            if (!ctx.executor().inEventLoop() || ctx.isRunning()) {
+            ctx.executor().execute(new Runnable() {
-                ctx.executor().execute(new Runnable() {
                 @Override
                 public void run() {
                     callHandlerRemoved0(ctx);
-                }
+                 }
             });
-                return;
+            return;
-            }
         }
         callHandlerRemoved0(ctx);
     }