clang/csa: address review and try to use symbolic values to split state

Author: pskrgag

clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp

@@ -30,6 +30,14 @@ using namespace ento;
 
 namespace {
 
+QualType getSufficientTypeForOverflowOp(CheckerContext &C, const QualType &T) {
+  assert(T->isIntegerType());
+  ASTContext &ACtx = C.getASTContext();
+
+  unsigned BitWidth = ACtx.getIntWidth(T);
+  return ACtx.getIntTypeForBitwidth(BitWidth * 2, T->isSignedIntegerType());
+}
+
 QualType getOverflowBuiltinResultType(const CallEvent &Call) {
   assert(Call.getNumArgs() == 3);
 
@@ -73,15 +81,18 @@ QualType getOverflowBuiltinResultType(const CallEvent &Call, CheckerContext &C,
     return getOverflowBuiltinResultType(Call);
   default:
     assert(false && "Unknown overflow builtin");
+    return Ast.IntTy;
   }
 }
 
 class BuiltinFunctionChecker : public Checker<eval::Call> {
 public:
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
-  void HandleOverflowBuiltin(const CallEvent &Call, CheckerContext &C,
+  void handleOverflowBuiltin(const CallEvent &Call, CheckerContext &C,
                              BinaryOperator::Opcode Op,
                              QualType ResultType) const;
+  std::pair<bool, bool> checkOverflow(CheckerContext &C, SVal RetVal,
+                                      QualType Res) const;
 
 private:
   // From: clang/include/clang/Basic/Builtins.def
@@ -101,7 +112,36 @@ class BuiltinFunctionChecker : public Checker<eval::Call> {
 
 } // namespace
 
-void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+std::pair<bool, bool>
+BuiltinFunctionChecker::checkOverflow(CheckerContext &C, SVal RetVal,
+                                      QualType Res) const {
+  ProgramStateRef State = C.getState();
+  SValBuilder &SVB = C.getSValBuilder();
+  auto SvalToBool = [&](SVal val) {
+    return State->isNonNull(val).isConstrainedTrue();
+  };
+  ASTContext &ACtx = C.getASTContext();
+
+  assert(Res->isIntegerType());
+
+  unsigned BitWidth = ACtx.getIntWidth(Res);
+  SVal MinType = nonloc::ConcreteInt(
+      llvm::APSInt::getMinValue(BitWidth, Res->isUnsignedIntegerType()));
+  SVal MaxType = nonloc::ConcreteInt(
+      llvm::APSInt::getMaxValue(BitWidth, Res->isUnsignedIntegerType()));
+
+  bool IsGreaterMax =
+      SvalToBool(SVB.evalBinOp(State, BO_GT, RetVal, MaxType, Res));
+  bool IsLessMin =
+      SvalToBool(SVB.evalBinOp(State, BO_LT, RetVal, MinType, Res));
+
+  bool IsLeMax = SvalToBool(SVB.evalBinOp(State, BO_LE, RetVal, MaxType, Res));
+  bool IsGeMin = SvalToBool(SVB.evalBinOp(State, BO_GE, RetVal, MinType, Res));
+
+  return {IsGreaterMax || IsLessMin, IsLeMax && IsGeMin};
+}
+
+void BuiltinFunctionChecker::handleOverflowBuiltin(const CallEvent &Call,
                                                    CheckerContext &C,
                                                    BinaryOperator::Opcode Op,
                                                    QualType ResultType) const {
@@ -115,14 +155,12 @@ void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
   SVal Arg1 = Call.getArgSVal(0);
   SVal Arg2 = Call.getArgSVal(1);
 
+  SVal RetValMax = SVB.evalBinOp(State, Op, Arg1, Arg2,
+                                 getSufficientTypeForOverflowOp(C, ResultType));
   SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
 
-  // TODO: Handle overflows with values that known to overflow. Like INT_MAX + 1
-  // should not produce state for non-overflow case and threat it as
-  // unreachable.
-
-  // Handle non-overflow case.
-  {
+  auto [Overflow, NotOverflow] = checkOverflow(C, RetValMax, ResultType);
+  if (NotOverflow) {
     ProgramStateRef StateSuccess =
         State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(false));
 
@@ -132,11 +170,9 @@ void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
     C.addTransition(StateSuccess);
   }
 
-  // Handle overflow case.
-  {
+  if (Overflow)
     C.addTransition(
         State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(true)));
-  }
 }
 
 bool BuiltinFunctionChecker::isBuiltinLikeFunction(
@@ -183,7 +219,7 @@ bool BuiltinFunctionChecker::evalCall(const CallEvent &Call,
   case Builtin::BI__builtin_umul_overflow:
   case Builtin::BI__builtin_umull_overflow:
   case Builtin::BI__builtin_umulll_overflow:
-    HandleOverflowBuiltin(Call, C, BO_Mul,
+    handleOverflowBuiltin(Call, C, BO_Mul,
                           getOverflowBuiltinResultType(Call, C, BI));
     return true;
   case Builtin::BI__builtin_sub_overflow:
@@ -193,7 +229,7 @@ bool BuiltinFunctionChecker::evalCall(const CallEvent &Call,
   case Builtin::BI__builtin_usub_overflow:
   case Builtin::BI__builtin_usubl_overflow:
   case Builtin::BI__builtin_usubll_overflow:
-    HandleOverflowBuiltin(Call, C, BO_Sub,
+    handleOverflowBuiltin(Call, C, BO_Sub,
                           getOverflowBuiltinResultType(Call, C, BI));
     return true;
   case Builtin::BI__builtin_add_overflow:
@@ -203,7 +239,7 @@ bool BuiltinFunctionChecker::evalCall(const CallEvent &Call,
   case Builtin::BI__builtin_uadd_overflow:
   case Builtin::BI__builtin_uaddl_overflow:
   case Builtin::BI__builtin_uaddll_overflow:
-    HandleOverflowBuiltin(Call, C, BO_Add,
+    handleOverflowBuiltin(Call, C, BO_Add,
                           getOverflowBuiltinResultType(Call, C, BI));
     return true;
   case Builtin::BI__builtin_assume:

clang/test/Analysis/builtin_overflow.c

@@ -1,65 +1,83 @@
 // RUN: %clang_analyze_cc1 -triple x86_64-unknown-unknown -verify %s \
 // RUN:   -analyzer-checker=core,debug.ExprInspection
 
-#define NULL ((void *)0)
-#define INT_MAX __INT_MAX__
+#define __UINT_MAX__ (__INT_MAX__  * 2U + 1U)
 
 void clang_analyzer_dump_int(int);
+void clang_analyzer_eval(int);
+void clang_analyzer_warnIfReached(void);
 
-void test1(void)
+void test_add_nooverflow(void)
 {
    int res;
 
    if (__builtin_add_overflow(10, 20, &res)) {
-     clang_analyzer_dump_int(res); //expected-warning{{1st function call argument is an uninitialized value}}
+     clang_analyzer_warnIfReached();
      return;
    }
 
-   clang_analyzer_dump_int(res); //expected-warning{{30}}
+   clang_analyzer_dump_int(res); //expected-warning{{30 S32b}}
 }
 
-void test2(void)
+void test_add_overflow(void)
 {
    int res;
 
-   __builtin_add_overflow(10, 20, &res);
-   clang_analyzer_dump_int(res); //expected-warning{{1st function call argument is an uninitialized value}} expected-warning{{S32b}}
+   if (__builtin_add_overflow(__INT_MAX__, 1, &res)) {
+     clang_analyzer_dump_int(res); //expected-warning{{1st function call argument is an uninitialized value}}
+     return;
+   }
+
+   clang_analyzer_warnIfReached();
 }
 
-void test3(void)
+void test_add_overflow_contraints(int a, int b)
 {
    int res;
 
-   if (__builtin_sub_overflow(10, 20, &res)) {
-     clang_analyzer_dump_int(res); //expected-warning{{1st function call argument is an uninitialized value}}
+   if (a != 10)
+     return;
+   if (b != 0)
+     return;
+
+   if (__builtin_add_overflow(a, b, &res)) {
+     clang_analyzer_warnIfReached();
      return;
    }
 
-   clang_analyzer_dump_int(res); //expected-warning{{-10}}
+   clang_analyzer_dump_int(res); //expected-warning{{10 S32b}}
 }
 
-void test4(void)
+void test_uaddll_overflow_contraints(unsigned long a, unsigned long b)
 {
-   int res;
+   unsigned long long res;
 
-   if (__builtin_sub_overflow(10, 20, &res)) {
-     clang_analyzer_dump_int(res); //expected-warning{{1st function call argument is an uninitialized value}}
+   if (a != 10)
+     return;
+   if (b != 10)
      return;
-   }
 
-   if (res != -10) {
-     *(volatile char *)NULL; //no warning
+   if (__builtin_uaddll_overflow(a, b, &res)) {
+     clang_analyzer_warnIfReached();
+     return;
    }
 }
 
-void test5(void)
+void test_uadd_overflow_contraints(unsigned a, unsigned b)
 {
-   int res;
+   unsigned res;
 
-   if (__builtin_mul_overflow(10, 20, &res)) {
-     clang_analyzer_dump_int(res); //expected-warning{{1st function call argument is an uninitialized value}}
+   if (a > 10)
      return;
-   }
+   if (b > 10)
+     return;
+
+   // clang_analyzer_eval(a + b < 30); <--- Prints 1 and 0, but why ???
 
-   clang_analyzer_dump_int(res); //expected-warning{{200}}
+   if (__builtin_uadd_overflow(a, b, &res)) {
+     /* clang_analyzer_warnIfReached(); */
+     return;
+   }
 }
+
+// TODO: more tests after figuring out what's going on.

clang/test/Analysis/out-of-bounds-diagnostics.c

@@ -280,14 +280,12 @@ int *mallocRegion(void) {
 
 int *custom_calloc(size_t a, size_t b) {
   size_t res;
-  if (__builtin_mul_overflow(a, b, &res))
-    return 0;
 
-  return malloc(res);
+  return __builtin_mul_overflow(a, b, &res) ? 0 : malloc(res);
 }
 
 int *mallocRegionOverflow(void) {
-  int *mem = (int*)custom_calloc(4, 10);
+  int *mem = (int*)custom_calloc(10, sizeof(int));
 
   mem[20] = 10;
   // expected-warning@-1 {{Out of bound access to memory after the end of the heap area}}