c++ boost map program crash after erase

Question

I have a problem and dont know how to proper solve it or WHY the error appear.

To my problem: I have 1 loop which execute a function every 2 seconds. That functions does a for() function and erase all entrys which remaining time is at 0. If remaining time is not 0 then it will decrease it by 2000 (2sec).

But after erasing an entry the program crashes...

boost map:

boost::unordered_map<unsigned int, sBUFF_INFO*> p_BuffInfo;

function which get executed from 2 seconds loop

void CSkill::DecreaseAllBuffRemTime()
{
    for( itertype(p_BuffInfo) it = p_BuffInfo.begin(); it != p_BuffInfo.end(); it++ )
    {
        sBUFF_INFO* buff = it->second;
        if(buff != NULL)
        {
            if(buff->dwTimeRemaining <= 0)
            {
                this->DelPcBuffInfo(buff->tblidx)
            }else{
                buff->dwTimeRemaining -= 2000;
            }
        }
    }
}

DelPcBuffInfo function:

void CSkill::DelPcBuffInfo(unsigned int tblidx)
{
    p_BuffInfo.erase(tblidx);
}

Now after DelPcBuffInfo gets executed the program crash.

At this line it crash:

sBUFF_INFO* buff = it->second;

At debug:

Unhandled exception at 0x00578e0f in GameServer.exe: 0xC0000005: Access violation reading location 0xddddddd9.

it + node_ = hash_ CXX0030; Error: expression cannot be evaluated

I dont really understand why this error appear..

edit: If I add a "return" after this->DelPcBuffInfo(buff->tblidx) then the program dont crash..

score 2 · Answer 1 · edited May 23 '17 at 12:13

2

Adding or removing items from a container will often invalidate your iterators. Check the documentation for unordered_map iterators or here: Iterator invalidation in boost::unordered_map

edited May 23 '17 at 12:13

Community

1
1

answered Nov 06 '14 at 00:45

iwolf

1,080
1
7
10

score 1 · Answer 2 · answered Nov 06 '14 at 01:11

1

the correct idiom is

for( itertype(p_BuffInfo) it = p_BuffInfo.begin(); it != p_BuffInfo.end(); )
{
    sBUFF_INFO* buff = it->second;
    if(buff != NULL)
    {
        if(buff->dwTimeRemaining <= 0)
        {
            it = this->DelPcBuffInfo(buff->tblidx)
        }else{
            buff->dwTimeRemaining -= 2000;
            it++;
        }
    }
}

ie dont increment in the loop. Instead increment if you dont delete otherwise have the delete operation return the new iterator. Thats why remove returns an iterator pointing at the next element

This is courtesy of the awesome Scott Myers

answered Nov 06 '14 at 01:11

pm100

48,078
23
82
145

thanks. Error at this line it = this->DelPcBuffInfo(buff->tblidx). "No operator found which akes a right-hand operand of type 'void' – dobby Nov 06 '14 at 01:16
this idiom is for erase on the iterator, wich is not what he is doing – sp2danny Nov 06 '14 at 01:17
delxxx smell like erase and the title says "c++ boost map program crash after erase" – pm100 Nov 06 '14 at 01:34
@pm100 he does erase on key, that returns count – sp2danny Nov 06 '14 at 06:57
it also gets stuck in infinite loop if `buff==NULL` – sp2danny Nov 06 '14 at 07:09

score 1 · Answer 3 · answered Nov 06 '14 at 07:24

To add to the existing answers pointing out the erase-iterator idiom: The reason for you crash is that the iterator it is invalidated due to the removal of the element. Thus, the increment on the (invalid) operator causes undefined behaviour and it will point to some arbitrary memory block. Dereferencing the "iterator" then crashes your program.

To avoid this problem, apply the idiom as demonstrated in the other answers, that is * Use the iterator version of erase. It returns an iterator to the next element ( which may be end()) * Use the return value of this erase as new value of it. Since it already points to the next element, do not increment again (otherwise you may skip an element in your map or cause undefined behaviour if it already points to the end of the map. * Only increment the iterator yourself, when you did not erase an element.

Note: If your intention is to get rid of the sBUFF_INFO element completely upon removal from the map, your programm shows a memory leak. Erasing the pointer from the map does not delete the pointed-to memory. You need to delete the pointee yourself (or use an appropriate smart pointer).

score 0 · Accepted Answer · answered Nov 06 '14 at 07:07

void CSkill::DecreaseAllBuffRemTime()
{
    auto it = p_BuffInfo.begin();
    while( it != p_BuffInfo.end() )
    {
        sBUFF_INFO* buff = it->second;
        if(buff)
        {
            if(buff->dwTimeRemaining <= 0)
            {
                // probably delete buff too
                it = p_BuffInfo.erase(it);
            } else {
                buff->dwTimeRemaining -= 2000;
                ++it;
            }
        } else {
            ++it;
        }
    }
}

c++ boost map program crash after erase

4 Answers4