Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #10059: Clamp config item values to int, disallow negative random deviation, and allow values up to 12 digits including '-' #10444

Merged
merged 5 commits into from Feb 20, 2023

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Feb 3, 2023

Motivation / Problem

We can't input custom negative values via query string on a config item and it can't be more than 10 digits.
Crash report is caused by implicit 64 bit to 32 bit conversions during construction of a script's settings.
We can't create labels for settings with a negative value.

Description

  • Clamp config item values to int32. They're already correctly read as int64 (or SQInteger).
  • Disallow negative random_deviation. It would cause weirdness during AddDeviation, because it expects uint. If it is negative, the abs of it is set instead.
  • Allow custom values via query string to be negative and up to 12 digits, to allow for a large enough number, like -2 147 483 648.
  • Also allow saving to the savegame and openttd.cfg numbers these large.
  • AddLabels now checks the second character of key_string identifiers. If it finds a '_', then the integer that comes after becomes negative, allowing adding labels to negative values of a setting.

Limitations

I suppose none.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@2TallTyler 2TallTyler added the needs review: Script API Review requested from GS/AI expert label Feb 3, 2023
Copy link
Contributor

@glx22 glx22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation should mention the clamping and the allowed range.

@SamuXarick SamuXarick force-pushed the clamp-config-item-values-to-int branch 2 times, most recently from 4f3b6f2 to e96f2e7 Compare February 4, 2023 14:40
src/script/script_info.cpp Outdated Show resolved Hide resolved
src/script/script_info.cpp Outdated Show resolved Hide resolved
@@ -218,17 +218,23 @@ class ScriptInfo {
* store the current configuration of Scripts. Required.
* - description A single line describing the setting. Required.
* - min_value The minimum value of this setting. Required for integer
* settings and not allowed for boolean settings.
* settings and not allowed for boolean settings. The value will be
* clamped in the range [-2147483648, 2147483647] (inclusive).
Copy link
Contributor

@glx22 glx22 Feb 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't check all documentation, but some places use MIN(int32) .. MAX(int32) style.
Not sure which one is better, but consistency is good.

src/script/api/script_info_docs.hpp Show resolved Hide resolved
@SamuXarick SamuXarick force-pushed the clamp-config-item-values-to-int branch 3 times, most recently from 3101b36 to 026c754 Compare February 8, 2023 17:08
@@ -216,7 +216,7 @@ std::string ScriptConfig::SettingsToString() const
char *s = string;
*s = '\0';
for (const auto &item : this->settings) {
char no[10];
char no[12];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all the commit message is not correct compared to this code. The 12 in here contains a trailing \0, so at most 11 digits could be placed.
Secondly, the whole function is trying hard to stay within the artificial 1024 byte limit, but then returns a theoreticall unbounded string, so why not use proper C++ constructs to create this settings string?
Thirdly, the ini file reading seems to use a 1024 byte buffer as well, so how can the written value ever be correctly read, because I think there is some key in front of the configuration.

So all-in-all it's more broken than this commit fixes, though I can agree that the second and third issue are not for this PR. The commit message definitely is broken, and the current number is magic-y without any comments, so what about something like:
char no[10 + 1 + 1]; // Maximum of 10 digits for MIN/MAX_INT32, 1 for the sign and 1 for '\0'.
And if the "12" gets used in many more places, a constant would be better as it can be reused, maybe something like INT32_DIGITS_WITH_SIGN_AND_TERMINATION.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I placed INT32_DIGITS_WITH_SIGN_AND_TERMINATION in the script_config.hpp with comment, added includes in script_gui.cpp and game_gui.cpp to read this value. Also edited 2 commit messages.

@SamuXarick SamuXarick force-pushed the clamp-config-item-values-to-int branch from 026c754 to eaf65c4 Compare February 18, 2023 11:43
@glx22 glx22 added the backport requested This PR should be backport to current release (RC / stable) label Feb 20, 2023
@glx22 glx22 merged commit 8351b97 into OpenTTD:master Feb 20, 2023
@SamuXarick SamuXarick deleted the clamp-config-item-values-to-int branch February 23, 2023 15:39
@rubidium42 rubidium42 added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR is backported to a current release (RC / stable) needs review: Script API Review requested from GS/AI expert
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants