pjfanning opened a new pull request, #1231:
URL: https://github.com/apache/pekko-http/pull/1231
### Motivation
`Decoder.readStringLiteral` asks for a whole HPACK string literal with a
single `read` and treats anything shorter as a decompression failure:
```java
byte[] buf = new byte[length];
if (in.read(buf) != length) {
throw DECOMPRESSION_EXCEPTION;
}
```
`InputStream.read(byte[])` is explicitly allowed to return fewer bytes than
requested even when more are available, so this depends on a guarantee the API
does not give.
**This is not an observable bug today**, and I want to be clear about that
rather than oversell it. The only caller wraps a *compacted* `ByteString` -
`HeaderDecompression.scala:99` does `payload.compact.asInputStream`, because
the decoder needs `mark`/`reset` and a meaningful `available()` - and the
decoder waits for `in.available() >= length` before calling
`readStringLiteral`, so the single read always fills the buffer. What this
change removes is a trap for anyone who later changes how that stream is
produced.
### Modification
```java
byte[] buf = in.readNBytes(length);
if (buf.length != length) {
throw DECOMPRESSION_EXCEPTION;
}
```
`readNBytes` loops until the requested count is read or the stream ends.
Available since JDK 11; this branch requires JDK 17.
Truncated input is still reported as a decompression failure, and a
zero-length literal still works (`readNBytes(0)` returns an empty array,
matching the old `read` of an empty buffer).
This file is already adapted from github.com/twitter/hpack rather than kept
byte-identical to upstream - it imports
`org.apache.pekko.http.impl.util.StringTools` - so a local change here is in
keeping with how it is maintained.
### Result
The decoder no longer depends on a single read filling the buffer, with no
behaviour change for a stream that does.
### Note on a tempting follow-up
Fixing this does *not* on its own make it safe to drop the `.compact` in
`HeaderDecompression` and save a copy per header block. The decoder also relies
on `in.available()` reporting the full remaining length (`available() <
nameLength` guards at `Decoder.java:280` and `:373`) and on `mark`/`reset`,
neither of which a rope-backed `ByteString` InputStream provides. Worth a
separate look, not a freebie from this change.
### Tests
- New `HpackDecoderSpec` round-trips header blocks through the shaded
`Encoder` and `Decoder` over a stream that has all of its data available but
hands it out 1 byte, and then 7 bytes, per `read`. **Both cases fail before
this change** with `java.io.IOException: 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. Note the Java
formatter was run on JDK 21 rather than the JDK 17 the contributing guide
suggests, since that is what was available here.
### References
None - found while reviewing the code base against the JDK 17 baseline
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
