Merge pull request #3378 from IvanSavenko/fix_lists

Fix possible memory corruption in town/hero lists

client/PlayerLocalState.cpp

@@ -204,7 +204,7 @@ const CGHeroInstance * PlayerLocalState::getWanderingHero(size_t index)
 {
 	if(index < wanderingHeroes.size())
 		return wanderingHeroes[index];
-	return nullptr;
+	throw std::runtime_error("No hero with index " + std::to_string(index));
 }
 
 void PlayerLocalState::addWanderingHero(const CGHeroInstance * hero)
@@ -235,10 +235,10 @@ void PlayerLocalState::removeWanderingHero(const CGHeroInstance * hero)
 		setSelection(ownedTowns.front());
 }
 
-void PlayerLocalState::swapWanderingHero(int pos1, int pos2)
+void PlayerLocalState::swapWanderingHero(size_t pos1, size_t pos2)
 {
 	assert(wanderingHeroes[pos1] && wanderingHeroes[pos2]);
-	std::swap(wanderingHeroes[pos1], wanderingHeroes[pos2]);
+	std::swap(wanderingHeroes.at(pos1), wanderingHeroes.at(pos2));
 
 	adventureInt->onHeroOrderChanged();
 }
@@ -252,7 +252,7 @@ const CGTownInstance * PlayerLocalState::getOwnedTown(size_t index)
 {
 	if(index < ownedTowns.size())
 		return ownedTowns[index];
-	return nullptr;
+	throw std::runtime_error("No town with index " + std::to_string(index));
 }
 
 void PlayerLocalState::addOwnedTown(const CGTownInstance * town)
@@ -278,10 +278,10 @@ void PlayerLocalState::removeOwnedTown(const CGTownInstance * town)
 		setSelection(ownedTowns.front());
 }
 
-void PlayerLocalState::swapOwnedTowns(int pos1, int pos2)
+void PlayerLocalState::swapOwnedTowns(size_t pos1, size_t pos2)
 {
 	assert(ownedTowns[pos1] && ownedTowns[pos2]);
-	std::swap(ownedTowns[pos1], ownedTowns[pos2]);
+	std::swap(ownedTowns.at(pos1), ownedTowns.at(pos2));
 
 	adventureInt->onTownOrderChanged();
 }

client/PlayerLocalState.h

@@ -66,14 +66,14 @@ public:
 	const CGTownInstance * getOwnedTown(size_t index);
 	void addOwnedTown(const CGTownInstance * hero);
 	void removeOwnedTown(const CGTownInstance * hero);
-	void swapOwnedTowns(int pos1, int pos2);
+	void swapOwnedTowns(size_t pos1, size_t pos2);
 
 	const std::vector<const CGHeroInstance *> & getWanderingHeroes();
 	const CGHeroInstance * getWanderingHero(size_t index);
 	const CGHeroInstance * getNextWanderingHero(const CGHeroInstance * hero);
 	void addWanderingHero(const CGHeroInstance * hero);
 	void removeWanderingHero(const CGHeroInstance * hero);
-	void swapWanderingHero(int pos1, int pos2);
+	void swapWanderingHero(size_t pos1, size_t pos2);
 
 	void setPath(const CGHeroInstance * h, const CGPath & path);
 	bool setPath(const CGHeroInstance * h, const int3 & destination);

client/adventureMap/CList.cpp

