HttpObjectDecoder ignores HTTP trailer header when empty line is rece…#8799
HttpObjectDecoder ignores HTTP trailer header when empty line is rece…#8799normanmaurer merged 3 commits into4.1from
Conversation
|
@KowalczykBartek PTAL and thanks for reporting |
…ived in seperate ByteBuf Motivation: When the empty line that termines the trailers was sent in a seperate ByteBuf we did ignore the previous parsed trailers and just returned none. Modifications: - Correct respect previous parsed trailers. - Add unit test. Result: Fixes #8736
7f702b2 to
322a4ba
Compare
| this.trailer = null; | ||
| return trailer; | ||
| } | ||
| // We have received the empty line which signals the trailer is complete. In this case we need to check if |
There was a problem hiding this comment.
This flow is a bit convoluted. How about we remove the surrounding if and make it clear why the condition is there:
if (line.length() == 0 && this.trailer == null) {
return LastHttpContent.EMPTY_LAST_CONTENT; // optimization
}
// everything from the if today
LastHttpContent trailer = this.trailer;
if (trailer == null) {
...
return trailer;
Alternatively the if could be changed to line.length() > 0 || this.trailer != null, but it's non-obvious why it would be that way. (It's still more obvious than this code, in my mind, though.)
|
@ejona86 PTAL again... I think it's better now (slightly different than what you suggested tho). |
|
My suggestion removed a special-case: it used the normal do-while to handle the "previously saw a trailer". Instead of having 2 special cases like it is now, there was only one: returning EMPTY_LAST_CONTENT. It is makes sense that we should only return EMPTY_LAST_CONTENT if With this code as-is, you really have to read more of the do-while loop to verify that the if block is correct. You have to look for things like making sure |
|
@ejona86 I am still not sure if I did what you suggested but please have a look again 😆 |
ejona86
left a comment
There was a problem hiding this comment.
Thanks, yeah, that looks good.
#8799) * HttpObjectDecoder ignores HTTP trailer header when empty line is received in seperate ByteBuf Motivation: When the empty line that termines the trailers was sent in a seperate ByteBuf we did ignore the previous parsed trailers and just returned none. Modifications: - Correct respect previous parsed trailers. - Add unit test. Result: Fixes #8736
Compared against 4.1.25.6.dse, this tag cherry-picks upstream commits that fixed bugs in HttpObjectDecoder/HttpRequestDecoder, plus two intermediate refactoring commits that indirectly affect those bugfix commits. What follows is a list of PR links, issue links, CVE links, and hashes associated with the cherry-picked commits. Verify we do not receive multiple content-length headers or a content-length and transfer-encoding: chunked header when using HTTP/1.1 (netty#9865) https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-7238 netty#9861 netty#9865 8494b04 Detect missing colon when parsing http headers with no value (netty#9871) GHSA-cqqj-4p63-rrmm netty#9866 netty#9871 a7c18d4 Fix typos in javadocs (netty#9527) skipped Correctly handle whitespaces in HTTP header names as defined by RFC7230#section-3.2.4 (netty#9585) https://nvd.nist.gov/vuln/detail/CVE-2019-16869 netty#9571 netty#9585 39cafcb Use `AppendableCharSequence.charAtUnsafe(int)` in `HttpObjectDecoder` (netty#9492) netty#9492 85fcf4e use checkPositive/checkPositiveOrZero (netty#8835) netty#8835 4c64c98 HttpObjectDecoder ignores HTTP trailer header when empty line is rece… (netty#8799) netty#8736 netty#8799 91d3920
…ived in seperate ByteBuf
Motivation:
When the empty line that termines the trailers was sent in a seperate ByteBuf we did ignore the previous parsed trailers and just returned none.
Modifications:
Result:
Fixes #8736