Pass -fwrapv to GCC compilation flags

in case you weren't aware, integer overflows and underflows are undefined behavior in C, and that of course means that anything goes with them. that said, though, GCC has been (ab)using this for optimization reasons, which is something that has lead to countless security holes and in the past, and also the well-known (or, at least if you work with this stuff a lot) article "GCC undefined behaviors are getting wild".

the problem, however, is that we rely on integer overflows and underflows a ton. in fact, angle_t actively exploits integer overflows to be able to perform rotations beyond 360 degrees. this puts us in a dangerous spot since it's very possible that GCC can start "optimizing" away important checks which can cause runtime bugs that only happens in release builds -- hell, this might've already happened without any of us being aware of it!

fortunately, there is a switch that fixes this: -fwrapv. it tells GCC to assume that numbers are in two's complement, and optimizes accordingly. this disables some performance optimizations and enables others, but at it's core, it fixes the integer overflow problems and stops any optimization bugs of this category from appearing.

Now, Mac system use clang, so I do we need to update our CMake system and our "dead" XCode project to also include this?

XCode build should now be updated, but i can't verify it since i don't have a Mac. it would be nice if you could verify it if you can.

added 1 commit

• 6d650728 - Update CMake and XCode build systems

Compare with previous version

resolved all threads

if the user just use CC=clang make -C src the binary will not be compiled with `-fwrapv`

Do you want to add check to the Makefile build system for check for clang or should we assume to pass -fwrapv , no matter what compiler is used?

assuming we only support GCC and CLang, then yes, it would be easiest if we just passed -fwrapv unconditionally.

changed this line in version 3 of the diff

added 1 commit

• 80475ada - Always apply -fwrapv on builds

Compare with previous version

approved this merge request

in case you weren't aware, integer overflows and underflows are undefined behavior in C

It is probably important to mention that the statement 'integer overflows and underflows are undefined behavior in C' is referring to signed integers and not unsigned integers. Looking at the C99 standard:

A computation involving unsigned operands can never overflow, because a result that cannot be represented by the resulting unsigned integer type is reduced modulo the number that is one greater than the largest value that can be represented by the resulting type."

https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf#page=46

in fact, angle_t actively exploits integer overflows to be able to perform rotations beyond 360 degrees

angle_t is typedefed to UINT32: https://git.do.srb2.org/STJr/SRB2/-/blob/1470d099e338aff3aad25178894e15bb6d1e8232/src/tables.h#L69 (and UINT32 is defined as uint32_t, which is unsigned)

The GCC docs as well describe -fwrapv as This option instructs the compiler to assume that signed arithmetic overflow of addition, subtraction and multiplication wraps around using twos-complement representation...: https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html

This is just for the sake of clarification and not a comment on the changes themselves. For debugging, there is another flag called -ftrapv which tells gcc to trigger exceptions on integer overflows, ofc there is also UBSAN on glibc (You can use CFLAGS="-fsanitize=address,undefined" LDFLAGS="-fsanitize=address,undefined" make -C src).

Testing it myself with UBSAN I did see signed integer overflows in r_fps.c, p_enemy.c, blua/lvm.c, p_floor.c, p_maputl.c, p_user.c, p_spec.c, st_stuff.c, f_finale.c, etc. with some involving fixed_t, which is signed. Ofc those are best for a separate issue. I also noticed a lot of shift of negative integer errors, which are also UB, when calling these functions: https://git.do.srb2.org/STJr/SRB2/-/blob/1470d099e338aff3aad25178894e15bb6d1e8232/src/v_video.h#L158-170.

If I missed something, feel free to correct me.

Correction: twos-complement integers will become mandatory in C23 but integer overflows will still be undefined behavior and C23 adds extra functions for checking if an overflow has occurred.

merged

mentioned in commit 2cf26236

added Ecosystem label