[clang] [Wunsafe-buffer-usage] False positives for & expression indexing constant size array (arr[anything & 0]) (PR #112284)
Malavika Samak via cfe-commits
cfe-commits at lists.llvm.org
Tue Dec 17 22:37:41 PST 2024
https://github.com/malavikasamak updated https://github.com/llvm/llvm-project/pull/112284
>From ea92377ca9b449e62dcb9385184c9edda8e14fdb 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 | 156 +++++++++++++++++-
.../warn-unsafe-buffer-usage-array.cpp | 70 ++++++++
2 files changed, 221 insertions(+), 5 deletions(-)
diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp
index a9aff39df64746..06c7cfb5c442e1 100644
--- a/clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -431,6 +431,151 @@ 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 (EvaluateExpression(E, EVResult)) {
+ return EVResult.Val.getInt();
+ } else if (ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)) {
+ return TraverseImplicitCastExpr(ICE);
+ } else if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E)) {
+ return Max;
+ } else if (ArraySubscriptExpr *ASE = 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;
+
+ case BO_Or:
+ case BO_OrAssign:
+ Result = LHS | RHS;
+ break;
+
+ case BO_Shl:
+ case BO_ShlAssign:
+ if (RHS != Max.getLimitedValue())
+ Result = LHS << RHS.getLimitedValue();
+ break;
+
+ case BO_Shr:
+ case BO_ShrAssign:
+ if (RHS == Max.getLimitedValue())
+ Result = LHS;
+ else
+ Result = LHS.getLimitedValue() >> RHS.getLimitedValue();
+ break;
+
+ case BO_Rem:
+ case BO_RemAssign:
+ if (LHS.getLimitedValue() < RHS.getLimitedValue())
+ Result = LHS;
+ else
+ Result = --RHS;
+ break;
+
+ default:
+ break;
+ }
+ return Result;
+ }
+ return Max;
+ }
+};
+
AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
// FIXME: Proper solution:
// - refactor Sema::CheckArrayAccess
@@ -453,11 +598,12 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
return false;
}
- if (const auto *IdxLit = dyn_cast<IntegerLiteral>(Node.getIdx())) {
- const APInt ArrIdx = IdxLit->getValue();
- 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 7dd6c83dbba2a8..0e9fe386137e22 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,74 @@ void constant_idx_safe0(unsigned idx) {
buffer[0] = 0;
}
+int array[10]; // expected-warning 4{{'array' is an unsafe buffer that does not perform bounds checks}}
+
+int foo(int x) {
+ if(x < 3)
+ return ++x;
+ else
+ return x + 5;
+};
+
+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 & 9) | 8];
+ array[idx &=5];
+ array[f.x & 5];
+}
+
+void masked_idx2(unsigned long long idx, unsigned long long idx2) {
+ array[idx] = 30; // expected-note{{used in buffer access here}}
+
+ // Remainder operation
+ array[idx % 10];
+ array[10 % idx]; // expected-note{{used in buffer access here}}
+ array[9 % idx];
+ array[idx % idx2]; // expected-note{{used in buffer access here}}
+ array[idx %= 10];
+ array[idx % foo(5)];// expected-note{{used in buffer access here}}
+}
+
+void masked_idx3(unsigned long long idx) {
+ // Left shift operation <<
+ array[2 << 1];
+ array[8 << 1]; // expected-note{{used in buffer access here}}
+ array[2 << idx]; // expected-note{{used in buffer access here}}
+ array[idx << 2]; // expected-note{{used in buffer access here}}
+
+ // Right shift operation >>
+ array[16 >> 1];
+ array[8 >> idx];
+ array[idx >> 63];
+ array[(idx + idx) >> 3]; // expected-note{{used in buffer access here}}
+}
+
+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[(uint32_t)idx1 + idx2]; // expected-note{{used in buffer access here}}
+ array[idx3 & 3];
+ array[idx1 + idx2]; // expected-note{{used in buffer access here}}
+ array[idx2 + idx1]; // expected-note{{used in buffer access here}}
+}
+
+int array2[5]; // expected-warning{{'array2' is an unsafe buffer that does not perform bounds checks}}
+
+void masked_idx_safe(unsigned long long idx) {
+ array2[6 & 5]; // no warning
+ array2[6 & idx & (idx + 1) & 5]; // no warning
+ array2[6 & idx & 5 & (idx + 1) | 4]; // no warning
+ array2[2 + foo(foo(1))]; // expected-note{{used in buffer access here}}
+}
+
+
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}}
More information about the cfe-commits
mailing list