Page 1 of 1

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

PostPosted: Sat Apr 27, 2019 10:16 am
by Edward-san
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:


full backtrace:

Spoiler:


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 allExpand view
gzdoom -iwad DOOM2.WAD -file *path to DoomRPG*/DoomRPG

and let it go without doing anything.

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

PostPosted: Sat Apr 27, 2019 7:56 pm
by Zhs2
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!

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

PostPosted: Thu May 09, 2019 8:35 am
by _mental_
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.

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

PostPosted: Thu May 09, 2019 9:34 am
by Graf Zahl
Best implement it and let it be reviewed. Currently I cannot do anything about it.

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

PostPosted: Fri May 10, 2019 3:36 am
by _mental_
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.

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

PostPosted: Fri May 10, 2019 3:43 am
by Graf Zahl
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.