fix: reject 5-byte LEB128 encodings that overflow uint32 in decodeUInt32 - #1690
Open
eeshsaxena wants to merge 1 commit into
Open
fix: reject 5-byte LEB128 encodings that overflow uint32 in decodeUInt32#1690eeshsaxena wants to merge 1 commit into
eeshsaxena wants to merge 1 commit into
Conversation
decodeUInt32 guards the maximum byte count, but not the range of the final byte. On the 5th byte only 4 of its 7 data bits fit in a uint32, so a byte above 0x0F encodes a value greater than MAX_UINT32. The 32-bit `<<` then silently drops the overflowing bits, so e.g. [0x80,0x80,0x80,0x80,0x10] (2^32) decodes to 0 and [0xFF,0xFF,0xFF,0xFF,0x7F] decodes to 4294967295 instead of being rejected. encodeUInt32 already validates the uint32 range, and the suite's existing 'exceeds uint32 range' test only covered the too-many-bytes case. Reject an over-range final byte before the shift so decode fails loudly on such input, matching the encoder. Valid encodings (final byte <= 0x0F, including MAX_UINT32) are unaffected.
Contributor
Greptile SummaryThe PR hardens unsigned LEB128 decoding by rejecting fifth-byte payloads that exceed the uint32 range before JavaScript’s bitwise shift can truncate them.
Confidence Score: 5/5The PR appears safe to merge. The new check rejects only fifth-byte payloads above the four bits available in a uint32, while preserving the valid maximum boundary and existing successful decoding behavior. Important Files Changed
Reviews (1): Last reviewed commit: "Reject 5-byte LEB128 encodings that over..." | Re-trigger Greptile |
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.
Problem
decodeUInt32insrc/common/utils/leb128.tsvalidates the byte count of a LEB128 sequence but not the range of its last byte.A uint32 needs at most 5 bytes. The 5th byte is read at
shift = 28, and only32 - 28 = 4of its 7 data bits fit in a uint32. When the 5th byte carries data above0x0F, the value is greater thanMAX_UINT32, but the JavaScript 32-bit<<silently drops the overflowing bits instead of erroring:So over-range 5-byte encodings decode to a wrong value rather than being rejected:
[0x80,0x80,0x80,0x80,0x10](2³²)0[0xFF,0xFF,0xFF,0xFF,0x1F]4294967295[0xFF,0xFF,0xFF,0xFF,0x7F]4294967295encodeUInt32already validates the uint32 range, and the suite's existingthrows for encoding that exceeds uint32 rangetest only covers the too-many-bytes case ([0x80,0x80,0x80,0x80,0x80,0x01]), so this class of over-range input slipped through — a decoder that's meant to fail on out-of-range input silently returns garbage instead.Fix
Reject an over-range final byte before the shift, so the decoder fails loudly to match the encoder:
FINAL_BYTE_SHIFTis(MAX_BYTES_FOR_UINT32 - 1) * DATA_BITS_PER_BYTE = 28andFINAL_BYTE_MAX_DATA = 0x0F. Every valid encoding (final byte ≤0x0F, includingMAX_UINT32→[0xFF,0xFF,0xFF,0xFF,0x0F]) is unaffected; only genuinely out-of-range input now throws.Tests
Added three cases to the
invalid inputsblock: two over-range 5-byte sequences that must throwLEB128 sequence exceeds uint32 range, and one assertingMAX_UINT32(final byte exactly0x0F) still decodes to4294967295. Fullleb128.spec.tspasses (42/42); reverting only the source change fails the two new "throws" cases, confirming they catch the bug.