[clang] [lldb] [Clang] Introduce OverflowBehaviorType for fine-grained overflow control (PR #148914)

[Oliver Hunt via cfe-commits]

ojhunt wrote:

I'm really sorry, I tried to trim this down. So here's a couple of the new problems I've added to this one:

* Breaking correct bounds checks:
  ```cpp
  uint8_t __obt_trap_wrapper u;
  if (__UINT32_T_MAX__ - u < some_other_value)
    return;
  if (u + some_other_value > some_int) {
    ...
  }
  ```
  It does not matter when/where narrowing occurs, it goes wrong in cases it was correct: the bounds check itself can trap and/or it incorrectly evaluates to false due to the narrowed type after the `-` evaluation.
* If the attribute is used in APIs or header types in conjunction with any kind of deduced types (decltype, templates, macros as used in many C code bases, etc) the narrowing behavior results in ABI changes. In C this is purely an ABI layout mismatch, in C++ even if the obt qualifiers are stripped the change underlying type changes the symbol name (on the plus side that might show up as a link failure).
* trapping behavior triggers even if the final result of an expression is in bounds.

** actual wall **

> Firstly, I see much of the discussion focuses on `__ob_wrap`, but what I see Linux using from this work is almost exclusively `__ob_trap`...

The focus on `__ob_wrap` is because it is silently problematic, if trapping is the only behavior that is wanted, then `__objt_wrap` should not be included at all.

But narrowing with `__ob_trap` is also problematic, take

```cpp
uint32_t __obt_trap u32 = ...;
uint64_t u64 = ...;
uint64_t result = u32 + u64;
```

Lets assume `(uint64_t)u32 + u64` is greater than `2^32 - 1`, without `__obt_*`, _any_ result with obt is wrong. Consider how `u32 + u64` is expected to be evaluated:

* `u32 + (uint32_t)u64` -> truncated result, no trap
* `u32 + clamp_u32(u64)` -> hypothetical `clamp_*` traps if the input does not fit in the target. Correct code traps.
* `clamp_u32((uint64_t)u32 + u64)` -> traps on correct code

And of course the same virality issues applies, where a single such attributed type will cause narrowing throughout an entire expression evaluation, which is either truncated incorrectly, or traps incorrectly.

> I took a look through a random handful of recent Linux integer overflow flaws that got fixed, and 9 out of 10 had no dependency on the alternative narrowing, so using standard narrowing still shows a strong benefit to Linux. So I'm not opposed to using standard narrowing, but I do want to explore the goals around why we wanted it originally.

Right - my _entire_ problem is the narrowing, as described previously I believe it renders it unadoptable in the kernel due to the completely different semantics, and even ignoring the compatibility problems, I believe that the semantics will be completely unexpected.

The question I'm wanting kernel folk to answer is whether they would be ok with something like

```cpp
uint8_t __obt_trap_wrapper u;
if (u + some_other_value > 0xff) {

}
```

Either always returning false (wrapping to uint8_t), or trapping in the overflow (?) check due to early narrowing, or returning false incorrectly but only if obt is enabled, and if obt is not enabled the only overflow exists on the promoted, but that can be prevented with:

```cpp
uint8_t __obt_trap_wrapper u;
if (__UINT32_T_MAX__ - u < some_other_value)
  return;
if (u + some_other_value > 0xff) {

}
```

But now even that overflow test is incorrect, no matter how or when the truncation happens: the overflow test now traps in some cases and produces incorrect false results in others.

> First, to give an example of where alternative narrowing is helpful, which I'll discuss below:
> 
> ```c
> u16 calculate(...) {
>   u16 __ob_trap result = 0;
>   int something;
>   // ...
>   return result - something;
> }
> ```
> 
> If we wanted anything involving a `__ob_trap` to be able to catch overflows, the above only works with alternative narrowing. Standard promotion would take `result - something` and make it an `int` (actually an `int __ob_trap`), which means we could go negative (with no overflow trap). And the return type being non-obt would silently truncate the result. So the intention of "make sure calculations involving `result` will trap" gets lost. With alternative narrowing, `result - something` treats the expression as `u16 __ob_trap` and the underflow gets trapped.

I don't believe this statement to be true. The model I believe is correct is that the above is equivalent to:

```cpp
u16 calculate(...) {
  u16 __ob_trap result = 0;
  int something;
  i32 __ob_trap widened_result = result; // I _think_ this is the correct widening rule :D
  i32 __ob_trap temp1 = widened_result - (i32 __ob_trap)something;
  u16 __ob_trap temp2 = (u16 __ob_trap)temp1; // traps if this overflows
  return temp2;
}
```

The issue is not trapping/wrapping on narrowing, it's that the narrowing is happening at a point where all existing mismatched width integer arithmetic either widens, or is an error.

> 
> Observations:
> 
> * if `something` were `int __ob_trap`, we would _also_ fail to trap since matched "kinds" (both OBTs) don't narrow, and so `result` gets promoted to `int __ob_trap`. This certainly feels inconsistent, but this was done to try to make adoption easier for a given code base. (More on this below.)
> * if `calculate` returned `u16 __ob_trap`, we'd have no problem: truncation would catch it.

I think this is the misunderstanding - the overflow mode would propagate: if there's an operation where only one input has an explicit overflow behavior, that is propagated, and I would really want removal of those tags to require an explicit cast, e.g.

```cpp
u16 calculate(...) {
  u16 __ob_trap result = 0;
  int something;
  return (u16)(result - something);
}
```

To address unintentional truncation, my belief is that this would be semantically equivalent to `(u16)(u16 __obt_mode)(result - something)`, eg obt_wrap would have defined wrapping behavior to the target size, obt_trap would trap if `result - something` was not in the range of the destination type.

One thing I think would be interesting would be whether such explicit casts would be _required_ to explicitly drop the obt mode in such casts, something like `__obt_none` (which in practice would be equivalent to `__obt_wrap` but distinguishing "I am intentionally removing the qualifier" vs "I am changing the mode" might be good?

> 
> So, the goal of narrowing the mismatched Kind, I realize now, was mainly around C's ambiguity of arithmetic operation's width given commutative operation. If operations were directional we might infer bit width from the "value being operated on": "subtract `something` from `result` (which has u16 width)", which is what we were trying to do. The functional arithmetic builtins (e.g. `__builtin_check_mul_overflow`) solve this by making the target width explicit. But no C programmer I know is excited about switching to functional arithmetic. ;) I imagine a wild way to declare this in the language by casting the operator, like `result (u16)- something` but I cannot imagine a way to make that acceptable to C programmers. :)

