Skip to content

[WebKit checkers] Recognize ensureFoo functions #119681

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.

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 3 commits into from
Dec 13, 2024
Merged
Changes from all commits
Commits
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
42 changes: 42 additions & 0 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
Original file line number Diff line number Diff line change
@@ -13,6 +13,7 @@
#include "clang/AST/DeclCXX.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/ExprObjC.h"
#include "clang/AST/StmtVisitor.h"
#include <optional>

namespace clang {
@@ -158,6 +159,9 @@ bool isConstOwnerPtrMemberExpr(const clang::Expr *E) {
E = ThisArg;
}
}
} else if (auto *OCE = dyn_cast<CXXOperatorCallExpr>(E)) {
if (OCE->getOperator() == OO_Star && OCE->getNumArgs() == 1)
E = OCE->getArg(0);
}
auto *ME = dyn_cast<MemberExpr>(E);
if (!ME)
@@ -169,4 +173,42 @@ bool isConstOwnerPtrMemberExpr(const clang::Expr *E) {
return isOwnerPtrType(T) && T.isConstQualified();
}

class EnsureFunctionVisitor
: public ConstStmtVisitor<EnsureFunctionVisitor, bool> {
public:
bool VisitStmt(const Stmt *S) {
for (const Stmt *Child : S->children()) {
if (Child && !Visit(Child))
return false;
}
return true;
}

bool VisitReturnStmt(const ReturnStmt *RS) {
if (auto *RV = RS->getRetValue()) {
RV = RV->IgnoreParenCasts();
if (isa<CXXNullPtrLiteralExpr>(RV))
return true;
return isConstOwnerPtrMemberExpr(RV);
}
return false;
}
};

bool EnsureFunctionAnalysis::isACallToEnsureFn(const clang::Expr *E) const {
auto *MCE = dyn_cast<CXXMemberCallExpr>(E);
if (!MCE)
return false;
auto *Callee = MCE->getDirectCallee();
if (!Callee)
return false;
auto *Body = Callee->getBody();
if (!Body)
return false;
auto [CacheIt, IsNew] = Cache.insert(std::make_pair(Callee, false));
if (IsNew)
CacheIt->second = EnsureFunctionVisitor().Visit(Body);
return CacheIt->second;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the fact you thought of performance optimization here!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I figured we should cache the result instead of keep analyzing the same function repeatedly.

}

} // namespace clang
10 changes: 10 additions & 0 deletions clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
Original file line number Diff line number Diff line change
@@ -67,6 +67,16 @@ bool isASafeCallArg(const clang::Expr *E);
/// \returns true if E is a MemberExpr accessing a const smart pointer type.
bool isConstOwnerPtrMemberExpr(const clang::Expr *E);

/// \returns true if E is a CXXMemberCallExpr which returns a const smart
/// pointer type.
class EnsureFunctionAnalysis {
using CacheTy = llvm::DenseMap<const FunctionDecl *, bool>;
mutable CacheTy Cache{};

public:
bool isACallToEnsureFn(const Expr *E) const;
};

/// \returns name of AST node or empty string.
template <typename T> std::string safeGetName(const T *ASTNode) {
const auto *const ND = llvm::dyn_cast_or_null<clang::NamedDecl>(ASTNode);
Original file line number Diff line number Diff line change
@@ -33,6 +33,7 @@ class RawPtrRefCallArgsChecker
mutable BugReporter *BR;

TrivialFunctionAnalysis TFA;
EnsureFunctionAnalysis EFA;

public:
RawPtrRefCallArgsChecker(const char *description)
@@ -140,7 +141,7 @@ class RawPtrRefCallArgsChecker

bool isPtrOriginSafe(const Expr *Arg) const {
return tryToFindPtrOrigin(Arg, /*StopAtFirstRefCountedObj=*/true,
[](const clang::Expr *ArgOrigin, bool IsSafe) {
[&](const clang::Expr *ArgOrigin, bool IsSafe) {
if (IsSafe)
return true;
if (isa<CXXNullPtrLiteralExpr>(ArgOrigin)) {
@@ -154,6 +155,8 @@ class RawPtrRefCallArgsChecker
}
if (isASafeCallArg(ArgOrigin))
return true;
if (EFA.isACallToEnsureFn(ArgOrigin))
return true;
return false;
});
}
Original file line number Diff line number Diff line change
@@ -166,6 +166,7 @@ class RawPtrRefLocalVarsChecker
: public Checker<check::ASTDecl<TranslationUnitDecl>> {
BugType Bug;
mutable BugReporter *BR;
EnsureFunctionAnalysis EFA;

public:
RawPtrRefLocalVarsChecker(const char *description)
@@ -278,6 +279,9 @@ class RawPtrRefLocalVarsChecker
if (isConstOwnerPtrMemberExpr(InitArgOrigin))
return true;

if (EFA.isACallToEnsureFn(InitArgOrigin))
return true;

if (auto *Ref = llvm::dyn_cast<DeclRefExpr>(InitArgOrigin)) {
if (auto *MaybeGuardian =
dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
Original file line number Diff line number Diff line change
@@ -49,15 +49,44 @@ class Foo {
Foo();
void bar();

CheckedObj& ensureObj3() {
if (!m_obj3)
const_cast<std::unique_ptr<CheckedObj>&>(m_obj3) = new CheckedObj;
return *m_obj3;
}

CheckedObj& badEnsureObj4() {
if (!m_obj4)
const_cast<std::unique_ptr<CheckedObj>&>(m_obj4) = new CheckedObj;
if (auto* next = m_obj4->next())
return *next;
return *m_obj4;
}

CheckedObj* ensureObj5() {
if (!m_obj5)
const_cast<std::unique_ptr<CheckedObj>&>(m_obj5) = new CheckedObj;
if (m_obj5->next())
return nullptr;
return m_obj5.get();
}

private:
const std::unique_ptr<CheckedObj> m_obj1;
std::unique_ptr<CheckedObj> m_obj2;
const std::unique_ptr<CheckedObj> m_obj3;
const std::unique_ptr<CheckedObj> m_obj4;
const std::unique_ptr<CheckedObj> m_obj5;
};

void Foo::bar() {
m_obj1->method();
m_obj2->method();
// expected-warning@-1{{Call argument for 'this' parameter is unchecked and unsafe}}
ensureObj3().method();
badEnsureObj4().method();
// expected-warning@-1{{Call argument for 'this' parameter is unchecked and unsafe}}
ensureObj5()->method();
}

} // namespace call_args_const_unique_ptr
Original file line number Diff line number Diff line change
@@ -117,7 +117,7 @@ namespace null_ptr {

namespace ref_counted_lookalike {
struct Decoy {
CheckedObj* get() { return nullptr; }
CheckedObj* get();
};

void foo() {
Original file line number Diff line number Diff line change
@@ -52,15 +52,44 @@ class Foo {
Foo();
void bar();

RefCountable& ensureObj3() {
if (!m_obj3)
const_cast<std::unique_ptr<RefCountable>&>(m_obj3) = RefCountable::makeUnique();
return *m_obj3;
}

RefCountable& badEnsureObj4() {
if (!m_obj4)
const_cast<std::unique_ptr<RefCountable>&>(m_obj4) = RefCountable::makeUnique();
if (auto* next = m_obj4->next())
return *next;
return *m_obj4;
}

RefCountable* ensureObj5() {
if (!m_obj5)
const_cast<std::unique_ptr<RefCountable>&>(m_obj5) = RefCountable::makeUnique();
if (m_obj5->next())
return nullptr;
return m_obj5.get();
}

private:
const std::unique_ptr<RefCountable> m_obj1;
std::unique_ptr<RefCountable> m_obj2;
const std::unique_ptr<RefCountable> m_obj3;
const std::unique_ptr<RefCountable> m_obj4;
const std::unique_ptr<RefCountable> m_obj5;
};

void Foo::bar() {
m_obj1->method();
m_obj2->method();
// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
ensureObj3().method();
badEnsureObj4().method();
// expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
ensureObj5()->method();
}

} // namespace call_args_const_unique_ptr
2 changes: 1 addition & 1 deletion clang/test/Analysis/Checkers/WebKit/call-args.cpp
Original file line number Diff line number Diff line change
@@ -117,7 +117,7 @@ namespace null_ptr {

namespace ref_counted_lookalike {
struct Decoy {
RefCountable* get() { return nullptr; }
RefCountable* get();
};

void foo() {
Original file line number Diff line number Diff line change
@@ -12,9 +12,36 @@ class Foo {
Foo();
void bar();

CheckedObj& ensureObj3() {
if (!m_obj3)
const_cast<CheckedPtr<CheckedObj>&>(m_obj3) = new CheckedObj;
return *m_obj3;
}

CheckedObj& ensureObj4() {
if (!m_obj4)
const_cast<CheckedPtr<CheckedObj>&>(m_obj4) = new CheckedObj;
if (auto* next = m_obj4->next()) {
// expected-warning@-1{{Local variable 'next' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
return *next;
}
return *m_obj4;
}

CheckedObj* ensureObj5() {
if (!m_obj5)
const_cast<CheckedPtr<CheckedObj>&>(m_obj5) = new CheckedObj;
if (m_obj5->next())
return nullptr;
return m_obj5.get();
}

private:
const CheckedPtr<CheckedObj> m_obj1;
CheckedPtr<CheckedObj> m_obj2;
const CheckedPtr<CheckedObj> m_obj3;
const CheckedPtr<CheckedObj> m_obj4;
const CheckedPtr<CheckedObj> m_obj5;
};

void Foo::bar() {
@@ -23,6 +50,12 @@ void Foo::bar() {
auto* obj2 = m_obj2.get();
// expected-warning@-1{{Local variable 'obj2' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
obj2->method();
auto& obj3 = ensureObj3();
obj3.method();
auto& obj4 = ensureObj4();
// expected-warning@-1{{Local variable 'obj4' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
obj4.method();
auto* obj5 = ensureObj5();
}

} // namespace local_vars_const_checkedptr_member
Original file line number Diff line number Diff line change
@@ -12,9 +12,36 @@ class Foo {
Foo();
void bar();

RefCountable& ensureObj3() {
if (!m_obj3)
const_cast<RefPtr<RefCountable>&>(m_obj3) = RefCountable::create();
return *m_obj3;
}

RefCountable& ensureObj4() {
if (!m_obj4)
const_cast<RefPtr<RefCountable>&>(m_obj4) = RefCountable::create();
if (auto* next = m_obj4->next()) {
// expected-warning@-1{{Local variable 'next' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
return *next;
}
return *m_obj4;
}

RefCountable* ensureObj5() {
if (!m_obj5)
const_cast<RefPtr<RefCountable>&>(m_obj5) = RefCountable::create();
if (m_obj5->next())
return nullptr;
return m_obj5.get();
}

private:
const RefPtr<RefCountable> m_obj1;
RefPtr<RefCountable> m_obj2;
const RefPtr<RefCountable> m_obj3;
const RefPtr<RefCountable> m_obj4;
const RefPtr<RefCountable> m_obj5;
};

void Foo::bar() {
@@ -23,6 +50,12 @@ void Foo::bar() {
auto* obj2 = m_obj2.get();
// expected-warning@-1{{Local variable 'obj2' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
obj2->method();
auto& obj3 = ensureObj3();
obj3.method();
auto& obj4 = ensureObj4();
// expected-warning@-1{{Local variable 'obj4' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
obj4.method();
auto* obj5 = ensureObj5();
}

} // namespace local_vars_const_refptr_member
Loading
Loading