Iterator invalid read of size 4

Question

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?

Yes. I am erasing map entries if they match the key. I'll add the body to the original question. Ah. I think I see what you are getting at. How can the iterator work when the container is shrinking. — jacknad, Nov 17 '16 at 19:12

score 3 · Accepted Answer · edited May 23 '17 at 11:59

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.

Should the `m_PacketMap.erase(it++)` be `m_PacketMap.erase(it)`? — jacknad, Nov 17 '16 at 19:36
`it = m_PacketMap.erase(it);` for C++11, `m_PacketMap.erase(it++);` for earlier versions (`it = m_PacketMap.erase(it);` should not compile then) — jpo38, Nov 17 '16 at 19:40

score 1 · Answer 2 · answered Nov 17 '16 at 19:29

You erase an element, which makes it invalid, then you increment it. You cannot increment an invalid iterator.

Rebuilding your code with -D_GLIBCXX_DEBUG would enable the libstdc++ Debug Mode, which would abort on an invalid iterator operation.

Unrelated to the valgrind error, you do not need to test the pointers are not null before using delete, because it is safe to use delete on a null pointer (it checks for null anyway, so you're just adding unnecessary duplicate checks).

Iterator invalid read of size 4

2 Answers2