This is an automated email from the ASF dual-hosted git repository.
pjfanning pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/pekko-http.git
The following commit(s) were added to refs/heads/main by this push:
new 9eb283ed4 fix: read HPACK string literals with readNBytes (#1231)
9eb283ed4 is described below
commit 9eb283ed45d274cd948a9dae315be42ef98a50e1
Author: PJ Fanning <[email protected]>
AuthorDate: Sun Aug 30 11:20:33 2026 +0100
fix: read HPACK string literals with readNBytes (#1231)
Motivation:
Decoder.readStringLiteral asked for a whole string literal with a single
`in.read(buf)` and treated any shorter result as a decompression failure.
InputStream.read(byte[]) is explicitly allowed to return fewer bytes than
requested even when more are available, so this relies on a contract the API
does not offer.
It is not an observable bug today: the only caller wraps a compacted
ByteString
(HeaderDecompression compacts because the decoder needs mark/reset and
meaningful available()), and the decoder waits for `in.available() >=
length`
before reading, so the single read always fills the buffer. It is a trap for
any future change to how that stream is produced.
Modification:
Use InputStream.readNBytes, which loops until the requested number of bytes
has
been read or the stream ends, and compare the returned length. Available
since
JDK 11 and this branch requires JDK 17.
Result:
The decoder no longer depends on a single read filling the buffer.
Behaviour is
unchanged for a stream that does fill it, and truncated input is still
reported
as a decompression failure.
Tests:
- New HpackDecoderSpec round-trips header blocks through the shaded Encoder
and
Decoder over a stream that has all its data available but returns 1, then
7,
bytes per read. Both cases fail before this change with a decompression
failure and pass after it
- sbt "http-core / Test / testOnly
org.apache.pekko.http.impl.engine.http2.hpack.HpackDecoderSpec" - 3 passed
- sbt "http2-tests / Test / testOnly
org.apache.pekko.http.impl.engine.http2.RequestParsingSpec" - 25 passed, 3
pending
- sbt http-core/javafmtCheck and scalafmt - clean
References:
None - found while reviewing the code base against the JDK 17 baseline
---
.../http/shaded/com/twitter/hpack/Decoder.java | 7 +-
.../impl/engine/http2/hpack/HpackDecoderSpec.scala | 87 ++++++++++++++++++++++
2 files changed, 92 insertions(+), 2 deletions(-)
diff --git
a/http-core/src/main/java/org/apache/pekko/http/shaded/com/twitter/hpack/Decoder.java
b/http-core/src/main/java/org/apache/pekko/http/shaded/com/twitter/hpack/Decoder.java
index ab12f4d26..0356a6690 100644
---
a/http-core/src/main/java/org/apache/pekko/http/shaded/com/twitter/hpack/Decoder.java
+++
b/http-core/src/main/java/org/apache/pekko/http/shaded/com/twitter/hpack/Decoder.java
@@ -529,8 +529,11 @@ public final class Decoder {
}
private String readStringLiteral(InputStream in, int length) throws
IOException {
- byte[] buf = new byte[length];
- if (in.read(buf) != length) {
+ // readNBytes rather than read: InputStream.read(byte[]) is free to return
fewer bytes than
+ // requested even when more are available, which would be reported here as
a decompression
+ // failure
+ byte[] buf = in.readNBytes(length);
+ if (buf.length != length) {
throw DECOMPRESSION_EXCEPTION;
}
final byte[] result;
diff --git
a/http-core/src/test/scala/org/apache/pekko/http/impl/engine/http2/hpack/HpackDecoderSpec.scala
b/http-core/src/test/scala/org/apache/pekko/http/impl/engine/http2/hpack/HpackDecoderSpec.scala
new file mode 100644
index 000000000..fa5643797
--- /dev/null
+++
b/http-core/src/test/scala/org/apache/pekko/http/impl/engine/http2/hpack/HpackDecoderSpec.scala
@@ -0,0 +1,87 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.pekko.http.impl.engine.http2.hpack
+
+import java.io.{ ByteArrayInputStream, ByteArrayOutputStream, InputStream }
+
+import scala.collection.mutable.ListBuffer
+
+import org.apache.pekko.http.shaded.com.twitter.hpack.{ Decoder, Encoder,
HeaderListener }
+
+import org.scalatest.matchers.should.Matchers
+import org.scalatest.wordspec.AnyWordSpec
+
+class HpackDecoderSpec extends AnyWordSpec with Matchers {
+
+ val maxHeaderSize = 4096
+ val maxHeaderTableSize = 4096
+
+ /**
+ * A stream that has all of its data available but hands it out in small
pieces, which
+ * `InputStream.read(byte[])` is explicitly allowed to do.
+ */
+ private class TricklingInputStream(bytes: Array[Byte], bytesPerRead: Int)
extends InputStream {
+ private val underlying = new ByteArrayInputStream(bytes)
+ override def read(): Int = underlying.read()
+ override def read(b: Array[Byte], off: Int, len: Int): Int =
+ underlying.read(b, off, math.min(len, bytesPerRead))
+ override def available(): Int = underlying.available()
+ override def markSupported(): Boolean = underlying.markSupported()
+ override def mark(readLimit: Int): Unit = underlying.mark(readLimit)
+ override def reset(): Unit = underlying.reset()
+ override def skip(n: Long): Long = underlying.skip(n)
+ }
+
+ private def encode(headers: (String, String)*): Array[Byte] = {
+ val out = new ByteArrayOutputStream
+ val encoder = new Encoder(maxHeaderTableSize)
+ headers.foreach { case (name, value) => encoder.encodeHeader(out, name,
value, false) }
+ out.toByteArray
+ }
+
+ private def decode(in: InputStream): Seq[(String, String)] = {
+ val decoded = ListBuffer.empty[(String, String)]
+ val decoder = new Decoder(maxHeaderSize, maxHeaderTableSize)
+ decoder.decode(in,
+ new HeaderListener {
+ override def addHeader(name: String, value: String, parsed: AnyRef,
sensitive: Boolean): AnyRef = {
+ decoded += (name -> value)
+ null
+ }
+ })
+ decoder.endHeaderBlock()
+ decoded.toList
+ }
+
+ "The HPACK decoder" should {
+ val headers = Seq("x-custom-header" -> "some-fairly-long-header-value",
"another-header" -> "value")
+
+ "decode a header block from a stream that returns everything at once" in {
+ decode(new ByteArrayInputStream(encode(headers: _*))) shouldEqual headers
+ }
+
+ "decode a header block from a stream that returns one byte per read" in {
+ // string literals are read with a single call, so a short read must not
be taken for truncation
+ decode(new TricklingInputStream(encode(headers: _*), bytesPerRead = 1))
shouldEqual headers
+ }
+
+ "decode a header block from a stream that returns a few bytes per read" in
{
+ decode(new TricklingInputStream(encode(headers: _*), bytesPerRead = 7))
shouldEqual headers
+ }
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
