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

Negativ Integer values for GRF Parameters crashes the game #7836

Closed
spnda opened this issue Nov 16, 2019 · 4 comments
Closed

Negativ Integer values for GRF Parameters crashes the game #7836

spnda opened this issue Nov 16, 2019 · 4 comments

Comments

@spnda
Copy link
Contributor

@spnda spnda commented Nov 16, 2019

Version of OpenTTD

Unknown. Issue exists in 1.9/master and JGR. Probably exists in older versions aswell.

Error

When a GRF Parameter has negativ integer values, clicking on the dropdown crashes the game.

Assertion Error

Reproduce

Example GRF to reproduce:
grf.zip

Link to the NML issue: OpenTTD/nml#57

@planetmaker
Copy link
Contributor

@planetmaker planetmaker commented Nov 18, 2019

It shouldn't crash.
Yet, parameters are by design unsigned integer; thus a NewGRF allowing negative numbers is going beyond the specs of allowed value ranges. See https://newgrf-specs.tt-wiki.net/wiki/Action14#Allowed_value_range_.28.22INFO.22_-.3E_.22PARA.22_-.3E_.3Csetting-number.3E_-.3E_.22LIMI.22.29

The default values (for limits) are "0" resp. "2^32 - 1".

Changing the interpretation of parameters to be signed requires a change and extension to the specs - or all existing NewGRF parameters would be interpreted wrongly

@Eddi-z
Copy link
Contributor

@Eddi-z Eddi-z commented Nov 18, 2019

Sounds like a signed/unsigned conversion bug going on. The value stored in the GRF should not be read as a signed integer, no matter what the author thought he put in there. but the dropdown seems to treat it as a signed value, which would mean even if the author set 2^32-1 as value, it would still trigger this bug

also... what would a dropdown with 2^32 entries even look like?

@glx22
Copy link
Contributor

@glx22 glx22 commented Nov 18, 2019

OpenTTD/src/newgrf_gui.cpp

Lines 377 to 382 in 7e22f24

DropDownList list;
for (uint32 i = par_info->min_value; i <= par_info->max_value; i++) {
list.emplace_back(new DropDownListCharStringItem(GetGRFStringFromGRFText(par_info->value_names.Find(i)->second), i, false));
}
ShowDropDownListAt(this, std::move(list), old_val, -1, wi_rect, COLOUR_ORANGE, true);

  | min_value | 4294967295 | unsigned int
  | max_value | 1 | unsigned int
so of course the list is empty

@Eddi-z
Copy link
Contributor

@Eddi-z Eddi-z commented Nov 18, 2019

ah, so there's a guard missing for min < max (don't trust the GRF/author)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.