From 96fa0085453738d79120963ad6cc002301487cd9 Mon Sep 17 00:00:00 2001 From: PJ Fanning Date: Sun, 30 Aug 2026 13:20:20 +0100 Subject: [PATCH] fix: keep the HPACK decoder usable after a header fails to parse 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 #1251 Co-Authored-By: Claude Opus 5 (1M context) --- .../http2/hpack/HeaderDecompression.scala | 32 ++++++++++++++++--- .../engine/http2/Http2ClientServerSpec.scala | 16 ++++++++++ 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderDecompression.scala b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderDecompression.scala index 6aad6e085..7d188bf3b 100644 --- a/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderDecompression.scala +++ b/http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderDecompression.scala @@ -21,7 +21,7 @@ import pekko.http.impl.engine.http2.Http2Protocol.ErrorCode import pekko.http.impl.engine.http2.RequestParsing.parseHeaderPair import pekko.http.impl.engine.http2._ import pekko.http.impl.engine.parsing.HttpHeaderParser -import pekko.http.scaladsl.model.ParsingException +import pekko.http.scaladsl.model.{ ErrorInfo, ParsingException } import pekko.http.scaladsl.settings.ParserSettings import pekko.http.shaded.com.twitter.hpack.HeaderListener import pekko.stream._ @@ -73,8 +73,14 @@ private[http2] final class HeaderDecompression(masterHeaderParser: HttpHeaderPar def parseAndEmit( streamId: Int, endStream: Boolean, payload: ByteString, prioInfo: Option[PriorityFrame]): Unit = { val headers = new VectorBuilder[(String, AnyRef)] + // A header that fails to parse must not unwind out of the decoder. Decoding has to run to the end of + // the block so that the HPACK dynamic table keeps tracking the peer's - insertHeader calls this + // listener before adding to the table, and the representations after this one would not be read at + // all - and so that endHeaderBlock resets the state machine. Both would otherwise stay wrong for + // every later HEADERS frame on the connection. Remember the first failure and report it afterwards. + var parsingError: Option[ErrorInfo] = None object Receiver extends HeaderListener { - def addHeader(name: String, value: String, parsed: AnyRef, sensitive: Boolean): AnyRef = { + def addHeader(name: String, value: String, parsed: AnyRef, sensitive: Boolean): AnyRef = try { if (parsed ne null) { headers += name -> parsed parsed @@ -104,6 +110,12 @@ private[http2] final class HeaderDecompression(masterHeaderParser: HttpHeaderPar handle(header) } } + } catch { + case ex: ParsingException => + if (parsingError.isEmpty) parsingError = Some(ex.info) + // nothing usable to cache against the table entry, so the value is parsed again if it is + // referenced again - and fails again, consistently + null } } val stream = payload.compact.asInputStream @@ -113,16 +125,28 @@ private[http2] final class HeaderDecompression(masterHeaderParser: HttpHeaderPar val truncated = decoder.endHeaderBlock() if (truncated) headerListSizeExceeded(streamId) - else push(eventsOut, ParsedHeadersFrame(streamId, endStream, headers.result(), prioInfo, None)) + else + parsingError match { + // push details further and let RequestErrorFlow handle responding with bad request + case Some(info) => + push(eventsOut, ParsedHeadersFrame(streamId, endStream, Seq.empty, prioInfo, Some(info))) + case None => + push(eventsOut, ParsedHeadersFrame(streamId, endStream, headers.result(), prioInfo, None)) + } } catch { case ex: ParsingException => - // push details further and let RequestErrorFlow handle responding with bad request + // not expected any more now that the listener catches them, kept so that one thrown from + // somewhere else still answers with a bad request rather than tearing down the connection push(eventsOut, ParsedHeadersFrame(streamId, endStream, Seq.empty, prioInfo, Some(ex.info))) case _: IOException => // this is signalled by the decoder when it failed, we want to react to this by rendering a GOAWAY frame fail(eventsOut, new Http2Compliance.Http2ProtocolException(ErrorCode.COMPRESSION_ERROR, "Decompression failed.")) } finally { + // endHeaderBlock is what resets the decoder for the next block, so it has to run even when decode + // unwound anyway - a malformed pseudo header raises an Http2ProtocolException, for one. Running it + // a second time after the call above is a no-op. + decoder.endHeaderBlock() stream.close() } } diff --git a/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ClientServerSpec.scala b/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ClientServerSpec.scala index 6f718ec56..eb3d2d721 100644 --- a/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ClientServerSpec.scala +++ b/http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ClientServerSpec.scala @@ -146,6 +146,22 @@ class Http2ClientServerSpec extends PekkoSpecWithMaterializer( response.status should be(StatusCodes.BadRequest) } + "keep the connection usable after a header parsing failure" in new TestSetup { + sendClientRequest(HttpRequest( + method = HttpMethod.custom("UNKNOWN_TO_SERVER"), + uri = "http://www.example.com/test").addAttribute(requestIdAttr, RequestId("bad"))) + expectClientResponse().status should be(StatusCodes.BadRequest) + + // the failing header must not have left the HPACK dynamic table out of step with the client's, nor the + // decoder's state machine part way through the previous block + sendClientRequest( + HttpRequest(uri = "http://www.example.com/afterwards").addAttribute(requestIdAttr, RequestId("good"))) + val serverRequest = expectServerRequest() + serverRequest.request.uri.path.toString shouldBe "/afterwards" + serverRequest.sendResponse(HttpResponse(entity = "pong")) + expectClientResponse().status should be(StatusCodes.OK) + } + "return internal server error when handler future fails" in new TestSetup { sendClientRequest() val serverRequest = expectServerRequest()