Repository: netty/netty
Issue: HttpPostMultipartRequestDecoder may not add content to an existing upload after being offered data

### Expected behavior

Once a file upload object exists in the multipart request decoder, but not finished, offering more data to the decoder should populate the buffer of the file object

### Actual behavior

The buffer is not created/updated

### Steps to reproduce

- `git clone https://github.com/micronaut-projects/micronaut-core`
- `git checkout upgrade-netty`
- `./gradlew test-suite:test --tests "io.micronaut.upload.StreamUploadSpec.test the file is not corrupted with transferTo"`

### Netty version
4.1.60+ due to https://github.com/netty/netty/pull/11001

### JVM version (e.g. `java -version`)
openjdk version "1.8.0_282"
OpenJDK Runtime Environment (AdoptOpenJDK)(build 1.8.0_282-b08)
OpenJDK 64-Bit Server VM (AdoptOpenJDK)(build 25.282-b08, mixed mode)

### OS version (e.g. `uname -a`)
Darwin MacBook-Pro.local 20.3.0 Darwin Kernel Version 20.3.0: Thu Jan 21 00:07:06 PST 2021; root:xnu-7195.81.3~1/RELEASE_X86_64 x86_64

This issue is a blocker for Micronaut to upgrade Netty. With the functionality as it is, it is impossible to read a chunk of the file and release it immediately because new buffers are not set on the underlying file upload object.

This line is the culprit. https://github.com/fredericBregier/netty/blob/6daeb0cc51d8689805c1a657e61d395450afec47/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java#L1187

In my case `posDelimiter` is 0, so the content is never added to the upload.

Issue comments:
@jameskleeh 
From the beginning: https://github.com/fredericBregier/netty/blob/6daeb0cc51d8689805c1a657e61d395450afec47/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java#L1160

It tries to find out the `delimiter` in the full buffer (multiple chunks up to now):
- if `posDelimiter<0`, the `delimiter` is not found. So the upload cannot be finalized, but maybe we can add some bytes already.
  - So, then it tries to search for CRLF or LF
  - If not found (`posDelimiter<0`), the buffer can be appened fully (since the `CRLF/LF + delimiter` is not there)
  - If found (`posDelimiter>0`), only up to CRLF/LF (without) can be appened (the `delimiter` could be just after in the next chunk)
  - If `posDelimiter==0`, the CRLF/LF is found but at position 0, so nothing can be added (the `delimiter` could be just after in the next chunk)
  - It returns false in all cases since delimiter is not found, so the next chunk will be added to the current buffer to try to find out the `CRLF/LF + delimiter`.
- If `posDelimiter >= 0`, it means it found the `CRLF/LF + delimiter`, so it finalizes the upload.


So I see nothing wrong there.
Could you propose a reproducer with a simpler code ? It is quite too heavy to check in this huge git.
@fredericBregier I don't have confidence I could create something outside of Micronaut to reproduce the issue. I'm not sure what you mean by "It is quite too heavy to check in this huge git". I clone and run the tests in that codebase everyday so surely you can as well. 

I don't understand the importance of CRLF/LF in this context so I really can't comment on whether it makes sense or not. All I can say is that it worked correctly prior to this change.
I verified that LF is found at position 0, however your claim of "nothing can be added (the delimiter could be just after in the next chunk)" seems invalid to me. Can the contents of a file not contain line feeds? How can you assume the delimiter is coming after a LF? Is that in the RFC for multipart requests somewhere?
@jameskleeh Hi, your code is quite heavy, so difficult to search for the reason behind and to be able to reproduce and fixing it.

So the reason if you have a reproducer, with simple code (one client and one server code), then we can extract a reproducer and fix it.

On the RFC part, on multipart side, each part is separated by a CRLF or LF following by the delimiter string.
So this method objective is to find out the first occurence of the CRLF/LF + delimiter in the buffer. Once found, it has found the final delimiter for the current part. And then it continues to the next part.

Of course a part can contains a CRLF/LF. But if this is found, there is a risk such that the next bytes are the delimiter but not yet there (due to HTTP chunk by chunk reception).
Note that if a file contains exactly the CRLF/LF+delimiter string, it can be considered as a end of a part. This is of course not wanted but that's how is the RFC.

Maybe your issue is that the upload is beginning by a CRLF/LF, but not fitting within one HTTP chunk, so the delimiter will come in a later chunk. If this is that, then, once all chunks concerning this upload will arrive on the server side, it will find out the delimite preceeded by CRLF/LF, and therefore ending the part (the upload).

I believe we could try to seak for the "last" CRLF/LF, not the first one when no delimiter is found. It might be better from a memory point of view, but it will not change the logic.

