[b84f7bc]CheckWeaponChange null VM Abort

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: [b84f7bc]CheckWeaponChange null VM Abort

Re: [b84f7bc]CheckWeaponChange null VM Abort

by Graf Zahl » Sun May 14, 2017 9:26 am

It doesn't matter where the overlays are, they are all ticked from TickPSprites, and that was were things went wrong.

Re: [b84f7bc]CheckWeaponChange null VM Abort

by Major Cooke » Sun May 14, 2017 7:43 am

A quick note, the overlays are on the player actors themselves, not the weapons. I know specifics weren't specified.

Re: [b84f7bc]CheckWeaponChange null VM Abort

by Graf Zahl » Sun May 14, 2017 4:34 am

Goddamnit. What's up with the editor? It's not the first time it removed some tildes on its own.

Re: [b84f7bc]CheckWeaponChange null VM Abort

by Edward-san » Sun May 14, 2017 4:23 am

Could this line actually be a typo? Shouldn't it be "thing->flags &= ~MF_PICKUP;" (missing ~)?

Re: [b84f7bc]CheckWeaponChange null VM Abort

by Graf Zahl » Sun May 14, 2017 3:58 am

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.

Re: [b84f7bc]CheckWeaponChange null VM Abort

by Graf Zahl » Sun May 14, 2017 3:43 am

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.

Re: [b84f7bc]CheckWeaponChange null VM Abort

by _mental_ » Sun May 14, 2017 2:53 am

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.

Re: [b84f7bc]CheckWeaponChange null VM Abort

by Graf Zahl » Sun May 14, 2017 2:43 am

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

by _mental_ » Sun May 14, 2017 2:30 am

It will crash only if you will add the following check to PlayerPawn.CheckWeaponChange() and PlayerPawn.CheckWeaponFire():

Code: Select all

if (!player) return;
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.

Re: [b84f7bc]CheckWeaponChange null VM Abort

by Graf Zahl » Sun May 14, 2017 2:18 am

Hard to say because I don't get it to crash.

Re: [b84f7bc]CheckWeaponChange null VM Abort

by _mental_ » Sun May 14, 2017 1:30 am

I tried to investigate the problem but would like to ask Graf to take a look.
Spoiler: Callstack for correct morph
Spoiler: Difference in callstack for morph that aborts VM
I 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"
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

[b84f7bc]CheckWeaponChange null VM Abort

by Major Cooke » Sat May 13, 2017 10:33 am

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.

Top