search

mupen64plus / mupen64plus-video-glide64mk2

Video plugin for Mupen64Plus 2.0 based on 10th anniversary release code from gonetz
28 stars 37 forks source link

Please use regression test with GCC/Clang sanitizer #58

Closed ghost closed 9 years ago

ghost commented 9 years ago

I've already reported multiple times that glide64mk2 has numerous buffer overflows and underflows which result in random crashes. And reverts like c025a891a142552d45e5bb11583a5b8f6fedb5d3 just add more again (see #54).

Please use the GCC/Clang sanitizers in your regression tests to get informed by them more easily. To build with them you can use

CFLAGS="-g3 -fsanitize=undefined -fsanitize=address" ./m64p_build.sh INSTALL_STRIP_FLAG="" OPTFLAGS=""
richard42 commented 9 years ago

conchurnavid, I ran some tests with the sanitizer settings that you gave. It required gcc 4.9 to compile. It did find some illegal memory accesses, which I fixed in 129ab88.

I examined the code which you patched in #51 (in Glide64/TexLoad4b.h) and the texture loading functions which it calls. Your patches are unnecessary. It is logically acceptable for the "ext" value to be negative. This value is in units of bytes and is used to foward the destination pointer from it's ending position after outputting one line, to the beginning position of the next line. The only potential illegal memory access which might be executed by this code is if the last line of the destination buffer is too short. The other lines can be smaller than the number of bytes written per line (which would cause negative "ext" values), but the last one has to be long enough to hold the bytes written. The texture loading functions always write the output lines in groups of 16 pixels.

But the sanitizer didn't complain about this (at least in my kirby64 test, which I know has negative ext values because your patch caused the visual artifacts) so the destination buffer must be big enough.

It did throw a warning about "../../src/Glide64/TexLoad4b.h:599:42: runtime error: left shift of negative value -8" but this is overly pedantic and of no practical consequence. Mupen64Plus is not going to support execution on machines which do not use twos complement arithmetic.

BTW, this texture loading code is quite bad. It looks like the original author translated it from some assembly language and didn't take the time to clean it up and make it nice.

ghost commented 9 years ago

See https://code.google.com/p/mupen64plus/issues/detail?id=579#c20 for the test to reproduce the crash

richard42 commented 9 years ago

thanks for the link. I just pushed a proper fix for this in changeset 15f1f88. I looked through all the texture loading routines in the TexLoad4/8/16/32.h files and this class of error was only present in the one function.