Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[alpha.webkit.UncountedCallArgsChecker] Don't assume local variables are safe & treat guarded local variable as safe function arguments #82305

Closed
wants to merge 2 commits into from

Conversation

rniwa
Copy link
Contributor

@rniwa rniwa commented Feb 20, 2024

[alpha.webkit.UncountedCallArgsChecker] Treat guarded local variable as safe function arguments

This PR extracts the code in UncountedLocalVarsChecker to identify a variable declaration which
has a guardian Ref / RefPtr variable as isVarDeclGuardedInit and integrates it with
alpha.webkit.UncountedCallArgsChecker so that we treat local variables that are "guarded" by
a Ref / RefPtr as safe function arguments.

[alpha.webkit.UncountedCallArgsChecker] Don't assume local variables are safe.

This PR makes alpha.webkit.UncountedCallArgsChecker no longer assume that every local variable to be safe.
Instead, we try to validate the local variable's origin in tryToFindPtrOrigin. To do this, this PR extracts
the code to decide whether a given type is a specialization of Ref / RefPtr as isTypeRefCounted and
calls it in tryToFindPtrOrigin for the declared variable. This PR also fixes a bug in the same function
that we were not detecting DeducedTemplateSpecializationType as a safe origin.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Feb 20, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 20, 2024

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Ryosuke Niwa (rniwa)

Changes

[alpha.webkit.UncountedCallArgsChecker] Treat guarded local variable as safe function arguments

This PR extracts the code in UncountedLocalVarsChecker to identify a variable declaration which
has a guardian Ref / RefPtr variable as isVarDeclGuardedInit and integrates it with
alpha.webkit.UncountedCallArgsChecker so that we treat local variables that are "guarded" by
a Ref / RefPtr as safe function arguments.

[alpha.webkit.UncountedCallArgsChecker] Don't assume local variables are safe.

This PR makes alpha.webkit.UncountedCallArgsChecker no longer assume that every local variable to be safe.
Instead, we try to validate the local variable's origin in tryToFindPtrOrigin. To do this, this PR extracts
the code to decide whether a given type is a specialization of Ref / RefPtr as isTypeRefCounted and
calls it in tryToFindPtrOrigin for the declared variable. This PR also fixes a bug in the same function
that we were not detecting DeducedTemplateSpecializationType as a safe origin.


Full diff: https://github.com/llvm/llvm-project/pull/82305.diff

