[??-gdd7ec1fe4] Use after free in DoomRPG SE Titlemap

Post by **Edward-san** » Sat Apr 27, 2019 10:16 am

When I load GZDoom with Doom RPG SE (mod git hash df7af0dd) and leave the titlemap run, there's a certain moment when this happens (if the Address sanitizer is used):

Spoiler:

Code: Select all

==24307==ERROR: AddressSanitizer: heap-use-after-free on address 0x61a000d36b58 at pc 0x555556dedb72 bp 0x7fffffff5770 sp 0x7fffffff5760
READ of size 4 at 0x61a000d36b58 thread T0
    #0 0x555556dedb71 in FActorIterator::Next() /home/edward-san/zdoom/gzdoom/trunk/src/./actor.h:1457
    #1 0x555556f2abaa in DLevelScript::SetActorProperty(int, int, int) /home/edward-san/zdoom/gzdoom/trunk/src/p_acs.cpp:4079
    #2 0x555556f75274 in DLevelScript::RunScript() /home/edward-san/zdoom/gzdoom/trunk/src/p_acs.cpp:9639
    #3 0x555556f25520 in DACSThinker::Tick() /home/edward-san/zdoom/gzdoom/trunk/src/p_acs.cpp:3421
    #4 0x55555736d076 in DThinker::CallTick() /home/edward-san/zdoom/gzdoom/trunk/src/g_shared/dthinker.cpp:854
    #5 0x55555736b07f in FThinkerList::TickThinkers(FThinkerList*) /home/edward-san/zdoom/gzdoom/trunk/src/g_shared/dthinker.cpp:575
    #6 0x555557368ab8 in FThinkerCollection::RunThinkers(FLevelLocals*) /home/edward-san/zdoom/gzdoom/trunk/src/g_shared/dthinker.cpp:114
    #7 0x555557197081 in P_Ticker() /home/edward-san/zdoom/gzdoom/trunk/src/p_tick.cpp:154
    #8 0x555556ecaaa8 in G_Ticker() /home/edward-san/zdoom/gzdoom/trunk/src/g_game.cpp:1197
    #9 0x555556e8ea29 in TryRunTics() /home/edward-san/zdoom/gzdoom/trunk/src/d_net.cpp:1984
    #10 0x555556e75d60 in D_DoomLoop() /home/edward-san/zdoom/gzdoom/trunk/src/d_main.cpp:1022
    #11 0x555556e7e92d in D_DoomMain() /home/edward-san/zdoom/gzdoom/trunk/src/d_main.cpp:2693
    #12 0x555555e90d38 in main /home/edward-san/zdoom/gzdoom/trunk/src/posix/sdl/i_main.cpp:263
    #13 0x7ffff50e2b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
    #14 0x555555e81e09 in _start (/home/edward-san/zdoom/gzdoom/trunk/debug-asan/gzdoom+0x92de09)

0x61a000d36b58 is located 728 bytes inside of 1360-byte region [0x61a000d36880,0x61a000d36dd0)
freed by thread T0 here:
    #0 0x7ffff6ef77a0 in free (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xed7a0)
    #1 0x555557ca6943 in M_Free(void*) /home/edward-san/zdoom/gzdoom/trunk/src/utility/m_alloc.cpp:208
    #2 0x555556ded720 in DObject::operator delete(void*) /home/edward-san/zdoom/gzdoom/trunk/src/dobject.h:279
    #3 0x5555570ec7c7 in AActor::~AActor() /home/edward-san/zdoom/gzdoom/trunk/src/p_mobj.cpp:180
    #4 0x555556eaa894 in SweepList /home/edward-san/zdoom/gzdoom/trunk/src/dobjgc.cpp:214
    #5 0x555556eab25d in SingleStep /home/edward-san/zdoom/gzdoom/trunk/src/dobjgc.cpp:371
    #6 0x555556eab3f5 in GC::Step() /home/edward-san/zdoom/gzdoom/trunk/src/dobjgc.cpp:413
    #7 0x555557368194 in CheckGC /home/edward-san/zdoom/gzdoom/trunk/src/./dobjgc.h:110
    #8 0x55555736b0ca in FThinkerList::TickThinkers(FThinkerList*) /home/edward-san/zdoom/gzdoom/trunk/src/g_shared/dthinker.cpp:577
    #9 0x555557368ab8 in FThinkerCollection::RunThinkers(FLevelLocals*) /home/edward-san/zdoom/gzdoom/trunk/src/g_shared/dthinker.cpp:114
    #10 0x555557197081 in P_Ticker() /home/edward-san/zdoom/gzdoom/trunk/src/p_tick.cpp:154
    #11 0x555556ecaaa8 in G_Ticker() /home/edward-san/zdoom/gzdoom/trunk/src/g_game.cpp:1197
    #12 0x555556e8ea29 in TryRunTics() /home/edward-san/zdoom/gzdoom/trunk/src/d_net.cpp:1984
    #13 0x555556e75d60 in D_DoomLoop() /home/edward-san/zdoom/gzdoom/trunk/src/d_main.cpp:1022
    #14 0x555556e7e92d in D_DoomMain() /home/edward-san/zdoom/gzdoom/trunk/src/d_main.cpp:2693
    #15 0x555555e90d38 in main /home/edward-san/zdoom/gzdoom/trunk/src/posix/sdl/i_main.cpp:263
    #16 0x7ffff50e2b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)

