https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/110222
>From 6e842a0135d097ffcb3c5991bc97543179972405 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa <rn...@webkit.org> Date: Fri, 27 Sep 2024 02:05:25 -0700 Subject: [PATCH 1/5] [WebKit Checkers] Allow a guardian CheckedPtr/CheckedRef This PR makes WebKit checkers allow a guardian variable which is CheckedPtr or CheckedRef as in addition to RefPtr or Ref. --- .../Checkers/WebKit/ASTUtils.cpp | 17 ++++--- .../Checkers/WebKit/PtrTypesSemantics.cpp | 44 ++++++++++++++-- .../Checkers/WebKit/PtrTypesSemantics.h | 17 +++++-- .../WebKit/UncountedCallArgsChecker.cpp | 2 + .../WebKit/UncountedLocalVarsChecker.cpp | 1 + .../Checkers/WebKit/call-args-checked.cpp | 46 +++++++++++++++++ .../Analysis/Checkers/WebKit/mock-types.h | 16 ++++-- .../Checkers/WebKit/uncounted-local-vars.cpp | 51 +++++++++++++++++++ 8 files changed, 177 insertions(+), 17 deletions(-) create mode 100644 clang/test/Analysis/Checkers/WebKit/call-args-checked.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp index 394cb26f03cf99..1b7614d3feeca5 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,8 @@ bool tryToFindPtrOrigin( if (StopAtFirstRefCountedObj) { if (auto *ConversionFunc = dyn_cast_or_null<FunctionDecl>(cast->getConversionFunction())) { - if (isCtorOfRefCounted(ConversionFunc)) + if (isCtorOfRefCounted(ConversionFunc) || + isCtorOfCheckedPtr(ConversionFunc)) return callback(E, true); } } @@ -68,7 +73,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 +92,7 @@ bool tryToFindPtrOrigin( } if (auto *callee = call->getDirectCallee()) { - if (isCtorOfRefCounted(callee)) { + if (isCtorOfRefCounted(callee) || isCtorOfCheckedPtr(callee)) { if (StopAtFirstRefCountedObj) return callback(E, true); @@ -95,7 +100,7 @@ bool tryToFindPtrOrigin( continue; } - if (isRefType(callee->getReturnType())) + if (isSafePtrType(callee->getReturnType())) return callback(E, true); if (isSingleton(callee)) @@ -109,7 +114,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 4d145be808f6d8..b40e470dc71e03 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -135,7 +135,12 @@ 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 isSafePtrType(const clang::QualType T) { QualType type = T; while (!type.isNull()) { if (auto *elaboratedT = type->getAs<ElaboratedType>()) { @@ -145,7 +150,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 +182,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 Type* T) { assert(T); @@ -189,7 +200,18 @@ std::optional<bool> isUncountedPtr(const Type* T) return false; } -std::optional<bool> isGetterOfRefCounted(const CXXMethodDecl* M) +std::optional<bool> isUnsafePtr(const Type *T) { + assert(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); @@ -198,6 +220,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" || @@ -211,7 +236,16 @@ std::optional<bool> isGetterOfRefCounted(const CXXMethodDecl* M) if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M)) { if (auto *targetConversionType = maybeRefToRawOperator->getConversionType().getTypePtrOrNull()) { - return isUncountedPtr(targetConversionType); + return isUnsafePtr(targetConversionType); + } + } + } + + if (isCheckedPtr(className)) { + if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M)) { + if (auto *targetConversionType = + maybeRefToRawOperator->getConversionType().getTypePtrOrNull()) { + return isUnsafePtr(targetConversionType); } } } @@ -464,7 +498,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 3528c52a7d659d..2b56edddc86364 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -63,18 +63,29 @@ std::optional<bool> isUncounted(const clang::CXXRecordDecl* Class); /// class, false if not, std::nullopt if inconclusive. std::optional<bool> isUncountedPtr(const clang::Type* 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 F creates ref-countable object from uncounted parameter, +/// false if not. +bool isCtorOfCheckedPtr(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 T is RefPtr, Ref, or its variant, false if not. bool isRefType(const clang::QualType T); /// \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 0ed93ab26bf5ca..084b789a008276 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 9d0a3bb5da7325..c389820297b3e0 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp @@ -231,6 +231,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@-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@-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 25776870dd3ae0..dabe699c44283a 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp @@ -289,3 +289,54 @@ void foo() { } } // namespace local_assignment_to_global + +namespace local_refcountable_checkable_object { + +RefCountableAndCheckable* provide_obj(); + +void local_raw_ptr() { + RefCountableAndCheckable* a = nullptr; + // expected-warning@-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@-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@-1{{Static local variable 'a' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + a = provide_obj(); +} + +} // namespace local_refcountable_checkable_object >From 04ea90c6d5730380515ed176179e29e834ad7fb2 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa <rn...@webkit.org> Date: Fri, 27 Sep 2024 11:38:00 -0700 Subject: [PATCH 2/5] Fix formatting. --- clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp | 3 +-- clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index b40e470dc71e03..249263d4fc6770 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -211,8 +211,7 @@ std::optional<bool> isUnsafePtr(const Type *T) { return false; } -std::optional<bool> isGetterOfSafePtr(const CXXMethodDecl* M) -{ +std::optional<bool> isGetterOfSafePtr(const CXXMethodDecl* M) { assert(M); if (isa<CXXMethodDecl>(M)) { diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index 2b56edddc86364..ed6122c0a8355d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -85,7 +85,7 @@ bool isCheckedPtr(const std::string &Name); bool isRefType(const clang::QualType T); /// \returns true if \p M is getter of a ref-counted class, false if not. -std::optional<bool> isGetterOfSafePtr(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. >From 7981562d588e9c368c9ff116ee163d5a03aad603 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa <rn...@webkit.org> Date: Fri, 27 Sep 2024 11:43:29 -0700 Subject: [PATCH 3/5] Fix formatting 2. --- clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 249263d4fc6770..c9489101f386e5 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -211,7 +211,7 @@ std::optional<bool> isUnsafePtr(const Type *T) { return false; } -std::optional<bool> isGetterOfSafePtr(const CXXMethodDecl* M) { +std::optional<bool> isGetterOfSafePtr(const CXXMethodDecl *M) { assert(M); if (isa<CXXMethodDecl>(M)) { >From e4aa98b17025bf5d159daa2e097b55a937ee1570 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa <rn...@webkit.org> Date: Thu, 17 Oct 2024 20:53:16 -0700 Subject: [PATCH 4/5] Fix merge error --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 20 ++++--------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index ba920b2c659003..439736b445ac83 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -196,9 +196,7 @@ std::optional<bool> isUncountedPtr(const QualType T) { return false; } -std::optional<bool> isUnsafePtr(const Type *T) { - assert(T); - +std::optional<bool> isUnsafePtr(const QualType T) { if (T->isPointerType() || T->isReferenceType()) { if (auto *CXXRD = T->getPointeeCXXRecordDecl()) { return isUncounted(CXXRD) || isUnchecked(CXXRD); @@ -228,23 +226,13 @@ std::optional<bool> isGetterOfSafePtr(const CXXMethodDecl *M) { // Ref<T> -> T conversion // FIXME: Currently allowing any Ref<T> -> whatever cast. if (isRefType(className)) { - if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M)) { - if (auto *targetConversionType = - maybeRefToRawOperator->getConversionType().getTypePtrOrNull()) { - return isUnsafePtr(targetConversionType); - } - } + if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M)) + return isUnsafePtr(maybeRefToRawOperator->getConversionType()); } if (isCheckedPtr(className)) { - if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M)) { - if (auto *targetConversionType = - maybeRefToRawOperator->getConversionType().getTypePtrOrNull()) { - return isUnsafePtr(targetConversionType); - } - } if (auto *maybeRefToRawOperator = dyn_cast<CXXConversionDecl>(M)) - return isUncountedPtr(maybeRefToRawOperator->getConversionType()); + return isUnsafePtr(maybeRefToRawOperator->getConversionType()); } } return false; >From a89a4d8fbd587680d22bf7ccf23ef02a8c8191ac Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa <rn...@webkit.org> Date: Thu, 17 Oct 2024 21:11:23 -0700 Subject: [PATCH 5/5] Fix more merge errors --- clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp index a0443fd92e5387..1c0df42cdda663 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp @@ -290,7 +290,6 @@ void foo() { } // namespace local_assignment_to_global -<<<<<<< HEAD namespace local_refcountable_checkable_object { RefCountableAndCheckable* provide_obj(); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits