MoveEvent::onRemoveItem: Stackoverflow

[Ezzz-dev]

The following code snippet crashed my server when attempting to add a candelabrum item for the map (that does not decay), so that when a player moves or uses this candelabrum it transforms to a decaying one.

<movevent event="RemoveItem" itemid="2042" script="candelabrum.lua" />
function onRemoveItem(item, tile, position)
    item:transform(2057)
    return true
end

The only way for it to not crash, is to use the AddItem function, but it will not work if the player moves the item around his containers, or from a tile to the inventory/container, rather only when it is added to a tile.

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

1. Add the given moveevent script code
2. Move the item from a tile to another tile

Expected behaviour

The item should simply transform to the given target

Actual behaviour

Stackoverflow Crash, more precisely: [Error - MoveEvent::executeAddRemItem] Call stack overflow

Environment

Windows 7 x64 Ultimate Visual Studio 2015 TFS 1.1 Code

[ranisalt]

So you're trying to transform an item that's supposed to be deleted?

[Ezzz-dev]

So this script transforms a deleted trap?

https://github.com/otland/forgottenserver/blob/master/data/movements/scripts/trap.lua#L32

[ranisalt]

Hmm that script checks if the item is really being moved prior to transforming it, but I doubt it's the problem.

You only get stack overflows when you reserve too much script environments (https://github.com/otland/forgottenserver/blob/4fa8f3a225d70304991555d560c56f8bd246e209/src/movement.cpp#L892), are you using TFS without any source code edits? I hit a similar problem once with wrong code.

[Ezzz-dev]

TFS 1.1 branch, the function onItemMove is being called over 15 times and all with "isAdd" set to false.

[Ezzz-dev]

I doubt this is normal (do not follow line numbers, as I hava added debug std::couts to check on variable states, and function calls)

Edit:

The ONLY way for it to work is to use the previous stated conditional branch specified in the example script file trap.lua

function onRemoveItem(item, tile, position)
    local itemPosition = item:getPosition()
    if itemPosition:getDistance(position) > 0 then
        item:transform(2912)
    end
    return true
end

[gugahoa]

I think I found what's happening:

1. 2042 fires onRemoveItem
2. 2042 is transformed into 2057
3. Inside Game::transformItem Tile::postRemoveNotification is called when 2042 is still 2042 (not transformed yet)
4. Goes to 1, because of 3

*: Inside Tile::postRemoveNotification MoveEvent::onItemMove is called

A solution would be to create a item at the desired position instead of transforming 2042, and letting 2042 be removed

The if for trap item is exactly because of that, so that item:transform doesn't trigger Tile::postRemoveNotification. It'll only transform the item if it's not above that certain tile.

This can happen with 1.2 too

[gugahoa]

I can confirm this bug on 1.2 too, just got home and tested it. Possible solution is, as on the onStep call stack overflow, make the return value matter. As in, delete the item, so that Game::transformItem does not trigger onRemoveItem again

[Ezzz-dev]

Still crashes with that line edit.

[gugahoa]

@TwistedScorpio with those same steps to reproduce?

[gugahoa]

There were some cases missing in #1900 In this scope is where it's happening: https://github.com/otland/forgottenserver/blob/master/src/game.cpp#L1611 Can't think of any easy way to fix it right now Same as before, Game::transformItem trigger onRemoveItem (as it should), and the overflow happens

Is this something that should be fixed in the source? As in, it's a script flawed by design that's causing this, not the source per see.

[Codinablack]

Is this not related to these?

https://github.com/otland/forgottenserver/issues/2280 https://github.com/otland/forgottenserver/issues/2260

[Zbizu]

still crashes (confirmed on repo cloned 24 Mar 2022)

workaround: using Player:onItemMoved (16 attempts, no crash)

	if item:getId() == 2042 then
		item:transform(2057)
	end

[Ezzz-dev]

still crashes (confirmed on repo cloned 24 Mar 2022)

workaround: using Player:onItemMoved (16 attempts, no crash)

	if item:getId() == 2042 then
		item:transform(2057)
	end

That is correct, I've since been using it on Player::onItemMoved and the problem is resolved since 1 year ago.