Forgottenserver: Reloading spells - crash

[infister]

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 C++ code in this repository

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

1. add spell with addEvent (example below)
2. cast it
3. /reload spells before spell executes combat:execute()

Expected behaviour

Actual behaviour

Segfault

#0  0x000056098a1f2106 in AreaCombat::getArea(Position const&, Position const&) const ()
#1  0x000056098a1efa8e in AreaCombat::getList(Position const&, Position const&, std::forward_list<Tile*, std::allocator<Tile*> >&) const ()
#2  0x000056098a1ebb9c in Combat::getCombatArea(Position const&, Position const&, AreaCombat const*, std::forward_list<Tile*, std::allocator<Tile*> >&) ()

Environment

Reproduction environment

Spell:

local combat = Combat()
combat:setParameter(COMBAT_PARAM_TYPE, COMBAT_PHYSICALDAMAGE)
combat:setParameter(COMBAT_PARAM_EFFECT, CONST_ME_FIREAREA)
combat:setArea(createCombatArea({{1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,3,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},}))

local combat2 = Combat()
combat2:setParameter(COMBAT_PARAM_EFFECT, CONST_ME_FIREATTACK)
combat2:setArea(createCombatArea({{1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,3,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},
                                {1,1,1,1,1,1,1,1,1,1,1,1,1,1,1},}))

function onTargetCreature(creature, target)
    doTargetCombatHealth(creature, target, COMBAT_PHYSICALDAMAGE, 4000, 6000, CONST_ME_FIREAREA)
    return true
end

combat:setCallback(CALLBACK_PARAM_TARGETCREATURE, "onTargetCreature")

function onCastSpell(creature, variant)

    addEvent(doCombat, 5000, creature:getId(), combat2, variant)
    addEvent(doCombat, 7000, creature:getId(), combat, variant)
    addEvent(doCreatureSay, 5000, creature:getId(), "789", TALKTYPE_MONSTER_SAY)
    addEvent(doCreatureSay, 3000, creature:getId(), "456", TALKTYPE_MONSTER_SAY)  
    creature:say("123", TALKTYPE_MONSTER_SAY)
    return true
end

[CristoferMartins]

Using combat objects in addEvent always end up bad even if you create a local function which uses the local combat.

[infister]

Using combat objects in addEvent always end up bad even if you create a local function which uses the local combat.

Time to change it.

[nekiro]

Duplicate of #2888
@infister we are open for suggestions :) (and prs)

[infister]

Duplicate of #2888
@infister we are open for suggestions :) (and prs)

I cannot see where this is duplicate?

[Zbizu]

Both are addevent, reload spells, crash

[CristoferMartins]

My solution to this is to create a function like spellAddEvent which calls addevent and save the event id in a table(list) so when the reload is done we can cancel all the events to prevent the issue.

[MillhioreBT]

This cannot be fixed in a clean way at the moment.

• First of all why use /reload spells once the spell is executed? this is a way to look for problems with the server, you should be thankful that the idea of ​​recharges still holds, if they eliminate this, this PR and many more like this would not exist for sure.

• A possible solution would be to modify this part of the sources:

// combat:setArea(area)
if (getScriptEnv()->getScriptId() != EVENT_ID_LOADING) {
    reportErrorFunc("This function can only be used while loading the script.");
    lua_pushnil(L);
    return 1;
}

If you could establish a combat area at any time, you could create the combat whenever you need to use it, so there is no need to send combat as a parameter. (I have no idea if this is efficient)

[DSpeichert]

We should make it such that it's not possible to crash the server from Lua.

[andersonfaaria]

why is this marked as 'bug' and mine is marked with 'wontfix' if they are the exact same thing?
https://github.com/otland/forgottenserver/issues/2888

[infister]

The problem is Combat persist reloading, while AreaCombat does not...

[Zbizu]

@DSpeichert currently the most notable offender is addEvent paired with objects. I was thinking of writing a wrapper that would clone/rebuild objects based on their properties, but not every object can be reconstructed.

Another issue is getting items from container slot: it uses C indexing without any protection so referencing item that is on slot 20 for a backpack will crash with segfault (because last item is on slot 19 and first item in slot 0). I will check if it's possible to reproduce with newest source and will create an issue if it's still happening.

Nevermind, it doesn't segfault on new builds.

[infister]

Fix
https://github.com/SaiyansKing/optimized_forgottenserver/commit/63f8930449b6f0c0c5d4ddbc21f9d4d2436a7f17

[DSpeichert]

Fix
SaiyansKing/optimized_forgottenserver@63f8930

If you want to propose this fix to be merged, you have to actually open a pull request.