Fix 32-bit overflow in byteList size calc in LZW_GenerateStream#118
Open
rootvector2 wants to merge 1 commit into
Open
Fix 32-bit overflow in byteList size calc in LZW_GenerateStream#118rootvector2 wants to merge 1 commit into
rootvector2 wants to merge 1 commit into
Conversation
MAX_CODE_LEN is an int literal (12) and lzwPos is uint32_t, so the intermediate product MAX_CODE_LEN * lzwPos was evaluated in 32-bit unsigned arithmetic before being divided into a uint64_t. For lzwPos above UINT32_MAX/12 (~358M) the multiplication wraps modulo 2^32, so MaxByteListLen / MaxByteListBlockLen end up much smaller than the actual number of bytes create_byte_list / create_byte_list_block can write into the buffer (heap overflow on the byteList malloc). For a max-size frame (65535 * 65535 pixels) the buggy value is about 536 MB whereas create_byte_list can write up to ~6.4 GB. Cast MAX_CODE_LEN to uint64_t so the entire expression is evaluated in 64-bit, matching the existing fix from dloebl#103 for pLZWData.
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.
Bug
In
LZW_GenerateStream(src/cgif_raw.c:352-353) the two byte-buffersize calculations evaluate
MAX_CODE_LEN * lzwPosin 32-bitunsigned arithmetic before extending into a
uint64_t:MAX_CODE_LENis the int literal12andlzwPosisuint32_t, sothe intermediate product is computed in
uint32_t. The/ 8ull(and the later
* (BLOCK_SIZE + 1ull)on the second line) onlyextends the result after the multiplication, so the cast comes too
late. Once
lzwPos > UINT32_MAX / 12(~358M), the multiplicationsilently wraps mod 2^32.
The maximum reachable
lzwPosis roughlynumPixel + numPixel/(MAX_DICT_LEN-initDictLen-2) + 2, andnumPixelcomes from
MULU16(width, height)where both areuint16_t. A frameof
65535 * 65535pixels (the max the raw API accepts) driveslzwPosto about4.295e9.For that frame:
MaxByteListLenMaxByteListBlockLenbyteListis thenmalloc'd to the truncated size, whilecreate_byte_list/create_byte_list_blockproceed to write up tothe real (un-truncated) byte count into it - a multi-GB heap buffer
overflow on the byteList allocation. This is the same shape of bug
that #103 fixed for
pLZWData, just on the very next twoallocations.
Reproduce / observe
A minimal reproducer of just the arithmetic, built with
-fsanitize=unsigned-integer-overflow:Whereas the correct upper bound for that
lzwPosis600000003.End-to-end the bug is reachable from
cgif_raw_addframewith asingle frame whose
width * heightis large enough to pushlzwPosabove ~358M. The raw API caps width/height at
uint16_tbut doesnot cap
width * height, so a single 18000+ x 18000+ frame isenough to trigger it on a host with sufficient memory for the input
buffers.
Fix
Cast
MAX_CODE_LENtouint64_tso the whole expression isevaluated in 64-bit and the multiplication can no longer wrap. The
change is local to
LZW_GenerateStreamand matches the(size_t)numPixelcast that #103 added to thepLZWDataallocationtwo lines above.
Build / test
Built locally with meson + clang and ran the full existing test
suite (
meson test -C builddir): all 60 tests pass. The minimalreproducer no longer trips
-fsanitize=unsigned-integer-overflowafter the patch, and thecomputed values match the un-truncated 64-bit results.