[RFC] add wraps attribute (for granular integer overflow handling)

Introduction and Background

tl;dr: I have a tree that implements the “wraps” attribute.

This attribute would allow for more granular control over what expressions can emit integer overflow warnings or integer overflow sanitizer errors.


Currently, you cannot attach __attribute__((no_sanitize("signed-integer-overflow"))) to variable or type declarations. If you wanted to disable signed or unsigned integer overflow checks you’d have to use the aforementioned attribute as a function attribute, thus disabling instrumentation for the entire function. This works but is not granular enough for many cases. Another (and more granular) solution is to use builtins to intentionally perform wrapping arithmetic. However, in some codebases, refactoring existing arithmetic to utilize these overflow builtins may only serve to complicate code. For example, the Linux Kernel has, like lots of other projects, some agreed upon idioms which are understood by its developers and changing these patterns is frowned upon:

“if there’s some unsigned wrap-around checker that doesn’t
understand this traditional way of doing overflow checking, that piece
of crap needs fixing.” - Linus

This was in response to a patch that was trying to change the commonly accepted idiom: base + offset < base to utilize a builtin (via a macro) to silence sanitizer errors.

Recently, there’s been some effort by Kees, myself and others to reintroduce the signed and unsigned integer overflow sanitizers in the Linux Kernel. Upon turning these sanitizers back on (or for the case of signed-integer-overflow, making it work at all) we encountered plently of existing instances of integer overflow in the Kernel. However, there’s some pushback when trying to alter the “traditional” way of doing things.

With this new wrapping attribute we can specifically craft types that disable overflow instrumentation, without modifying traditional and widely understood code patterns.

It should be noted that constant expressions containing a wrapping type or variable should not result in -Winteger-overflow warnings either.

Examples

When compiling with -fsanitize=signed-integer-overflow,unsigned-integer-overflow:


typedef int __attribute__((wraps)) wrapping_int;

wrapping_int A = INT_MAX;
++A; /* This will not trigger a runtime sanitizer error */


unsigned long addr __attribute__((wraps)); /* wraps can be added to variables */
unsigned long size; /* both operands don't need wraps, just one */
...
if (addr + size < addr) {
  // handle overflow gracefully, because the overflow sanitizer won't trip
}
...


typedef int __attribute__((wraps)) wrapping_int;

int a = (wrapping_int) INT_MAX + 1; // no sanitizer trip (or -Winteger-overflow)

The next few examples will show some cases where sanitizers will still trip:


wrapping_int A = INT_MAX;
int B = 1;

A = INT_MAX + 1; // overflow sanitizer trips! (INT_MAX + 1) is computed and trips the sanitizer before assignment to A

B = (INT_MAX+B) * A; // overflow sanitizer trips!

B =  INT_MAX + B * A; // overflow sanitizer does NOT trip!
// This is because B * A is computed first, the result type carries the wraps attribute
// with it from A, then that result is used in summation with INT_MAX.

To summarize this behavior:

Using __attribute__((wraps)) on a typedef or variable declaration makes it a “wrapping” type or variable thereby disabling overflow instrumentation for either 1) arithmetic performed directly on wrapping variables or types or 2) arithmetic performed on the result of calculations containing a wrapping variable or type. Instrumentation is not disabled for calculations containing subexpressions wherein no wrapping variables are present.

Other Notes

  • [[wraps]] and [[clang::wraps]] are supported for C++11

  • The wraps attribute cannot be applied to functions

  • The wraps attribute can be applied to member variables

  • The wraps attribute can still be used even when the sanitizers and warnings
    are disabled, at this point it’s just an annotation to readers of the code that
    arithmetic may wraparound, expectedly so.

  • No documentation or full frontend diagnostics yet – I’m waiting to hear back from folks regarding this RFC

CCs

@kees @nickdesaulniers @void

1 Like

Your example of ‘still trips’ shows the problem of making this a declaration attribute, which is that ANY operations on it is lost almost immediately when doing math.

That is:

 ++ (++A); 

Will likely hit the sanitizer (since the attribute is gone as soon as the first operation happens).

SO I question the value to said attribute when it disappears so quickly.

Secondly, I am not a big fan of the name, I think we should bikeshed this better. wraps isn’t a particularly descriptive name, and takes up a single-word that might be more useful in the future for other purposes.

Third: I’d like to have someone more familiar with the sanitizer take a look to see if there isn’t a better way to do this.

2 Likes

Side note: this is a rough time to send an RFC, much of the Clang team is on our way (some in the air as we speak!) to the WG21 meeting, so we likely wont’ get sufficient response here until April.

2 Likes

I put some effort into making sure the attribute sticks around even after some operations.

Can you provide a different example other than ++ (++A);. I can’t get that to compile.

edit: Ah, I see now that ++(++A); compiles in C++ but not C, at any rate the attribute stops the sanitizer from tripping there (ignore my follow-up below)

I think this example is in the same spirit as what you proposed:

  wrapping_int A = INT_MAX - 1;

  if ((++A) + 1)  { // doesn't trip sanitizer

  }

Ah, i see, you DO end up putting it in an AttributedType as well as it being a Decl attribute, I’d suspect the attribute should exist as a type attribute explicitly, else I’m not sure what we’re really doing with it on declarations. That said, it’ll get lost basically any time there is an integral promotion, or an implicit cast (or if it is ever used as a template arg/etc).

That said, I find myself wondering if putting this on the Statement that overflows would be a better use case here, so that you highlight the operation that you expect to wrap, not the variable itself.

The granularity that this provides is much less invasive that doing per-statement annotations. (Though since you capitalized “Statement”, perhaps I have misunderstood.)

For example, there is a (wrapping) counter in the kernel for doing timekeeping (“jiffies”). There are tons of places all over the kernel where a value derived from jiffies is manipulated, and adding annotations (or wrappers) for each of these statements would be seen as unacceptably intrusive. However, making a jiffies type, and doing conversions for that would likely be seen as a viable cleanup, as it would make timekeeping code more clear.

And to state the ultimate goal here: we want the Linux kernel to have unambiguously described arithmetic: it should be clear for a given operation whether the author’s intent was for a calculation to wrap or not, allowing Linux to have deterministic mitigation of unexpected arithmetic overflows. The most complete path to this is through the signed/unsigned-integer-overflow and implicit-signed/unsigned-integer-truncation sanitizers. But they have a gap in that there is not currently a way to describe sanitizer coverage through types.

1 Like

How does __attribute__((wraps)) interact with the definedness of signed integer overflow? How does it interact with integer promotion?

Regarding definedness of overflow behavior: If -fwrapv is enabled then __attribute__((wraps)) should be a nop as the behavior of integer arithmetic is already defined as wrapping.

As for integer promotion, AFAIK a less-than-int type cannot overflow and thus __attribute__((wraps)) is again a nop when used with less-than-int types.

Currently, __attribute__((wraps)) does nothing to stop -fsanitize=implicit-(unsi|si)gned-integer-truncation from tripping in a case like this:

u8 A = 255;
++A;

wraps.c:15:3: runtime error: implicit conversion from type 'int' of value 256 (32-bit, signed) to type 'u8' (aka 'unsigned char') changed the value to 0 (8-bit, unsigned)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior wraps.c:15:3 

I am not sure if wraps should or should not prevent these. I’d almost envision another attribute like may_truncate.

The interesting case to address is what happens if you use wraps on a signed integer and -fwrapv is not specified. Specifying that wrapping is OK is incompatible with wrapping being undefined behavior. I think that should at least get you a warning.

IIRC, -fsanitize=%-integer-overflow implies -fsanitize=implicit-%-integer-truncation, so the fact ++a isn’t legalized by __attribute__((wraps)) seems like a shortcoming to me. However, the original question I had was actually whether wraps is inherited through promotions from int to long.

1 Like

When the attribute suppresses a warning about what’s technically undefined behavior, should it also instruct CodeGen to make it not UB? Or just never suppress such warnings?

“if there’s some unsigned wrap-around checker that doesn’t
understand this traditional way of doing overflow checking, that piece
of crap needs fixing.” - Linus

It looks like they’re always building with -fno-strict-overflow which makes all overflows well-defined. In this case, yeah, we’re entering the weird territory where we need to discriminate between intentional and unintended overflows and establish coding conventions. The tool needs to give a different answer for different arithmetic code over the same variable. Your attribute doesn’t seem to address this issue right?

1 Like

Hi @NoQ, thanks for the comments.

When the attribute suppresses a warning about what’s technically undefined behavior, should it also instruct CodeGen to make it not UB? Or just never suppress such warnings?

I agree. In the current implementation, during CodeGen I am using CreateMul instead of CreateNSWMul (and same for other scenarios). I believe this is enough to avoid eager compiler optimizations based on the UB of overflowing arithmetic.

It looks like they’re always building with -fno-strict-overflow which makes all overflows well-defined.

Yeah, the Linux Kernel builds with -fno-strict-overflow. This is because they also build with -O2 and compilers were being way too eager about optimizing away some things in the name of UB:

if (x + 1337 < x) {
    // eager optimizations can make this unreachable
}

In this case, yeah, we’re entering the weird territory where we need to discriminate between intentional and unintended overflows and establish coding conventions.

Yep! That’s a big reason for this RFC. So many bugs are caused by unintentional overflow, we need some way to say “this thing should overflow” without having to use -fwrapv or -fno-strict-overflow which defines overflow for entire programs. There are legitimate use cases for using twos-complement wrap-around semantics and the wraps attribute says “this is one of those legitimate use cases”.

The tool needs to give a different answer for different arithmetic code over the same variable . Your attribute doesn’t seem to address this issue right?

Can you elaborate here? I think I covered most types of arithmetic that could trip sanitizers or warnings, ensuring to disable them when this attribute is present.

I don’t mean different kinds of arithmetic, I mean the same kind of arithmetic (eg. simple +) found in different contexts.

