This is an automated email from the ASF dual-hosted git repository.

btellier pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/james-project.git

commit 3222c5dcbb2886c5c75cacd29917b8997e32777e
Author: Benoit Tellier <btell...@linagora.com>
AuthorDate: Wed Mar 9 15:43:31 2022 +0700

    JAMES-3715 IMAP fix framer issues (IndexOutOfBound on bytebuf when turned 
on/off)
    
    This execption was raised for commands following a IMAP APPEND
    (that did turn off and on the framer), Issue was happening all the time 
following
    threading model changes.
    
    ```
    java.lang.IndexOutOfBoundsException: index: 24, length: -15 (expected: 
range(0, 65536))
        at 
io.netty.buffer.AbstractByteBuf.checkRangeBounds(AbstractByteBuf.java:1390)
        at 
io.netty.buffer.AbstractByteBuf.checkIndex0(AbstractByteBuf.java:1397)
        at io.netty.buffer.AbstractByteBuf.checkIndex(AbstractByteBuf.java:1384)
        at 
io.netty.buffer.AbstractByteBuf.forEachByte(AbstractByteBuf.java:1290)
        at 
io.netty.handler.codec.LineBasedFrameDecoder.findEndOfLine(LineBasedFrameDecoder.java:169)
        at 
io.netty.handler.codec.LineBasedFrameDecoder.decode(LineBasedFrameDecoder.java:99)
        at 
org.apache.james.protocols.netty.AllButStartTlsLineBasedChannelHandler.decode(AllButStartTlsLineBasedChannelHandler.java:61)
        at 
io.netty.handler.codec.LineBasedFrameDecoder.decode(LineBasedFrameDecoder.java:84)
        at 
io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:507)
        at 
io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:446)
        ... 10 common frames omitted
    Wrapped by: io.netty.handler.codec.DecoderException: 
java.lang.IndexOutOfBoundsException: index: 24, length: -15 (expected: range(0, 
65536))
        at 
io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:477)
        at 
io.netty.handler.codec.ByteToMessageDecoder.channelInputClosed(ByteToMessageDecoder.java:404)
        at 
io.netty.handler.codec.ByteToMessageDecoder.channelInputClosed(ByteToMessageDecoder.java:371)
        at 
io.netty.handler.codec.ByteToMessageDecoder.channelInactive(ByteToMessageDecoder.java:354)
        at 
io.netty.channel.AbstractChannelHandlerContext.invokeChannelInactive(AbstractChannelHandlerContext.java:262)
        at 
io.netty.channel.AbstractChannelHandlerContext.access$300(AbstractChannelHandlerContext.java:61)
        at 
io.netty.channel.AbstractChannelHandlerContext$4.run(AbstractChannelHandlerContext.java:253)
        at 
io.netty.util.concurrent.DefaultEventExecutor.run(DefaultEventExecutor.java:66)
        at 
io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:986)
        at 
io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
        at java.base/java.lang.Thread.run(Thread.java:829)
    ```
---
 .../apache/james/imapserver/netty/IMAPServer.java  |  3 +-
 .../imapserver/netty/ImapRequestFrameDecoder.java  | 39 +++++++++++++++++-----
 .../netty/SwitchableLineBasedFrameDecoder.java     | 26 ---------------
 3 files changed, 32 insertions(+), 36 deletions(-)

diff --git 
a/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/IMAPServer.java
 
b/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/IMAPServer.java
index 649f4a8..15132e3 100644
--- 
a/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/IMAPServer.java
+++ 
b/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/IMAPServer.java
@@ -250,7 +250,8 @@ public class IMAPServer extends 
AbstractConfigurableAsyncServer implements ImapC
 
                 pipeline.addLast(getExecutorGroup(), CHUNK_WRITE_HANDLER, new 
ChunkedWriteHandler());
 
-                pipeline.addLast(getExecutorGroup(), REQUEST_DECODER, new 
ImapRequestFrameDecoder(decoder, inMemorySizeLimit, literalSizeLimit));
+                pipeline.addLast(getExecutorGroup(), REQUEST_DECODER, new 
ImapRequestFrameDecoder(decoder, inMemorySizeLimit,
+                    literalSizeLimit, getExecutorGroup(), maxLineLength));
 
                 pipeline.addLast(getExecutorGroup(), CORE_HANDLER, 
createCoreHandler());
             }
diff --git 
a/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/ImapRequestFrameDecoder.java
 
b/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/ImapRequestFrameDecoder.java
index 485362b..027fd3c 100644
--- 
a/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/ImapRequestFrameDecoder.java
+++ 
b/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/ImapRequestFrameDecoder.java
@@ -44,8 +44,8 @@ import io.netty.buffer.Unpooled;
 import io.netty.channel.ChannelFutureListener;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelInboundHandlerAdapter;
-import io.netty.channel.ChannelPipeline;
 import io.netty.handler.codec.ByteToMessageDecoder;
