[Scummvm-tracker] [ScummVM :: Bugs] #11494: Possible usage of invalid iterators

[ScummVM :: Bugs]

#11494: Possible usage of invalid iterators
-----------------------------------------+-----------------------
Reporter:  Tkachov                       |      Owner:  (none)
    Type:  defect                        |     Status:  new
Priority:  low                           |  Component:  --Unset--
Keywords:  iterators, invalidate, erase  |       Game:
-----------------------------------------+-----------------------
 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<Gump *>::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.
-- 
Ticket URL: <https://bugs.scummvm.org/ticket/11494>
ScummVM :: Bugs <https://bugs.scummvm.org>
ScummVM