Bug in CCustomExplosionGenerator

[zerver]

I've had some crashes pointing to explosiongenerator.

It seems to store a pointer to a CEGData element in a static std::map, which is illegal to do if the data structure is going to be modified - and it is...

cachedCEGs[tag] = cegData;
currentCEG = &cachedCEGs[tag]; // yikes

To me this bug seems pretty severe, I'm surprised the game is running at all
What do you think?

[Tobi]

I've had some crashes pointing to explosiongenerator.

It seems to store a pointer to a CEGData element in a static std::map, which is illegal to do if the data structure is going to be modified - and it is...

Is it?

Storing iterator to element of std::map is fine even when the map is being modified later:

Map has the important property that inserting a new element into a map does not invalidate iterators that point to existing elements. Erasing an element from a map also does not invalidate any iterators, except, of course, for iterators that actually point to the element that is being erased.

I'd be surprised if STL implementations store a list of all iterators which point to the map in the map, so they can be updated when the map randomly moves memory around. (as you imply it does - at least, I can't think of any other thing that would cause pointer to be invalidated)

Even then, we could use iterator instead of pointer

cachedCEGs[tag] = cegData;
currentCEG = &cachedCEGs[tag]; // yikes

To me this bug seems pretty severe, I'm surprised the game is running at all
What do you think?

Not surprised at all

Though, having static variable in random function as global cache of some stuff isn't best pattern I can think of...

[Kloot]

At a glance, currentCEG is always a valid pointer because each sequence of calls made to CEG::Explosion() is preceded by a call to CEG::Load(), and cachedCEGs is never changed by anything other than CEG::Load() itself, which either finds the CEG or inserts it (setting the pointer to the new element).

However, that construction should probably be modified.

[zerver]

Aha, so the map is special. If you try the same trick with a vector you are dead meat.

I'm chasing this bug elsewhere then...

[Tobi]

Yep, in vector it is not allowed, cause on every addition/deletion it may reallocate it's memory.

Map and set are a tree so they probably allocate memory for every node separately.