Fix potential undefined behaviour when using OverflowSafeInt #9451

LordAro · 2021-07-19T12:23:27Z

Motivation / Problem

OverflowSafeInt can actually underflow.

Description

Changes ~~completely copied from~~ inspired by JGR's work:
JGRennison/OpenTTD-patches@6821c0e
JGRennison/OpenTTD-patches@bb8d2c3
JGRennison/OpenTTD-patches@609f37c
JGRennison/OpenTTD-patches@db0e25f

But mildly neatened and modernised.

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')
- newgrf_debug_data.h may need updating.
- PR must be added to API tracker

src/core/overflowsafe_type.hpp

TrueBrain · 2021-07-19T13:50:37Z

I am really certain I do not understand this code, so I just brute-forced it what I could think of:

https://godbolt.org/z/KE3qc1cqq

If you think I missed cases, feel free to add them and post the new link ;)

Seems all cases I could come up with work fine.

Edit: just to smoketest the compiler built-in variants: https://godbolt.org/z/5T8bPeYKe . Also shows no issues for the cases I came up with.

rubidium42 · 2021-07-19T19:43:37Z

Edit: just to smoketest the compiler built-in variants: https://godbolt.org/z/5T8bPeYKe . Also shows no issues for the cases I came up with.

Maybe guard against the built-in variant being used when min/max are not the min/max of the type? For example, the following works with the original code, but fails with the built-in variant.

OverflowSafeInt<int8_t, 5, 3> c = 3;
assert(c + 3 == 5);

LordAro · 2021-07-19T19:59:11Z

Is it worth supporting that though? We can get rid of the extra 2 tparams with std::numeric_limits these days

rubidium42 · 2021-07-19T20:02:36Z

Is it worth supporting that though? We can get rid of the extra 2 tparams with std::numeric_limits these days

Knowing that std::numeric_limits exists was kind of the reason I was trying this as I (naively) assumed that it might have worked. After all, why have those parameters. Though removing the parameters seems another viable solution to the problem.

TrueBrain · 2021-07-19T20:03:45Z

I think using numeric_limits would be great. I just worry about the depth of this rabbithole you found. Also:

OpenTTD/src/script/api/script_company.cpp

Lines 143 to 145 in d1cf566

    
           	return ::Company::Get(company)->cur_economy.delivered_cargo.GetSum<OverflowSafeInt<int32, INT32_MAX, INT32_MIN> >(); 
        
           } 
        
           return ::Company::Get(company)->old_economy[quarter - 1].delivered_cargo.GetSum<OverflowSafeInt<int32, INT32_MAX, INT32_MIN> >();

Can we address those 2 instances while at it? Seems it should just be OverflowSafeInt32 :)

src/core/overflowsafe_type.hpp

INT64_MIN negated is above INT64_MAX, and would overflow. Instead, when negating INT64_MIN make it INT64_MAX. This does mean that -(-(INT64_MIN)) != INT64_MIN.

TrueBrain

For future-us: the mistake made here is that -T_MAX != T_MIN, but the code did assume it. In essence, that is what broke underflow protection, and what this PR addresses.

Notably, a company with an extremely negative amount of money would suddenly become very rich

src/core/overflowsafe_type.hpp

…eInt

TrueBrain

Nice.

TrueBrain reviewed Jul 19, 2021

View reviewed changes

src/core/overflowsafe_type.hpp Outdated Show resolved Hide resolved

src/core/overflowsafe_type.hpp Show resolved Hide resolved

LordAro force-pushed the overflow-fixes branch from f91806e to e7caa23 Compare July 20, 2021 07:19

TrueBrain reviewed Jul 20, 2021

View reviewed changes

src/core/overflowsafe_type.hpp Outdated Show resolved Hide resolved

Fix: OverflowSafeInt negation not handling INT64_MIN

07eb4a9

INT64_MIN negated is above INT64_MAX, and would overflow. Instead, when negating INT64_MIN make it INT64_MAX. This does mean that -(-(INT64_MIN)) != INT64_MIN.

LordAro force-pushed the overflow-fixes branch 2 times, most recently from af1e8c3 to e6dc779 Compare July 20, 2021 07:48

TrueBrain previously approved these changes Jul 20, 2021

View reviewed changes

Fix: OverflowSafeInt could underflow

c780f43

Notably, a company with an extremely negative amount of money would suddenly become very rich

LordAro dismissed TrueBrain’s stale review via 5d7d5b6 July 20, 2021 08:05

LordAro force-pushed the overflow-fixes branch from e6dc779 to 5d7d5b6 Compare July 20, 2021 08:05

TrueBrain previously approved these changes Jul 20, 2021

View reviewed changes

LordAro dismissed TrueBrain’s stale review via 415dc4b July 20, 2021 08:50

LordAro force-pushed the overflow-fixes branch from 5d7d5b6 to 415dc4b Compare July 20, 2021 08:50

TrueBrain reviewed Jul 20, 2021

View reviewed changes

src/core/overflowsafe_type.hpp Outdated Show resolved Hide resolved

LordAro added 3 commits July 20, 2021 09:55

Codechange: Use GCC/clang builtins for overflow safety when supported

1108651

Codechange: Remove (unused) ability to specify min/max of OverflowSaf…

27da300

…eInt

Codechange: Make OverflowSafeInt constexpr

2a0a219

LordAro force-pushed the overflow-fixes branch from 415dc4b to 2a0a219 Compare July 20, 2021 09:00

TrueBrain approved these changes Jul 20, 2021

View reviewed changes

LordAro merged commit f1dfc2f into OpenTTD:master Jul 20, 2021

LordAro deleted the overflow-fixes branch July 20, 2021 09:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix potential undefined behaviour when using OverflowSafeInt #9451

Fix potential undefined behaviour when using OverflowSafeInt #9451

LordAro commented Jul 19, 2021

TrueBrain commented Jul 19, 2021 •

edited

Loading

rubidium42 commented Jul 19, 2021

LordAro commented Jul 19, 2021

rubidium42 commented Jul 19, 2021

TrueBrain commented Jul 19, 2021

TrueBrain left a comment

TrueBrain left a comment

Fix potential undefined behaviour when using OverflowSafeInt #9451

Fix potential undefined behaviour when using OverflowSafeInt #9451

Conversation

LordAro commented Jul 19, 2021

Motivation / Problem

Description

Limitations

Checklist for review

TrueBrain commented Jul 19, 2021 • edited Loading

rubidium42 commented Jul 19, 2021

LordAro commented Jul 19, 2021

rubidium42 commented Jul 19, 2021

TrueBrain commented Jul 19, 2021

TrueBrain left a comment

Choose a reason for hiding this comment

TrueBrain left a comment

Choose a reason for hiding this comment

TrueBrain commented Jul 19, 2021 •

edited

Loading