The idea would be to change:
https://github.com/netty/netty/blob/1529ef1794e0a6654fa4334fd979b769d6940e61/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java#L1163

      posDelimiter = HttpPostBodyUtil.findLineBreak(undecodedChunk, startReaderIndex);

by something that will search for "last" CRLF/LF: (not yet implemented)

     posDelimiter = HttpPostBodyUtil.findLastLineBreak(undecodedChunk, startReaderIndex);

We might even check if the new posDelimiter has more space than delimiter size, in order to ensure that the delimiter is not splitten across 2 HTTP chunks as:

- HTTP Chunk 1

       file body example line 1CRLFThen line 2CRLF--del

It might takes `file body example line 1CRLFThen line 2` as first content for file.

- HTTP Chunk 2

       imiter

The delimiter is then found fully and the file is filled with a 0 length buffer.
Therefore, the file contains `file body example line 1CRLFThen line 2`.

Currently, on first step, it will gives: `file body example line 1`, then after Chunk 2: ``file body example line 1CRLFThen line 2`


But not that this will not change the behaviour: if there is never a CRLF/LF + delimiter, then the part is never ending. The difference is that, if there is a CRLF/LF in the middle (or start) of the file, it will however populate the file behind, but still waiting to find out the CRLF/LF+delimiter.
@fredericBregier I'd be happy to test any local branch with the changes
OK, I will try to make it, but I'm quite sure it will not change anything since I do not have a simple reproducer test code to test it.
But if it does the trick, well, I would be surprised since it seems to me not a fix but an improvement.

In other words, it seems your upload does not end up with a CRLF/LF + delimiter... ? But as I don't know the code, I cannot say for sure of course.
@fredericBregier I'm not quite sure but its not getting to the end of the file. I'm uploading 15 megabytes and I'm seeing this behavior on the second chunk of data being received. I'm fairly confident all of the data in the buffer when `posDelimiter` is 0 is all data for the file and does not contain any delimiters or anything specific to HTTP
If you'd like to do a screen share I'd be glad to show you
Well, could you at least point me out directly to the code:
- client side (I suppose the test itself)
- server side
I may try to do a reproducer such that I understand where the problem is. My previous point seems to me only an optimisation.
I would be glad to help to fix this out.

Of course, I could donwload all your project and following your original guidance, but perhaps I could read the code itself first ?
When I go to your github repo, I found out it is so huge, that it was difficult for me to find out where it is...

For the screen sharing, might be useful next, if I don't get it ;-)
@fredericBregier Sure

Client
--------
Here is the client that is sending the data
https://github.com/micronaut-projects/micronaut-core/blob/upgrade-netty/test-suite/src/test/groovy/io/micronaut/upload/StreamUploadSpec.groovy#L552
You can see its a random set of bytes 15mb long

Here is where the Netty specific multipart stuff is created from our API
https://github.com/micronaut-projects/micronaut-core/blob/upgrade-netty/http-client/src/main/java/io/micronaut/http/client/netty/DefaultHttpClient.java#L1525

Server
--------
Here is where the data is offered to the decoder
https://github.com/micronaut-projects/micronaut-core/blob/upgrade-netty/http-server-netty/src/main/java/io/micronaut/http/server/netty/FormDataHttpContentProcessor.java#L113

In the second invocation of the `onData` method above, the `currentPartialHttpData()` returned from the decoder will have a null buffer. See https://github.com/micronaut-projects/micronaut-core/blob/upgrade-netty/http-server-netty/src/main/java/io/micronaut/http/server/netty/FormDataHttpContentProcessor.java#L134

The buffer is null because the data previously received has already been read and released and no new data was added to the upload.

The data is released via https://github.com/micronaut-projects/micronaut-core/blob/upgrade-netty/http-server-netty/src/main/java/io/micronaut/http/server/netty/multipart/NettyStreamingFileUpload.java#L136

OK, I think I got it.
For the current Http Chunk, once `while (postRequestDecoder.hasNext())` is false, it does not mean it has nothing, it could be in the middle of creation of a new `HttpData`. It just means that there is not enough data to create a new one yet.

So I believe that maybe https://github.com/micronaut-projects/micronaut-core/blob/e67ca50cf2a778cb6c7354b1ecd2c7fe7d2910ed/http-server-netty/src/main/java/io/micronaut/http/server/netty/FormDataHttpContentProcessor.java#L134 is wrong. Current `HttpData` might be null (empty or not yet any new `HttpData`).

For instance, let say the last chunk was ending like this:

       my previous Http DataCRLF--delimiterCRLFcontent-disposition: form-data; na

Such that the next Part from multipart is not yet available to know what it is.
Therefore, the previous one is ready (`hasNext()` is true once, but not twice).
The second one is not yet created, so no new `HttpData` yet.

Another example:

       my previous Http DataCRLF--delimiterCRLFcontent-disposition: form-data; name="field1"CRLF

Then the second one `HttpData` is created but empty (no data yet).

I don't know why you are adding like this `messages.add(currentPartialHttpData);` on line 134-135, because for me, it has no meaning. As long as the decoder is not giving you something new through `postRequestDecoder.hasNext()`, you don't have to care about new `HttpData`, the decoder will keep the already available data inside and will give you as soon as it has all the next `HttpData`.

I'm on my way however to try to improve the decoder part to opimize it, but I feel like you are not using correctly the decoder however.
> I don't know why you are adding like this messages.add(currentPartialHttpData); on line 134-135, because for me, it has no meaning. As long as the decoder is not giving you something new through postRequestDecoder.hasNext(), you don't have to care about new HttpData, the decoder will keep the already available data inside and will give you as soon as it has all the next HttpData.

Because we need to notify the downstream that new data is available on the upload. Users can read and release pieces of uploads as they become available. It is often the case we don't want to buffer any data beyond a single chunk

> So I believe that maybe https://github.com/micronaut-projects/micronaut-core/blob/e67ca50cf2a778cb6c7354b1ecd2c7fe7d2910ed/http-server-netty/src/main/java/io/micronaut/http/server/netty/FormDataHttpContentProcessor.java#L134 is wrong. Current HttpData might be null (empty or not yet any new HttpData).

If it's null it wouldn't be passed through, so I don't think its wrong. This code has been in place for some time and working well

> Then the second one HttpData is created but empty (no data yet).

It's possible this is a valid case and we need to handle it, however I don't think its relevant to this issue
You can checkout the 2.4.x branch of micronaut-core to see the difference in behavior. In Netty 4.1.59 the partial upload is populated with a buffer on the second invocation and with 4.1.60+ it is not.
Yes I understood. So the "API behavior" change. But what I'm saying, is that the goal of the decoder is not to give you a "partial HttpData" but a full one when ready. You have made assumption which are internal operations.
It is almost as a Queue: when the Queue has no more yet element, it says hasNext is false. You have to wait to see the next available element when it is ready.
There you assume the Queue will say to you it has "almost" something. It implementation dependent.

I will try to make the current API to "looks like" what is was before, but my feeling IMHO is that you rely too much on the underlying implementation and not the API contract (which is not the implementation).
>But what I'm saying, is that the goal of the decoder is not to give you a "partial HttpData" but a full one when ready.

If that is the case then why is there a method to retrieve the partial data?
Good point ;-)
For me, it is the opportunity to know if there is any new HttpData on going, but not for sure.

However, I was able to reproduce easily this bug (changing behavior), by setting the first bytes of the new file to be CRLF. 
While it is not really a bug, since the decoder is going on and finish correctly to get the full `HttpData`, it indeed has a `HttpData` through `currentPartialHttpData()` but with an empty buffer at first step, while it is ok at the end.

Note that from a Disk based `HttpData`, using `HttpData.content()` or `HttpData.getByteBuf()` will read the `File` and create a newly allocated `ByteBuf` for that, giving a chance of Out Of Memory exception (if large file).

I fix also the default behavior such that when a Memory based `HttpData` has not yet data, it returns an empty buffer.
Note that current implementation does not ensure this buffer will remain the same until the `HttpData` is fully decoded.
@jameskleeh You can check out my proposal to fix the behaviour.

I believe it will run as expected now, even if it is not really a bug.
@jameskleeh Could you try https://github.com/fredericBregier/netty/tree/improveMultipartAddingBuffer ?
Yeah I’ll give it a go today

On Thu, Apr 8, 2021 at 7:56 AM Frédéric Brégier ***@***.***>
wrote:

> @jameskleeh <https://github.com/jameskleeh> Could you try
> https://github.com/fredericBregier/netty/tree/improveMultipartAddingBuffer
> ?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/netty/netty/issues/11143#issuecomment-815712194>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAMCVLIOCP4ZVP3MLRIIBS3THWKVVANCNFSM42PB7GYA>
> .
>

@fredericBregier This is better, but still not ideal. The very first time the decoder is offered data results in the upload being created with a composite buffer where the first buffer is empty and the second one contains the data.

<img width="795" alt="Screen Shot 2021-04-08 at 3 39 03 PM" src="https://user-images.githubusercontent.com/1583789/114086536-b8499d00-9880-11eb-8bde-b2275f063f23.png">

This is a deviation from previous behavior where it would be a non composite buffer. The buffer being a composite would not necessarily be a problem, however having 2 components is because I'm relying on a chunk of data being offered to the decoder only resulting in a single chunk of data being added to any single upload.
Hmm, I see. I will try to optimize one more time.
I cannot prevent CompositeByteBuf due to the algorithm, but I can prevent that, if the first and current buffer is an empty one, the n the second one will replace the first one. But if there is a third one, then it will necessary be a Composite one.
@jameskleeh  I've done the new fix. You can rebase and recheck when you can.
@fredericBregier Another step in the right direction, however I'm now finding that the upload never completes.
@jameskleeh Very strange since the only difference is the following with the previous version:

            } else if (byteBuf.readableBytes() == 0) {
                // Previous buffer is empty, so just replace it
                byteBuf.release();
                byteBuf = buffer;

I will check but I don't understand. Have you a trace (log error) ? It might be the release of the first empty buffer, but ver strange then.
Moreover, all Junit tests are passing... So the reason I don't get it. I double check, and I see nothing wrong.
If you have traces or error logs ?
Just in case I understand well your code: https://github.com/micronaut-projects/micronaut-core/blob/e67ca50cf2a778cb6c7354b1ecd2c7fe7d2910ed/http-server-netty/src/main/java/io/micronaut/http/server/netty/FormDataHttpContentProcessor.java#L111

                while (postRequestDecoder.hasNext()) {
                    InterfaceHttpData data = postRequestDecoder.next();
                    switch (data.getHttpDataType()) {
                        case Attribute:
                            Attribute attribute = (Attribute) data;
                            messages.add(attribute); //2
                            break;
                        case FileUpload:
                            FileUpload fileUpload = (FileUpload) data;
                            if (fileUpload.isCompleted()) { //1
                                messages.add(fileUpload);
                            }
                            break;
                        default:
                            // no-op
                    }
                }

                InterfaceHttpData currentPartialHttpData = postRequestDecoder.currentPartialHttpData();
                if (currentPartialHttpData instanceof HttpData) {
                    messages.add(currentPartialHttpData); //3
                }

In 1) you are checking if the FileUpload is completed, and if so, add it to the messages list.
In 2) you are not checking this, while nothing is sure that this Attribute is completed (it can be, as for a file, in the middle due to HttpChunk). Can it be an issue ?
In 3) you are adding a partial HttpData, it can be an Attribute already added in 2 just before. Moreover, if it is a FileUpload, when the FileUpload is over, it will be added another time.

Could those being an issue ?


@fredericBregier That isn't an issue. I believe the check at 1) is actually redundant given only completed items get passed through the iterable of the decoder.

Basically if you put a breakpoint at

```
                postRequestDecoder.offer(httpContent);

                while (postRequestDecoder.hasNext()) {
                    InterfaceHttpData data = postRequestDecoder.next();  //after here
```

You will find that `data` is never set to the file upload
@fredericBregier I've done some debugging and found this to be the issue: 

https://github.com/fredericBregier/netty/blob/improveMultipartAddingBuffer/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java#L1168

The readable bytes in this case was 6, but for some reason it is subtracting the delimiter length. That doesn't make sense to me since the delimiter wasn't found. The 6 bytes should be added to the upload yes?
@fredericBregier I changed this specific section of code back to how it is on the netty repo and my test is green

https://github.com/netty/netty/blob/6724786dcc9fa38ba516dab97a04e2bbc17e81d9/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java#L1161

@jameskleeh Thanks ! I will check and update ;-)
Good catch !
@fredericBregier Sorry I forgot to mention I did change it to `lastLineBreak`
@jameskleeh Could you give me the changes you've made ?

The reason of substracting delimiter size is the following:
- once we don't find out the delimiter, it can means
  - either it is not at all within the buffer
  - either it is partially only present (missing some bytes)
- Therefore we could search for CRLF/LF only from this maximum position (delimiter length from the end)
  - If no CRLF/LF found : full buffer can be added
  - If CRLF/LF is found : only up to this position, the buffer can be added (CRLF/LF can be followed by an incomplete delimiter)

I may have make an over hypothesis, so I've changed from now: in loadDataMultipartOptimized

            // Not found but however perhaps because incomplete so search LF or CRLF from the end within delimiter
            // possible partial last bytes
            int lastPosition = undecodedChunk.readableBytes() - bdelimiter.length;
            if (lastPosition < 0) {
                // Not enough bytes but can still try to find CRLF
                lastPosition = 0; // Change HERE
            }

Indeed, if the lastPosition is < 0, then it means the buffer has less than delimiter length, so we can afford to check for CRLF/LF within from relative position 0.

@jameskleeh With this change, I've got the following:

- delimiter is of size 42
- previous last chunk is of size 24 with half real data, half begining of the delimiter => the half is indeed added
  - decoder.currentPartialHttpData() is not null
- last chunk is of size 30 (last part of the delimiter) => the HttpData is over
  - decoder.currentPartialHttpData() is null

@fredericBregier I'll try with your updated branch now
@jameskleeh  Use the next one ;-) Bad commit
@fredericBregier All tests green on my end. I had to do a couple tweaks to handle no data being added to the upload, however that was probably something that should have been done regardless.
Note that there are 2 other issues we've found during testing, but I haven't gotten to the bottom of them yet and they aren't related to this.
@jameskleeh OK, thanks for your feedback !
I will let the issue open until you 've got the point of the new issues, in order to ensure they are not related to.

Thank you for your help ! It was really helpful!!
@fredericBregier I have the other issues resolved. I appreciate your help getting this resolved. I hope this change can go into 4.1.64
@jameskleeh Great ! We will close this one as soon as the merge is done after review.
Thank you again !
@jameskleeh Current review introduces some changes. When you have time, maybe you can try again to ensure it does not change again the behavior (should not) ?
@fredericBregier I tried with the latest commit and its still good
@jameskleeh Thanks a lot!

Pull request review comments:
Why this change? There are a number of places in the code where we check if `byteBuf` is `null`. Also, when `delete` is called, the `byteBuf` is set to `null`.
The English for these two comments reads weird. How about:

> Note: this method will allocate a lot of memory, if the data is currently stored on the file system.
I'd prefer this method was iterative rather than recursive. Recursion, especially when based on external input, can cause unpredictable stack usage. It's also harder on the JIT.
Since we scan start to end, this method can be implemented in terms of `findLineBreak`, right?
The `else` branch can be unwrapped since the `if` branch above never completes normally.
So… we didn't find a complete delimiter, _but_ if we see a CRLF within a delimiters length of the end, then we _might_ have a delimiter that straddles two read calls i.e. two buffers. And that means we can't be sure that we won't have a delimiter in that byte range, so we can only add the content prior to the CRLF to the HttpData. And then in the next call, the data we didn't read here will be carried over, and get more added to it, and then we'll be able to tell if there's really a delimiter there or not.

Is my understanding correct?
The reason is the different behavior between Memory and Disk. When we allocate a Disk based, the buffer, not here, is like it is there. In Memory, it was not.
Therefore, for those realying on existing buffer, even empty, is not correct (as reporting by the user in his issue). He used the `decoder.currentPartialHttpData()` in his code (legal), but then when he asked for the buffer, he's got "null" instead of an empty buffer when the buffer has not yet data pushed by the decoder.

When the buffer is released, the buffer is set to null after releasing it.
So it does not change the global behavior but makes both implementation seemless the same, which was the main issue.

Note that the change also gives more chance to use the current decoded buffer to fill the `currentHttpData`.
It's the reverse. I would then propose:

     Note: this method will allocate a lot of memory, if the data is not currently stored on the file system.
OK, let me try to break the recursion.
Let me see (double check) if those two methods are indeed the same, just by changing the index value.

I'm not that sure since `fineLineBreak` finds the "first" one, while `findLastLineBreak` finds the "last" one (so the recursion for the moment). And as it is not optimized to make a reverse reading (we cannot use `bytesBefore`)...
So the response is no, the two methods differ (first vs last).
However, since it is only from the end-delimiter.length, we can perhaps afford to use the same (small number of bytes to search in).
? I don't get it.
The previous check is "do we found CRLF/LF?" : no, so all the buffer is OK.
The second check is "do we found CRLF/LF?": yes, so all the buffer up to this index is OK.
If the position is 0: no buffer to add since position is at startup of index

Note: Same error than before here: missing the case where it is 0 but `lastPosition is > 0` (or -1 due to current error on CRLF instead of simple LF) => as the second check.
Yes, exactly. The delimiter might be in the middle of 2 HttpChunks, so we cannot ensure the full buffer is OK.
- We then search only between `end-delimiter.length` and `end` (as it is incomplete, at least 1 byte is missing there, so the LF at least at `end-delimiter` position). This prevents CRLF/LF in first position of the current buffer to be taken as a start of delimiter (while it is probable not). So first we compute this new start position (`lastPosition`).
- if `lastPosition < 0`, the buffer is too small to contains the `delimiter.length`, so we can afford to search within the full buffer (very small), so `lastPosition = 0`
- if `lastPosition >= 0`
  - If there is no CRLF/LF (`<0`), then OK, full buffer is OK
  - If there is a CRLF/LF (`>0`), then up to this position, the buffer is OK
  - If the answear is 0, no part of buffer at all shall be added.


Note I found a possible error (not real one but not optimized): if there is a CRLF (not LF), but the delimiter is almost there (`delimiter.length-1`, just missing 1 byte), then the CR is in -1 position, which is not good. So we should start at `end-delimiter.length-1`.
In `AbstractMemoryHttpData`, the `getByteBuf` method just returns the contained `ByteBuf` instance, so no memory is allocated just from calling that method. Though for the `get` method, it does allocate a new byte array.

For `AbstractDiskHttpData`, both of those methods allocate a byte array.

So the only case that doesn't allocate a lot is `getByteBuf` on `AbstractMemoryHttpData`. So maybe the warning should be unconditional.
Yes right ! I just write the exact opposite of my own logic, and the reason I wrote this warning.
Shall be kept
Done
Not the same (first vs last)
Done
Correction done
I mean that

```
if … {
    return …;
} else if … {
    return …;
}
return …;
```

Can be written as

```
if … {
    return …;
}
if … {
    return …;
}
return …;
```

Intellij likes to point this out.
I was thinking something like this:

```java
    static int findLastLineBreak(ByteBuf buffer, int index) {
        int candidate = findLineBreak(buffer, index);
        int next;
        while (candidate >= 0 && (next = findLineBreak(buffer, index + candidate)) > 0) {
            candidate += next;
        }
        return candidate;
    }
```

Any reason that can't work?
Yes indeed, but almost since we need to recheck if there were CR at first pos, or LF, in order to restart at candudate + 2 or +1.
However simplest than current version, I agree.
I will change and check of course.
@chrisvest  I checked with this version working. Is it ok for you ?

    static int findLastLineBreak(ByteBuf buffer, int index) {
        int candidate = findLineBreak(buffer, index);
        int findCRLF = 0;
        if (candidate >= 0) {
            if (buffer.getByte(index + candidate) == HttpConstants.CR) {
                findCRLF = 2;
            } else {
                findCRLF = 1;
            }
            candidate += findCRLF;
        }
        int next;
        while (candidate > 0 && (next = findLineBreak(buffer, index + candidate)) >= 0) {
            candidate += next;
            if (buffer.getByte(index + candidate) == HttpConstants.CR) {
                findCRLF = 2;
            } else {
                findCRLF = 1;
            }
            candidate += findCRLF;
        }
        return candidate - findCRLF;
    }
Yes, that looks good. Thanks for explaining.
Commited ;-)

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpData.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpData.java
index 14071dad606..dc8dd02f1fa 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpData.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractMemoryHttpData.java
@@ -43,6 +43,7 @@ public abstract class AbstractMemoryHttpData extends AbstractHttpData {
 
     protected AbstractMemoryHttpData(String name, Charset charset, long size) {
         super(name, charset, size);
+        byteBuf = EMPTY_BUFFER;
     }
 
     @Override
@@ -109,6 +110,10 @@ public void addContent(ByteBuf buffer, boolean last)
             } else if (localsize == 0) {
                 // Nothing to add and byteBuf already exists
                 buffer.release();
+            } else if (byteBuf.readableBytes() == 0) {
+                // Previous buffer is empty, so just replace it
+                byteBuf.release();
+                byteBuf = buffer;
             } else if (byteBuf instanceof CompositeByteBuf) {
                 CompositeByteBuf cbb = (CompositeByteBuf) byteBuf;
                 cbb.addComponent(true, buffer);
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactory.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactory.java
index 3f730c9e777..254ff2f964b 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactory.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactory.java
@@ -19,7 +19,6 @@
 import io.netty.handler.codec.http.HttpConstants;
 import io.netty.handler.codec.http.HttpRequest;
 
-import java.io.File;
 import java.io.IOException;
 import java.nio.charset.Charset;
 import java.util.ArrayList;
@@ -38,6 +37,15 @@
  * <li>MemoryAttribute, DiskAttribute or MixedAttribute</li>
  * <li>MemoryFileUpload, DiskFileUpload or MixedFileUpload</li>
  * </ul>
+ * A good example of releasing HttpData once all work is done is as follow:<br>
+ * <pre>{@code
+ *   for (InterfaceHttpData httpData: decoder.getBodyHttpDatas()) {
+ *     httpData.release();
+ *     factory.removeHttpDataFromClean(request, httpData);
+ *   }
+ *   factory.cleanAllHttpData();
+ *   decoder.destroy();
+ *  }</pre>
  */
 public class DefaultHttpDataFactory implements HttpDataFactory {
 
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpData.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpData.java
index 2e464225be1..266e566523a 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpData.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpData.java
@@ -121,7 +121,8 @@ public interface HttpData extends InterfaceHttpData, ByteBufHolder {
     void delete();
 
     /**
-     * Returns the contents of the file item as an array of bytes.
+     * Returns the contents of the file item as an array of bytes.<br>
+     * Note: this method will allocate a lot of memory, if the data is currently stored on the file system.
      *
      * @return the contents of the file item as an array of bytes.
      * @throws IOException
@@ -129,7 +130,8 @@ public interface HttpData extends InterfaceHttpData, ByteBufHolder {
     byte[] get() throws IOException;
 
     /**
-     * Returns the content of the file item as a ByteBuf
+     * Returns the content of the file item as a ByteBuf.<br>
+     * Note: this method will allocate a lot of memory, if the data is currently stored on the file system.
      *
      * @return the content of the file item as a ByteBuf
      * @throws IOException
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostBodyUtil.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostBodyUtil.java
index 13f75f55bf2..f174327d912 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostBodyUtil.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostBodyUtil.java
@@ -152,7 +152,7 @@ static int findEndOfString(String sb) {
     }
 
     /**
-     * Try to find LF or CRLF as Line Breaking
+     * Try to find first LF or CRLF as Line Breaking
      *
      * @param buffer the buffer to search in
      * @param index the index to start from in the buffer
@@ -164,7 +164,7 @@ static int findLineBreak(ByteBuf buffer, int index) {
         int posFirstChar = buffer.bytesBefore(index, toRead, HttpConstants.LF);
         if (posFirstChar == -1) {
             // No LF, so neither CRLF
-            return  -1;
+            return -1;
         }
         if (posFirstChar > 0 && buffer.getByte(index + posFirstChar - 1) == HttpConstants.CR) {
             posFirstChar--;
@@ -172,6 +172,38 @@ static int findLineBreak(ByteBuf buffer, int index) {
         return posFirstChar;
     }
 
+    /**
+     * Try to find last LF or CRLF as Line Breaking
+     *
+     * @param buffer the buffer to search in
+     * @param index the index to start from in the buffer
+     * @return a relative position from index > 0 if LF or CRLF is found
+     *         or < 0 if not found
+     */
+    static int findLastLineBreak(ByteBuf buffer, int index) {
+        int candidate = findLineBreak(buffer, index);
+        int findCRLF = 0;
+        if (candidate >= 0) {
+            if (buffer.getByte(index + candidate) == HttpConstants.CR) {
+                findCRLF = 2;
+            } else {
+                findCRLF = 1;
+            }
+            candidate += findCRLF;
+        }
+        int next;
+        while (candidate > 0 && (next = findLineBreak(buffer, index + candidate)) >= 0) {
+            candidate += next;
+            if (buffer.getByte(index + candidate) == HttpConstants.CR) {
+                findCRLF = 2;
+            } else {
+                findCRLF = 1;
+            }
+            candidate += findCRLF;
+        }
+        return candidate - findCRLF;
+    }
+
     /**
      * Try to find the delimiter, with LF or CRLF in front of it (added as delimiters) if needed
      *
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java
index ebf88f23360..52229b8297f 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java
@@ -41,6 +41,7 @@
 import java.util.Map;
 import java.util.TreeMap;
 
+import static io.netty.buffer.Unpooled.EMPTY_BUFFER;
 import static io.netty.util.internal.ObjectUtil.*;
 
 /**
@@ -1159,8 +1160,16 @@ private static boolean loadDataMultipartOptimized(ByteBuf undecodedChunk, String
         final byte[] bdelimiter = delimiter.getBytes(httpData.getCharset());
         int posDelimiter = HttpPostBodyUtil.findDelimiter(undecodedChunk, startReaderIndex, bdelimiter, true);
         if (posDelimiter < 0) {
-            // Not found but however perhaps because incomplete so search LF or CRLF
-            posDelimiter = HttpPostBodyUtil.findLineBreak(undecodedChunk, startReaderIndex);
+            // Not found but however perhaps because incomplete so search LF or CRLF from the end.
+            // Possible last bytes contain partially delimiter
+            // (delimiter is possibly partially there, at least 1 missing byte),
+            // therefore searching last delimiter.length +1 (+1 for CRLF instead of LF)
+            int lastPosition = undecodedChunk.readableBytes() - bdelimiter.length - 1;
+            if (lastPosition < 0) {
+                // Not enough bytes, but at most delimiter.length bytes available so can still try to find CRLF there
+                lastPosition = 0;
+            }
+            posDelimiter = HttpPostBodyUtil.findLastLineBreak(undecodedChunk, startReaderIndex  + lastPosition);
             if (posDelimiter < 0) {
                 // not found so this chunk can be fully added
                 ByteBuf content = undecodedChunk.copy();
@@ -1172,18 +1181,21 @@ private static boolean loadDataMultipartOptimized(ByteBuf undecodedChunk, String
                 undecodedChunk.readerIndex(startReaderIndex);
                 undecodedChunk.writerIndex(startReaderIndex);
                 return false;
-            } else if (posDelimiter > 0) {
-                // Not fully but still some bytes to provide: httpData is not yet finished since delimiter not found
-                ByteBuf content = undecodedChunk.copy(startReaderIndex, posDelimiter);
-                try {
-                    httpData.addContent(content, false);
-                } catch (IOException e) {
-                    throw new ErrorDataDecoderException(e);
-                }
-                rewriteCurrentBuffer(undecodedChunk, posDelimiter);
+            }
+            // posDelimiter is not from startReaderIndex but from startReaderIndex + lastPosition
+            posDelimiter += lastPosition;
+            if (posDelimiter == 0) {
+                // Nothing to add
                 return false;
             }
-            // Empty chunk or so
+            // Not fully but still some bytes to provide: httpData is not yet finished since delimiter not found
+            ByteBuf content = undecodedChunk.copy(startReaderIndex, posDelimiter);
+            try {
+                httpData.addContent(content, false);
+            } catch (IOException e) {
+                throw new ErrorDataDecoderException(e);
+            }
+            rewriteCurrentBuffer(undecodedChunk, posDelimiter);
             return false;
         }
         // Delimiter found at posDelimiter, including LF or CRLF, so httpData has its last chunk


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java
index 93991c375a5..40f2f400a4e 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java
@@ -25,6 +25,7 @@
 import io.netty.handler.codec.http.DefaultHttpRequest;
 import io.netty.handler.codec.http.DefaultLastHttpContent;
 import io.netty.handler.codec.http.FullHttpRequest;
+import io.netty.handler.codec.http.HttpConstants;
 import io.netty.handler.codec.http.HttpHeaderNames;
 import io.netty.handler.codec.http.HttpHeaderValues;
 import io.netty.handler.codec.http.HttpMethod;
@@ -1003,27 +1004,44 @@ private void commonTestBigFileDelimiterInMiddleChunk(HttpDataFactory factory, bo
 
         HttpPostMultipartRequestDecoder decoder = new HttpPostMultipartRequestDecoder(factory, request);
         decoder.offer(new DefaultHttpContent(Unpooled.wrappedBuffer(prefix.getBytes(CharsetUtil.UTF_8))));
+        assertNotNull(((HttpData) decoder.currentPartialHttpData()).content());
 
         byte[] body = new byte[bytesPerChunk];
         Arrays.fill(body, (byte) 1);
+        // Set first bytes as CRLF to ensure it is correctly getting the last CRLF
+        body[0] = HttpConstants.CR;
+        body[1] = HttpConstants.LF;
         for (int i = 0; i < nbChunks; i++) {
             ByteBuf content = Unpooled.wrappedBuffer(body, 0, bytesPerChunk);
-            decoder.offer(new DefaultHttpContent(content)); // **OutOfMemory here**
+            decoder.offer(new DefaultHttpContent(content)); // **OutOfMemory previously here**
+            assertNotNull(((HttpData) decoder.currentPartialHttpData()).content());
             content.release();
         }
 
         byte[] bsuffix1 = suffix1.getBytes(CharsetUtil.UTF_8);
-        byte[] lastbody = new byte[bytesLastChunk + bsuffix1.length];
-        Arrays.fill(body, (byte) 1);
+        byte[] previousLastbody = new byte[bytesLastChunk - bsuffix1.length];
+        byte[] lastbody = new byte[2 * bsuffix1.length];
+        Arrays.fill(previousLastbody, (byte) 1);
+        previousLastbody[0] = HttpConstants.CR;
+        previousLastbody[1] = HttpConstants.LF;
+        Arrays.fill(lastbody, (byte) 1);
+        lastbody[0] = HttpConstants.CR;
+        lastbody[1] = HttpConstants.LF;
         for (int i = 0; i < bsuffix1.length; i++) {
-            lastbody[bytesLastChunk + i] = bsuffix1[i];
+            lastbody[bsuffix1.length + i] = bsuffix1[i];
         }
 
-        ByteBuf content2 = Unpooled.wrappedBuffer(lastbody, 0, lastbody.length);
+        ByteBuf content2 = Unpooled.wrappedBuffer(previousLastbody, 0, previousLastbody.length);
+        decoder.offer(new DefaultHttpContent(content2));
+        assertNotNull(((HttpData) decoder.currentPartialHttpData()).content());
+        content2.release();
+        content2 = Unpooled.wrappedBuffer(lastbody, 0, lastbody.length);
         decoder.offer(new DefaultHttpContent(content2));
+        assertNotNull(((HttpData) decoder.currentPartialHttpData()).content());
         content2.release();
         content2 = Unpooled.wrappedBuffer(suffix2.getBytes(CharsetUtil.UTF_8));
         decoder.offer(new DefaultHttpContent(content2));
+        assertNull(decoder.currentPartialHttpData());
         content2.release();
         decoder.offer(new DefaultLastHttpContent());