8 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp (+137-1)
  • (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h (+5)
  • (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp (+10-1)
  • (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h (+5-1)
  • (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp (+10)
  • (modified) clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp (+1-104)
  • (added) clang/test/Analysis/Checkers/WebKit/guarded-uncounted-obj-arg.cpp (+65)
  • (modified) clang/test/Analysis/Checkers/WebKit/mock-types.h (+23-1)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index 1a9d6d3127fb7f..4597dd4b067cb5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -12,6 +12,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/ParentMapContext.h"
 #include <optional>
 
 namespace clang {
@@ -27,6 +28,13 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
       E = tempExpr->getSubExpr();
       continue;
     }
+    if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
+      auto *decl = DRE->getFoundDecl();
+      if (auto *VD = dyn_cast<VarDecl>(decl)) {
+        if (isTypeRefCounted(VD->getType()))
+          return {E, true};
+      }
+    }
     if (auto *cast = dyn_cast<CastExpr>(E)) {
       if (StopAtFirstRefCountedObj) {
         if (auto *ConversionFunc =
@@ -95,11 +103,139 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
   return {E, false};
 }
 
+bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded,
+                                           const VarDecl *MaybeGuardian) {
+  assert(Guarded);
+  assert(MaybeGuardian);
+
+  if (!MaybeGuardian->isLocalVarDecl())
+    return false;
+
+  const CompoundStmt *guardiansClosestCompStmtAncestor = nullptr;
+
+  ASTContext &ctx = MaybeGuardian->getASTContext();
+
+  for (DynTypedNodeList guardianAncestors = ctx.getParents(*MaybeGuardian);
+       !guardianAncestors.empty();
+       guardianAncestors = ctx.getParents(
+           *guardianAncestors
+                .begin()) // FIXME - should we handle all of the parents?
+  ) {
+    for (auto &guardianAncestor : guardianAncestors) {
+      if (auto *CStmtParentAncestor = guardianAncestor.get<CompoundStmt>()) {
+        guardiansClosestCompStmtAncestor = CStmtParentAncestor;
+        break;
+      }
+    }
+    if (guardiansClosestCompStmtAncestor)
+      break;
+  }
+
+  if (!guardiansClosestCompStmtAncestor)
+    return false;
+
+  // We need to skip the first CompoundStmt to avoid situation when guardian is
+  // defined in the same scope as guarded variable.
+  bool HaveSkippedFirstCompoundStmt = false;
+  for (DynTypedNodeList guardedVarAncestors = ctx.getParents(*Guarded);
+       !guardedVarAncestors.empty();
+       guardedVarAncestors = ctx.getParents(
+           *guardedVarAncestors
+                .begin()) // FIXME - should we handle all of the parents?
+  ) {
+    for (auto &guardedVarAncestor : guardedVarAncestors) {
+      if (guardedVarAncestor.get<ForStmt>()) {
+        if (!HaveSkippedFirstCompoundStmt)
+          HaveSkippedFirstCompoundStmt = true;
+        continue;
+      }
+      if (guardedVarAncestor.get<IfStmt>()) {
+        if (!HaveSkippedFirstCompoundStmt)
+          HaveSkippedFirstCompoundStmt = true;
+        continue;
+      }
+      if (guardedVarAncestor.get<WhileStmt>()) {
+        if (!HaveSkippedFirstCompoundStmt)
+          HaveSkippedFirstCompoundStmt = true;
+        continue;
+      }
+      if (auto *CStmtAncestor = guardedVarAncestor.get<CompoundStmt>()) {
+        if (!HaveSkippedFirstCompoundStmt) {
+          HaveSkippedFirstCompoundStmt = true;
+          continue;
+        }
+        if (CStmtAncestor == guardiansClosestCompStmtAncestor)
+          return true;
+      }
+    }
+  }
+
+  return false;
+}
+
+// FIXME: should be defined by annotations in the future
+bool isRefcountedStringsHack(const VarDecl *V) {
+  assert(V);
+  auto safeClass = [](const std::string &className) {
+    return className == "String" || className == "AtomString" ||
+           className == "UniquedString" || className == "Identifier";
+  };
+  QualType QT = V->getType();
+  auto *T = QT.getTypePtr();
+  if (auto *CXXRD = T->getAsCXXRecordDecl()) {
+    if (safeClass(safeGetName(CXXRD)))
+      return true;
+  }
+  if (T->isPointerType() || T->isReferenceType()) {
+    if (auto *CXXRD = T->getPointeeCXXRecordDecl()) {
+      if (safeClass(safeGetName(CXXRD)))
+        return true;
+    }
+  }
+  return false;
+}
+
+bool isVarDeclGuardedInit(const VarDecl *V, const Expr *InitE)
+{
+  // "this" parameter like any other argument is considered safe.
+  if (isa<CXXThisExpr>(InitE))
+    return true;
+
+  if (auto *Ref = llvm::dyn_cast<DeclRefExpr>(InitE)) {
+    if (auto *MaybeGuardian =
+            dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
+      // Parameters are guaranteed to be safe for the duration of the call.
+      if (isa<ParmVarDecl>(MaybeGuardian))
+        return true;
+
+      const auto *MaybeGuardianArgType =
+          MaybeGuardian->getType().getTypePtr();
+      if (!MaybeGuardianArgType)
+        return false;
+
+      const CXXRecordDecl *const MaybeGuardianArgCXXRecord =
+          MaybeGuardianArgType->getAsCXXRecordDecl();
+
+      if (!MaybeGuardianArgCXXRecord)
+        return false;
+
+      if (MaybeGuardian->isLocalVarDecl() &&
+          (isRefCounted(MaybeGuardianArgCXXRecord) ||
+           isRefcountedStringsHack(MaybeGuardian)) &&
+          isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian)) {
+        return true;
+      }
+    }
+  }
+
+  return false;
+}
+
 bool isASafeCallArg(const Expr *E) {
   assert(E);
   if (auto *Ref = dyn_cast<DeclRefExpr>(E)) {
     if (auto *D = dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
-      if (isa<ParmVarDecl>(D) || D->isLocalVarDecl())
+      if (isa<ParmVarDecl>(D))
         return true;
     }
   }
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
index e35ea4ef05dd17..bdbe295df6ea39 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
@@ -53,6 +53,11 @@ class Expr;
 std::pair<const clang::Expr *, bool>
 tryToFindPtrOrigin(const clang::Expr *E, bool StopAtFirstRefCountedObj);
 
+// Returns true if \P V is a variable declaration and \p E is its
+// initialization expression, and there is a "guardian" RefPtr or RefPtr
+// protecting the same value.
+bool isVarDeclGuardedInit(const VarDecl *V, const clang::Expr *InitE);
+
 /// For \p E referring to a ref-countable/-counted pointer/reference we return
 /// whether it's a safe call argument. Examples: function parameter or
 /// this-pointer. The logic relies on the set of recursive rules we enforce for
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index a7891d2da07c18..58212625349889 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -122,12 +122,21 @@ bool isCtorOfRefCounted(const clang::FunctionDecl *F) {
 
 bool isReturnValueRefCounted(const clang::FunctionDecl *F) {
   assert(F);
-  QualType type = F->getReturnType();
+  return isTypeRefCounted(F->getReturnType());
+}
+
+bool isTypeRefCounted(QualType type) {
   while (!type.isNull()) {
     if (auto *elaboratedT = type->getAs<ElaboratedType>()) {
       type = elaboratedT->desugar();
       continue;
     }
+    if (auto *deduced = type->getAs<DeducedTemplateSpecializationType>()) {
+      if (auto *decl = deduced->getTemplateName().getAsTemplateDecl()) {
+        auto name = decl->getNameAsString();
+        return name == "Ref" || name == "RefPtr";
+      }
+    }
     if (auto *specialT = type->getAs<TemplateSpecializationType>()) {
       if (auto *decl = specialT->getTemplateName().getAsTemplateDecl()) {
         auto name = decl->getNameAsString();
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
index e07cd31395747d..91865a85b3040d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -9,7 +9,8 @@
 #ifndef LLVM_CLANG_ANALYZER_WEBKIT_PTRTYPESEMANTICS_H
 #define LLVM_CLANG_ANALYZER_WEBKIT_PTRTYPESEMANTICS_H
 
-#include "llvm/ADT/APInt.h"
+#include "clang/AST/Type.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseMap.h"
 #include <optional>
 
@@ -55,6 +56,9 @@ bool isCtorOfRefCounted(const clang::FunctionDecl *F);
 /// \returns true if \p F returns a ref-counted object, false if not.
 bool isReturnValueRefCounted(const clang::FunctionDecl *F);
 
+/// \returns true if \p type is ref-counted object, false if not.
+bool isTypeRefCounted(QualType type);
+
 /// \returns true if \p M is getter of a ref-counted class, false if not.
 std::optional<bool> isGetterOfRefCounted(const clang::CXXMethodDecl* Method);
 
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
index 8d344f9b63961a..75ea5d99435b7a 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
@@ -137,6 +137,16 @@ class UncountedCallArgsChecker
       return true;
     }
 
+    if (auto *DRE = dyn_cast<DeclRefExpr>(ArgOrigin.first)) {
+      auto *Decl = DRE->getFoundDecl();
+      if (auto *VD = dyn_cast<VarDecl>(Decl)) {
+        std::pair<const clang::Expr *, bool> ArgOrigin =
+            tryToFindPtrOrigin(VD->getInit(), false);
+        if (ArgOrigin.first && isVarDeclGuardedInit(VD, ArgOrigin.first))
+          return true;
+      }
+    }
+
     return isASafeCallArg(ArgOrigin.first);
   }
 
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
index 5a72f53b12edaa..9b976c0947edcd 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
@@ -48,83 +48,6 @@ bool isDeclaredInForOrIf(const VarDecl *Var) {
   return false;
 }
 
-// FIXME: should be defined by anotations in the future
-bool isRefcountedStringsHack(const VarDecl *V) {
-  assert(V);
-  auto safeClass = [](const std::string &className) {
-    return className == "String" || className == "AtomString" ||
-           className == "UniquedString" || className == "Identifier";
-  };
-  QualType QT = V->getType();
-  auto *T = QT.getTypePtr();
-  if (auto *CXXRD = T->getAsCXXRecordDecl()) {
-    if (safeClass(safeGetName(CXXRD)))
-      return true;
-  }
-  if (T->isPointerType() || T->isReferenceType()) {
-    if (auto *CXXRD = T->getPointeeCXXRecordDecl()) {
-      if (safeClass(safeGetName(CXXRD)))
-        return true;
-    }
-  }
-  return false;
-}
-
-bool isGuardedScopeEmbeddedInGuardianScope(const VarDecl *Guarded,
-                                           const VarDecl *MaybeGuardian) {
-  assert(Guarded);
-  assert(MaybeGuardian);
-
-  if (!MaybeGuardian->isLocalVarDecl())
-    return false;
-
-  const CompoundStmt *guardiansClosestCompStmtAncestor = nullptr;
-
-  ASTContext &ctx = MaybeGuardian->getASTContext();
-
-  for (DynTypedNodeList guardianAncestors = ctx.getParents(*MaybeGuardian);
-       !guardianAncestors.empty();
-       guardianAncestors = ctx.getParents(
-           *guardianAncestors
-                .begin()) // FIXME - should we handle all of the parents?
-  ) {
-    for (auto &guardianAncestor : guardianAncestors) {
-      if (auto *CStmtParentAncestor = guardianAncestor.get<CompoundStmt>()) {
-        guardiansClosestCompStmtAncestor = CStmtParentAncestor;
-        break;
-      }
-    }
-    if (guardiansClosestCompStmtAncestor)
-      break;
-  }
-
-  if (!guardiansClosestCompStmtAncestor)
-    return false;
-
-  // We need to skip the first CompoundStmt to avoid situation when guardian is
-  // defined in the same scope as guarded variable.
-  bool HaveSkippedFirstCompoundStmt = false;
-  for (DynTypedNodeList guardedVarAncestors = ctx.getParents(*Guarded);
-       !guardedVarAncestors.empty();
-       guardedVarAncestors = ctx.getParents(
-           *guardedVarAncestors
-                .begin()) // FIXME - should we handle all of the parents?
-  ) {
-    for (auto &guardedVarAncestor : guardedVarAncestors) {
-      if (auto *CStmtAncestor = guardedVarAncestor.get<CompoundStmt>()) {
-        if (!HaveSkippedFirstCompoundStmt) {
-          HaveSkippedFirstCompoundStmt = true;
-          continue;
-        }
-        if (CStmtAncestor == guardiansClosestCompStmtAncestor)
-          return true;
-      }
-    }
-  }
-
-  return false;
-}
-
 class UncountedLocalVarsChecker
     : public Checker<check::ASTDecl<TranslationUnitDecl>> {
   BugType Bug{this,
@@ -181,35 +104,9 @@ class UncountedLocalVarsChecker
       if (!InitArgOrigin)
         return;
 
-      if (isa<CXXThisExpr>(InitArgOrigin))
+      if (isVarDeclGuardedInit(V, InitArgOrigin))
         return;
 
-      if (auto *Ref = llvm::dyn_cast<DeclRefExpr>(InitArgOrigin)) {
-        if (auto *MaybeGuardian =
-                dyn_cast_or_null<VarDecl>(Ref->getFoundDecl())) {
-          const auto *MaybeGuardianArgType =
-              MaybeGuardian->getType().getTypePtr();
-          if (!MaybeGuardianArgType)
-            return;
-          const CXXRecordDecl *const MaybeGuardianArgCXXRecord =
-              MaybeGuardianArgType->getAsCXXRecordDecl();
-          if (!MaybeGuardianArgCXXRecord)
-            return;
-
-          if (MaybeGuardian->isLocalVarDecl() &&
-              (isRefCounted(MaybeGuardianArgCXXRecord) ||
-               isRefcountedStringsHack(MaybeGuardian)) &&
-              isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian)) {
-            return;
-          }
-
-          // Parameters are guaranteed to be safe for the duration of the call
-          // by another checker.
-          if (isa<ParmVarDecl>(MaybeGuardian))
-            return;
-        }
-      }
-
       reportBug(V);
     }
   }
diff --git a/clang/test/Analysis/Checkers/WebKit/guarded-uncounted-obj-arg.cpp b/clang/test/Analysis/Checkers/WebKit/guarded-uncounted-obj-arg.cpp
new file mode 100644
index 00000000000000..63611bace72d1f
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/guarded-uncounted-obj-arg.cpp
@@ -0,0 +1,65 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s
+
+#include "mock-types.h"
+
+class Number {
+public:
+  Number();
+  ~Number();
+};
+
+int someFunction();
+
+class RefCounted {
+public:
+  void ref() const;
+  void deref() const;
+  void someMethod();
+  bool isDerived() const { return false; }
+};
+
+class Derived : public RefCounted {
+public:
+  void otherMethod();
+  bool isDerived() const { return true; }
+};
+
+template <typename S>
+struct TypeCastTraits<const Derived, S> {
+  static bool isOfType(const S &arg) { return arg.isDerived(); }
+};
+
+RefCounted *object();
+void someFunction(const RefCounted&);
+
+void test() {
+  RefPtr obj = object();
+
+  if (!obj->isDerived()) {
+    auto* base = obj.get();
+    base->someMethod();
+  }
+
+  if (auto *derived = dynamicDowncast<Derived>(*obj))
+    derived->otherMethod();
+
+  while (auto *derived = dynamicDowncast<Derived>(*obj)) {
+    derived->otherMethod();
+    break;
+  }
+
+  for (auto *derived = dynamicDowncast<Derived>(*obj); true; ) {
+    derived->otherMethod();
+    break;
+  }
+
+  do {
+    auto *derived = dynamicDowncast<Derived>(*obj);
+    derived->otherMethod();
+  } while (0);
+
+  auto* derived = dynamicDowncast<Derived>(*obj);
+  derived->otherMethod();
+  // expected-warning@-1{{Call argument for 'this' parameter is uncounted and unsafe}}
+}
+
diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h
index 82db67bb031dd6..82d78d8a20f0e1 100644
--- a/clang/test/Analysis/Checkers/WebKit/mock-types.h
+++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h
@@ -63,6 +63,28 @@ struct RefCountable {
   void deref() {}
 };
 
-template <typename T> T *downcast(T *t) { return t; }
+template <typename T, typename S>
+struct TypeCastTraits {
+  static bool isOfType(S&);
+};
+
+// Type checking function, to use before casting with downcast<>().
+template <typename T, typename S>
+inline bool is(const S &source) {
+    return TypeCastTraits<const T, const S>::isOfType(source);
+}
+
+template <typename T, typename S>
+inline bool is(S *source) {
+    return source && TypeCastTraits<const T, const S>::isOfType(*source);
+}
+
+template <typename T, typename S> T *downcast(S *t) { return static_cast<T*>(t); }
+template <typename T, typename S> T *dynamicDowncast(S &t) {
+  return is<T>(t) ? &static_cast<T&>(t) : nullptr;
+}
+template <typename T, typename S> T *dynamicDowncast(S *t) {
+  return is<T>(t) ? static_cast<T*>(t) : nullptr;
+}
 
 #endif

Copy link

github-actions bot commented Feb 20, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@rniwa rniwa force-pushed the check-call-arg-local-vars branch 2 times, most recently from 481becf to 321ad9e Compare February 20, 2024 06:05
…are safe.

This PR makes alpha.webkit.UncountedCallArgsChecker no longer assume that every local variable to be safe.
Instead, we try to validate the local variable's origin in tryToFindPtrOrigin. To do this, this PR extracts
the code to decide whether a given type is a specialization of Ref / RefPtr as isTypeRefCounted and
calls it in tryToFindPtrOrigin for the declared variable. This PR also fixes a bug in the same function
that we were not detecting DeducedTemplateSpecializationType as a safe origin.
…as safe function arguments

This PR extracts the code in UncountedLocalVarsChecker to identify a variable declaration which
has a guardian Ref / RefPtr variable as isVarDeclGuardedInit and integrates it with
alpha.webkit.UncountedCallArgsChecker so that we treat local variables that are "guarded" by
a Ref / RefPtr as safe function arguments.
@rniwa rniwa requested a review from haoNoQ February 21, 2024 20:01
@rniwa rniwa closed this Mar 7, 2024
@rniwa rniwa deleted the check-call-arg-local-vars branch March 7, 2024 09:25
Copy link
Collaborator

@haoNoQ haoNoQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, looks like I never got to properly review this one! I don't have major objections but I have some things to consider fixing in follow-up patches.


ASTContext &ctx = MaybeGuardian->getASTContext();

for (DynTypedNodeList guardianAncestors = ctx.getParents(*MaybeGuardian);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't spent nearly enough time in such code but I suspect that instead of dealing with Stmts, you should be dealing with DeclContexts. Decl contexts represent all the places where something can potentially be declared. Unlike statements, decl contexts naturally have parent pointers (so you don't have to invoke the Parent Map), and they should get you straight to the point without figuring out how many statements of which kind do you need to skip.

@@ -27,6 +28,13 @@ tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) {
E = tempExpr->getSubExpr();
continue;
}
if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
auto *decl = DRE->getFoundDecl();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really better than getDecl()? For variables I don't think there could be many of those, and even if there are, it's probably easier to deal with the default one.

@rniwa
Copy link
Contributor Author

rniwa commented Mar 8, 2024

So this PR turned out to be not really correct because we can't get rid of local variable checker when calling a non-trivial function.

@haoNoQ
Copy link
Collaborator

haoNoQ commented Mar 8, 2024

Oh right right right, so it's closed without committing, nvm then!

@haoNoQ
Copy link
Collaborator

haoNoQ commented Mar 8, 2024

I just thought two PRs, accepted 1, then there were 0 left, and I was confused 😅

@rniwa
Copy link
Contributor Author

rniwa commented Mar 8, 2024

haha, sorry for the confusion. I don't have any outstanding PR at this point. Thanks for all the reviews :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants