[cfe-dev] [PATCH] Integer Sanitizer Initial Patches

[Richard Smith]

Hi,

On Wed, Nov 7, 2012 at 5:15 PM, Will Dietz <willdtz at gmail.com> wrote:
> Attached are patches that add a new 'sanitizer' to clang for detecting
> and reporting integer overfllows.  Unlike the checks added by
> -fcatch-undefined-behavior, these also include non-undefined-behavior
> checks.

This seems like it could be valuable to me, and I think it's in scope
as a -fsanitize= feature (there are some other checks which I'd like
to eventually include in -fsanitize=, which check for possible bugs
which don't result in undefined behavior). For instance, I could
imagine this being something people would turn on when their code is
behaving strangely, as part of a "tell me about suspicious things that
my program did" mode, but that would depend on making these
diagnostics non-fatal (and maybe supporting a suppression system).

Assuming we reach consensus that we want this...

> The attached clang patch adds:
>
> -fsanitize=unsigned-integer-overflow
> and
> -fsanitize=integer
>
> The first adds support for inserting checks for unsigned integer
> overflow, the latter is a new 'sanitizer group' which is used to
> enable all integer-related checking.  In the future I'd like to
> include value-losing conversions, but for now this includes the
> existing checks (signed overflow, divide-by-zero, shifts) as well as
> the new unsigned overflow checks.

Please split the divide-by-zero check into integer and floating-point
cases, and only include the integer case in the -fsanitize=integer
group.

Please also provide a patch to the user's manual documenting the new arguments.

> Also attached is a corresponding patch for compiler-rt that extends
> ubsan to include support for reporting unsigned as well as signed
> overflows.
>
> Two issues with this that I'm hoping can be discussed:
> * As per PR14247 (http://llvm.org/bugs/show_bug.cgi?id=14247), the
> ubsan checks presently aren't recoverable.  This reduces these checks'
> utility for quickly getting a new large codebase into shape as
> mentioned in that bug, but this is of course even more important to be
> made optional when reporting unsigned overflows is enabled as well.

I think this is something we should pursue. My only reservation here
is a concern about the performance impact of making the checks
recoverable, but I have no data there.

> * Extending "ubsan" is unfortunate given its name, but these checks
> don't seem to merit a separate library either.  Thoughts?

I don't think that's a problem. "One Hour Photo" is just the name of
the shop, sir. :)


Clang patch:

--- a/lib/CodeGen/CGExprScalar.cpp
+++ b/lib/CodeGen/CGExprScalar.cpp
@@ -414,6 +414,11 @@ public:
       }
     }

+    if (Ops.Ty->isUnsignedIntegerType() &&
+        CGF.getLangOpts().SanitizeUnsignedIntegerOverflow) {
+      return EmitOverflowCheckedBinOp(Ops);
+    }

No braces here, please (and for this same construct later in the file).


   case BO_Mul:
   case BO_MulAssign:
     OpID = 3;
     IID = llvm::Intrinsic::smul_with_overflow;
+    IID = isSigned ? llvm::Intrinsic::smul_with_overflow :
+                     llvm::Intrinsic::umul_with_overflow;
     break;

There's a dead store left behind here.


@@ -2031,7 +2054,8 @@ Value
*ScalarExprEmitter::EmitOverflowCheckedBinOp(const BinOpInfo &Ops) {
   if (handlerName->empty()) {
     // If the signed-integer-overflow sanitizer is enabled, emit a call to its
     // runtime. Otherwise, this is a -ftrapv check, so just emit a trap.
-    if (CGF.getLangOpts().SanitizeSignedIntegerOverflow)
+    if (CGF.getLangOpts().SanitizeSignedIntegerOverflow ||
+        CGF.getLangOpts().SanitizeUnsignedIntegerOverflow)
       EmitBinOpCheck(Builder.CreateNot(overflow), Ops);
     else
       CGF.EmitTrapvCheck(Builder.CreateNot(overflow));

This doesn't look right -- building with -ftrapv
-fsanitize=unsigned-integer-overflow will use the -fsanitize path for
signed overflow, not the -ftrapv path.

The Clang patch should include a test -- it's not sufficient for this
to be tested just in the compiler-rt tests (they're not run as a part
of normal Clang development).


compiler-rt patch:

--- a/lib/ubsan/ubsan_handlers.cc
+++ b/lib/ubsan/ubsan_handlers.cc
@@ -55,30 +55,35 @@ void
__ubsan::__ubsan_handle_type_mismatch(TypeMismatchData *Data,
   Die();
 }

-/// \brief Common diagnostic emission for various forms of signed overflow.
-template<typename T> static void HandleSignedOverflow(OverflowData *Data,
+/// \brief Common diagnostic emission for various forms of integer overflow.
+template<typename T> static void HandleIntegerOverflow(OverflowData *Data,
                                                       ValueHandle LHS,
                                                       const char *Operator,
-                                                      T RHS) {
-  Diag(Data->Loc, "signed integer overflow: "
-                  "%0 %1 %2 cannot be represented in type %3")
+                                                      T RHS,
+                                                      bool isSigned) {
+  Diag(Data->Loc, "%0 integer overflow: "
+                  "%1 %2 %3 cannot be represented in type %4")
+    << (isSigned ? "signed" : "unsigned")

Please look at Data->Type.isSignedIntegerTy() in here, rather than
passing in an extra flag.