[Webkit Checkers] Treat const member variables as a safe origin #115594
+404
−22
Conversation
This file contains 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
Treat const Ref, RefPtr, CheckedRef, CheckedPtr member variables as safe pointer origin in WebKit's local variable and call arguments checkers.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Ryosuke Niwa (rniwa) ChangesTreat const Ref, RefPtr, CheckedRef, CheckedPtr member variables as safe pointer origin in WebKit's local variable and call arguments checkers. Full diff: https://github.com/llvm/llvm-project/pull/115594.diff 7 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index 9d34dfd3cea636..ad2be6f6793cea 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -142,6 +142,12 @@ bool isASafeCallArg(const Expr *E) {
return true;
}
}
+ if (auto *ME = dyn_cast<MemberExpr>(E)) {
+ if (auto *D = ME->getMemberDecl()) {
+ auto T = D->getType();
+ return isSafePtrType(T) && T.isConstQualified();
+ }
+ }
// TODO: checker for method calls on non-refcounted objects
return isa<CXXThisExpr>(E);
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
index 06f8f43cee8151..be954ad3026c22 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp
@@ -281,6 +281,14 @@ class RawPtrRefLocalVarsChecker
if (isa<IntegerLiteral>(InitArgOrigin))
return true;
+ if (auto *ME = dyn_cast<MemberExpr>(InitArgOrigin)) {
+ if (auto *D = ME->getMemberDecl()) {
+ auto T = D->getType();
+ if (isSafePtrType(T) && T.isConstQualified())
+ 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
new file mode 100644
index 00000000000000..6a54ac7b2ca791
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/call-args-checked-const-member.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncheckedCallArgsChecker -verify %s
+
+#include "mock-types.h"
+
+namespace call_args_const_checkedptr_member {
+
+class Foo {
+public:
+ Foo();
+ void bar();
+
+private:
+ const CheckedPtr<CheckedObj> m_obj1;
+ CheckedPtr<CheckedObj> m_obj2;
+};
+
+void Foo::bar() {
+ m_obj1->method();
+ m_obj2->method();
+ // expected-warning@-1{{Call argument for 'this' parameter is unchecked and unsafe}}
+}
+
+} // namespace call_args_const_checkedptr_member
+
+namespace call_args_const_checkedref_member {
+
+class Foo {
+public:
+ Foo();
+ void bar();
+
+private:
+ const CheckedRef<CheckedObj> m_obj1;
+ CheckedRef<CheckedObj> m_obj2;
+};
+
+void Foo::bar() {
+ m_obj1->method();
+ m_obj2->method();
+ // expected-warning@-1{{Call argument for 'this' parameter is unchecked and unsafe}}
+}
+
+} // namespace call_args_const_checkedref_member
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
new file mode 100644
index 00000000000000..33af5ad9014de4
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/call-args-counted-const-member.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s
+
+#include "mock-types.h"
+
+namespace call_args_const_refptr_member {
+
+class Foo {
+public:
+ Foo();
+ void bar();
+
+private:
+ const RefPtr<RefCountable> m_obj1;
+ RefPtr<RefCountable> m_obj2;
+};
+
+void Foo::bar() {
+ m_obj1->method();
+ m_obj2->method();
+ // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
+}
+
+} // namespace call_args_const_refptr_member
+
+namespace call_args_const_ref_member {
+
+class Foo {
+public:
+ Foo();
+ void bar();
+
+private:
+ const Ref<RefCountable> m_obj1;
+ Ref<RefCountable> m_obj2;
+};
+
+void Foo::bar() {
+ m_obj1->method();
+ m_obj2->method();
+ // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
+}
+
+} // namespace call_args_const_ref_member
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
new file mode 100644
index 00000000000000..3deef66c271096
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/local-vars-checked-const-member.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncheckedLocalVarsChecker -verify %s
+
+#include "mock-types.h"
+#include "mock-system-header.h"
+
+void someFunction();
+
+namespace local_vars_const_checkedptr_member {
+
+class Foo {
+public:
+ Foo();
+ void bar();
+
+private:
+ const CheckedPtr<CheckedObj> m_obj1;
+ CheckedPtr<CheckedObj> m_obj2;
+};
+
+void Foo::bar() {
+ auto* obj1 = m_obj1.get();
+ obj1->method();
+ auto* obj2 = m_obj2.get();
+ // expected-warning@-1{{Local variable 'obj2' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
+ obj2->method();
+}
+
+} // namespace local_vars_const_checkedptr_member
+
+namespace local_vars_const_checkedref_member {
+
+class Foo {
+public:
+ Foo();
+ void bar();
+
+private:
+ const CheckedRef<CheckedObj> m_obj1;
+ CheckedRef<CheckedObj> m_obj2;
+};
+
+void Foo::bar() {
+ auto& obj1 = m_obj1.get();
+ obj1.method();
+ auto& obj2 = m_obj2.get();
+ // expected-warning@-1{{Local variable 'obj2' is unchecked and unsafe [alpha.webkit.UncheckedLocalVarsChecker]}}
+ obj2.method();
+}
+
+} // namespace local_vars_const_ref_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
new file mode 100644
index 00000000000000..6d6a7718244002
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/local-vars-counted-const-member.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedLocalVarsChecker -verify %s
+
+#include "mock-types.h"
+#include "mock-system-header.h"
+
+void someFunction();
+
+namespace local_vars_const_refptr_member {
+
+class Foo {
+public:
+ Foo();
+ void bar();
+
+private:
+ const RefPtr<RefCountable> m_obj1;
+ RefPtr<RefCountable> m_obj2;
+};
+
+void Foo::bar() {
+ auto* obj1 = m_obj1.get();
+ obj1->method();
+ auto* obj2 = m_obj2.get();
+ // expected-warning@-1{{Local variable 'obj2' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ obj2->method();
+}
+
+} // namespace local_vars_const_refptr_member
+
+namespace local_vars_const_ref_member {
+
+class Foo {
+public:
+ Foo();
+ void bar();
+
+private:
+ const Ref<RefCountable> m_obj1;
+ Ref<RefCountable> m_obj2;
+};
+
+void Foo::bar() {
+ auto& obj1 = m_obj1.get();
+ obj1.method();
+ auto& obj2 = m_obj2.get();
+ // expected-warning@-1{{Local variable 'obj2' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ obj2.method();
+}
+
+} // namespace local_vars_const_ref_member
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h
index 9c9326f0f11cfb..8f84589477bff1 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h
@@ -66,11 +66,10 @@ template <typename T, typename PtrTraits = RawPtrTraits<T>, typename RefDerefTra
t = o.t;
o.t = tmp;
}
- T &get() { return *PtrTraits::unwrap(t); }
- T *ptr() { return PtrTraits::unwrap(t); }
- T *operator->() { return PtrTraits::unwrap(t); }
- operator const T &() const { return *PtrTraits::unwrap(t); }
- operator T &() { return *PtrTraits::unwrap(t); }
+ T &get() const { return *PtrTraits::unwrap(t); }
+ T *ptr() const { return PtrTraits::unwrap(t); }
+ T *operator->() const { return PtrTraits::unwrap(t); }
+ operator T &() const { return *PtrTraits::unwrap(t); }
T* leakRef() { return PtrTraits::exchange(t, nullptr); }
};
@@ -102,9 +101,8 @@ template <typename T> struct RefPtr {
t = o.t;
o.t = tmp;
}
- T *get() { return t; }
- T *operator->() { return t; }
- const T *operator->() const { return t; }
+ T *get() const { return t; }
+ T *operator->() const { return t; }
T &operator*() { return *t; }
RefPtr &operator=(T *t) {
RefPtr o(t);
@@ -149,9 +147,9 @@ template <typename T> struct CheckedRef {
CheckedRef(T &t) : t(&t) { t.incrementCheckedPtrCount(); }
CheckedRef(const CheckedRef &o) : t(o.t) { if (t) t->incrementCheckedPtrCount(); }
~CheckedRef() { if (t) t->decrementCheckedPtrCount(); }
- T &get() { return *t; }
- T *ptr() { return t; }
- T *operator->() { return t; }
+ T &get() const { return *t; }
+ T *ptr() const { return t; }
+ T *operator->() const { return t; }
operator const T &() const { return *t; }
operator T &() { return *t; }
};
@@ -174,9 +172,8 @@ template <typename T> struct CheckedPtr {
if (t)
t->decrementCheckedPtrCount();
}
- T *get() { return t; }
- T *operator->() { return t; }
- const T *operator->() const { return t; }
+ T *get() const { return t; }
+ T *operator->() const { return t; }
T &operator*() { return *t; }
CheckedPtr &operator=(T *) { return *this; }
operator bool() const { return t; }
|
Also allow .get() and .ptr() on const member variable.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
t-rasmud
reviewed
Nov 15, 2024
| bool isOwnerPtrType(const clang::QualType T) { | ||
| return isPtrOfType(T, [](auto Name) { | ||
| return isRefType(Name) || isCheckedPtr(Name) || Name == "unique_ptr" || | ||
| Name == "UniqueRef" || Name == "LazyUniqueRef"; |
There was a problem hiding this comment.
Out of curiosity: Is this (UniqueRef and LazyUniqueRef) the exhaustive list of additional Webkit defined pointer owner types(I'm assuming)?
There was a problem hiding this comment.
There also BlockPtr to retain a Obj-C block and RetainPtr for retaining Obj-C objects. For now, static analyzer don't recognize those pointer types.
|
Thanks for the review! |
rniwa
added a commit
to rniwa/llvm-project
that referenced
this pull request
Nov 15, 2024
…#115594) Treat const Ref, RefPtr, CheckedRef, CheckedPtr member variables as safe pointer origin in WebKit's local variable and call arguments checkers.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/52/builds/3732 Here is the relevant piece of the build log for the reference |
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.
Treat const Ref, RefPtr, CheckedRef, CheckedPtr member variables as safe pointer origin in WebKit's local variable and call arguments checkers.