[WebKit checkers] Recognize ensureFoo functions #119681
Merged
+218
−32
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In WebKit, we often write Foo::ensureBar function which lazily initializes m_bar and returns a raw pointer or a raw reference to m_bar. Such a return value is safe to use for the duration of a member function call in Foo so long as m_bar is const so that it never gets unset or updated with a new value once it's initialized. This PR adds support for recognizing these types of functions and treating its return value as a safe origin of a function argument (including "this") or a local variable.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Ryosuke Niwa (rniwa) ChangesIn WebKit, we often write Foo::ensureBar function which lazily initializes m_bar and returns a raw pointer or a raw reference to m_bar. Such a return value is safe to use for the duration of a member function call in Foo so long as m_bar is const so that it never gets unset or updated with a new value once it's initialized. This PR adds support for recognizing these types of functions and treating its return value as a safe origin of a function argument (including "this") or a local variable. Full diff: https://github.com/llvm/llvm-project/pull/119681.diff 9 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index b3cd594a0f3529..4086b267b6dad8 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -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;
+}
+
} // namespace clang
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
index ddbef0fba04489..a4d46235dc9c47 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
@@ -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);
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
index ef2d42ccada65c..56fa72c100ec8c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
@@ -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;
});
}
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
index e0433c5c2c1a09..c225bbfcd46dc5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
@@ -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())) {
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args-checked-const-member.cpp b/clang/test/Analysis/Checkers/WebKit/call-args-checked-const-member.cpp
index 76f80a12c1703c..f7095606c77a4c 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args-checked-const-member.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args-checked-const-member.cpp
@@ -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
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args-counted-const-member.cpp b/clang/test/Analysis/Checkers/WebKit/call-args-counted-const-member.cpp
index b3296507a5c92d..215238a7fcf071 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args-counted-const-member.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args-counted-const-member.cpp
@@ -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
diff --git a/clang/test/Analysis/Checkers/WebKit/local-vars-checked-const-member.cpp b/clang/test/Analysis/Checkers/WebKit/local-vars-checked-const-member.cpp
index e52d1e735f6379..be04cf26be2e82 100644
--- a/clang/test/Analysis/Checkers/WebKit/local-vars-checked-const-member.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/local-vars-checked-const-member.cpp
@@ -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
diff --git a/clang/test/Analysis/Checkers/WebKit/local-vars-counted-const-member.cpp b/clang/test/Analysis/Checkers/WebKit/local-vars-counted-const-member.cpp
index 03d16285f88b53..e12c9b900a423c 100644
--- a/clang/test/Analysis/Checkers/WebKit/local-vars-counted-const-member.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/local-vars-counted-const-member.cpp
@@ -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
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h
index fb1ee51c7ec1de..f3bd20f8bcf603 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h
@@ -1,6 +1,34 @@
#ifndef mock_types_1103988513531
#define mock_types_1103988513531
+namespace std {
+
+template <typename T>
+class unique_ptr {
+private:
+ T *t;
+
+public:
+ unique_ptr() : t(nullptr) { }
+ unique_ptr(T *t) : t(t) { }
+ ~unique_ptr() {
+ if (t)
+ delete t;
+ }
+ template <typename U> unique_ptr(unique_ptr<U>&& u)
+ : t(u.t)
+ {
+ u.t = nullptr;
+ }
+ T *get() const { return t; }
+ T *operator->() const { return t; }
+ T &operator*() const { return *t; }
+ unique_ptr &operator=(T *) { return *this; }
+ explicit operator bool() const { return !!t; }
+};
+
+};
+
template<typename T>
struct RawPtrTraits {
using StorageType = T*;
@@ -103,7 +131,7 @@ template <typename T> struct RefPtr {
}
T *get() const { return t; }
T *operator->() const { return t; }
- T &operator*() { return *t; }
+ T &operator*() const { return *t; }
RefPtr &operator=(T *t) {
RefPtr o(t);
swap(o);
@@ -130,6 +158,7 @@ template <typename T> bool operator!=(const RefPtr<T> &, T &) { return false; }
struct RefCountable {
static Ref<RefCountable> create();
+ static std::unique_ptr<RefCountable> makeUnique();
void ref() {}
void deref() {}
void method();
@@ -176,7 +205,7 @@ template <typename T> struct CheckedPtr {
}
T *get() const { return t; }
T *operator->() const { return t; }
- T &operator*() { return *t; }
+ T &operator*() const { return *t; }
CheckedPtr &operator=(T *) { return *this; }
operator bool() const { return t; }
};
@@ -187,6 +216,7 @@ class CheckedObj {
void decrementCheckedPtrCount();
void method();
int trivial() { return 123; }
+ CheckedObj* next();
};
class RefCountableAndCheckable {
@@ -220,31 +250,4 @@ class UniqueRef {
UniqueRef &operator=(T &) { return *this; }
};
-namespace std {
-
-template <typename T>
-class unique_ptr {
-private:
- T *t;
-
-public:
- unique_ptr() : t(nullptr) { }
- unique_ptr(T *t) : t(t) { }
- ~unique_ptr() {
- if (t)
- delete t;
- }
- template <typename U> unique_ptr(unique_ptr<U>&& u)
- : t(u.t)
- {
- u.t = nullptr;
- }
- T *get() const { return t; }
- T *operator->() const { return t; }
- T &operator*() { return *t; }
- unique_ptr &operator=(T *) { return *this; }
-};
-
-};
-
#endif
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
t-rasmud
reviewed
Dec 12, 2024
| auto [CacheIt, IsNew] = Cache.insert(std::make_pair(Callee, false)); | ||
| if (IsNew) | ||
| CacheIt->second = EnsureFunctionVisitor().Visit(Body); | ||
| return CacheIt->second; |
There was a problem hiding this comment.
I like the fact you thought of performance optimization here!
There was a problem hiding this comment.
Yeah, I figured we should cache the result instead of keep analyzing the same function repeatedly.
…ow a function which just returns nullptr.
rniwa
added a commit
to rniwa/llvm-project
that referenced
this pull request
Feb 3, 2025
In WebKit, we often write Foo::ensureBar function which lazily initializes m_bar and returns a raw pointer or a raw reference to m_bar. Such a return value is safe to use for the duration of a member function call in Foo so long as m_bar is const so that it never gets unset or updated with a new value once it's initialized. This PR adds support for recognizing these types of functions and treating its return value as a safe origin of a function argument (including "this") or a local variable.
devincoughlin
pushed a commit
to swiftlang/llvm-project
that referenced
this pull request
Feb 25, 2025
In WebKit, we often write Foo::ensureBar function which lazily initializes m_bar and returns a raw pointer or a raw reference to m_bar. Such a return value is safe to use for the duration of a member function call in Foo so long as m_bar is const so that it never gets unset or updated with a new value once it's initialized. This PR adds support for recognizing these types of functions and treating its return value as a safe origin of a function argument (including "this") or a local variable.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In WebKit, we often write Foo::ensureBar function which lazily initializes m_bar and returns a raw pointer or a raw reference to m_bar. Such a return value is safe to use for the duration of a member function call in Foo so long as m_bar is const so that it never gets unset or updated with a new value once it's initialized.
This PR adds support for recognizing these types of functions and treating its return value as a safe origin of a function argument (including "this") or a local variable.