[clang] [WebKit checkers] Recognize ensureFoo functions (PR #119681)
Ryosuke Niwa via cfe-commits
cfe-commits at lists.llvm.org
Thu Dec 12 13:33:17 PST 2024
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/119681
>From 0982f5ca3f9d1ea713b1e34b6e6b9d08ff65e6f1 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Thu, 12 Dec 2024 01:31:42 -0800
Subject: [PATCH 1/3] [WebKit checkers] Recognize ensureFoo functions
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.
---
.../Checkers/WebKit/ASTUtils.cpp | 42 +++++++++++++
.../StaticAnalyzer/Checkers/WebKit/ASTUtils.h | 10 +++
.../WebKit/RawPtrRefCallArgsChecker.cpp | 5 +-
.../WebKit/RawPtrRefLocalVarsChecker.cpp | 4 ++
.../WebKit/call-args-checked-const-member.cpp | 29 +++++++++
.../WebKit/call-args-counted-const-member.cpp | 29 +++++++++
.../local-vars-checked-const-member.cpp | 33 ++++++++++
.../local-vars-counted-const-member.cpp | 33 ++++++++++
.../Analysis/Checkers/WebKit/mock-types.h | 61 ++++++++++---------
9 files changed, 216 insertions(+), 30 deletions(-)
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 at -1{{Call argument for 'this' parameter is unchecked and unsafe}}
+ ensureObj3().method();
+ badEnsureObj4().method();
+ // expected-warning at -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 at -1{{Call argument for 'this' parameter is uncounted and unsafe}}
+ ensureObj3().method();
+ badEnsureObj4().method();
+ // expected-warning at -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 at -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 at -1{{Local variable 'obj2' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
obj2->method();
+ auto& obj3 = ensureObj3();
+ obj3.method();
+ auto& obj4 = ensureObj4();
+ // expected-warning at -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 at -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 at -1{{Local variable 'obj2' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
obj2->method();
+ auto& obj3 = ensureObj3();
+ obj3.method();
+ auto& obj4 = ensureObj4();
+ // expected-warning at -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
>From 3ab83561cb1a9a9ece90f022147a0dc775c9d144 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Thu, 12 Dec 2024 09:44:49 -0800
Subject: [PATCH 2/3] Fix formatting.
---
clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp | 2 +-
clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index 4086b267b6dad8..da6d4598cb0de3 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -202,7 +202,7 @@ bool EnsureFunctionAnalysis::isACallToEnsureFn(const clang::Expr *E) const {
auto *Callee = MCE->getDirectCallee();
if (!Callee)
return false;
- auto* Body = Callee->getBody();
+ auto *Body = Callee->getBody();
if (!Body)
return false;
auto [CacheIt, IsNew] = Cache.insert(std::make_pair(Callee, false));
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
index a4d46235dc9c47..b508043d0f37fd 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
@@ -67,8 +67,8 @@ 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.
+/// \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{};
>From f4226ce03cef1daa464e04387dd8a6f28c265d4e Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Thu, 12 Dec 2024 13:31:52 -0800
Subject: [PATCH 3/3] Fix tests. We need to make the decoy function opaque
since we now allow a function which just returns nullptr.
---
clang/test/Analysis/Checkers/WebKit/call-args-checked-ptr.cpp | 2 +-
clang/test/Analysis/Checkers/WebKit/call-args.cpp | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args-checked-ptr.cpp b/clang/test/Analysis/Checkers/WebKit/call-args-checked-ptr.cpp
index 072bceedcf9610..59f247d6d007c9 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args-checked-ptr.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args-checked-ptr.cpp
@@ -117,7 +117,7 @@ namespace null_ptr {
namespace ref_counted_lookalike {
struct Decoy {
- CheckedObj* get() { return nullptr; }
+ CheckedObj* get();
};
void foo() {
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
index 94efddeaf66cd8..0100fe68131405 100644
--- a/clang/test/Analysis/Checkers/WebKit/call-args.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp
@@ -117,7 +117,7 @@ namespace null_ptr {
namespace ref_counted_lookalike {
struct Decoy {
- RefCountable* get() { return nullptr; }
+ RefCountable* get();
};
void foo() {
More information about the cfe-commits
mailing list