id summary reporter owner description type status priority component version resolution keywords cc game 11494 BASE: Possible use of invalid iterators Tkachov criezy "I've stumbled onto C++ iterators-related comment (in Russian: https://habr.com/ru/post/492410/#comment_21566838): someone cached `collection.end()` and then used `i = collection.erase(i)`, which invalidates old iterators. So I've decided to look through ScummVM code and I've found some places I think might use invalid iterators. I've used GitHub search and briefly read the files, so I might've skipped something or made a false positive mistake. Also, I'm not sure if ScummVM iterators invalidate after `erase()` or not. Anyways, here are these places: [https://github.com/scummvm/scummvm/blob/30c00656e1d6e6e8420c662755527ac3b08ee8ff/backends/timer/default/default-timer.cpp#L187L189] {{{ for (TimerSlotMap::iterator i = _callbacks.begin(), end = _callbacks.end(); i != end; ++i) { if (i->_value == callback) _callbacks.erase(i); }}} 1. `_callbacks.end()` is cached, but `erase()` is used. 2. `erase()` returns new valid iterator, but this code continues to use the old one. ---- [https://github.com/scummvm/scummvm/blob/46ba0500cebf4ed6334d43b6b48b2079f06d05d0/engines/ultima/ultima8/gumps/gump.cpp#L63L67] {{{ Std::list::iterator end = _children.end(); while (it != end) { Gump *g = *it; it = _children.erase(it); }}} `erase()` probably invalidates that cached `end`. Comment up there says ""Delete all children"", so I don't see why `erase()` is even needed. One could iterate through all the children, deleting each of them and then use `clear()`. Or, maybe, do something like this: {{{ while (!ch.empty()) { delete *ch.begin(); ch.erase(ch.begin()); } }}} ---- [https://github.com/scummvm/scummvm/blob/1a097b1d971e6e3cf3c10f6eb7c7a30def2d2f7c/engines/cruise/gfxModule.cpp#L252L265] and the same code at [https://github.com/scummvm/scummvm/blob/1a097b1d971e6e3cf3c10f6eb7c7a30def2d2f7c/engines/tony/gfxcore.cpp#L398L412] {{{ for (rOuter = _dirtyRects.begin(); rOuter != _dirtyRects.end(); ++rOuter) { rInner = rOuter; while (++rInner != _dirtyRects.end()) { ... // remove the inner rect from the list _dirtyRects.erase(rInner); // move back to beginning of list rInner = rOuter; }}} I see why this probably works: `erase()` only invalidates iterators pointing to elements further than the erased one, so earlier iterators are fine. If that's not so, `rOuter` should be as invalid as `rInner`, and code probably should be rewritten to use integer indexes instead. ---- [https://github.com/scummvm/scummvm/blob/6ed8dea8297480d4c42ed0d38a23734df48067e6/engines/zvision/graphics/render_manager.cpp#L743] {{{ _subsList.erase(it); }}} The cycle goes on without `break` or `return` there, so `erase()` return value should be used to update `it` and `it++` should not be called. ---- [https://github.com/scummvm/scummvm/blob/e3abab45ab27349a6296a5c3c447d331f3e5167b/engines/gob/inter.h#L61] {{{ #define CLEAROPCODEGOB(i) _opcodesGob.erase(i) }}} I couldn't find usages of `CLEAROPCODEGOB`, so I don't know if that is even related to iterators, but if it is, its return value should be used and `end()` around there should not be cached. " defect closed high --Unset-- invalid iterators, invalidate, erase