diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 46819d5ca12058..b32483369b5db1 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -211,7 +211,13 @@ std::optional isUncheckedPtr(const QualType T) { std::optional isUnsafePtr(const QualType T) { if (T->isPointerType() || T->isReferenceType()) { if (auto *CXXRD = T->getPointeeCXXRecordDecl()) { - return isUncounted(CXXRD) || isUnchecked(CXXRD); + auto isUncountedPtr = isUncounted(CXXRD); + auto isUncheckedPtr = isUnchecked(CXXRD); + if (isUncountedPtr && isUncheckedPtr) + return *isUncountedPtr || *isUncheckedPtr; + if (isUncountedPtr) + return *isUncountedPtr; + return isUncheckedPtr; } } return false; diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index 30bdaed706bb53..97baeabea8c7f0 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -67,6 +67,10 @@ std::optional isUncountedPtr(const clang::QualType T); /// class, false if not, std::nullopt if inconclusive. std::optional isUncheckedPtr(const clang::QualType T); +/// \returns true if \p T is either a raw pointer or reference to an uncounted +/// or unchecked class, false if not, std::nullopt if inconclusive. +std::optional isUnsafePtr(const QualType T); + /// \returns true if \p T is a RefPtr, Ref, CheckedPtr, CheckedRef, or its /// variant, false if not. bool isSafePtrType(const clang::QualType T); diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp index 998bd4ccee07db..ef61099416376d 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp @@ -6,6 +6,7 @@ // //===----------------------------------------------------------------------===// +#include "ASTUtils.h" #include "DiagOutputUtils.h" #include "PtrTypesSemantics.h" #include "clang/AST/CXXInheritance.h" @@ -26,6 +27,7 @@ class UncountedLambdaCapturesChecker BugType Bug{this, "Lambda capture of uncounted variable", "WebKit coding guidelines"}; mutable BugReporter *BR = nullptr; + TrivialFunctionAnalysis TFA; public: void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR, @@ -37,6 +39,11 @@ class UncountedLambdaCapturesChecker // want to visit those, so we make our own RecursiveASTVisitor. struct LocalVisitor : public RecursiveASTVisitor { const UncountedLambdaCapturesChecker *Checker; + llvm::DenseSet DeclRefExprsToIgnore; + QualType ClsType; + + using Base = RecursiveASTVisitor; + explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker) : Checker(Checker) { assert(Checker); @@ -45,32 +52,119 @@ class UncountedLambdaCapturesChecker bool shouldVisitTemplateInstantiations() const { return true; } bool shouldVisitImplicitCode() const { return false; } - bool VisitLambdaExpr(LambdaExpr *L) { - Checker->visitLambdaExpr(L); + bool TraverseDecl(Decl *D) { + if (auto *CXXMD = dyn_cast(D)) { + llvm::SaveAndRestore SavedDecl(ClsType); + ClsType = CXXMD->getThisType(); + return Base::TraverseDecl(D); + } + return Base::TraverseDecl(D); + } + + bool shouldCheckThis() { + auto result = !ClsType.isNull() ? isUnsafePtr(ClsType) : std::nullopt; + return result && *result; + } + + bool VisitDeclRefExpr(DeclRefExpr *DRE) { + if (DeclRefExprsToIgnore.contains(DRE)) + return true; + auto *VD = dyn_cast_or_null(DRE->getDecl()); + if (!VD) + return true; + auto *Init = VD->getInit(); + if (!Init) + return true; + auto *L = dyn_cast_or_null(Init->IgnoreParenCasts()); + if (!L) + return true; + Checker->visitLambdaExpr(L, shouldCheckThis()); + return true; + } + + // WTF::switchOn(T, F... f) is a variadic template function and couldn't + // be annotated with NOESCAPE. We hard code it here to workaround that. + bool shouldTreatAllArgAsNoEscape(FunctionDecl *Decl) { + auto *NsDecl = Decl->getParent(); + if (!NsDecl || !isa(NsDecl)) + return false; + return safeGetName(NsDecl) == "WTF" && safeGetName(Decl) == "switchOn"; + } + + bool VisitCallExpr(CallExpr *CE) { + checkCalleeLambda(CE); + if (auto *Callee = CE->getDirectCallee()) { + bool TreatAllArgsAsNoEscape = shouldTreatAllArgAsNoEscape(Callee); + unsigned ArgIndex = 0; + for (auto *Param : Callee->parameters()) { + if (ArgIndex >= CE->getNumArgs()) + break; + auto *Arg = CE->getArg(ArgIndex)->IgnoreParenCasts(); + if (!Param->hasAttr() && !TreatAllArgsAsNoEscape) { + if (auto *L = dyn_cast_or_null(Arg)) { + Checker->visitLambdaExpr(L, shouldCheckThis()); + } + } + ++ArgIndex; + } + } return true; } + + void checkCalleeLambda(CallExpr *CE) { + auto *Callee = CE->getCallee(); + if (!Callee) + return; + auto *DRE = dyn_cast(Callee->IgnoreParenCasts()); + if (!DRE) + return; + auto *MD = dyn_cast_or_null(DRE->getDecl()); + if (!MD || CE->getNumArgs() != 1) + return; + auto *Arg = CE->getArg(0)->IgnoreParenCasts(); + auto *ArgRef = dyn_cast(Arg); + if (!ArgRef) + return; + auto *VD = dyn_cast_or_null(ArgRef->getDecl()); + if (!VD) + return; + auto *Init = VD->getInit()->IgnoreParenCasts(); + auto *L = dyn_cast_or_null(Init); + if (!L) + return; + DeclRefExprsToIgnore.insert(ArgRef); + Checker->visitLambdaExpr(L, shouldCheckThis(), + /* ignoreParamVarDecl */ true); + } }; LocalVisitor visitor(this); visitor.TraverseDecl(const_cast(TUD)); } - void visitLambdaExpr(LambdaExpr *L) const { + void visitLambdaExpr(LambdaExpr *L, bool shouldCheckThis, + bool ignoreParamVarDecl = false) const { + if (TFA.isTrivial(L->getBody())) + return; for (const LambdaCapture &C : L->captures()) { if (C.capturesVariable()) { ValueDecl *CapturedVar = C.getCapturedVar(); + if (ignoreParamVarDecl && isa(CapturedVar)) + continue; QualType CapturedVarQualType = CapturedVar->getType(); - if (auto *CapturedVarType = CapturedVarQualType.getTypePtrOrNull()) { - auto IsUncountedPtr = isUncountedPtr(CapturedVarQualType); - if (IsUncountedPtr && *IsUncountedPtr) - reportBug(C, CapturedVar, CapturedVarType); - } + auto IsUncountedPtr = isUnsafePtr(CapturedVar->getType()); + if (IsUncountedPtr && *IsUncountedPtr) + reportBug(C, CapturedVar, CapturedVarQualType); + } else if (C.capturesThis() && shouldCheckThis) { + if (ignoreParamVarDecl) // this is always a parameter to this function. + continue; + reportBugOnThisPtr(C); } } } void reportBug(const LambdaCapture &Capture, ValueDecl *CapturedVar, - const Type *T) const { + const QualType T) const { assert(CapturedVar); SmallString<100> Buf; @@ -89,7 +183,25 @@ class UncountedLambdaCapturesChecker } printQuotedQualifiedName(Os, Capture.getCapturedVar()); - Os << " to uncounted type is unsafe."; + Os << " to ref-counted / CheckedPtr capable type is unsafe."; + + PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager()); + auto Report = std::make_unique(Bug, Os.str(), BSLoc); + BR->emitReport(std::move(Report)); + } + + void reportBugOnThisPtr(const LambdaCapture &Capture) const { + SmallString<100> Buf; + llvm::raw_svector_ostream Os(Buf); + + if (Capture.isExplicit()) { + Os << "Captured "; + } else { + Os << "Implicitly captured "; + } + + Os << "raw-pointer 'this' to ref-counted / CheckedPtr capable type is " + "unsafe."; PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager()); auto Report = std::make_unique(Bug, Os.str(), BSLoc); diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h index 8d95926e419beb..0d7c5a415af662 100644 --- a/clang/test/Analysis/Checkers/WebKit/mock-types.h +++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h @@ -135,7 +135,9 @@ struct RefCountable { void ref() {} void deref() {} void method(); + void constMethod() const; int trivial() { return 123; } + RefCountable* next(); }; template T *downcast(T *t) { return t; } diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp index 27e0a74d583cd3..5d6941aee71aec 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp @@ -1,39 +1,73 @@ -// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.UncountedLambdaCapturesChecker %s 2>&1 | FileCheck %s --strict-whitespace -#include "mock-types.h" +// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.UncountedLambdaCapturesChecker -verify %s +//#include "mock-types.h" + +struct RefCountable { +// static Ref create(); + void ref() {} + void deref() {} + void method(); + void constMethod() const; + int trivial() { return 123; } + RefCountable* next(); +}; + +RefCountable* make_obj(); + +void someFunction(); +template void call(Callback callback) { + someFunction(); + callback(); +} void raw_ptr() { - RefCountable* ref_countable = nullptr; - auto foo1 = [ref_countable](){}; - // CHECK: warning: Captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker] - // CHECK-NEXT:{{^ 6 | }} auto foo1 = [ref_countable](){}; - // CHECK-NEXT:{{^ | }} ^ - auto foo2 = [&ref_countable](){}; - // CHECK: warning: Captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker] - auto foo3 = [&](){ ref_countable = nullptr; }; - // CHECK: warning: Implicitly captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker] - // CHECK-NEXT:{{^ 12 | }} auto foo3 = [&](){ ref_countable = nullptr; }; - // CHECK-NEXT:{{^ | }} ^ - auto foo4 = [=](){ (void) ref_countable; }; - // CHECK: warning: Implicitly captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker] + RefCountable* ref_countable = make_obj(); + auto foo1 = [ref_countable](){ + // expected-warning@-1{{Captured raw-pointer 'ref_countable' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}} + ref_countable->method(); + }; + auto foo2 = [&ref_countable](){ + // expected-warning@-1{{Captured raw-pointer 'ref_countable' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}} + ref_countable->method(); + }; + auto foo3 = [&](){ + ref_countable->method(); + // expected-warning@-1{{Implicitly captured raw-pointer 'ref_countable' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}} + ref_countable = nullptr; + }; + + auto foo4 = [=](){ + ref_countable->method(); + // expected-warning@-1{{Implicitly captured raw-pointer 'ref_countable' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}} + }; + + call(foo1); + call(foo2); + call(foo3); + call(foo4); // Confirm that the checker respects [[clang::suppress]]. RefCountable* suppressed_ref_countable = nullptr; [[clang::suppress]] auto foo5 = [suppressed_ref_countable](){}; - // CHECK-NOT: warning: Captured raw-pointer 'suppressed_ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker] + // no warning. + call(foo5); } void references() { RefCountable automatic; RefCountable& ref_countable_ref = automatic; + auto foo1 = [ref_countable_ref](){ ref_countable_ref.constMethod(); }; + // expected-warning@-1{{Captured reference 'ref_countable_ref' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}} + auto foo2 = [&ref_countable_ref](){ ref_countable_ref.method(); }; + // expected-warning@-1{{Captured reference 'ref_countable_ref' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}} + auto foo3 = [&](){ ref_countable_ref.method(); }; + // expected-warning@-1{{Implicitly captured reference 'ref_countable_ref' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}} + auto foo4 = [=](){ ref_countable_ref.constMethod(); }; + // expected-warning@-1{{Implicitly captured reference 'ref_countable_ref' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}} - auto foo1 = [ref_countable_ref](){}; - // CHECK: warning: Captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker] - auto foo2 = [&ref_countable_ref](){}; - // CHECK: warning: Captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker] - auto foo3 = [&](){ (void) ref_countable_ref; }; - // CHECK: warning: Implicitly captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker] - auto foo4 = [=](){ (void) ref_countable_ref; }; - // CHECK: warning: Implicitly captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker] + call(foo1); + call(foo2); + call(foo3); + call(foo4); } void quiet() { @@ -45,5 +79,100 @@ void quiet() { auto foo3 = [&]() {}; auto foo4 = [=]() {}; + + call(foo3); + call(foo4); + RefCountable *ref_countable = nullptr; } + +template +void map(RefCountable* start, [[clang::noescape]] Callback&& callback) +{ + while (start) { + callback(*start); + start = start->next(); + } +} + +template +void doubleMap(RefCountable* start, [[clang::noescape]] Callback1&& callback1, Callback2&& callback2) +{ + while (start) { + callback1(*start); + callback2(*start); + start = start->next(); + } +} + +void noescape_lambda() { + RefCountable* someObj = make_obj(); + RefCountable* otherObj = make_obj(); + map(make_obj(), [&](RefCountable& obj) { + otherObj->method(); + }); + doubleMap(make_obj(), [&](RefCountable& obj) { + otherObj->method(); + }, [&](RefCountable& obj) { + otherObj->method(); + // expected-warning@-1{{Implicitly captured raw-pointer 'otherObj' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}} + }); + ([&] { + someObj->method(); + })(); +} + +void lambda_capture_param(RefCountable* obj) { + auto someLambda = [&] { + obj->method(); + }; + someLambda(); + someLambda(); +} + +struct RefCountableWithLambdaCapturingThis { + void ref() const; + void deref() const; + void nonTrivial(); + + void method_captures_this_safe() { + auto lambda = [&]() { + nonTrivial(); + }; + lambda(); + } + + void method_captures_this_unsafe() { + auto lambda = [&]() { + nonTrivial(); + // expected-warning@-1{{Implicitly captured raw-pointer 'this' to ref-counted / CheckedPtr capable type is unsafe [webkit.UncountedLambdaCapturesChecker]}} + }; + call(lambda); + } +}; + +struct NonRefCountableWithLambdaCapturingThis { + void nonTrivial(); + + void method_captures_this_safe() { + auto lambda = [&]() { + nonTrivial(); + }; + lambda(); + } + + void method_captures_this_unsafe() { + auto lambda = [&]() { + nonTrivial(); + }; + call(lambda); + } +}; + +void trivial_lambda() { + RefCountable* ref_countable = make_obj(); + auto trivial_lambda = [&]() { + return ref_countable->trivial(); + }; + trivial_lambda(); +}