fix: keep the HPACK decoder usable after a header fails to parse - #1252
Open
pjfanning wants to merge 1 commit into
Open
fix: keep the HPACK decoder usable after a header fails to parse#1252pjfanning wants to merge 1 commit into
pjfanning wants to merge 1 commit into
Conversation
Motivation: HeaderDecompression's HeaderListener threw ParsingException straight through the HPACK decoder, which left the connection unable to decode any later HEADERS frame: - Decoder.insertHeader calls the listener before adding the entry to the dynamic table, so the entry for the offending header was never added - decode() unwound at that point, so every representation after it in the block was never read and never added either - endHeaderBlock() was called inside the try, so it was skipped and the decoder kept the state and headerSize of the abandoned block The first two desynchronise the decoder's dynamic table from the peer's encoder's, which HPACK cannot recover from; the third resumes the next block part way through a representation. HeaderDecompression answers a parse failure with a bad request and keeps the connection open, so this is reachable with a single malformed header - an unknown method is enough. Modification: Catch ParsingException in the listener, remember the first ErrorInfo and return null so that decoding runs to the end of the block and the dynamic table keeps tracking the peer's. Report the remembered failure once the block is decoded. Call endHeaderBlock() in a finally as well, so anything else that unwinds - a malformed pseudo header raises Http2ProtocolException - still resets the decoder. The outer ParsingException handler stays as a fallback. Result: A request with an unparseable header still gets a bad request response, and subsequent requests on the same connection are decoded correctly. Tests: - New "keep the connection usable after a header parsing failure" in Http2ClientServerSpec sends a request with an unknown method, expects the bad request, then sends a valid request on the same connection. Without the fix the second request never reaches the handler at all - the spec times out waiting for it - and with the fix it is served normally - sbt "http2-tests/testOnly ...Http2ClientServerSpec" - 8 passed - sbt "http-core/testOnly org.apache.pekko.http.impl.engine.http2.*" - 13 passed - sbt http2-tests/test - 353 passed, 25 ignored, 26 pending - scalafmtCheckAll, headerCheck, http-core/mimaReportBinaryIssues - clean References: Noticed while working on apache#1251 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Noticed while working on #1251. A single malformed header currently makes every later HEADERS frame on that connection undecodable.
The bug
HeaderDecompression'sHeaderListenerthrewParsingExceptionstraight through the HPACK decoder. Three things follow from that, and they compound:Decoder.insertHeadercalls the listener beforedynamicTable.add(...), so the entry for the offending header never reached the table.decode()unwound at that point, so every representation after it in the block was never read, and never added to the table either.endHeaderBlock()sat inside thetry, so it was skipped — leaving the decoder holding thestateandheaderSizeof the abandoned block.(1) and (2) desynchronise the decoder's dynamic table from the peer's encoder's, which is not something HPACK can recover from: from then on the peer's indexed references resolve to the wrong entries. (3) resumes the next block part way through a representation.
This is reachable, not theoretical.
HeaderDecompressiondeliberately answers a parse failure with a bad request and keeps the connection open, so an unknown method — already covered by "return bad request response when header parsing fails" — is enough to trigger it.The fix
Catch
ParsingExceptionin the listener, remember the firstErrorInfo, and returnnullso decoding runs to the end of the block and the dynamic table keeps tracking the peer's. Report the remembered failure once the block is fully decoded, which produces the same bad request response as before.nullrather than the raw value on purpose: the decoder caches what the listener returns against the table entry, and caching an unparsedStringwhere aContentTypeis expected would hand a wrong type to a later indexed reference.nullmeans it is parsed again — and fails again, consistently.endHeaderBlock()also moves into afinally, so anything else that unwinds still resets the decoder — a malformed:pathraisesHttp2ProtocolException, which is not aParsingException. Running it twice on the success path is a no-op (headerSizeis already 0). The outerParsingExceptionhandler stays as a fallback for anything thrown outside the listener.Tests
New
"keep the connection usable after a header parsing failure"inHttp2ClientServerSpec: send a request with an unknown method, expect the bad request, then send a valid request on the same connection.Verified it actually catches the bug — with the fix stashed so
HeaderDecompressionmatchedmainexactly:The follow-up request does not merely come back wrong — it never reaches the handler at all, which is the desync showing up end to end.
http2-tests/testOnly ...Http2ClientServerSpec— 8 passedhttp-core/testOnly org.apache.pekko.http.impl.engine.http2.*— 13 passedhttp2-tests/test— 353 passed, 25 ignored, 26 pendingscalafmtCheckAll,headerCheck,http-core/mimaReportBinaryIssues— cleanNote on #1251
This is based on
mainso it can be reviewed and backported on its own. It touches the sametryblock that #1251 rewrites, so the two conflict textually; I will rebase whichever lands second.🤖 Generated with Claude Code