Author: Ryosuke Niwa Date: 2024-12-13T01:48:29-08:00 New Revision: 05860f9b384b9b8f8bb01fa8984dbc2833669a27
URL: https://github.com/llvm/llvm-project/commit/05860f9b384b9b8f8bb01fa8984dbc2833669a27 DIFF: https://github.com/llvm/llvm-project/commit/05860f9b384b9b8f8bb01fa8984dbc2833669a27.diff LOG: [WebKit checkers] Recognize ensureFoo functions (#119681) 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. Added: Modified: clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefLocalVarsChecker.cpp clang/test/Analysis/Checkers/WebKit/call-args-checked-const-member.cpp clang/test/Analysis/Checkers/WebKit/call-args-checked-ptr.cpp clang/test/Analysis/Checkers/WebKit/call-args-counted-const-member.cpp clang/test/Analysis/Checkers/WebKit/call-args.cpp clang/test/Analysis/Checkers/WebKit/local-vars-checked-const-member.cpp clang/test/Analysis/Checkers/WebKit/local-vars-counted-const-member.cpp clang/test/Analysis/Checkers/WebKit/mock-types.h Removed: ################################################################################ diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp index 69b63240d2075e..abf5d3ec193a41 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..b508043d0f37fd 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 bb580b06e2c53f..d5866683995022 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-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-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/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp index 9920690746dafc..2146eae9975b93 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() { 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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
