Forgottenserver: Crash on eventPlayerOnLook

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

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

1. no idea, happens rarely and i cannot reproduce (dont know a way)

Expected behaviour

Actual behaviour

crash

Environment

#0  0x00007f4b37b1918c in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#1  0x00007f4b383c652b in std::basic_streambuf<char, std::char_traits<char> >::xsputn(char const*, long) () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#2  0x00007f4b383b758e in std::basic_ostream<char, std::char_traits<char> >& std::__ostream_insert<char, std::char_traits<char> >(std::basic_ostream<char, std::char_traits<char> >&, char const*, long) () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#3  0x00005627f5f7e105 in Player::getDescription[abi:cxx11](int) const ()
#4  0x00005627f5eeff77 in LuaScriptInterface::luaCreatureGetDescription(lua_State*) ()
#5  0x00007f4b39660c75 in ?? () from /usr/lib/x86_64-linux-gnu/liblua5.2.so.0
#6  0x00007f4b3966c825 in ?? () from /usr/lib/x86_64-linux-gnu/liblua5.2.so.0
#7  0x00007f4b39660f9e in ?? () from /usr/lib/x86_64-linux-gnu/liblua5.2.so.0
#8  0x00007f4b396605cf in ?? () from /usr/lib/x86_64-linux-gnu/liblua5.2.so.0
#9  0x00007f4b39661201 in ?? () from /usr/lib/x86_64-linux-gnu/liblua5.2.so.0
#10 0x00007f4b3965d0a1 in lua_pcallk () from /usr/lib/x86_64-linux-gnu/liblua5.2.so.0
#11 0x00005627f5e7ba24 in LuaScriptInterface::protectedCall(lua_State*, int, int) ()
#12 0x00005627f5e7ca0d in LuaScriptInterface::callVoidFunction(int) ()
#13 0x00005627f5da7ecd in Events::eventPlayerOnLook(Player*, Position const&, Thing*, unsigned char, int) ()
#14 0x00005627f5dc03f4 in Game::playerLookAt(unsigned int, Position const&, unsigned char) ()

[EvilHero90]

show your onLook function from events/scripts/player.lua

[infister]

function Player:onLook(thing, position, distance)
    if (not thing) then
        return true
    end

    local description = "You see "
    if (thing:isItem() or thing:isCreature()) then
        description = description .. thing:getDescription(distance)
    end

    if thing:isItem() then
        local itemType = thing:getType()  
        if (itemType and itemType:getImbuingSlots() > 0) then
            local imbuingSlots = "Imbuements: ("

            local hasActiveImbue = false
            for i = 1, itemType:getImbuingSlots() do
                if (thing:isActiveImbuement(i+3)) then
                    hasActiveImbue = true
                end
            end

            if not hasActiveImbue and thing:hasAttribute(ITEM_ATTRIBUTE_SPECIAL) then
                thing:removeAttribute(ITEM_ATTRIBUTE_SPECIAL)
            end

            for i = 1, itemType:getImbuingSlots() do
                local specialAttr = thing:getSpecialAttribute(i)
                local time = 0
                if (thing:getSpecialAttribute(i+3)) then
                    time = getTime(thing:getSpecialAttribute(i+3))
                end

                if (specialAttr and specialAttr ~= 0) then
                    if (i ~= itemType:getImbuingSlots()) then
                        imbuingSlots = imbuingSlots.. "" ..specialAttr.." " ..time..", "
                    else
                        imbuingSlots = imbuingSlots.. "" ..specialAttr.." " ..time..")."
                    end
                else
                    if (i ~= itemType:getImbuingSlots()) then
                        imbuingSlots = imbuingSlots.. "Free Slot, "
                    else
                        imbuingSlots = imbuingSlots.. "Free Slot)."
                    end
                end
            end
            description = string.gsub(description, "It weighs", imbuingSlots.. "\nIt weighs")
        end
    end

    if self:getGroup():getAccess() then
        if thing:isItem() then
            local itemType = thing:getType()  
            description = string.format("%s\nItem ID: %d", description, thing:getId())

            local actionId = thing:getActionId()
            if actionId ~= 0 then
                description = string.format("%s, Action ID: %d", description, actionId)
            end

            local uniqueId = thing:getAttribute(ITEM_ATTRIBUTE_UNIQUEID)
            if uniqueId > 0 and uniqueId < 65536 then
                description = string.format("%s, Unique ID: %d", description, uniqueId)
            end

            local itemType = thing:getType()  
            local transformEquipId = itemType:getTransformEquipId()
            local transformDeEquipId = itemType:getTransformDeEquipId()

            if transformEquipId ~= 0 then
                description = string.format("%s\nTransforms to: %d (onEquip)", description, transformEquipId)
            elseif transformDeEquipId ~= 0 then
                description = string.format("%s\nTransforms to: %d (onDeEquip)", description, transformDeEquipId)
            end

            local decayId = itemType:getDecayId()
            if decayId ~= -1 then
                description = string.format("%s\nDecays to: %d", description, decayId)
            end
        elseif thing:isCreature() then
            local str = "%s\nHealth: %d / %d"
            if thing:getMaxMana() > 0 then
                str = string.format("%s, Mana: %d / %d", str, thing:getMana(), thing:getMaxMana())
            end
            description = string.format(str, description, thing:getHealth(), thing:getMaxHealth()) .. "."
        end

        local position = thing:getPosition()
        description = string.format(
            "%s\nPosition: %d, %d, %d",
            description, position.x, position.y, position.z
        )

        if thing:isItem() then
            description = string.format("%s\nClientID: %d", description, thing:getType()  :getClientId())
        end

        if thing:isCreature() and self:getPlayerCast().status == 0 then
            if thing:isPlayer() then
                description = string.format("%s\nIP: %s.", description, Game.convertIpToString(thing:getIp()))
            end
        end
    end
    self:sendTextMessage(MESSAGE_INFO_DESCR, description)