+import io.netty.util.concurrent.EventExecutorGroup;
 
 
 /**
@@ -62,11 +62,16 @@ public class ImapRequestFrameDecoder extends 
ByteToMessageDecoder implements Net
     private final int inMemorySizeLimit;
     private final int literalSizeLimit;
     private final Deque<ChannelInboundHandlerAdapter> behaviourOverrides = new 
ConcurrentLinkedDeque<>();
+    private final EventExecutorGroup eventExecutors;
+    private int maxFrameLength;
 
-    public ImapRequestFrameDecoder(ImapDecoder decoder, int inMemorySizeLimit, 
int literalSizeLimit) {
+    public ImapRequestFrameDecoder(ImapDecoder decoder, int inMemorySizeLimit, 
int literalSizeLimit, EventExecutorGroup eventExecutors,
+                                   int maxFrameLength) {
         this.decoder = decoder;
         this.inMemorySizeLimit = inMemorySizeLimit;
         this.literalSizeLimit = literalSizeLimit;
+        this.eventExecutors = eventExecutors;
+        this.maxFrameLength = maxFrameLength;
     }
 
     @Override
@@ -180,7 +185,7 @@ public class ImapRequestFrameDecoder extends 
ByteToMessageDecoder implements Net
                     reader.consumeLine();
                 }
                 
-                ((SwitchableLineBasedFrameDecoder) 
ctx.channel().pipeline().get(FRAMER)).enableFraming();
+                enableFraming(ctx);
                 
                 attachment.clear();
                 out.add(message);
@@ -190,15 +195,15 @@ public class ImapRequestFrameDecoder extends 
ByteToMessageDecoder implements Net
                 int neededData = e.getNeededSize();
                 // store the needed data size for later usage
                 attachment.put(NEEDED_DATA, neededData);
-                
-                final ChannelPipeline pipeline = ctx.channel().pipeline();
-                final ChannelHandlerContext framerContext = 
pipeline.context(FRAMER);
 
                 // SwitchableDelimiterBasedFrameDecoder added further to 
JAMES-1436.
-                final SwitchableLineBasedFrameDecoder framer = 
(SwitchableLineBasedFrameDecoder) pipeline.get(FRAMER);
-
+                disableFraming(ctx);
+                if (in.readableBytes() > 0) {
+                    ByteBuf spareBytes = in.retainedDuplicate();
+                    internalBuffer().clear();
+                    ctx.fireChannelRead(spareBytes);
+                }
                 in.resetReaderIndex();
-                framer.disableFraming(framerContext);
             }
         } else {
             // The session was null so may be the case because the channel was 
already closed but there were still bytes in the buffer.
@@ -209,6 +214,22 @@ public class ImapRequestFrameDecoder extends 
ByteToMessageDecoder implements Net
         }
     }
 
+    public void disableFraming(ChannelHandlerContext ctx) {
+        ctx.channel().config().setAutoRead(false);
+        ctx.channel().eventLoop().execute(() -> 
ctx.channel().pipeline().remove(FRAMER));
+        ctx.channel().config().setAutoRead(true);
+    }
+
+    public void enableFraming(ChannelHandlerContext ctx) {
+        if (ctx.channel().pipeline().get(FRAMER) == null) {
+            ctx.channel().config().setAutoRead(false);
+            ctx.channel().eventLoop().execute(() ->
+                ctx.channel().pipeline().addBefore(eventExecutors, 
REQUEST_DECODER, FRAMER,
+                        new 
SwitchableLineBasedFrameDecoder(ctx.channel().pipeline(), maxFrameLength, 
false)));
+            ctx.channel().config().setAutoRead(true);
+        }
+    }
+
     @Override
     public void pushLineHandler(ChannelInboundHandlerAdapter 
lineHandlerUpstreamHandler) {
         behaviourOverrides.addFirst(lineHandlerUpstreamHandler);
diff --git 
a/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/SwitchableLineBasedFrameDecoder.java
 
b/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/SwitchableLineBasedFrameDecoder.java
index 7ad1257..2f4d117 100644
--- 
a/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/SwitchableLineBasedFrameDecoder.java
+++ 
b/server/protocols/protocols-imap4/src/main/java/org/apache/james/imapserver/netty/SwitchableLineBasedFrameDecoder.java
@@ -25,43 +25,17 @@ import org.apache.james.imap.api.ImapConstants;
 import org.apache.james.protocols.api.CommandDetectionSession;
 import org.apache.james.protocols.netty.AllButStartTlsLineBasedChannelHandler;
 
-import io.netty.buffer.ByteBuf;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelPipeline;
 
 public class SwitchableLineBasedFrameDecoder extends 
AllButStartTlsLineBasedChannelHandler {
     public static final String PATTERN = 
ImapConstants.STARTTLS_COMMAND.getName().toLowerCase();
 
-    private volatile boolean framingEnabled = true;
-
     public SwitchableLineBasedFrameDecoder(ChannelPipeline pipeline, int 
maxFrameLength, boolean stripDelimiter) {
         super(pipeline, maxFrameLength, stripDelimiter, PATTERN);
     }
 
     @Override
-    public synchronized void channelRead(ChannelHandlerContext ctx, Object 
msg) throws Exception {
-        if (this.framingEnabled) {
-            super.channelRead(ctx, msg);
-        } else {
-            ctx.fireChannelRead(msg);
-        }
-    }
-
-    public synchronized void enableFraming() {
-        this.framingEnabled = true;
-    }
-
-    public synchronized void disableFraming(ChannelHandlerContext ctx) {
-        this.framingEnabled = false;
-
-        if (internalBuffer().readableBytes() > 0) {
-            ByteBuf spareBytes = internalBuffer().retainedDuplicate();
-            internalBuffer().clear();
-            ctx.fireChannelRead(spareBytes);
-        }
-    }
-
-    @Override
     protected CommandDetectionSession retrieveSession(ChannelHandlerContext 
ctx) {
         return 
ctx.channel().attr(NettyConstants.IMAP_SESSION_ATTRIBUTE_KEY).get();
     }

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@james.apache.org
For additional commands, e-mail: notifications-h...@james.apache.org

Reply via email to