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 UB in random generator #4588

Merged
merged 1 commit into from
May 18, 2022
Merged

Fix UB in random generator #4588

merged 1 commit into from
May 18, 2022

Conversation

AMDmi3
Copy link
Contributor

@AMDmi3 AMDmi3 commented May 17, 2022

abs(INT_MIN) is undefined behavior, so process this case without calling abs()

This problem manifestates itself on FreeBSD with clang 11.0 by breaking QuestTest.SinglePlayerBadPools and RandomTest.ShiftModDistributionSignCarry

Copy link
Contributor

@ephphatha ephphatha left a comment

Choose a reason for hiding this comment

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

Good to see it fails on another system :D, makes the tests worthwhile. Beyond the nit about magic numbers it's good to be more explicit here.

@@ -29,7 +29,8 @@ uint32_t GetLCGEngineState()

int32_t GetRndSeed()
{
return abs(static_cast<int32_t>(sglGameSeed));
const int32_t seed = static_cast<int32_t>(sglGameSeed);
return seed == -2147483648 ? -2147483648 : abs(seed);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return seed == -2147483648 ? -2147483648 : abs(seed);
return seed == std::numeric_limits<int32_t>::min() ? std::numeric_limits<int32_t>::min() : abs(seed);

abs(INT_MIN) is undefined behavior, so process this case without
calling abs()
@AJenbo AJenbo merged commit 04d0950 into diasurgical:master May 18, 2022
@AMDmi3 AMDmi3 deleted the patch-2 branch May 18, 2022 10:43
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.

None yet

3 participants