end

[raymondtfr]

This code does look like you have made changes to it, so you should ask for help on the support forums.

Though, you should probably look into the operations you are doing with indexes cause that's what's probably causing the crash.

[infister]

This code does look like you have made changes to it, so you should ask for help on the support forums.

Though, you should probably look into the operations you are doing with indexes cause that's what's probably causing the crash.

issue also persists with unmodified file.
what indexes do you mean?

[nekiro]

Does it? Because everytime I hear about that crash its related to some otx code or other modified one.

[infister]

maybe because nobody actually uses unmodified tfs (i dont recall anyone doing it). as I said, this is extremly rare bug - some specific condition must be met

anyway, that does not matter, since crucial functions that may be involved in this crash, are the same in otx and forgottenserver => this distro is affected too

[joseluis2g]

if you can not reproduce this bug with a clean tfs server then its not tfs related

[infister]

i dont know a way to reproduce on any server.

[nekiro]

This crash is not related to this repository also it crashed in creature:getdescription (specifically player) lua event was called later. Its not related until your code (getDescription methods) in source is exactly the same as repo one, then we can think.

[infister]

they are exactly the same in sources.
this bug is repo-related.

[westwol]

This bug can be reproduced in master, it's a bug related to the guild rank pointer.

[Znote]

@westwol provide steps to reproduce crash please.

[westwol]

I don't know how to reproduce it, but I've heard that it has something to do with changing guild rank to strange characters.

[Znote]

Could it happen if you have a really long name, is in a guild that has a really long name and rank name, with a long guild nickname?

[vankk]

@Znote I don't think so, I faced this issue a few months ago. I don't know exactly how to reproduce it but it is something about the player guild nick, that changes the nick while the player are online.

I didn't tested because I don't know how to reproduce, but it may "fix" if on our AAC the player guild nick can only be changed while they are offline. That "fix" is only something that is on my head a few months.

[infister]

I will give you a hint... check OTSYS_TIME() on creating Guild object in iologindata.cpp and OTSYS_TIME() on adding a member to it in onCreatureAppear in player.cpp.
Further, look at how deleting a guild object in Game is handled.

[gudan-garam]

If someone could host a clean tfs server with a acc maker and send the link my away I think I know what it is, just don't have the time to set everything up.

[Zbizu]

unable to reproduce
tried Polish characters, unicode characters, Chinese characters, emoji, arabic characters and DOS characters

setting data ways tested: client, phpmyadmin

[infister]

This is not related to the content of description, but they are trying to access a not-nulled, non-existant, pointer. There is a small time window, when you can logout with one char/login with another one and that may happen. This is not related to eventPlayerOnLook directly.

Suspected cause:
When you logout, guildMember is removed and guildobject removed, if member count == 0.
When you login, a guild object is created, but guild member is added in onCreatureAppear. This is problematic, because when you login with char A when guild object is already created, and immediately logout with char B (in the same guild), a guildobject is removed before onCreatureAppear is fired -> guild gets deleted and is not re-created.
https://github.com/otland/forgottenserver/blob/master/src/player.cpp#L162
This line is not null then and code continues, so probably crashes here:
https://github.com/otland/forgottenserver/blob/master/src/player.cpp#L174

[Zbizu]

Then I think that the problem is bigger because if we store object of any class (player, item, etc) and it changes, it leads to a crash

steps to reproduce: using addEvent or declaring player/item in lua channel and changing its state

I think that TFS needs a safe way of dealing with "broken" objects

[infister]

No, this is not the same. This is just poorly coded, missed at some point. Problem is not located in LUA.

[Kamenuvol]

It isnt sequential? Player A should appear before B exit

[gudan-garam]

@infister which linux were you running on?

[SaiyansKing]

The problem with guildrank isn't because of some race condition as infister suggested but a more classical problem, the ranks are contained in vector stl container which invalidate all pointers when reallocation happens and it actually can happen easily when "ranks.emplace_back(rankId, rankName, level);" will need more memory.

So it is easy to crash any server: create a guild, login on character that is in that guild -> create new ranks and login on other character that is in the same guild -> reallocation happen then the first character have invalid pointer -> look on him -> crash.

[Xaekai]

Somebody has read their Kalev.