[WebKit checkers] Don't treat virtual functions as safe. (#129632)

Prior to this PR, WebKit checkers erroneously treated functions to be
safe if it has a trivial body even if it was marked as virtual. In the
case of a virtual function, it can have an override which does not pass
the triviality check so we must not make such an assumption.

This PR also restricts the allowed operator overloading while finding
the pointer origin to just operators on smart pointer types: Ref,
RefPtr, CheckedRef, CheckedPtr, RetainPtr, WeakPtr, WeakRef, unique_ptr,
and UniqueRef.

6 files changed, 49 insertions, 6 deletions

diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index c8151e932997..5e67cb29d08e 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -91,9 +91,17 @@ bool tryToFindPtrOrigin(
       }
 
       if (auto *operatorCall = dyn_cast<CXXOperatorCallExpr>(E)) {
-        if (operatorCall->getNumArgs() == 1) {
-          E = operatorCall->getArg(0);
-          continue;
+        if (auto *Callee = operatorCall->getDirectCallee()) {
+          auto ClsName = safeGetName(Callee->getParent());
+          if (isRefType(ClsName) || isCheckedPtr(ClsName) ||
+              isRetainPtr(ClsName) || ClsName == "unique_ptr" ||
+              ClsName == "UniqueRef" || ClsName == "WeakPtr" ||
+              ClsName == "WeakRef") {
+            if (operatorCall->getNumArgs() == 1) {
+              E = operatorCall->getArg(0);
+              continue;
+            }
+          }
         }
       }
 
@@ -215,7 +223,7 @@ bool EnsureFunctionAnalysis::isACallToEnsureFn(const clang::Expr *E) const {
   if (!Callee)
     return false;
   auto *Body = Callee->getBody();
-  if (!Body)
+  if (!Body || Callee->isVirtualAsWritten())
     return false;
   auto [CacheIt, IsNew] = Cache.insert(std::make_pair(Callee, false));
   if (IsNew)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index 8a304a07296f..419d9c232541 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -480,6 +480,10 @@ public:
   TrivialFunctionAnalysisVisitor(CacheTy &Cache) : Cache(Cache) {}
 
   bool IsFunctionTrivial(const Decl *D) {
+    if (auto *FnDecl = dyn_cast<FunctionDecl>(D)) {
+      if (FnDecl->isVirtualAsWritten())
+        return false;
+    }
     return WithCachedResult(D, [&]() {
       if (auto *CtorDecl = dyn_cast<CXXConstructorDecl>(D)) {
         for (auto *CtorInit : CtorDecl->inits()) {
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index fcc1a41dba78..323d47366588 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -131,6 +131,9 @@ 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 Name is RetainPtr or its variant, false if not.
+bool isRetainPtr(const std::string &Name);
+
 /// \returns true if \p M is getter of a ref-counted class, false if not.
 std::optional<bool> isGetterOfSafePtr(const clang::CXXMethodDecl *Method);
 
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
index d633fbcbd798..9d07c65da88a 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/RawPtrRefCallArgsChecker.cpp
@@ -172,7 +172,7 @@ public:
     if (BR->getSourceManager().isInSystemHeader(CE->getExprLoc()))
       return true;
 
-    if (Callee && TFA.isTrivial(Callee))
+    if (Callee && TFA.isTrivial(Callee) && !Callee->isVirtualAsWritten())
       return true;
 
     if (CE->getNumArgs() == 0)
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
index 52854cd10f68..07b6de21df80 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
@@ -464,4 +464,17 @@ namespace local_var_for_singleton {
     RefCountable* bar = singleton();
     RefCountable* baz = otherSingleton();
   }
+}
+
+namespace virtual_function {
+  struct SomeObject {
+    virtual RefCountable* provide() { return nullptr; }
+    virtual RefCountable* operator&() { return nullptr; }
+  };
+  void foo(SomeObject* obj) {
+    auto* bar = obj->provide();
+    // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+    auto* baz = &*obj;
+    // expected-warning@-1{{Local variable 'baz' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+  }
 }
\ No newline at end of file
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
index fe7ce158eb8b..0279e2c68ec6 100644
--- a/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-obj-arg.cpp
@@ -196,6 +196,10 @@ public:
   ComplexNumber& operator+();
 
   const Number& real() const { return realPart; }
+  const Number& complex() const;
+
+  void ref() const;
+  void deref() const;
 
 private:
   Number realPart;
@@ -240,6 +244,11 @@ public:
   using BaseType::BaseType;
 };
 
+struct OtherObj {
+  unsigned v { 0 };
+  OtherObj* children[4] { nullptr };
+};
+
 void __libcpp_verbose_abort(const char *__format, ...);
 
 class RefCounted {
@@ -375,7 +384,7 @@ public:
     double y;
   };
   void trivial68() { point pt = { 1.0 }; }
-  unsigned trivial69() { return offsetof(RefCounted, children); }
+  unsigned trivial69() { return offsetof(OtherObj, children); }
   DerivedNumber* trivial70() { [[clang::suppress]] return static_cast<DerivedNumber*>(number); }
 
   static RefCounted& singleton() {
@@ -467,6 +476,8 @@ public:
   unsigned nonTrivial22() { return ComplexNumber(123, "456").real().value(); }
   unsigned nonTrivial23() { return DerivedNumber("123").value(); }
   SomeType nonTrivial24() { return SomeType("123"); }
+  virtual void nonTrivial25() { }
+  virtual ComplexNumber* operator->() { return nullptr; }
 
   static unsigned s_v;
   unsigned v { 0 };
@@ -642,6 +653,10 @@ public:
     // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
     getFieldTrivial().nonTrivial24();
     // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
+    getFieldTrivial().nonTrivial25();
+    // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
+    getFieldTrivial()->complex();
+    // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
   }
 
   void setField(RefCounted*);