Skip to content

Cherrypick for 143986251 & 145705060 #10156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 5, 2025
Merged

Cherrypick for 143986251 & 145705060 #10156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4fbd507
[-Wunsafe-buffer-usage][BoundsSafety] Extend safe patterns of argumen…
ziqingluo-90 Feb 12, 2025
11996ee
[Safe Buffers][BoundsSafety] Fix a bug in the interop analysis that c…
ziqingluo-90 Feb 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
195 changes: 128 additions & 67 deletions clang/lib/Analysis/UnsafeBufferUsage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,66 +412,55 @@ getDependentValuesFromCall(const CountAttributedType *CAT,
return {std::move(Values)};
}

// Checks if Self and Other are the same member bases. This supports only very
// simple forms of member bases.
bool isSameMemberBase(const Expr *Self, const Expr *Other) {
for (;;) {
if (Self == Other)
return true;

const auto *SelfICE = dyn_cast<ImplicitCastExpr>(Self);
const auto *OtherICE = dyn_cast<ImplicitCastExpr>(Other);
if (SelfICE && OtherICE &&
SelfICE->getCastKind() == OtherICE->getCastKind() &&
(SelfICE->getCastKind() == CK_LValueToRValue ||
SelfICE->getCastKind() == CK_UncheckedDerivedToBase)) {
Self = SelfICE->getSubExpr();
Other = OtherICE->getSubExpr();
}

const auto *SelfDRE = dyn_cast<DeclRefExpr>(Self);
const auto *OtherDRE = dyn_cast<DeclRefExpr>(Other);
if (SelfDRE && OtherDRE)
return SelfDRE->getDecl() == OtherDRE->getDecl();

if (isa<CXXThisExpr>(Self) && isa<CXXThisExpr>(Other)) {
// `Self` and `Other` should be evaluated at the same state so `this` must
// mean the same thing for both:
return true;
}

const auto *SelfME = dyn_cast<MemberExpr>(Self);
const auto *OtherME = dyn_cast<MemberExpr>(Other);
if (!SelfME || !OtherME ||
SelfME->getMemberDecl() != OtherME->getMemberDecl()) {
return false;
}

Self = SelfME->getBase();
Other = OtherME->getBase();
}
}

