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

Validate that "max" values in settings fit in their storage container #8780

Merged
merged 3 commits into from Mar 1, 2021

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Mar 1, 2021

Fixes #8610.

Motivation / Problem

Yesterday I added a setting where the maximum value exceeded the storage size. I was not aware of this, and nothing told me it was wrong. The reviewer missed it too.

To prevent this simple mistake from ever happening again, we should simply validate this.

Found 4 current instances that already were out-of-bounds. By accident this fixes an issue :D

Description

Make settingsgen generate a post-amble that does a ton of static_assert to validate max is within the storage size assigned to the setting. This means that if you add a new setting, the compiler will tell you you suck at cookie, euh, coding.

It required moving some code in settingsgen to its own function, to avoid code duplication.

This could not be done by settingsgen btw, as settingsgen doesn't understand the values it is reading/writing .. it just copies them byte-by-byte. This makes sense, as you can use constants, additions, etc etc.

Fixed the four issues found too.

Limitations

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 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')

def = DEF_SNOWLINE_HEIGHT * TILE_HEIGHT
min = MIN_SNOWLINE_HEIGHT * TILE_HEIGHT
max = MAX_SNOWLINE_HEIGHT * TILE_HEIGHT
# "max" used to be MAX_SNOWLINE_HEIGHT * TILE_HEIGHT, but this would overflow the storage.
max = UINT8_MAX
Copy link
Contributor

@nielsmh nielsmh Mar 1, 2021

Choose a reason for hiding this comment

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

Wait a minute here, what unit is snow line height measured in? Because as far as I remember, TILE_HEIGHT is a constant for converting height levels into pixel lengths. Are sub-tile height snow line heights supported even?

Copy link
Member Author

@TrueBrain TrueBrain Mar 1, 2021

Choose a reason for hiding this comment

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

Don't get me started about this mess. But there is a AfterLoad routine that converts the values here back to their non-TILE_HEIGHT counter-part. So in our current code, snow_line_height is just a number between 1 and 255. Just for older savegames it can have other values ...

I wasn't sure how to fix this properly btw, so I went with this. "dead" code anyway, was savegame revision 22 .. lol :)

https://github.com/OpenTTD/OpenTTD/blob/master/src/saveload/afterload.cpp#L2774

Copy link
Member Author

@TrueBrain TrueBrain Mar 1, 2021

Choose a reason for hiding this comment

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

To add to that, I have no clue when clamping is done .. before or after afterload. As there is a second snow_line_height entry, that is the current version, but the afterload suggests older versions can have the value multiplied by TILE_HEIGHT too .. so if it is clamped before afterload, that line in afterload won't work anyway ..

This confuses me :P

Cleaning up mistakes of the past :D

src/settings_type.h Outdated Show resolved Hide resolved
src/settingsgen/settingsgen.cpp Outdated Show resolved Hide resolved
TrueBrain added 2 commits Mar 1, 2021
…ored

So any old game made with this setting was overflowing anyway;
not really a lot we can do about that now.
@TrueBrain TrueBrain force-pushed the validate-max-fits-in-storage branch 3 times, most recently from 3509bc4 to 2539652 Compare Mar 1, 2021
This is an easy mistake to make, so protect us against making such
mistakes, by validating it doesn't happen.
@TrueBrain TrueBrain force-pushed the validate-max-fits-in-storage branch from 2539652 to 8f57c3a Compare Mar 1, 2021
LordAro
LordAro approved these changes Mar 1, 2021
@TrueBrain TrueBrain merged commit 74aa934 into OpenTTD:master Mar 1, 2021
11 checks passed
@TrueBrain TrueBrain deleted the validate-max-fits-in-storage branch Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants