Skip to content

Commit ea92377

Browse files
author
MalavikaSamak
committed
[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)
1 parent f65a21a commit ea92377

File tree

2 files changed

+221
-5
lines changed

2 files changed

+221
-5
lines changed

‎clang/lib/Analysis/UnsafeBufferUsage.cpp‎

Lines changed: 151 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,151 @@ AST_MATCHER(CXXConstructExpr, isSafeSpanTwoParamConstruct) {
431431
return false;
432432
}
433433

434+
class MaxValueEval : public ConstStmtVisitor<MaxValueEval, llvm::APInt> {
435+
436+
ASTContext &Context;
437+
llvm::APInt Max;
438+
unsigned bit_width;
439+
440+
public:
441+
typedef ConstStmtVisitor<MaxValueEval, llvm::APInt> VisitorBase;
442+
443+
explicit MaxValueEval(ASTContext &Ctx, const Expr *exp) : Context(Ctx) {
444+
bit_width = Ctx.getIntWidth(exp->getType());
445+
Max = llvm::APInt::getSignedMaxValue(bit_width);
446+
// val.clear();
447+
}
448+
449+
llvm::APInt findMatch(Expr *exp) { return TraverseStmt(exp); }
450+
451+
llvm::APInt TraverseStmt(Stmt *S) {
452+
if (Expr *E = dyn_cast<Expr>(S)) {
453+
Expr::EvalResult EVResult;
454+
if (EvaluateExpression(E, EVResult)) {
455+
return EVResult.Val.getInt();
456+
} else if (ImplicitCastExpr *ICE = dyn_cast<ImplicitCastExpr>(E)) {
457+
return TraverseImplicitCastExpr(ICE);
458+
} else if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E)) {
459+
return Max;
460+
} else if (ArraySubscriptExpr *ASE = dyn_cast<ArraySubscriptExpr>(E)) {
461+
return Max;
462+
} else if (BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
463+
return TraverseBinaryOperator(BO);
464+
} else if (IntegerLiteral *IL = dyn_cast<IntegerLiteral>(E)) {
465+
return IL->getValue();
466+
}
467+
}
468+
return Max;
469+
}
470+
471+
llvm::APInt TraverseImplicitCastExpr(ImplicitCastExpr *E) {
472+
Expr::EvalResult EVResult;
473+
if (EvaluateExpression(E, EVResult)) {
474+
return EVResult.Val.getInt();
475+
} else {
476+
Expr *eExpr = E->getSubExpr();
477+
llvm::APInt subEValue = TraverseStmt(eExpr);
478+
switch (E->getCastKind()) {
479+
case CK_LValueToRValue:
480+
return subEValue;
481+
case CK_IntegralCast: {
482+
Expr *eExpr = E->getSubExpr();
483+
clang::IntegerLiteral *intLiteral = clang::IntegerLiteral::Create(
484+
Context, subEValue, eExpr->getType(), {});
485+
E->setSubExpr(intLiteral);
486+
bool evaluated = EvaluateExpression(E, EVResult);
487+
E->setSubExpr(eExpr);
488+
if (evaluated) {
489+
return EVResult.Val.getInt();
490+
}
491+
break;
492+
}
493+
default:
494+
break;
495+
}
496+
return Max;
497+
}
498+
}
499+
500+
bool EvaluateExpression(Expr *exp, Expr::EvalResult &EVResult) {
501+
if (exp->EvaluateAsInt(EVResult, Context)) {
502+
return true;
503+
}
504+
return false;
505+
}
506+
507+
llvm::APInt TraverseBinaryOperator(BinaryOperator *E) {
508+
unsigned bwidth = Context.getIntWidth(E->getType());
509+
510+
auto evaluateSubExpr = [&, bwidth](Expr *E) -> llvm::APInt {
511+
llvm::APInt Result = TraverseStmt(E);
512+
unsigned width = Result.getBitWidth();
513+
514+
// Fix the bit length.
515+
if (bwidth < width)
516+
Result = Result.trunc(bwidth);
517+
else if (bwidth > width)
518+
Result =
519+
APInt(bwidth, Result.getLimitedValue(), Result.isSignedIntN(width));
520+
return Result;
521+
};
522+
523+
Expr::EvalResult EVResult;
524+
if (EvaluateExpression(E, EVResult)) {
525+
return EVResult.Val.getInt();
526+
} else {
527+
Expr *LHSExpr = E->getLHS()->IgnoreParenCasts();
528+
Expr *RHSExpr = E->getRHS()->IgnoreParenCasts();
529+
530+
unsigned bwidth = Context.getIntWidth(E->getType());
531+
532+
llvm::APInt LHS = evaluateSubExpr(LHSExpr);
533+
llvm::APInt RHS = evaluateSubExpr(RHSExpr);
534+
535+
llvm::APInt Result = Max;
536+
537+
switch (E->getOpcode()) {
538+
case BO_And:
539+
case BO_AndAssign:
540+
Result = LHS & RHS;
541+
break;
542+
543+
case BO_Or:
544+
case BO_OrAssign:
545+
Result = LHS | RHS;
546+
break;
547+
548+
case BO_Shl:
549+
case BO_ShlAssign:
550+
if (RHS != Max.getLimitedValue())
551+
Result = LHS << RHS.getLimitedValue();
552+
break;
553+
554+
case BO_Shr:
555+
case BO_ShrAssign:
556+
if (RHS == Max.getLimitedValue())
557+
Result = LHS;
558+
else
559+
Result = LHS.getLimitedValue() >> RHS.getLimitedValue();
560+
break;
561+
562+
case BO_Rem:
563+
case BO_RemAssign:
564+
if (LHS.getLimitedValue() < RHS.getLimitedValue())
565+
Result = LHS;
566+
else
567+
Result = --RHS;
568+
break;
569+
570+
default:
571+
break;
572+
}
573+
return Result;
574+
}
575+
return Max;
576+
}
577+
};
578+
434579
AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
435580
// FIXME: Proper solution:
436581
// - refactor Sema::CheckArrayAccess
@@ -453,11 +598,12 @@ AST_MATCHER(ArraySubscriptExpr, isSafeArraySubscript) {
453598
return false;
454599
}
455600

