https://github.com/rniwa created https://github.com/llvm/llvm-project/pull/92501
When a base class B has a deref function which calls delete operator on a
derived class D, don't emit a warning for B even if it did not have a virtual
destructor.
>From 87cfc8234e1294dedc103b9bcd2b7d9d31874c4a Mon Sep 17 00:00:00 2001
From: Ryosuke Niwa <rn...@apple.com>
Date: Thu, 16 May 2024 23:24:13 -0700
Subject: [PATCH] [webkit.RefCntblBaseVirtualDtor] Ignore a base class which
has a specialized deref function for a given derived class.
When a base class B has a deref function which calls delete operator on a
derived class D,
don't emit a warning for B even if it did not have a virtual destructor.
---
.../WebKit/RefCntblBaseVirtualDtorChecker.cpp | 140 ++++++++++++-
.../ref-cntbl-base-virtual-dtor-templates.cpp | 196 +++++++++++++++++-
2 files changed, 330 insertions(+), 6 deletions(-)
diff --git
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
index 7f4c3a7b787e8..36ab581f1edbd 100644
---
a/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++
b/clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -11,16 +11,128 @@
#include "PtrTypesSemantics.h"
#include "clang/AST/CXXInheritance.h"
#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/StmtVisitor.h"
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "llvm/ADT/DenseSet.h"
#include <optional>
using namespace clang;
using namespace ento;
namespace {
+
+class DerefAnalysisVisitor
+ : public ConstStmtVisitor<DerefAnalysisVisitor, bool> {
+ // Returns true if any of child statements return true.
+ bool VisitChildren(const Stmt *S) {
+ for (const Stmt *Child : S->children()) {
+ if (Child && Visit(Child))
+ return true;
+ }
+ return false;
+ }
+
+ bool VisitBody(const Stmt *Body) {
+ if (!Body)
+ return false;
+
+ auto [It, IsNew] = VisitedBody.insert(Body);
+ if (!IsNew) // This body is recursive
+ return false;
+
+ return Visit(Body);
+ }
+
+public:
+ DerefAnalysisVisitor(const TemplateArgumentList &ArgList,
+ const CXXRecordDecl *ClassDecl)
+ : ArgList(&ArgList)
+ , ClassDecl(ClassDecl)
+ { }
+
+ DerefAnalysisVisitor(const CXXRecordDecl *ClassDecl)
+ : ClassDecl(ClassDecl)
+ { }
+
+ bool HasSpecializedDelete(CXXMethodDecl *Decl) {
+ if (auto *Tmpl = Decl->getTemplateInstantiationPattern())
+ return VisitBody(Tmpl->getBody());
+ return VisitBody(Decl->getBody());
+ }
+
+ bool VisitCallExpr(const CallExpr *CE) {
+ auto *Callee = CE->getCallee();
+ while (auto *Expr = dyn_cast<CastExpr>(Callee))
+ Callee = Expr->getSubExpr();
+ if (auto *DeclRef = dyn_cast<DeclRefExpr>(Callee)) {
+ auto *Decl = DeclRef->getDecl();
+ if (auto *VD = dyn_cast<VarDecl>(Decl)) {
+ if (auto *Init = VD->getInit()) {
+ if (auto *Lambda = dyn_cast<LambdaExpr>(Init))
+ return VisitBody(Lambda->getBody());
+ }
+ } else if (auto *FD = dyn_cast<FunctionDecl>(Decl))
+ return VisitBody(FD->getBody());
+ }
+ return false;
+ }
+
+ bool VisitCXXMemberCallExpr(const CXXMemberCallExpr *MCE) {
+ auto *Callee = MCE->getMethodDecl();
+ if (!Callee)
+ return false;
+ return VisitBody(Callee->getBody());
+ }
+
+ bool VisitCXXDeleteExpr(const CXXDeleteExpr *E) {
+ auto *Arg = E->getArgument();
+ while (Arg) {
+ if (auto *Paren = dyn_cast<ParenExpr>(Arg))
+ Arg = Paren->getSubExpr();
+ else if (auto *Cast = dyn_cast<ExplicitCastExpr>(Arg)) {
+ Arg = Cast->getSubExpr();
+ auto CastType = Cast->getType();
+ if (auto *PtrType = dyn_cast<PointerType>(CastType)) {
+ auto PointeeType = PtrType->getPointeeType();
+ while (auto *ET = dyn_cast<ElaboratedType>(PointeeType)) {
+ if (ET->isSugared())
+ PointeeType = ET->desugar();
+ }
+ if (auto *ParmType = dyn_cast<TemplateTypeParmType>(PointeeType)) {
+ if (ArgList) {
+ auto ParmIndex = ParmType->getIndex();
+ auto Type = ArgList->get(ParmIndex).getAsType();
+ if (auto *RD = dyn_cast<RecordType>(Type)) {
+ if (RD->getDecl() == ClassDecl)
+ return true;
+ }
+ }
+ } else if (auto *RD = dyn_cast<RecordType>(PointeeType)) {
+ if (RD->getDecl() == ClassDecl)
+ return true;
+ }
+ }
+ } else
+ break;
+ }
+ return false;
+ }
+
+ bool VisitStmt(const Stmt *S) { return VisitChildren(S); }
+
+ // Return false since the contents of lambda isn't necessarily executed.
+ // If it is executed, VisitCallExpr above will visit its body.
+ bool VisitLambdaExpr(const LambdaExpr *) { return false; }
+
+private:
+ const TemplateArgumentList* ArgList { nullptr };
+ const CXXRecordDecl* ClassDecl;
+ llvm::DenseSet<const Stmt *> VisitedBody;
+};
+
class RefCntblBaseVirtualDtorChecker
: public Checker<check::ASTDecl<TranslationUnitDecl>> {
private:
@@ -91,8 +203,6 @@ class RefCntblBaseVirtualDtorChecker
const CXXRecordDecl *C = T->getAsCXXRecordDecl();
if (!C)
return false;
- if (isRefCountedClass(C))
- return false;
bool AnyInconclusiveBase = false;
const auto hasPublicRefInBase =
@@ -124,6 +234,9 @@ class RefCntblBaseVirtualDtorChecker
if (AnyInconclusiveBase || !hasRef || !hasDeref)
return false;
+ if (isClassWithSpecializedDelete(C, RD))
+ return false;
+
const auto *Dtor = C->getDestructor();
if (!Dtor || !Dtor->isVirtual()) {
ProblematicBaseSpecifier = Base;
@@ -182,6 +295,29 @@ class RefCntblBaseVirtualDtorChecker
ClsName == "ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr");
}
+ static bool isClassWithSpecializedDelete(const CXXRecordDecl *C,
+ const CXXRecordDecl *DerivedClass) {
+ if (auto *ClsTmplSpDecl = dyn_cast<ClassTemplateSpecializationDecl>(C)) {
+ for (auto *MethodDecl : C->methods()) {
+ if (safeGetName(MethodDecl) == "deref") {
+ DerefAnalysisVisitor DerefAnalysis(ClsTmplSpDecl->getTemplateArgs(),
+ DerivedClass);
+ if (DerefAnalysis.HasSpecializedDelete(MethodDecl))
+ return true;
+ }
+ }
+ return false;
+ }
+ for (auto *MethodDecl : C->methods()) {
+ if (safeGetName(MethodDecl) == "deref") {
+ DerefAnalysisVisitor DerefAnalysis(DerivedClass);
+ if (DerefAnalysis.HasSpecializedDelete(MethodDecl))
+ return true;
+ }
+ }
+ return false;
+ }
+
void reportBug(const CXXRecordDecl *DerivedClass,
const CXXBaseSpecifier *BaseSpec,
const CXXRecordDecl *ProblematicBaseClass) const {
diff --git
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
index eeb62d5d89ec4..37f5d46110896 100644
---
a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
+++
b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
@@ -58,24 +58,101 @@ class RefCounted : public RefCountedBase {
RefCounted() { }
};
+template <typename X, typename T>
+class ExoticRefCounted : RefCountedBase {
+public:
+ void deref() const {
+ if (derefBase())
+ delete (const_cast<T*>(static_cast<const T*>(this)));
+ }
+};
+
+template <typename X, typename T>
+class BadBase : RefCountedBase {
+public:
+ void deref() const {
+ if (derefBase())
+ delete (const_cast<X*>(static_cast<const X*>(this)));
+ }
+};
+
+template <typename T>
+class FancyDeref {
+public:
+ void ref() const
+ {
+ ++refCount;
+ }
+
+ void deref() const
+ {
+ --refCount;
+ if (refCount)
+ return;
+ auto deleteThis = [this] {
+ delete static_cast<const T*>(this);
+ };
+ deleteThis();
+ }
+private:
+ mutable unsigned refCount { 0 };
+};
+
+template <typename T>
+class BadFancyDeref {
+public:
+ void ref() const
+ {
+ ++refCount;
+ }
+
+ void deref() const
+ {
+ --refCount;
+ if (refCount)
+ return;
+ auto deleteThis = [this] {
+ delete static_cast<const T*>(this);
+ };
+ delete this;
+ }
+private:
+ mutable unsigned refCount { 0 };
+};
+
template <typename T>
class ThreadSafeRefCounted {
public:
- void ref() const;
- bool deref() const;
+ void ref() const { ++refCount; }
+ void deref() const {
+ if (!--refCount)
+ delete const_cast<T*>(static_cast<const T*>(this));
+ }
+private:
+ mutable unsigned refCount { 0 };
};
template <typename T>
class ThreadSafeRefCountedAndCanMakeThreadSafeWeakPtr {
public:
- void ref() const;
- bool deref() const;
+ void ref() const { ++refCount; }
+ void deref() const {
+ if (!--refCount)
+ delete const_cast<T*>(static_cast<const T*>(this));
+ }
+private:
+ mutable unsigned refCount { 0 };
};
} // namespace WTF
class DerivedClass4 : public WTF::RefCounted<DerivedClass4> { };
+class DerivedClass4b : public WTF::ExoticRefCounted<int, DerivedClass4b> { };
+
+class DerivedClass4c : public WTF::BadBase<int, DerivedClass4c> { };
+// expected-warning@-1{{Class 'WTF::BadBase<int, DerivedClass4c>' is used as a
base of class 'DerivedClass4c' but doesn't have virtual destructor}}
+
class DerivedClass5 : public DerivedClass4 { };
// expected-warning@-1{{Class 'DerivedClass4' is used as a base of class
'DerivedClass5' but doesn't have virtual destructor}}
@@ -88,3 +165,114 @@ class DerivedClass8 : public
WTF::ThreadSafeRefCountedAndCanMakeThreadSafeWeakPt
class DerivedClass9 : public DerivedClass8 { };
// expected-warning@-1{{Class 'DerivedClass8' is used as a base of class
'DerivedClass9' but doesn't have virtual destructor}}
+
+class DerivedClass10 : public WTF::FancyDeref<DerivedClass10> { };
+
+class DerivedClass10b : public WTF::BadFancyDeref<DerivedClass10b> { };
+// expected-warning@-1{{Class 'WTF::BadFancyDeref<DerivedClass10b>' is used as
a base of class 'DerivedClass10b' but doesn't have virtual destructor}}
+
+class BaseClass1 {
+public:
+ void ref() const { ++refCount; }
+ void deref() const;
+private:
+ enum class Type { Base, Derived } type { Type::Base };
+ mutable unsigned refCount { 0 };
+};
+
+class DerivedClass11 : public BaseClass1 { };
+
+void BaseClass1::deref() const
+{
+ --refCount;
+ if (refCount)
+ return;
+ switch (type) {
+ case Type::Base:
+ delete const_cast<BaseClass1*>(this);
+ break;
+ case Type::Derived:
+ delete const_cast<DerivedClass11*>(static_cast<const
DerivedClass11*>(this));
+ break;
+ }
+}
+
+class BaseClass2;
+static void deleteBase2(BaseClass2*);
+
+class BaseClass2 {
+public:
+ void ref() const { ++refCount; }
+ void deref() const
+ {
+ if (!--refCount)
+ deleteBase2(const_cast<BaseClass2*>(this));
+ }
+ virtual bool isDerived() { return false; }
+private:
+ mutable unsigned refCount { 0 };
+};
+
+class DerivedClass12 : public BaseClass2 {
+ bool isDerived() final { return true; }
+};
+
+void deleteBase2(BaseClass2* obj) {
+ if (obj->isDerived())
+ delete static_cast<DerivedClass12*>(obj);
+ else
+ delete obj;
+}
+
+class BaseClass3 {
+public:
+ void ref() const { ++refCount; }
+ void deref() const
+ {
+ if (!--refCount)
+ const_cast<BaseClass3*>(this)->destory();
+ }
+ virtual bool isDerived() { return false; }
+
+private:
+ void destory();
+
+ mutable unsigned refCount { 0 };
+};
+
+class DerivedClass13 : public BaseClass3 {
+ bool isDerived() final { return true; }
+};
+
+void BaseClass3::destory() {
+ if (isDerived())
+ delete static_cast<DerivedClass13*>(this);
+ else
+ delete this;
+}
+
+class RecursiveBaseClass {
+public:
+ void ref() const {
+ if (otherObject)
+ otherObject->ref();
+ else
+ ++refCount;
+ }
+ void deref() const {
+ if (otherObject)
+ otherObject->deref();
+ else {
+ --refCount;
+ if (refCount)
+ return;
+ delete this;
+ }
+ }
+private:
+ RecursiveBaseClass* otherObject { nullptr };
+ mutable unsigned refCount { 0 };
+};
+
+class RecursiveDerivedClass : public RecursiveBaseClass { };
+// expected-warning@-1{{Class 'RecursiveBaseClass' is used as a base of class
'RecursiveDerivedClass' but doesn't have virtual destructor}}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
