[analyzer] Fix handling of zero-sized elements in ArrayBound

clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h

@@ -29,6 +29,7 @@ namespace clang::ento::bounds {
 struct CheckFlags {
   unsigned CheckUnderflow : 1;
   unsigned OffsetObviouslyNonnegative : 1;
+  unsigned AlsoAcceptEquality : 1;
 };
 
 class CheckResult;
@@ -91,16 +92,30 @@ class CheckResult {
   ProgramStateRef InBoundsState = nullptr;
 };
 
-// Evaluate the comparison Value < Threshold with the help of the custom
+enum class Comparison { LT, LE, EQ };
+
+inline BinaryOperator::Opcode asOpcode(Comparison C) {
+  switch (C) {
+  case Comparison::LT:
+    return BO_LT;
+  case Comparison::LE:
+    return BO_LE;
+  case Comparison::EQ:
+    return BO_EQ;
+  }
+  llvm_unreachable("unhandled Comparison kind");
+}
+
+// Evaluate the comparison \p Value < \p Threshold with the help of the custom
 // simplification algorithm. Return a pair of states, where the first one
 // corresponds to "value below threshold" and the second corresponds to "value
 // at or above threshold". Returns {nullptr, nullptr} in the case when the
 // evaluation fails.
-// If the optional argument CheckEquality is true, then use BO_EQ instead of
-// the default BO_LT after consistently applying the same simplification steps.
+// If the optional argument \p CmpKind is specified, then that comparison
+// operator is used (instead of the default '<') after the same simplification
 std::pair<ProgramStateRef, ProgramStateRef>
 compareValueToThreshold(ProgramStateRef State, SValBuilder &SVB, NonLoc Value,
-                        NonLoc Threshold, bool CheckEquality = false);
+                        NonLoc Threshold, Comparison CmpKind = Comparison::LT);
 } // namespace clang::ento::bounds
 
 #endif // LLVM_CLANG_STATICANALYZER_CHECKERS_BOUNDSCHECKING_H

clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp

@@ -200,6 +200,14 @@ static bool isDeterminedByInterestingSymbol(SVal SV,
   return false;
 }
 
+static int64_t getElementSize(const ElementRegion *ER, SValBuilder &SVB) {
+  QualType ElemType = ER->getElementType();
+
+  assert(!ElemType->isIncompleteType() && "ElemType cannot be incomplete");
+
+  return SVB.getContext().getTypeSizeInChars(ElemType).getQuantity();
+}
+
 /// For a given \p CurRegion that can be represented as a symbolic expression
 /// Arr[Idx] (or perhaps Arr[Idx1][Idx2] etc.), return the parent memory block
 /// Arr and the distance of Location from the beginning of Arr (expressed in a
@@ -222,17 +230,8 @@ computeOffset(ProgramStateRef State, SValBuilder &SVB,
     if (!Index)
       return std::nullopt;
 
-    QualType ElemType = CurRegion->getElementType();
-
-    // FIXME: The following early return was presumably added to safeguard the
-    // getTypeSizeInChars() call (which doesn't accept an incomplete type), but
-    // it seems that `ElemType` cannot be incomplete at this point.
-    if (ElemType->isIncompleteType())
-      return std::nullopt;
-
     // Calculate Delta = Index * sizeof(ElemType).
-    NonLoc Size = SVB.makeArrayIndex(
-        SVB.getContext().getTypeSizeInChars(ElemType).getQuantity());
+    NonLoc Size = SVB.makeArrayIndex(getElementSize(CurRegion, SVB));
     auto Delta = EvalBinOp(BO_Mul, *Index, Size);
     if (!Delta)
       return std::nullopt;
@@ -324,7 +323,7 @@ static BugDescription describeInvalidAccess(bounds::CheckResult Res,
 
     Out << ' ' << SU.asElementName();
 
-    if (*ExtentN > 1)
+    if (*ExtentN != 1)
       Out << "s";
   }
 
@@ -452,7 +451,8 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E,
   bounds::CheckFlags Flags = {
       /*CheckUnderflow=*/!(isa<SymbolicRegion>(Reg) &&
                            isa<UnknownSpaceRegion>(Space)),
-      /*OffsetObviouslyNonnegative=*/isOffsetObviouslyNonnegative(E, C)};
+      /*OffsetObviouslyNonnegative=*/isOffsetObviouslyNonnegative(E, C),
+      /*AlsoAcceptEquality=*/(getElementSize(AccessedER, SVB) == 0)};
 
   bounds::CheckResult Res = checkBounds(State, SVB, ByteOffset, Extent, Flags);
 
