diff --git a/ChangeLog.md b/ChangeLog.md index 3f2d41954..5dd3b17e1 100644 --- a/ChangeLog.md +++ b/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 diff --git a/android/vcmi-app/build.gradle b/android/vcmi-app/build.gradle index 4960a1551..c8bb991f0 100644 --- a/android/vcmi-app/build.gradle +++ b/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") } diff --git a/client/CMT.cpp b/client/CMT.cpp index 10c4ea893..be8e262d5 100644 --- a/client/CMT.cpp +++ b/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 headlessQuit = false; +static std::optional 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); } diff --git a/client/CServerHandler.cpp b/client/CServerHandler.cpp index 401a8e5d6..9bfeaae32 100644 --- a/client/CServerHandler.cpp +++ b/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 newCampaign) @@ -901,6 +905,8 @@ void CServerHandler::onPacketReceived(const std::shared_ptr void CServerHandler::onDisconnected(const std::shared_ptr & 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 & 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")); diff --git a/client/Client.cpp b/client/Client.cpp index eab9fea01..05d495004 100644 --- a/client/Client.cpp +++ b/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 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()); diff --git a/client/adventureMap/AdventureMapShortcuts.cpp b/client/adventureMap/AdventureMapShortcuts.cpp index 1d4d54e17..4fa492004 100644 --- a/client/adventureMap/AdventureMapShortcuts.cpp +++ b/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() diff --git a/client/widgets/Images.cpp b/client/widgets/Images.cpp index f4ce12ce3..6699476da 100644 --- a/client/widgets/Images.cpp +++ b/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; diff --git a/client/widgets/MiscWidgets.cpp b/client/widgets/MiscWidgets.cpp index 28503f72f..20ff09686 100644 --- a/client/widgets/MiscWidgets.cpp +++ b/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()); }); diff --git a/client/windows/CCastleInterface.cpp b/client/windows/CCastleInterface.cpp index 449f87871..2fbd723c2 100644 --- a/client/windows/CCastleInterface.cpp +++ b/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()); }); diff --git a/lib/CArtHandler.cpp b/lib/CArtHandler.cpp index c85d67918..9cdd430e9 100644 --- a/lib/CArtHandler.cpp +++ b/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 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(); diff --git a/lib/CMakeLists.txt b/lib/CMakeLists.txt index 7ddfd46d5..b00c41afe 100644 --- a/lib/CMakeLists.txt +++ b/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 diff --git a/lib/ExceptionsCommon.h b/lib/ExceptionsCommon.h new file mode 100644 index 000000000..b33d78106 --- /dev/null +++ b/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; +}; diff --git a/lib/MetaString.cpp b/lib/MetaString.cpp index 92d84e851..db2e015dd 100644 --- a/lib/MetaString.cpp +++ b/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(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; diff --git a/lib/gameState/CGameState.cpp b/lib/gameState/CGameState.cpp index 58163b285..69427e7fd 100644 --- a/lib/gameState/CGameState.cpp +++ b/lib/gameState/CGameState.cpp @@ -75,8 +75,6 @@ public: void applyOnGS(CGameState *gs, CPack * pack) const override { T *ptr = static_cast(pack); - - boost::unique_lock 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)); } diff --git a/lib/network/NetworkConnection.cpp b/lib/network/NetworkConnection.cpp index 9c2972cdc..cef7f8263 100644 --- a/lib/network/NetworkConnection.cpp +++ b/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 message(expectedPacketSize); diff --git a/lib/networkPacks/NetPacksLib.cpp b/lib/networkPacks/NetPacksLib.cpp index 28d8779d7..ead5778d2 100644 --- a/lib/networkPacks/NetPacksLib.cpp +++ b/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); diff --git a/lib/rmg/CMapGenerator.cpp b/lib/rmg/CMapGenerator.cpp index fddf69f8e..34ef5fe7d 100644 --- a/lib/rmg/CMapGenerator.cpp +++ b/lib/rmg/CMapGenerator.cpp @@ -126,6 +126,9 @@ std::unique_ptr 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) {