Skip to content

Iterator invalidation cause by MineFacility extensions #2126

@DanRStevens

Description

@DanRStevens

There is an iterator invalidation problem in the for loop in StructureManager::updateStructures:

for (auto* structure : structures)
{
	updateStructure(resources, population, *structure);
}

The problem comes from how the MineFacility can dig new levels and create new MineShaft entries, which happens during iteration of the above loop. Any resize of the underlying collection caused by the new MineShaft will invalidate the for loop iterator, leading to undefined behavior upon the next iteration.

With knowledge of the current size() and capacity() of the mDeployedStructures collection, we can create the right number of Structure instances such that a new MineShaft will cause an allocation.


The problem was validated using valgrind:

==69052== Invalid read of size 8
==69052== at 0x1B91FC: StructureManager::updateStructures(StorableResources const&, PopulationPool&, std::vector<Structure*, std::allocator<Structure*> >&) (StructureManager.cpp:673)
==69052== by 0x1B9237: StructureManager::update(StorableResources const&, PopulationPool&) (StructureManager.cpp:636)
==69052== by 0x1AC512: MapViewState::nextTurn() (MapViewStateTurn.cpp:574)
==69052== by 0x19ACF9: MapViewState::onKeyDown(NAS2D::KeyCode, NAS2D::KeyModifier, bool) (MapViewState.cpp:523)
==69052== by 0x1E9058: NAS2D::DelegateX<void, NAS2D::KeyCode, NAS2D::KeyModifier, bool>::operator()(NAS2D::KeyCode, NAS2D::KeyModifier, bool) const (Delegate.h:411)
==69052== by 0x1E9A63: NAS2D::Signal<NAS2D::KeyCode, NAS2D::KeyModifier, bool>::emit(NAS2D::KeyCode, NAS2D::KeyModifier, bool) const (Signal.h:49)
==69052== by 0x1E8B64: operator() (Signal.h:53)
==69052== by 0x1E8B64: NAS2D::EventHandler::onMessage(SDL_Event const&) (EventHandler.cpp:599)
==69052== by 0x1E8DDC: NAS2D::EventHandler::pump() (EventHandler.cpp:585)
==69052== by 0x1D56D1: NAS2D::StateManager::update() (StateManager.cpp:71)
==69052== by 0x135E53: main (main.cpp:178)

==69052== Address 0x1caacaa0 is 64 bytes inside a block of size 1,024 free'd
==69052== at 0x484A61D: operator delete(void*, unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==69052== by 0x12F795: std::__new_allocator<Structure*>::deallocate(Structure**, unsigned long) (new_allocator.h:172)
==69052== by 0x1B94BF: deallocate (allocator.h:210)
==69052== by 0x1B94BF: deallocate (alloc_traits.h:517)
==69052== by 0x1B94BF: _M_deallocate (stl_vector.h:390)
==69052== by 0x1B94BF: void std::vector<Structure*, std::allocator<Structure*> >::_M_realloc_insert<Structure*>(__gnu_cxx::__normal_iterator<Structure**, std::vector<Structure*, std::allocator<Structure*> > >, Structure*&&) (vector.tcc:519)
==69052== by 0x1B953C: Structure*& std::vector<Structure*, std::allocator<Structure*> >::emplace_back<Structure*>(Structure*&&) (vector.tcc:123)
==69052== by 0x1B6C56: push_back (stl_vector.h:1299)
==69052== by 0x1B6C56: StructureManager::addStructure(Structure&, Tile&) (StructureManager.cpp:151)
==69052== by 0x1B6CA8: StructureManager::create(StructureID, Tile&) (StructureManager.cpp:139)
==69052== by 0x188EC3: MapViewState::onMineFacilityExtend(MineFacility*) (MapViewStateEvent.cpp:264)
==69052== by 0x14DD9E: NAS2D::DelegateX<void, MineFacility*>::operator()(MineFacility*) const (Delegate.h:411)
==69052== by 0x14DC21: MineFacility::think() (MineFacility.cpp:79)
==69052== by 0x1B6AC3: StructureManager::updateStructure(StorableResources const&, PopulationPool&, Structure&) (StructureManager.cpp:749)
==69052== by 0x1B920C: StructureManager::updateStructures(StorableResources const&, PopulationPool&, std::vector<Structure*, std::allocator<Structure*> >&) (StructureManager.cpp:675)
==69052== by 0x1B9237: StructureManager::update(StorableResources const&, PopulationPool&) (StructureManager.cpp:636)

==69052== Block was alloc'd at
==69052== at 0x4846FA3: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==69052== by 0x12FBCD: std::__new_allocator<Structure*>::allocate(unsigned long, void const*) (new_allocator.h:151)
==69052== by 0x1B946E: allocate (allocator.h:198)
==69052== by 0x1B946E: allocate (alloc_traits.h:482)
==69052== by 0x1B946E: _M_allocate (stl_vector.h:381)
==69052== by 0x1B946E: void std::vector<Structure*, std::allocator<Structure*> >::_M_realloc_insert<Structure*>(__gnu_cxx::__normal_iterator<Structure**, std::vector<Structure*, std::allocator<Structure*> > >, Structure*&&) (vector.tcc:459)
==69052== by 0x1B953C: Structure*& std::vector<Structure*, std::allocator<Structure*> >::emplace_back<Structure*>(Structure*&&) (vector.tcc:123)
==69052== by 0x1B6C56: push_back (stl_vector.h:1299)
==69052== by 0x1B6C56: StructureManager::addStructure(Structure&, Tile&) (StructureManager.cpp:151)
==69052== by 0x1B6CA8: StructureManager::create(StructureID, Tile&) (StructureManager.cpp:139)
==69052== by 0x18F7F7: MapViewState::readStructures(NAS2D::Xml::XmlElement*) (MapViewStateIO.cpp:399)
==69052== by 0x1921F2: MapViewState::load(SavedGameFile&) (MapViewStateIO.cpp:267)
==69052== by 0x19B9AE: MapViewState::initialize() (MapViewState.cpp:292)
==69052== by 0x1A633C: GameState::initializeGameState() (GameState.cpp:59)
==69052== by 0x1A63EC: GameState::initialize() (GameState.cpp:81)
==69052== by 0x1D56A0: NAS2D::StateManager::setState(NAS2D::State*) (StateManager.cpp:59)

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions