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

[NagyDonat]

Previously the security.ArrayBound checker mishandled the following code under non-windows platforms where sizeof(struct Empty) == 0:

struct Empty {};
struct Empty Array[10];
struct Empty foo(void) { return Array[5]; }

The explanation of the false positive was "Access of 'Array' at byte offset 0, while it holds only 0 bytes" -- that is, the checker handled this as the access of a past-the-end pointer, which is usually invalid.

This commit suppresses this false positive by saying that accessing a zero-sized object starting at the past-the-end pointer is valid.

This is implemented by adding an AlsoAcceptEquality flag for checkBounds. This flag will also be useful for implementing checkers that check pointer arithmetic (where forming the past-the-end pointer is completely valid).

This commit also includes a small grammatical fix: the explanation notes now say "0 bytes" or "0 ... elements" instead of "0 byte" / "0 element".

[llvmorg-github-actions]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Donát Nagy (NagyDonat)

Changes

Previously the security.ArrayBound checker mishandled the following code under non-windows platforms where sizeof(struct Empty) == 0:

struct Empty {};
struct Empty Array[10];
struct Empty foo(void) { return Array[5]; }

The explanation of the false positive was "Access of 'Array' at byte offset 0, while it holds only 0 bytes" -- that is, the checker thought that this is access of a past-the-end pointer.

This commit suppresses this false positive by saying that accessing a zero-sized object starting at the past-the-end pointer is valid.

This is implemented by adding an AlsoAcceptEquality flag for checkBounds. This will also be useful when I will write checkers that vaildate pointer arithmetics (where forming the past-the-end pointer is completely valid).

This commit also includes a small grammatical fix: the explanation notes now say "0 bytes" or "0 ... elements" instead of "0 byte" / "0 element".

---

Full diff: https://github.com/llvm/llvm-project/pull/218712.diff

4 Files Affected:

• (modified) clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h (+19-4)
• (modified) clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp (+13-13)
• (modified) clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp (+9-14)
• (modified) clang/test/Analysis/ArrayBound/verbose-tests.c (+35-3)

diff --git a/clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h b/clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h
index 2c8469694b661..e90e3767bfd62 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/BoundsChecking.h
+++ b/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
diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
index c4054ee8cf5c0..3acbf7cd75ab8 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
+++ b/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;
diff --git a/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp b/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp
index 9c11e9e2bd69b..416fa1c4c4d3f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BoundsChecking.cpp
+++ b/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)...
diff --git a/clang/test/Analysis/ArrayBound/verbose-tests.c b/clang/test/Analysis/ArrayBound/verbose-tests.c
index f4619fcc14006..a8aada2f7e8f5 100644
--- a/clang/test/Analysis/ArrayBound/verbose-tests.c
+++ b/clang/test/Analysis/ArrayBound/verbose-tests.c
@@ -475,9 +475,41 @@ struct Empty {};
 struct Empty ZeroSizeElements[10];
 
 struct Empty zeroSizeElements(void) {
-  // FIXME: We probably shouldn't report this access.
-  return ZeroSizeElements[5];
+  // Previously this had produced the false positive warning {{Access of
+  // 'ZeroSizeElements' at byte offset 0, while it holds only 0 bytes}}.
+  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

[steakhal]

I guess you could have defined a struct with an implicit conversion operator to BinaryOperator::Opcode, right? TBH this is completely fine to me. There is value in being explicit by default and this is basically only called once.

[steakhal]

Could you add some tests for C++ and cross check the expectations against codegen?
I think the rules (thus the codegen) is different for C and C++ for empty structs and classes.
I want to be sure that the behavior we implement is aligned with codegen.

[steakhal]

So now we reject incomplete types while we used to gracefully return nullopt in that case. What makes tightening the preconditions safe?

[steakhal]

I don't think there is usually value in mentioning old behavior. Especially not old warning message spellings. Those comments go stale and turn irrelevant really quickly.

I think what we should stress is why we don't expect a warning here.