summerwind / h2spec

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

Test case HPACK/4.2 is wrong #78

Open OctarineDragon opened 7 years ago

OctarineDragon commented 7 years ago

According to the RFC 7541, section 4.2:

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]) As I understand this, when HTTP/2 sends a SETTINGS frame with a SETTINGS_HEADER_TABLE_SIZE change, only then it is mandatory (MUST) for the headers block to start with the dynamic table size update (notice the word "this" I highlighted in the quote). All the other dynamic table size updates can be in any location in the headers block.

tatsuhiro-t commented 7 years ago

I admit that it can be read in that way. It is better to ask this in httpbis mailing list.

tatsuhiro-t commented 7 years ago

OTOH, "This" refers to "A change in the maximum size of the dynamic table is signaled via a dynamic table size update", which is HPACK's dynamic table size update, and not HTTP/2 SETTINGS. So I can read it in the way that dynamic table size update must appear at the beginning of the header block.

OctarineDragon commented 7 years ago

But this whole section (4.2) is dedicated for the maximum table size that is generated from the protocol that uses HPACK (i.e. HTTP/2). Furthermore, writing "This dynamic table size update" makes it sound like we are dealing with a special case of update, not the usual case. Also the last sentence of the quoted paragraph explicitly states that this behavior follows HTTP/2's settings acknowledgement. If this behavior was expected in every size update then it would have been written in somewhere else, not in the maximum table size section.

MikeBishop commented 7 years ago

The decoder specifies a greatest-acceptable value for a maximum table size. The encoder specifies the actual max table size, which may be the same or smaller. You're correct that the section is unclear in a few ways:

Might be worth an erratum.

OctarineDragon commented 7 years ago

I'm pretty new to this. Does this require me to report here about new errata? Looking at the report form a little bit more and it seems that it requires a decision of what is the correction from the submitter, which I don't have. So what is the proper action to take now?

ThetaSinner commented 6 years ago

Hello, what's the status of this? I've just run into this test case while running h2spec against my server and would like progress this.

My understanding is that the flow (from the server's perspective) should be

What's important here is the synchronisation point. As soon as the client receives the settings acknowledgment it is safe to reduce their dynamic table size, because they know that the server will update the table size to the value set by the client or lower.

The point of having two settings, one in http2 and one in hpack, is that each endpoint should be able to control the resources they want to dedicate to the dynamic table. So you can restrict what space the remote encore may use, and in return you don't use more space than the remote decoder allows.

This should give some idea of what the test is supposed to do, at least in the case of reducing the setting. In the case of increasing the setting, it is unclear whether the server MUST send a size update. For the sake of consistency though, it is probably better that and update is always sent following a settings change.

It is also important to note that the encoder/decoder pairs are COMPLETELY independent. So the test certainly cannot expect the server to send a size update on it's encoder when the client sends a size update on its encoder.

P.S. Finding this tool extremely useful, and finding lots of bugs in my server because of it :)