Merge pull request #3973 from vcmi/master

Merge master -> beta

ChangeLog.md

@@ -1,3 +1,14 @@
+# 1.5.0 -> 1.5.1
+
+### Stability
+* Fixed possible crash on accessing faction description
+* Fixed possible thread race on leaving to main menu
+* Game will now show error message instead of silent crash on corrupted H3 data
+* Fixed possible crash on double-deletion of quest artifacts placed by RMG
+
+### Interface
+* Fixed possible freeze on attempt to move hero when hero has non-zero movement points but not enough to reach first tile in path
+
 # 1.4.5 -> 1.5.0
 
 ### General

android/vcmi-app/build.gradle

@@ -10,7 +10,7 @@ android {
 		applicationId "is.xyz.vcmi"
 		minSdk 19
 		targetSdk 33
-		versionCode 1510
+		versionCode 1511
 		versionName "1.5.1"
 		setProperty("archivesBaseName", "vcmi")
 	}

client/CMT.cpp

@@ -31,6 +31,7 @@
 #include "../lib/CConfigHandler.h"
 #include "../lib/CGeneralTextHandler.h"
 #include "../lib/CThreadHelper.h"
+#include "../lib/ExceptionsCommon.h"
 #include "../lib/VCMIDirs.h"
 #include "../lib/VCMI_Lib.h"
 #include "../lib/filesystem/Filesystem.h"
@@ -56,6 +57,7 @@ namespace po = boost::program_options;
 namespace po_style = boost::program_options::command_line_style;
 
 static std::atomic<bool> headlessQuit = false;
+static std::optional<std::string> criticalInitializationError;
 
 #ifndef VCMI_IOS
 void processCommand(const std::string &message);
@@ -69,9 +71,16 @@ static CBasicLogConfigurator *logConfig;
 void init()
 {
 	CStopWatch tmh;
-
-	loadDLLClasses();
-	CGI->setFromLib();
+	try
+	{
+		loadDLLClasses();
+		CGI->setFromLib();
+	}
+	catch (const DataLoadingException & e)
+	{
+		criticalInitializationError = e.what();
+		return;
+	}
 
 	logGlobal->info("Initializing VCMI_Lib: %d ms", tmh.getDiff());
 
@@ -322,6 +331,11 @@ int main(int argc, char * argv[])
 	#endif // ANDROID
 #endif // THREADED
 
+	if (criticalInitializationError.has_value())
+	{
+		handleFatalError(criticalInitializationError.value(), false);
+	}
+
 	if(!settings["session"]["headless"].Bool())
 	{
 		pomtime.getDiff();
@@ -412,7 +426,7 @@ static void mainLoop()
 	}
 }
 
-[[noreturn]] static void quitApplicationImmediately()
+[[noreturn]] static void quitApplicationImmediately(int error_code)
 {
 	// Perform quick exit without executing static destructors and let OS cleanup anything that we did not
 	// We generally don't care about them and this leads to numerous issues, e.g.
@@ -420,9 +434,9 @@ static void mainLoop()
 	// Android - std::quick_exit is available only starting from API level 21
 	// Mingw, macOS and iOS - std::quick_exit is unavailable (at least in current version of CI)
 #if (defined(__ANDROID_API__) && __ANDROID_API__ < 21) || (defined(__MINGW32__)) || defined(VCMI_APPLE)
-	::exit(0);
+	::exit(error_code);
 #else
-	std::quick_exit(0);
+	std::quick_exit(error_code);
 #endif
 }
 
@@ -476,7 +490,7 @@ static void mainLoop()
 	}
 
 	std::cout << "Ending...\n";
-	quitApplicationImmediately();
+	quitApplicationImmediately(0);
 }
 
 void handleQuit(bool ask)
@@ -500,7 +514,7 @@ void handleQuit(bool ask)
 	// proper solution would be to abort init thread (or wait for it to finish)
 	if (!CCS->curh)
 	{
-		quitApplicationImmediately();
+		quitApplicationImmediately(0);
 	}
 
 	if (LOCPLINT)
@@ -521,5 +535,5 @@ void handleFatalError(const std::string & message, bool terminate)
 	if (terminate)
 		throw std::runtime_error(message);
 	else
-		exit(1);
+		quitApplicationImmediately(1);
 }

client/CServerHandler.cpp

@@ -119,7 +119,10 @@ CServerHandler::~CServerHandler()
 		if (serverRunner)
 			serverRunner->wait();
 		serverRunner.reset();
