views:

101

answers:

2

I ran valgrind on my program because of a segfault I can't figure out. It detected a problem here...

Address 0x75c7670 is 0 bytes inside a block of size 12 free'd
  at 0x4024851: operator delete(void*) (vg_replace_malloc.c:387)
  by 0x805F6D8: std::list<Object*, std::allocator<Object*>::remove(O
  bject* const&) (new_allocator.h:95)

The removal is in this method...

void ObjectManager::AdjustGridCoord( int x, int y, Object* const obj ) {
  // GetTileX and GetTileY guaranteed to be valid indices
  int newX = ObjectAttorney::GetTileX( obj );
  int newY = ObjectAttorney::GetTileY( obj );
  if ( x != newX || y != newY  ) {
    m_objGrid[x][y].remove( obj );
    m_objGrid[newX][newY].push_back( obj );
  }
} 

I didn't think removing a pointer from a list would call delete on it. What looks suspect here? If you need more information let me know.

P.S. Previously when debugging this, I noticed the problem occured because GetTileX and GetTileY weren't valid indices, and would return ridiculous numbers like 13775864. I think this is related to the delete issue though, and the removal or push_back is causing the problem.

Edit: Here is another code snippet

for ( unsigned int x = 0; x < m_objGrid.size(); ++x ) {
  for ( unsigned int y = 0; y < m_objGrid[x].size(); ++y ) {
    for ( ListItr obj = m_objGrid[x][y].begin(); obj != m_objGrid[x][y].end(); ++obj ) {
      ObjectAttorney::UpdateAI( *obj );
      AdjustGridCoord( x, y, *obj );
    }
  }
}

Could AdjustGridCoord be invalidating the iterator?

+1  A: 

The block of size 12 free'd is actually the list node, not your object. So, std::list::remove() didn't call delete on your pointer, it simply deleted the list node containing it.

I can't tell from your code snippets where you actually (wrongly) use that memory.

doublep
+2  A: 

In response to your edit, yes, I think you have diagnosed it correctly.

Your code is a bit confusing (mainly because you give the name obj to both an object pointer and the iterator referring to its cell in the list), but this line:

m_objGrid[x][y].remove( obj );

where you remove the obj object will invalidate the obj iterator in the calling function. As you can see from the valgrind output, removing the object cause the list to delete the cell holding the object pointer, which is what the obj iterator refers to. Thus, the obj iterator is invalidated. Then, when the call returns, the very next thing that happens is the loop increment:

++obj

Here, obj is the iterator, which was just invalidated and its referent cell deleted within the call to AdjustGridCoord. This causes an access to memory that was deallocated, which is what valgrind is complaining about.

You essentially have two options:

  1. Re-structure your loop so that you get the subsequent iterator before you call AdjustGridCoord
  2. Iterate through the list once and record what changes you need to make in some other data structure, and then do a second loop over that secondary "change list" structure, and within that loop actually make those changes to the original list.

An example of 2 would be to create a std::vector<std::pair<unsigned int, unsigned int> > that holds the coordinates that you need to call AdjustGridCoord on, and then iterate over that to actually make the calls.

Tyler McHenry
Another option: pass the ListItr to AdjustGridCoord and have it return a ListItr to the next object when finished. If it needs to remove the object, call std::list::erase() and return the returned iterator; otherwise return the incremented iterator.
Jon-Eric