previously allocated by thread T0 here:
    #0 0x7ffff6ef7b60 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xedb60)
    #1 0x555557ca6785 in M_Malloc_Dbg(unsigned long, char const*, int) /home/edward-san/zdoom/gzdoom/trunk/src/utility/m_alloc.cpp:137
    #2 0x555556eaf0bd in PClass::CreateNew() /home/edward-san/zdoom/gzdoom/trunk/src/dobjtype.cpp:452
    #3 0x555556e170b5 in FLevelLocals::CreateThinker(PClass*, int) /home/edward-san/zdoom/gzdoom/trunk/src/./g_levellocals.h:411
    #4 0x55555712167a in AActor::StaticSpawn(FLevelLocals*, PClassActor*, TVector3<double> const&, replace_t, bool) /home/edward-san/zdoom/gzdoom/trunk/src/p_mobj.cpp:4547
    #5 0x555557121fe9 in AF_AActor_Spawn /home/edward-san/zdoom/gzdoom/trunk/src/p_mobj.cpp:4561
    #6 0x555557a20dbb in VMNativeFunction::NativeScriptCall(VMFunction*, VMValue*, int, VMReturn*, int) /home/edward-san/zdoom/gzdoom/trunk/src/scripting/vm/vmframe.cpp:304
    #7 0x7fffd9dbf44e  (<unknown module>)

SUMMARY: AddressSanitizer: heap-use-after-free /home/edward-san/zdoom/gzdoom/trunk/src/./actor.h:1457 in FActorIterator::Next()
Shadow bytes around the buggy address:
  0x0c348019ed10: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c348019ed20: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c348019ed30: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c348019ed40: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c348019ed50: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c348019ed60: fd fd fd fd fd fd fd fd fd fd fd[fd]fd fd fd fd
  0x0c348019ed70: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c348019ed80: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c348019ed90: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c348019eda0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c348019edb0: fd fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa
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
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  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

full backtrace:

Spoiler:

Code: Select all

#6  0x0000555556dedb72 in FActorIterator::Next (this=0x7fffffff5800) at /home/edward-san/zdoom/gzdoom/trunk/src/./actor.h:1457
No locals.
#7  0x0000555556f2abab in DLevelScript::SetActorProperty (this=0x611000b0d780, tid=1417, property=16, value=1) at /home/edward-san/zdoom/gzdoom/trunk/src/p_acs.cpp:4079
        actor = 0x7fffffffbdb0
        iterator = {TIDHash = 0x5555591ad5b8 <level+1112>, base = 0x61a000d36880, id = 1417}
