Download D4D here.
There is a VM abort with mods like D4D occurring whenever picking up demon runes. One of the things I noticed is the null pointer check that was actually doable in the C++ side of things.
This scriptified counterpart does not and cannot from the looks of it, since attempting to put a null pointer check for the player causes a crash since the player is a struct.
The abort happens on this line precisely.
[b84f7bc]CheckWeaponChange null VM Abort
Moderator: GZDoom Developers
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.
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.
-
Major Cooke
- Posts: 8212
- Joined: Sun Jan 28, 2007 3:55 pm
- Preferred Pronouns: He/Him
- Operating System Version (Optional): Windows 10
- Graphics Processor: nVidia with Vulkan support
- Location: GZBoomer Town
- Contact:
Re: [b84f7bc]CheckWeaponChange null VM Abort
I tried to investigate the problem but would like to ask Graf to take a look.
For some reason this works fine when morph happens during APlayerPawn::Tick() but fails if called from P_PlayerThink() function.
I'm using D4D Alpha 5 with these bindings:
Picking up summoned rune will end with one of callstacks above and depending on origin of morph it will work fine or abort with error.
Addition of null checks to scripting side won't solve the problem.
This will rather postpone it to a later moment when the game will crash because of nullptr value of player argument:
Spoiler: Callstack for correct morph
Spoiler: Difference in callstack for morph that aborts VMI guess the issue with the latter is changing of PlayerPawn object, unmorphed one will have player member set to nullptr.
For some reason this works fine when morph happens during APlayerPawn::Tick() but fails if called from P_PlayerThink() function.
I'm using D4D Alpha 5 with these bindings:
Code: Select all
+bind k "summon baronrune"
+bind l "morphme"Addition of null checks to scripting side won't solve the problem.
This will rather postpone it to a later moment when the game will crash because of nullptr value of player argument:
Spoiler: Callstack
-
Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49237
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: [b84f7bc]CheckWeaponChange null VM Abort
Hard to say because I don't get it to crash.
Re: [b84f7bc]CheckWeaponChange null VM Abort
It will crash only if you will add the following check to PlayerPawn.CheckWeaponChange() and PlayerPawn.CheckWeaponFire():
Otherwise it will abort VM with reading NULL address exception. Of course, it will abort if morphing will happen during code path from the second callstack.
For this reason the issue cannot be reproduced with give baronrune and morphme CCMDs. The rune must be picked up during P_PlayerThink() call.
Code: Select all
if (!player) return;
For this reason the issue cannot be reproduced with give baronrune and morphme CCMDs. The rune must be picked up during P_PlayerThink() call.
-
Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49237
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: [b84f7bc]CheckWeaponChange null VM Abort
I don't even get it to abort. What precisely do I have to do to trigger the error?
Re: [b84f7bc]CheckWeaponChange null VM Abort
Load any map with D4D, press K to summon the rune and pick it up.
It doesn't have 100% reproduction rate but it somehow depends on moment when the rune has been picked up.
If morphing has been completed (this means that code path from the first callstack was taken) I press L to unmorph and repeat.
Sometimes the rune spawns in front of player and you need to move to grab it.
But sometimes it spawns too close and so will be picked up in the same or maybe in the next frame. This case aborts VM usually.
It doesn't have 100% reproduction rate but it somehow depends on moment when the rune has been picked up.
If morphing has been completed (this means that code path from the first callstack was taken) I press L to unmorph and repeat.
Sometimes the rune spawns in front of player and you need to move to grab it.
But sometimes it spawns too close and so will be picked up in the same or maybe in the next frame. This case aborts VM usually.
-
Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49237
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: [b84f7bc]CheckWeaponChange null VM Abort
It only happens with some weapons. And from the look of it there's some exceptional fuckery going on here, because the item pickup gets called from some overlay! That's a situation the code isn't prepared for. It worked in older versions because it reused the same player pointer that got initially passed in, but the new code re-retrieves it from the passed actor. And with how morphs work that link got broken in here.
Now I got to find the messed up movement in D4D.
EDIT: A_CheckBlock is the perpetrator. This function calls P_CheckPosition which handles item pickups. Not a good idea to allow this in a supposedly pure checking function.
Now I got to find the messed up movement in D4D.
EDIT: A_CheckBlock is the perpetrator. This function calls P_CheckPosition which handles item pickups. Not a good idea to allow this in a supposedly pure checking function.
-
Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49237
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: [b84f7bc]CheckWeaponChange null VM Abort
I also think that the morphing logic needs some serious evaluation. Having the pickup function morph the player is inherently unsafe so all it should really do is to queue the transition and let it actually be done in a safe place like PlayerThink or Tick.
-
Edward-san
- Posts: 1774
- Joined: Sat Oct 17, 2009 9:40 am
Re: [b84f7bc]CheckWeaponChange null VM Abort
Could this line actually be a typo? Shouldn't it be "thing->flags &= ~MF_PICKUP;" (missing ~)?
-
Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49237
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: [b84f7bc]CheckWeaponChange null VM Abort
Goddamnit. What's up with the editor? It's not the first time it removed some tildes on its own.
-
Major Cooke
- Posts: 8212
- Joined: Sun Jan 28, 2007 3:55 pm
- Preferred Pronouns: He/Him
- Operating System Version (Optional): Windows 10
- Graphics Processor: nVidia with Vulkan support
- Location: GZBoomer Town
- Contact:
Re: [b84f7bc]CheckWeaponChange null VM Abort
A quick note, the overlays are on the player actors themselves, not the weapons. I know specifics weren't specified.
-
Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49237
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: [b84f7bc]CheckWeaponChange null VM Abort
It doesn't matter where the overlays are, they are all ticked from TickPSprites, and that was were things went wrong.