From 689f55a0eae476ba245a175fbf6816f24ca26cbd Mon Sep 17 00:00:00 2001 From: frosch Date: Sun, 20 Apr 2025 23:01:49 +0200 Subject: [PATCH] Fix #14044: Negative string parameters from GS were rendered as zero. (#14049) String parameters are always stored as uint64_t. Negative values are sign-extended to int64_t and then casted to uint64_t. The same applies to encoded strings. But ScriptText encoded them as int64_t. Co-authored-by: rubidium42 --- src/saveload/saveload.cpp | 38 ++++++++++++++++++++++++++++++++ src/saveload/saveload.h | 1 + src/script/api/script_text.cpp | 3 ++- src/tests/string_func.cpp | 40 +++++++++++++++++++++++++++++++++- 4 files changed, 80 insertions(+), 2 deletions(-) diff --git a/src/saveload/saveload.cpp b/src/saveload/saveload.cpp index d6f2536f67..51482a6153 100644 --- a/src/saveload/saveload.cpp +++ b/src/saveload/saveload.cpp @@ -981,6 +981,43 @@ void FixSCCEncoded(std::string &str, bool fix_code) str = std::move(result); } +/** + * Scan the string for SCC_ENCODED_NUMERIC with negative values, and reencode them as uint64_t. + * @param str the string to fix. + */ +void FixSCCEncodedNegative(std::string &str) +{ + if (str.empty()) return; + + StringConsumer consumer(str); + + /* Check whether this is an encoded string */ + if (!consumer.ReadUtf8If(SCC_ENCODED)) return; + + std::string result; + StringBuilder builder(result); + builder.PutUtf8(SCC_ENCODED); + while (consumer.AnyBytesLeft()) { + /* Copy until next record */ + builder.Put(consumer.ReadUntilUtf8(SCC_RECORD_SEPARATOR, StringConsumer::READ_ONE_SEPARATOR)); + + /* Check whether this is a numeric parameter */ + if (!consumer.ReadUtf8If(SCC_ENCODED_NUMERIC)) continue; + builder.PutUtf8(SCC_ENCODED_NUMERIC); + + /* First try unsigned */ + if (auto u = consumer.TryReadIntegerBase(16); u.has_value()) { + builder.PutIntegerBase(*u, 16); + } else { + /* Read as signed, store as unsigned */ + auto s = consumer.ReadIntegerBase(16); + builder.PutIntegerBase(static_cast(s), 16); + } + } + + str = std::move(result); +} + /** * Read the given amount of bytes from the buffer into the string. * @param str The string to write to. @@ -1024,6 +1061,7 @@ static void SlStdString(void *ptr, VarType conv) if ((conv & SLF_ALLOW_CONTROL) != 0) { settings.Set(StringValidationSetting::AllowControlCode); if (IsSavegameVersionBefore(SLV_ENCODED_STRING_FORMAT)) FixSCCEncoded(*str, IsSavegameVersionBefore(SLV_169)); + if (IsSavegameVersionBefore(SLV_FIX_SCC_ENCODED_NEGATIVE)) FixSCCEncodedNegative(*str); } if ((conv & SLF_ALLOW_NEWLINE) != 0) { settings.Set(StringValidationSetting::AllowNewline); diff --git a/src/saveload/saveload.h b/src/saveload/saveload.h index 246088625e..267aa081da 100644 --- a/src/saveload/saveload.h +++ b/src/saveload/saveload.h @@ -400,6 +400,7 @@ enum SaveLoadVersion : uint16_t { SLV_ENCODED_STRING_FORMAT, ///< 350 PR#13499 Encoded String format changed. SLV_PROTECT_PLACED_HOUSES, ///< 351 PR#13270 Houses individually placed by players can be protected from town/AI removal. SLV_SCRIPT_SAVE_INSTANCES, ///< 352 PR#13556 Scripts are allowed to save instances. + SLV_FIX_SCC_ENCODED_NEGATIVE, ///< 353 PR#14049 Fix encoding of negative parameters. SL_MAX_VERSION, ///< Highest possible saveload version }; diff --git a/src/script/api/script_text.cpp b/src/script/api/script_text.cpp index 89333e6982..56cb77f7ed 100644 --- a/src/script/api/script_text.cpp +++ b/src/script/api/script_text.cpp @@ -211,7 +211,8 @@ void ScriptText::ParamCheck::Encode(StringBuilder &builder, std::string_view cmd void operator()(const SQInteger &value) { this->builder.PutUtf8(SCC_ENCODED_NUMERIC); - this->builder.PutIntegerBase(value, 16); + /* Sign-extend the value, then store as unsigned */ + this->builder.PutIntegerBase(static_cast(static_cast(value)), 16); } void operator()(const ScriptTextRef &value) diff --git a/src/tests/string_func.cpp b/src/tests/string_func.cpp index fb88f77a13..a5cc4b66ed 100644 --- a/src/tests/string_func.cpp +++ b/src/tests/string_func.cpp @@ -464,6 +464,9 @@ TEST_CASE("FixSCCEncoded") /* Test conversion with one numeric parameter. */ CHECK(FixSCCEncodedWrapper("\uE00022:1", false) == Compose(SCC_ENCODED, "22", SCC_RECORD_SEPARATOR, SCC_ENCODED_NUMERIC, "1")); + /* Test conversion with signed numeric parameter. */ + CHECK(FixSCCEncodedWrapper("\uE00022:-1", false) == Compose(SCC_ENCODED, "22", SCC_RECORD_SEPARATOR, SCC_ENCODED_NUMERIC, "-1")); + /* Test conversion with two numeric parameters. */ CHECK(FixSCCEncodedWrapper("\uE0003:12:2", false) == Compose(SCC_ENCODED, "3", SCC_RECORD_SEPARATOR, SCC_ENCODED_NUMERIC, "12", SCC_RECORD_SEPARATOR, SCC_ENCODED_NUMERIC, "2")); @@ -480,7 +483,27 @@ TEST_CASE("FixSCCEncoded") CHECK(FixSCCEncodedWrapper("\uE000777:\uE0008888:\"Foo\":\"BarBaz\"", false) == Compose(SCC_ENCODED, "777", SCC_RECORD_SEPARATOR, SCC_ENCODED, "8888", SCC_RECORD_SEPARATOR, SCC_ENCODED_STRING, "Foo", SCC_RECORD_SEPARATOR, SCC_ENCODED_STRING, "BarBaz")); } -TEST_CASE("EncodedString::ReplaceParam") +extern void FixSCCEncodedNegative(std::string &str); + +/* Helper to call FixSCCEncodedNegative and return the result in a new string. */ +static std::string FixSCCEncodedNegativeWrapper(const std::string &str) +{ + std::string result = str; + FixSCCEncodedNegative(result); + return result; +} + +TEST_CASE("FixSCCEncodedNegative") +{ + auto positive = Compose(SCC_ENCODED, "777", SCC_RECORD_SEPARATOR, SCC_ENCODED_NUMERIC, "ffffffffffffffff"); + auto negative = Compose(SCC_ENCODED, "777", SCC_RECORD_SEPARATOR, SCC_ENCODED_NUMERIC, "-1"); + + CHECK(FixSCCEncodedNegativeWrapper("") == ""); + CHECK(FixSCCEncodedNegativeWrapper(positive) == positive); + CHECK(FixSCCEncodedNegativeWrapper(negative) == positive); +} + +TEST_CASE("EncodedString::ReplaceParam - positive") { /* Test that two encoded strings with different parameters are not the same. */ EncodedString string1 = GetEncodedString(STR_NULL, "Foo", 10, "Bar"); @@ -491,3 +514,18 @@ TEST_CASE("EncodedString::ReplaceParam") EncodedString string3 = string1.ReplaceParam(1, 15); CHECK(string2 == string3); } + +TEST_CASE("EncodedString::ReplaceParam - negative") +{ + EncodedString string1 = GetEncodedString(STR_NULL, "Foo", -1, "Bar"); + EncodedString string2 = GetEncodedString(STR_NULL, "Foo", -2, "Bar"); + EncodedString string3 = GetEncodedString(STR_NULL, "Foo", 0xFFFF'FFFF'FFFF'FFFF, "Bar"); + /* Test that two encoded strings with different parameters are not the same. */ + CHECK(string1 != string2); + /* Test that signed values are stored as unsigned. */ + CHECK(string1 == string3); + + /* Test that replacing parameter results in the same string. */ + EncodedString string4 = string1.ReplaceParam(1, -2); + CHECK(string2 == string4); +}