#8  0x0000555556f75275 in DLevelScript::RunScript (this=0x611000b0d780) at /home/edward-san/zdoom/gzdoom/trunk/src/p_acs.cpp:9639
        pcd = 245
        sp = @0x7fffffffbd70: 3
        pc = 0x7fffd1ca14ac
        fmt = ACS_Enhanced
        runaway = 269
        optstart = -1
        temp = -33208
        Stack = @0x7fffffff7d70: {buffer = {1417, 16, 1, 0, 0, 0, 1874048, 25024, 4232176, 24688, 20, 0, 1948872, 25248, 408716, 1, 34, 0, 0, 1000, 0, 9, 2, 64531145, -524288, 1874048, 25024, -33424, 32767, 2, 0, 1421272, 25264, 2266640, 1, 155, -33104, 266701096, 12296, 8, 2, 0, 0, 0, 0, 0, 1494930104, 21845, 1102416563, 0, 1476016174, 21845, 1442988392, 65536, -33136, 32767, -33040, 32767, 1443072515, 21845, -883540790, -1069566319, -33072, 65536, 1102416563, 0, 1476017376, 21845, 1443072358, 21845, -217770252, -1063579364, 65536, 1076494336, -32976, 32767, -31752, 32767, -33072, 32767, -32864, 32767, 1460556131, 21845, -33008, 32767, -59438080, 1775085954, -33008, 32767, -59438080, 1775085954, -32912, 32767, 1, 0, -4082, 4095, -32656, 32767, 0, 0, 1, 0, -32960, 32767, 1472883012, 21845, 1457631780, 21845, 13818800, 24608, 16859136, 0, -31280, 32767, -32928, 32767, 1459577938, 21845, 0, 1076494336, -31384, 32767, -32896, 32767, 1459573528, 21845, -32656, 32767, -32024, 32767, -32864, 32767, 1459577470, 21845, 1, 0, -32048, 32767, -31232, 32767, 1460249487, 21845, -883540790, -1069566319, -217770252, -1063579364, 0, 1077936128, 16859136, 1075838976, 14303580, 24992, -32656, 32767, 0, 0, -21744, 32767, -24112, 32767, 14303360, 24992, 1463020550, 21845, 1459687146, 0, 0, 0, 0, 1075838976, -790371264, 32767, 0, 1078198272, 13627392, 24784, 0, 1064, 0, 0, -32400, 32767, -32624, 32767, 7956608, 24992, -387816017, 1079052828, -2077427360, -1062830699, 1102416563, 0, 1477335360, 21845, 1460239389, 21845, 20, 0...}}
        savedActiveBehavior = 0x61c0001c9880
        work = {Chars = 0x55555890d42c <FString::NullString+12> "", static NullString = {Len = 0, AllocLen = 2, RefCount = 129331, Nothing = "\000"}}
        lookup = 0x7fffffff5bf0 ""
        controller = 0x60b0006ff8e0
        locals = {memory = 0x6070003d9b90, count = 20}
        noarrays = {Count = 0, Info = 0x0}
        localarrays = 0x62a0001ded90
        activeFunction = 0x0
        translation = 0x0
        resultValue = 1
        __PRETTY_FUNCTION__ = "int DLevelScript::RunScript()"
        specialargmask = -1
        stackobj = {buffer = {buffer = {1417, 16, 1, 0, 0, 0, 1874048, 25024, 4232176, 24688, 20, 0, 1948872, 25248, 408716, 1, 34, 0, 0, 1000, 0, 9, 2, 64531145, -524288, 1874048, 25024, -33424, 32767, 2, 0, 1421272, 25264, 2266640, 1, 155, -33104, 266701096, 12296, 8, 2, 0, 0, 0, 0, 0, 1494930104, 21845, 1102416563, 0, 1476016174, 21845, 1442988392, 65536, -33136, 32767, -33040, 32767, 1443072515, 21845, -883540790, -1069566319, -33072, 65536, 1102416563, 0, 1476017376, 21845, 1443072358, 21845, -217770252, -1063579364, 65536, 1076494336, -32976, 32767, -31752, 32767, -33072, 32767, -32864, 32767, 1460556131, 21845, -33008, 32767, -59438080, 1775085954, -33008, 32767, -59438080, 1775085954, -32912, 32767, 1, 0, -4082, 4095, -32656, 32767, 0, 0, 1, 0, -32960, 32767, 1472883012, 21845, 1457631780, 21845, 13818800, 24608, 16859136, 0, -31280, 32767, -32928, 32767, 1459577938, 21845, 0, 1076494336, -31384, 32767, -32896, 32767, 1459573528, 21845, -32656, 32767, -32024, 32767, -32864, 32767, 1459577470, 21845, 1, 0, -32048, 32767, -31232, 32767, 1460249487, 21845, -883540790, -1069566319, -217770252, -1063579364, 0, 1077936128, 16859136, 1075838976, 14303580, 24992, -32656, 32767, 0, 0, -21744, 32767, -24112, 32767, 14303360, 24992, 1463020550, 21845, 1459687146, 0, 0, 0, 0, 1075838976, -790371264, 32767, 0, 1078198272, 13627392, 24784, 0, 1064, 0, 0, -32400, 32767, -32624, 32767, 7956608, 24992, -387816017, 1079052828, -2077427360, -1062830699, 1102416563, 0, 1477335360, 21845, 1460239389, 21845, 20, 0...}}, sp = 3, next = 0x0, prev = 0x0, static head = 0x7fffffff7d70}
#9  0x0000555556f25521 in DACSThinker::Tick (this=0x60b0006ff8e0) at /home/edward-san/zdoom/gzdoom/trunk/src/p_acs.cpp:3421
        next = 0x611000b00bc0
        script = 0x611000b0d780
#10 0x000055555736d077 in DThinker::CallTick (this=0x60b0006ff8e0) at /home/edward-san/zdoom/gzdoom/trunk/src/g_shared/dthinker.cpp:854
        VIndex = 1
        __PRETTY_FUNCTION__ = "void DThinker::CallTick()"
        clss = 0x60e0000020a0
        func = 0x0
#11 0x000055555736b080 in FThinkerList::TickThinkers (this=0x5555591ae898 <level+5944>, dest=0x0) at /home/edward-san/zdoom/gzdoom/trunk/src/g_shared/dthinker.cpp:575
        count = 1
        node = 0x60b0006ff8e0
#12 0x0000555557368ab9 in FThinkerCollection::RunThinkers (this=0x5555591ae560 <level+5120>, Level=0x5555591ad160 <level>) at /home/edward-san/zdoom/gzdoom/trunk/src/g_shared/dthinker.cpp:114
        i = 103
        count = 4095
#13 0x0000555557197082 in P_Ticker () at /home/edward-san/zdoom/gzdoom/trunk/src/p_tick.cpp:154
        it = {<FThinkerIterator> = {m_ParentType = 0x60e000002260, Level = 0x5555591ad160 <level>, m_CurrThinker = 0x60f0002afac0, m_Stat = 32 ' ', m_SearchStats = true, m_SearchingFresh = false}, <No data fields>}
        ac = 0x0
        Level = 0x5555591ad160 <level>
        __for_range = @0x7fffffffc420: {Array = 0x5555586d6ee0 <primaryLevel>, Count = 1}
        __for_begin = {<std::iterator<std::random_access_iterator_tag, FLevelLocals*, long, FLevelLocals**, FLevelLocals*&>> = {<No data fields>}, m_ptr = 0x5555586d6ee0 <primaryLevel>}
        __for_end = {<std::iterator<std::random_access_iterator_tag, FLevelLocals*, long, FLevelLocals**, FLevelLocals*&>> = {<No data fields>}, m_ptr = 0x5555586d6ee8}
        i = 8
#14 0x0000555556ecaaa9 in G_Ticker () at /home/edward-san/zdoom/gzdoom/trunk/src/g_game.cpp:1197
        i = 8
        oldgamestate = GS_TITLELEVEL
        buf = 8
        rngsum = 3292483865
#15 0x0000555556e8ea2a in TryRunTics () at /home/edward-san/zdoom/gzdoom/trunk/src/d_net.cpp:1984
        i = 8
        lowtic = 1857
        realtics = 22
        availabletics = 17
        counts = 12
        numplaying = 1
        doWait = true
#16 0x0000555556e75d61 in D_DoomLoop () at /home/edward-san/zdoom/gzdoom/trunk/src/d_main.cpp:1022
        lasttic = 1840

the ACS script call happens inside the ACS script "PissOffMarines" in DoomRPG/scripts/Outpost.c . I believe the specific call is this one, since it seems the only place where SetActorProperty with these parameters is called with the InTitle condition true.

Steps to reproduce:

Code: Select all

gzdoom -iwad DOOM2.WAD -file *path to DoomRPG*/DoomRPG

and let it go without doing anything.

Zhs2 · Post by **Zhs2** » Sat Apr 27, 2019 7:56 pm

I've only seen the titlemap crash once with a VFE, never been able to reproduce it again. Glad to see someone found the reason why!

Post by **_mental_** » Thu May 09, 2019 8:35 am

The crash is caused by a random spawner with TID assigned that doesn't spawn an actor. It boils down to a wrong order of function calls.
AActor::OnDestroy() which can remove actor from TID hash is called before setting a new TID which adds an actor to the given hash.

Objects with OF_EuthanizeMe flag set must not be added to TID hash in order to avoid dangling pointers in its linked lists.
This needs to be fixed in at least two places, for scripted spawning and for summon<...> CCMDs.

Although, the design with a writable public AActor::tid member and add/remove functions looks broken to me.
I think that TID should be changed by one function that handles corner cases like this one.
The right approach is ChangeTid() ZScript function.

There should no write access to TID outside of AActor class and things like AddToHash() should be private.
RemoveFromHash() function already exposed to ZScript should be deprecated.

I would like to know other opinions on this topic though.

Post by **Graf Zahl** » Thu May 09, 2019 9:34 am

Best implement it and let it be reviewed. Currently I cannot do anything about it.

Post by **_mental_** » Fri May 10, 2019 3:36 am

Here is a minimum set of changes.

On the second though, almost the same can be achieved by adding check for OF_EuthanizeMe object flag to AActor::AddToHash().
It the flag is set, reset TID to zero and skip linking into the hash.

Post by **Graf Zahl** » Fri May 10, 2019 3:43 am

The 'second thought' should be done as the minimum. A destroyed actor should never be handled in there.

But if your PR makes using these functions unnecessary, all the better.

ZDoom