Crash DoTargetCombatCondition

[infister]

Before creating an issue, please ensure:

• [+] 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)
• [+] This issue is reproducible without changes to the code in this repository

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

1. create StepIn movement with doTargetCombatCondition
2. step 2 times on it

Expected behaviour

Not crash

Actual behaviour

Crash

Environment

Ubuntu 16

#0  0x0000000000000065 in ?? ()
[Current thread is 1 (Thread 0x7f3380cdc700 (LWP 13194))]
(gdb) bt
#0  0x0000000000000065 in ?? ()
#1  0x000000000073fe8c in std::default_delete<Condition const>::operator()(Condition const*) const ()
#2  0x000000000073ee81 in std::unique_ptr<Condition const, std::default_delete<Condition const> >::~unique_ptr() ()
#3  0x000000000073c9d4 in void __gnu_cxx::new_allocator<std::unique_ptr<Condition const, std::default_delete<Condition const> > >::destroy<std::unique_ptr<Condition const, std::default_delete<Condition const> > >(std::unique_ptr<Condition const, std::default_delete<Condition const> >*) ()
#4  0x0000000000739537 in void std::allocator_traits<std::allocator<std::unique_ptr<Condition const, std::default_delete<Condition const> > > >::destroy<std::unique_ptr<Condition const, std::default_delete<Condition const> > >(std::allocator<std::unique_ptr<Condition const, std::default_delete<Condition const> > >&, std::unique_ptr<Condition const, std::default_delete<Condition const> >*) ()
#5  0x00000000007355c4 in std::_Fwd_list_base<std::unique_ptr<Condition const, std::default_delete<Condition const> >, std::allocator<std::unique_ptr<Condition const, std::default_delete<Condition const> > > >::_M_erase_after(std::_Fwd_list_node_base*, std::_Fwd_list_node_base*) ()
#6  0x0000000000730a7c in std::_Fwd_list_base<std::unique_ptr<Condition const, std::default_delete<Condition const> >, std::allocator<std::unique_ptr<Condition const, std::default_delete<Condition const> > > >::~_Fwd_list_base() ()
#7  0x000000000072bb94 in std::forward_list<std::unique_ptr<Condition const, std::default_delete<Condition const> >, std::allocator<std::unique_ptr<Condition const, std::default_delete<Condition const> > > >::~forward_list() ()
#8  0x0000000000729bf4 in CombatParams::~CombatParams() ()
#9  0x0000000000708b99 in LuaScriptInterface::luaDoTargetCombatCondition(lua_State*) () ```

[nekiro]

Can you please use issue template?

[infister]

@thexamx I do not really know what it has changed, but here you are

[ranisalt]

Could you please share the script you are having issue with? It seems like a double free and you are probably adding the same condition twice.

[infister]

local condition2 = Condition(CONDITION_FIRE)
Condition.addDamage(condition2, 7, 9000, -10)

function onStepIn(cid, item, position, fromPosition)
return doRemoveCondition(cid, CONDITION_FIRE), doTargetCombatCondition(0, cid, condition2, CONST_ME_HITBYFIRE)
end

No, it seems you are using incorrect list type for ConditionList in CombatParam. Don't you see it ?

[ranisalt]

You have extra parenthesis after CONDITION_FIRE

[infister]

It is issue that happened while copying the script. The script is correct itself. There is a bug in a repo connected with forward_list and ConditionList in CombatParams

[nekiro]

local condition = Condition(CONDITION_FIRE)
condition:addDamage(7, 9000, -10)

function onStepIn(creature, item, position, fromPosition)
	creature:removeCondition(CONDITION_FIRE)
	creature:addCondition(condition)
	return true
end

Does not crash.
Is it different from your example?

[infister]

It is, function doTargetCombatCondition implies damage dealt by specific player.

[nekiro]

Confirmed, it is crashing.

[ranisalt]

It can't be an issue with the structure itself, as STL is rock solid and thoroughly tested.

You can't declare a condition outside a function as it will be cleared when removed and you will try to reuse it later. Instantiate the condition right before using it, i.e.

function onStepIn(creature, item, position, fromPosition)
	creature:removeCondition(CONDITION_FIRE)

        local condition = Condition(CONDITION_FIRE)
        condition:addDamage(7, 9000, -10)
	creature:addCondition(condition)
	return true
end

And you should be clear.

[infister]

This has to be done via doTargetCombatCondition , as your example does not provide feature called setting damage dealer.

And you are using forward_list in the wrong way

[ranisalt]

As you wish:

function onStepIn(cid, item, position, fromPosition)
    local condition2 = Condition(CONDITION_FIRE)
    Condition.addDamage(condition2, 7, 9000, -10)

    return doRemoveCondition(cid, CONDITION_FIRE), doTargetCombatCondition(0, cid, condition2, CONST_ME_HITBYFIRE)
end

And I personally hate forward list, but why do you keep saying "using in the wrong way"?

[infister]

The code you provided me crashes the server if you step on a field 2 times (did not check). That's the point. No LUA code should crash the server.

[ranisalt]

You didn't answer my question. No code should have bugs, yet no code is free of bugs. What is wrong with the usage of forward_list here? No issue should go without answers or fixes.

[infister]

I do not really know what exactly happens the bug. I suspect this is connected with doRemoveCondition and its erase function. But I do not want to waste the time debugging it.

I have fixed it by changing:

Combat.h

std::forward_list<std::unique_ptr<const Condition>> conditionList;

to

std::list<const Condition*> conditionList;

And by replacing in luascript.cpp

params.conditionList.emplace_front(condition);

to

params.conditionList.push_back(condition);

Works as expected.

[nekiro]

@infister Lua*

[DanielChabrowski]

@infister That's not a fix, that's introducing a memory leak.

A quick fix would be to change two calls in luascript.cpp from:
params.conditionList.emplace_front(condition);
To:
params.conditionList.emplace_front(condition->clone());

btw. since clone() is returning an owning pointer, it should return unique_ptr

[ranisalt]

That's what I suspected, you are getting a double free and it fixes because you miss the second delete by removing the unique_ptr. It either introduces a leak or a dangling pointer.

Also, it should use a vector for better performance and memory usage, if one wants to change.

[infister]

Cloning condition fixes the crash, so I recommend introducing it in the code.

[DanielChabrowski]

Sorry for this little reference mess, got lost in terminals.

I'm not sure if we should change std::forward_list to std::vector, since for some reason it is expected to push the condition to the beginning. What is interesting tho is that we can add multiple conditions to combat, but the function is called "setCondition" as if only one was supported.

[ranisalt]

Please update your source. It should be fixed with the latest commit.