Revert HttpPostMultipartRequestDecoder and HttpPostStandardRequestDecoder to e5951d46fc (#10989)

Motivation:

The changes introduced in 1c230405fd did cause various issues while the fix itself is not considered critical. For now it is considered the best to just rollback and investigate more.

Modifications:

- Revert changes done in 1c230405fd (and later) for
the post decoders.
- Ensure we give memory back to the system as soon as possible in a safe manner

Result:

Fixes https://github.com/netty/netty/issues/10973

codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java

@@ -128,11 +128,6 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest
      */
     private Attribute currentAttribute;
 
-    /**
-     * The current Data position before finding delimiter
-     */
-    private int lastDataPosition;
-
     private boolean destroyed;
 
     private int discardThreshold = HttpPostRequestDecoder.DEFAULT_DISCARD_THRESHOLD;
@@ -347,15 +342,22 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest
                     // which is not really usable for us as we may exceed it once we add more bytes.
                     buf.alloc().buffer(buf.readableBytes()).writeBytes(buf);
         } else {
-            int readPos = undecodedChunk.readerIndex();
-            int writable = undecodedChunk.writableBytes();
-            int toWrite = buf.readableBytes();
-            if (undecodedChunk.refCnt() == 1 && writable < toWrite && readPos + writable >= toWrite) {
-                undecodedChunk.discardReadBytes();
-            }
             undecodedChunk.writeBytes(buf);
         }
         parseBody();
+        if (undecodedChunk != null && undecodedChunk.writerIndex() > discardThreshold) {
+            if (undecodedChunk.refCnt() == 1) {
+                // It's safe to call discardBytes() as we are the only owner of the buffer.
+                undecodedChunk.discardReadBytes();
+            } else {
+                // There seems to be multiple references of the buffer. Let's copy the data and release the buffer to
+                // ensure we can give back memory to the system.
+                ByteBuf buffer = undecodedChunk.alloc().buffer(undecodedChunk.readableBytes());
+                buffer.writeBytes(undecodedChunk);
+                undecodedChunk.release();
+                undecodedChunk = buffer;
+            }
+        }
         return this;
     }
 
@@ -981,24 +983,86 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest
      *             Need more chunks and reset the {@code readerIndex} to the previous
      *             value
      */
