okhttp: fix HPACK reader bug#4276
Merged
ericgribkoff merged 4 commits intogrpc:masterfrom Mar 28, 2018
Merged
Conversation
Contributor
Author
|
@ejona86 I locally ran the OkHttp 2.7 tests (https://github.com/square/okhttp/blob/okhttp_27/okhttp-tests/src/test/java/com/squareup/okhttp/internal/framed/HpackTest.java) on this change, and also added a new test case which verifies the fix for this bug. It seems like it might be time to add (at least) these tests to our forked codebase; what do you think? |
ejona86
approved these changes
Mar 27, 2018
ejona86
added
the
TODO:release blocker
ericgribkoff
force-pushed
the
okhttp_hpack
branch
from
e07b181 to
e5df51b
Compare
ericgribkoff
force-pushed
the
okhttp_hpack
branch
from
e5df51b to
5980890
Compare
ericgribkoff
removed
the
TODO:release blocker
Contributor
Author
ericgribkoff
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
ericgribkoff
added a commit
that referenced
this pull request
Mar 28, 2018
Backport of the first commit of #4276.
Member
|
Could this be merged? I'd like it in master before doing the release with the backport. |
ericgribkoff
force-pushed
the
okhttp_hpack
branch
from
0c32dfb to
8453a59
Compare
Contributor
Author
|
Fixed the third commit (needed bazel build file update for new path), will merge on green |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes #4261.
Our OkHttp HPACK decoder was improperly tossing out the already-parsed header fields when it resized the dynamic table. From the HPACK RFC section 3.1:
This bug appears to have also been present on the OkHttp 2.7 upstream branch.
This was caught in #4261 by interop with nginx, which was setting the max dynamic table size to 0. When considering adding an item to the table and finding that item exceeds the max dynamic table size,
OkHttp clears the dynamic table. This is the correct behavior, but it was also deleting the already processed header fields. The HTTP status code was among the fields that was erroneously thrown away, triggering the exception in #4261.
The only substantive change in this PR is the deletion of line 167 (
headerList.clear()) in theclearDynamicTable()method. The other changes are just two variable renames to make it clear that the values tracked in the dynamic table are not the same as the already-processed header values to return to the application inheaderList.