# HG changeset patch # User Loïc Guilloux # Date 2024-01-18 17:06:30 # Node ID 4fd07c3f848d0d7cf227b7ab4d40e771a6ff813a # Parent 5212736360c6e875dd98c27dc2621f86732511d9 Fix: [Script] Improve ScriptText validation (#11721) The validation is now done in two steps: - First we get the list of parameters in the same order they used to be in encoded string - Then we validate the parameter types like FormatString would use them while encoding the string diff --git a/src/game/game_text.cpp b/src/game/game_text.cpp --- a/src/game/game_text.cpp +++ b/src/game/game_text.cpp @@ -268,9 +268,15 @@ static void ExtractStringParams(const St StringParams ¶m = params.emplace_back(); ParsedCommandStruct pcs = ExtractCommandString(ls->english.c_str(), false); - for (const CmdStruct *cs : pcs.consuming_commands) { - if (cs == nullptr) break; - param.emplace_back(GetParamType(cs), cs->consumes); + for (auto it = pcs.consuming_commands.begin(); it != pcs.consuming_commands.end(); it++) { + if (*it == nullptr) { + /* Skip empty param unless a non empty param exist after it. */ + if (std::all_of(it, pcs.consuming_commands.end(), [](auto cs) { return cs == nullptr; })) break; + param.emplace_back(StringParam::UNUSED, 1, nullptr); + continue; + } + const CmdStruct *cs = *it; + param.emplace_back(GetParamType(cs), cs->consumes, cs->cmd); } } } diff --git a/src/game/game_text.hpp b/src/game/game_text.hpp --- a/src/game/game_text.hpp +++ b/src/game/game_text.hpp @@ -12,6 +12,7 @@ struct StringParam { enum ParamType { + UNUSED, RAW_STRING, STRING, OTHER @@ -19,8 +20,9 @@ struct StringParam { ParamType type; uint8_t consumes; + const char *cmd; - StringParam(ParamType type, uint8_t consumes) : type(type), consumes(consumes) {} + StringParam(ParamType type, uint8_t consumes, const char *cmd) : type(type), consumes(consumes), cmd(cmd) {} }; using StringParams = std::vector; using StringParamsList = std::vector; diff --git a/src/script/api/script_text.cpp b/src/script/api/script_text.cpp --- a/src/script/api/script_text.cpp +++ b/src/script/api/script_text.cpp @@ -160,17 +160,28 @@ SQInteger ScriptText::_set(HSQUIRRELVM v std::string ScriptText::GetEncodedText() { - static StringIDList seen_ids; + StringIDList seen_ids; + ParamList params; int param_count = 0; - seen_ids.clear(); std::string result; auto output = std::back_inserter(result); - this->_GetEncodedText(output, param_count, seen_ids); + this->_FillParamList(params); + this->_GetEncodedText(output, param_count, seen_ids, params); if (param_count > SCRIPT_TEXT_MAX_PARAMETERS) throw Script_FatalError(fmt::format("{}: Too many parameters", GetGameStringName(this->string))); return result; } -void ScriptText::_GetEncodedText(std::back_insert_iterator &output, int ¶m_count, StringIDList &seen_ids) +void ScriptText::_FillParamList(ParamList ¶ms) +{ + for (int i = 0; i < this->paramc; i++) { + Param *p = &this->param[i]; + params.emplace_back(this->string, i, p); + if (!std::holds_alternative(*p)) continue; + std::get(*p)->_FillParamList(params); + } +} + +void ScriptText::_GetEncodedText(std::back_insert_iterator &output, int ¶m_count, StringIDList &seen_ids, ParamSpan args) { const std::string &name = GetGameStringName(this->string); @@ -181,63 +192,51 @@ void ScriptText::_GetEncodedText(std::ba fmt::format_to(output, "{:X}", this->string); const StringParams ¶ms = GetGameStringParams(this->string); - int cur_idx = 0; - int prev_string = -1; - int prev_idx = -1; - int prev_count = -1; + + size_t idx = 0; + auto get_next_arg = [&]() { + if (idx >= args.size()) throw Script_FatalError(fmt::format("{}({}): Not enough parameters", name, param_count + 1)); + ParamCheck &pc = args[idx++]; + if (pc.owner != this->string) ScriptLog::Warning(fmt::format("{}({}): Consumes {}({})", name, param_count + 1, GetGameStringName(pc.owner), pc.idx + 1)); + return pc.param; + }; + auto skip_args = [&](size_t nb) { idx += nb; }; for (const StringParam &cur_param : params) { - if (cur_idx >= this->paramc) throw Script_FatalError(fmt::format("{}: Not enough parameters", name)); + switch (cur_param.type) { + case StringParam::UNUSED: + skip_args(cur_param.consumes); + break; - if (prev_string != -1) { - /* The previous substring added more parameters than expected, means we will consume them but can't properly validate them. */ - for (int i = 0; i < cur_param.consumes; i++) { - if (prev_idx < prev_count) { - ScriptLog::Warning(fmt::format("{}: Parameter {} uses parameter {} from substring {} and cannot be validated", name, param_count + i, prev_idx++, prev_string)); - } else { - /* No more extra parameters, assume SQInteger are expected. */ - if (cur_idx >= this->paramc) throw Script_FatalError(fmt::format("{}: Not enough parameters", name)); - if (!std::holds_alternative(this->param[cur_idx])) throw Script_FatalError(fmt::format("{}: Parameter {} expects an integer", name, param_count + i)); - fmt::format_to(output, ":{:X}", std::get(this->param[cur_idx++])); - } + case StringParam::RAW_STRING: { + Param *p = get_next_arg(); + if (!std::holds_alternative(*p)) throw Script_FatalError(fmt::format("{}({}): {{{}}} expects a raw string", name, param_count + 1, cur_param.cmd)); + fmt::format_to(output, ":\"{}\"", std::get(*p)); + break; } - if (prev_idx == prev_count) { - /* Re-enable validation. */ - prev_string = -1; - } - } else { - switch (cur_param.type) { - case StringParam::RAW_STRING: - if (!std::holds_alternative(this->param[cur_idx])) throw Script_FatalError(fmt::format("{}: Parameter {} expects a raw string", name, param_count)); - fmt::format_to(output, ":\"{}\"", std::get(this->param[cur_idx++])); - break; - case StringParam::STRING: { - if (!std::holds_alternative(this->param[cur_idx])) throw Script_FatalError(fmt::format("{}: Parameter {} expects a substring", name, param_count)); - int count = 0; - fmt::format_to(output, ":"); - std::get(this->param[cur_idx++])->_GetEncodedText(output, count, seen_ids); - if (++count != cur_param.consumes) { - ScriptLog::Error(fmt::format("{}: Parameter {} substring consumes {}, but expected {} to be consumed", name, param_count, count - 1, cur_param.consumes - 1)); - /* Fill missing params if needed. */ - for (int i = count; i < cur_param.consumes; i++) fmt::format_to(output, ":0"); - /* Disable validation for the extra params if any. */ - if (count > cur_param.consumes) { - prev_string = param_count; - prev_idx = cur_param.consumes - 1; - prev_count = count - 1; - } - } - break; + case StringParam::STRING: { + Param *p = get_next_arg(); + if (!std::holds_alternative(*p)) throw Script_FatalError(fmt::format("{}({}): {{{}}} expects a GSText", name, param_count + 1, cur_param.cmd)); + int count = 0; + fmt::format_to(output, ":"); + ScriptTextRef &ref = std::get(*p); + ref->_GetEncodedText(output, count, seen_ids, args.subspan(idx)); + if (++count != cur_param.consumes) { + ScriptLog::Error(fmt::format("{}({}): {{{}}} expects {} to be consumed, but {} consumes {}", name, param_count + 1, cur_param.cmd, cur_param.consumes - 1, GetGameStringName(ref->string), count - 1)); + /* Fill missing params if needed. */ + for (int i = count; i < cur_param.consumes; i++) fmt::format_to(output, ":0"); } + skip_args(cur_param.consumes - 1); + break; + } - default: - if (cur_idx + cur_param.consumes > this->paramc) throw Script_FatalError(fmt::format("{}: Not enough parameters", name)); - for (int i = 0; i < cur_param.consumes; i++) { - if (!std::holds_alternative(this->param[cur_idx])) throw Script_FatalError(fmt::format("{}: Parameter {} expects an integer", name, param_count + i)); - fmt::format_to(output, ":{:X}", std::get(this->param[cur_idx++])); - } - } + default: + for (int i = 0; i < cur_param.consumes; i++) { + Param *p = get_next_arg(); + if (!std::holds_alternative(*p)) throw Script_FatalError(fmt::format("{}({}): {{{}}} expects an integer", name, param_count + i + 1, cur_param.cmd)); + fmt::format_to(output, ":{:X}", std::get(*p)); + } } param_count += cur_param.consumes; diff --git a/src/script/api/script_text.hpp b/src/script/api/script_text.hpp --- a/src/script/api/script_text.hpp +++ b/src/script/api/script_text.hpp @@ -130,19 +130,39 @@ public: private: using ScriptTextRef = ScriptObjectRef; using StringIDList = std::vector; + using Param = std::variant; + + struct ParamCheck { + StringID owner; + int idx; + Param *param; + + ParamCheck(StringID owner, int idx, Param *param) : owner(owner), idx(idx), param(param) {} + }; + + using ParamList = std::vector; + using ParamSpan = std::span; StringID string; - std::variant param[SCRIPT_TEXT_MAX_PARAMETERS]; + Param param[SCRIPT_TEXT_MAX_PARAMETERS]; int paramc; /** + * Internal function to recursively fill a list of parameters. + * The parameters are added as _GetEncodedText used to encode them + * before the addition of parameter validation. + * @param params The list of parameters to fill. + */ + void _FillParamList(ParamList ¶ms); + + /** * Internal function for recursive calling this function over multiple * instances, while writing in the same buffer. * @param output The output to write the encoded text to. * @param param_count The number of parameters that are in the string. * @param seen_ids The list of seen StringID. */ - void _GetEncodedText(std::back_insert_iterator &output, int ¶m_count, StringIDList &seen_ids); + void _GetEncodedText(std::back_insert_iterator &output, int ¶m_count, StringIDList &seen_ids, ParamSpan args); /** * Set a parameter, where the value is the first item on the stack.