mirror of
https://github.com/google/brotli.git
synced 2026-09-22 06:35:52 +00:00
Potential buffer over-reads due to incorrect masking #150
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @randomascii on GitHub (Dec 23, 2016).
When building Chromium with the v1.0.0 version of Brotli using the VC++ /analyze feature I see the following warnings:
brotli\enc\compress_fragment_two_pass.c(461) : warning C6385: Reading invalid data from 'cmd_histo': the readable size is '512' bytes, but '1024' bytes may be read.
brotli\enc\compress_fragment_two_pass.c(474) : warning C6385: Reading invalid data from 'cmd_bits': the readable size is '256' bytes, but '512' bytes may be read.
brotli\enc\compress_fragment_two_pass.c(474) : warning C6385: Reading invalid data from 'cmd_depths': the readable size is '128' bytes, but '256' bytes may be read.
brotli\enc\compress_fragment_two_pass.c(475) : warning C6385: Reading invalid data from 'kNumExtraBits': the readable size is '512' bytes, but '1024' bytes may be read.
brotli\enc\static_dict.c(90) : warning C6385: Reading invalid data from 'kBrotliDictionarySizeBitsByLength': the readable size is '25' bytes, but '128' bytes may be read.
Manual examination of these warnings suggest that they are all valid. In all cases an index is being anded with a bit mask but the bit mask has too many set bits.
In the first four cases a bit mask of 0xff is used by the arrays only have 128 elements. It would be safer to define a COUNTOF macro and change the code like this:
to something like this:
Or, just change the constant to 0x7f, or use a named constant to make sure they stay in sync.
The fifth warning is a bit different because the index is being masked down to 0-127 but the array is 25 elements long. This seems odd. If the value of 'len' is guaranteed to be in range then the masking is not needed, and if isn't guaranteed to be in range then the masking is insufficient. Maybe there is other data in the high bits of 'len' - I can't tell. The safest thing to do, it seems, would be to pad kBrotliDictionarySizeBitsByLength out to 32 bytes (at a cost of six bytes of storage) and then mask with 0x1f instead of 0x7f.
@eustas commented on GitHub (Dec 23, 2016):
About static_dict: it is masked with 0x7F to filter out highest bit. Going to add assert + comment + expand kBrotliDictionarySizeBitsByLength and make mask more narrow; just to be on a safe side.
@eustas commented on GitHub (Dec 23, 2016):
As for other cases: this never can happen, according to BL (take a look at
EmitXXXfunctions).We mask with 0xFF for performance reasons (it turns to
movzblinstead of real masking).Going to add asserts to calm down VC++
@randomascii commented on GitHub (Dec 27, 2016):
Thanks for the explanation. VC++ gives many warnings in its /analyze mode and I ignore many of them, so I am fine with ignoring these as well. However they pointed out constructs which were suspicious - which looked wrong to a human eye. Comments to explain that the mask amounts are in fact correct (and why) would be sufficient I think. Suppressing /analyze warnings is a black art and making the code clear for human readers is more important, IMHO.
@eustas commented on GitHub (Jan 26, 2017):
Should be fixed with #497