JA: Invalid memory usage fixes and a some minor stuff.

[Civil]

Please don't merge it for now. This PR is intended to track and discuss bad things I've found during compiling game with memory sanitize enabled.

[Civil]

I'm not sure about the bits with color ('Color always have alpha' commit) and would like to hear comments from you about it.

[Civil]

P.S. I also think that at some point you should get rid of ALL false-positives here. Especially the one within Z_IsFromZone is pissing me off.

[Razish]

I occasionally chuck on -fsanitize=address with debug builds, but there are so many issues it was unplayable

[Civil]

Actually I was surprised that I need to fix only two places for MP to work.

About last commit - after 10-15 minutes of playing with bots I've got:

==25518==ERROR: AddressSanitizer: strncpy-param-overlap: memory ranges [0x000002e951c0,0x000002e951d8) and [0x000002e951c0, 0x000002e951d8) overlap
    #0 0x4587be in strncpy /var/tmp/portage/sys-devel/llvm-3.7.0-r4/work/llvm-3.7.0.src/projects/compiler-rt/lib/asan/asan_interceptors.cc:631
    #1 0x592627 in Q_strncpyz(char*, char const*, int) /home/civil/src/OpenJK-civil/codemp/qcommon/q_shared.c:969:2
    #2 0x7ddeef in S_StartBackgroundTrack_Actual(MusicInfo_s*, qboolean_e, char const*, char const*) /home/civil/src/OpenJK-civil/codemp/client/snd_dma.cpp:4101:2
    #3 0x7e2727 in S_UpdateBackgroundTrack_Actual(MusicInfo_s*, qboolean_e, float) /home/civil/src/OpenJK-civil/codemp/client/snd_dma.cpp:4820:6
    #4 0x7dae15 in S_UpdateBackgroundTrack() /home/civil/src/OpenJK-civil/codemp/client/snd_dma.cpp:5034:31
    #5 0x7da206 in S_Update() /home/civil/src/OpenJK-civil/codemp/client/snd_dma.cpp:2785:3
    #6 0x74b3b7 in CL_Frame(int) /home/civil/src/OpenJK-civil/codemp/client/cl_main.cpp:2202:2
    #7 0x5270ba in Com_Frame() /home/civil/src/OpenJK-civil/codemp/qcommon/common.cpp:1562:4
    #8 0x88605f in main /home/civil/src/OpenJK-civil/shared/sys/sys_main.cpp:789:3
    #9 0x7f2c2d9df5af in __libc_start_main (/lib64/libc.so.6+0x205af)
    #10 0x41c9f8 in _start (/home/civil/src/OpenJK/dist/usr/local/JediAcademy/openjk.x86_64+0x41c9f8)

Basically I've done the same as strncpy in glibc do, but in a different order. Memcpy's behavior is defined in C and C++ standard for overlaping strings (as far as I remember)

[Civil]

@Razish well, with this set of patches I was able to play two levels in single player and play 30 minutes with bots in multiplayer, while the game was compiled with Address Sanitizer.

[Civil]

I'll also test Jedi Outcast with address sanitizer, but all the fixes for JO will go to another PR (along with the fixed I've already made in my fork for it).

[Civil]

That's still not all, cause now I'm getting one "INVALID READ" with following trace and I can't find out why:

==32350==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7f168aec06b8 at pc 0x7f1689f756ea bp 0x7fff66daaec0 sp 0x7fff66daaeb8
READ of size 4 at 0x7f168aec06b8 thread T0
    #0 0x7f1689f756e9 in WP_SaberDamageEffects(trace_t*, float const*, float, float, float*, float*, int, saberType_t, saberInfo_t*, int) /home/civil/src/OpenJK-civil/code/game/wp_saber.cpp:2277:10
    #1 0x7f1689f7d7a0 in WP_SaberDamageForTrace(int, float*, float*, float, float*, int, int, saberType_t, int, int, int) /home/civil/src/OpenJK-civil/code/game/wp_saber.cpp:2929:13
    #2 0x7f1689f972bd in WP_SaberDamageTrace(gentity_s*, int, int) /home/civil/src/OpenJK-civil/code/game/wp_saber.cpp:5006:10
    #3 0x7f1689fa2e11 in WP_SabersDamageTrace(gentity_s*, int) /home/civil/src/OpenJK-civil/code/game/wp_saber.cpp:5812:3
    #4 0x7f1689c21016 in ClientEvents(gentity_s*, int) /home/civil/src/OpenJK-civil/code/game/g_active.cpp:1832:5
    #5 0x7f1689c5708b in ClientThink_real(gentity_s*, usercmd_s*) /home/civil/src/OpenJK-civil/code/game/g_active.cpp:5481:2
    #6 0x7f1689c5a6eb in ClientThink(int, usercmd_s*) /home/civil/src/OpenJK-civil/code/game/g_active.cpp:5696:3
    #7 0x63fb80 in SV_ClientThink(client_s*, usercmd_s*) /home/civil/src/OpenJK-civil/code/server/sv_client.cpp:399:2
    #8 0x640c89 in SV_UserMove(client_s*, msg_t*) /home/civil/src/OpenJK-civil/code/server/sv_client.cpp:520:3
    #9 0x63fce0 in SV_ExecuteClientMessage(client_s*, msg_t*) /home/civil/src/OpenJK-civil/code/server/sv_client.cpp:565:4
    #10 0x64d225 in SV_PacketEvent(netadr_s, msg_t*) /home/civil/src/OpenJK-civil/code/server/sv_main.cpp:348:5
    #11 0x5d1f8c in Com_RunAndTimeServerPacket(netadr_s*, msg_t*) /home/civil/src/OpenJK-civil/code/qcommon/common.cpp:815:2
    #12 0x5d2617 in Com_EventLoop() /home/civil/src/OpenJK-civil/code/qcommon/common.cpp:854:6
    #13 0x5d4b53 in Com_Frame() /home/civil/src/OpenJK-civil/code/qcommon/common.cpp:1369:20
    #14 0x7570ed in main /home/civil/src/OpenJK-civil/shared/sys/sys_main.cpp:789:3
    #15 0x7f16b06735af in __libc_start_main (/lib64/libc.so.6+0x205af)
    #16 0x41cf98 in _start (/home/civil/src/OpenJK/dist/usr/local/JediAcademy/openjk_sp.x86_64+0x41cf98)

