From 2c6424e691e32f79bc303203deb1c91634d62286 Mon Sep 17 00:00:00 2001 From: Ryosuke Niwa Date: Tue, 12 Nov 2024 09:46:28 -0800 Subject: [PATCH] [webkit.UncountedLambdaCapturesChecker] Ignore trivial functions and [[clang::noescape]]. (#114897) This PR makes webkit.UncountedLambdaCapturesChecker ignore trivial functions as well as the one being passed to an argument with [[clang::noescape]] attribute. This dramatically reduces the false positive rate for this checker. To do this, this PR replaces VisitLambdaExpr in favor of checking lambdas via VisitDeclRefExpr and VisitCallExpr. The idea is that if a lambda is defined but never called or stored somewhere, then capturing whatever variable in such a lambda is harmless. VisitCallExpr explicitly looks for direct invocation of lambdas and registers its DeclRefExpr to be ignored in VisitDeclRefExpr. If a lambda is being passed to a function, it checks whether its argument is annotated with [[clang::noescape]]. If it's not annotated such, it checks captures for their safety. Because WTF::switchOn could not be annotated with [[clang::noescape]] as function type parameters are variadic template function so we hard-code this function into the checker. In order to check whether "this" pointer is ref-counted type or not, we override TraverseDecl and record the most recent method's declaration. In addition, this PR fixes a bug in isUnsafePtr that it was erroneously checking whether std::nullopt was returned by isUncounted and isUnchecked as opposed to the actual boolean value. Finally, this PR also converts the accompanying test to use -verify and adds a bunch of tests. --- .../Checkers/WebKit/PtrTypesSemantics.cpp | 8 +- .../Checkers/WebKit/PtrTypesSemantics.h | 4 + .../WebKit/UncountedLambdaCapturesChecker.cpp | 131 ++++++++++++- .../Analysis/Checkers/WebKit/mock-types.h | 2 + .../WebKit/uncounted-lambda-captures.cpp | 175 +++++++++++++++--- 5 files changed, 285 insertions(+), 35 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 31bebdb07dbd..5412fea6f132 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -222,7 +222,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 7c6c0a63f22a..681ea55c6135 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -75,6 +75,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 998bd4ccee07..914ff6d44270 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 @@ private: 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 @@ public: // 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,118 @@ public: bool shouldVisitTemplateInstantiations() const { return true; } bool shouldVisitImplicitCode() const { return false; } - bool VisitLambdaExpr(LambdaExpr *L) { - Checker->visitLambdaExpr(L); + bool TraverseCXXMethodDecl(CXXMethodDecl *CXXMD) { + llvm::SaveAndRestore SavedDecl(ClsType); + ClsType = CXXMD->getThisType(); + return Base::TraverseCXXMethodDecl(CXXMD); + } + + 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()) + return true; + 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(); + if (!Init) + return; + auto *L = dyn_cast_or_null(Init->IgnoreParenCasts()); + 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 +182,25 @@ public: } printQuotedQualifiedName(Os, Capture.getCapturedVar()); - Os << " to uncounted type is unsafe."; + Os << " to ref-counted type or 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 type or 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 9c9326f0f11c..4a55ddab64d8 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 27e0a74d583c..9bfcdea04755 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp @@ -1,39 +1,71 @@ -// 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 + +struct RefCountable { + 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 type or 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 type or 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 type or 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 type or 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 type or 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 type or 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 type or 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 type or 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 +77,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 type or 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 type or 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(); +}