Find a safer safe math

[miniksa]

intsafe.h is apparently included in the SDK but is cumbersome and somewhat gross.
For instance, it always returns an HRESULT.

But the bigger problem is that it's neither constexpr nor noexcept which makes an issue when combined with the extended rollout of cppcorechecks through audit mode.

This issue represents switching over to a more modern safe math class.

https://github.com/dcleblanc/SafeInt/blob/master/SafeInt.hpp is the candidate that I found as most likely at this moment.

[j4james]

I'd like to note that a number of the places I've seen "safe math" functions used in the VT code, that's actually not the behavior we need. If something overflows, what you often want is for the result to be clamped rather than having it fail. I've seen the same sort of problems with the safe conversion functions (e.g. UIntToShort), where clamping would be preferable to failure.

So before we start replacing everything with a new library, it may be worth auditing the existing safe math usage to see if that's really the behavior we want.

[miniksa]

I'd like to note that a number of the places I've seen "safe math" functions used in the VT code, that's actually not the behavior we need. If something overflows, what you often want is for the result to be clamped rather than having it fail. I've seen the same sort of problems with the safe conversion functions (e.g. UIntToShort), where clamping would be preferable to failure.

So before we start replacing everything with a new library, it may be worth auditing the existing safe math usage to see if that's really the behavior we want.

Good input. I'm trying to discern if that should be a separate effort from this one or all in the same.

[miniksa]

@j4james, I think that should be the same effort as this one or that we should add some functions when settling on a safe math library.

I think we could make some templates like the ones in SafeInt.hpp above that are basically like add_clamped that will try to use the add safe math and on failure, just clamp it to the std::numeric_limits().max value. And then walk through the VT adapters and use that instead of the incorrect safemath usages.

[lhecker]

For clamped (aka "saturation-") arithmetics not much can be found that's written in C++ and readily available. (…apart from the first result on Google of course - StefanHamminga's library.)
And we probably should actually use saturation arithmetic in more places just like @j4james said.

Arguably the best option I'm personally aware of is Chrome's base/numerics library btw (BSD license).

[miniksa]

The Chrome one looks interesting but it would probably take some adaptation to get it into a format we could consume. @DHowett-MSFT, opinion?

I'm still mostly inclined to just use the MSVC-related SafeInt with some til templates to provide clamping.

[lhecker]

@miniksa The reason I mentioned the Chromium one is because proper saturation arithmetic is of course just as annoying to implement as e.g. SafeInt itself (since in both cases you need to carefully check for overflows even in case of e.g. 64-bit multiplications).
I suppose it’s quite likely that we‘ll never hit such corner cases though and the use of "some til templates" (e.g. only supporting ≤32 bit arithmetic) will be enough for a long time to come. 🙂
SafeInt truly looks amazing and it’s a good choice regardless. 🙂

[j4james]

A couple of points I wanted to make regarding this issue...

1. I believe everything in the conhost coordinate system uses shorts. And as far as VT inputs go, parameters should never be larger than an unsigned short at most (DEC only supported up to 16384, and both VTE and Xterm clamp their parameters at 65535). Performing safe math on these values should really not require anything more than an intermediate int that clamps down to a short on exit.

   I don't know how much we care about performance, but I'd hate for us to double the size of the code, and half the speed, just so we can handle some theoretically pure situation where all the inputs are 64-bit integers.

2. It's difficult to say how feasible this is without evaluating all the use cases, but it would be really great if this library was largely transparent in terms of usage. The code would be a lot more readable, and easier to review, if we could write something like a = b + c (where one or more of the variables are some safeint/clampedint type), instead of having to do something like int a = 0; AddClamped(b, c, &a).

[miniksa]

SafeInt.h above contains both mechanisms of operation. It contains a templated type that can be used to create variables of type SafeInt where a = b + c works and it also contains the other mechanism template for SafeAdd<T, U, V>(a, b, &c).

I would imagine what we would do is create til::ClampInt<T> and/or til::ClampAdd<T, U, V> that uses the SafeInt.h templates to do the underlying math and clamp to std::numeric_limits<T>().max() or min() should it exceed boundary conditions.

Yes, most of the conhost system uses shorts. I did want to expand those to larger values to some degree internally such that we could address a more "limitless" backing buffer size in the future because it's been a big request in the past. But perhaps it's still reasonable to maintain that VT only applies to SHORT limits.

Generally speaking, I'm fine if the VT stuff would use til::ClampInt<SHORT> going forward. Does that sound reasonable, @j4james?

[miniksa]

Though, glancing at the saturating math library from @lhecker, maybe I should just run it through our licensing process to see if we could just be approved to use that... that sounds easier.

Then maybe we include both SafeInt.h for regular safe math and saturating for saturating math.

[lhecker]

@miniksa I wrote a saturating integer class a long time ago and extracted parts of the logic into a Gist.
It's pretty minimalistic that way and should cover the 80/20 rule. 😄 You may feel free to copy that shabby code if you ever want to. 🙂
…Although the more I look at the Chromium library the more I'm convinced I might even use it for myself in the future. The only thing that I'd change is that CheckedNumeric doesn't throw but rather abort()s the process.

[j4james]

@miniksa From a cursory look at the SafeInt lib, I don't think using something like ClampInt<short> would be any better than ClampInt<int32_t>, because it would still end up having to clamp after every operation. I was hoping that could be avoided by having the internal representation be of a higher precision than the clamp range.

But I'm probably fixating on performance issues that aren't really a big deal. The most important thing for me is readability, and it sounds like that is not a problem for any of these libs.

[miniksa]

@j4james, not having to clamp after every operation but finally clamping on the transition is part of why I was trying to use size_t everywhere. I was reasonably sure that it would be big enough to hold anything and we could just clamp once at the function boundary, if necessary.

I don't think any of these will drastically ruin performance. If it does, we can optimize after we fix the problem at hand.

[j4james]

The problem with size_t is it's unsigned, and you typically need the intermediate value of a calculation to be signed, so it can easily clamp to zero if it goes negative. That means the first thing I tend to do with a size_t is narrow_cast it to an int. That used to be safe when the parameters where short, but that isn't the case anymore.

Anyway, if we can fix all of that just by changing a few variable types to ClampInt, or something along those lines, then I'll be a happy camper.

[DHowett-MSFT]

happy clamper

[miniksa]

@lhecker, @j4james, as of right now, I'm liking the "lift the chromium numerics library" option the best because it provides a consistent and unified way of handling all of these math problems.

I'm looking into the best way we can include it into our repository and transition to it. I think it will handle all of our concerns.