On Tue, Aug 04, 2020 at 08:11:45AM +0200, Rasmus Villemoes wrote: > On 03/08/2020 20.29, Kees Cook wrote: > > Hi, > > > > I wonder if we should explicitly saturate the output of the overflow > > helpers as a side-effect of overflow detection? > > Please no.
I'm entirely on the fence about this, so I'm fine with that answer. (And I see your PS about why -- thanks!) > > (That way the output > > is never available with a "bad" value, if the caller fails to check the > > result or forgets that *d was written...) since right now, *d will hold > > the wrapped value. > > Exactly. I designed the fallback ones so they would have the same > semantics as when using gcc's __builtin_* - though with the "all > operands have same type" restriction, since it would be completely > unwieldy to handle stuff like (s8) + (u64) -> (s32) in macros. Right -- a totally sane requirement. :) > > > Also, if we enable arithmetic overflow detection sanitizers, we're going > > to trip over the fallback implementation (since it'll wrap and then do > > the overflow test in the macro). > > Huh? The fallback code only ever uses unsigned arithmetic, precisely to > avoid triggering such warnings. Or are you saying there are some > sanitizers out there which also warn for, say, (~0u) + 1u? Yes, > detecting overflow/underflow for a (s32)-(s32)->(s32) without relying on > -fwrapv is a bit messy, but it's done and AFAIK works just fine even > with UBSAN enabled. GCC only has a signed overflow sanitizer. Clang has signed and unsigned. Dealing with -fwrapv is yet another exercise. And I can solve this differently, too, with a static inline helper that does basic mul and carries a no-sanitize attribute. > What we might do, to deal with the "caller fails to check the result", > is to add a > > static inline bool __must_check must_check_overflow(bool b) { return > unlikely(b); } > > and wrap all the final "did it overflow" results in that one - perhaps > also for the __builtin_* cases, I don't know if those are automatically > equipped with that attribute. [I also don't know if gcc propagates > likely/unlikely out to the caller, but it shouldn't hurt to have it > there and might improve code gen if it does.] (What is the formal name for the ({ ...; return_value; }) C construct?) Will that work as a macro return value? If so, that's extremely useful. > PS: Another reason not to saturate is that there are two extreme values, > and choosing between them makes the code very messy (especially when > using the __builtins). 5u-10u should saturate to 0u, not UINT_MAX, and > even for for underflowing a signed computation like INT_MIN + (-7); it > makes no sense for that to saturate to INT_MAX. Ah, gotcha. -- Kees Cook