Skip to content

[Analyzer][WebKit] cherry-pick 2 more checkers #1802

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions clang/docs/analyzer/checkers.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1423,6 +1423,25 @@ Raw pointers and references to uncounted types can't be used as class members. O
// ...
};

.. _webkit-UncountedLambdaCapturesChecker:

webkit.UncountedLambdaCapturesChecker
"""""""""""""""""""""""""""""""""""""
Raw pointers and references to uncounted types can't be captured in lambdas. Only ref-counted types are allowed.

.. code-block:: cpp

struct RefCntbl {
void ref() {}
void deref() {}
};

void foo(RefCntbl* a, RefCntbl& b) {
[&, a](){ // warn about 'a'
do_something(b); // warn about 'b'
};
};

.. _alpha-checkers:

Experimental Checkers
Expand Down Expand Up @@ -2482,6 +2501,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
---------------
Expand Down
8 changes: 8 additions & 0 deletions clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
Original file line number Diff line number Diff line change
Expand Up @@ -1654,6 +1654,10 @@ def NoUncountedMemberChecker : Checker<"NoUncountedMemberChecker">,
HelpText<"Check for no uncounted member variables.">,
Documentation<HasDocumentation>;

def UncountedLambdaCapturesChecker : Checker<"UncountedLambdaCapturesChecker">,
HelpText<"Check uncounted lambda captures.">,
Documentation<HasDocumentation>;

} // end webkit

let ParentPackage = WebKitAlpha in {
Expand All @@ -1662,4 +1666,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
2 changes: 2 additions & 0 deletions clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ add_clang_library(clangStaticAnalyzerCheckers
WebKit/PtrTypesSemantics.cpp
WebKit/RefCntblBaseVirtualDtorChecker.cpp
WebKit/UncountedCallArgsChecker.cpp
WebKit/UncountedLambdaCapturesChecker.cpp
WebKit/UncountedLocalVarsChecker.cpp

LINK_LIBS
clangAST
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
//=======- UncountedLambdaCapturesChecker.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 "DiagOutputUtils.h"
#include "PtrTypesSemantics.h"
#include "clang/AST/CXXInheritance.h"
#include "clang/AST/RecursiveASTVisitor.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"

using namespace clang;
using namespace ento;

namespace {
class UncountedLambdaCapturesChecker
: public Checker<check::ASTDecl<TranslationUnitDecl>> {
private:
BugType Bug{this, "Lambda capture of uncounted 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 UncountedLambdaCapturesChecker *Checker;
explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
: Checker(Checker) {
assert(Checker);
}

bool shouldVisitTemplateInstantiations() const { return true; }
bool shouldVisitImplicitCode() const { return false; }

bool VisitLambdaExpr(LambdaExpr *L) {
Checker->visitLambdaExpr(L);
return true;
}
};

LocalVisitor visitor(this);
visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD));
}

void visitLambdaExpr(LambdaExpr *L) const {
for (const LambdaCapture &C : L->captures()) {
if (C.capturesVariable()) {
VarDecl *CapturedVar = C.getCapturedVar();
if (auto *CapturedVarType = CapturedVar->getType().getTypePtrOrNull()) {
if (isUncountedPtr(CapturedVarType)) {
reportBug(C, CapturedVar, CapturedVarType);
}
}
}
}
}

void reportBug(const LambdaCapture &Capture, VarDecl *CapturedVar,
const Type *T) const {
assert(CapturedVar);

SmallString<100> Buf;
llvm::raw_svector_ostream Os(Buf);

if (Capture.isExplicit()) {
Os << "Captured ";
} else {
Os << "Implicitly captured ";
}
if (T->isPointerType()) {
Os << "raw-pointer ";
} else {
assert(T->isReferenceType());
Os << "reference ";
}

printQuotedQualifiedName(Os, Capture.getCapturedVar());
Os << " to uncounted type is unsafe.";

PathDiagnosticLocation BSLoc(Capture.getLocation(), BR->getSourceManager());
auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc);
BR->emitReport(std::move(Report));
}
};
} // namespace

void ento::registerUncountedLambdaCapturesChecker(CheckerManager &Mgr) {
Mgr.registerChecker<UncountedLambdaCapturesChecker>();
}

bool ento::shouldRegisterUncountedLambdaCapturesChecker(
const CheckerManager &mgr) {
return true;
}
Loading