Skip to content

fix: keep the HPACK decoder usable after a header fails to parse - #1252

Open
pjfanning wants to merge 1 commit into
apache:mainfrom
pjfanning:hpack-parse-error-keeps-connection-usable
Open

fix: keep the HPACK decoder usable after a header fails to parse#1252
pjfanning wants to merge 1 commit into
apache:mainfrom
pjfanning:hpack-parse-error-keeps-connection-usable

Conversation

@pjfanning

Copy link
Copy Markdown
Member

Noticed while working on #1251. A single malformed header currently makes every later HEADERS frame on that connection undecodable.

The bug

HeaderDecompression's HeaderListener threw ParsingException straight through the HPACK decoder. Three things follow from that, and they compound:

  1. Decoder.insertHeader calls the listener before dynamicTable.add(...), so the entry for the offending header never reached the table.
  2. decode() unwound at that point, so every representation after it in the block was never read, and never added to the table either.
  3. endHeaderBlock() sat inside the try, so it was skipped — leaving the decoder holding the state and headerSize of 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. HeaderDecompression deliberately 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 ParsingException in the listener, remember the first ErrorInfo, and return null so 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.

null rather than the raw value on purpose: the decoder caches what the listener returns against the table entry, and caching an unparsed String where a ContentType is expected would hand a wrong type to a later indexed reference. null means it is parsed again — and fails again, consistently.

endHeaderBlock() also moves into a finally, so anything else that unwinds still resets the decoder — a malformed :path raises Http2ProtocolException, which is not a ParsingException. Running it twice on the success path is a no-op (headerSize is already 0). The outer ParsingException handler stays as a fallback for anything thrown outside the listener.

Tests

New "keep the connection usable after a header parsing failure" in Http2ClientServerSpec: 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 HeaderDecompression matched main exactly:

- should keep the connection usable after a header parsing failure *** FAILED ***
  java.lang.AssertionError: assertion failed: timeout (3 seconds) during expectMsgClass
  waiting for class ...Http2ClientServerSpec$ServerRequest

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 passed
  • http-core/testOnly org.apache.pekko.http.impl.engine.http2.* — 13 passed
  • http2-tests/test — 353 passed, 25 ignored, 26 pending
  • scalafmtCheckAll, headerCheck, http-core/mimaReportBinaryIssues — clean

Note on #1251

This is based on main so it can be reviewed and backported on its own. It touches the same try block that #1251 rewrites, so the two conflict textually; I will rebase whichever lands second.

🤖 Generated with Claude Code

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant