From b74da11fc6f6ac746bc7e7ccc73e5921b1a61499 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Tue, 12 Oct 2021 21:04:11 +0100 Subject: [PATCH] Don't try to get the address of the zeroth element in an empty array It happens to behave nicely with MSVC's RelWithDebInfo, but it's undefined behaviour, and GCC (I don't know what parameters were used) errors due to a bounds-check assertion failure. --- src/api/game/game.cpp | 4 +- src/api/helpers/text.cpp | 15 ++++++ src/api/metadata/condition_evaluator.cpp | 46 +++++++++++----- src/api/metadata/condition_evaluator.h | 5 +- src/api/metadata/yaml/tag.h | 2 +- src/tests/api/internals/helpers/text_test.h | 5 ++ .../metadata/condition_evaluator_test.h | 53 ++++++++++++++++++- 7 files changed, 109 insertions(+), 21 deletions(-) diff --git a/src/api/game/game.cpp b/src/api/game/game.cpp index acc0b196..d6433344 100644 --- a/src/api/game/game.cpp +++ b/src/api/game/game.cpp @@ -191,7 +191,7 @@ void Game::LoadPlugins(const std::vector& plugins, thread.join(); } - conditionEvaluator_->RefreshState(cache_); + conditionEvaluator_->RefreshLoadedPluginsState(GetLoadedPlugins()); } std::shared_ptr Game::GetPlugin( @@ -224,7 +224,7 @@ std::vector Game::SortPlugins( void Game::LoadCurrentLoadOrderState() { loadOrderHandler_->LoadCurrentState(); - conditionEvaluator_->RefreshState(loadOrderHandler_); + conditionEvaluator_->RefreshActivePluginsState(loadOrderHandler_->GetActivePlugins()); } bool Game::IsPluginActive(const std::string& pluginName) const { diff --git a/src/api/helpers/text.cpp b/src/api/helpers/text.cpp index 2f1e1843..f06a44a2 100644 --- a/src/api/helpers/text.cpp +++ b/src/api/helpers/text.cpp @@ -127,6 +127,11 @@ std::optional ExtractVersion(const std::string& text) { #ifdef _WIN32 std::wstring ToWinWide(const std::string& str) { size_t len = MultiByteToWideChar(CP_UTF8, 0, str.c_str(), str.length(), 0, 0); + + if (len == 0) { + return std::wstring(); + } + std::wstring wstr(len, 0); MultiByteToWideChar(CP_UTF8, 0, str.c_str(), str.length(), &wstr[0], len); return wstr; @@ -135,6 +140,11 @@ std::wstring ToWinWide(const std::string& str) { std::string FromWinWide(const std::wstring& wstr) { size_t len = WideCharToMultiByte( CP_UTF8, 0, wstr.c_str(), wstr.length(), NULL, 0, NULL, NULL); + + if (len == 0) { + return std::string(); + } + std::string str(len, 0); WideCharToMultiByte( CP_UTF8, 0, wstr.c_str(), wstr.length(), &str[0], len, NULL, NULL); @@ -170,6 +180,11 @@ int CompareFilenames(const std::string& lhs, const std::string& rhs) { std::string NormalizeFilename(const std::string& filename) { #ifdef _WIN32 auto wideString = ToWinWide(filename); + + if (wideString.empty()) { + return std::string(); + } + CharUpperBuffW(&wideString[0], wideString.length()); return FromWinWide(wideString); #else diff --git a/src/api/metadata/condition_evaluator.cpp b/src/api/metadata/condition_evaluator.cpp index 41be72c1..776b83a4 100644 --- a/src/api/metadata/condition_evaluator.cpp +++ b/src/api/metadata/condition_evaluator.cpp @@ -186,28 +186,34 @@ void ConditionEvaluator::ClearConditionCache() { HandleError("clear the condition cache", result); } -void ConditionEvaluator::RefreshState(std::shared_ptr loadOrderHandler) { +void ConditionEvaluator::RefreshActivePluginsState( + std::vector activePluginNames) { ClearConditionCache(); - std::vector activePluginNameStrings = loadOrderHandler->GetActivePlugins(); - std::vector activePluginNames; - for (auto& pluginName : activePluginNameStrings) { - activePluginNames.push_back(pluginName.c_str()); + std::vector activePluginNameCStrings; + for (auto& pluginName : activePluginNames) { + activePluginNameCStrings.push_back(pluginName.c_str()); } - int result = lci_state_set_active_plugins(lciState_.get(), - &activePluginNames[0], - activePluginNames.size()); + const char* const* cActivePluginNames; + if (activePluginNameCStrings.empty()) { + cActivePluginNames = {}; + } else { + cActivePluginNames = &activePluginNameCStrings[0]; + } + + int result = lci_state_set_active_plugins(lciState_.get(), cActivePluginNames, activePluginNameCStrings.size()); HandleError("cache active plugins for condition evaluation", result); } -void ConditionEvaluator::RefreshState(std::shared_ptr gameCache) { +void ConditionEvaluator::RefreshLoadedPluginsState( + std::vector> plugins) { ClearConditionCache(); std::vector pluginNames; std::vector pluginVersionStrings; std::vector crcs; - for (auto plugin : gameCache->GetPlugins()) { + for (auto plugin : plugins) { pluginNames.push_back(plugin->GetName()); pluginVersionStrings.push_back(plugin->GetVersion().value_or("")); crcs.push_back(plugin->GetCRC().value_or(0)); @@ -231,13 +237,25 @@ void ConditionEvaluator::RefreshState(std::shared_ptr gameCache) { } } - int result = lci_state_set_plugin_versions(lciState_.get(), - &pluginVersions[0], + const plugin_version* cPluginVersions; + if (pluginVersions.empty()) { + cPluginVersions = {}; + } else { + cPluginVersions = &pluginVersions[0]; + } + + int result = lci_state_set_plugin_versions(lciState_.get(), cPluginVersions, pluginVersions.size()); HandleError("cache plugin versions for condition evaluation", result); - result = lci_state_set_crc_cache(lciState_.get(), - &pluginCrcs[0], + const plugin_crc* cPluginCrcs; + if (pluginCrcs.empty()) { + cPluginCrcs = {}; + } else { + cPluginCrcs = &pluginCrcs[0]; + } + + result = lci_state_set_crc_cache(lciState_.get(), cPluginCrcs, pluginCrcs.size()); HandleError("fill CRC cache for condition evaluation", result); } diff --git a/src/api/metadata/condition_evaluator.h b/src/api/metadata/condition_evaluator.h index 85117ffb..41ac97eb 100644 --- a/src/api/metadata/condition_evaluator.h +++ b/src/api/metadata/condition_evaluator.h @@ -45,8 +45,9 @@ public: PluginMetadata EvaluateAll(const PluginMetadata& pluginMetadata); void ClearConditionCache(); - void RefreshState(std::shared_ptr loadOrderHandler); - void RefreshState(std::shared_ptr gameCache); + void RefreshActivePluginsState(std::vector activePluginNames); + void RefreshLoadedPluginsState(std::vector> plugins); + private: bool Evaluate(const PluginCleaningData& cleaningData, const std::string& pluginName); diff --git a/src/api/metadata/yaml/tag.h b/src/api/metadata/yaml/tag.h index 0d01ac69..dfeb7dbf 100644 --- a/src/api/metadata/yaml/tag.h +++ b/src/api/metadata/yaml/tag.h @@ -64,7 +64,7 @@ struct convert { } else tag = node.as(); - if (tag[0] == '-') + if (!tag.empty() && tag[0] == '-') rhs = loot::Tag(tag.substr(1), false, condition); else rhs = loot::Tag(tag, true, condition); diff --git a/src/tests/api/internals/helpers/text_test.h b/src/tests/api/internals/helpers/text_test.h index 11f1d752..06179a5b 100644 --- a/src/tests/api/internals/helpers/text_test.h +++ b/src/tests/api/internals/helpers/text_test.h @@ -288,6 +288,11 @@ TEST(NormalizeFilename, shouldUppercaseStringsAndBeLocaleInvariant) { // Reset locale. std::locale::global(std::locale::classic()); } + +TEST(NormalizeFilename, shouldReturnAnEmptyStringIfGivenAnEmptyString) { + EXPECT_EQ("", NormalizeFilename(std::string())); + EXPECT_EQ("", NormalizeFilename("")); +} #else TEST(NormalizeFilename, shouldCaseFoldStringsAndBeLocaleInvariant) { // ICU folds all greek rhos to the lowercase rho, unlike Windows. The result diff --git a/src/tests/api/internals/metadata/condition_evaluator_test.h b/src/tests/api/internals/metadata/condition_evaluator_test.h index b99325cc..78dcd8db 100644 --- a/src/tests/api/internals/metadata/condition_evaluator_test.h +++ b/src/tests/api/internals/metadata/condition_evaluator_test.h @@ -54,8 +54,9 @@ protected: out.close(); loadInstalledPlugins(); - evaluator_.RefreshState(game_.GetCache()); - evaluator_.RefreshState(game_.GetLoadOrderHandler()); + evaluator_.RefreshLoadedPluginsState(game_.GetLoadedPlugins()); + evaluator_.RefreshActivePluginsState( + game_.GetLoadOrderHandler()->GetActivePlugins()); } std::string IntToHexString(const uint32_t value) { @@ -206,6 +207,54 @@ TEST_P(ConditionEvaluatorTest, evaluateAllShouldPreserveGroupExplicitness) { EXPECT_NO_THROW(plugin = evaluator_.EvaluateAll(plugin)); EXPECT_FALSE(plugin.GetGroup()); } + +TEST_P(ConditionEvaluatorTest, + refreshActivePluginsStateShouldClearTheConditionCache) { + std::string condition("active(\"" + blankEsm + "\")"); + ASSERT_TRUE(evaluator_.Evaluate(condition)); + + evaluator_.RefreshActivePluginsState({blankEsp}); + + EXPECT_FALSE(evaluator_.Evaluate(condition)); +} + +TEST_P(ConditionEvaluatorTest, + refreshActivePluginsStateShouldClearTheActivePluginsCacheIfGivenAnEmptyVector) { + std::string condition("active(\"" + blankEsm + "\")"); + ASSERT_TRUE(evaluator_.Evaluate(condition)); + + evaluator_.RefreshActivePluginsState({}); + + EXPECT_FALSE(evaluator_.Evaluate(condition)); +} + +TEST_P(ConditionEvaluatorTest, + refreshLoadedPluginsStateShouldClearTheConditionCache) { + std::string condition("version(\"" + blankEsm + "\", \"5.0\", ==)"); + + ASSERT_TRUE(evaluator_.Evaluate(condition)); + + auto plugins = game_.GetLoadedPlugins(); + auto pluginsIt = + std::find_if(plugins.cbegin(), plugins.cend(), [&](auto plugin) { + return plugin->GetName() == blankEsm; + }); + plugins.erase(pluginsIt); + evaluator_.RefreshLoadedPluginsState(plugins); + + EXPECT_FALSE(evaluator_.Evaluate(condition)); +} + +TEST_P(ConditionEvaluatorTest, + refreshLoadedPluginsStateShouldClearTheVersionsCacheIfGivenAnEmptyVector) { + std::string condition("version(\"" + blankEsm + "\", \"5.0\", ==)"); + + ASSERT_TRUE(evaluator_.Evaluate(condition)); + + evaluator_.RefreshLoadedPluginsState({}); + + EXPECT_FALSE(evaluator_.Evaluate(condition)); +} } }