Forgottenserver: Crash after reloading with addEvent spells using getMinMaxValues

[andersonfaaria]

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. Create a simple spell that uses callback onGetFormulaValues
   combat:setCallback(CALLBACK_PARAM_LEVELMAGICVALUE, "onGetFormulaValues")
2. Put a few rounds of addEvent with combat:execute
3. Reload spells in the middle of its execution and enter spell area

Expected behaviour

After the reload the spell should stop

Actual behaviour

Crash
Thread 2 "tfs" received signal SIGSEGV, Segmentation fault.
0|stg | [Switching to Thread 0x7ffff43ce700 (LWP 13933)]
0|stg | 0x00000000006ec5da in ValueCallback::getMinMaxValues(Player*, CombatDamage&) const ()

Environment

I do not have any kind of modification in combat.cpp, the environment is the same as this repository.

[EvilHero90]

This is a prime example of why using /reload is just a bad idea in general but I'm still curious how the script looks like , so post it please.

[andersonfaaria]

This is a prime example of why using /reload is just a bad idea in general but I'm still curious how the script looks like , so post it please.

local config = {
velocidade = 200, -- interval between hits (lesser = quicker)
hits = 20, -- how many rounds
key = 13871, -- storage for cooldown
cooldown = 15, -- time between uses
effect3 = 44, -- effect by casting spell
}

local combat = Combat()
combat:setParameter(COMBAT_PARAM_TYPE, COMBAT_ICEDAMAGE)
function onGetFormulaValues(cid, level, maglevel)
    local min = (level / 5) + (maglevel * 1.8) + 12
    local max = (level / 5) + (maglevel * 3) + 17
    return -min, -max
end

combat:setCallback(CALLBACK_PARAM_LEVELMAGICVALUE, "onGetFormulaValues")

local arr = {
{1, 1, 1},
{1, 3, 1}, -- area que vai acertar a spell enquanto estiver rodando
{1, 1, 1},
}

local area = createCombatArea(arr)
combat:setArea(area)

local function middleEffect(cid, var, position, lim, count)
    n = count or 0
    local player = Player(cid)
    if player and n < lim then
        if n % 3 == 0 then
            combat:execute(player, var)
            position:sendMagicEffect(CONST_ME_POFF)
        end
        addEvent(middleEffect, config.velocidade, cid, var, position, lim, n + 1)
    end
return true
end


function onCastSpell(player, var)
    local position = player:getPosition()
    if getPlayerStorageValue(player.uid, config.key) - os.time() <= 0 then
        if not player:getGroup():getAccess() then
            setPlayerStorageValue(player.uid, config.key, os.time() + config.cooldown)
        end
        position:sendMagicEffect(config.effect3)
        addEvent(middleEffect, config.velocidade, player.uid, var, position, config.hits)
    else
        player:sendCancelMessage("You are exhausted.")
        position:sendMagicEffect(CONST_ME_POFF)
        return false
    end
    return true
end

[nekiro]

It's not onGetFormulaValues who crash your server, but you passing combat object into the addEvent which gets destroyed upon reload and causes server to crash. That's not how you should use combat the way its designed in tfs, so imo its not a bug. Your code is just wrong, its like you would pass player userdata to addEvent what would happen if player logged out? (it would crash)

We should be able to create combat, areas wherever we want in the code, but current system has that limited and its impossible, because they arent dynamic.

[andersonfaaria]

you joking right?
that same logic can be said about players, so whenever you have a crash you can just say "you were using the system in an un-planned way"

Definetely no, this should not crash the server. This is a valid issue because somewhere in the source it does not check if combat exists. If this is not the correct way, what other alternative do we have? because addEvents in spells it's something that goes back to the very principle of otserver, if tfs don't have the option to do so, it's obviously a failed project design imo.

[nekiro]

You definitely do not understand how does that work, to enlighten you a bit
https://github.com/otland/forgottenserver/blob/master/src/luascript.cpp#L11823
is the combat really not checked for validity? You are literally trying to use saved userdata which was deleted in source, but you still keep reference to it in lua...
it's like you would delete pointer and then tried to dereference it after it was deleted, who is the one to blame? Code, because it tried to dereference deleted pointer or you who wrote that code that dereferences deleted pointer?
There is a reason for recreating player when using addEvents (and you cannot do that with combat, cause they do not have unique ids or anything that would differentiate them from other combats)

[andersonfaaria]

I'm sorry but you're missing the point here, under no circumstance we should have a crash and if I use a player that logged out/died in an addEvent it gives me a console error because that is handled. The same does not happen with combats though and it's not because they don't have unique ids...

If you don't know how to solve the issue you don't need to say it's not an issue at all, I just feel bad to see you acting like this knowing you call yourself a developer, this is a very shitty attitude that would end with you getting fired at any serious company. Not to mention that "enlighten" bullshit, this is the type of ego attitude that pushes people away from otland/tfs.

[infernumx]

Combats are destroyed upon reload and reconstructed on script load (this is why you initialize combats outside of any functions or event callbacks). When you reload and still have a reference in an addEvent, you're creating a volatile situation where that destroyed object is still available and your script is attempting to use it when it doesn't exist. What Nekiro said is literally correct, you shouldn't be reloading at all in a stable server environment in the first place, reload should only be used for testing purposes. There is no way to check the validity of userdata memory in Lua which is why you need to be careful using addEvents with any type of userdata because the value held in memory held by that userdata can be destroyed at any time.

TL;DR: Don't use reload :)

[Zbizu]

It's an issue since we started using userdata. Userdata argument in addEvent is what causes this crash.

I propose throwing an error when somebody tries addEvent with userdata. If this can't be done in c++, it can be redeclared:

if not SOME_FLAG then
    SOME_FLAG = true -- prevent infinite loop on reload

    addEvent = function(callback, delay, ...)
        -- loop through arguments, if userdata then error("error message")
        -- I forgot how to unpack args properly xd
        -- return addEvent(callback, delay, ...)
    end
end