cpp/openttd-patchpack/source Changeset - r28506:4fd07c3f848d

Changeset - r28506:4fd07c3f848d

Parent rev.

Child rev.

[Not reviewed]

master

0 4 0

Loïc Guilloux - 11 months ago 2024-01-18 17:06:30
glx22@users.noreply.github.com

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

4 files changed with 76 insertions and 49 deletions:

src/game/game_text.cpp

src/game/game_text.hpp

src/script/api/script_text.cpp

src/script/api/script_text.hpp

0 comments (0 inline, 0 general)

src/game/game_text.cpp

➞

Show inline comments

@@ @@ -268,9 +268,15 @@ static void ExtractStringParams(const St @@
 			StringParams &param = 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);
+			}
+		}
+	}

src/game/game_text.hpp

➞

Show inline comments

@@ @@ -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<StringParam>;
 using StringParamsList = std::vector<StringParams>;

src/script/api/script_text.cpp

➞

Show inline comments

@@ @@ -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<std::string> &output, int &param_count, StringIDList &seen_ids)
 void ScriptText::_FillParamList(ParamList &params)
+{
 	for (int i = 0; i < this->paramc; i++) {
 		Param *p = &this->param[i];
 		params.emplace_back(this->string, i, p);
 		if (!std::holds_alternative<ScriptTextRef>(*p)) continue;
 		std::get<ScriptTextRef>(*p)->_FillParamList(params);
+	}
+}
 void ScriptText::_GetEncodedText(std::back_insert_iterator<std::string> &output, int &param_count, StringIDList &seen_ids, ParamSpan args)
+{
 	const std::string &name = GetGameStringName(this->string);
@@ @@ -181,62 +192,50 @@ void ScriptText::_GetEncodedText(std::ba @@
 	fmt::format_to(output, "{:X}", this->string);
 	const StringParams &params = 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));
 		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<SQInteger>(this->param[cur_idx])) throw Script_FatalError(fmt::format("{}: Parameter {} expects an integer", name, param_count + i));
 					fmt::format_to(output, ":{:X}", std::get<SQInteger>(this->param[cur_idx++]));
+				}
+			}
 			if (prev_idx == prev_count) {
 				/* Re-enable validation. */
 				prev_string = -1;
+			}
 		} else {
 			switch (cur_param.type) {
 				case StringParam::RAW_STRING:
 					if (!std::holds_alternative<std::string>(this->param[cur_idx])) throw Script_FatalError(fmt::format("{}: Parameter {} expects a raw string", name, param_count));
 					fmt::format_to(output, ":\"{}\"", std::get<std::string>(this->param[cur_idx++]));
 			case StringParam::UNUSED:
 				skip_args(cur_param.consumes);
 					break;
 			case StringParam::RAW_STRING: {
 				Param *p = get_next_arg();
 				if (!std::holds_alternative<std::string>(*p)) throw Script_FatalError(fmt::format("{}({}): {{{}}} expects a raw string", name, param_count + 1, cur_param.cmd));
 				fmt::format_to(output, ":\"{}\"", std::get<std::string>(*p));
 				break;
+			}
 				case StringParam::STRING: {
 					if (!std::holds_alternative<ScriptTextRef>(this->param[cur_idx])) throw Script_FatalError(fmt::format("{}: Parameter {} expects a substring", name, param_count));
 				Param *p = get_next_arg();
 				if (!std::holds_alternative<ScriptTextRef>(*p)) throw Script_FatalError(fmt::format("{}({}): {{{}}} expects a GSText", name, param_count + 1, cur_param.cmd));
 					int count = 0;
 					fmt::format_to(output, ":");
 					std::get<ScriptTextRef>(this->param[cur_idx++])->_GetEncodedText(output, count, seen_ids);
 				ScriptTextRef &ref = std::get<ScriptTextRef>(*p);
 				ref->_GetEncodedText(output, count, seen_ids, args.subspan(idx));
 					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));
+					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");
 						/* 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;
+						}
+					}
+				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<SQInteger>(this->param[cur_idx])) throw Script_FatalError(fmt::format("{}: Parameter {} expects an integer", name, param_count + i));
 						fmt::format_to(output, ":{:X}", std::get<SQInteger>(this->param[cur_idx++]));
+					}
 					Param *p = get_next_arg();
 					if (!std::holds_alternative<SQInteger>(*p)) throw Script_FatalError(fmt::format("{}({}): {{{}}} expects an integer", name, param_count + i + 1, cur_param.cmd));
 					fmt::format_to(output, ":{:X}", std::get<SQInteger>(*p));
+			}
+		}

src/script/api/script_text.hpp

➞

Show inline comments

@@ @@ -130,19 +130,39 @@ public: @@
 private:
 	using ScriptTextRef = ScriptObjectRef<ScriptText>;
 	using StringIDList = std::vector<StringID>;
 	using Param = std::variant<SQInteger, std::string, ScriptTextRef>;
 	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<ParamCheck>;
 	using ParamSpan = std::span<ParamCheck>;
 	StringID string;
-	std::variant<SQInteger, std::string, ScriptTextRef> 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 &params);
 	/**
 	 * 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<std::string> &output, int &param_count, StringIDList &seen_ids);
+	void _GetEncodedText(std::back_insert_iterator<std::string> &output, int &param_count, StringIDList &seen_ids, ParamSpan args);
 	/**
 	 * Set a parameter, where the value is the first item on the stack.

0 comments (0 inline, 0 general)