Repository navigation
Fix header table size assignment - #209
pointlessone wants to merge 1 commit into
Conversation
|
Thx for the PR @pointlessone This proposes a change which reverts this very old commit. I did it back then because of an integration issue with a ruby server, where the browser page wouldn't render. I think I'll need an example in the wild of a peer server which actually fails to process a request. Either that, or references to other HTTP/2 parsers and how they do it. |
|
Now that you mention that fix this turns out to be incomplete. The order in the PR (and before the linked commit) is the correct order. I was proposing this PR as a fix because in my case I user http-2 as the client (and the commit tried to fix issue from the server side). That commit only masked the issue. The real bug is that table size change is not signalled to the counterpart. Do not merge this yet. I'll have to look into that a bit closer. |
SETTINGS_HEADER_TABLE_SIZE sets the maximum size of the table used by the encoder on the other side. Our acknowledged setting limits the peer's encoder, which is our decompressor. The peer's setting limits our encoder, which is our compressor. 3b51a2e swapped the two. That commit fixed igrigorik#97, where Chrome sent SETTINGS_HEADER_TABLE_SIZE=65536 and then failed to decode our headers with COMPRESSION_ERROR. The swap removed that error, but the assignment had been correct before it. The actual cause was that the compressor grew its table to 65536 without sending a dynamic table size update. Chrome's decoder kept its table at 4096, so our encoder referred to entries the decoder had already evicted. With the swap, the compressor stayed at 4096 and the error went away. But a peer that advertised a smaller table (for example 0) could no longer decode our headers, and our decoder evicted entries the peer still referred to. Restore the correct assignment. Change table sizes the way RFC 7541 section 4.2 requires: - SETTINGS_HEADER_TABLE_SIZE only sets the maximum table size. The table changes with a dynamic table size update at the beginning of a header block. - The compressor uses its own table size, capped at the peer's maximum. When the size has to change, the next header block starts with a size update. If the maximum was reduced and then increased between header blocks, it sends the smallest maximum first and then the final size. - The decompressor rejects size updates above the maximum from our acknowledged setting. Before, it compared against the initial settings. When the maximum is reduced below the current table size, the next header block has to start with a size update to the smallest maximum or lower.
3abab39 to
fa18ed9
Compare
|
I updated the PR. As I said before the order of compressor/decompressor is correct in this PR (and was before the linked commit). The issue is that table size updates are not correctly signalled to the peer. This is a bit tricky part of the protocol. The peer needs to know about the biggest reduction and the final table size in order to correctly evict table entries. So we need to track two numbers: the smallest table size, and the final table size before the next header block. Table size also can be updated only between header blocks so tracking is we started encoding headers or not adds another layer of complexity. The previous fix worked by accident. Since we didn't acknowledge table size changes to the peer we appear as if we're at the default. Here it worked because we set decoder table size to what came from the peer. It works if peer want to increase the table, which was the case in #97: Chrome tries to set table size to 64k, we increase decompressor table size but do not acknowledge it so Chrome still uses the default 4k. From our point of view Chrome would just happen to not use all table when encoding. The bug I hit that prompted this PR is from the other side. I use http-2 as a client and I want to increase table size to 64k. Current main would set compressor table size to 64k but not signal it to the peer, they would use 4k table by default and wouldn't be able to decode a bigger table because they would evict entries needed for decoding. In a way, it's a reverse of #97: I'm Chrome trying to send large table and peer can not decode it because they use smaller table. |
HoneyryderChuck
left a comment
HoneyryderChuck
left a comment
There was a problem hiding this comment.
@pointlessone I think this is going in the right direction. I'm still rereading that section of the RFC, but the overall flow seems correct. Left some suggestions.
| # Set the maximum dynamic table +size+ allowed by the peer's | ||
| # SETTINGS_HEADER_TABLE_SIZE. The next header block starts with a | ||
| # dynamic table size update when the table size has to change. | ||
| def table_size=(size) |
There was a problem hiding this comment.
I think that this method should be rename to max_table_size, wdyt?
| # Set the maximum dynamic table +size+ allowed by our acknowledged | ||
| # SETTINGS_HEADER_TABLE_SIZE. When the current table is larger, the | ||
| # next header block has to start with a dynamic table size update. | ||
| def table_size=(size) |
| @limit = settings.table_size | ||
| # Maximum dynamic table size allowed by SETTINGS_HEADER_TABLE_SIZE. | ||
| @max_limit = @limit | ||
| # Smallest maximum size set since the last dynamic table size update. | ||
| @lowest_max_limit = @limit |
There was a problem hiding this comment.
| @limit = settings.table_size | |
| # Maximum dynamic table size allowed by SETTINGS_HEADER_TABLE_SIZE. | |
| @max_limit = @limit | |
| # Smallest maximum size set since the last dynamic table size update. | |
| @lowest_max_limit = @limit | |
| @limit = | |
| # Maximum dynamic table size allowed by SETTINGS_HEADER_TABLE_SIZE. | |
| @max_limit = | |
| # Smallest maximum size set since the last dynamic table size update. | |
| @lowest_max_limit = settings.table_size |
(reads slightly better)
|
|
||
| # The maximum size was reduced below the current table size and the | ||
| # encoder has not signalled a size that fits into it. | ||
| raise CompressionError, "dynamic table size update required" if type != :changetablesize && @lowest_max_limit < @limit |
There was a problem hiding this comment.
this clause can be moved below L163
|
|
||
| if @lowest_max_limit < @limit | ||
| self.table_size = @lowest_max_limit | ||
| yield({ type: :changetablesize, value: @limit }) |
There was a problem hiding this comment.
| yield({ type: :changetablesize, value: @limit }) | |
| yield({ type: :changetablesize, value: @lowest_max_limit }) |
I know that ultimately it's the same, but it is easier to read.
| end | ||
| @lowest_max_limit = @max_limit | ||
|
|
||
| size = @settings.table_size |
There was a problem hiding this comment.
💅 renaming var to max_size or max_table_size would help reading this.
| @lowest_max_limit = @max_limit | ||
|
|
||
| size = @settings.table_size | ||
| size = @max_limit if @max_limit < size |
There was a problem hiding this comment.
| @lowest_max_limit = @max_limit | |
| size = @settings.table_size | |
| size = @max_limit if @max_limit < size | |
| max_table_size = @lowest_max_limit = @max_limit | |
| max_table_size = @settings.table_size if @settings.table_size > max_table_size |
| # - https://www.rfc-editor.org/rfc/rfc7541#section-4.2 | ||
| def max_table_size=(size) | ||
| @max_limit = size | ||
| @lowest_max_limit = size if size < @lowest_max_limit |
There was a problem hiding this comment.
I think there should be a boolean variable set to true here, i.e. @max_table_size_updated = true. This variable should then be used to skip the branch you added into #encode (which should perhaps be moved to a private method), and the same goes for the initial clause you added under #process (... if type != :changetablesize && @max_table_size_updated).
|
thx again, cherry picked in 0d16d2a |
|
Thank you for merging and sorry for the delay. |
2 participants
Wrong connection side's header table size was set.