[PATCH] D48958: [clang][ubsan] Implicit Cast Sanitizer - integer truncation - clang part

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri updated this revision to Diff 154591.
lebedev.ri retitled this revision from "[private][clang] Implicit Cast Sanitizer - integer truncation  - clang part" to "[clang][ubsan] Implicit Cast Sanitizer - integer truncation  - clang part".
lebedev.ri edited the summary of this revision.
lebedev.ri added reviewers: rjmccall, rsmith, samsonov, pcc, vsk, eugenis, efriedma, kcc.
lebedev.ri added a project: Sanitizers.
lebedev.ri added subscribers: cfe-commits, mclow.lists, milianw, dvyukov, ygribov, danielaustin, filcab, dtzWill, RKSimon, aaron.ballman, Sanitizers.
lebedev.ri added a comment.

C and C++ are interesting languages. They are statically typed, but weakly.
The implicit conversions are allowed. This is nice, allows to write code
while balancing between getting drowned in everything being convertible,
and nothing being convertible. As usual, this comes with a price:

  unsigned char store = 0;
  
  bool consume(unsigned int val);
  
  void test(unsigned long val) {
    if (consume(val)) {
      // the 'val' is `unsigned long`, but `consume()` takes `unsigned int`.
      // If their bit widths are different on this platform, the implicit
      // truncation happens. And if that `unsigned long` had a value bigger
      // than UINT_MAX, then you may or may not have a bug.
  
      // Similarly, integer addition happens on `int`s, so `store` will
      // be promoted to an `int`, the sum calculated (0+768=768),
      // and the result demoted to `unsigned char`, and stored to `store`.
      // In this case, the `store` will still be 0. Again, not always intended.
      store = store + 768; // before addition, 'store' was promoted to int.
    }
  
    // But yes, sometimes this is intentional.
    // You can either make the cast explicit
    (void)consume((unsigned int)val);
    // or mask the value so no bits will be *implicitly* lost.
    (void)consume((~((unsigned int)0)) & val);
  }

Yes, there is a `-Wconversion`` diagnostic group, but first, it is kinda
noisy, since it warns on everything (unlike sanitizers, warning on an
actual issues), and second, there are cases where it does **not** warn.
So a Sanitizer is needed. I don't have any motivational numbers, but i know
i had this kind of problem 10-20 times, and it was never easy to track down.

The logic to detect whether an truncation has happened is pretty simple
if you think about it - https://godbolt.org/g/NEzXbb - basically, just
extend (using the new, not original!, signedness) the 'truncated' value
back to it's original width, and equality-compare it with the original value.

The most non-trivial thing here is the logic to detect whether this
`ImplicitCastExpr` AST node is **actually** an implicit cast, //or// part of
an explicit cast. Because the explicit casts are modeled as an outer
`ExplicitCastExpr` with some `ImplicitCastExpr`'s as **direct** children.
https://godbolt.org/g/eE1GkJ

It would //seem//, we can just check that the current `ImplicitCastExpr`
we are processing either has no `CastExpr` parents, or all of them are
`ImplicitCastExpr`.

As you may have noted, this isn't just named `-fsanitize=implicit-integer-truncation`.
There are potentially some more implicit casts to be warned about.
Namely, implicit casts that result in sign change; implicit cast
between different floating point types, or between fp and an integer,
when again, that conversion is lossy.

I suspect, there may be some false-negatives, cases yet to be handled.

This is a clang part.
The compiler-rt part is https://reviews.llvm.org/D48959.

Fixes PR21530 <https://bugs.llvm.org/show_bug.cgi?id=21530>, PR37552 <https://bugs.llvm.org/show_bug.cgi?id=37552>, PR35409 <https://bugs.llvm.org/show_bug.cgi?id=35409>.
Partially fixes PR9821 <https://bugs.llvm.org/show_bug.cgi?id=9821>.
Fixes https://github.com/google/sanitizers/issues/940.


Repository:
  rC Clang

https://reviews.llvm.org/D48958

Files:
  docs/ReleaseNotes.rst
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  lib/CodeGen/CGExprScalar.cpp
  lib/CodeGen/CodeGenFunction.h
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  test/CodeGen/catch-implicit-integer-truncations.c
  test/CodeGenCXX/catch-implicit-integer-truncations.cpp
  test/Driver/fsanitize.c

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D48958.154591.patch
Type: text/x-patch
Size: 42273 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180709/a1168131/attachment-0001.bin>