Sometimes I don’t want to suppress all warnings for a variable, because I am actually worried about overflow, I want it to be caught at runtime. But at the same time I need a way to perform traditional overflow checks on that variable without being caught at runtime.

What are my options in this case?

1 Like

Hi @fay59, thanks for the comments.

The interesting case to address is what happens if you use wraps on a signed integer and -fwrapv is not specified. Specifying that wrapping is OK is incompatible with wrapping being undefined behavior. I think that should at least get you a warning.

Right, in my response to NoQ I also mentioned how I altered the CodeGen so there is no UB emitted. I think this attribute has some value when -fwrapv is not enabled, introducing lots of warnings from users intentionally marking things as wraps will discourage its use.

IIRC, -fsanitize=%-integer-overflow implies -fsanitize=implicit-%-integer-truncation

This doesn’t seem to be the case in my testing: Compiler Explorer

However, the original question I had was actually whether wraps is inherited through promotions from int to long.

Do you mean something like this:

  int a __attribute__((wraps)) = 1;
  long b = a;
  b += LONG_MAX;

Currently, this will still trip the sanitizers. The attribute isn’t inherited through assignments (with our without promotions).

Sometimes I don’t want to suppress all warnings for a variable, because I am actually worried about overflow, I want it to be caught at runtime. But at the same time I need a way to perform traditional overflow checks on that variable without being caught at runtime.
What are my options in this case?

Ahh, gotcha.

The current idea is you can downcast off the attribute like this:

typedef int __attribute__((wraps)) wrapping_int;

...

wrapping_int a = INT_MAX;
int b;

// b = a + 1; // will not trip sanitizers

b = (int) a + 1; // will trip sanitizers

Even some macro like (to be explicit):

#define check_overflow(type, e) ((type)e)
...

b = check_overflow(int, a) + 1;

Admittedly, I am not the biggest fan of the aesthetics of the above solution.

The wraps attribute is not the only solution to this whole overflow vs wrap-around problem. Previously mentioned stuff like -fwrapv works but isn’t so granular. In the case you mentioned, if you have a variable that you sometimes want to check for bad overflow and sometimes want its overflow to be defined (and not warn) you can use compiler builtins like __builtin_add_overflow.

So it goes something like this:

  • I want all overflow to be treated as wrapping === -fwrapv

  • I want a specific variable to be treated as wrapping === wraps
    ** but sometimes I want to overflow to be caught === downcast ad-hoc

  • I want a specific variable to have its overflow caught === default behavior
    ** but sometimes I want to allow it to wrap-around in a non-UB way === builtins

1 Like

Thanks. This is specifically the example that I have in mind:

int a __attribute__((wraps)) = 1;
long b = a + LONG_MAX;

a is implicitly promoted to long before participating in the addition. Do implicit promotions keep wraps?

a is implicitly promoted to long before participating in the addition. Do implicit promotions keep wraps ?

In that example, the wraps attribute is still applied, here’s the AST:

        `-BinaryOperator 0x55f0fcb78788 <col:12, <built-in>:60:22> 'long __attribute__((wraps))':'long' '+'
          |-ImplicitCastExpr 0x55f0fcb78770 <wraps.c:14:12> 'long' <IntegralCast>
          | `-ImplicitCastExpr 0x55f0fcb78758 <col:12> 'int __attribute__((wraps))':'int' <LValueToRValue>
          |   `-DeclRefExpr 0x55f0fcb78718 <col:12> 'int __attribute__((wraps))':'int' lvalue Var 0x55f0fcb785a0 'a' 'int __attribute__((wraps))':'int'
          `-IntegerLiteral 0x55f0fcb78738 <<built-in>:60:22> 'long' 9223372036854775807

The BinaryOperator is promoted to long and the attribute is also present.

Whether this is good depends entirely on how often this happens. It doesn’t sound like Linus is excited to add in-code suppressions for every manual overflow check, though other folks may be interested in such proposition.

Hardcoding the type of the variable is probably very undesirable. It’s too easy to accidentally change the type of the variable and forget to update the suppression. It adds another layer of complexity on an already super error-prone construct.

You might be able to get away with some sort of typeof - assuming it does not propagate your type attribute, otherwise it’s kind of pointless.

Or you could try developing a statement attribute variant of __attribute__((wraps)), so that it suppressed all overflow checks in a given statement:

wrapping_int a = INT_MAX;
int b;

__attribute__((wraps)) b = a + 1;

(I’m also very much open to adding support for __attribute__((suppress)) in UBSan.)

Yes, folks other than kernel (and compiler) developers may well be the target market for this proposal. When doing static analysis at companies with lower standards, I’ve come across a lot of code which uses unsigneds improperly, basically as a signed integer not expected to be negative. This fundamental misunderstanding of unsigneds has led to security vulnerabilities, some in financial code. I daresay many (perhaps most) normal developers should be required to use a signaling version of unsigned until they explicitly opt in to a version in which (unlike actual integers, whose fundamental nature is that there is always a new successor) wrapping is actually the required behavior.

Hi,

I’ve opened up a PR and I tried to CC the folks who interacted with this RFC.

Thanks :slight_smile: