[clang] 5c20891 - [WebKit Checkers] Allow a guardian CheckedPtr/CheckedRef (#110222)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 25 08:53:00 PDT 2024
Author: Ryosuke Niwa
Date: 2024-10-25T08:52:56-07:00
New Revision: 5c20891b2bb60f82dd82a8e90b111f8c13a13ad4
URL: https://github.com/llvm/llvm-project/commit/5c20891b2bb60f82dd82a8e90b111f8c13a13ad4
DIFF: https://github.com/llvm/llvm-project/commit/5c20891b2bb60f82dd82a8e90b111f8c13a13ad4.diff
LOG: [WebKit Checkers] Allow a guardian CheckedPtr/CheckedRef (#110222)
This PR makes WebKit checkers allow a guardian variable which is
CheckedPtr or CheckedRef as in addition to RefPtr or Ref.
Added:
clang/test/Analysis/Checkers/WebKit/call-args-checked.cpp
Modified:
clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
clang/test/Analysis/Checkers/WebKit/mock-types.h
clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index b7b2f8a16f07b3..9d34dfd3cea636 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -17,6 +17,10 @@
namespace clang {
+bool isSafePtr(clang::CXXRecordDecl *Decl) {
+ return isRefCounted(Decl) || isCheckedPtr(Decl);
+}
+
bool tryToFindPtrOrigin(
const Expr *E, bool StopAtFirstRefCountedObj,
std::function<bool(const clang::Expr *, bool)> callback) {
@@ -31,7 +35,7 @@ bool tryToFindPtrOrigin(
}
if (auto *tempExpr = dyn_cast<CXXTemporaryObjectExpr>(E)) {
if (auto *C = tempExpr->getConstructor()) {
- if (auto *Class = C->getParent(); Class && isRefCounted(Class))
+ if (auto *Class = C->getParent(); Class && isSafePtr(Class))
return callback(E, true);
break;
}
@@ -56,7 +60,7 @@ bool tryToFindPtrOrigin(
if (StopAtFirstRefCountedObj) {
if (auto *ConversionFunc =
dyn_cast_or_null<FunctionDecl>(cast->getConversionFunction())) {
- if (isCtorOfRefCounted(ConversionFunc))
+ if (isCtorOfSafePtr(ConversionFunc))
return callback(E, true);
}
}
@@ -68,7 +72,7 @@ bool tryToFindPtrOrigin(
if (auto *call = dyn_cast<CallExpr>(E)) {
if (auto *memberCall = dyn_cast<CXXMemberCallExpr>(call)) {
if (auto *decl = memberCall->getMethodDecl()) {
- std::optional<bool> IsGetterOfRefCt = isGetterOfRefCounted(decl);
+ std::optional<bool> IsGetterOfRefCt = isGetterOfSafePtr(decl);
if (IsGetterOfRefCt && *IsGetterOfRefCt) {
E = memberCall->getImplicitObjectArgument();
if (StopAtFirstRefCountedObj) {
@@ -87,7 +91,7 @@ bool tryToFindPtrOrigin(
}
if (auto *callee = call->getDirectCallee()) {
- if (isCtorOfRefCounted(callee)) {
+ if (isCtorOfRefCounted(callee) || isCtorOfCheckedPtr(callee)) {
if (StopAtFirstRefCountedObj)
return callback(E, true);
@@ -95,7 +99,7 @@ bool tryToFindPtrOrigin(
continue;
}
- if (isRefType(callee->getReturnType()))
+ if (isSafePtrType(callee->getReturnType()))
return callback(E, true);
if (isSingleton(callee))
@@ -114,7 +118,7 @@ bool tryToFindPtrOrigin(
}
if (auto *ObjCMsgExpr = dyn_cast<ObjCMessageExpr>(E)) {
if (auto *Method = ObjCMsgExpr->getMethodDecl()) {
- if (isRefType(Method->getReturnType()))
+ if (isSafePtrType(Method->getReturnType()))
return callback(E, true);
}
}
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 71440e6d08a1c9..2293dcf1d4bd64 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -135,7 +135,16 @@ bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
|| FunctionName == "Identifier";
}
-bool isRefType(const clang::QualType T) {
+bool isCtorOfCheckedPtr(const clang::FunctionDecl *F) {
+ assert(F);
+ return isCheckedPtr(safeGetName(F));
+}
+
+bool isCtorOfSafePtr(const clang::FunctionDecl *F) {
+ return isCtorOfRefCounted(F) || isCtorOfCheckedPtr(F);
+}
+
+bool isSafePtrType(const clang::QualType T) {
QualType type = T;
while (!type.isNull()) {
if (auto *elaboratedT = type->getAs<ElaboratedType>()) {
@@ -145,7 +154,7 @@ bool isRefType(const clang::QualType T) {
if (auto *specialT = type->getAs<TemplateSpecializationType>()) {
if (auto *decl = specialT->getTemplateName().getAsTemplateDecl()) {
auto name = decl->getNameAsString();
- return isRefType(name);
+ return isRefType(name) || isCheckedPtr(name);
}
return false;
}
@@ -177,6 +186,12 @@ std::optional<bool> isUncounted(const CXXRecordDecl* Class)
return (*IsRefCountable);
}
+std::optional<bool> isUnchecked(const CXXRecordDecl *Class) {
+ if (isCheckedPtr(Class))
+ return false; // Cheaper than below
+ return isCheckedPtrCapable(Class);
+}
+
std::optional<bool> isUncountedPtr(const QualType T) {
if (T->isPointerType() || T->isReferenceType()) {
if (auto *CXXRD = T->getPointeeCXXRecordDecl())
@@ -185,8 +200,16 @@ std::optional<bool> isUncountedPtr(const QualType T) {
return false;
}
-std::optional<bool> isGetterOfRefCounted(const CXXMethodDecl* M)
-{
+std::optional<bool> isUnsafePtr(const QualType T) {
+ if (T->isPointerType() || T->isReferenceType()) {
+ if (auto *CXXRD = T->getPointeeCXXRecordDecl()) {
+ return isUncounted(CXXRD) || isUnchecked(CXXRD);
+ }
+ }
+ return false;
+}
+
+std::optional<bool> isGetterOfSafePtr(const CXXMethodDecl *M) {
assert(M);
if (isa<CXXMethodDecl>(M)) {
@@ -194,6 +217,9 @@ std::optional<bool> isGetterOfRefCounted(const CXXMethodDecl* M)
auto className = safeGetName(calleeMethodsClass);
auto method = safeGetName(M);
+ if (isCheckedPtr(className) && (method == "get" || method == "ptr"))
+ return true;
+
if ((isRefType(className) && (method == "get" || method == "ptr")) ||
((className == "String" || className == "AtomString" ||
className == "AtomStringImpl" || className == "UniqueString" ||
@@ -205,7 +231,12 @@ std::optional<bool> isGetterOfRefCounted(const CXXMethodDecl* M)
// FIXME: Currently allowing any Ref<T> -> whatever cast.
if (isRefType(className)) {
if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M))
- return isUncountedPtr(maybeRefToRawOperator->getConversionType());
+ return isUnsafePtr(maybeRefToRawOperator->getConversionType());
+ }
+
+ if (isCheckedPtr(className)) {
+ if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M))
+ return isUnsafePtr(maybeRefToRawOperator->getConversionType());
}
}
return false;
@@ -448,7 +479,7 @@ class TrivialFunctionAnalysisVisitor
if (!Callee)
return false;
- std::optional<bool> IsGetterOfRefCounted = isGetterOfRefCounted(Callee);
+ std::optional<bool> IsGetterOfRefCounted = isGetterOfSafePtr(Callee);
if (IsGetterOfRefCounted && *IsGetterOfRefCounted)
return true;
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index 8e6aadf63b6d67..4b41ca96e1df1d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -63,18 +63,30 @@ std::optional<bool> isUncounted(const clang::CXXRecordDecl* Class);
/// class, false if not, std::nullopt if inconclusive.
std::optional<bool> isUncountedPtr(const clang::QualType T);
-/// \returns true if Name is a RefPtr, Ref, or its variant, false if not.
-bool isRefType(const std::string &Name);
+/// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or its
+/// variant, false if not.
+bool isSafePtrType(const clang::QualType T);
/// \returns true if \p F creates ref-countable object from uncounted parameter,
/// false if not.
bool isCtorOfRefCounted(const clang::FunctionDecl *F);
-/// \returns true if \p T is RefPtr, Ref, or its variant, false if not.
-bool isRefType(const clang::QualType T);
+/// \returns true if \p F creates checked ptr object from uncounted parameter,
+/// false if not.
+bool isCtorOfCheckedPtr(const clang::FunctionDecl *F);
+
+/// \returns true if \p F creates ref-countable or checked ptr object from
+/// uncounted parameter, false if not.
+bool isCtorOfSafePtr(const clang::FunctionDecl *F);
+
+/// \returns true if \p Name is RefPtr, Ref, or its variant, false if not.
+bool isRefType(const std::string &Name);
+
+/// \returns true if \p Name is CheckedRef or CheckedPtr, false if not.
+bool isCheckedPtr(const std::string &Name);
/// \returns true if \p M is getter of a ref-counted class, false if not.
-std::optional<bool> isGetterOfRefCounted(const clang::CXXMethodDecl* Method);
+std::optional<bool> isGetterOfSafePtr(const clang::CXXMethodDecl *Method);
/// \returns true if \p F is a conversion between ref-countable or ref-counted
/// pointer types.
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
index cea3503fa2c314..1a5a7309a54f16 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
@@ -96,6 +96,8 @@ class UncountedCallArgsChecker
auto name = safeGetName(MD);
if (name == "ref" || name == "deref")
return;
+ if (name == "incrementPtrCount" || name == "decrementPtrCount")
+ return;
}
auto *E = MemberCallExpr->getImplicitObjectArgument();
QualType ArgType = MemberCallExpr->getObjectType().getCanonicalType();
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
index 81d21100de878d..5cdf047738abcb 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
@@ -227,6 +227,7 @@ class UncountedLocalVarsChecker
if (MaybeGuardianArgCXXRecord) {
if (MaybeGuardian->isLocalVarDecl() &&
(isRefCounted(MaybeGuardianArgCXXRecord) ||
+ isCheckedPtr(MaybeGuardianArgCXXRecord) ||
isRefcountedStringsHack(MaybeGuardian)) &&
isGuardedScopeEmbeddedInGuardianScope(
V, MaybeGuardian))
diff --git a/clang/test/Analysis/Checkers/WebKit/call-args-checked.cpp b/clang/test/Analysis/Checkers/WebKit/call-args-checked.cpp
new file mode 100644
index 00000000000000..49b6bfcd7cadfd
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/call-args-checked.cpp
@@ -0,0 +1,46 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s
+
+#include "mock-types.h"
+
+RefCountableAndCheckable* makeObj();
+CheckedRef<RefCountableAndCheckable> makeObjChecked();
+void someFunction(RefCountableAndCheckable*);
+
+namespace call_args_unchecked_uncounted {
+
+static void foo() {
+ someFunction(makeObj());
+ // expected-warning at -1{{Call argument is uncounted and unsafe [alpha.webkit.UncountedCallArgsChecker]}}
+}
+
+} // namespace call_args_checked
+
+namespace call_args_checked {
+
+static void foo() {
+ CheckedPtr<RefCountableAndCheckable> ptr = makeObj();
+ someFunction(ptr.get());
+}
+
+static void bar() {
+ someFunction(CheckedPtr { makeObj() }.get());
+}
+
+static void baz() {
+ someFunction(makeObjChecked().ptr());
+}
+
+} // namespace call_args_checked
+
+namespace call_args_default {
+
+void someFunction(RefCountableAndCheckable* = makeObj());
+// expected-warning at -1{{Call argument is uncounted and unsafe [alpha.webkit.UncountedCallArgsChecker]}}
+void otherFunction(RefCountableAndCheckable* = makeObjChecked().ptr());
+
+void foo() {
+ someFunction();
+ otherFunction();
+}
+
+}
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h
index 933b4c5e62a79c..8d8a90f0afae0e 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h
@@ -114,8 +114,8 @@ template <typename T> struct CheckedRef {
public:
CheckedRef() : t{} {};
- CheckedRef(T &t) : t(t) { t->incrementPtrCount(); }
- CheckedRef(const CheckedRef& o) : t(o.t) { if (t) t->incrementPtrCount(); }
+ CheckedRef(T &t) : t(&t) { t.incrementPtrCount(); }
+ CheckedRef(const CheckedRef &o) : t(o.t) { if (t) t->incrementPtrCount(); }
~CheckedRef() { if (t) t->decrementPtrCount(); }
T &get() { return *t; }
T *ptr() { return t; }
@@ -135,7 +135,7 @@ template <typename T> struct CheckedPtr {
if (t)
t->incrementPtrCount();
}
- CheckedPtr(Ref<T>&& o)
+ CheckedPtr(Ref<T> &&o)
: t(o.leakRef())
{ }
~CheckedPtr() {
@@ -156,4 +156,14 @@ class CheckedObj {
void decrementPtrCount();
};
+class RefCountableAndCheckable {
+public:
+ void incrementPtrCount() const;
+ void decrementPtrCount() const;
+ void ref() const;
+ void deref() const;
+ void method();
+ int trivial() { return 0; }
+};
+
#endif
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
index b5f6b8535bf418..1c0df42cdda663 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
@@ -290,6 +290,57 @@ void foo() {
} // namespace local_assignment_to_global
+namespace local_refcountable_checkable_object {
+
+RefCountableAndCheckable* provide_obj();
+
+void local_raw_ptr() {
+ RefCountableAndCheckable* a = nullptr;
+ // expected-warning at -1{{Local variable 'a' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ a = provide_obj();
+ a->method();
+}
+
+void local_checked_ptr() {
+ CheckedPtr<RefCountableAndCheckable> a = nullptr;
+ a = provide_obj();
+ a->method();
+}
+
+void local_var_with_guardian_checked_ptr() {
+ CheckedPtr<RefCountableAndCheckable> a = provide_obj();
+ {
+ auto* b = a.get();
+ b->method();
+ }
+}
+
+void local_var_with_guardian_checked_ptr_with_assignment() {
+ CheckedPtr<RefCountableAndCheckable> a = provide_obj();
+ {
+ RefCountableAndCheckable* b = a.get();
+ // expected-warning at -1{{Local variable 'b' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ b = provide_obj();
+ b->method();
+ }
+}
+
+void local_var_with_guardian_checked_ref() {
+ CheckedRef<RefCountableAndCheckable> a = *provide_obj();
+ {
+ RefCountableAndCheckable& b = a;
+ b.method();
+ }
+}
+
+void static_var() {
+ static RefCountableAndCheckable* a = nullptr;
+ // expected-warning at -1{{Static local variable 'a' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ a = provide_obj();
+}
+
+} // namespace local_refcountable_checkable_object
+
namespace local_var_in_recursive_function {
struct TreeNode {
More information about the cfe-commits
mailing list