You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/05/04 17:57:24 UTC
[GitHub] [trafficserver] mrdg opened a new pull request, #8817: Fix `COMPRESSION_ERROR` on valid HPACK input
mrdg opened a new pull request, #8817:
URL: https://github.com/apache/trafficserver/pull/8817
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] randall commented on pull request #8817: Fix `COMPRESSION_ERROR` on valid HPACK input
Posted by GitBox <gi...@apache.org>.
randall commented on PR #8817:
URL: https://github.com/apache/trafficserver/pull/8817#issuecomment-1117686385
@ezelkow1 Can you kick off builds here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #8817: Fix `COMPRESSION_ERROR` on valid HPACK input
Posted by GitBox <gi...@apache.org>.
zwoop commented on PR #8817:
URL: https://github.com/apache/trafficserver/pull/8817#issuecomment-1137874730
Cherry-picked to v9.2.x
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #8817: Fix `COMPRESSION_ERROR` on valid HPACK input
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #8817:
URL: https://github.com/apache/trafficserver/pull/8817#issuecomment-1120531746
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit merged pull request #8817: Fix `COMPRESSION_ERROR` on valid HPACK input
Posted by GitBox <gi...@apache.org>.
maskit merged PR #8817:
URL: https://github.com/apache/trafficserver/pull/8817
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8817: Fix `COMPRESSION_ERROR` on valid HPACK input
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #8817:
URL: https://github.com/apache/trafficserver/pull/8817#issuecomment-1117689026
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] mrdg commented on pull request #8817: Fix `COMPRESSION_ERROR` on valid HPACK input
Posted by GitBox <gi...@apache.org>.
mrdg commented on PR #8817:
URL: https://github.com/apache/trafficserver/pull/8817#issuecomment-1125780754
@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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #8817: Fix `COMPRESSION_ERROR` on valid HPACK input
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #8817:
URL: https://github.com/apache/trafficserver/pull/8817#issuecomment-1120582102
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8817: Fix `COMPRESSION_ERROR` on valid HPACK input
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #8817:
URL: https://github.com/apache/trafficserver/pull/8817#issuecomment-1117687884
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] mrdg commented on pull request #8817: Fix `COMPRESSION_ERROR` on valid HPACK input
Posted by GitBox <gi...@apache.org>.
mrdg commented on PR #8817:
URL: https://github.com/apache/trafficserver/pull/8817#issuecomment-1119415598
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](https://github.com/apache/trafficserver/pull/8817/commits/f70728ac342b07bffe8b0fbfd1c52d56f9ed333c). This should catch some other invalid inputs that the previous approach didn't catch (see added tests).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org