HttpPostMultipartRequestDecoder may not add content to an existing upload after being offered data

[jameskleeh]

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 #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.

[fredericBregier]

@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.

[jameskleeh]

@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.

[jameskleeh]

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?

[fredericBregier]

@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.

[fredericBregier]

The idea would be to change:

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

Line 1163 in 1529ef1

posDelimiter = HttpPostBodyUtil.findLineBreak(undecodedChunk, startReaderIndex);

  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.

[jameskleeh]

@fredericBregier I'd be happy to test any local branch with the changes

[fredericBregier]

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.

[jameskleeh]

@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

[jameskleeh]

If you'd like to do a screen share I'd be glad to show you

[fredericBregier]

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 ;-)

[jameskleeh]

@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

[fredericBregier]

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.

[jameskleeh]

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

[jameskleeh]

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.

[fredericBregier]

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).

[jameskleeh]

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?

[fredericBregier]

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.

[fredericBregier]

@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.

[fredericBregier]

@jameskleeh Could you try https://github.com/fredericBregier/netty/tree/improveMultipartAddingBuffer ?

[jameskleeh]

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 <#11143 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAMCVLIOCP4ZVP3MLRIIBS3THWKVVANCNFSM42PB7GYA> .

[jameskleeh]

@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.

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.

[fredericBregier]

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]

@fredericBregier Another step in the right direction, however I'm now finding that the upload never completes.

[fredericBregier]

@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.

[fredericBregier]

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 ?

[fredericBregier]

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 ?

[jameskleeh]

@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

[jameskleeh]

@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?

[jameskleeh]

@fredericBregier I changed this specific section of code back to how it is on the netty repo and my test is green

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

Line 1161 in 6724786

if (posDelimiter < 0) {

[fredericBregier]

@jameskleeh Thanks ! I will check and update ;-)
Good catch !

[jameskleeh]

@fredericBregier Sorry I forgot to mention I did change it to lastLineBreak

[fredericBregier]

@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.

[fredericBregier]

@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

[jameskleeh]

@fredericBregier I'll try with your updated branch now

[fredericBregier]

@jameskleeh Use the next one ;-) Bad commit

[jameskleeh]

@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.

[jameskleeh]

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.

[fredericBregier]

@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!!

[jameskleeh]

@fredericBregier I have the other issues resolved. I appreciate your help getting this resolved. I hope this change can go into 4.1.64

[fredericBregier]

@jameskleeh Great ! We will close this one as soon as the merge is done after review.
Thank you again !

[fredericBregier]

@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) ?

[jameskleeh]

@fredericBregier I tried with the latest commit and its still good

[fredericBregier]

@jameskleeh Thanks a lot!