@@ -472,7 +472,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E,
         // forms the past-the-end pointer without actually dereferencing it.
         auto [EqualsToThreshold, NotEqualToThreshold] =
             bounds::compareValueToThreshold(State, SVB, ByteOffset, *Extent,
-                                            /*CheckEquality=*/true);
+                                            bounds::Comparison::EQ);
         if (EqualsToThreshold && !NotEqualToThreshold) {
           C.addTransition(EqualsToThreshold);
           return;

clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp

@@ -77,7 +77,7 @@ static bool isUnsigned(SValBuilder &SVB, NonLoc Value) {
 std::pair<ProgramStateRef, ProgramStateRef>
 bounds::compareValueToThreshold(ProgramStateRef State, SValBuilder &SVB,
                                 NonLoc Value, NonLoc Threshold,
-                                bool CheckEquality) {
+                                Comparison CmpKind) {
   if (auto ConcreteThreshold = Threshold.getAs<nonloc::ConcreteInt>()) {
     std::tie(Value, Threshold) =
         getSimplifiedOffsets(Value, *ConcreteThreshold, SVB);
@@ -91,16 +91,16 @@ bounds::compareValueToThreshold(ProgramStateRef State, SValBuilder &SVB,
   // To avoid automatic conversions, we evaluate the "obvious" cases without
   // calling `evalBinOpNN`:
   if (isNegative(SVB, State, Value) && isUnsigned(SVB, Threshold)) {
-    if (CheckEquality) {
-      // negative_value == unsigned_threshold is always false
+    if (CmpKind == Comparison::EQ) {
+      // negative == unsigned is always false
       return {nullptr, State};
     }
-    // negative_value < unsigned_threshold is always true
+    // negative < unsigned and negative <= unsigned are always true
     return {State, nullptr};
   }
   if (isUnsigned(SVB, Value) && isNegative(SVB, State, Threshold)) {
-    // unsigned_value == negative_threshold and
-    // unsigned_value < negative_threshold are both always false
+    // unsigned == negative, unsigned < negative and unsigned <= negative are
+    // all always false
     return {nullptr, State};
   }
   // FIXME: These special cases are sufficient for handling real-world
@@ -114,7 +114,7 @@ bounds::compareValueToThreshold(ProgramStateRef State, SValBuilder &SVB,
   // evaluate these "mathematical" comparisons through a separate pathway would
   // be a step backwards in this sense.
 
-  const BinaryOperatorKind OpKind = CheckEquality ? BO_EQ : BO_LT;
+  const BinaryOperatorKind OpKind = asOpcode(CmpKind);
   auto BelowThreshold =
       SVB.evalBinOpNN(State, OpKind, Value, Threshold, SVB.getConditionType())
           .getAs<NonLoc>();
@@ -183,14 +183,9 @@ bounds::CheckResult bounds::checkBounds(ProgramStateRef State, SValBuilder &SVB,
 
   // CHECK UPPER BOUND
   if (Extent) {
-    // In a situation where both underflow and overflow are possible (but the
-    // index is either tainted or known to be invalid), the logic of this
-    // checker will first assume that the offset is non-negative, and then
-    // (with this additional assumption) it will detect an overflow error.
-    // In this situation the warning message should mention both possibilities.
-
+    Comparison CK = Flags.AlsoAcceptEquality ? Comparison::LE : Comparison::LT;
     auto [WithinUpperBound, ExceedsUpperBound] =
-        compareValueToThreshold(State, SVB, Offset, *Extent);
+        compareValueToThreshold(State, SVB, Offset, *Extent, /*CmpKind=*/CK);
 
     if (ExceedsUpperBound) {
       // The offset may be invalid (>= Size)...

clang/test/Analysis/ArrayBound/verbose-tests.c

@@ -470,14 +470,48 @@ int *nothingIsCertain(int x, int y) {
 // We disable this test under Windows because 'struct Empty {}' has a nozero
 // size on that platform. Note that '_WIN32' is also defined on 64-bit systems
 // and is apparently the customary way to detect Windows OS.
+// The empty struct also has nonzero size under C++ so these corner cases are
+// only relevant under C.
 
 struct Empty {};
 struct Empty ZeroSizeElements[10];
 
 struct Empty zeroSizeElements(void) {
-  // FIXME: We probably shouldn't report this access.
-  return ZeroSizeElements[5];
+  // Here the offset and extent are both 0, which previously caused a false
+  // positive out of bounds report.
+  return ZeroSizeElements[5]; // no-warning
+}
+
+struct Empty zeroSizeElementsNegativeIndex(void) {
+  // The negative index does not change anything, it still means offset = 0.
+  return ZeroSizeElements[-5]; // no-warning
+}
+
+int zeroSizeContainerIntAccess(void) {
+  return ((int*)ZeroSizeElements)[5];
   // expected-warning@-1 {{Out of bound access to memory after the end of 'ZeroSizeElements'}}
-  // expected-note@-2 {{Access of 'ZeroSizeElements' at byte offset 0, while it holds only 0 byte}}
+  // expected-note@-2 {{Access of 'ZeroSizeElements' at index 5, while it holds only 0 'int' elements}}
 }
+
+struct Empty zeroSizeAccessOfPastTheEnd(void) {
+  // We currently allow zero-sized access of past-the-end pointers as a side
+  // effect of the logic that handles the testcase 'zeroSizeElements'.
+  return *(struct Empty *)(TenElements + 10); // no-warning
+}
+
+struct Empty zeroSizeAccessFarAway(void) {
+  // However, zero-sized access of other out-of-bounds pointers is reported
+  // (with byte offsets, because the zero-sized element is not suitable for
+  // calculating indices).
+  return *(struct Empty *)(TenElements + 20);
+  // expected-warning@-1 {{Out of bound access to memory after the end of 'TenElements'}}
+  // expected-note@-2 {{Access of 'TenElements' at byte offset 80, while it holds only 40 bytes}}
+}
+
+struct Empty zeroSizeAccessUnderflow(void) {
+  return *(struct Empty *)(TenElements - 10);
+  // expected-warning@-1 {{Out of bound access to memory preceding 'TenElements'}}
+  // expected-note@-2 {{Access of 'TenElements' at negative byte offset -40}}
+}
+
 #endif