-    private static String readLine(ByteBuf undecodedChunk, Charset charset) {
+    private static String readLineStandard(ByteBuf undecodedChunk, Charset charset) {
-        final int readerIndex = undecodedChunk.readerIndex();
+        int readerIndex = undecodedChunk.readerIndex();
-        int posLf = undecodedChunk.bytesBefore(HttpConstants.LF);
+        ByteBuf line = undecodedChunk.alloc().heapBuffer(64);
-        if (posLf == -1) {
+        try {
+            while (undecodedChunk.isReadable()) {
+                byte nextByte = undecodedChunk.readByte();
+                if (nextByte == HttpConstants.CR) {
+                    // check but do not changed readerIndex
+                    nextByte = undecodedChunk.getByte(undecodedChunk.readerIndex());
+                    if (nextByte == HttpConstants.LF) {
+                        // force read
+                        undecodedChunk.readByte();
+                        return line.toString(charset);
+                    } else {
+                        // Write CR (not followed by LF)
+                        line.writeByte(HttpConstants.CR);
+                    }
+                } else if (nextByte == HttpConstants.LF) {
+                    return line.toString(charset);
+                } else {
+                    line.writeByte(nextByte);
+                }
+            }
+        } catch (IndexOutOfBoundsException e) {
+            undecodedChunk.readerIndex(readerIndex);
+            throw new NotEnoughDataDecoderException(e);
+        } finally {
+            line.release();
+        }
+        undecodedChunk.readerIndex(readerIndex);
         throw new NotEnoughDataDecoderException();
     }
-        boolean crFound =
+
-            undecodedChunk.getByte(readerIndex + posLf - 1) == HttpConstants.CR;
+    /**
-        if (crFound) {
+     * Read one line up to the CRLF or LF
-            posLf--;
+     *
+     * @return the String from one line
+     * @throws NotEnoughDataDecoderException
+     *             Need more chunks and reset the {@code readerIndex} to the previous
+     *             value
+     */
+    private static String readLine(ByteBuf undecodedChunk, Charset charset) {
+        if (!undecodedChunk.hasArray()) {
+            return readLineStandard(undecodedChunk, charset);
         }
-        CharSequence line = undecodedChunk.readCharSequence(posLf, charset);
+        SeekAheadOptimize sao = new SeekAheadOptimize(undecodedChunk);
-        if (crFound) {
+        int readerIndex = undecodedChunk.readerIndex();
-            undecodedChunk.skipBytes(2);
+        ByteBuf line = undecodedChunk.alloc().heapBuffer(64);
+        try {
+            while (sao.pos < sao.limit) {
+                byte nextByte = sao.bytes[sao.pos++];
+                if (nextByte == HttpConstants.CR) {
+                    if (sao.pos < sao.limit) {
+                        nextByte = sao.bytes[sao.pos++];
+                        if (nextByte == HttpConstants.LF) {
+                            sao.setReadPosition(0);
+                            return line.toString(charset);
                         } else {
-            undecodedChunk.skipBytes(1);
+                            // Write CR (not followed by LF)
+                            sao.pos--;
+                            line.writeByte(HttpConstants.CR);
                         }
-        return line.toString();
+                    } else {
+                        line.writeByte(nextByte);
+                    }
+                } else if (nextByte == HttpConstants.LF) {
+                    sao.setReadPosition(0);
+                    return line.toString(charset);
+                } else {
+                    line.writeByte(nextByte);
+                }
+            }
+        } catch (IndexOutOfBoundsException e) {
+            undecodedChunk.readerIndex(readerIndex);
+            throw new NotEnoughDataDecoderException(e);
+        } finally {
+            line.release();
+        }
+        undecodedChunk.readerIndex(readerIndex);
+        throw new NotEnoughDataDecoderException();
     }
 
     /**
@@ -1016,27 +1080,31 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest
      *             Need more chunks and reset the {@code readerIndex} to the previous
      *             value
      */
-    private String readDelimiter(ByteBuf undecodedChunk, String delimiter) {
+    private static String readDelimiterStandard(ByteBuf undecodedChunk, String delimiter) {
-        final int readerIndex = undecodedChunk.readerIndex();
+        int readerIndex = undecodedChunk.readerIndex();
         try {
-            final int len = delimiter.length();
+            StringBuilder sb = new StringBuilder(64);
-            if (len + 2 > undecodedChunk.readableBytes()) {
+            int delimiterPos = 0;
-                // Not able to check if "--" is present
+            int len = delimiter.length();
+            while (undecodedChunk.isReadable() && delimiterPos < len) {
+                byte nextByte = undecodedChunk.readByte();
+                if (nextByte == delimiter.charAt(delimiterPos)) {
+                    delimiterPos++;
+                    sb.append((char) nextByte);
+                } else {
+                    // delimiter not found so break here !
+                    undecodedChunk.readerIndex(readerIndex);
                     throw new NotEnoughDataDecoderException();
                 }
-            int newPositionDelimiter = findDelimiter(undecodedChunk, delimiter, 0);
-            if (newPositionDelimiter != 0) {
-                // Delimiter not fully found
-                throw new NotEnoughDataDecoderException();
             }
-            byte nextByte = undecodedChunk.getByte(readerIndex + len);
+            // Now check if either opening delimiter or closing delimiter
+            if (undecodedChunk.isReadable()) {
+                byte nextByte = undecodedChunk.readByte();
                 // first check for opening delimiter
                 if (nextByte == HttpConstants.CR) {
-                nextByte = undecodedChunk.getByte(readerIndex + len + 1);
+                    nextByte = undecodedChunk.readByte();
                     if (nextByte == HttpConstants.LF) {
-                    CharSequence line = undecodedChunk.readCharSequence(len, charset);
+                        return sb.toString();
-                    undecodedChunk.skipBytes(2);
-                    return line.toString();
                     } else {
                         // error since CR must be followed by LF
                         // delimiter not found so break here !
@@ -1044,21 +1112,20 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest
                         throw new NotEnoughDataDecoderException();
                     }
                 } else if (nextByte == HttpConstants.LF) {
-                CharSequence line = undecodedChunk.readCharSequence(len, charset);
+                    return sb.toString();
-                undecodedChunk.skipBytes(1);
-                return line.toString();
                 } else if (nextByte == '-') {
+                    sb.append('-');
                     // second check for closing delimiter
-                nextByte = undecodedChunk.getByte(readerIndex + len + 1);
+                    nextByte = undecodedChunk.readByte();
                     if (nextByte == '-') {
-                    CharSequence line = undecodedChunk.readCharSequence(len + 2, charset);
+                        sb.append('-');
                         // now try to find if CRLF or LF there
                         if (undecodedChunk.isReadable()) {
                             nextByte = undecodedChunk.readByte();
                             if (nextByte == HttpConstants.CR) {
                                 nextByte = undecodedChunk.readByte();
                                 if (nextByte == HttpConstants.LF) {
-                                return line.toString();
+                                    return sb.toString();
                                 } else {
                                     // error CR without LF
                                     // delimiter not found so break here !
@@ -1066,24 +1133,25 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest
                                     throw new NotEnoughDataDecoderException();
                                 }
                             } else if (nextByte == HttpConstants.LF) {
-                            return line.toString();
+                                return sb.toString();
                             } else {
                                 // No CRLF but ok however (Adobe Flash uploader)
                                 // minus 1 since we read one char ahead but
                                 // should not
                                 undecodedChunk.readerIndex(undecodedChunk.readerIndex() - 1);
-                            return line.toString();
+                                return sb.toString();
                             }
                         }
                         // FIXME what do we do here?
                         // either considering it is fine, either waiting for
                         // more data to come?
                         // lets try considering it is fine...
-                    return line.toString();
+                        return sb.toString();
                     }
                     // only one '-' => not enough
                     // whatever now => error since incomplete
                 }
+            }
         } catch (IndexOutOfBoundsException e) {
             undecodedChunk.readerIndex(readerIndex);
             throw new NotEnoughDataDecoderException(e);
@@ -1093,93 +1161,220 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest
     }
 
     /**
-     * @param undecodedChunk the source where the delimiter is to be found
+     * Read one line up to --delimiter or --delimiter-- and if existing the CRLF
-     * @param delimiter the string to find out
+     * or LF. Note that CRLF or LF are mandatory for opening delimiter
-     * @param offset the offset from readerIndex within the undecodedChunk to
+     * (--delimiter) but not for closing delimiter (--delimiter--) since some
-     *     start from to find out the delimiter
+     * clients does not include CRLF in this case.
      *
-     * @return a number >= 0 if found, else new offset with negative value
+     * @param delimiter
-     *     (to inverse), both from readerIndex
+     *            of the form --string, such that '--' is already included
+     * @return the String from one line as the delimiter searched (opening or
+     *         closing)
      * @throws NotEnoughDataDecoderException
-     *             Need more chunks while relative position with readerIndex is 0
+     *             Need more chunks and reset the readerInder to the previous
+     *             value
      */
-    private static int findDelimiter(ByteBuf undecodedChunk, String delimiter, int offset) {
+    private static String readDelimiter(ByteBuf undecodedChunk, String delimiter) {
-        final int startReaderIndex = undecodedChunk.readerIndex();
+        if (!undecodedChunk.hasArray()) {
-        final int delimeterLength = delimiter.length();
+            return readDelimiterStandard(undecodedChunk, delimiter);
-        final int toRead = undecodedChunk.readableBytes();
-        int newOffset = offset;
-        boolean delimiterNotFound = true;
-        while (delimiterNotFound && newOffset + delimeterLength <= toRead) {
-            int posFirstChar = undecodedChunk
-                .bytesBefore(startReaderIndex + newOffset, toRead - newOffset,
-                             (byte) delimiter.codePointAt(0));
-            if (posFirstChar == -1) {
-                newOffset = toRead;
-                return -newOffset;
         }
-            newOffset = posFirstChar + newOffset;
+        SeekAheadOptimize sao = new SeekAheadOptimize(undecodedChunk);
-            if (newOffset + delimeterLength > toRead) {
+        int readerIndex = undecodedChunk.readerIndex();
-                return -newOffset;
+        int delimiterPos = 0;
-            }
+        int len = delimiter.length();
-            // assume will found it
+        try {
-            delimiterNotFound = false;
+            StringBuilder sb = new StringBuilder(64);
-            for (int index = 1; index < delimeterLength; index++) {
+            // check conformity with delimiter
-                if (undecodedChunk.getByte(startReaderIndex + newOffset + index) != delimiter.codePointAt(index)) {
+            while (sao.pos < sao.limit && delimiterPos < len) {
-                    // ignore first found offset and redo search from next char
+                byte nextByte = sao.bytes[sao.pos++];
-                    newOffset++;
+                if (nextByte == delimiter.charAt(delimiterPos)) {
-                    delimiterNotFound = true;
+                    delimiterPos++;
-                    break;
+                    sb.append((char) nextByte);
-                }
+                } else {
-            }
+                    // delimiter not found so break here !
-        }
+                    undecodedChunk.readerIndex(readerIndex);
-        if (delimiterNotFound || newOffset + delimeterLength > toRead) {
-            if (newOffset == 0) {
                     throw new NotEnoughDataDecoderException();
                 }
-            return -newOffset;
             }
-        return newOffset;
+            // Now check if either opening delimiter or closing delimiter
+            if (sao.pos < sao.limit) {
+                byte nextByte = sao.bytes[sao.pos++];
+                if (nextByte == HttpConstants.CR) {
+                    // first check for opening delimiter
+                    if (sao.pos < sao.limit) {
+                        nextByte = sao.bytes[sao.pos++];
+                        if (nextByte == HttpConstants.LF) {
+                            sao.setReadPosition(0);
+                            return sb.toString();
+                        } else {
+                            // error CR without LF
+                            // delimiter not found so break here !
+                            undecodedChunk.readerIndex(readerIndex);
+                            throw new NotEnoughDataDecoderException();
+                        }
+                    } else {
+                        // error since CR must be followed by LF
+                        // delimiter not found so break here !
+                        undecodedChunk.readerIndex(readerIndex);
+                        throw new NotEnoughDataDecoderException();
+                    }
+                } else if (nextByte == HttpConstants.LF) {
+                    // same first check for opening delimiter where LF used with
+                    // no CR
+                    sao.setReadPosition(0);
+                    return sb.toString();
+                } else if (nextByte == '-') {
+                    sb.append('-');
+                    // second check for closing delimiter
+                    if (sao.pos < sao.limit) {
+                        nextByte = sao.bytes[sao.pos++];
+                        if (nextByte == '-') {
+                            sb.append('-');
+                            // now try to find if CRLF or LF there
+                            if (sao.pos < sao.limit) {
+                                nextByte = sao.bytes[sao.pos++];
+                                if (nextByte == HttpConstants.CR) {
+                                    if (sao.pos < sao.limit) {
+                                        nextByte = sao.bytes[sao.pos++];
+                                        if (nextByte == HttpConstants.LF) {
+                                            sao.setReadPosition(0);
+                                            return sb.toString();
+                                        } else {
+                                            // error CR without LF
+                                            // delimiter not found so break here !
+                                            undecodedChunk.readerIndex(readerIndex);
+                                            throw new NotEnoughDataDecoderException();
+                                        }
+                                    } else {
+                                        // error CR without LF
+                                        // delimiter not found so break here !
+                                        undecodedChunk.readerIndex(readerIndex);
+                                        throw new NotEnoughDataDecoderException();
+                                    }
+                                } else if (nextByte == HttpConstants.LF) {
+                                    sao.setReadPosition(0);
+                                    return sb.toString();
+                                } else {
+                                    // No CRLF but ok however (Adobe Flash
+                                    // uploader)
+                                    // minus 1 since we read one char ahead but
+                                    // should not
+                                    sao.setReadPosition(1);
+                                    return sb.toString();
+                                }
+                            }
+                            // FIXME what do we do here?
+                            // either considering it is fine, either waiting for
+                            // more data to come?
+                            // lets try considering it is fine...
+                            sao.setReadPosition(0);
+                            return sb.toString();
+                        }
+                        // whatever now => error since incomplete
+                        // only one '-' => not enough or whatever not enough
+                        // element
+                    }
+                }
+            }
+        } catch (IndexOutOfBoundsException e) {
+            undecodedChunk.readerIndex(readerIndex);
+            throw new NotEnoughDataDecoderException(e);
+        }
+        undecodedChunk.readerIndex(readerIndex);
+        throw new NotEnoughDataDecoderException();
+    }
+
+    /**
+     * Load the field value or file data from a Multipart request
+     *
+     * @return {@code true} if the last chunk is loaded (boundary delimiter found), {@code false} if need more chunks
+     * @throws ErrorDataDecoderException
+     */
+    private static boolean loadDataMultipartStandard(ByteBuf undecodedChunk, String delimiter, HttpData httpData) {
+        final int startReaderIndex = undecodedChunk.readerIndex();
+        final int delimeterLength = delimiter.length();
+        int index = 0;
+        int lastPosition = startReaderIndex;
+        byte prevByte = HttpConstants.LF;
+        boolean delimiterFound = false;
+        while (undecodedChunk.isReadable()) {
+            final byte nextByte = undecodedChunk.readByte();
+            // Check the delimiter
+            if (prevByte == HttpConstants.LF && nextByte == delimiter.codePointAt(index)) {
+                index++;
+                if (delimeterLength == index) {
+                    delimiterFound = true;
+                    break;
+                }
+                continue;
+            }
+            lastPosition = undecodedChunk.readerIndex();
+            if (nextByte == HttpConstants.LF) {
+                index = 0;
+                lastPosition -= (prevByte == HttpConstants.CR)? 2 : 1;
+            }
+            prevByte = nextByte;
+        }
+        if (prevByte == HttpConstants.CR) {
+            lastPosition--;
+        }
+        ByteBuf content = undecodedChunk.retainedSlice(startReaderIndex, lastPosition - startReaderIndex);
+        try {
+            httpData.addContent(content, delimiterFound);
+        } catch (IOException e) {
+            throw new ErrorDataDecoderException(e);
+        }
+        undecodedChunk.readerIndex(lastPosition);
+        return delimiterFound;
     }
 
     /**
      * Load the field value from a Multipart request
      *
      * @return {@code true} if the last chunk is loaded (boundary delimiter found), {@code false} if need more chunks
-     *
      * @throws ErrorDataDecoderException
      */
-    private boolean loadDataMultipart(ByteBuf undecodedChunk, String delimiter,
+    private static boolean loadDataMultipart(ByteBuf undecodedChunk, String delimiter, HttpData httpData) {
-                                      HttpData httpData) {
+        if (!undecodedChunk.hasArray()) {
+            return loadDataMultipartStandard(undecodedChunk, delimiter, httpData);
+        }
+        final SeekAheadOptimize sao = new SeekAheadOptimize(undecodedChunk);
         final int startReaderIndex = undecodedChunk.readerIndex();
-        int newOffset;
+        final int delimeterLength = delimiter.length();
+        int index = 0;
+        int lastRealPos = sao.pos;
+        byte prevByte = HttpConstants.LF;
+        boolean delimiterFound = false;
+        while (sao.pos < sao.limit) {
+            final byte nextByte = sao.bytes[sao.pos++];
+            // Check the delimiter
+            if (prevByte == HttpConstants.LF && nextByte == delimiter.codePointAt(index)) {
+                index++;
+                if (delimeterLength == index) {
+                    delimiterFound = true;
+                    break;
+                }
+                continue;
+            }
+            lastRealPos = sao.pos;
+            if (nextByte == HttpConstants.LF) {
+                index = 0;
+                lastRealPos -= (prevByte == HttpConstants.CR)? 2 : 1;
+            }
+            prevByte = nextByte;
+        }
+        if (prevByte == HttpConstants.CR) {
+            lastRealPos--;
+        }
+        final int lastPosition = sao.getReadPosition(lastRealPos);
+        final ByteBuf content = undecodedChunk.retainedSlice(startReaderIndex, lastPosition - startReaderIndex);
         try {
-            newOffset = findDelimiter(undecodedChunk, delimiter, lastDataPosition);
+            httpData.addContent(content, delimiterFound);
-            if (newOffset < 0) {
-                // delimiter not found
-                lastDataPosition = -newOffset;
-                return false;
-            }
-        } catch (NotEnoughDataDecoderException e) {
-            // Not enough data and no change to lastDataPosition
-            return false;
-        }
-        // found delimiter but still need to check if CRLF before
-        int startDelimiter = newOffset;
-        if (undecodedChunk.getByte(startReaderIndex + startDelimiter - 1) == HttpConstants.LF) {
-            startDelimiter--;
-            if (undecodedChunk.getByte(startReaderIndex + startDelimiter - 1) == HttpConstants.CR) {
-                startDelimiter--;
-            }
-        }
-        ByteBuf content = undecodedChunk.retainedSlice(startReaderIndex, startDelimiter);
-        try {
-            httpData.addContent(content, true);
         } catch (IOException e) {
             throw new ErrorDataDecoderException(e);
         }
-        lastDataPosition = 0;
+        undecodedChunk.readerIndex(lastPosition);
-        undecodedChunk.readerIndex(startReaderIndex + startDelimiter);
+        return delimiterFound;
-        return true;
     }
 
     /**

codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostStandardRequestDecoder.java

@@ -300,15 +300,12 @@ public class HttpPostStandardRequestDecoder implements InterfaceHttpPostRequestD
                     // which is not really usable for us as we may exceed it once we add more bytes.
                     buf.alloc().buffer(buf.readableBytes()).writeBytes(buf);
         } else {
-            int readPos = undecodedChunk.readerIndex();
-            int writable = undecodedChunk.writableBytes();
-            int toWrite = buf.readableBytes();
-            if (undecodedChunk.refCnt() == 1 && writable < toWrite && readPos + writable >= toWrite) {
-                undecodedChunk.discardReadBytes();
-            }
             undecodedChunk.writeBytes(buf);
         }
         parseBody();
+        if (undecodedChunk != null && undecodedChunk.writerIndex() > discardThreshold) {
+            undecodedChunk.discardReadBytes();
+        }
         return this;
     }