[WebKit checkers] Recognize ensureFoo functions (llvm#119681)

rniwa · rniwa · commit 17cea609391f · 2025-02-07T02:59:34.000-08:00
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.
diff --git 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
@@ -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
@@ -33,6 +33,7 @@ class RawPtrRefCallArgsChecker
   mutable BugReporter *BR;
 
   TrivialFunctionAnalysis TFA;
+  EnsureFunctionAnalysis EFA;
 
 public:
   RawPtrRefCallArgsChecker(const char *description)
@@ -143,7 +144,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)) {
@@ -157,6 +158,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
@@ -169,6 +169,7 @@ class RawPtrRefLocalVarsChecker
     : public Checker<check::ASTDecl<TranslationUnitDecl>> {
   BugType Bug;
   mutable BugReporter *BR;
+  EnsureFunctionAnalysis EFA;
 
 public:
   RawPtrRefLocalVarsChecker(const char *description)
@@ -284,6 +285,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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
