Auto-port 4.2: MQTT: Make the decodeProperties early-REPLAY check actually fire#16919
Merged
Conversation
) Motivation: The CVE-2026-44248 fix in 82f47fa added a guard at the top of `decodeProperties` to trigger an early REPLAY when the cumulation buffer did not yet have the full properties block: ```java if (buffer.readableBytes() < totalPropertiesLength) { buffer.readSlice(totalPropertiesLength); } ``` Because `MqttDecoder` extends `ReplayingDecoder`, the buffer passed to `decodeProperties` is a `ReplayingDecoderByteBuf` whose `readableBytes()` returns `Integer.MAX_VALUE - readerIndex` rather than the actual number of bytes available (see `ReplayingDecoderByteBuf.readableBytes()`). The `if`-condition is therefore false in practice and the `readSlice()` call never executes, so the early-REPLAY optimization is effectively dead code. When the properties block arrives in chunks the decoder still falls back to partial parsing followed by a mid-loop REPLAY, which defeats the optimization's intent on slow streams. Modification: Replace the broken `readableBytes()` check with a `getByte()` probe at `buffer.readerIndex() + totalPropertiesLength - 1`. `ReplayingDecoderByteBuf.checkIndex` throws `Signal.REPLAY` when `index + 1 > writerIndex` — i.e. exactly when the cumulation buffer does not yet hold the full properties block. The call has no side effect on `readerIndex`, so subsequent parsing is unchanged once the data arrives. A `totalPropertiesLength > 0` guard skips the probe when there are no properties. Result: The early-REPLAY guard now actually fires when properties data is incomplete, restoring the CVE-2026-44248 fix's intent of avoiding repeated partial-properties parsing on slow streams. No semantic change on the happy path; all 107 existing `codec-mqtt` tests pass locally. Note: this is complementary to #16787, which addresses the *outer* variable-header REPLAY handling broken by the same CVE patch (same `buffer.readableBytes()` antipattern in a ReplayingDecoder context). Both fixes are independent and can land in either order; together they restore the optimization the CVE-2026-44248 fix originally aimed for. (cherry picked from commit 2d781e2)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Auto-port of #16813 to 4.2
Cherry-picked commit: 2d781e2
Motivation:
The CVE-2026-44248 fix in 82f47fa added a guard at the top of
decodePropertiesto trigger an early REPLAY when the cumulation buffer did not yet have the full properties block:Because
MqttDecoderextendsReplayingDecoder, the buffer passed todecodePropertiesis aReplayingDecoderByteBufwhosereadableBytes()returnsInteger.MAX_VALUE - readerIndexrather than the actual number of bytes available (seeReplayingDecoderByteBuf.readableBytes()). Theif-condition is therefore false in practice and thereadSlice()call never executes, so the early-REPLAY optimization is effectively dead code. When the properties block arrives in chunks the decoder still falls back to partial parsing followed by a mid-loop REPLAY, which defeats the optimization's intent on slow streams.Modification:
Replace the broken
readableBytes()check with agetByte()probe atbuffer.readerIndex() + totalPropertiesLength - 1.ReplayingDecoderByteBuf.checkIndexthrowsSignal.REPLAYwhenindex + 1 > writerIndex— i.e. exactly when the cumulation buffer does not yet hold the full properties block. The call has no side effect onreaderIndex, so subsequent parsing is unchanged once the data arrives. AtotalPropertiesLength > 0guard skips the probe when there are no properties.Result:
The early-REPLAY guard now actually fires when properties data is incomplete, restoring the CVE-2026-44248 fix's intent of avoiding repeated partial-properties parsing on slow streams. No semantic change on the happy path; all 107 existing
codec-mqtttests pass locally.Note: this is complementary to #16787, which addresses the outer variable-header REPLAY handling broken by the same CVE patch (same
buffer.readableBytes()antipattern in a ReplayingDecoder context). Both fixes are independent and can land in either order; together they restore the optimization the CVE-2026-44248 fix originally aimed for.