[clang] [Wunsafe-buffer-usage] False positives for & expression indexing constant size array (arr[anything & 0]) (PR #112284)

[Malavika Samak via cfe-commits]

https://github.com/malavikasamak updated https://github.com/llvm/llvm-project/pull/112284

>From f5b583585b96557377e6e2cb524c8e87fab81dd7 Mon Sep 17 00:00:00 2001
From: MalavikaSamak <malavika2 at apple.com>
Date: Fri, 11 Oct 2024 12:24:58 -0700
Subject: [PATCH] [Wunsafe-buffer-usage] False positives for & expression
 indexing constant size array (arr[anything & 0])

Do not warn when a constant sized array is indexed with an expression that contains bitwise and operation
involving constants and it always results in a bound safe access.

rdar://136684050
---
 clang/lib/Analysis/UnsafeBufferUsage.cpp      | 123 ++++++++++++++++++
 .../warn-unsafe-buffer-usage-array.cpp        |  31 +++++
 2 files changed, 154 insertions(+)

diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index c064aa30e8aedc..dd593945cbc286 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -431,6 +431,122 @@ AST_MATCHER(CXXConstructExpr, isSafeSpanTwoParamConstruct) {
   return false;
 }
 
+class MaxValueEval : public ConstStmtVisitor<MaxValueEval, llvm::APInt> {
+
+  ASTContext &Context;
+  llvm::APInt Max;
+  unsigned bit_width;
+
+public:
+  typedef ConstStmtVisitor<MaxValueEval, llvm::APInt> VisitorBase;
+
+  explicit MaxValueEval(ASTContext &Ctx, const Expr *exp) : Context(Ctx) {
+    bit_width = Ctx.getIntWidth(exp->getType());
+    Max = llvm::APInt::getSignedMaxValue(bit_width);
+    // val.clear();
+  }
+
+  llvm::APInt findMatch(Expr *exp) { return TraverseStmt(exp); }
+
+  llvm::APInt TraverseStmt(Stmt *S) {
+    if (Expr *E = dyn_cast<Expr>(S)) {
+      Expr::EvalResult EVResult;
+      if (ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)) {
+        return TraverseImplicitCastExpr(ICE);
+      } else if (dyn_cast<DeclRefExpr>(E)) {
+        return Max;
+      } else if (dyn_cast<ArraySubscriptExpr>(E)) {
+        return Max;
+      } else if (BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
+        return TraverseBinaryOperator(BO);
+      } else if (IntegerLiteral *IL = dyn_cast<IntegerLiteral>(E)) {
+        return IL->getValue();
+      }
+    }
+    return Max;
+  }
+
+  llvm::APInt TraverseImplicitCastExpr(ImplicitCastExpr *E) {
+    Expr::EvalResult EVResult;
+    if (EvaluateExpression(E, EVResult)) {
+      return EVResult.Val.getInt();
+    } else {
+      Expr *eExpr = E->getSubExpr();
+      llvm::APInt subEValue = TraverseStmt(eExpr);
+      switch (E->getCastKind()) {
+      case CK_LValueToRValue:
+        return subEValue;
+      case CK_IntegralCast: {
+        Expr *eExpr = E->getSubExpr();
+        clang::IntegerLiteral *intLiteral = clang::IntegerLiteral::Create(
+            Context, subEValue, eExpr->getType(), {});
+        E->setSubExpr(intLiteral);
+        bool evaluated = EvaluateExpression(E, EVResult);
+        E->setSubExpr(eExpr);
+        if (evaluated) {
+          return EVResult.Val.getInt();
+        }
+        break;
+      }
+      default:
+        break;
+      }
+      return Max;
+    }
+  }
+
+  bool EvaluateExpression(Expr *exp, Expr::EvalResult &EVResult) {
+    if (exp->EvaluateAsInt(EVResult, Context)) {
+      return true;
+    }
+    return false;
+  }
+
+  llvm::APInt TraverseBinaryOperator(BinaryOperator *E) {
+    unsigned bwidth = Context.getIntWidth(E->getType());
+
+    auto evaluateSubExpr = [&, bwidth](Expr *E) -> llvm::APInt {
+      llvm::APInt Result = TraverseStmt(E);
+      unsigned width = Result.getBitWidth();
+
+      // Fix the bit length.
+      if (bwidth < width)
+        Result = Result.trunc(bwidth);
+      else if (bwidth > width)
+        Result =
+            APInt(bwidth, Result.getLimitedValue(), Result.isSignedIntN(width));
+      return Result;
+    };
+
+    Expr::EvalResult EVResult;
+    if (EvaluateExpression(E, EVResult)) {
+      return EVResult.Val.getInt();
+    } else {
+      Expr *LHSExpr = E->getLHS()->IgnoreParenCasts();
+      Expr *RHSExpr = E->getRHS()->IgnoreParenCasts();
+
+      unsigned bwidth = Context.getIntWidth(E->getType());
+
+      llvm::APInt LHS = evaluateSubExpr(LHSExpr);
+      llvm::APInt RHS = evaluateSubExpr(RHSExpr);
+
+      llvm::APInt Result = Max;
+
+      switch (E->getOpcode()) {
+      case BO_And:
+      case BO_AndAssign:
+        Result = LHS & RHS;
+        break;
+
+      default:
+        break;
+      }
+      return Result;
+    }
+    return Max;
+  }
+};
+
 AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
   // FIXME: Proper solution:
   //  - refactor Sema::CheckArrayAccess
