Forgottenserver: [Crash] Creature::executeConditions

[ninjalulz]

Before creating an issue, please ensure:

• [x] This is a bug in the software that resides in this repository, and not a
  support matter (use https://otland.net/forums/support.16/ for support)
• [x] This issue is reproducible without changes to the code in this repository

Steps to reproduce (include any configuration/script required to reproduce)

1. Use an onPrepareDeath script which removes one condition or more with the help of Creature.removeCondition. However you can bypass the crash if you replace L9 with condition:setTicks(0), but it doesn't remove the condition immediately.
2. Kill your target with a dot (damage-over-time) spell or rune.

Expected behaviour

Removes the condition safely.

Actual behaviour

Server crashes. It might not always crash for you, but it did for me at least.

Stacktrace

http://pastebin.com/cQKYN9kW

Environment

macOS 10.12 (Sierra) / Ubuntu 15.10 (Wily Werewolf).

[wgrr]

i can confirm this.

[gugahoa]

I can't reproduce it no matter how many times I try, could you share the crash report if possible?

[ninjalulz]

@gugahoa You can view it by clicking on the pastebin link above.

[gugahoa]

Ops, I expected a wall of text somewhere in the issue hahaha

[gugahoa]

Could reliable reproduce this: Step on fire field and let the tick damage kill you

[Cjaker]

Crash player.cpp in Player::death(Creature* lastHitCreature)

[joseluis2g]

@Eternal-Scripts How did you reproduce it?

[Cjaker]

@joseluis2g I don't know my friend, my server is online and when i open gdb i see that.

[joseluis2g]

@Eternal-Scripts
If you find out the solution I would be very happy if you share it with me, imma do the same.

[jo3bingham]

@Eternal-Scripts, could you supply a stacktrace to compare against @ninjalulz's? Those screenshots don't really help.

[Cjaker]

@jo3bingham Yes, but i lost the first printscreen (two in total).
(i don't have the bt full saved, only this print left, i will wait to check if have other crash).
Second:

[joseluis2g]

@Eternal-Scripts
are you using the version with mkalo changes?

[Cjaker]

@joseluis2g Yes, i will try crash again in my tfs and i will answer here.

[joseluis2g]

I had lot of logs with the same problem, when im back at home gonna edit this post.

[darkjav]

@joseluis2g @Eternal-Scripts
What mkalo changes do you mean?

[TaigaTenori]

Maybe an old issue, but still open. Tried to reproduce on win 10 with ninja's script and using this .exe https://ci.appveyor.com/project/kornholi/forgottenserver/build/job/8oncjox5thiv0uct/artifacts.

Script was working, I couldn't die. I had a fire condition on me repeatedly from walking onto fire fields, which was being properly removed on supposed death. No crashes

[kito2]

Still happening:

https://pastebin.com/tiKAN2xp

[DSpeichert]

I could reproduce as of 5438d50

[jo3bingham]

I got around to checking this bug report out today. Luckily, it's reproducible 100% of the time, which helped me track down the exact cause. I'll outline it below:

To begin, conditions are executed on a regular interval when checking creatures:
https://github.com/otland/forgottenserver/blob/ba0be848318923edd28f8f8bbfd39ed0c405c34f/src/game.cpp#L3599

The function called iterates through every condition the creature has and tries to execute them:
https://github.com/otland/forgottenserver/blob/ba0be848318923edd28f8f8bbfd39ed0c405c34f/src/creature.cpp#L1286

Because this is a ConditionDamage type condition, and it does damage over a period of time, it calls ConditionDamage::doDamage:
https://github.com/otland/forgottenserver/blob/ba0be848318923edd28f8f8bbfd39ed0c405c34f/src/condition.cpp#L1101
Which calls into Game::combatChangeHealth:
https://github.com/otland/forgottenserver/blob/ba0be848318923edd28f8f8bbfd39ed0c405c34f/src/condition.cpp#L1176

This is where onPrepareDeath events are triggered:
https://github.com/otland/forgottenserver/blob/ba0be848318923edd28f8f8bbfd39ed0c405c34f/src/game.cpp#L4177

You can see where this is going... Because your onPrepareDeath event script calls player:removeCondition, which calls Creature::removeCondition, the same iterator that Creature::executeConditions is currently using is then removed from the condition list:
https://github.com/otland/forgottenserver/blob/ba0be848318923edd28f8f8bbfd39ed0c405c34f/src/creature.cpp#L1254

I'm currently contemplating a proper fix, but others feel free to chime in.

[Znote]

Perhaps find a way to add "remove condition" requests in a queue? A soft remove until executeConditions is done triggering active conditions?

if condition variable is_executing is true then soft delete? (which waits until next iteration)?

Perhaps Creature::executeConditions should be the only thing allowed to remove conditions, and the other condition functions only flag itself to deletion. Which then is checked and deleted the next time executeConditions triggers?

[jo3bingham]

Perhaps Creature::executeConditions should be the only thing allowed to remove conditions, and the other condition functions only flag itself to deletion. Which then is checked and deleted the next time executeConditions triggers?

This is the conclusion I came to. Considering Creature::executeConditions is invoked regularly, cleaning up there makes sense. Will make a PR.