[PATCH] D81061: [Analyzer][VLASizeChecker] Fix problem with zero index assumption.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 272361.
balazske added a comment.

- Rebase
- Using condition type for `evalBinOp`
- Comments updated


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81061/new/

https://reviews.llvm.org/D81061

Files:
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/test/Analysis/vla.c


Index: clang/test/Analysis/vla.c
===================================================================
--- clang/test/Analysis/vla.c
+++ clang/test/Analysis/vla.c
@@ -139,7 +139,7 @@
 }
 
 // https://bugs.llvm.org/show_bug.cgi?id=46128
-// analyzer doesn't handle more than simple symbolic expressions.
+// Analyzer doesn't handle more than simple symbolic expressions correct.
 // Just don't crash.
 extern void foo(void);
 int a;
Index: clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
@@ -122,16 +122,19 @@
       return State;
 
     if (const llvm::APSInt *IndexLVal = SVB.getKnownValue(State, IndexLength)) {
+      uint64_t IndexL = IndexLVal->getZExtValue();
+      if (IndexL == 0) {
+        // Despite the previous assumption for positive size, value returned by
+        // getKnownValue could be zero. Double-check for zero here.
+        // See https://reviews.llvm.org/D80903 for discussion of this problem.
+        // This is just a safety check, the case could not be reproduced with
+        // the current code.
+        reportBug(VLA_Zero, SizeE, State, C);
+        return nullptr;
+      }
       // Check if the array size will overflow.
       // Size overflow check does not work with symbolic expressions because a
       // overflow situation can not be detected easily.
-      uint64_t IndexL = IndexLVal->getZExtValue();
-      // FIXME: See https://reviews.llvm.org/D80903 for discussion of
-      // some difference in assume and getKnownValue that leads to
-      // unexpected behavior. Just bail on IndexL == 0 at this point.
-      if (IndexL == 0)
-        return nullptr;
-
       if (KnownSize <= SizeMax / IndexL) {
         KnownSize *= IndexL;
       } else {
@@ -171,35 +174,22 @@
     return nullptr;
   }
 
-  // Check if the size is zero.
+  QualType SizeTy = SizeE->getType();
   DefinedSVal SizeD = SizeV.castAs<DefinedSVal>();
-
-  ProgramStateRef StateNotZero, StateZero;
-  std::tie(StateNotZero, StateZero) = State->assume(SizeD);
-
-  if (StateZero && !StateNotZero) {
-    reportBug(VLA_Zero, SizeE, StateZero, C);
-    return nullptr;
-  }
-
-  // From this point on, assume that the size is not zero.
-  State = StateNotZero;
-
-  // Check if the size is negative.
   SValBuilder &SVB = C.getSValBuilder();
-
-  QualType SizeTy = SizeE->getType();
   DefinedOrUnknownSVal Zero = SVB.makeZeroVal(SizeTy);
 
-  SVal LessThanZeroVal = SVB.evalBinOp(State, BO_LT, SizeD, Zero, SizeTy);
-  if (Optional<DefinedSVal> LessThanZeroDVal =
-          LessThanZeroVal.getAs<DefinedSVal>()) {
-    ConstraintManager &CM = C.getConstraintManager();
-    ProgramStateRef StatePos, StateNeg;
-
-    std::tie(StateNeg, StatePos) = CM.assumeDual(State, *LessThanZeroDVal);
-    if (StateNeg && !StatePos) {
-      reportBug(VLA_Negative, SizeE, State, C);
+  // Check if the size is zero or negative.
+  SVal PositiveVal =
+      SVB.evalBinOp(State, BO_GT, SizeD, Zero, SVB.getConditionType());
+  if (Optional<DefinedSVal> PositiveDVal = PositiveVal.getAs<DefinedSVal>()) {
+    ProgramStateRef StatePos, StateNotPos;
+
+    std::tie(StatePos, StateNotPos) = State->assume(*PositiveDVal);
+    if (StateNotPos && !StatePos) {
+      ConditionTruthVal IsZeroSize = StateNotPos->isNull(SizeD);
+      reportBug(IsZeroSize.isConstrainedTrue() ? VLA_Zero : VLA_Negative, SizeE,
+                State, C);
       return nullptr;
     }
     State = StatePos;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D81061.272361.patch
Type: text/x-patch
Size: 3580 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200622/b14b713c/attachment-0001.bin>