From ef7b8b8da0b84fddbe5be40e02b4a0f374adc969 Mon Sep 17 00:00:00 2001 From: Gymnasiast Date: Mon, 14 May 2018 21:39:38 +0200 Subject: [PATCH 1/5] Return MONEY32_UNDEFINED on unsalvagable string_to_money() input --- src/openrct2/localisation/Localisation.cpp | 61 +++++++++++++++------- 1 file changed, 42 insertions(+), 19 deletions(-) diff --git a/src/openrct2/localisation/Localisation.cpp b/src/openrct2/localisation/Localisation.cpp index e5177bccfb..5132eb7b2a 100644 --- a/src/openrct2/localisation/Localisation.cpp +++ b/src/openrct2/localisation/Localisation.cpp @@ -1232,38 +1232,61 @@ money32 string_to_money(char * string_to_monetise) { const char* decimal_char = language_get_string(STR_LOCALE_DECIMAL_POINT); char * text_ptr = string_to_monetise; - int i, j, sign; + uint32 numNumbers = 0; + bool hasMinus = false; + bool hasDecSep = false; + // Remove everything except numbers decimal, and minus sign(s) - for (i = 0; text_ptr[i] != '\0'; ++i) { + for (uint32 i = 0, j; text_ptr[i] != '\0'; ++i) + { + if (text_ptr[i] >= '0' && text_ptr[i] <= '9') + { + numNumbers++; + } + else if (text_ptr[i] == decimal_char[0]) + { + if (hasDecSep) + return MONEY32_UNDEFINED; + else + hasDecSep = true; + } + else if (text_ptr[i] == '-') + { + if (hasMinus) + return MONEY32_UNDEFINED; + else + hasMinus = true; + } + while (!( (text_ptr[i] >= '0' && text_ptr[i] <= '9') || (text_ptr[i] == decimal_char[0]) || (text_ptr[i] == '-') || - (text_ptr[i] == '\0') - )) { + (text_ptr[i] == '\0'))) + { // Move everything over to the left by one - for (j = i; text_ptr[j] != '\0'; ++j) { + for (j = i; text_ptr[j] != '\0'; ++j) + { text_ptr[j] = text_ptr[j + 1]; } text_ptr[j] = '\0'; } } - // If first character of shortened string is a minus, consider number negative - if (text_ptr[0] == '-') { - sign = -1; - } - else { - sign = 1; - } + if (numNumbers == 0) + return MONEY32_UNDEFINED; - // Now minus signs can be removed from string - for (i = 0; text_ptr[i] != '\0'; ++i) { - if (text_ptr[i] == '-') { - for (j = i; text_ptr[j] != '\0'; ++j) { - text_ptr[j] = text_ptr[j + 1]; - } - text_ptr[j] = '\0'; + sint32 sign = 1; + if (hasMinus) + { + // If there is a minus sign, it has to be at position 0 in order to be valid. + if (text_ptr[0] == '-') + { + sign = -1; + } + else + { + return MONEY32_UNDEFINED; } } From b9186ce83d0a55d04e5bc71a22e544238fc7d909 Mon Sep 17 00:00:00 2001 From: Michael Steenbeek Date: Thu, 24 May 2018 13:44:09 +0200 Subject: [PATCH 2/5] Make argument const --- src/openrct2/localisation/Localisation.cpp | 14 ++++++++------ src/openrct2/localisation/Localisation.h | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/openrct2/localisation/Localisation.cpp b/src/openrct2/localisation/Localisation.cpp index 5132eb7b2a..8fd9e1ef1a 100644 --- a/src/openrct2/localisation/Localisation.cpp +++ b/src/openrct2/localisation/Localisation.cpp @@ -1228,10 +1228,12 @@ void format_string_to_upper(utf8 *dest, size_t size, rct_string_id format, void dest[upperString.size()] = '\0'; } -money32 string_to_money(char * string_to_monetise) +money32 string_to_money(const char* string_to_monetise) { + char processedString[128]; + safe_strcpy(processedString, string_to_monetise, 128); const char* decimal_char = language_get_string(STR_LOCALE_DECIMAL_POINT); - char * text_ptr = string_to_monetise; + char* text_ptr = processedString; uint32 numNumbers = 0; bool hasMinus = false; bool hasDecSep = false; @@ -1292,13 +1294,13 @@ money32 string_to_money(char * string_to_monetise) // Due to the nature of strstr and strtok, decimals at the very beginning will be ignored, causing // ".1" to be interpreted as "1". To prevent this, prefix with "0" if decimal is at the beginning. - char * buffer = (char *)malloc(strlen(string_to_monetise) + 4); - if (string_to_monetise[0] == decimal_char[0]) { + char * buffer = (char *)malloc(strlen(processedString) + 4); + if (processedString[0] == decimal_char[0]) { strcpy(buffer, "0"); - strcpy(buffer + 1, string_to_monetise); + strcpy(buffer + 1, processedString); } else { - strcpy(buffer, string_to_monetise); + strcpy(buffer, processedString); } int number = 0, decimal = 0; diff --git a/src/openrct2/localisation/Localisation.h b/src/openrct2/localisation/Localisation.h index b3208a83c3..c3b9d93d14 100644 --- a/src/openrct2/localisation/Localisation.h +++ b/src/openrct2/localisation/Localisation.h @@ -42,7 +42,7 @@ sint32 get_string_length(const utf8 *text); // The maximum number of characters allowed for string/money conversions (anything above will risk integer overflow issues) #define MONEY_STRING_MAXLENGTH 14 -money32 string_to_money(char * string_to_monetise); +money32 string_to_money(const char* string_to_monetise); void money_to_string(money32 amount, char * buffer_to_put_value_to, size_t buffer_len); void user_string_clear_all(); From 847d4e8b032be70c0a0ddc34a3c6f524ba9326fc Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Thu, 24 May 2018 15:03:15 +0200 Subject: [PATCH 3/5] =?UTF-8?q?Process=20string=5Fto=5Fmoney's=20worst=20c?= =?UTF-8?q?ase=20in=20O(n)=20rather=20than=20O(n=C2=B2).?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/openrct2/localisation/Localisation.cpp | 46 ++++++++++------------ 1 file changed, 21 insertions(+), 25 deletions(-) diff --git a/src/openrct2/localisation/Localisation.cpp b/src/openrct2/localisation/Localisation.cpp index 8fd9e1ef1a..aee9815ca5 100644 --- a/src/openrct2/localisation/Localisation.cpp +++ b/src/openrct2/localisation/Localisation.cpp @@ -1230,51 +1230,51 @@ void format_string_to_upper(utf8 *dest, size_t size, rct_string_id format, void money32 string_to_money(const char* string_to_monetise) { - char processedString[128]; - safe_strcpy(processedString, string_to_monetise, 128); const char* decimal_char = language_get_string(STR_LOCALE_DECIMAL_POINT); - char* text_ptr = processedString; + char processedString[128]; + uint32 numNumbers = 0; bool hasMinus = false; bool hasDecSep = false; + const char* src_ptr = string_to_monetise; + char* dst_ptr = processedString; - // Remove everything except numbers decimal, and minus sign(s) - for (uint32 i = 0, j; text_ptr[i] != '\0'; ++i) + // Process the string, keeping only numbers decimal, and minus sign(s). + while (*src_ptr != '\0') { - if (text_ptr[i] >= '0' && text_ptr[i] <= '9') + if (*src_ptr >= '0' && *src_ptr <= '9') { numNumbers++; } - else if (text_ptr[i] == decimal_char[0]) + else if (*src_ptr == decimal_char[0]) { if (hasDecSep) return MONEY32_UNDEFINED; else hasDecSep = true; } - else if (text_ptr[i] == '-') + else if (*src_ptr == '-') { if (hasMinus) return MONEY32_UNDEFINED; else hasMinus = true; } - - while (!( - (text_ptr[i] >= '0' && text_ptr[i] <= '9') || - (text_ptr[i] == decimal_char[0]) || - (text_ptr[i] == '-') || - (text_ptr[i] == '\0'))) + else { - // Move everything over to the left by one - for (j = i; text_ptr[j] != '\0'; ++j) - { - text_ptr[j] = text_ptr[j + 1]; - } - text_ptr[j] = '\0'; + // Skip invalid characters. + src_ptr++; + continue; } + + // Copy numeric values. + *dst_ptr++ = *src_ptr; + src_ptr++; } + // Terminate destination string. + *dst_ptr = '\0'; + if (numNumbers == 0) return MONEY32_UNDEFINED; @@ -1282,14 +1282,10 @@ money32 string_to_money(const char* string_to_monetise) if (hasMinus) { // If there is a minus sign, it has to be at position 0 in order to be valid. - if (text_ptr[0] == '-') - { + if (processedString[0] == '-') sign = -1; - } else - { return MONEY32_UNDEFINED; - } } // Due to the nature of strstr and strtok, decimals at the very beginning will be ignored, causing From 1d7d8a80439504c391f706d3c67fa40976ee8e0a Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Thu, 24 May 2018 15:23:07 +0200 Subject: [PATCH 4/5] Do not reallocate to prefix decimal money with '0'. --- src/openrct2/localisation/Localisation.cpp | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/openrct2/localisation/Localisation.cpp b/src/openrct2/localisation/Localisation.cpp index aee9815ca5..7f4cbbc710 100644 --- a/src/openrct2/localisation/Localisation.cpp +++ b/src/openrct2/localisation/Localisation.cpp @@ -15,6 +15,7 @@ #pragma endregion #include +#include #include #ifdef _WIN32 @@ -30,6 +31,7 @@ #include "../common.h" #include "../config/Config.h" +#include "../core/Guard.hpp" #include "../core/Math.hpp" #include "../core/String.hpp" #include "../core/Util.hpp" @@ -1231,7 +1233,9 @@ void format_string_to_upper(utf8 *dest, size_t size, rct_string_id format, void money32 string_to_money(const char* string_to_monetise) { const char* decimal_char = language_get_string(STR_LOCALE_DECIMAL_POINT); - char processedString[128]; + char processedString[128] = {}; + + Guard::Assert(strlen(string_to_monetise) < sizeof(processedString)); uint32 numNumbers = 0; bool hasMinus = false; @@ -1290,13 +1294,11 @@ money32 string_to_money(const char* string_to_monetise) // Due to the nature of strstr and strtok, decimals at the very beginning will be ignored, causing // ".1" to be interpreted as "1". To prevent this, prefix with "0" if decimal is at the beginning. - char * buffer = (char *)malloc(strlen(processedString) + 4); - if (processedString[0] == decimal_char[0]) { - strcpy(buffer, "0"); - strcpy(buffer + 1, processedString); - } - else { - strcpy(buffer, processedString); + if (processedString[0] == decimal_char[0]) + { + for (size_t i = strlen(processedString); i >= 1; i--) + processedString[i] = processedString[i - 1]; + processedString[0] = '0'; } int number = 0, decimal = 0; @@ -1317,7 +1319,6 @@ money32 string_to_money(const char* string_to_monetise) while (decimal > 10) decimal /= 10; if (decimal < 10) decimal *= 10; } - free(buffer); money32 result = MONEY(number, decimal); // Check if MONEY resulted in overflow From 85575b612e72752576cbbd61ed5d3445dc88e8e8 Mon Sep 17 00:00:00 2001 From: Aaron van Geffen Date: Thu, 24 May 2018 15:24:04 +0200 Subject: [PATCH 5/5] Apply formatting to string_to_money. --- src/openrct2/localisation/Localisation.cpp | 25 ++++++++++++++-------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/openrct2/localisation/Localisation.cpp b/src/openrct2/localisation/Localisation.cpp index 7f4cbbc710..23d387386d 100644 --- a/src/openrct2/localisation/Localisation.cpp +++ b/src/openrct2/localisation/Localisation.cpp @@ -1302,27 +1302,34 @@ money32 string_to_money(const char* string_to_monetise) } int number = 0, decimal = 0; - if (strstr(buffer, decimal_char) == nullptr) { + if (strstr(processedString, decimal_char) == nullptr) + { // If decimal char does not exist, no tokenising is needed. - number = atoi(buffer); + number = atoi(processedString); } - else { - char *numberText = strtok(buffer, decimal_char); + else + { + char *numberText = strtok(processedString, decimal_char); char *decimalText = strtok(nullptr, decimal_char); - if (numberText != nullptr) number = atoi(numberText); - if (decimalText != nullptr) decimal = atoi(decimalText); + if (numberText != nullptr) + number = atoi(numberText); + if (decimalText != nullptr) + decimal = atoi(decimalText); // The second parameter in MONEY must be two digits in length, while the game only ever uses // the first of the two digits. // Convert invalid numbers, such as ".6", ".234", ".05", to ".60", ".20", ".00" (respectively) - while (decimal > 10) decimal /= 10; - if (decimal < 10) decimal *= 10; + while (decimal > 10) + decimal /= 10; + if (decimal < 10) + decimal *= 10; } money32 result = MONEY(number, decimal); // Check if MONEY resulted in overflow - if ((number > 0 && result < 0) || result / 10 < number) { + if ((number > 0 && result < 0) || result / 10 < number) + { result = INT_MAX; } result *= sign;