From a06295fe0ad72358f980d09feb8c971613793527 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 1 Aug 2013 09:54:07 +0200 Subject: [PATCH] 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 3b1881b523e61496d9bc50e44927f7b58fa449e6 . The problem with the original commit was that it could delay handlerRemove(..) calls and so mess up the order or forward bytes to late. --- .../handler/codec/ByteToMessageDecoder.java | 9 ++ .../netty/handler/codec/ReplayingDecoder.java | 18 +++ .../channel/DefaultChannelHandlerContext.java | 124 +++--------------- .../netty/channel/DefaultChannelPipeline.java | 10 +- 4 files changed, 48 insertions(+), 113 deletions(-) diff --git a/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java b/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java index e6169783bc..e1f07ac713 100644 --- a/codec/src/main/java/io/netty/handler/codec/ByteToMessageDecoder.java +++ b/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; diff --git a/codec/src/main/java/io/netty/handler/codec/ReplayingDecoder.java b/codec/src/main/java/io/netty/handler/codec/ReplayingDecoder.java index 639a58aecd..48f74dff75 100644 --- a/codec/src/main/java/io/netty/handler/codec/ReplayingDecoder.java +++ b/codec/src/main/java/io/netty/handler/codec/ReplayingDecoder.java @@ -358,6 +358,15 @@ public abstract class ReplayingDecoder 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 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) { diff --git a/transport/src/main/java/io/netty/channel/DefaultChannelHandlerContext.java b/transport/src/main/java/io/netty/channel/DefaultChannelHandlerContext.java index 3140b113a4..4d95f2dfef 100644 --- a/transport/src/main/java/io/netty/channel/DefaultChannelHandlerContext.java +++ b/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 ++; - try { - ((ChannelInboundHandler) handler).channelRegistered(this); - } finally { - callDepth --; - } + ((ChannelInboundHandler) handler).channelRegistered(this); } catch (Throwable t) { notifyHandlerException(t); } @@ -190,12 +180,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements private void invokeChannelUnregistered() { try { - callDepth ++; - try { - ((ChannelInboundHandler) handler).channelUnregistered(this); - } finally { - callDepth --; - } + ((ChannelInboundHandler) handler).channelUnregistered(this); } catch (Throwable t) { notifyHandlerException(t); } @@ -220,12 +205,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements private void invokeChannelActive() { try { - callDepth ++; - try { - ((ChannelInboundHandler) handler).channelActive(this); - } finally { - callDepth --; - } + ((ChannelInboundHandler) handler).channelActive(this); } catch (Throwable t) { notifyHandlerException(t); } @@ -250,12 +230,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements private void invokeChannelInactive() { try { - callDepth ++; - try { - ((ChannelInboundHandler) handler).channelInactive(this); - } finally { - callDepth --; - } + ((ChannelInboundHandler) handler).channelInactive(this); } catch (Throwable t) { notifyHandlerException(t); } @@ -293,12 +268,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements private void invokeExceptionCaught(final Throwable cause) { try { - callDepth ++; - try { - handler.exceptionCaught(this, cause); - } finally { - callDepth --; - } + handler.exceptionCaught(this, cause); } 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 ++; - try { - ((ChannelInboundHandler) handler).userEventTriggered(this, event); - } finally { - callDepth --; - } + ((ChannelInboundHandler) handler).userEventTriggered(this, event); } catch (Throwable t) { notifyHandlerException(t); } @@ -365,12 +330,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements private void invokeChannelRead(Object msg) { try { - callDepth ++; - try { - ((ChannelInboundHandler) handler).channelRead(this, msg); - } finally { - callDepth --; - } + ((ChannelInboundHandler) handler).channelRead(this, msg); } catch (Throwable t) { notifyHandlerException(t); } @@ -399,12 +359,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements private void invokeChannelReadComplete() { try { - callDepth ++; - try { - ((ChannelInboundHandler) handler).channelReadComplete(this); - } finally { - callDepth --; - } + ((ChannelInboundHandler) handler).channelReadComplete(this); } catch (Throwable t) { notifyHandlerException(t); } @@ -433,12 +388,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements private void invokeChannelWritabilityChanged() { try { - callDepth ++; - try { - ((ChannelInboundHandler) handler).channelWritabilityChanged(this); - } finally { - callDepth --; - } + ((ChannelInboundHandler) handler).channelWritabilityChanged(this); } catch (Throwable t) { notifyHandlerException(t); } @@ -499,12 +449,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements private void invokeBind(SocketAddress localAddress, ChannelPromise promise) { try { - callDepth ++; - try { - ((ChannelOutboundHandler) handler).bind(this, localAddress, promise); - } finally { - callDepth --; - } + ((ChannelOutboundHandler) handler).bind(this, localAddress, promise); } 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 ++; - try { - ((ChannelOutboundHandler) handler).connect(this, remoteAddress, localAddress, promise); - } finally { - callDepth --; - } + ((ChannelOutboundHandler) handler).connect(this, remoteAddress, localAddress, promise); } catch (Throwable t) { notifyOutboundHandlerException(t, promise); } @@ -582,12 +522,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements private void invokeDisconnect(ChannelPromise promise) { try { - callDepth ++; - try { - ((ChannelOutboundHandler) handler).disconnect(this, promise); - } finally { - callDepth --; - } + ((ChannelOutboundHandler) handler).disconnect(this, promise); } catch (Throwable t) { notifyOutboundHandlerException(t, promise); } @@ -615,12 +550,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements private void invokeClose(ChannelPromise promise) { try { - callDepth ++; - try { - ((ChannelOutboundHandler) handler).close(this, promise); - } finally { - callDepth --; - } + ((ChannelOutboundHandler) handler).close(this, promise); } catch (Throwable t) { notifyOutboundHandlerException(t, promise); } @@ -648,12 +578,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements private void invokeDeregister(ChannelPromise promise) { try { - callDepth ++; - try { - ((ChannelOutboundHandler) handler).deregister(this, promise); - } finally { - callDepth --; - } + ((ChannelOutboundHandler) handler).deregister(this, promise); } catch (Throwable t) { notifyOutboundHandlerException(t, promise); } @@ -683,12 +608,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements private void invokeRead() { try { - callDepth ++; - try { - ((ChannelOutboundHandler) handler).read(this); - } finally { - callDepth --; - } + ((ChannelOutboundHandler) handler).read(this); } catch (Throwable t) { notifyHandlerException(t); } @@ -724,12 +644,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements private void invokeWrite(Object msg, ChannelPromise promise) { try { - callDepth ++; - try { - ((ChannelOutboundHandler) handler).write(this, msg, promise); - } finally { - callDepth --; - } + ((ChannelOutboundHandler) handler).write(this, msg, promise); } catch (Throwable t) { notifyOutboundHandlerException(t, promise); } @@ -759,12 +674,7 @@ final class DefaultChannelHandlerContext extends DefaultAttributeMap implements private void invokeFlush() { try { - callDepth ++; - try { - ((ChannelOutboundHandler) handler).flush(this); - } finally { - callDepth --; - } + ((ChannelOutboundHandler) handler).flush(this); } catch (Throwable t) { notifyHandlerException(t); } diff --git a/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java b/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java index 121baedf77..e835e8c63f 100644 --- a/transport/src/main/java/io/netty/channel/DefaultChannelPipeline.java +++ b/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.executor().inEventLoop() || ctx.isRunning()) { - ctx.executor().execute(new Runnable() { + if (ctx.channel().isRegistered() && !ctx.executor().inEventLoop()) { + ctx.executor().execute(new Runnable() { @Override public void run() { callHandlerRemoved0(ctx); - } + } }); - return; - } + return; } callHandlerRemoved0(ctx); }