456-
if (const auto *IdxLit = dyn_cast<IntegerLiteral>(Node.getIdx())) {
457-
const APInt ArrIdx = IdxLit->getValue();
458-
if (ArrIdx.isNonNegative() && ArrIdx.getLimitedValue() < limit)
459-
return true;
460-
}
601+
MaxValueEval Vis(Finder->getASTContext(), Node.getIdx());
602+
APInt result = Vis.findMatch(const_cast<Expr *>(Node.getIdx()));
603+
604+
if (result.isNonNegative() && result.getLimitedValue() < limit)
605+
return true;
606+
461607
return false;
462608
}
463609

‎clang/test/SemaCXX/warn-unsafe-buffer-usage-array.cpp‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@ void foo2(unsigned idx) {
1818

1919
struct Foo {
2020
int member_buffer[10];
21+
int x;
2122
};
23+
2224
void foo2(Foo& f, unsigned idx) {
2325
f.member_buffer[idx] = 0; // expected-warning{{unsafe buffer access}}
2426
}
@@ -33,6 +35,74 @@ void constant_idx_safe0(unsigned idx) {
3335
buffer[0] = 0;
3436
}
3537

38+
int array[10]; // expected-warning 4{{'array' is an unsafe buffer that does not perform bounds checks}}
39+
40+
int foo(int x) {
41+
if(x < 3)
42+
return ++x;
43+
else
44+
return x + 5;
45+
};
46+
47+
void masked_idx1(unsigned long long idx, Foo f) {
48+
// Bitwise and operation
49+
array[idx & 5] = 10; // no warning
50+
array[idx & 11 & 5] = 3; // no warning
51+
array[idx & 11] = 20; // expected-note{{used in buffer access here}}
52+
array[(idx & 9) | 8];
53+
array[idx &=5];
54+
array[f.x & 5];
55+
}
56+
57+
void masked_idx2(unsigned long long idx, unsigned long long idx2) {
58+
array[idx] = 30; // expected-note{{used in buffer access here}}
59+
60+
// Remainder operation
61+
array[idx % 10];
62+
array[10 % idx]; // expected-note{{used in buffer access here}}
63+
array[9 % idx];
64+
array[idx % idx2]; // expected-note{{used in buffer access here}}
65+
array[idx %= 10];
66+
array[idx % foo(5)];// expected-note{{used in buffer access here}}
67+
}
68+
69+
void masked_idx3(unsigned long long idx) {
70+
// Left shift operation <<
71+
array[2 << 1];
72+
array[8 << 1]; // expected-note{{used in buffer access here}}
73+
array[2 << idx]; // expected-note{{used in buffer access here}}
74+
array[idx << 2]; // expected-note{{used in buffer access here}}
75+
76+
// Right shift operation >>
77+
array[16 >> 1];
78+
array[8 >> idx];
79+
array[idx >> 63];
80+
array[(idx + idx) >> 3]; // expected-note{{used in buffer access here}}
81+
}
82+
83+
typedef unsigned long long uint64_t;
84+
typedef unsigned int uint32_t;
85+
typedef unsigned char uint8_t;
86+
87+
void type_conversions(uint64_t idx1, uint32_t idx2, uint8_t idx3) {
88+
array[(uint32_t)idx1 & 3];
89+
array[idx2 & 3];
90+
array[(uint32_t)idx1 + idx2]; // expected-note{{used in buffer access here}}
91+
array[idx3 & 3];
92+
array[idx1 + idx2]; // expected-note{{used in buffer access here}}
93+
array[idx2 + idx1]; // expected-note{{used in buffer access here}}
94+
}
95+
96+
int array2[5]; // expected-warning{{'array2' is an unsafe buffer that does not perform bounds checks}}
97+
98+
void masked_idx_safe(unsigned long long idx) {
99+
array2[6 & 5]; // no warning
100+
array2[6 & idx & (idx + 1) & 5]; // no warning
101+
array2[6 & idx & 5 & (idx + 1) | 4]; // no warning
102+
array2[2 + foo(foo(1))]; // expected-note{{used in buffer access here}}
103+
}
104+
105+
36106
void constant_idx_unsafe(unsigned idx) {
37107
int buffer[10]; // expected-warning{{'buffer' is an unsafe buffer that does not perform bounds checks}}
38108
// expected-note@-1{{change type of 'buffer' to 'std::array' to label it for hardening}}

0 commit comments

Comments
 (0)