0x7f168aec06b8 is located 8 bytes to the left of global variable 'SaberParms' defined in '/home/civil/src/OpenJK-civil/code/game/wp_saberLoad.cpp:39:6' (0x7f168aec06c0) of size 1048576
0x7f168aec06b8 is located 52 bytes to the right of global variable 'sabersCrossed' defined in '/home/civil/src/OpenJK-civil/code/game/wp_saber.cpp:48:14' (0x7f168aec0680) of size 4
SUMMARY: AddressSanitizer: global-buffer-overflow /home/civil/src/OpenJK-civil/code/game/wp_saber.cpp:2277:10 in WP_SaberDamageEffects(trace_t*, float const*, float, float, float*, float*, int, saberType_t, saberInfo_t*, int)
Shadow bytes around the buggy address:
  0x0fe3515d0080: 00 00 00 00 00 00 00 00 00 00 00 00 f9 f9 f9 f9
  0x0fe3515d0090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe3515d00a0: 00 00 00 00 00 00 00 00 f9 f9 f9 f9 00 00 00 00
  0x0fe3515d00b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe3515d00c0: 00 00 00 00 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
=>0x0fe3515d00d0: 04 f9 f9 f9 f9 f9 f9[f9]00 00 00 00 00 00 00 00
  0x0fe3515d00e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe3515d00f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe3515d0100: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe3515d0110: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe3515d0120: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==32350==ABORTING

INVALID READ happens when it's getting hitLoc[hitEntNum[numHitEnts]]

Found out that hitEntNum [numHitEnts] for some reason was 350.

[Civil]

I've also added SIGFPE fix to this PR, cause it's really simple and also happens while I was playing a game, testing other fixes. Though it's not related to sanitize-address.

That's I think all I was able to find during quick playing with AddressSanitizer in JASP and JAMP.

[ensiform]

Rejecting changes to Q_strncpyz. It is up to the user to not use it in overlap cases. Also strnlen is not standard, even if it is supported by gcc, clang and msvc.

[Civil]

@ensiform well, what's behavior of strncpy in overlapping cases? Because JA Engine have them. The thing is that behavior of compiles can be different in that case, and for example on Windows, Mac OS X and Linux you may get 3 different behaviors of something (I really haven't investigated what was the strings it tried to copy, cause ASAN won't provide that info). So it's either you should make Q_strncpyz overlapping-safe or fix all cases when it happens. If you decide to ignore it and leave as-is you most likely will get platform dependent bugs.

[ensiform]

You fix all areas where it happens. They've been fixed last I checked as you didn't point any specific ones out.

[Civil]

Well, ok, I'll revert srncpyz changes, and will just open new bugs about all areas where I get overlapping strings then, cause if they are not accidentally fixed by this PR, there are some, cause I've definitely got some errors related to that during playing the game.

[ensiform]

This is how its been fixed in the past.

[ensiform]

Do you really need to open bugs constantly for every single thing it finds instantly?

[Civil]

@ensiform well, if you have another way to tell that there is a bug, and that's debug info I was able to get - then tell me. I'm opening bug per thing I don't know how to fix myself. Things that I know how to fix, I've fixed in this PR.

[ensiform]

Well try to hold off until you can verify they're actually bugs I guess? I'm still not convinced there isn't a major memory issue causing a lot of extra problems. NaN issues are already posted for example, just not on the first page.

[Civil]

@ensiform ok.

[Civil]

Gr... Looks like I've deleted some of useful commits. I'll restore them tomorrow and also will get rid of Q_strncpyz.