search

summerwind / h2spec

A conformance testing tool for HTTP/2 implementation.
MIT License
661 stars 73 forks source link

frame size test should be more permissive #15

Closed shane-kearns closed 9 years ago

shane-kearns commented 9 years ago

4.2. Frame Size × Sends large size frame that exceeds the SETTINGS_MAX_FRAME_SIZE

25 313.167882115 10.0.0.1 10.0.0.2 HTTP2 1982 DATA Stream: DATA, Stream ID: 1, Length 16385 27 313.168127324 10.0.0.2 10.0.0.1 HTTP2 81
RST_STREAM Error: FRAME_SIZE_ERROR (6)

"A frame size error in a frame that could alter the state of the entire connection MUST be treated as a connection error"

However DATA frames do not affect the whole connection so either a stream error (RST_STREAM) or connection error (GOAWAY) are valid responses. It's even allowed to close a connection abruptly without sending a GOAWAY "An endpoint might choose to close a connection without sending GOAWAY for misbehaving peers.", but I'm not sure how a test framework would usefully report that. Perhaps a ? (inconclusive) result where an error was expected but the connection was closed.

I do plan to use GOAWAY because a bad sized frame could be very big and probably indicates corruption of the connection.

summerwind commented 9 years ago

Thanks for your feedback. I agree with you. I'm going to permit that the peer choose to close the connection in the next version. Patches are welcome of course ;-)

summerwind commented 9 years ago

I have released h2spec v0.0.5. This version treats as the connection error when peer choose to close the connection. Can you try to test again?

shane-kearns commented 9 years ago

I modified my code to drop connections without sending a GOAWAY frame in response to connection errors. With this in place, I get the following test result:

4.2. Frame Size × Sends large size frame that exceeds the SETTINGS_MAX_FRAME_SIZE

I like the new behaviour of showing the expected and actual test results very much, it avoids needing to use wireshark a lot of the time.

Closing the socket with a FIN packet produces a test pass: 4.2. Frame Size ✓ Sends large size frame that exceeds the SETTINGS_MAX_FRAME_SIZE

However, if dropping a connection without sending GOAWAY is a response to misbehaving peers, then an immediate disconnect is more likely. The oversized frame can be detected by just reading the 9 byte header.

shane-kearns commented 9 years ago

For the old version that used stream errors for badly sized DATA frames: 4.2. Frame Size × Sends large size frame that exceeds the SETTINGS_MAX_FRAME_SIZE

summerwind commented 9 years ago

@shane-kearns Thank you for your feedback. I fixed again to handle the ECONNRESET as the connection closed. Landed in aabf5d1fb9b329f99c1add024b1110128bf5b4f8

shane-kearns commented 9 years ago

Thanks - this works for me. I've made a pull request for the other issue about RST_STREAM.

summerwind commented 9 years ago

I have merged your another PR and close this issue. Thank you for your support!