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

Bugs that have been investigated and resolved somehow.

Moderator: GZDoom Developers

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

Postby Edward-san » Sat Apr 27, 2019 11: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:


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.
Edward-san
Mathematics is the language with which God has written the universe. (Galilei)
 
Joined: 17 Oct 2009

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

Postby Zhs2 » Sat Apr 27, 2019 8: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!
User avatar
Zhs2
Power of meh.
 
Joined: 07 Nov 2008
Location: Maryland, USA, but probably also in someone's mod somewhere
Operating System: Windows 10/8.1/8 64-bit
Graphics Processor: ATI/AMD with Vulkan Support

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

Postby _mental_ » Thu May 09, 2019 9: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.
_mental_
 
 
 
Joined: 07 Aug 2011

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

Postby Graf Zahl » Thu May 09, 2019 10:34 am

Best implement it and let it be reviewed. Currently I cannot do anything about it.
User avatar
Graf Zahl
Lead GZDoom Developer
Lead GZDoom Developer
 
Joined: 19 Jul 2003
Location: Germany

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

Postby _mental_ » Fri May 10, 2019 4: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.
_mental_
 
 
 
Joined: 07 Aug 2011

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

Postby Graf Zahl » Fri May 10, 2019 4: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.
User avatar
Graf Zahl
Lead GZDoom Developer
Lead GZDoom Developer
 
Joined: 19 Jul 2003
Location: Germany


Return to Closed Bugs

Who is online

Users browsing this forum: Awario [RSS], MSN [Bot] and 0 guests