The thing is that I believe developers have no difficulty understanding the widening behavior, but this narrowing behaviour is extremely unintuitive: why would adding a narrower value to a wider one _ever_ be expected to produce a narrowed result?

**Thing I just realised**

All this discussion has been in the context of differing width, what happens with same width but mismatched signedness? e.g

```cpp
u32 __obt_X a = ...;
i32 b = ...;
type1 r1 = a + b;
u32 c = ...;
i32 __obt_X d = ...;
type2 r2 = c + d;
```

I think my understanding of the expected semantics would mean `type1` is not the same as `type2`, which adds even more confusion to the semantics of signed vs unsigned promotion. Honestly I would say that given the clearly confusing signed vs unsigned promotion - sidemotion? - rules, any mixed sign expressions where any obt mode is present should be an error.

I think we would also want to warn if we see at least a warning for things like:

```cpp
(target_type)differently_signed_value + target_type_value
```

Because the unqualified target_type cast could unintentionally truncate/overflow (dropping negative, wrapping to negative, etc).


> Looking at Rust's arithmetic, there is just simply _no_ mismatched bit widths allowed.

Honestly I think that would not be terrible, it would certainly be preferable to unexpected narrowing, but you get into ergonomic issues with literals `some_nont_int_obt_value + unsuffixed_literal` - ergonomics mean we don't want to require manual annotation (is there actually a suffix for int or char literals? I just realized I don't actually know :D). The problem with literals is that if we want to maintain the narrowing behavior you get overflows where there should not be, you get differences in behavior when obt modes are available vs not, and clean up refactorings also change behavior, e.g

