[clang] [-Wunsafe-buffer-usage] Fix false positives for constant cases (PR #92432)

Artem Dergachev via cfe-commits cfe-commits at lists.llvm.org
Mon May 20 11:59:45 PDT 2024


================
@@ -420,25 +420,63 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
   //    already duplicated
   //  - call both from Sema and from here
 
-  const auto *BaseDRE =
-      dyn_cast<DeclRefExpr>(Node.getBase()->IgnoreParenImpCasts());
-  if (!BaseDRE)
+  if (const auto *BaseDRE =
+          dyn_cast<DeclRefExpr>(Node.getBase()->IgnoreParenImpCasts())) {
+    if (!BaseDRE->getDecl())
+      return false;
+    if (const auto *CATy = Finder->getASTContext().getAsConstantArrayType(
+            BaseDRE->getDecl()->getType())) {
+      if (const auto *IdxLit = dyn_cast<IntegerLiteral>(Node.getIdx())) {
+        const APInt ArrIdx = IdxLit->getValue();
+        // FIXME: ArrIdx.isNegative() we could immediately emit an error as
+        // that's a bug
+        if (ArrIdx.isNonNegative() &&
+            ArrIdx.getLimitedValue() < CATy->getLimitedSize())
+          return true;
+      }
+    }
+  }
+
+  if (const auto *BaseSL =
+          dyn_cast<StringLiteral>(Node.getBase()->IgnoreParenImpCasts())) {
+    if (const auto *CATy =
+            Finder->getASTContext().getAsConstantArrayType(BaseSL->getType())) {
+      if (const auto *IdxLit = dyn_cast<IntegerLiteral>(Node.getIdx())) {
+        const APInt ArrIdx = IdxLit->getValue();
+        // FIXME: ArrIdx.isNegative() we could immediately emit an error as
+        // that's a bug
+        if (ArrIdx.isNonNegative() &&
+            ArrIdx.getLimitedValue() < CATy->getLimitedSize())
+          return true;
+      }
+    }
+  }
+
+  return false;
+}
+
+AST_MATCHER(BinaryOperator, isSafePtrArithmetic) {
+  if (Node.getOpcode() != BinaryOperatorKind::BO_Add)
     return false;
-  if (!BaseDRE->getDecl())
+
+  const auto *LHSDRE = dyn_cast<DeclRefExpr>(Node.getLHS()->IgnoreImpCasts());
+  if (!LHSDRE)
     return false;
+
   const auto *CATy = Finder->getASTContext().getAsConstantArrayType(
-      BaseDRE->getDecl()->getType());
+      LHSDRE->getDecl()->getType());
   if (!CATy)
     return false;
 
-  if (const auto *IdxLit = dyn_cast<IntegerLiteral>(Node.getIdx())) {
-    const APInt ArrIdx = IdxLit->getValue();
-    // FIXME: ArrIdx.isNegative() we could immediately emit an error as that's a
-    // bug
-    if (ArrIdx.isNonNegative() &&
-        ArrIdx.getLimitedValue() < CATy->getLimitedSize())
-      return true;
-  }
+  const auto *RHSIntLit = dyn_cast<IntegerLiteral>(Node.getRHS());
+  if (!RHSIntLit)
+    return false;
+
+  const APInt BufferOffset = RHSIntLit->getValue();
+
+  if (BufferOffset.isNonNegative() &&
----------------
haoNoQ wrote:

There's this whole `Expr->EvaluateAsInt()` thing which is arguably even simpler to use than your existing code, and it constant-folds pretty well. We should probably just use that, instead of matching integer literals.

But it still won't handle the case where there's nested layers of arithmetic over the base pointer, like `(ptr + 1) - 1`. It'll only handle like `ptr + (1 - 1)`. We'll have to support that separately. I suspect that it's still relatively easy to do with plain old recursion but this probably doesn't need to block this patch.

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


More information about the cfe-commits mailing list