jacknad jacknad - 17 days ago 6
C++ Question

Iterator invalid read of size 4

Why might Valgrind indicate

Invalid read of size 4
in the following line?

for (map<uint16_t, SPacket *>::iterator it = m_PacketMap.begin() ; it != m_PacketMap.end(); ++it)
{
if (it->first < ackNumber)
{
if (it->second->data) delete [] it->second->data;
if (it->second) delete it->second;
m_PacketMap.erase(it);
}
}


I verify that
m_PacketMap.size() > 0
before the loop and have temporarily added debug before the loop to verify the m_PacketMap contents but it all looks as expected. This is the Valgrind error message and RadioManager.cpp:1042 is the line above:

==5535== Invalid read of size 4
==5535== at 0x421EBE5: std::_Rb_tree_increment(std::_Rb_tree_node_base*) (in /usr/lib/i386-linux-gnu/libstdc++.so.6.0.16)
==5535== by 0x80AD20D: RadioManager::DecodeAcknowledgementNumber(unsigned char*, unsigned int) (RadioManager.cpp:1042)


This is how SPacket and m_PacketMap are defined

typedef struct SPacket
{
uint8_t * data;
size_t size;
timeval tval;
} SPacket;
map<uint16_t, SPacket *> m_PacketMap;


Is there an issue with my iterator, a possible issue in _Rb_tree_increment, or something else entirely?

Answer

Erasing container elements from a for loop has to be done with care...

You must do:

map<uint16_t, SPacket *>::iterator it = m_PacketMap.begin()
while ( it != m_PacketMap.end() ) 
{
    if (it->first < ackNumber)
    {
        if (it->second->data) delete [] it->second->data;
        if (it->second) delete it->second;
        // it updated by erase, no need to increment
        it = m_PacketMap.erase(it);
    }
    else
    {
        // move to next item
        ++it;
    }
}

That's the way a loop removing some elements of a container must be written (works the same for set, vector...).

For maps, code above only works from C++11. If you use an earlier version, according to this post (not tested), you should do:

for ( map<uint16_t, SPacket *>::iterator it = m_PacketMap.begin(); it != m_PacketMap.end();  ) 
{
    if (it->first < ackNumber)
    {
        if (it->second->data) delete [] it->second->data;
        if (it->second) delete it->second;
        // it updated by erase, no need to increment
        m_PacketMap.erase(it++);
    }
    else
    {
        // move to next item
        ++it;
    }
}

Because erasing the item and then incrementing it (what you end up doing with your for loop) will make you loose elements and possible pass over m_PacketMap.end() (and then have your loop cover items out of container's limits) if you erase the last element from the container.

Comments