@@ -279,14 +279,14 @@ void CHeroList::CHeroItem::gesture(bool on, const Point & initialPosition, const
 	if(heroes.size() < 2)
 		return;
 
-	int heroPos = vstd::find_pos(heroes, hero);
-	const CGHeroInstance * heroUpper = (heroPos < 1) ? nullptr : heroes[heroPos - 1];
-	const CGHeroInstance * heroLower = (heroPos > heroes.size() - 2) ? nullptr : heroes[heroPos + 1];
+	size_t heroPos = vstd::find_pos(heroes, hero);
+	const CGHeroInstance * heroUpper = (heroPos < 1) ? nullptr : heroes.at(heroPos - 1);
+	const CGHeroInstance * heroLower = (heroPos > heroes.size() - 2) ? nullptr : heroes.at(heroPos + 1);
 
 	std::vector<RadialMenuConfig> menuElements = {
 		{ RadialMenuConfig::ITEM_ALT_NN, heroUpper != nullptr, "altUpTop", "vcmi.radialWheel.moveTop", [heroPos]()
 		{
-			for (int i = heroPos; i > 0; i--)
+			for (size_t i = heroPos; i > 0; i--)
 				LOCPLINT->localState->swapWanderingHero(i, i - 1);
 		} },
 		{ RadialMenuConfig::ITEM_ALT_NW, heroUpper != nullptr, "altUp", "vcmi.radialWheel.moveUp", [heroPos](){LOCPLINT->localState->swapWanderingHero(heroPos, heroPos - 1); } },
@@ -326,7 +326,7 @@ void CHeroList::updateElement(const CGHeroInstance * hero)
 
 void CHeroList::updateWidget()
 {
-	auto & heroes = LOCPLINT->localState->getWanderingHeroes();
+	const auto & heroes = LOCPLINT->localState->getWanderingHeroes();
 
 	listBox->resize(heroes.size());
 
@@ -337,7 +337,7 @@ void CHeroList::updateWidget()
 		if (!item)
 			continue;
 
-		if (item->hero == heroes[i])
+		if (item->hero == heroes.at(i))
 		{
 			item->update();
 		}
@@ -362,11 +362,9 @@ std::shared_ptr<CIntObject> CTownList::createItem(size_t index)
 }
 
 CTownList::CTownItem::CTownItem(CTownList *parent, const CGTownInstance *Town):
-	CListItem(parent)
+	CListItem(parent),
+	town(Town)
 {
-	const std::vector<const CGTownInstance *> towns = LOCPLINT->localState->getOwnedTowns();
-	townIndex = std::distance(towns.begin(), std::find(towns.begin(), towns.end(), Town));
-
 	OBJECT_CONSTRUCTION_CAPTURING(255-DISPOSE);
 	picture = std::make_shared<CAnimImage>(AnimationPath::builtin("ITPA"), 0);
 	pos = picture->pos;
@@ -382,7 +380,6 @@ std::shared_ptr<CIntObject> CTownList::CTownItem::genSelection()
 
 void CTownList::CTownItem::update()
 {
-	const CGTownInstance * town = LOCPLINT->localState->getOwnedTowns()[townIndex];
 	size_t iconIndex = town->town->clientInfo.icons[town->hasFort()][town->builded >= CGI->settings()->getInteger(EGameSettings::TOWNS_BUILDINGS_PER_TURN_CAP)];
 
 	picture->setFrame(iconIndex + 2);
@@ -392,17 +389,17 @@ void CTownList::CTownItem::update()
 void CTownList::CTownItem::select(bool on)
 {
 	if(on)
-		LOCPLINT->localState->setSelection(LOCPLINT->localState->getOwnedTowns()[townIndex]);
+		LOCPLINT->localState->setSelection(town);
 }
 
 void CTownList::CTownItem::open()
 {
-	LOCPLINT->openTownWindow(LOCPLINT->localState->getOwnedTowns()[townIndex]);
+	LOCPLINT->openTownWindow(town);
 }
 
 void CTownList::CTownItem::showTooltip()
 {
-	CRClickPopup::createAndPush(LOCPLINT->localState->getOwnedTowns()[townIndex], GH.getCursorPosition());
+	CRClickPopup::createAndPush(town, GH.getCursorPosition());
 }
 
 void CTownList::CTownItem::gesture(bool on, const Point & initialPosition, const Point & finalPosition)
@@ -411,8 +408,9 @@ void CTownList::CTownItem::gesture(bool on, const Point & initialPosition, const
 		return;
 
 	const std::vector<const CGTownInstance *> towns = LOCPLINT->localState->getOwnedTowns();
+	size_t townIndex = vstd::find_pos(towns, town);
 
-	if(townIndex < 0 || townIndex > towns.size() - 1 || !towns[townIndex])
+	if(townIndex + 1 > towns.size() || !towns.at(townIndex))
 		return;
 
 	if(towns.size() < 2)
@@ -422,14 +420,14 @@ void CTownList::CTownItem::gesture(bool on, const Point & initialPosition, const
 	int townLowerPos = (townIndex > towns.size() - 2) ? -1 : townIndex + 1;
 
 	std::vector<RadialMenuConfig> menuElements = {
-		{ RadialMenuConfig::ITEM_ALT_NN, townUpperPos > -1, "altUpTop", "vcmi.radialWheel.moveTop", [this]()
+		{ RadialMenuConfig::ITEM_ALT_NN, townUpperPos > -1, "altUpTop", "vcmi.radialWheel.moveTop", [townIndex]()
 		{
 			for (int i = townIndex; i > 0; i--)
 				LOCPLINT->localState->swapOwnedTowns(i, i - 1);
 		} },
-		{ RadialMenuConfig::ITEM_ALT_NW, townUpperPos > -1, "altUp", "vcmi.radialWheel.moveUp", [this, townUpperPos](){LOCPLINT->localState->swapOwnedTowns(townIndex, townUpperPos); } },
-		{ RadialMenuConfig::ITEM_ALT_SW, townLowerPos > -1, "altDown", "vcmi.radialWheel.moveDown", [this, townLowerPos](){ LOCPLINT->localState->swapOwnedTowns(townIndex, townLowerPos); } },
-		{ RadialMenuConfig::ITEM_ALT_SS, townLowerPos > -1, "altDownBottom", "vcmi.radialWheel.moveBottom", [this, towns]()
+		{ RadialMenuConfig::ITEM_ALT_NW, townUpperPos > -1, "altUp", "vcmi.radialWheel.moveUp", [townIndex, townUpperPos](){LOCPLINT->localState->swapOwnedTowns(townIndex, townUpperPos); } },
+		{ RadialMenuConfig::ITEM_ALT_SW, townLowerPos > -1, "altDown", "vcmi.radialWheel.moveDown", [townIndex, townLowerPos](){ LOCPLINT->localState->swapOwnedTowns(townIndex, townLowerPos); } },
+		{ RadialMenuConfig::ITEM_ALT_SS, townLowerPos > -1, "altDownBottom", "vcmi.radialWheel.moveBottom", [townIndex, towns]()
 		{
 			for (int i = townIndex; i < towns.size() - 1; i++)
 				LOCPLINT->localState->swapOwnedTowns(i, i + 1);
@@ -441,7 +439,7 @@ void CTownList::CTownItem::gesture(bool on, const Point & initialPosition, const
 
 std::string CTownList::CTownItem::getHoverText()
 {
-	return LOCPLINT->localState->getOwnedTowns()[townIndex]->getObjectName();
+	return town->getObjectName();
 }
 
 CTownList::CTownList(int visibleItemsCount, Rect widgetPosition, Point firstItemOffset, Point itemOffsetDelta, size_t initialItemsCount)
@@ -473,7 +471,15 @@ void CTownList::updateWidget()
 		if (!item)
 			continue;
 
-		listBox->reset();
+		if (item->town == towns[i])
+		{
+			item->update();
+		}
+		else
+		{
+			listBox->reset();
+			break;
+		}
 	}
 
 	if (LOCPLINT->localState->getCurrentTown())

client/adventureMap/CList.h

@@ -152,7 +152,7 @@ class CTownList	: public CList
 	{
 		std::shared_ptr<CAnimImage> picture;
 	public:
-		int townIndex;
+		const CGTownInstance * const town;
 
 		CTownItem(CTownList *parent, const CGTownInstance * town);