// Impl of `isCompatibleWithCountExpr`. See `isCompatibleWithCountExpr` for
// document.
// high-level document.
//
// This visitor compares two expressions in a tiny subset of the language,
// including DRE of VarDecls, constants, binary operators, subscripts,
// dereferences, member accesses, and member function calls.
//
// - For constants, they are literal constants and expressions have
// compile-time constant values.
// - For a supported dereference expression, it can be either of the forms '*e'
// or '*&e', where 'e' is a supported expression.
// - For a subscript expression, it can be either an array subscript or
// overloaded subscript operator.
// - For a member function call, it must be a call to a 'const' (non-static)
// member function with zero argument. This is to ensure side-effect free.
// Other kinds of function calls are not supported, so an expression of the
// form `f(...)` is not supported.
struct CompatibleCountExprVisitor
: public ConstStmtVisitor<CompatibleCountExprVisitor, bool, const Expr *> {
: public ConstStmtVisitor<CompatibleCountExprVisitor, bool, const Expr *,
bool> {
// The third 'bool' type parameter for each visit method indicates whether a
// parameter has been substituted with its argument expression in the
// expression AST branch being visited. Since the argument expression may
// contain DREs referencing to the same parameter Decl, the analysis may hit
// an infinite loop of not knowing whether the substitution has happened. A
// typical example that could introduce infinite loop without this knowledge
// is shown below.
// ```
// void f(int * __counted_by(n) p, size_t n) {
// f(p, n);
// }
// ```
using BaseVisitor =
ConstStmtVisitor<CompatibleCountExprVisitor, bool, const Expr *>;
ConstStmtVisitor<CompatibleCountExprVisitor, bool, const Expr *, bool>;

const Expr *MemberBase;
const DependentValuesTy *DependentValues;
ASTContext &Ctx;

// If `Deref` has the form `*&e`, return `e`; otherwise return nullptr.
const Expr *trySimplifyDerefAddressof(const UnaryOperator *Deref) {
const Expr *trySimplifyDerefAddressof(const UnaryOperator *Deref,
bool hasBeenSubstituted) {
const Expr *DerefOperand = Deref->getSubExpr()->IgnoreParenImpCasts();

if (const auto *UO = dyn_cast<UnaryOperator>(DerefOperand))
if (UO->getOpcode() == UO_AddrOf)
return UO->getSubExpr();
if (const auto *DRE = dyn_cast<DeclRefExpr>(DerefOperand)) {
if (!DependentValues)
if (!DependentValues || hasBeenSubstituted)
return nullptr;

auto I = DependentValues->find(DRE->getDecl());
Expand All @@ -489,18 +478,22 @@ struct CompatibleCountExprVisitor
ASTContext &Ctx)
: MemberBase(MemberBase), DependentValues(DependentValues), Ctx(Ctx) {}

bool VisitStmt(const Stmt *S, const Expr *E) { return false; }
bool VisitStmt(const Stmt *S, const Expr *E, bool hasBeenSubstituted) {
return false;
}

bool VisitImplicitCastExpr(const ImplicitCastExpr *SelfICE,
const Expr *Other) {
return Visit(SelfICE->getSubExpr(), Other);
bool VisitImplicitCastExpr(const ImplicitCastExpr *SelfICE, const Expr *Other,
bool hasBeenSubstituted) {
return Visit(SelfICE->getSubExpr(), Other, hasBeenSubstituted);
}

bool VisitParenExpr(const ParenExpr *SelfPE, const Expr *Other) {
return Visit(SelfPE->getSubExpr(), Other);
bool VisitParenExpr(const ParenExpr *SelfPE, const Expr *Other,
bool hasBeenSubstituted) {
return Visit(SelfPE->getSubExpr(), Other, hasBeenSubstituted);
}

bool VisitIntegerLiteral(const IntegerLiteral *SelfIL, const Expr *Other) {
bool VisitIntegerLiteral(const IntegerLiteral *SelfIL, const Expr *Other,
bool hasBeenSubstituted) {
if (const auto *IntLit =
dyn_cast<IntegerLiteral>(Other->IgnoreParenImpCasts())) {
return SelfIL == IntLit ||
Expand All @@ -510,7 +503,8 @@ struct CompatibleCountExprVisitor
}

bool VisitUnaryExprOrTypeTraitExpr(const UnaryExprOrTypeTraitExpr *Self,
const Expr *Other) {
const Expr *Other,
bool hasBeenSubstituted) {
// If `Self` is a `sizeof` expression, try to evaluate and compare the two
// expressions as constants:
if (Self->getKind() == UnaryExprOrTypeTrait::UETT_SizeOf) {
Expand All @@ -527,13 +521,19 @@ struct CompatibleCountExprVisitor
return false;
}

bool VisitDeclRefExpr(const DeclRefExpr *SelfDRE, const Expr *Other) {
bool VisitCXXThisExpr(const CXXThisExpr *SelfThis, const Expr *Other,
bool hasBeenSubstituted) {
return isa<CXXThisExpr>(Other->IgnoreParenImpCasts());
}

bool VisitDeclRefExpr(const DeclRefExpr *SelfDRE, const Expr *Other,
bool hasBeenSubstituted) {
const ValueDecl *SelfVD = SelfDRE->getDecl();

if (DependentValues) {
if (DependentValues && !hasBeenSubstituted) {
const auto It = DependentValues->find(SelfVD);
if (It != DependentValues->end())
return Visit(It->second, Other);
return Visit(It->second, Other, true);
}

const auto *O = Other->IgnoreParenImpCasts();
Expand All @@ -548,49 +548,110 @@ struct CompatibleCountExprVisitor
const auto *OtherME = dyn_cast<MemberExpr>(O);
if (MemberBase && OtherME) {
return OtherME->getMemberDecl() == SelfVD &&
isSameMemberBase(OtherME->getBase(), MemberBase);
Visit(OtherME->getBase(), MemberBase, hasBeenSubstituted);
}

return false;
}

bool VisitMemberExpr(const MemberExpr *Self, const Expr *Other) {
bool VisitMemberExpr(const MemberExpr *Self, const Expr *Other,
bool hasBeenSubstituted) {
// Even though we don't support member expression in counted-by, actual
// arguments can be member expressions.
if (Self == Other)
return true;
if (const auto *DRE = dyn_cast<DeclRefExpr>(Other->IgnoreParenImpCasts()))
return MemberBase && Self->getMemberDecl() == DRE->getDecl() &&
isSameMemberBase(Self->getBase(), MemberBase);
Visit(Self->getBase(), MemberBase, hasBeenSubstituted);
if (const auto *OtherME =
dyn_cast<MemberExpr>(Other->IgnoreParenImpCasts())) {
return Self->getMemberDecl() == OtherME->getMemberDecl() &&
Visit(Self->getBase(), OtherME->getBase(), hasBeenSubstituted);
}
return false;
}

bool VisitUnaryOperator(const UnaryOperator *SelfUO, const Expr *Other) {
bool VisitUnaryOperator(const UnaryOperator *SelfUO, const Expr *Other,
bool hasBeenSubstituted) {
if (SelfUO->getOpcode() != UO_Deref)
return false; // We don't support any other unary operator

if (const auto *OtherUO =
dyn_cast<UnaryOperator>(Other->IgnoreParenImpCasts())) {
if (SelfUO->getOpcode() == OtherUO->getOpcode())
return Visit(SelfUO->getSubExpr(), OtherUO->getSubExpr());
return Visit(SelfUO->getSubExpr(), OtherUO->getSubExpr(),
hasBeenSubstituted);
}
// If `Other` is not a dereference expression, try to simplify `SelfUO`:
if (const auto *SimplifiedSelf = trySimplifyDerefAddressof(SelfUO)) {
return Visit(SimplifiedSelf, Other);
if (const auto *SimplifiedSelf =
trySimplifyDerefAddressof(SelfUO, hasBeenSubstituted)) {
return Visit(SimplifiedSelf, Other, hasBeenSubstituted);
}
return false;
}

bool VisitBinaryOperator(const BinaryOperator *SelfBO, const Expr *Other) {
bool VisitBinaryOperator(const BinaryOperator *SelfBO, const Expr *Other,
bool hasBeenSubstituted) {
const auto *OtherBO =
dyn_cast<BinaryOperator>(Other->IgnoreParenImpCasts());
if (OtherBO && OtherBO->getOpcode() == SelfBO->getOpcode()) {
return Visit(SelfBO->getLHS(), OtherBO->getLHS()) &&
Visit(SelfBO->getRHS(), OtherBO->getRHS());
return Visit(SelfBO->getLHS(), OtherBO->getLHS(), hasBeenSubstituted) &&
Visit(SelfBO->getRHS(), OtherBO->getRHS(), hasBeenSubstituted);
}

return false;
}

// Support any overloaded operator[] so long as it is a const method.
bool VisitCXXOperatorCallExpr(const CXXOperatorCallExpr *SelfOpCall,
const Expr *Other, bool hasBeenSubstituted) {
if (SelfOpCall->getOperator() != OverloadedOperatorKind::OO_Subscript)
return false;

const auto *MD = dyn_cast<CXXMethodDecl>(SelfOpCall->getCalleeDecl());

if (!MD || !MD->isConst())
return false;
if (const auto *OtherOpCall =
dyn_cast<CXXOperatorCallExpr>(Other->IgnoreParenImpCasts()))
if (SelfOpCall->getOperator() == OtherOpCall->getOperator()) {
return Visit(SelfOpCall->getArg(0), OtherOpCall->getArg(0),
hasBeenSubstituted) &&
Visit(SelfOpCall->getArg(1), OtherOpCall->getArg(1),
hasBeenSubstituted);
}
return false;
}

// Support array/pointer subscript. Even though these operators are generally
// considered unsafe, they can be safely used on constant arrays with
// known-safe literal indexes.
bool VisitArraySubscriptExpr(const ArraySubscriptExpr *SelfAS,
const Expr *Other, bool hasBeenSubstituted) {
if (const auto *OtherAS =
dyn_cast<ArraySubscriptExpr>(Other->IgnoreParenImpCasts()))
return Visit(SelfAS->getLHS(), OtherAS->getLHS(), hasBeenSubstituted) &&
Visit(SelfAS->getRHS(), OtherAS->getRHS(), hasBeenSubstituted);
return false;
}

// Support non-static member call:
bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *SelfCall,
const Expr *Other, bool hasBeenSubstituted) {
const CXXMethodDecl *MD = SelfCall->getMethodDecl();

// The callee member function must be a const function with no parameter:
if (MD->isConst() && MD->param_empty()) {
if (auto *OtherCall =
dyn_cast<CXXMemberCallExpr>(Other->IgnoreParenImpCasts())) {
return OtherCall->getMethodDecl() == MD &&
Visit(SelfCall->getImplicitObjectArgument(),
OtherCall->getImplicitObjectArgument(),
hasBeenSubstituted);
}
}
return false;
}
};

// TL'DR:
Expand Down Expand Up @@ -632,7 +693,7 @@ bool isCompatibleWithCountExpr(const Expr *E, const Expr *ExpectedCountExpr,
const DependentValuesTy *DependentValues,
ASTContext &Ctx) {
CompatibleCountExprVisitor Visitor(MemberBase, DependentValues, Ctx);
return Visitor.Visit(ExpectedCountExpr, E);
return Visitor.Visit(ExpectedCountExpr, E, /* hasBeenSubstituted*/ false);
}

// Returns if a pair of expressions contain method calls to .data()/.c_str() and
Expand Down
Loading