Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1643,4 +1643,8 @@ HelpText<"Check uncounted call arguments.">, Documentation; +def UncountedLocalVarsChecker : Checker<"UncountedLocalVarsChecker">, + HelpText<"Check uncounted local variables.">, + Documentation; + } // end alpha.webkit Index: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -126,6 +126,7 @@ WebKit/PtrTypesSemantics.cpp WebKit/RefCntblBaseVirtualDtorChecker.cpp WebKit/UncountedCallArgsChecker.cpp + WebKit/UncountedLocalVarsChecker.cpp LINK_LIBS clangAST Index: clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp =================================================================== --- /dev/null +++ clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp @@ -0,0 +1,252 @@ +//=======- UncountedLocalVarsChecker.cpp -------------------------*- C++ -*-==// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "ASTUtils.h" +#include "DiagOutputUtils.h" +#include "PtrTypesSemantics.h" +#include "clang/AST/CXXInheritance.h" +#include "clang/AST/Decl.h" +#include "clang/AST/DeclCXX.h" +#include "clang/AST/ParentMapContext.h" +#include "clang/AST/RecursiveASTVisitor.h" +#include "clang/Basic/SourceLocation.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" + +using namespace clang; +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); + 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; + + // TODO: cache these + 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()) { + 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()) { + if (!HaveSkippedFirstCompoundStmt) { + HaveSkippedFirstCompoundStmt = true; + continue; + } + if (CStmtAncestor == guardiansClosestCompStmtAncestor) + return true; + } + } + } + + return false; +} + +class UncountedLocalVarsChecker + : public Checker> { + BugType Bug{ + this, "Uncounted local variable of type raw pointer/reference parameter", + "WebKit coding guidelines"}; + mutable BugReporter *BR; + +public: + void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR, + BugReporter &BRArg) const { + BR = &BRArg; + + // The calls to checkAST* from AnalysisConsumer don't + // visit template instantiations or lambda classes. We + // want to visit those, so we make our own RecursiveASTVisitor. + struct LocalVisitor : public RecursiveASTVisitor { + const UncountedLocalVarsChecker *Checker; + explicit LocalVisitor(const UncountedLocalVarsChecker *Checker) + : Checker(Checker) { + assert(Checker); + } + + bool shouldVisitTemplateInstantiations() const { return true; } + bool shouldVisitImplicitCode() const { return false; } + + bool VisitVarDecl(VarDecl *V) { + Checker->visitVarDecl(V); + return true; + } + }; + + LocalVisitor visitor(this); + visitor.TraverseDecl(const_cast(TUD)); + } + + void visitVarDecl(const VarDecl *V) const { + if (shouldSkipVarDecl(V)) + return; + + const auto *ArgType = V->getType().getTypePtr(); + if (!ArgType) + return; + + if (isUncountedPtr(ArgType)) { + const Expr *const InitExpr = V->getInit(); + if (!InitExpr) + return; // FIXME: later on we might warn on uninitialized vars too + + const clang::Expr *const InitArgOrigin = + tryToFindPtrOrigin(InitExpr, /*StopAtFirstRefCountedObj=*/false) + .first; + if (!InitArgOrigin) + return; + + // TODO: there should be another rule making sure methods are only called + // on ref-counted objects + if (isa(InitArgOrigin)) + return; + + if (auto *Ref = llvm::dyn_cast(InitArgOrigin)) { + if (auto *MaybeGuardian = + 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; + } + + // Parameters are guaranteed to be safe for the duration of the call + // by another checker. + if (isa(MaybeGuardian)) + return; + } + } + + reportBug(V); + } + } + + bool shouldSkipVarDecl(const VarDecl *V) const { + assert(V); + if (!V->isLocalVarDecl()) + return true; + + if (isDeclaredInForOrIf(V)) + return true; + + return false; + } + + void reportBug(const VarDecl *V) const { + assert(V); + SmallString<100> Buf; + llvm::raw_svector_ostream Os(Buf); + + Os << "Local variable "; + printQuotedQualifiedName(Os, V); + Os << " is uncounted and unsafe."; + + PathDiagnosticLocation BSLoc(V->getLocation(), BR->getSourceManager()); + auto Report = std::make_unique(Bug, Os.str(), BSLoc); + Report->addRange(V->getSourceRange()); + BR->emitReport(std::move(Report)); + } +}; +} // namespace + +void ento::registerUncountedLocalVarsChecker(CheckerManager &Mgr) { + Mgr.registerChecker(); +} + +bool ento::shouldRegisterUncountedLocalVarsChecker(const CheckerManager &) { + return true; +} Index: clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp @@ -0,0 +1,110 @@ +// RUN: %clang_analyze_cc1 +// -analyzer-checker=alpha.webkit.UncountedLocalVarsChecker -verify %s + +#include "mock-types.h" + +namespace raw_ptr { +void foo() { + RefCountable *bar; + // FIXME: later on we might warn on uninitialized vars too +} + +void bar(RefCountable *) {} +} // namespace raw_ptr + +namespace reference { +void foo_ref() { + RefCountable automatic; + RefCountable &bar = automatic; + // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe + // [alpha.webkit.UncountedLocalVarsChecker]}} +} + +void bar_ref(RefCountable &) {} +} // namespace reference + +namespace guardian_scopes { +void foo1() { + RefPtr foo; + { RefCountable *bar = foo.get(); } +} + +void foo2() { + RefPtr foo; + // missing embedded scope here + RefCountable *bar = foo.get(); + // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe + // [alpha.webkit.UncountedLocalVarsChecker]}} +} + +void foo3() { + RefPtr foo; + { + { RefCountable *bar = foo.get(); } + } +} + +void foo4() { + { + RefPtr foo; + { RefCountable *bar = foo.get(); } + } +} +} // namespace guardian_scopes + +namespace auto_keyword { +class Foo { + RefCountable *provide_ref_ctnbl() { return nullptr; } + + void evil_func() { + RefCountable *bar = provide_ref_ctnbl(); + // expected-warning@-1{{Local variable 'bar' is uncounted and unsafe + // [alpha.webkit.UncountedLocalVarsChecker]}} + auto *baz = provide_ref_ctnbl(); + // expected-warning@-1{{Local variable 'baz' is uncounted and unsafe + // [alpha.webkit.UncountedLocalVarsChecker]}} + auto *baz2 = this->provide_ref_ctnbl(); + // expected-warning@-1{{Local variable 'baz2' is uncounted and unsafe + // [alpha.webkit.UncountedLocalVarsChecker]}} + } +}; +} // namespace auto_keyword + +namespace guardian_casts { +void foo1() { + RefPtr foo; + { RefCountable *bar = downcast(foo.get()); } +} + +void foo2() { + RefPtr foo; + { + RefCountable *bar = + static_cast(downcast(foo.get())); + } +} +} // namespace guardian_casts + +namespace guardian_ref_conversion_operator { +void foo() { + Ref rc; + { RefCountable &rr = rc; } +} +} // namespace guardian_ref_conversion_operator + +namespace ignore_for_if { +RefCountable *provide_ref_ctnbl() { return nullptr; } + +void foo() { + if (RefCountable *a = provide_ref_ctnbl()) { + } + // CHECK-NOT: warning: [WebKit] + for (RefCountable *a = provide_ref_ctnbl(); a != nullptr;) { + } + // CHECK-NOT: warning: [WebKit] + RefCountable *array[1]; + for (RefCountable *a : array) { + } + // CHECK-NOT: warning: [WebKit] +} +} // namespace ignore_for_if