@@ -463,6 +579,13 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
     if (ArrIdx.isNonNegative() && ArrIdx.getLimitedValue() < limit)
       return true;
   }
+
+  MaxValueEval Vis(Finder->getASTContext(), Node.getIdx());
+  APInt result = Vis.findMatch(const_cast<Expr *>(Node.getIdx()));
+
+  if (result.isNonNegative() && result.getLimitedValue() < limit)
+    return true;
+
   return false;
 }
 
diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
index e80b54b7c69677..93a3be5d9d1f23 100644
--- a/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
+++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp
@@ -18,7 +18,9 @@ void foo2(unsigned idx) {
 
 struct Foo {
   int member_buffer[10];
+  int x;
 };
+
 void foo2(Foo& f, unsigned idx) {
   f.member_buffer[idx] = 0; // expected-warning{{unsafe buffer access}}
 }
@@ -33,6 +35,35 @@ void constant_idx_safe0(unsigned idx) {
   buffer[0] = 0;
 }
 
+int array[10]; // expected-warning {{'array' is an unsafe buffer that does not perform bounds checks}}
+
+void masked_idx1(unsigned long long idx, Foo f) {
+  // Bitwise and operation
+  array[idx & 5] = 10; // no warning
+  array[idx & 11 & 5] = 3; // no warning
+  array[idx & 11] = 20; // expected-note{{used in buffer access here}}
+  array[idx &=5];
+  array[f.x & 5];
+}
+
+typedef unsigned long long uint64_t;
+typedef unsigned int uint32_t;
+typedef unsigned char uint8_t;
+
+void type_conversions(uint64_t idx1, uint32_t idx2, uint8_t idx3) {
+  array[(uint32_t)idx1 & 3];
+  array[idx2 & 3];
+  array[idx3 & 3];
+}
+
+int array2[5];
+
+void masked_idx_safe(unsigned long long idx) {
+  array2[6 & 5]; // no warning
+  array2[6 & idx & (idx + 1) & 5]; // no warning
+}
+
+
 void constant_idx_unsafe(unsigned idx) {
   int buffer[10];       // expected-warning{{'buffer' is an unsafe buffer that does not perform bounds checks}}
                         // expected-note at -1{{change type of 'buffer' to 'std::array' to label it for hardening}}