Prevent signed integer overflow in compound dictionary total_size#1438
Closed
mohammadmseet-hue wants to merge 1 commit intogoogle:masterfrom
Closed
Prevent signed integer overflow in compound dictionary total_size#1438mohammadmseet-hue wants to merge 1 commit intogoogle:masterfrom
mohammadmseet-hue wants to merge 1 commit intogoogle:masterfrom
Conversation
AttachCompoundDictionary() accumulates dictionary chunk sizes into total_size (int) via: addon->total_size += (int)size; With up to 15 chunks allowed, cumulative sizes exceeding INT_MAX cause signed integer overflow — undefined behavior in C. UBSAN confirms this: decode.c:1545:21: runtime error: signed integer overflow: 1879048192 + 268435456 cannot be represented in type 'int' The overflowed total_size propagates to chunk_offsets[], which is used by EnsureCompoundDictionaryInitialized() and CopyFromCompoundDictionary() during decompression. Negative offsets can cause incorrect memcpy lengths. This commit adds overflow checks before the addition: 1. Reject individual chunks larger than INT_MAX 2. Reject chunks that would cause total_size to overflow The encoder side (encode.c) correctly uses size_t for the equivalent computation, confirming this is an oversight in the decoder.
Collaborator
|
Hi. Thanks for reporting. Would you mind if I make an alternative fix? (of course, this PR will be mentioned) |
Author
|
Yes of course, implement the fix the way you like, happy to help and would appreciate the mention |
Collaborator
|
Has been landed (merged) via #1450 |
Author
Collaborator
|
Very sorry. Will fix that ASAP. |
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.
Summary
AttachCompoundDictionary()inc/dec/decode.caccumulates dictionary chunk sizes intototal_size(typeint) without overflow checking. With up to 15 chunks allowed and no limit on individual chunk sizes, cumulative sizes exceedingINT_MAXcause signed integer overflow — undefined behavior in C.The Bug
Line 1545:
total_sizeisint(32-bit signed).sizeissize_t. With 15 chunks of 256MB each:UBSAN Confirmation
Downstream Impact
The overflowed
total_sizepropagates to:chunk_offsets[](line 1546) — stores incorrect accumulated offsetsEnsureCompoundDictionaryInitialized()(line 1557) —(addon->total_size - 1) >> block_bitswith negativetotal_sizecauses incorrect block mappingCopyFromCompoundDictionary()(line 1598) — computesrem_chunk_lengthfrom corruptedchunk_offsets, potentially yielding incorrectmemcpylengthsNote: the encoder side (
encode.c) correctly usessize_tfor the equivalent computation, confirming this is an oversight in the decoder.The Fix
Two overflow checks added before the addition:
0x7FFFFFFF(INT_MAX)total_sizeto exceed0x7FFFFFFFReproduction
Build with:
gcc -fsanitize=undefinedto see the UBSAN error.