Fix COMPRESSION_ERROR on valid HPACK input

[mrdg]

This fixes a bug that causes a valid http2 header value to be rejected with a COMPRESSION_ERROR:

With the test input I added, a leaf node is found exactly on a byte boundary. Because the leaf node check happens before the shift check, byte_boundary_crossed is set to 0, but then immediately incremented again. And because finding the next leaf node crosses three byte boundaries, we end up with 4 total and return an error. Reordering the checks fixes the issue.

Note: I wasn't able to run the whole test suite so I'm not sure if this breaks anything else.

[randall]

@ezelkow1 Can you kick off builds here?

[mrdg]

I found that my first commit still had problems: the byte_boundary_crossed > 3 check still fails for some valid inputs. When a code is more than 3 bytes long, and starts right before a byte boundary, you still cross 4 byte boundaries. I think the better approach is to count the number of bits parsed and check that you don't exceed the length of longest code in the table. I did that in f70728a. This should catch some other invalid inputs that the previous approach didn't catch (see added tests).

[maskit]

Thank you for the fix. I ran test_HPACK with a couple of external test data and I was realized that current HPACK decoder seems to be broken and it's still broken even with your fix.

What I did are:

$ cd trafficserver/proxy/http2
$ git clone https://github.com/http2jp/hpack-test-case.git
$ ./test_HPACK -i hpack-test-case/go-hpack/

test_HPACK(16235,0x10da86600) malloc: nano zone abandoned due to inability to preallocate reserved vm space.
32 test stories
REGRESSION_TEST initialization begun
REGRESSION TEST HPACK_Encoding started
    REGRESSION_RESULT HPACK_Encoding:                           PASSED
REGRESSION TEST HPACK_Decoding started
BX=c99r6jp89a7nou0026b=3u0026s=q4; localization=en-us%3Bus%3Bus <-> BX=c99r6jp89a7no&b=3&s=q4; localization=en-us%3Bus%3Bus
RPRINT HPACK_Decoding: Story 8 sequence 1 failed.
    REGRESSION_RESULT HPACK_Decoding:                           FAILED
REGRESSION TEST Regression started
RPRINT Regression: regression test
RPERF Regression.speed 100.000000
    REGRESSION_RESULT Regression:                               PASSED

It might be an error in the test data, but I think I checked ATS can decode all the encoded data from different implementations when we were actively working on HTTP/2 and HPACK. Let me check the error above first.

I haven't looked into your change closely, but I'd suggest to move the test case you added into test_HuffmanCode since it's purely for Huffman decoding.

[maskit]

Seems like the errors I found are due to incompatibility between the test data and test program. We can ignore the errors for now.

And good news is that your change passed all the test we can try. I'll look into your change closely later.

[maskit]

Actually, it failed on a test in HTTP/2 test suite (h2spec). I thought this test is ran on CI but it didn't catch the error somehow.

I ran h2spec locally and got this error below.

HPACK: Header Compression for HTTP/2
  5. Primitive Type Representations
    5.2. String Literal Representation
      using source address 127.0.0.1:62782
      × 3: Sends a Huffman-encoded string literal representation containing the EOS symbol
        -> The endpoint MUST treat this as a decoding error.
           Expected: GOAWAY Frame (Error Code: COMPRESSION_ERROR)
                     Connection closed
             Actual: DATA Frame (length:219, flags:0x01, stream_id:1)

I get your fix is needed for the byte boundary issue but the error above need to be fixed.

Also, to give a bit of context about byte_boundary_crossed, it was added for another test case in h2spec.
#2172

[mrdg]

@maskit sorry for the delay: I had to update my machine to get the autest suite to run. I pushed a fix for the failing EOS test. That fix itself made one of the Huffman tests fail, because it was trying to decode an EOS symbol, which is invalid, so I changed it to check for an error.

I also moved the tests I added to test_Huffmancode.cc

[maskit]

Thank you for the updates. I confirmed all the tests pass.

[zwoop]

Cherry-picked to v9.2.x