[clang] [NFC][analyzer] Clarify that ArrayBound only handles element regions (PR #216049)

[via cfe-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

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

Author: Donát Nagy (NagyDonat)

<details>
<summary>Changes</summary>

The `security.ArrayBound` checker does not do anything when the value of the expression that activated its callbacks isn't expressed as an `ElementRegion`.

Previously this was a sneaky consequence of the logic in `computeOffset`, but this commit highlights it with a straightforward early return statement to ensure that the accessed element region is available within the body of `ArrayBoundChecker::handleAccessExpr`.

(I intend to use this in a follow-up commit.)

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


1 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp (+13-13) 


``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
index 9e4e8e83749ad..5201eeddca16f 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
@@ -75,11 +75,10 @@ class SizeUnit {
   bool isBytes() const { return AsType.isNull(); }
 
   /// Return the element type that is "natural" for reporting out-of-bounds
-  /// memory access to 'Location'.
-  static SizeUnit forSVal(SVal Location, const ASTContext &ACtx) {
-    if (const auto *R = Location.getAsRegion()->getAs<TypedValueRegion>())
-      return SizeUnit(R->getValueType(), ACtx);
-    return bytes();
+  /// memory access to \p ER.
+  static SizeUnit forElementRegion(const ElementRegion *ER,
+                                   const ASTContext &ACtx) {
+    return SizeUnit(ER->getElementType(), ACtx);
   }
 
   /// If `E` is a "clean" array subscript expression, return the type of the
@@ -201,13 +200,14 @@ static bool isDeterminedByInterestingSymbol(SVal SV,
   return false;
 }
 
-/// For a given Location that can be represented as a symbolic expression
+/// 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
 /// NonLoc that specifies the number of CharUnits). Returns nullopt when these
 /// cannot be determined.
 static std::optional<std::pair<const SubRegion *, NonLoc>>
-computeOffset(ProgramStateRef State, SValBuilder &SVB, SVal Location) {
+computeOffset(ProgramStateRef State, SValBuilder &SVB,
+              const ElementRegion *CurRegion) {
   QualType T = SVB.getArrayIndexType();
   auto EvalBinOp = [&SVB, State, T](BinaryOperatorKind Op, NonLoc L, NonLoc R) {
     // We will use this utility to add and multiply values.
@@ -217,9 +217,6 @@ computeOffset(ProgramStateRef State, SValBuilder &SVB, SVal Location) {
   const SubRegion *OwnerRegion = nullptr;
   std::optional<NonLoc> Offset = SVB.makeZeroArrayIndex();
 
-  const ElementRegion *CurRegion =
-      dyn_cast_or_null<ElementRegion>(Location.getAsRegion());
-
   while (CurRegion) {
     const auto Index = CurRegion->getIndex().getAs<NonLoc>();
     if (!Index)
@@ -412,7 +409,10 @@ static std::string getAssumptionNote(bounds::CheckResult Res,
 void ArrayBoundChecker::handleAccessExpr(const Expr *E,
                                          CheckerContext &C) const {
   ASTContext &ACtx = C.getASTContext();
-  const SVal Location = C.getSVal(E);
+  const ElementRegion *AccessedER =
+      dyn_cast_or_null<ElementRegion>(C.getSVal(E).getAsRegion());
+  if (!AccessedER)
+    return;
 
   // The header ctype.h (from e.g. glibc) implements the isXXXXX() macros as
   //   #define isXXXXX(arg) (LOOKUP_TABLE[arg] & BITMASK_FOR_XXXXX)
@@ -426,7 +426,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E,
   SValBuilder &SVB = C.getSValBuilder();
 
   const std::optional<std::pair<const SubRegion *, NonLoc>> &RawOffset =
-      computeOffset(State, SVB, Location);
+      computeOffset(State, SVB, AccessedER);
 
   if (!RawOffset)
     return;
@@ -474,7 +474,7 @@ void ArrayBoundChecker::handleAccessExpr(const Expr *E,
         }
       }
 
-      SizeUnit SU = SizeUnit::forSVal(Location, ACtx);
+      SizeUnit SU = SizeUnit::forElementRegion(AccessedER, ACtx);
       BugDescription Desc = describeInvalidAccess(Res, RegName, SU);
       reportOOB(C, State, Desc, ByteOffset, Res.getExtentIfMayOverflow());
       return;

``````````

</details>


https://github.com/llvm/llvm-project/pull/216049