MahathirMohammadShuvo opened a new pull request, #23273:
URL: https://github.com/apache/kafka/pull/23273
`unsignedIntToShort` in `AbstractApiMessageSerde` checked only the upper
bound:
```java
if (val > Short.MAX_VALUE) {
throw new MetadataParseException("Value for " + entity + " was too
large.");
}
return (short) val;
```
`ByteUtils.readUnsignedVarint` decodes an unsigned 32-bit value into a signed
`int` (`result |= (tmp = buffer.get()) << 28`), so an encoded value in
`[2^31, 2^32)` comes back negative. A negative value is not `>
Short.MAX_VALUE`,
so it passes the guard and is then truncated to its low 16 bits by the cast.
Measured against a transcription of `readUnsignedVarint`:
| bytes | decodes to | before | after |
| --- | --- | --- | --- |
| `81 80 80 80 08` | -2147483647 | returns `1` | rejected |
| `85 80 80 80 08` | -2147483643 | returns `5` | rejected |
| `ff ff ff ff 0f` | -1 | returns `-1` | rejected |
| `ff ff ff 7f` | 268435455 | rejected | rejected |
| `01` | 1 | returns `1` | returns `1` |
`DEFAULT_FRAME_VERSION` is `1`, so the first row is accepted as the default
frame version and parsing continues against a record that never declared one.
The same bytes in the type field make the record deserialize as a different
`apiKey`.
### Scope
The guard now rejects a varint that decodes negative. It does not attempt to
reject every encoding above `Integer.MAX_VALUE` — `81 80 80 80 10` encodes
4294967297, and `16 << 28` wraps to `0`, so it decodes to a positive `1` and
is
still accepted. That is a property of the primitive rather than of this
check.
The fix is deliberately in `unsignedIntToShort` and not in
`readUnsignedVarint`.
`readVarint` is built on the unsigned reader and zigzag-decodes its result,
so
the primitive **must** be able to return negative — `81 80 80 80 08` is the
legitimate encoding of `readVarint(-1073741825)`. Rejecting it there would
break
record-batch lengths, offset deltas and every compact string, tagged field
and
generated `read()`. The bytes are a valid varint; only their interpretation
as an
unsigned short-range field is wrong.
No compatibility concern: `write()` only ever emits `DEFAULT_FRAME_VERSION`,
`apiKey()` and `version()`, all small non-negative shorts, so no compliant
writer
can produce an encoding the new guard rejects.
### Tests
`testParsingVersionEncodedAboveIntMax` added next to the existing
`testParsingVersionTooLarge`, which uses a four-byte varint (`ff ff ff 7f`)
that
stays positive — which is why the negative path was never covered.
- `./gradlew :metadata:test --tests
"org.apache.kafka.metadata.MetadataRecordSerdeTest"` —
**10 passed, 0 failed**.
- With only `AbstractApiMessageSerde.java` reverted to `trunk`, the new test
is
the **only** failure; the other nine still pass.
- Weakening the guard to `val < 0` alone makes `testParsingVersionTooLarge`
fail,
so both halves of the condition are independently pinned.
- `./gradlew :server-common:checkstyleMain :metadata:checkstyleTest` — clean.
--
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]
