[clang] 8a64689 - [Analyzer][WebKit] UncountedLocalVarsChecker
Jan Korous via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 22 11:05:19 PDT 2020
Author: Jan Korous
Date: 2020-09-22T11:05:04-07:00
New Revision: 8a64689e264ce039e4fb0a09c3e136a1c8451838
URL: https://github.com/llvm/llvm-project/commit/8a64689e264ce039e4fb0a09c3e136a1c8451838
DIFF: https://github.com/llvm/llvm-project/commit/8a64689e264ce039e4fb0a09c3e136a1c8451838.diff
LOG: [Analyzer][WebKit] UncountedLocalVarsChecker
Differential Review: https://reviews.llvm.org/D83259
Added:
clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
Modified:
clang/docs/analyzer/checkers.rst
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
Removed:
################################################################################
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index 9fb6782cf5a5..dbbd49085dac 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -2538,6 +2538,53 @@ We also define a set of safe transformations which if passed a safe value as an
- casts
- unary operators like ``&`` or ``*``
+alpha.webkit.UncountedLocalVarsChecker
+""""""""""""""""""""""""""""""""""""""
+The goal of this rule is to make sure that any uncounted local variable is backed by a ref-counted object with lifetime that is strictly larger than the scope of the uncounted local variable. To be on the safe side we require the scope of an uncounted variable to be embedded in the scope of ref-counted object that backs it.
+
+These are examples of cases that we consider safe:
+
+ .. code-block:: cpp
+ void foo1() {
+ RefPtr<RefCountable> counted;
+ // The scope of uncounted is EMBEDDED in the scope of counted.
+ {
+ RefCountable* uncounted = counted.get(); // ok
+ }
+ }
+
+ void foo2(RefPtr<RefCountable> counted_param) {
+ RefCountable* uncounted = counted_param.get(); // ok
+ }
+
+ void FooClass::foo_method() {
+ RefCountable* uncounted = this; // ok
+ }
+
+Here are some examples of situations that we warn about as they *might* be potentially unsafe. The logic is that either we're able to guarantee that an argument is safe or it's considered if not a bug then bug-prone.
+
+ .. code-block:: cpp
+
+ void foo1() {
+ RefCountable* uncounted = new RefCountable; // warn
+ }
+
+ RefCountable* global_uncounted;
+ void foo2() {
+ RefCountable* uncounted = global_uncounted; // warn
+ }
+
+ void foo3() {
+ RefPtr<RefCountable> counted;
+ // The scope of uncounted is not EMBEDDED in the scope of counted.
+ RefCountable* uncounted = counted.get(); // warn
+ }
+
+We don't warn about these cases - we don't consider them necessarily safe but since they are very common and usually safe we'd introduce a lot of false positives otherwise:
+- variable defined in condition part of an ```if``` statement
+- variable defined in init statement condition of a ```for``` statement
+
+For the time being we also don't warn about uninitialized uncounted local variables.
Debug Checkers
---------------
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 3540fe5fe55c..444b00d73f0b 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -1668,4 +1668,8 @@ def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">,
HelpText<"Check uncounted call arguments.">,
Documentation<HasDocumentation>;
+def UncountedLocalVarsChecker : Checker<"UncountedLocalVarsChecker">,
+ HelpText<"Check uncounted local variables.">,
+ Documentation<HasDocumentation>;
+
} // end alpha.webkit
diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 31f971e33cb2..eb4f30137732 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -128,6 +128,7 @@ add_clang_library(clangStaticAnalyzerCheckers
WebKit/RefCntblBaseVirtualDtorChecker.cpp
WebKit/UncountedCallArgsChecker.cpp
WebKit/UncountedLambdaCapturesChecker.cpp
+ WebKit/UncountedLocalVarsChecker.cpp
LINK_LIBS
clangAST
diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
new file mode 100644
index 000000000000..101aba732aea
--- /dev/null
+++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
@@ -0,0 +1,250 @@
+//=======- 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<DeclStmt>()) {
+ DynTypedNodeList grandParent = ASTCtx.getParents(*DS);
+ if (grandParent.size() == 1) {
+ return grandParent.begin()->get<ForStmt>() ||
+ grandParent.begin()->get<IfStmt>() ||
+ grandParent.begin()->get<CXXForRangeStmt>();
+ }
+ }
+ }
+ 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;
+
+ 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<CompoundStmt>()) {
+ 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<CompoundStmt>()) {
+ if (!HaveSkippedFirstCompoundStmt) {
+ HaveSkippedFirstCompoundStmt = true;
+ continue;
+ }
+ if (CStmtAncestor == guardiansClosestCompStmtAncestor)
+ return true;
+ }
+ }
+ }
+
+ return false;
+}
+
+class UncountedLocalVarsChecker
+ : public Checker<check::ASTDecl<TranslationUnitDecl>> {
+ BugType Bug{this,
+ "Uncounted raw pointer or reference not provably backed by "
+ "ref-counted variable",
+ "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<LocalVisitor> {
+ 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<TranslationUnitDecl *>(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;
+
+ if (isa<CXXThisExpr>(InitArgOrigin))
+ return;
+
+ if (auto *Ref = llvm::dyn_cast<DeclRefExpr>(InitArgOrigin)) {
+ if (auto *MaybeGuardian =
+ dyn_cast_or_null<VarDecl>(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<ParmVarDecl>(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<BasicBugReport>(Bug, Os.str(), BSLoc);
+ Report->addRange(V->getSourceRange());
+ BR->emitReport(std::move(Report));
+ }
+};
+} // namespace
+
+void ento::registerUncountedLocalVarsChecker(CheckerManager &Mgr) {
+ Mgr.registerChecker<UncountedLocalVarsChecker>();
+}
+
+bool ento::shouldRegisterUncountedLocalVarsChecker(const CheckerManager &) {
+ return true;
+}
diff --git a/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
new file mode 100644
index 000000000000..8694d5fb85b8
--- /dev/null
+++ b/clang/test/Analysis/Checkers/WebKit/uncounted-local-vars.cpp
@@ -0,0 +1,99 @@
+// 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 at -1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+}
+
+void bar_ref(RefCountable &) {}
+} // namespace reference
+
+namespace guardian_scopes {
+void foo1() {
+ RefPtr<RefCountable> foo;
+ { RefCountable *bar = foo.get(); }
+}
+
+void foo2() {
+ RefPtr<RefCountable> foo;
+ // missing embedded scope here
+ RefCountable *bar = foo.get();
+ // expected-warning at -1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+}
+
+void foo3() {
+ RefPtr<RefCountable> foo;
+ {
+ { RefCountable *bar = foo.get(); }
+ }
+}
+
+void foo4() {
+ {
+ RefPtr<RefCountable> 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 at -1{{Local variable 'bar' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ auto *baz = provide_ref_ctnbl();
+ // expected-warning at -1{{Local variable 'baz' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ auto *baz2 = this->provide_ref_ctnbl();
+ // expected-warning at -1{{Local variable 'baz2' is uncounted and unsafe [alpha.webkit.UncountedLocalVarsChecker]}}
+ }
+};
+} // namespace auto_keyword
+
+namespace guardian_casts {
+void foo1() {
+ RefPtr<RefCountable> foo;
+ { RefCountable *bar = downcast<RefCountable>(foo.get()); }
+}
+
+void foo2() {
+ RefPtr<RefCountable> foo;
+ {
+ RefCountable *bar =
+ static_cast<RefCountable *>(downcast<RefCountable>(foo.get()));
+ }
+}
+} // namespace guardian_casts
+
+namespace guardian_ref_conversion_operator {
+void foo() {
+ Ref<RefCountable> rc;
+ { RefCountable &rr = rc; }
+}
+} // namespace guardian_ref_conversion_operator
+
+namespace ignore_for_if {
+RefCountable *provide_ref_ctnbl() { return nullptr; }
+
+void foo() {
+ // no warnings
+ if (RefCountable *a = provide_ref_ctnbl()) { }
+ for (RefCountable *a = provide_ref_ctnbl(); a != nullptr;) { }
+ RefCountable *array[1];
+ for (RefCountable *a : array) { }
+}
+} // namespace ignore_for_if
More information about the cfe-commits
mailing list