[clang] [alpha.webkit.UncountedCallArgsChecker] Don't assume local variables are safe & treat guarded local variable as safe function arguments (PR #82305)
Ryosuke Niwa via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 19 21:10:29 PST 2024
https://github.com/rniwa updated https://github.com/llvm/llvm-project/pull/82305
>From 4d3e50636630dfe8be02c05bb7f0705461594c94 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Fri, 16 Feb 2024 19:29:54 -0800
Subject: [PATCH 1/2] [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.
---
.../lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp | 9 ++++++++-
.../Checkers/WebKit/PtrTypesSemantics.cpp | 12 +++++++++++-
.../Checkers/WebKit/PtrTypesSemantics.h | 5 ++++-
3 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index 1a9d6d3127fb7f..d8738bac5dae3a 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
@@ -27,6 +27,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 =
@@ -99,7 +106,7 @@ 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/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
index a7891d2da07c18..99bf1ed634db56 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -122,12 +122,22 @@ 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) {
+ assert(F);
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..5051f4e321bb6d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -9,7 +9,7 @@
#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 <optional>
@@ -55,6 +55,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);
>From 481becffb2a8e12d5377e95acce28b722a327945 Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rniwa at webkit.org>
Date: Sat, 17 Feb 2024 22:43:58 -0800
Subject: [PATCH 2/2] [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.
---
.../Checkers/WebKit/ASTUtils.cpp | 129 ++++++++++++++++++
.../StaticAnalyzer/Checkers/WebKit/ASTUtils.h | 5 +
.../Checkers/WebKit/PtrTypesSemantics.cpp | 1 -
.../WebKit/UncountedCallArgsChecker.cpp | 10 ++
.../WebKit/UncountedLocalVarsChecker.cpp | 105 +-------------
.../WebKit/guarded-uncounted-obj-arg.cpp | 65 +++++++++
.../Analysis/Checkers/WebKit/mock-types.h | 24 +++-
7 files changed, 233 insertions(+), 106 deletions(-)
create mode 100644 clang/test/Analysis/Checkers/WebKit/guarded-uncounted-obj-arg.cpp
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
index d8738bac5dae3a..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 {
@@ -102,6 +103,134 @@ 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)) {
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 99bf1ed634db56..58212625349889 100644
--- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
@@ -126,7 +126,6 @@ bool isReturnValueRefCounted(const clang::FunctionDecl *F) {
}
bool isTypeRefCounted(QualType type) {
- assert(F);
while (!type.isNull()) {
if (auto *elaboratedT = type->getAs<ElaboratedType>()) {
type = elaboratedT->desugar();
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 at -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
More information about the cfe-commits
mailing list