Critical errors (Sphere crash)

[coruja747]

using latest build on my live server I'm getting some random errors making the sphere crash. I'm a bit busy this week and probably won't have some time to fix it, so I'll post these errors here if someone want take a look or even try to fix it

this error I got on random moments:

19:11:DEBUG:__ thread (15348) __ | # | _____ function _____________ | ticks passed from previous function start ______
19:11:DEBUG:>> 15348 | 0 | CWorld::OnTick | +0
19:11:DEBUG:>> 15348 | 1 | CSector::OnTick | +46
19:11:DEBUG:>> 15348 | 2 | CChar::OnTick | +0 <-- exception catch point (below is guessed and could be incorrect!)
19:11:DEBUG:>> 15348 | 3 | CChar::Skill_Done | +0
19:11:DEBUG:>> 15348 | 4 | CChar::Skill_Stage | +0
19:11:DEBUG:>> 15348 | 5 | CChar::Skill_Magery | +0
19:11:DEBUG:>> 15348 | 6 | CChar::Spell_CastDone | +0
19:11:DEBUG:>> 15348 | 7 | CVarDefMap::Empty | +32
19:11:CRITICAL:"Access Violation" (0xe79e7), in CChar::Tick() #2 "timer expired"

---

and this one is the critical error on CChar::Use_Item() that can make sphere crash. Probably the error is on CCharUse.cpp (the loop starting on line 1768). When the client click on a unknown item, the function Use_Item() call another Use_Item(), creating a infinite loop and making the server crash. This code already have a protection for infinite loop but probably it's not working

19:50:DEBUG:__ thread (15348) __ | # | _____ function _____________ | ticks passed from previous function start ______
19:50:DEBUG:>> 15348 | 0 | NetworkManager::processAllInput | +0
19:50:DEBUG:>> 15348 | 1 | NetworkInput::processInput | +0
19:50:DEBUG:>> 15348 | 2 | NetworkInput::processData | +0
19:50:DEBUG:>> 15348 | 3 | NetworkInput::processData | +0
19:50:DEBUG:>> 15348 | 4 | NetworkInput::processGameClientData | +0
19:50:DEBUG:>> 15348 | 5 | PacketDoubleClick::onReceive | +0
19:50:DEBUG:>> 15348 | 6 | CClient::Event_DoubleClick | +0
19:50:DEBUG:>> 15348 | 7 | CClient::Cmd_Use_Item | +0
19:50:DEBUG:>> 15348 | 8 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 9 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 10 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 11 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 12 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 13 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 14 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 15 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 16 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 17 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 18 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 19 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 20 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 21 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 22 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 23 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 24 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 25 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 26 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 27 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 28 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 29 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 30 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 31 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 32 | CChar::Use_Item | +0
19:50:DEBUG:>> 15348 | 33 | CChar::Use_Item | +0
[ ... ]

[roberpot]

OMG the code on that section has no sense (from my noob sphere skills...)

    // Try to follow the link as well (if it has one)
    CItem *pLinkItem = pItem->m_uidLink.ItemFind();
    if ( pLinkItem && (pLinkItem != pItem) )
    {
        static CItem *pItemFirst = NULL;        // watch out for loops
        size_t iCount = 0; // probably needs to be static.
        if ( fLink )
        {
            if ( pItemFirst == pItem )  // <--- This is always false!
                return true;    // kill the loop
            if ( ++iCount > 64 )  // <--- iCount not static, always false!
                return true;
        }
        else
            pItemFirst = pItem; // Always updated with the last item, so this is not the first item.

        fAction |= Use_Item(pLinkItem, true);
    }

From my sphere noob skills, i think the correct code is:

    // Try to follow the link as well (if it has one)
    CItem *pLinkItem = pItem->m_uidLink.ItemFind();
    if ( pLinkItem && (pLinkItem != pItem) )
    {
        static CItem *pItemFirst = NULL;        // watch out for loops
        static size_t iCount = 0;
        if ( fLink )
        {
            if ( pItemFirst == pItem )  
                return true;    // kill the loop
            if ( ++iCount > 64 )  
                return true;
        }
        else if (!pItemFirst) 
            pItemFirst = pItem;

        fAction |= Use_Item(pLinkItem, true);
    }

I'm totally noob with sphere code and i really do not know if this works. I'm sorry if i'm wrong :(.

[lintax]

You are probably damn right! Waiting for your pull request)
PS: Actually the code smells and should be refactored.

[coruja747]

thx for the help, I merged this fix on the source, will test it tonight on my live server :D

[roberpot]

I think i'm wrong with this solution. This will work only for the first dclick on a item with link initialized. I think we have to add a bool parameter to the function to know when to reset iCount and pItemFirst.

[lintax]

Code should be refactored to several functions. One with switch, other with
loop and no static variables and other mess at all. It would be simplier
and correct. I can watch it in the evening unless you will do this before.
4 нояб. 2015 г. 10:27 пользователь "roberpot" notifications@github.com
написал:

I think i'm wrong with this solution. This will work only for the first
dclick on a item with link initialized. I think we have to add a bool
parameter to the function to know when to reset iCount and pItemFirst.

—
Reply to this email directly or view it on GitHub
#13 (comment).

[lintax]

Refactored method to separate processing and link following and removed recursion with static variables at all. Please test on a life server that had the original problems.

[roberpot]

Nice change from recursion to a loop, very nice clean code :-)