JA: Invalid memory usage fixes and a some minor stuff. by Civil · Pull Request #785 · JACoders/OpenJK

Civil · 2016-01-07T00:59:08Z

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.

* Add NO_SANITIZE_ADDRESS macro that allows to pass attribute * Add NO_SANITIZE attribute to Z_IsFromZone and VM_DllSyscall functions because they are doing nasty things with memory.

Even though vec3_t is an array of 3 elements, sizeof(vec3_t) returns correct value. So no need to allocate 3x space and copy it 3x times.

Civil · 2016-01-07T01:40:13Z

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 · 2016-01-07T14:59:01Z

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 · 2016-01-08T17:31:59Z

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

Civil · 2016-01-09T02:13:33Z

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 · 2016-01-09T02:28:09Z

@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 · 2016-01-09T02:32:15Z

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 · 2016-01-10T20:25:25Z

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 · 2016-01-10T22:21:04Z

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 · 2016-01-10T22:40:19Z

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 · 2016-01-10T22:51:15Z

@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.

We assume that color is rgba, but for flares a was garbage

We are have only sTemp - strlen(sTemp) bytes left, but we are copying up to sizeof(sTemp) bytes which will overwrite strlen(sTemp) bytes of memory in some rare cases.

Same thing as in single player

ensiform · 2016-01-10T22:58:07Z

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

And this sometimes (rarely) happens. AddressSanitizer says on this 'strncpy-param-overlap'.

Civil · 2016-01-10T23:03:00Z

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 · 2016-01-10T23:07:05Z

This is how its been fixed in the past.

ensiform · 2016-01-10T23:07:33Z

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

Civil · 2016-01-10T23:09:17Z

@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 · 2016-01-10T23:11:00Z

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 · 2016-01-10T23:19:19Z

@ensiform ok.

Civil · 2016-01-10T23:38:36Z

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

Civil added 3 commits January 7, 2016 01:50

Fix false-positives for address sanitizer.

8fba9ec

* Add NO_SANITIZE_ADDRESS macro that allows to pass attribute * Add NO_SANITIZE attribute to Z_IsFromZone and VM_DllSyscall functions because they are doing nasty things with memory.

Fix Invalid Read while reading savegame

87abc4e

Even though vec3_t is an array of 3 elements, sizeof(vec3_t) returns correct value. So no need to allocate 3x space and copy it 3x times.

Fix another False-positive for ASAN

f8bf359

Civil changed the title ~~Mem fixes~~ JA: Invalid memory usage fixes Jan 7, 2016

Civil changed the title ~~JA: Invalid memory usage fixes~~ JA: Invalid memory usage fixes and a some minor stuff. Jan 10, 2016

Civil added 3 commits January 10, 2016 23:57

Color always have alpha

19d7679

We assume that color is rgba, but for flares a was garbage

Fix invalid write in rare cases during savegame

0491b5f

We are have only sTemp - strlen(sTemp) bytes left, but we are copying up to sizeof(sTemp) bytes which will overwrite strlen(sTemp) bytes of memory in some rare cases.

Color always have alpha even in MP

262d565

Same thing as in single player

strncpy behavior is undefined for overlaping strings

b0964ab

And this sometimes (rarely) happens. AddressSanitizer says on this 'strncpy-param-overlap'.

Civil force-pushed the mem-fixes branch from 28c6a28 to 023c16d Compare January 10, 2016 22:58

Civil force-pushed the mem-fixes branch from 023c16d to b0964ab Compare January 10, 2016 23:21

Conversation

Civil commented Jan 7, 2016

Uh oh!

Civil commented Jan 7, 2016

Uh oh!

Civil commented Jan 7, 2016

Uh oh!

Razish commented Jan 8, 2016

Uh oh!

Civil commented Jan 9, 2016

Uh oh!

Civil commented Jan 9, 2016

Uh oh!

Civil commented Jan 9, 2016

Uh oh!

Civil commented Jan 10, 2016

Uh oh!

Civil commented Jan 10, 2016

Uh oh!

ensiform commented Jan 10, 2016

Uh oh!

Civil commented Jan 10, 2016

Uh oh!

ensiform commented Jan 10, 2016

Uh oh!

Civil commented Jan 10, 2016

Uh oh!

ensiform commented Jan 10, 2016

Uh oh!

ensiform commented Jan 10, 2016

Uh oh!

Civil commented Jan 10, 2016

Uh oh!

ensiform commented Jan 10, 2016

Uh oh!

Civil commented Jan 10, 2016

Uh oh!

Civil commented Jan 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants