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

prevent stored cal integer overflow #63849

Merged

Conversation

anoobindisguise
Copy link
Contributor

@anoobindisguise anoobindisguise commented Mar 1, 2023

Summary

Bugfixes "stored calories cap instead of integer overflowing"

Purpose of change

Fixes #63727

Describe the solution

Simply limit max stored calories in the function that adjusts stored calories.
Should go along just fine with #60597.

Describe alternatives you've considered

Testing

Compiled and tested this by setting stored kcal to 2 million and guts kcal to 2 million and didn't see stored kcal exceed ~2.14 million after waiting over 24 hours repeatedly refreshing guts kcal to 2 million whenever it fell below 1 million kcal.

Additional context

@anoobindisguise anoobindisguise marked this pull request as draft March 1, 2023 01:28
@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Mar 1, 2023
@anoobindisguise anoobindisguise changed the title limit stored cal to 2147483645 limit stored cal to 2147000000 Mar 1, 2023
@anoobindisguise anoobindisguise marked this pull request as ready for review March 1, 2023 02:50
@mqrause
Copy link
Contributor

mqrause commented Mar 1, 2023

You need to check if stored_calories + ncal is safe to do without overflow. E.g. check if INT_MAX - stored_calories > ncal, otherwise just pass INT_MAX along.

A simple test case would be nice to have, too.

@anoobindisguise
Copy link
Contributor Author

anoobindisguise commented Mar 1, 2023

You need to check if...

shouldn't the std::abs do that anyways? if it overflows it will be -2147483647 which will correct it to a positive value, which is larger than the 2147000000 and thus min will reduce it to 2147000000. right?

@mqrause
Copy link
Contributor

mqrause commented Mar 1, 2023

There's one more negative number because positive numbers start at 0, so you could get undefined behavior. You could also end up with less calories than before if you do it this way, which might not be the biggest concern, but it's weird behavior anyway.

@Qrox
Copy link
Contributor

Qrox commented Mar 1, 2023

if it overflows it will be -2147483647

Signed overflow is actually undefined by the standard, so it's best to check if it would overflow here.

@anoobindisguise anoobindisguise changed the title limit stored cal to 2147000000 prevent stored cal integer overflow Mar 1, 2023
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 1, 2023
src/character.cpp Outdated Show resolved Hide resolved
src/character.cpp Outdated Show resolved Hide resolved
@irwiss
Copy link
Contributor

irwiss commented Mar 5, 2023

I'll allow myself to voice an opinion as a normal contributor;
I feel this is over the top for the job it's supposed to be doing, in cata codebase there's tons of places where overflows may occur, in none of them we are doing even the trivial checks, it's also a non trivial issue for C++ newcomers to grasp. Some alternatives I can think of;
Use a uint32_t - makes it non-UB to overflow, keep same memory footprint but overflow will still kill the character, just at ~4b calories instead with the extra bit
Use a int64_t: give up 4 bytes per Character instance, but: over/underflow not practically reachable, calorie code doesn't seem to be on any hot paths, trivial for contributors
Make it a mix; widen to int64_t, do the addition/subtraction, check for overflow and clamp, cast back to int32 to store, still saves the 4 bytes per Character, which is overall worst of the bunch(imo) but still fairly trivial for contributors

src/character.cpp Outdated Show resolved Hide resolved
@kevingranade kevingranade merged commit 0d5db3e into CleverRaven:master Mar 7, 2023
@anoobindisguise anoobindisguise deleted the anoobindisguise-2147483647 branch July 31, 2024 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instant death by integer overflow if stored kcal > ~2,100,000
5 participants