Skip to content
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

Should sending a GOAWAY frame on header decompression error be valid? #60

Closed
carllerche opened this issue Oct 13, 2016 · 7 comments
Closed

Comments

@carllerche
Copy link

My current HTTP/2 server implementation sends a GOAWAY frame before terminating the connection when it hits an hpack decompression error. My read of the http2 spec is that this is valid, however this test fails:

https://github.com/summerwind/h2spec/blob/master/4_3.go#L26-L47

The test passes if I don't send the GOAWAY frame before terminating the connection. Is this expected?

@summerwind
Copy link
Owner

Your understanding of the spec is correct. However, this test is expected that the stream is closed properly, by sending two "Dynamic Table Size Update". Does your implementation treats "Dynamic Table Size Update" of HPACK as decompression error?

@carllerche
Copy link
Author

I believe so. I receive the two hpack dynamic size updates, detect the decompression error, send a GOAWAY w/ COMPRESSION_ERROR set as the reason, then terminate the connection.

I believe that the issue is the test uses TestStreamClose (https://github.com/summerwind/h2spec/blob/master/4_3.go#L45) and in that function implementation, the test fails if it receives GOAWAY: https://github.com/summerwind/h2spec/blob/master/h2spec.go#L800

If I removed the GOAWAY frame before closing the connection, the test passed.

@summerwind
Copy link
Owner

This test is verifying that your implementation can handle correctly the dynamic table size update. If your implementation respond with GOAWAY, this test will be failed.

Why does your implementation treats the dynamic table size update as a decompression error?

@carllerche
Copy link
Author

Ok, I understand the question.

The test sends two dynamic table size updates (0 and 4096) at the beginning of a single header block. The HPack spec contains the following:

A change in the maximum size of the dynamic table is signaled via a dynamic table size update (see Section 6.3). This dynamic table size update MUST occur at the beginning of the first header block following the change to the dynamic table size. In HTTP/2, this follows a settings acknowledgment (see Section 6.5.3 of [HTTP2]).

My interpretation of this is that a single header frame can contain at most one dynamic table update and it must be at the beginning of the frame.

I also see this:

This mechanism can be used to completely clear entries from the dynamic table by setting a maximum size of 0, which can subsequently be restored.

Again, my interpretation of this is that these dynamic updates must be in separate header frames.

I guess this is an incorrect interpretation?

@carllerche
Copy link
Author

In paragraph:

Multiple updates to the maximum table size can occur between the transmission of two header blocks. In the case that this size is changed more than once in this interval, the smallest maximum table size that occurs in that interval MUST be signaled in a dynamic table size update. The final maximum size is always signaled, resulting in at most two dynamic table size updates. This ensures that the decoder is able to perform eviction based on reductions in dynamic table size (see Section 4.3).

I was reading "Multiple updates to the maximum table size" as receiving HTTP/2 settings frames w/ the max table size setting set, not multiple dynamic updates in the header block.

@tatsuhiro-t
Copy link
Contributor

the smallest maximum table size that occurs in that interval MUST be signaled in a dynamic table size update. The final maximum size is always signaled, resulting in at most two dynamic table size updates.

I think this states that 2 dynamic table size updates can occur at the front of encoded header block. If you still think it shouldn't, please ask this on IETF httpbis mailing list.

@carllerche
Copy link
Author

Ok, thanks. I believe this is a misread on my part. Sorry for the trouble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants