Prevent integer overflows in bionic power #34684
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Prevent an integer overflow that can occur if the new power added in Character::mod_power is greater than the max power level. This would cause power to be clamped to 0, because the integer overflow would make the power to be added massively negative, when it should be positive. Also prevent an integer overflow that occurs when units::from_joule and units::from_kilojoule are given a value that, when converted to millijoules, is larger than it's type can store. This was another cause of problems in bionic power, because the new power could overflow, and then not be the correct value. This occurred in testing when trying to use the battery charger system with a heavy disposable battery.
KorGgenT
added
<Bugfix>
jbytheway
reviewed
Dec 6, 2019
| int new_power = units::to_millijoule( power_level ) + units::to_millijoule( npower ); | ||
| // An integer overflow has occurred - the sum of two positive things (power_level is always positive) | ||
| // cannot be negative, so if it is, we know integer overflow has occured | ||
| if( npower >= 0_mJ && new_power < 0 ) { |
There was a problem hiding this comment.
This test is relying on signed integer overflow, which is undefined behaviour, so it could potentially break depending on compiler optimization. Easiest solution is to do the computation in int64_ts instead, then clamp and cast back to int. Or you can do the more roundabout approach of subtracting npower from max_power_level and comparing with power_level (but then you start risking underflow if you're not careful, so it's hard to do correctly).
There was a problem hiding this comment.
Whoops, thanks #36686!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
SUMMARY: Bugfixes "Fix integer overflows when bionic power is modified and when a value is converted to units::energy"
Purpose of change
Resolves #34665
The value by which power is modified in
Character::mod_power_levelcould overflow when the new power was large enough that when it was added to the player's power_level the value it produced was larger than the value an int could store.An overflow could also happen in
units::from_(kilo)joulewhen the value provided, when converted to millijoules, was larger than it's type could store.Describe the solution
Add checks to prevent overflow in all the functions mentioned.
Describe alternatives you've considered
Add a check in
units::from_millijouleto prevent overflow, but I couldn't think of a good way to do that without loosing precision.Testing
Save that I used for testing provided.
overflow.tar.gz
To test for the overflow in
Character::mod_power_level, eat the battery with ~650 charges (the character has 1980kJ of power, and their max capacity is the max power). The power will overflow and be clamped to 0kJ.To test for overflow in
units::from_kilojoule, consume one of the batteries with 2500 charges, because this is more than can be stored in an int as a millijoule.