Deny shrink of dynamic table to zero size

swamp-agr commented 5 years ago

As it was pointed out in #16, Pull request created.

kazu-yamamoto commented 5 years ago

@lucasdicioccio May I ask you to review this PR?

lucasdicioccio commented 5 years ago

@kazu-yamamoto surely ! i'm a little swamped as well and I do not know the HPACK RFC well yet.

I'd say the MR is a good workaround but we should have a proper fix before cutting a release to Hackage.

swamp-agr commented 5 years ago

Hi @kazu-yamamoto, @lucasdicioccio,

Let's move on! According to HPACK RFC:

4.2 Maximum Table Size:

...

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.

It is possible to reduce maximum table size (of dynamic table) to 0. When?

6.3.  Dynamic Table Size Update

     0   1   2   3   4   5   6   7
   +---+---+---+---+---+---+---+---+
   | 0 | 0 | 1 |   Max size (5+)   |
   +---+---------------------------+

               Figure 12: Maximum Dynamic Table Size Change

   A dynamic table size update starts with the '001' 3-bit pattern,
   followed by the new maximum size, represented as an integer with a
   5-bit prefix (see Section 5.1).

OK. Let's see what we have in http2.

isTableSizeUpdate :: Word8 -> Bool
isTableSizeUpdate w = w .&. 0xe0 == 0x20

where 0xe0 == 224 and 0x20 == 32. I tried to represent it. Please see below.

 +---+---+---+---+---+---+---+---+
 | 0 | 0 | 0 | 0 | 1 | 1 | 1 | 1 | -- 224
 +---+---+---+---+---+---+---+---+
 | 0 | 0 | 0 | 0 | 1 | 0 | 0 | 0 | -- 32
 +---+---+---+---+---+---+---+---+
 and the result of "bitwise AND" would be 32:
 +---+---+---+---+---+---+---+---+
 | 0 | 0 | 0 | 0 | 1 | 0 | 0 | 0 | -- 32
 +---+---+---+---+---+---+---+---+
 001 - pattern:
 +---+---+---+---+---+---+---+---+
 | 0 | 0 | 1 | 0 | 0 | 0 | 0 | 0 | -- 4
 +---+---+---+---+---+---+---+---+

Is it correct?

@kazu-yamamoto could you please explain isTableSizeUpdate?

swamp-agr commented 5 years ago

Hello! Did you have any chances to look into it?

lucasdicioccio commented 5 years ago

Hi, sorry, I'm super busy these days and force myself to take the majority of my time off away from computers. I'll likely be in better shape starting next week end!

swamp-agr commented 5 years ago

Hi @lucasdicioccio, how are you? Hope, you are in the good shape to make review this weekend. Please let me know if I can help anything with.

lucasdicioccio commented 5 years ago

Hey Sorry for being so slow to answer :-)! Things are more quiet this WE, I've spent a bit of time reading through the HPACK spec (I'm not really comfortable with it yet though).

To summarize this patch looks good to me:

it's not perfect as it does not handle the case where the table size shrinks then regrows (we should be emptying the HPACK table on this "event")
doesn't look like it will trigger new bugs because current implementation just explodes on the table shrink
address one example of observed problem
more work would be needed for a proper support => please keep the issue open

kazu-yamamoto commented 5 years ago

May I ask to add a test case as regression test?

swamp-agr commented 5 years ago

Yes, sure. Please expect it by tomorrow.

swamp-agr commented 5 years ago

@kazu-yamamoto, you are welcome to review added regression test as requested. I tried to fit your style in tests. Please correct me if I am wrong.

swamp-agr commented 5 years ago

@kazu-yamamoto, could you please trigger Travis one more time?

1 check failed due to something not related to test itself: https://travis-ci.org/kazu-yamamoto/http2/jobs/514807025

kazu-yamamoto commented 5 years ago

Restarted CI and now passed.

kazu-yamamoto commented 5 years ago

This PR has been merged as follows:

test is moved to the first
.gitignore stuff is removed

kazu-yamamoto commented 5 years ago

A new version has been released. Thank you for your contribution!

kazu-yamamoto / http2

Deny shrink of dynamic table to zero size #17