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

Forum rules
Please don't bump threads here if you have a problem - it will often be forgotten about if you do. Instead, make a new thread here.

Post a reply

Smilies
:D :) :( :o :shock: :? 8-) :lol: :x :P :oops: :cry: :evil: :twisted: :roll: :wink: :geek: :ugeek: :!: :?: :idea: :arrow: :| :mrgreen: :3: :wub: >:( :blergh:
View more smilies

BBCode is OFF
Smilies are ON

Topic review
   

Expand view Topic review: [??-gdd7ec1fe4] Use after free in DoomRPG SE Titlemap

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

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.

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

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.

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

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.

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

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.

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

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!

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

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:
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 all

gzdoom -iwad DOOM2.WAD -file *path to DoomRPG*/DoomRPG
and let it go without doing anything.

Top