```cpp
u16 __obt_mode x = ...;
x + 1 // a literal
x + (1 << 10) // not a literal but generally treated as such
const int refactored = 1 << 10;
x + refactored // no longer a literal but what should be a semantic no-op
```

> Things need to be explicitly cast, so there is no ambiguity about bit width. Going all the way to this requirement feels like the furthest swing away from usability in C, especially with the goal of slowly adding `__ob_trap` types/annotations to an existing codebase.

Right, I think widening should be permitted, though the widening would propagate obt qualifiers, dropping obt qualifiers should require a cast. It's probably worth emitting a warning in compound expressions that include arithmetic on unqualified values leading into arithmetic with qualified ones.

> So, I think the spectrum of solution is: "require matching kinds and widths" to "use standard narrowing". @JustinStitt and I considered two possible compromises:
> 
> * use standard narrowing/promotion but preserve bit width overflows along the way, and surface it at the end somehow

I still don't understand what hazard you are trying to prevent here, narrowing only applies if the obt type is the narrower/different type, so:

* If you widen the obt qualified type, the widened type still has the obt qualifier
* If you perform a subsequent operation involving a narrower type it's promoted and you retain the selected obt semantics
* If you then try to store the widened type into a narrower type, the obt qualifier applies to the narrowing operation

These semantics also mean you get sensible and consistent behaviour for obt qualified bitfields as well (i'm not sure if the current proposal permits application to bitfields, but these semantics result in this being reasonable):

```cpp
struct S {
  uint32_t __obt_trap field: 20;
}

// with narrowing consistency would imply that this actually should be
// uint20_t __obt_trap. With standard widening that weirdness goes away
// and the result is uint32_t __obt_trap
auto u32 = 100 + someS.field;
S.field = u32; // this then traps if the value is >= 1<<20

```

> * use sub-int promotion only with matching Kinds, e.g. `u8 __ob_trap + u16 __ob_trap` results in `u16 __ob_trap` not `int __ob_trap`

See i don't understand why this would be expected behavior - I (and i would expect most devs) would expect the result to be `decltype((u8)0 + (u16)0) __ob_trap`. Subsequent narrowing would trap if the narrowing would be out of bounds.


> The case that I find most troublesome (and has the largest security impact) is that of wrapping positive during int promotion, which I think is handled by making sure that promotions always promote toward `__ob_trap` when present. For example:

As stated above that safety depends on the point at which the narrowing truncation happens. If such operations are performed by first narrowing the wider type, if that loses information without trapping/wrapping, you truncate from the correct behavior, resulting in incorrect result and that result may be invalid, and you trap on the narrowing operation you can end up with operations that would have produced an _in bounds_ result would trap.

I've given examples of where the cast point may impact out of bounds result, but the in bounds result case is also worth considering:

```cpp
uint64_t u64 = 1ull<<32;
uint32_t __obt_trap u32 = 1;
// converting u64 to u32 at the start traps incorrectly
uint32_t __obt_trap result = u64 - u32;
```

> 
> ```c
> int a, b, c;
> a = INT_MAX;
> b = INT_MAX - 99;
> c = a * b; // wraps to 100
> ```
> 
> If `a` or `b` is `__ob_trap`, the calculation needs to use `__ob_trap` to see the overflow. If only `c` is `__ob_trap`, it'll miss the overflow, so it'd be nice if the `__ob_trap` got applied too.

Yup, that's why I wondered if we want to issue warnings if part of an integer expression is qualified and part is not but performs operations that may overflow

> I'm not sure my wall of text ended up as organized as I want it, but I think the problem we saw as unsolved was dealing with commutativity for catching overflows meaningfully: we either need directionality or homogeneity. Directionality is hard to articulate in the language, and homogeneity requires assumptions/ambiguity.

I do not believe any approach that involves implicit narrowing is safe - it makes conditional adoption unsound due to the extremely different semantics across compilers, it changes deduced types, and it makes code that would reasonably be expected to produce a widened result (both due to that being the semantics everywhere else, and because it does not make sense to narrow one of the inputs). I think that the different types that this implicit narrowing produces can possibly result in abi/odr problems, that introduce security bugs:

```cpp
struct APIStruct {
  u32 __conditional_obt_mode_wrapper a;
  template <integral intype> auto f(std::span<intype> in) -> std::vector<decltype(a + in[0])> {
    std::vector<decltype(a + *out)> v;
    for (auto x : in)
      v.push_back(x + a);
    return v;
  }
}
```

These now have different sizes, and if the output of `f` is passed to an implementation with a different obt mode, then use either get a potential buffer overflow, or incomplete initialization. This problem occurs whether obt_wrap or obt_trap is used.

This is technically an ODR violation, but if your code is compiled with different compilers, or different modules, this seemingly innocuous "remove overflow UB" flag results in an ABI mismatch. The fix for this is to not ever use the qualifier in any non-local context - e.g. local variables only. The ODR violation is only due to the narrowing behaviour, but the viral nature of that narrowing means that the qualifier essentially becomes unsafe in any ABI facing position, or any environment where the `obt` semantics can't be used everywhere. That would include the linux kernel due to module interfaces at least, as binary modules would not match the abi - i recall the linux kernel using macros to do/wrap things similar decltype but that was more than a decade ago.

Firstly, I really believe it would be entirely warranted to reject expressions with mismatching signedness if any are obt qualified.

Beyond that change I really believe that the safe semantics are one of:

1. Reject any mismatch in underlying types. This would mean `(char __obt_..)0 + 1` would be required to be a compile time error, or you get the kinds of errors and incompatibilities I've discussed previously. This removes many of the narrowing hazards, but retaining the smaller than `int` types for those expressions still retains all the previous problems discussed for `int` and wider types.
2. Match existing promotion rules, propagating the obt semantics through those promotions
  - Any conversion that involves an obt qualified type the truncates the range of any value would apply the specified semantics if such truncation occurs (trapping, wrapping)
  - Equivalently casts (explicit or implicit) or assignment to storage with a mismatched/narrower type (if either the source or destination type is obt qualifier) would propagate and enforce the obt semantics

I think (1) is still problematic due to the smaller than int narrowing behaviour, e.g `u8 __obt_wrap a = 255; u32 __obt_wrap b = a + 1` will produce `b == 0` if obt is enabled, whereas it produces `b == 256` if not, meanwhile `u8 __obt_trap a = 255; u32 __obt_trap b = a + 1) - 1;` now traps. Consider something like a buffer joining operation:

```cpp
template<class T> struct Buffer {
  unsigned size; // type and qualifier not relevant here
  using data_type_t = T;
  data_type_t *data;
  std::span<data_type_t> span() { return std::span(data, size); }
};

// Again these types may not be specified explicitly, but aquired via macros,
// typedefs, template deduction, etc
template <class In> auto zip(auto f, std::span<In> a, std::span<In> b) -> std::vector<decltype(f(a[0], b[0]))> {
  std::vector<decltype(f(a[0], b[0]))> result;
  for (size_t i = 0; i < b.size(); i++) // Sssshhhh, assume a and b are the same size :D
    result.push_back(f(a[i], b[i]));
  return result;
}

void f(Buffer<uint8_t> buffer1, Buffer<uint8_t> buffer2) {
  ...
  auto v = zip([](auto a, auto b){ return a + b; }, buffer1.span(), buffer2.span());
  ...
}
```

At this point this produces a `std::vector<int>` that does not lose any data. Now imagine someone comes along and says "signed overflow is ub, so lets be safe and change `data_type_t` to `T __obt_wrap` in case `T` is a signed type" (imagine logic exists that only adds this qualifier if not already specified) - now the result type has changed from `vector<int>` to `vector<uint8_t __obt_wrap>`, and results are incorrect, and subsequent use of the vector will also truncate those subsequent results as well.

`__obt_trap` isn't much better - the intermediate calculations now trap instead of producing the expected output.

It's unclear if even a warning for passing/storing values of type `narrower_type __obt_x` to `wider_type_t` would be sufficient, because that doesn't work for deduced types, it doesn't work for non stored results (arguments to `switch`, `if`, etc). Even if we consider that sufficient that warning would occur a _lot_.



https://github.com/llvm/llvm-project/pull/148914