Critical errors (Sphere crash)

coruja747 commented 9 years ago

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:

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

roberpot commented 9 years ago

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 commented 9 years ago

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

coruja747 commented 9 years ago

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

roberpot commented 9 years ago

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 commented 9 years ago

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 https://github.com/Sphereserver/Source/issues/13#issuecomment-153633468.

lintax commented 9 years ago

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 commented 9 years ago

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

Sphereserver / Source

Critical errors (Sphere crash) #13