diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp index 01b191ab0eeaf4..287f6a52870056 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp @@ -253,6 +253,19 @@ class TrivialFunctionAnalysisVisitor return true; } + template + bool WithCachedResult(const Stmt *S, CheckFunction Function) { + // If the statement isn't in the cache, conservatively assume that + // it's not trivial until analysis completes. Insert false to the cache + // first to avoid infinite recursion. + auto [It, IsNew] = Cache.insert(std::make_pair(S, false)); + if (!IsNew) + return It->second; + bool Result = Function(); + Cache[S] = Result; + return Result; + } + public: using CacheTy = TrivialFunctionAnalysis::CacheTy; @@ -267,7 +280,7 @@ class TrivialFunctionAnalysisVisitor bool VisitCompoundStmt(const CompoundStmt *CS) { // A compound statement is allowed as long each individual sub-statement // is trivial. - return VisitChildren(CS); + return WithCachedResult(CS, [&]() { return VisitChildren(CS); }); } bool VisitReturnStmt(const ReturnStmt *RS) { @@ -279,17 +292,36 @@ class TrivialFunctionAnalysisVisitor bool VisitDeclStmt(const DeclStmt *DS) { return VisitChildren(DS); } bool VisitDoStmt(const DoStmt *DS) { return VisitChildren(DS); } - bool VisitIfStmt(const IfStmt *IS) { return VisitChildren(IS); } + bool VisitIfStmt(const IfStmt *IS) { + return WithCachedResult(IS, [&]() { return VisitChildren(IS); }); + } + bool VisitForStmt(const ForStmt *FS) { + return WithCachedResult(FS, [&]() { return VisitChildren(FS); }); + } + bool VisitCXXForRangeStmt(const CXXForRangeStmt *FS) { + return WithCachedResult(FS, [&]() { return VisitChildren(FS); }); + } + bool VisitWhileStmt(const WhileStmt *WS) { + return WithCachedResult(WS, [&]() { return VisitChildren(WS); }); + } bool VisitSwitchStmt(const SwitchStmt *SS) { return VisitChildren(SS); } bool VisitCaseStmt(const CaseStmt *CS) { return VisitChildren(CS); } bool VisitDefaultStmt(const DefaultStmt *DS) { return VisitChildren(DS); } bool VisitUnaryOperator(const UnaryOperator *UO) { // Operator '*' and '!' are allowed as long as the operand is trivial. - if (UO->getOpcode() == UO_Deref || UO->getOpcode() == UO_AddrOf || - UO->getOpcode() == UO_LNot) + auto op = UO->getOpcode(); + if (op == UO_Deref || op == UO_AddrOf || op == UO_LNot) return Visit(UO->getSubExpr()); + if (UO->isIncrementOp() || UO->isDecrementOp()) { + // Allow increment or decrement of a POD type. + if (auto *RefExpr = dyn_cast(UO->getSubExpr())) { + if (auto *Decl = dyn_cast(RefExpr->getDecl())) + return Decl->isLocalVarDeclOrParm() && + Decl->getType().isPODType(Decl->getASTContext()); + } + } // Other operators are non-trivial. return false; } @@ -304,22 +336,6 @@ class TrivialFunctionAnalysisVisitor return VisitChildren(CO); } - bool VisitDeclRefExpr(const DeclRefExpr *DRE) { - if (auto *decl = DRE->getDecl()) { - if (isa(decl)) - return true; - if (isa(decl)) - return true; - if (auto *VD = dyn_cast(decl)) { - if (VD->hasConstantInitialization() && VD->getEvaluatedValue()) - return true; - auto *Init = VD->getInit(); - return !Init || Visit(Init); - } - } - return false; - } - bool VisitAtomicExpr(const AtomicExpr *E) { return VisitChildren(E); } bool VisitStaticAssertDecl(const StaticAssertDecl *SAD) { @@ -436,6 +452,11 @@ class TrivialFunctionAnalysisVisitor return true; } + bool VisitDeclRefExpr(const DeclRefExpr *DRE) { + // The use of a variable is trivial. + return true; + } + // Constant literal expressions are always trivial bool VisitIntegerLiteral(const IntegerLiteral *E) { return true; } bool VisitFloatingLiteral(const FloatingLiteral *E) { return true; } @@ -449,7 +470,7 @@ class TrivialFunctionAnalysisVisitor } private: - CacheTy Cache; + CacheTy &Cache; }; bool TrivialFunctionAnalysis::isTrivialImpl( @@ -474,4 +495,17 @@ bool TrivialFunctionAnalysis::isTrivialImpl( return Result; } +bool TrivialFunctionAnalysis::isTrivialImpl( + const Stmt *S, TrivialFunctionAnalysis::CacheTy &Cache) { + // If the statement isn't in the cache, conservatively assume that + // it's not trivial until analysis completes. Unlike a function case, + // we don't insert an entry into the cache until Visit returns + // since Visit* functions themselves make use of the cache. + + TrivialFunctionAnalysisVisitor V(Cache); + bool Result = V.Visit(S); + assert(Cache.contains(S) && "Top-level statement not properly cached!"); + return Result; +} + } // namespace clang diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h index e07cd31395747d..9ed8e7cab6abb9 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h @@ -11,6 +11,7 @@ #include "llvm/ADT/APInt.h" #include "llvm/ADT/DenseMap.h" +#include "llvm/ADT/PointerUnion.h" #include namespace clang { @@ -19,6 +20,7 @@ class CXXMethodDecl; class CXXRecordDecl; class Decl; class FunctionDecl; +class Stmt; class Type; // Ref-countability of a type is implicitly defined by Ref and RefPtr @@ -71,14 +73,17 @@ class TrivialFunctionAnalysis { public: /// \returns true if \p D is a "trivial" function. bool isTrivial(const Decl *D) const { return isTrivialImpl(D, TheCache); } + bool isTrivial(const Stmt *S) const { return isTrivialImpl(S, TheCache); } private: friend class TrivialFunctionAnalysisVisitor; - using CacheTy = llvm::DenseMap; + using CacheTy = + llvm::DenseMap, bool>; mutable CacheTy TheCache{}; static bool isTrivialImpl(const Decl *D, CacheTy &Cache); + static bool isTrivialImpl(const Stmt *S, CacheTy &Cache); }; } // namespace clang diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp index 5a72f53b12edaa..6036ad58cf253c 100644 --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp @@ -26,28 +26,6 @@ using namespace ento; namespace { -// for ( int a = ...) ... true -// for ( int a : ...) ... true -// if ( int* a = ) ... true -// anything else ... false -bool isDeclaredInForOrIf(const VarDecl *Var) { - assert(Var); - auto &ASTCtx = Var->getASTContext(); - auto parent = ASTCtx.getParents(*Var); - - if (parent.size() == 1) { - if (auto *DS = parent.begin()->get()) { - DynTypedNodeList grandParent = ASTCtx.getParents(*DS); - if (grandParent.size() == 1) { - return grandParent.begin()->get() || - grandParent.begin()->get() || - grandParent.begin()->get(); - } - } - } - return false; -} - // FIXME: should be defined by anotations in the future bool isRefcountedStringsHack(const VarDecl *V) { assert(V); @@ -143,6 +121,11 @@ class UncountedLocalVarsChecker // want to visit those, so we make our own RecursiveASTVisitor. struct LocalVisitor : public RecursiveASTVisitor { const UncountedLocalVarsChecker *Checker; + + TrivialFunctionAnalysis TFA; + + using Base = RecursiveASTVisitor; + explicit LocalVisitor(const UncountedLocalVarsChecker *Checker) : Checker(Checker) { assert(Checker); @@ -155,6 +138,36 @@ class UncountedLocalVarsChecker Checker->visitVarDecl(V); return true; } + + bool TraverseIfStmt(IfStmt *IS) { + if (!TFA.isTrivial(IS)) + return Base::TraverseIfStmt(IS); + return true; + } + + bool TraverseForStmt(ForStmt *FS) { + if (!TFA.isTrivial(FS)) + return Base::TraverseForStmt(FS); + return true; + } + + bool TraverseCXXForRangeStmt(CXXForRangeStmt *FRS) { + if (!TFA.isTrivial(FRS)) + return Base::TraverseCXXForRangeStmt(FRS); + return true; + } + + bool TraverseWhileStmt(WhileStmt *WS) { + if (!TFA.isTrivial(WS)) + return Base::TraverseWhileStmt(WS); + return true; + } + + bool TraverseCompoundStmt(CompoundStmt *CS) { + if (!TFA.isTrivial(CS)) + return Base::TraverseCompoundStmt(CS); + return true; + } }; LocalVisitor visitor(this); @@ -189,18 +202,16 @@ class UncountedLocalVarsChecker dyn_cast_or_null(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; + if (MaybeGuardianArgType) { + const CXXRecordDecl *const MaybeGuardianArgCXXRecord = + MaybeGuardianArgType->getAsCXXRecordDecl(); + if (MaybeGuardianArgCXXRecord) { + if (MaybeGuardian->isLocalVarDecl() && + (isRefCounted(MaybeGuardianArgCXXRecord) || + isRefcountedStringsHack(MaybeGuardian)) && + isGuardedScopeEmbeddedInGuardianScope(V, MaybeGuardian)) + return; + } } // Parameters are guaranteed to be safe for the duration of the call @@ -219,9 +230,6 @@ class UncountedLocalVarsChecker if (!V->isLocalVarDecl()) return true; - if (isDeclaredInForOrIf(V)) - return true; - return false; } diff --git a/clang/test/Analysis/Checkers/WebKit/mock-types.h b/clang/test/Analysis/Checkers/WebKit/mock-types.h index e2b3401d407392..aab99197dfa49e 100644 --- a/clang/test/Analysis/Checkers/WebKit/mock-types.h +++ b/clang/test/Analysis/Checkers/WebKit/mock-types.h @@ -62,6 +62,8 @@ struct RefCountable { static Ref create(); void ref() {} void deref() {} + void method(); + int trivial() { return 123; } }; template T *downcast(T *t) { return t; } diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp index 0fcd3b21376caf..00673e91f471ea 100644 --- a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp +++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp @@ -2,6 +2,8 @@ #include "mock-types.h" +void someFunction(); + namespace raw_ptr { void foo() { RefCountable *bar; @@ -16,6 +18,13 @@ void foo_ref() { RefCountable automatic; RefCountable &bar = automatic; // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + someFunction(); + bar.method(); +} + +void foo_ref_trivial() { + RefCountable automatic; + RefCountable &bar = automatic; } void bar_ref(RefCountable &) {} @@ -32,6 +41,8 @@ void foo2() { // missing embedded scope here RefCountable *bar = foo.get(); // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + someFunction(); + bar->method(); } void foo3() { @@ -47,11 +58,35 @@ void foo4() { { RefCountable *bar = foo.get(); } } } + +void foo5() { + RefPtr foo; + auto* bar = foo.get(); + bar->trivial(); +} + +void foo6() { + RefPtr foo; + auto* bar = foo.get(); + // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + bar->method(); +} + +struct SelfReferencingStruct { + SelfReferencingStruct* ptr; + RefCountable* obj { nullptr }; +}; + +void foo7(RefCountable* obj) { + SelfReferencingStruct bar = { &bar, obj }; + bar.obj->method(); +} + } // namespace guardian_scopes namespace auto_keyword { class Foo { - RefCountable *provide_ref_ctnbl() { return nullptr; } + RefCountable *provide_ref_ctnbl(); void evil_func() { RefCountable *bar = provide_ref_ctnbl(); @@ -62,13 +97,24 @@ class Foo { // expected-warning@-1{{Local variable 'baz2' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} [[clang::suppress]] auto *baz_suppressed = provide_ref_ctnbl(); // no-warning } + + void func() { + RefCountable *bar = provide_ref_ctnbl(); + // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + if (bar) + bar->method(); + } }; } // namespace auto_keyword namespace guardian_casts { void foo1() { RefPtr foo; - { RefCountable *bar = downcast(foo.get()); } + { + RefCountable *bar = downcast(foo.get()); + bar->method(); + } + foo->method(); } void foo2() { @@ -76,6 +122,7 @@ void foo2() { { RefCountable *bar = static_cast(downcast(foo.get())); + someFunction(); } } } // namespace guardian_casts @@ -83,7 +130,11 @@ void foo2() { namespace guardian_ref_conversion_operator { void foo() { Ref rc; - { RefCountable &rr = rc; } + { + RefCountable &rr = rc; + rr.method(); + someFunction(); + } } } // namespace guardian_ref_conversion_operator @@ -92,9 +143,47 @@ RefCountable *provide_ref_ctnbl() { return nullptr; } void foo() { // no warnings - if (RefCountable *a = provide_ref_ctnbl()) { } - for (RefCountable *a = provide_ref_ctnbl(); a != nullptr;) { } + if (RefCountable *a = provide_ref_ctnbl()) + a->trivial(); + for (RefCountable *b = provide_ref_ctnbl(); b != nullptr;) + b->trivial(); RefCountable *array[1]; - for (RefCountable *a : array) { } + for (RefCountable *c : array) + c->trivial(); + while (RefCountable *d = provide_ref_ctnbl()) + d->trivial(); + do { + RefCountable *e = provide_ref_ctnbl(); + e->trivial(); + } while (1); + someFunction(); } + +void bar() { + if (RefCountable *a = provide_ref_ctnbl()) { + // expected-warning@-1{{Local variable 'a' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + a->method(); + } + for (RefCountable *b = provide_ref_ctnbl(); b != nullptr;) { + // expected-warning@-1{{Local variable 'b' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + b->method(); + } + RefCountable *array[1]; + for (RefCountable *c : array) { + // expected-warning@-1{{Local variable 'c' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + c->method(); + } + + while (RefCountable *d = provide_ref_ctnbl()) { + // expected-warning@-1{{Local variable 'd' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + d->method(); + } + do { + RefCountable *e = provide_ref_ctnbl(); + // expected-warning@-1{{Local variable 'e' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}} + e->method(); + } while (1); + someFunction(); +} + } // namespace ignore_for_if