New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update HTTP/2 HPACK Decoder to check for oversized headers after header processing #6273

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

Projects
None yet
3 participants
@exell-christopher

exell-christopher commented Jan 24, 2017

Currently the HPACK Decoder checks to see if the maxHeaderListSize has been exceeded while it is still in the decode loop. This means that we can abort early, and leave the decoder side table corrupted.

Modification:

Moved the maxHeaderListSize limit check to after the decoder processing loop, so that we abort only when the maxHeaderListSizeGoAway limit is exceeded.

Added a test for this behavior.

@exell-christopher

This comment has been minimized.

Show comment
Hide comment
@exell-christopher

exell-christopher Jan 24, 2017

@Scottmitch Found this in local testing. I should have caught this when I was looking at your commit. Sorry :(

exell-christopher commented Jan 24, 2017

@Scottmitch Found this in local testing. I should have caught this when I was looking at your commit. Sorry :(

@@ -419,4 +423,35 @@ public void testLiteralNeverIndexedWithLargeValue() throws Http2Exception {
}
decode(sb.toString());
}
@Test
public void testDecodeLargerThanMaxHeaderListSizeButSmallerThanMaxHeaderListSizeUpdatesDynamicTable()

This comment has been minimized.

@normanmaurer

normanmaurer Jan 24, 2017

Member

Long method names FTW 🎉

@normanmaurer

normanmaurer Jan 24, 2017

Member

Long method names FTW 🎉

This comment has been minimized.

@exell-christopher

exell-christopher Jan 24, 2017

I was going for descriptive over short. Happy to rename :)

@exell-christopher

exell-christopher Jan 24, 2017

I was going for descriptive over short. Happy to rename :)

This comment has been minimized.

@normanmaurer

normanmaurer Jan 24, 2017

Member

nah... its ok. just looks funny 👍

@normanmaurer

normanmaurer Jan 24, 2017

Member

nah... its ok. just looks funny 👍

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 24, 2017

Member

@Scottmitch assigned to you 👍

Member

normanmaurer commented Jan 24, 2017

@Scottmitch assigned to you 👍

Show outdated Hide outdated ...c/test/java/io/netty/handler/codec/http2/internal/hpack/DecoderTest.java
@@ -295,10 +295,13 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers) throws Http2E
default:
throw new Error("should not reach here state: " + state);
}
}

This comment has been minimized.

@Scottmitch

Scottmitch Jan 24, 2017

Member

doh ... good catch. Double fail bcz I forgot to add a unit test for the decoder side of the house ... unit test focused on the encoder 2fd42cf#diff-928e2c1c5d3a80187c2936e9a4bfced7R217

@Scottmitch

Scottmitch Jan 24, 2017

Member

doh ... good catch. Double fail bcz I forgot to add a unit test for the decoder side of the house ... unit test focused on the encoder 2fd42cf#diff-928e2c1c5d3a80187c2936e9a4bfced7R217

Show outdated Hide outdated ...c/test/java/io/netty/handler/codec/http2/internal/hpack/DecoderTest.java
Show outdated Hide outdated ...c/test/java/io/netty/handler/codec/http2/internal/hpack/DecoderTest.java
@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jan 24, 2017

Member

thanks @exell-christopher ... once comments are resolved I will pull this in.

Member

Scottmitch commented Jan 24, 2017

thanks @exell-christopher ... once comments are resolved I will pull this in.

Christopher Exell
Move location of where oversized headers that don't exceed the go awa…
…y limit is done

so that the check occurs after all headers have been read
@exell-christopher

This comment has been minimized.

Show comment
Hide comment
@exell-christopher

exell-christopher Jan 24, 2017

I believe I've resolved all the requested changes. Let me know if there's anything else that needs to change.

exell-christopher commented Jan 24, 2017

I believe I've resolved all the requested changes. Let me know if there's anything else that needs to change.

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jan 24, 2017

Member

4.1 (d509365)

thanks again @exell-christopher!

Member

Scottmitch commented Jan 24, 2017

4.1 (d509365)

thanks again @exell-christopher!

@Scottmitch Scottmitch closed this Jan 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment