From d08bff6c22e92e9181a07c185d7b3aa3a7246f69 Mon Sep 17 00:00:00 2001 From: inogenous <123803852+inogenous@users.noreply.github.com> Date: Sun, 19 Nov 2023 17:01:24 +0100 Subject: [PATCH] Bugfix: Prevent infinite loop when spawning monsters * Prevents game occasionally seemingly hanging when moving to new submaps The reason for the previous problem was an infinite loop caused by: 1. `map::spawn_monsters_submap` for-loops the list of `current_submap->spawns` 2. for every spawned monster, it calls `monster::on_load` 3. `monster::on_load` calls `monster::try_reproduce`, which in turn calls `map::add_spawn` 4. So a new spawn is added, thus invalidating the iterator used in step 1 5. Undefined behavior caused by using invaliated iterators. On my compiler (gcc 13.2.0), the above problem had the following effect: * The reference `spawn_point &i` pointed to something totally different, so that in particular, `i.count` had garbage values * Instead of `i.count` being reasonable values such as `3` or `1`, the above undefined behavior made it have values such as `925969776` or `-632214304` * `i.count` is the upper bound for the inner for-loop in `map::spawn_monsters_submap`, so depending on the garbage value, it might seem like an infinite loop. Stacktrace of app when frozen and problem happened: ``` #0 0x000055a1eaf36dcb in creature_tracker::find(coords::coord_point const&) const () #1 0x000055a1eaf39253 in Creature* creature_tracker::creature_at(coords::coord_point const&, bool) () #2 0x000055a1eaf393b5 in Creature* creature_tracker::creature_at(tripoint const&, bool) () #3 0x000055a1eb357a53 in map::spawn_monsters_submap(tripoint const&, bool, bool)::{lambda(tripoint const&)#1}::operator()(tripoint const&) const () #4 0x000055a1eb3a976e in random_point(tripoint_range const&, std::function const&) () #5 0x000055a1eb37dd2d in map::spawn_monsters_submap(tripoint const&, bool, bool) () #6 0x000055a1eb37de77 in map::spawn_monsters(bool, bool) () #7 0x000055a1eb093981 in game::update_map(int&, int&, bool) () #8 0x000055a1eb094451 in game::update_map(Character&, bool) () #9 0x000055a1eb09530f in game::place_player(tripoint const&, bool) () #10 0x000055a1eb0b3a0e in game::walk_move(tripoint const&, bool, bool) () #11 0x000055a1ead27490 in avatar_action::move(avatar&, map&, tripoint const&) () #12 0x000055a1eb0f338c in game::do_regular_action(action_id&, avatar&, std::optional const&) () #13 0x000055a1eb0f6e63 in game::handle_action() () #14 0x000055a1eafbd9ea in do_turn() () #15 0x000055a1eaa5ec13 in main () ``` This commit instead changes the loop in step 1 above so that it explicitly *not* uses iterators, but instead old-fashioned indexed loop. The intention with the change is to allow other parts of the code to add items to the vector `current_submap->spawns` while we are iterating it here. If new items are added, they will be handled in later steps of the loop. --- src/map.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/map.cpp b/src/map.cpp index d49fa77fe103a..c0e8465400c00 100644 --- a/src/map.cpp +++ b/src/map.cpp @@ -8720,7 +8720,13 @@ void map::spawn_monsters_submap( const tripoint &gp, bool ignore_sight, bool spa const tripoint gp_ms = sm_to_ms_copy( gp ); creature_tracker &creatures = get_creature_tracker(); - for( spawn_point &i : current_submap->spawns ) { + + // The list of spawns on the submap might be updated while we are iterating it. + // For example, `monster::on_load` -> `monster::try_reproduce` calls `map::add_spawn`. + // Therefore, this intentionally uses old-school indexed for-loop with re-check against `.size()` each step. + // NOLINTNEXTLINE(modernize-loop-convert) + for( size_t sp_i = 0; sp_i < current_submap->spawns.size(); ++sp_i ) { + const spawn_point i = current_submap->spawns[sp_i]; // intentional copy const tripoint center = gp_ms + i.pos; const tripoint_range points = points_in_radius( center, 3 );