lvandeve / lodepng

PNG encoder and decoder in C and C++.
zlib License
2.04k stars 420 forks source link

UB sanitizer catches left shifts that overflow signed int #117

Closed pinobatch closed 4 years ago

pinobatch commented 4 years ago

Inspired by a Tweet by John Regehr, I decided to try compiling a simple LodePNG test program with GCC's undefined behavior sanitizer. Both GCC 7 and GCC 8 catch an undefined behavior that the fix for #91 may not have caught.

Software versions used:

Steps to reproduce:

Place this sample.png in a cloned tree sample and then run the following:

echo '#include "lodepng.cpp"' > lodepng.c
gcc -Wall -Wextra -O -DLODEPNG_NO_COMPILE_ENCODER examples/example_decode.c lodepng.c -o nosan && ./nosan sample.png
gcc -Wall -Wextra -O -fsanitize=address,undefined -DLODEPNG_NO_COMPILE_ENCODER examples/example_decode.c lodepng.c -o withsan && ./withsan sample.png

Expected result: No output.

Actual result: The following warning messages.

lodepng.cpp:565:102: runtime error: left shift of 184 by 24 places cannot be represented in type 'int'
lodepng.cpp:586:59: runtime error: left shift of 186 by 24 places cannot be represented in type 'int'
lodepng.cpp:584:102: runtime error: left shift of 162 by 24 places cannot be represented in type 'int'
lvandeve commented 4 years ago

LodePNG does get tested and fuzzed with both address and undefined behavior sanitizer, interestingly this case did not come up before. Added a TODO in the test to add coverage for this.

Also fixed the issue: C has the "interesting" behavior that when you combine an unsigned char with an unsigned int, it outputs a signed int (and also, C's behavior on unsigned ints is well defined but not on signed ones). Fixed with unsigned casts in better location.