Open AliceLR opened 3 years ago
libFuzzer and UBSan found another one of these: the sample clamping used in several functions can result in signed integer overflows with extreme values. No test inputs yet and I'm not sure if this one is even worth patching here, but I ended up doing this locally:
- if ((unsigned int) (v + 32768) > 65535)
+ if (v < -32768 || v > 32767)
v = v < 0 ? -32768 : 32767;
MINGW64/GCC 11.2's x86_64 output is identical with and without the change but it could presumably emit slower code with old compilers or for other architectures.
without testing, i would guess for the latest issue that moving the cast inside the parentheses (so it applies to 'v' only) would fix it
without testing, i would guess for the latest issue that moving the cast inside the parentheses (so it applies to 'v' only) would fix it
This does appear to work as well.
Here are OGGs I pulled out of the fuzz files I have for the clamping warnings. UBSan also emits the FAST_SCALED_FLOAT_TO_INT
warnings for these: OGG_clamp_overflow.tar.gz
lookup1_values
After looking at the spec: since this function just returns the largest integer value less than or equal to the codebook_dimensions
th root of codebook_entries
, it seems like it could be simplified to floor(pow(codebook_entries, 1.0 / codebook_dimensions))
, which I think should never cause UB with correctly bounded entries and dimensions. (In fact, libvorbis seems to do exactly this followed by some integer-based rounding correction.)
Two more minor errors output by UBSan:
1U << ...
: OGG_code_length_32.tar.gz
/home/a/libxmp-testing/OGG_code_length_32/640e58e038dfe66099e135c9e09c43037877ebfe.ogg: stb_vorbis.c:1699:70: runtime error: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior stb_vorbis.c:1699:70 in
entries
* dimensions
>= 2^31 cause a signed overflow at line 3860. The output range of this operation is 40 bits (24-bit entries
, 16-bit dimensions
):
OGG_40bit_values.tar.gz/home/a/libxmp-testing/OGG_40bit_values/173c22065af31cbe73c59baea0aa22a0c9be198b.ogg: stb_vorbis.c:3860:43: runtime error: signed integer overflow: 2994923 * 44210 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior stb_vorbis.c:3860:43 in
These issues cropped up during fuzzing sessions for libxmp. I considered submitting a patch but I'm not familiar enough with Vorbis to know how to handle the first and the second is fairly minor.
OGG_lookup1_values.zip
OGG_fast_ftoi.zip
The second issue in
FAST_SCALED_FLOAT_TO_INT
might be fixable without performance penalties by clampingtemp.i
prior to subtracting offADDEND(15)
, but considering the kind of bit hack being used here it might be cleaner to just make them both unsigned.