-		threadNetwork.join();
+		{
+			auto unlockInterface = vstd::makeUnlockGuard(GH.interfaceMutex);
+			threadNetwork.join();
+		}
 	}
 	catch (const std::runtime_error & e)
 	{
@@ -421,6 +424,7 @@ void CServerHandler::sendClientDisconnecting()
 	networkConnection->close();
 	networkConnection.reset();
 	logicConnection.reset();
+	waitForServerShutdown();
 }
 
 void CServerHandler::setCampaignState(std::shared_ptr<CampaignState> newCampaign)
@@ -901,6 +905,8 @@ void CServerHandler::onPacketReceived(const std::shared_ptr<INetworkConnection>
 
 void CServerHandler::onDisconnected(const std::shared_ptr<INetworkConnection> & connection, const std::string & errorMessage)
 {
+	boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
+
 	if (connection != networkConnection)
 	{
 		// ServerHandler already closed this connection on its own
@@ -918,8 +924,6 @@ void CServerHandler::onDisconnected(const std::shared_ptr<INetworkConnection> &
 		return;
 	}
 
-	boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
-
 	logNetwork->error("Lost connection to server! Connection has been closed");
 
 	if(client)
@@ -954,7 +958,6 @@ void CServerHandler::waitForServerShutdown()
 	}
 	else
 	{
-		boost::mutex::scoped_lock interfaceLock(GH.interfaceMutex);
 		if (getState() == EClientState::CONNECTING)
 		{
 			showServerError(CGI->generaltexth->translate("vcmi.server.errors.existingProcess"));

client/Client.cpp

@@ -519,7 +519,10 @@ void CClient::handlePack(CPack * pack)
 	{
 		apply->applyOnClBefore(this, pack);
 		logNetwork->trace("\tMade first apply on cl: %s", typeid(*pack).name());
-		gs->apply(pack);
+		{
+			boost::unique_lock<boost::shared_mutex> lock(CGameState::mutex);
+			gs->apply(pack);
+		}
 		logNetwork->trace("\tApplied on gs: %s", typeid(*pack).name());
 		apply->applyOnClAfter(this, pack);
 		logNetwork->trace("\tMade second apply on cl: %s", typeid(*pack).name());

client/adventureMap/AdventureMapShortcuts.cpp

@@ -456,7 +456,7 @@ bool AdventureMapShortcuts::optionHeroSelected()
 bool AdventureMapShortcuts::optionHeroCanMove()
 {
 	const auto * hero = LOCPLINT->localState->getCurrentHero();
-	return optionInMapView() && hero && hero->movementPointsRemaining() != 0 && LOCPLINT->localState->hasPath(hero);
+	return optionInMapView() && hero && LOCPLINT->localState->hasPath(hero) && LOCPLINT->localState->getPath(hero).nextNode().turns == 0;
 }
 
 bool AdventureMapShortcuts::optionHasNextHero()

client/widgets/Images.cpp

@@ -334,8 +334,12 @@ CShowableAnim::CShowableAnim(int x, int y, const AnimationPath & name, ui8 Flags
 	anim->loadGroup(group);
 	last = anim->size(group);
 
-	pos.w = anim->getImage(0, group)->width();
-	pos.h = anim->getImage(0, group)->height();
+	auto image = anim->getImage(0, group);
+	if (!image)
+		throw std::runtime_error("Failed to load group " + std::to_string(group) + " of animation file " + name.getOriginalName());
+
+	pos.w = image->width();
+	pos.h = image->height();
 	pos.x+= x;
 	pos.y+= y;
 

client/widgets/MiscWidgets.cpp

@@ -466,7 +466,7 @@ void CInteractableTownTooltip::init(const CGTownInstance * town)
 			if(town->id == townId && town->builtBuildings.count(BuildingID::TAVERN))
 				LOCPLINT->showTavernWindow(town, nullptr, QueryID::NONE);
 		}
-	}, [&]{
+	}, [town]{
 		if(!town->town->faction->getDescriptionTranslated().empty())
 			CRClickPopup::createAndPush(town->town->faction->getDescriptionTranslated());
 	});

client/windows/CCastleInterface.cpp

@@ -1351,7 +1351,7 @@ void CCastleInterface::recreateIcons()
 	{
 		if(town->builtBuildings.count(BuildingID::TAVERN))
 			LOCPLINT->showTavernWindow(town, nullptr, QueryID::NONE);
-	}, [&]{
+	}, [this]{
 		if(!town->town->faction->getDescriptionTranslated().empty())
 			CRClickPopup::createAndPush(town->town->faction->getDescriptionTranslated());
 	});

lib/CArtHandler.cpp

@@ -12,6 +12,7 @@
 
 #include "ArtifactUtils.h"
 #include "CGeneralTextHandler.h"
+#include "ExceptionsCommon.h"
 #include "GameSettings.h"
 #include "mapObjects/MapObjects.h"
 #include "constants/StringConstants.h"
@@ -354,7 +355,13 @@ std::vector<JsonNode> CArtHandler::loadLegacyData()
 				artData["slot"].Vector().emplace_back(artSlot);
 			}
 		}
-		artData["class"].String() = classes.at(parser.readString()[0]);
+
+		std::string artClass = parser.readString();
+
+		if (classes.count(artClass[0]))
+			artData["class"].String() = classes.at(artClass[0]);
+		else
+			throw DataLoadingException("File ARTRAITS.TXT is invalid or corrupted! Please reinstall Heroes III data files");
 		artData["text"]["description"].String() = parser.readString();
 
 		parser.endLine();

lib/CMakeLists.txt

@@ -660,6 +660,7 @@ set(lib_MAIN_HEADERS
 	CStack.h
 	CStopWatch.h
 	CTownHandler.h
+	ExceptionsCommon.h
 	ExtraOptionsInfo.h
 	FunctionList.h
 	GameCallbackHolder.h

lib/ExceptionsCommon.h

@@ -0,0 +1,16 @@
+/*
+ * ExceptionsCommon.h, part of VCMI engine
+ *
+ * Authors: listed in file AUTHORS in main folder
+ *
+ * License: GNU General Public License v2.0 or later
+ * Full text of license available in license.txt file, in main folder
+ *
+ */
+#pragma once
+
+class DLL_LINKAGE DataLoadingException: public std::runtime_error
+{
+public:
+    using std::runtime_error::runtime_error;
+};

lib/MetaString.cpp

@@ -144,33 +144,33 @@ DLL_LINKAGE std::string MetaString::toString() const
 		switch(elem)
 		{
 			case EMessage::APPEND_RAW_STRING:
-				dst += exactStrings[exSt++];
+				dst += exactStrings.at(exSt++);
 				break;
 			case EMessage::APPEND_LOCAL_STRING:
-				dst += getLocalString(localStrings[loSt++]);
+				dst += getLocalString(localStrings.at(loSt++));
 				break;
 			case EMessage::APPEND_TEXTID_STRING:
-				dst += VLC->generaltexth->translate(stringsTextID[textID++]);
+				dst += VLC->generaltexth->translate(stringsTextID.at(textID++));
 				break;
 			case EMessage::APPEND_NUMBER:
-				dst += std::to_string(numbers[nums++]);
+				dst += std::to_string(numbers.at(nums++));
 				break;
 			case EMessage::REPLACE_RAW_STRING:
-				boost::replace_first(dst, "%s", exactStrings[exSt++]);
+				boost::replace_first(dst, "%s", exactStrings.at(exSt++));
 				break;
 			case EMessage::REPLACE_LOCAL_STRING:
-				boost::replace_first(dst, "%s", getLocalString(localStrings[loSt++]));
+				boost::replace_first(dst, "%s", getLocalString(localStrings.at(loSt++)));
 				break;
 			case EMessage::REPLACE_TEXTID_STRING:
-				boost::replace_first(dst, "%s", VLC->generaltexth->translate(stringsTextID[textID++]));
+				boost::replace_first(dst, "%s", VLC->generaltexth->translate(stringsTextID.at(textID++)));
 				break;
 			case EMessage::REPLACE_NUMBER:
-				boost::replace_first(dst, "%d", std::to_string(numbers[nums++]));
+				boost::replace_first(dst, "%d", std::to_string(numbers.at(nums++)));
 				break;
 			case EMessage::REPLACE_POSITIVE_NUMBER:
 				if (dst.find("%+d") != std::string::npos)
 				{
-					int64_t value = numbers[nums];
+					int64_t value = numbers.at(nums);
 					if (value > 0)
 						boost::replace_first(dst, "%+d", '+' + std::to_string(value));
 					else
@@ -179,7 +179,7 @@ DLL_LINKAGE std::string MetaString::toString() const
 					nums++;
 				}
 				else
-					boost::replace_first(dst, "%d", std::to_string(numbers[nums++]));
+					boost::replace_first(dst, "%d", std::to_string(numbers.at(nums++)));
 				break;
 			default:
 				logGlobal->error("MetaString processing error! Received message of type %d", static_cast<int>(elem));
@@ -199,41 +199,41 @@ DLL_LINKAGE std::string MetaString::buildList() const
 	std::string lista;
 	for(int i = 0; i < message.size(); ++i)
 	{
-		if(i > 0 && (message[i] == EMessage::APPEND_RAW_STRING || message[i] == EMessage::APPEND_LOCAL_STRING))
+		if(i > 0 && (message.at(i) == EMessage::APPEND_RAW_STRING || message.at(i) == EMessage::APPEND_LOCAL_STRING))
 		{
 			if(exSt == exactStrings.size() - 1)
 				lista += VLC->generaltexth->allTexts[141]; //" and "
 			else
 				lista += ", ";
 		}
-		switch(message[i])
+		switch(message.at(i))
 		{
 			case EMessage::APPEND_RAW_STRING:
-				lista += exactStrings[exSt++];
+				lista += exactStrings.at(exSt++);
 				break;
 			case EMessage::APPEND_LOCAL_STRING:
-				lista += getLocalString(localStrings[loSt++]);
+				lista += getLocalString(localStrings.at(loSt++));
 				break;
 			case EMessage::APPEND_TEXTID_STRING:
-				lista += VLC->generaltexth->translate(stringsTextID[textID++]);
+				lista += VLC->generaltexth->translate(stringsTextID.at(textID++));
 				break;
 			case EMessage::APPEND_NUMBER:
-				lista += std::to_string(numbers[nums++]);
+				lista += std::to_string(numbers.at(nums++));
 				break;
 			case EMessage::REPLACE_RAW_STRING:
-				lista.replace(lista.find("%s"), 2, exactStrings[exSt++]);
+				lista.replace(lista.find("%s"), 2, exactStrings.at(exSt++));
 				break;
 			case EMessage::REPLACE_LOCAL_STRING:
-				lista.replace(lista.find("%s"), 2, getLocalString(localStrings[loSt++]));
+				lista.replace(lista.find("%s"), 2, getLocalString(localStrings.at(loSt++)));
 				break;
 			case EMessage::REPLACE_TEXTID_STRING:
-				lista.replace(lista.find("%s"), 2, VLC->generaltexth->translate(stringsTextID[textID++]));
+				lista.replace(lista.find("%s"), 2, VLC->generaltexth->translate(stringsTextID.at(textID++)));
 				break;
 			case EMessage::REPLACE_NUMBER:
-				lista.replace(lista.find("%d"), 2, std::to_string(numbers[nums++]));
+				lista.replace(lista.find("%d"), 2, std::to_string(numbers.at(nums++)));
 				break;
 			default:
-				logGlobal->error("MetaString processing error! Received message of type %d", int(message[i]));
+				logGlobal->error("MetaString processing error! Received message of type %d", int(message.at(i)));
 		}
 	}
 	return lista;

lib/gameState/CGameState.cpp

@@ -75,8 +75,6 @@ public:
 	void applyOnGS(CGameState *gs, CPack * pack) const override
 	{
 		T *ptr = static_cast<T*>(pack);
-
-		boost::unique_lock<boost::shared_mutex> lock(CGameState::mutex);
 		ptr->applyGs(gs);
 	}
 };
@@ -1101,6 +1099,9 @@ BattleField CGameState::battleGetBattlefieldType(int3 tile, CRandomGenerator & r
 	if(map->isCoastalTile(tile)) //coastal tile is always ground
 		return BattleField(*VLC->identifiers()->getIdentifier("core", "battlefield.sand_shore"));
 	
+	if (t.terType->battleFields.empty())
+		throw std::runtime_error("Failed to find battlefield for terrain " + t.terType->getJsonKey());
+
 	return BattleField(*RandomGeneratorUtil::nextItem(t.terType->battleFields, rand));
 }
 

lib/network/NetworkConnection.cpp

@@ -117,7 +117,7 @@ void NetworkConnection::onPacketReceived(const boost::system::error_code & ec, u
 
 	if (readBuffer.size() < expectedPacketSize)
 	{
-		throw std::runtime_error("Failed to read packet!");
+		throw std::runtime_error("Failed to read packet! " + std::to_string(readBuffer.size()) + " bytes read, but " + std::to_string(expectedPacketSize) + " bytes expected!");
 	}
 
 	std::vector<std::byte> message(expectedPacketSize);

lib/networkPacks/NetPacksLib.cpp

@@ -1579,7 +1579,7 @@ void ChangeStackCount::applyGs(CGameState * gs)
 {
 	auto * srcObj = gs->getArmyInstance(army);
 	if(!srcObj)
-		logNetwork->error("[CRITICAL] ChangeStackCount: invalid army object %d, possible game state corruption.", army.getNum());
+		throw std::runtime_error("ChangeStackCount: invalid army object " + std::to_string(army.getNum()) + ", possible game state corruption.");
 
 	if(absoluteValue)
 		srcObj->setStackCount(slot, count);
@@ -1591,7 +1591,7 @@ void SetStackType::applyGs(CGameState * gs)
 {
 	auto * srcObj = gs->getArmyInstance(army);
 	if(!srcObj)
-		logNetwork->error("[CRITICAL] SetStackType: invalid army object %d, possible game state corruption.", army.getNum());
+		throw std::runtime_error("SetStackType: invalid army object " + std::to_string(army.getNum()) + ", possible game state corruption.");
 
 	srcObj->setStackType(slot, type);
 }
@@ -1600,7 +1600,7 @@ void EraseStack::applyGs(CGameState * gs)
 {
 	auto * srcObj = gs->getArmyInstance(army);
 	if(!srcObj)
-		logNetwork->error("[CRITICAL] EraseStack: invalid army object %d, possible game state corruption.", army.getNum());
+		throw std::runtime_error("EraseStack: invalid army object " + std::to_string(army.getNum()) + ", possible game state corruption.");
 
 	srcObj->eraseStack(slot);
 }
@@ -1609,11 +1609,11 @@ void SwapStacks::applyGs(CGameState * gs)
 {
 	auto * srcObj = gs->getArmyInstance(srcArmy);
 	if(!srcObj)
-		logNetwork->error("[CRITICAL] SwapStacks: invalid army object %d, possible game state corruption.", srcArmy.getNum());
+		throw std::runtime_error("SwapStacks: invalid army object " + std::to_string(srcArmy.getNum()) + ", possible game state corruption.");
 
 	auto * dstObj = gs->getArmyInstance(dstArmy);
 	if(!dstObj)
-		logNetwork->error("[CRITICAL] SwapStacks: invalid army object %d, possible game state corruption.", dstArmy.getNum());
+		throw std::runtime_error("SwapStacks: invalid army object " + std::to_string(dstArmy.getNum()) + ", possible game state corruption.");
 
 	CStackInstance * s1 = srcObj->detachStack(srcSlot);
 	CStackInstance * s2 = dstObj->detachStack(dstSlot);
@@ -1627,18 +1627,18 @@ void InsertNewStack::applyGs(CGameState *gs)
 	if(auto * obj = gs->getArmyInstance(army))
 		obj->putStack(slot, new CStackInstance(type, count));
 	else
-		logNetwork->error("[CRITICAL] InsertNewStack: invalid army object %d, possible game state corruption.", army.getNum());
+		throw std::runtime_error("InsertNewStack: invalid army object " + std::to_string(army.getNum()) + ", possible game state corruption.");
 }
 
 void RebalanceStacks::applyGs(CGameState * gs)
 {
 	auto * srcObj = gs->getArmyInstance(srcArmy);
 	if(!srcObj)
-		logNetwork->error("[CRITICAL] RebalanceStacks: invalid army object %d, possible game state corruption.", srcArmy.getNum());
+		throw std::runtime_error("RebalanceStacks: invalid army object " + std::to_string(srcArmy.getNum()) + ", possible game state corruption.");
 
 	auto * dstObj = gs->getArmyInstance(dstArmy);
 	if(!dstObj)
-		logNetwork->error("[CRITICAL] RebalanceStacks: invalid army object %d, possible game state corruption.", dstArmy.getNum());
+		throw std::runtime_error("RebalanceStacks: invalid army object " + std::to_string(dstArmy.getNum()) + ", possible game state corruption.");
 
 	StackLocation src(srcObj, srcSlot);
 	StackLocation dst(dstObj, dstSlot);

lib/rmg/CMapGenerator.cpp

@@ -126,6 +126,9 @@ std::unique_ptr<CMap> CMapGenerator::generate()
 		fillZones();
 		//updated guarded tiles will be calculated in CGameState::initMapObjects()
 		map->getZones().clear();
+
+		// undo manager keeps pointers to object that might be removed during gameplay. Remove them to prevent any hanging pointer after gameplay
+		map->getEditManager()->getUndoManager().clearAll();
 	}
 	catch (rmgException &e)
 	{