Hey Mirko
On 10.11.21 11:07, Mirko Brodesser wrote:
>
> I don't know, how much static analysis currently helps to detect
> overflows of signed types, which would be one benefit of using signed
> instead of unsigned types. Does anyone know?
>
Static analysis is not very good in accurately detecting these issues,
but we do have dynamic analysis though UndefinedBehaviorSanitizer [1]
available to detect both types of overflows. The primary difference
between signed and unsigned here is that signed overflow is actual
undefined behavior and considered harmful in certain situations (the
compiler can perform optimizations and remove code based on the
assumption that signed types will not overflow).
One problem in our codebase is that we have a lot of old code that makes
excessive use of signed overflow where the results don't matter, which
makes deploying this kind of analysis challenging. I believe we are not
running this anywhere right now, despite the fact that we have an
extensive suppression list [2] and build system support for it [3]. I'll
actually go and figure out if we are planning to deploy this at any
point and get back to you when I know more about that.
For new code, if possible we should use `CheckedInt` which has a
diagnostic assert that indicates unchecked overflows. If that is not
possible for some reason and a variable is not supposed to contain
negative values (and not supposed to overflow), then from a memory
safety standpoint, it might be better to have it signed (if the variable
flows into a size parameter, it will be too large rather than too
small), but it really depends on the underlying code I guess. The UB
occurring might have negative side effects too, but that at least makes
the bug detectable, while we have very little chance of finding
unintended unsigned overflows unless they crash in some way.
For the situation above we should also always investigate why
`CheckedInt` wouldn't be applicable.
- Chris
[1]
https://releases.llvm.org/13.0.0/tools/clang/docs/UndefinedBehaviorSanitizer.html
[2]
https://searchfox.org/mozilla-central/rev/a48e21143960b383004afa9ff9411c5cf6d5a958/build/moz.configure/toolchain.configure#1832
[3]
https://searchfox.org/mozilla-central/rev/a48e21143960b383004afa9ff9411c5cf6d5a958/build/sanitizers/ubsan_signed_overflow_blacklist.txt