From 9e26450d734e718520846b93da75b836301db4d4 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Wed, 22 Jul 2015 17:52:07 +0100 Subject: [PATCH] Parse conditions and regex during YAML decoding. To prevent syntax errors sneaking in and later causing problems. Closes #473. --- src/backend/masterlist.cpp | 10 --- src/backend/metadata/condition_grammar.h | 26 ++++--- src/backend/metadata/file.h | 8 +++ src/backend/metadata/message.h | 9 +++ src/backend/metadata/plugin_metadata.cpp | 18 ----- src/backend/metadata/plugin_metadata.h | 12 +++- src/backend/metadata/tag.h | 8 +++ .../metadata/test_conditional_metadata.h | 4 ++ src/tests/backend/metadata/test_file.h | 11 +-- src/tests/backend/metadata/test_message.h | 9 ++- .../backend/metadata/test_plugin_metadata.h | 70 +++++++++---------- src/tests/backend/metadata/test_tag.h | 9 ++- src/validator/main.cpp | 8 --- 13 files changed, 112 insertions(+), 90 deletions(-) diff --git a/src/backend/masterlist.cpp b/src/backend/masterlist.cpp index 6c64574a..238f8d64 100644 --- a/src/backend/masterlist.cpp +++ b/src/backend/masterlist.cpp @@ -368,16 +368,6 @@ namespace loot { try { this->Load(path); - for (auto &plugin : plugins) { - plugin.ParseAllConditions(); - } - for (auto &plugin : regexPlugins) { - plugin.ParseAllConditions(); - } - for (auto &message : messages) { - message.ParseCondition(); - } - parsingFailed = false; } catch (std::exception& e) { diff --git a/src/backend/metadata/condition_grammar.h b/src/backend/metadata/condition_grammar.h index c26a659f..a15f7f2b 100644 --- a/src/backend/metadata/condition_grammar.h +++ b/src/backend/metadata/condition_grammar.h @@ -184,6 +184,13 @@ namespace loot { In C++ string literals, the backslash must be escaped once more to give "\\\\". Split the regex with another regex! */ + try { + std::regex(regex, std::regex::ECMAScript | std::regex::icase); + } + catch (std::regex_error& e) { + throw loot::error(loot::error::invalid_args, (boost::format(boost::locale::translate("Invalid regex string \"%1%\": %2%")) % regex % e.what()).str()); + } + std::regex sepReg("/|(\\\\\\\\)", std::regex::ECMAScript | std::regex::icase); std::vector components; @@ -213,23 +220,24 @@ namespace loot { try { reg = std::regex(filename, std::regex::ECMAScript | std::regex::icase); } - catch (std::exception& /*e*/) { + catch (std::regex_error& e) { BOOST_LOG_TRIVIAL(error) << "Invalid regex string:" << filename; - throw loot::error(loot::error::invalid_args, boost::locale::translate("Invalid regex string:").str() + " " + filename); + throw loot::error(loot::error::invalid_args, (boost::format(boost::locale::translate("Invalid regex string \"%1%\": %2%")) % filename % e.what()).str()); } return std::pair(parent, reg); } void CheckRegex(bool& result, const std::string& regexStr) const { - if (_parseOnly) - return; result = false; BOOST_LOG_TRIVIAL(trace) << "Checking to see if any files matching the regex \"" << regexStr << "\" exist."; std::pair pathRegex = SplitRegex(regexStr); + if (_parseOnly) + return; + //Now we have a valid parent path and a regex filename. Check that //the parent path exists and is a directory. @@ -249,14 +257,15 @@ namespace loot { } void CheckMany(bool& result, const std::string& regexStr) const { - if (_parseOnly) - return; result = false; BOOST_LOG_TRIVIAL(trace) << "Checking to see if more than one file matching the regex \"" << regexStr << "\" exist."; std::pair pathRegex = SplitRegex(regexStr); + if (_parseOnly) + return; + //Now we have a valid parent path and a regex filename. Check that //the parent path exists and is a directory. @@ -278,8 +287,6 @@ namespace loot { } void CheckSum(bool& result, const std::string& file, const uint32_t checksum) { - if (_parseOnly) - return; BOOST_LOG_TRIVIAL(trace) << "Checking the CRC of the file \"" << file << "\"."; @@ -288,6 +295,9 @@ namespace loot { throw loot::error(loot::error::invalid_args, boost::locale::translate("Invalid file path:").str() + " " + file); } + if (_parseOnly) + return; + uint32_t crc; std::unordered_map::iterator it = _game->crcCache.find(boost::locale::to_lower(file)); diff --git a/src/backend/metadata/file.h b/src/backend/metadata/file.h index e6ed2a8e..6c22d132 100644 --- a/src/backend/metadata/file.h +++ b/src/backend/metadata/file.h @@ -83,6 +83,14 @@ namespace YAML { else rhs = loot::File(node.as()); + // Test condition syntax. + try { + rhs.ParseCondition(); + } + catch (std::exception& e) { + throw RepresentationException(node.Mark(), std::string("bad conversion: invalid condition syntax: ") + e.what()); + } + return true; } }; diff --git a/src/backend/metadata/message.h b/src/backend/metadata/message.h index 4e85ed46..ef21335c 100644 --- a/src/backend/metadata/message.h +++ b/src/backend/metadata/message.h @@ -144,6 +144,15 @@ namespace YAML { condition = node["condition"].as(); rhs = loot::Message(typeNo, content, condition); + + // Test condition syntax. + try { + rhs.ParseCondition(); + } + catch (std::exception& e) { + throw RepresentationException(node.Mark(), std::string("bad conversion: invalid condition syntax: ") + e.what()); + } + return true; } }; diff --git a/src/backend/metadata/plugin_metadata.cpp b/src/backend/metadata/plugin_metadata.cpp index 281a082e..4ef39356 100644 --- a/src/backend/metadata/plugin_metadata.cpp +++ b/src/backend/metadata/plugin_metadata.cpp @@ -332,24 +332,6 @@ namespace loot { return *this; } - void PluginMetadata::ParseAllConditions() const { - for (const File& file : loadAfter) { - file.ParseCondition(); - } - for (const File& file : requirements) { - file.ParseCondition(); - } - for (const File& file : incompatibilities) { - file.ParseCondition(); - } - for (const Message& message : messages) { - message.ParseCondition(); - } - for (const Tag& tag : tags) { - tag.ParseCondition(); - } - } - bool PluginMetadata::HasNameOnly() const { return !IsPriorityExplicit() && loadAfter.empty() && requirements.empty() && incompatibilities.empty() && messages.empty() && tags.empty() && _dirtyInfo.empty() && _locations.empty(); } diff --git a/src/backend/metadata/plugin_metadata.h b/src/backend/metadata/plugin_metadata.h index 95da590c..b9f69e94 100644 --- a/src/backend/metadata/plugin_metadata.h +++ b/src/backend/metadata/plugin_metadata.h @@ -37,6 +37,7 @@ #include #include #include +#include #include @@ -89,7 +90,6 @@ namespace loot { void Locations(const std::set& locations); PluginMetadata& EvalAllConditions(Game& game, const unsigned int language); - void ParseAllConditions() const; bool HasNameOnly() const; bool IsRegexPlugin() const; bool IsPriorityExplicit() const; @@ -160,6 +160,16 @@ namespace YAML { rhs = loot::PluginMetadata(node["name"].as()); + // Test for valid regex. + if (rhs.IsRegexPlugin()) { + try { + std::regex(rhs.Name(), std::regex::ECMAScript | std::regex::icase); + } + catch (std::regex_error& e) { + throw RepresentationException(node.Mark(), std::string("bad conversion: invalid regex in 'name' key: ") + e.what()); + } + } + if (node["enabled"]) rhs.Enabled(node["enabled"].as()); diff --git a/src/backend/metadata/tag.h b/src/backend/metadata/tag.h index 6676cad4..8188d2a0 100644 --- a/src/backend/metadata/tag.h +++ b/src/backend/metadata/tag.h @@ -82,6 +82,14 @@ namespace YAML { else rhs = loot::Tag(tag, true, condition); + // Test condition syntax. + try { + rhs.ParseCondition(); + } + catch (std::exception& e) { + throw RepresentationException(node.Mark(), std::string("bad conversion: invalid condition syntax: ") + e.what()); + } + return true; } }; diff --git a/src/tests/backend/metadata/test_conditional_metadata.h b/src/tests/backend/metadata/test_conditional_metadata.h index b1afe8a8..25de042e 100644 --- a/src/tests/backend/metadata/test_conditional_metadata.h +++ b/src/tests/backend/metadata/test_conditional_metadata.h @@ -72,6 +72,10 @@ TEST_F(ConditionalMetadata, ParseCondition) { cm = loot::ConditionalMetadata("condition"); EXPECT_THROW(cm.ParseCondition(), loot::error); + // Check that invalid regex also throws. + cm = loot::ConditionalMetadata("regex(\"RagnvaldBook(Farengar(+Ragnvald)?)?\\.esp\")"); + EXPECT_THROW(cm.ParseCondition(), loot::error); + cm = loot::ConditionalMetadata("file(\"Blank.esm\")"); EXPECT_NO_THROW(cm.ParseCondition()); diff --git a/src/tests/backend/metadata/test_file.h b/src/tests/backend/metadata/test_file.h index 6977cbd7..1c81ea1e 100644 --- a/src/tests/backend/metadata/test_file.h +++ b/src/tests/backend/metadata/test_file.h @@ -144,11 +144,11 @@ TEST(File, YamlEncode) { } TEST(File, YamlDecode) { - YAML::Node node = YAML::Load("{name: name1, display: display1, condition: condition1}"); + YAML::Node node = YAML::Load("{name: name1, display: display1, condition: 'file(\"Foo.esp\")'}"); File file = node.as(); EXPECT_EQ("name1", file.Name()); EXPECT_EQ("display1", file.DisplayName()); - EXPECT_EQ("condition1", file.Condition()); + EXPECT_EQ("file(\"Foo.esp\")", file.Condition()); node = YAML::Load("name1"); file = node.as(); @@ -162,11 +162,14 @@ TEST(File, YamlDecode) { EXPECT_EQ("display1", file.DisplayName()); EXPECT_EQ("", file.Condition()); - node = YAML::Load("{name: name1, condition: condition1}"); + node = YAML::Load("{name: name1, condition: 'file(\"Foo.esp\")'}"); file = node.as(); EXPECT_EQ("name1", file.Name()); EXPECT_EQ("name1", file.DisplayName()); - EXPECT_EQ("condition1", file.Condition()); + EXPECT_EQ("file(\"Foo.esp\")", file.Condition()); + + node = YAML::Load("{name: name1, condition: invalid}"); + EXPECT_THROW(node.as(), YAML::RepresentationException); node = YAML::Load("[0, 1, 2]"); EXPECT_ANY_THROW(node.as()); diff --git a/src/tests/backend/metadata/test_message.h b/src/tests/backend/metadata/test_message.h index c9d517e6..7b483df9 100644 --- a/src/tests/backend/metadata/test_message.h +++ b/src/tests/backend/metadata/test_message.h @@ -326,11 +326,11 @@ TEST_F(Message, YamlDecode) { node = YAML::Load("type: say\n" "content: content1\n" - "condition: condition1"); + "condition: 'file(\"Foo.esp\")'"); message = node.as(); EXPECT_EQ(loot::Message::say, message.Type()); EXPECT_EQ(MessageContents({MessageContent("content1", Language::english)}), message.Content()); - EXPECT_EQ("condition1", message.Condition()); + EXPECT_EQ("file(\"Foo.esp\")", message.Condition()); node = YAML::Load("type: say\n" "content:\n" @@ -405,6 +405,11 @@ TEST_F(Message, YamlDecode) { EXPECT_EQ(MessageContents({MessageContent("con%1%tent1", Language::english)}), message.Content()); EXPECT_EQ("", message.Condition()); + node = YAML::Load("type: say\n" + "content: content1\n" + "condition: invalid"); + EXPECT_THROW(node.as(), YAML::RepresentationException); + node = YAML::Load("scalar"); EXPECT_THROW(node.as(), YAML::RepresentationException); diff --git a/src/tests/backend/metadata/test_plugin_metadata.h b/src/tests/backend/metadata/test_plugin_metadata.h index b92afa85..6a1a0ed2 100644 --- a/src/tests/backend/metadata/test_plugin_metadata.h +++ b/src/tests/backend/metadata/test_plugin_metadata.h @@ -502,7 +502,6 @@ TEST_F(PluginMetadata, EvalAllConditions) { "msg:\n" " - type: say\n" " content: 'content'\n" - " condition: 'condition'\n" "tag:\n" " - name: Relev\n" " condition: 'file(\"Blank.missing.esm\")'\n" @@ -517,8 +516,6 @@ TEST_F(PluginMetadata, EvalAllConditions) { " nav: 2" ).as()); - EXPECT_ANY_THROW(pm.EvalAllConditions(game, loot::Language::english)); - pm.Messages({loot::Message(loot::Message::say, "content")}); EXPECT_NO_THROW(pm.EvalAllConditions(game, loot::Language::english)); EXPECT_EQ(std::set({ @@ -537,33 +534,6 @@ TEST_F(PluginMetadata, EvalAllConditions) { }), pm.DirtyInfo()); } -TEST_F(PluginMetadata, ParseAllConditions) { - loot::PluginMetadata pm(YAML::Load( - "name: 'Blank.esp'\n" - "after:\n" - " - name: 'Blank.esm'\n" - " condition: 'file(\"Blank.esm\")'\n" - "req:\n" - " - name: 'Blank.esm'\n" - " condition: 'file(\"Blank.missing.esm\")'\n" - "inc:\n" - " - name: 'Blank.esm'\n" - " condition: 'file(\"Blank.esm\")'\n" - "msg:\n" - " - type: say\n" - " content: 'content'\n" - " condition: 'condition'\n" - "tag:\n" - " - name: Relev\n" - " condition: 'file(\"Blank.missing.esm\")'" - ).as()); - - EXPECT_ANY_THROW(pm.ParseAllConditions()); - - pm.Messages({loot::Message(loot::Message::say, "content")}); - EXPECT_NO_THROW(pm.ParseAllConditions()); -} - TEST_F(PluginMetadata, HasNameOnly) { loot::PluginMetadata pm; EXPECT_TRUE(pm.HasNameOnly()); @@ -823,9 +793,6 @@ TEST_F(PluginMetadata, YamlDecode) { YAML::Node node; loot::PluginMetadata pm; - node = YAML::Load("Blank.esp"); - EXPECT_ANY_THROW(node.as()); - node = YAML::Load("name: Blank.esp"); pm = node.as(); EXPECT_EQ("Blank.esp", pm.Name()); @@ -887,13 +854,44 @@ TEST_F(PluginMetadata, YamlDecode) { " util: 'utility'\n" " udr: 1\n" " nav: 2"); - EXPECT_ANY_THROW(node.as()); + EXPECT_THROW(node.as(), YAML::RepresentationException); + + // Don't allow invalid regex. + node = YAML::Load("name: 'RagnvaldBook(Farengar(+Ragnvald)?)?\\.esp'\n" + "dirty:\n" + " - crc: 0x5\n" + " util: 'utility'\n" + " udr: 1\n" + " nav: 2"); + EXPECT_THROW(node.as(), YAML::RepresentationException); + + // Catch condition syntax errors. + node = YAML::Load( + "name: 'Blank.esp'\n" + "after:\n" + " - name: 'Blank.esm'\n" + " condition: 'file(\"Blank.esm\")'\n" + "req:\n" + " - name: 'Blank.esm'\n" + " condition: 'file(\"Blank.missing.esm\")'\n" + "inc:\n" + " - name: 'Blank.esm'\n" + " condition: 'file(\"Blank.esm\")'\n" + "msg:\n" + " - type: say\n" + " content: 'content'\n" + " condition: 'condition'\n" + "tag:\n" + " - name: Relev\n" + " condition: 'file(\"Blank.missing.esm\")'" + ); + EXPECT_THROW(node.as(), YAML::RepresentationException); node = YAML::Load("scalar"); - EXPECT_ANY_THROW(node.as()); + EXPECT_THROW(node.as(), YAML::RepresentationException); node = YAML::Load("[0, 1, 2]"); - EXPECT_ANY_THROW(node.as()); + EXPECT_THROW(node.as(), YAML::RepresentationException); } #endif diff --git a/src/tests/backend/metadata/test_tag.h b/src/tests/backend/metadata/test_tag.h index 697e90fe..245371fa 100644 --- a/src/tests/backend/metadata/test_tag.h +++ b/src/tests/backend/metadata/test_tag.h @@ -164,14 +164,17 @@ TEST(Tag, YamlDecode) { EXPECT_FALSE(tag.IsAddition()); EXPECT_EQ("", tag.Condition()); - node = YAML::Load("{name: name1, condition: condition1}"); + node = YAML::Load("{name: name1, condition: 'file(\"Foo.esp\")'}"); tag = node.as(); EXPECT_EQ("name1", tag.Name()); EXPECT_TRUE(tag.IsAddition()); - EXPECT_EQ("condition1", tag.Condition()); + EXPECT_EQ("file(\"Foo.esp\")", tag.Condition()); + + node = YAML::Load("{name: name1, condition: invalid}"); + EXPECT_THROW(node.as(), YAML::RepresentationException); node = YAML::Load("[0, 1, 2]"); - EXPECT_ANY_THROW(node.as()); + EXPECT_THROW(node.as(), YAML::RepresentationException); } #endif diff --git a/src/validator/main.cpp b/src/validator/main.cpp index 6c4d5f20..492dfd22 100644 --- a/src/validator/main.cpp +++ b/src/validator/main.cpp @@ -65,14 +65,6 @@ int main(int argc, char **argv) { // Test YAML parsing. loot::MetadataList metadata; metadata.Load(argv[1]); - - // Test condition parsing. - for (auto &plugin : metadata.Plugins()) { - plugin.ParseAllConditions(); - } - for (auto &message : metadata.messages) { - message.ParseCondition(); - } } catch (std::exception& e) { std::cout << "ERROR: " << e.what() << std::endl << std::endl;