diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -1407,7 +1407,7 @@ .. _webkit-WebKitNoUncountedMemberChecker: webkit.WebKitNoUncountedMemberChecker -"""""""""""""""""""""""""""""""""""" +""""""""""""""""""""""""""""""""""""" Raw pointers and references to uncounted types can't be used as class members. Only ref-counted types are allowed. .. code-block:: cpp @@ -2377,6 +2377,99 @@ } +alpha.WebKit +^^^^^^^^^^^^ + +.. _alpha-webkit-UncountedCallArgsChecker: + +alpha.webkit.UncountedCallArgsChecker +""""""""""""""""""""""""""""""""""""" +The goal of this rule is to make sure that lifetime of any dynamically allocated ref-countable object passed as a call argument spans past the end of the call. This applies to call to any function, method, lambda, function pointer or functor. Ref-countable types aren't supposed to be allocated on stack so we check arguments for parameters of raw pointers and references to uncounted types. + +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 + + RefCountable* provide_uncounted(); + void consume(RefCountable*); + + // In these cases we can't make sure callee won't directly or indirectly call `deref()` on the argument which could make it unsafe from such point until the end of the call. + + void foo1() { + consume(provide_uncounted()); // warn + } + + void foo2() { + RefCountable* uncounted = provide_uncounted(); + consume(uncounted); // warn + } + +Although we are enforcing member variables to be ref-counted by `webkit.WebKitNoUncountedMemberChecker` any method of the same class still has unrestricted access to these. Since from a caller's perspective we can't guarantee a particular member won't get modified by callee (directly or indirectly) we don't consider values obtained from members safe. + +Note: It's likely this heuristic could be made more precise with fewer false positives - for example calls to free functions that don't have any parameter other than the pointer should be safe as the callee won't be able to tamper with the member unless it's a global variable. + + .. code-block:: cpp + + struct Foo { + RefPtr member; + void consume(RefCountable*) { /* ... */ } + void bugprone() { + consume(member.get()); // warn + } + }; + +The implementation of this rule is a heuristic - we define a whitelist of kinds of values that are considered safe to be passed as arguments. If we can't prove an argument is safe it's considered an error. + +Allowed kinds of arguments: + +- values obtained from ref-counted objects (including temporaries as those survive the call too) + + .. code-block:: cpp + + RefCountable* provide_uncounted(); + void consume(RefCountable*); + + void foo() { + RefPtr rc = makeRef(provide_uncounted()); + consume(rc.get()); // ok + consume(makeRef(provide_uncounted()).get()); // ok + } + +- forwarding uncounted arguments from caller to callee + + .. code-block:: cpp + + void foo(RefCountable& a) { + bar(a); // ok + } + + Caller of ``foo()`` is responsible for ``a``'s lifetime. + +- ``this`` pointer + + .. code-block:: cpp + + void Foo::foo() { + baz(this); // ok + } + + Caller of ``foo()`` is responsible for keeping the memory pointed to by ``this`` pointer safe. + +- constants + + .. code-block:: cpp + + foo(nullptr, NULL, 0); // ok + +We also define a set of safe transformations which if passed a safe value as an input provide (usually it's the return value) a safe value (or an object that provides safe values). This is also a heuristic. + +- constructors of ref-counted types (including factory methods) +- getters of ref-counted types +- member overloaded operators +- casts +- unary operators like ``&`` or ``*`` + + Debug Checkers --------------- diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1628,4 +1628,13 @@ def WebKitNoUncountedMemberChecker : Checker<"WebKitNoUncountedMemberChecker">, HelpText<"Check for no uncounted member variables.">, Documentation; + } // end webkit + +let ParentPackage = WebKitAlpha in { + +def UncountedCallArgsChecker : Checker<"UncountedCallArgsChecker">, + HelpText<"Check uncounted call arguments.">, + Documentation; + +} // end alpha.webkit diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -122,8 +122,10 @@ ValistChecker.cpp VirtualCallChecker.cpp WebKit/NoUncountedMembersChecker.cpp + WebKit/ASTUtils.cpp WebKit/PtrTypesSemantics.cpp WebKit/RefCntblBaseVirtualDtorChecker.cpp + WebKit/UncountedCallArgsChecker.cpp LINK_LIBS clangAST diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h --- a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h @@ -23,9 +23,23 @@ class CXXMethodDecl; class Expr; +/// This function de-facto defines a set of transformations that we consider +/// safe (in heuristical sense). These transformation if passed a safe value as +/// an input should provide a safe value (or an object that provides safe +/// values). +/// +/// For more context see Static Analyzer checkers documentation - specifically +/// webkit.UncountedCallArgsChecker checker. Whitelist of transformations: +/// - constructors of ref-counted types (including factory methods) +/// - getters of ref-counted types +/// - member overloaded operators +/// - casts +/// - unary operators like ``&`` or ``*`` +/// /// If passed expression is of type uncounted pointer/reference we try to find -/// the origin of this pointer. Example: Origin can be a local variable, nullptr -/// constant or this-pointer. +/// the "origin" of the pointer value. +/// Origin can be for example a local variable, nullptr, constant or +/// this-pointer. /// /// Certain subexpression nodes represent transformations that don't affect /// where the memory address originates from. We try to traverse such diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp new file mode 100644 --- /dev/null +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp @@ -0,0 +1,93 @@ +//=======- ASTUtils.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 "PtrTypesSemantics.h" +#include "clang/AST/CXXInheritance.h" +#include "clang/AST/Decl.h" +#include "clang/AST/DeclCXX.h" +#include "clang/AST/ExprCXX.h" + +using llvm::Optional; +namespace clang { + +std::pair +tryToFindPtrOrigin(const Expr *E, bool StopAtFirstRefCountedObj) { + while (E) { + if (auto *cast = dyn_cast(E)) { + if (StopAtFirstRefCountedObj) { + if (auto *ConversionFunc = + dyn_cast_or_null(cast->getConversionFunction())) { + if (isCtorOfRefCounted(ConversionFunc)) + return {E, true}; + } + } + // FIXME: This can give false "origin" that would lead to false negatives + // in checkers. See https://reviews.llvm.org/D37023 for reference. + E = cast->getSubExpr(); + continue; + } + if (auto *call = dyn_cast(E)) { + if (auto *memberCall = dyn_cast(call)) { + if (isGetterOfRefCounted(memberCall->getMethodDecl())) { + E = memberCall->getImplicitObjectArgument(); + if (StopAtFirstRefCountedObj) { + return {E, true}; + } + continue; + } + } + + if (auto *operatorCall = dyn_cast(E)) { + if (operatorCall->getNumArgs() == 1) { + E = operatorCall->getArg(0); + continue; + } + } + + if (auto *callee = call->getDirectCallee()) { + if (isCtorOfRefCounted(callee)) { + if (StopAtFirstRefCountedObj) + return {E, true}; + + E = call->getArg(0); + continue; + } + + if (isPtrConversion(callee)) { + E = call->getArg(0); + continue; + } + } + } + if (auto *unaryOp = dyn_cast(E)) { + // FIXME: Currently accepts ANY unary operator. Is it OK? + E = unaryOp->getSubExpr(); + continue; + } + + break; + } + // Some other expression. + return {E, false}; +} + +bool isASafeCallArg(const Expr *E) { + assert(E); + if (auto *Ref = dyn_cast(E)) { + if (auto *D = dyn_cast_or_null(Ref->getFoundDecl())) { + if (isa(D) || D->isLocalVarDecl()) + return true; + } + } + + // TODO: checker for method calls on non-refcounted objects + return isa(E); +} + +} // namespace clang diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp new file mode 100644 --- /dev/null +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp @@ -0,0 +1,195 @@ +//=======- UncountedCallArgsChecker.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/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 { + +class UncountedCallArgsChecker + : public Checker> { + BugType Bug{this, + "Uncounted call argument for a 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 UncountedCallArgsChecker *Checker; + explicit LocalVisitor(const UncountedCallArgsChecker *Checker) + : Checker(Checker) { + assert(Checker); + } + + bool shouldVisitTemplateInstantiations() const { return true; } + bool shouldVisitImplicitCode() const { return false; } + + bool VisitCallExpr(const CallExpr *CE) { + Checker->visitCallExpr(CE); + return true; + } + }; + + LocalVisitor visitor(this); + visitor.TraverseDecl(const_cast(TUD)); + } + + void visitCallExpr(const CallExpr *CE) const { + if (shouldSkipCall(CE)) + return; + + if (auto *F = CE->getDirectCallee()) { + // Skip the first argument for overloaded member operators (e. g. lambda + // or std::function call operator). + unsigned ArgIdx = + isa(CE) && dyn_cast_or_null(F); + + for (auto P = F->param_begin(); + // FIXME: Also check variadic function parameters. + // FIXME: Also check default function arguments. Probably a different + // checker. In case there are default arguments the call can have + // fewer arguments than the callee has parameters. + P < F->param_end() && ArgIdx < CE->getNumArgs(); ++P, ++ArgIdx) { + // TODO: attributes. + // if ((*P)->hasAttr()) + // continue; + + const auto *ArgType = (*P)->getType().getTypePtrOrNull(); + if (!ArgType) + continue; // FIXME? Should we bail? + + // FIXME: more complex types (arrays, references to raw pointers, etc) + if (!isUncountedPtr(ArgType)) + continue; + + const auto *Arg = CE->getArg(ArgIdx); + + std::pair ArgOrigin = + tryToFindPtrOrigin(Arg, true); + + // Temporary ref-counted object created as part of the call argument + // would outlive the call. + if (ArgOrigin.second) + continue; + + if (isa(ArgOrigin.first)) { + // foo(nullptr) + continue; + } + if (isa(ArgOrigin.first)) { + // FIXME: Check the value. + // foo(NULL) + continue; + } + + if (isASafeCallArg(ArgOrigin.first)) + continue; + + reportBug(Arg, *P); + } + } + } + + bool shouldSkipCall(const CallExpr *CE) const { + if (CE->getNumArgs() == 0) + return false; + + // If an assignment is problematic we should warn about the sole existence + // of object on LHS. + if (auto *MemberOp = dyn_cast(CE)) { + // Note: assignemnt to built-in type isn't derived from CallExpr. + if (MemberOp->isAssignmentOp()) + return false; + } + + const auto *Callee = CE->getDirectCallee(); + if (!Callee) + return false; + + auto overloadedOperatorType = Callee->getOverloadedOperator(); + if (overloadedOperatorType == OO_EqualEqual || + overloadedOperatorType == OO_ExclaimEqual || + overloadedOperatorType == OO_LessEqual || + overloadedOperatorType == OO_GreaterEqual || + overloadedOperatorType == OO_Spaceship || + overloadedOperatorType == OO_AmpAmp || + overloadedOperatorType == OO_PipePipe) + return true; + + if (isCtorOfRefCounted(Callee)) + return true; + + auto name = safeGetName(Callee); + if (name == "adoptRef" || name == "getPtr" || name == "WeakPtr" || + name == "makeWeakPtr" || name == "downcast" || name == "bitwise_cast" || + name == "is" || name == "equal" || name == "hash" || + name == "isType" + // FIXME: Most/all of these should be implemented via attributes. + || name == "equalIgnoringASCIICase" || + name == "equalIgnoringASCIICaseCommon" || + name == "equalIgnoringNullity") + return true; + + return false; + } + + void reportBug(const Expr *CallArg, const ParmVarDecl *Param) const { + assert(CallArg); + + SmallString<100> Buf; + llvm::raw_svector_ostream Os(Buf); + + const std::string paramName = safeGetName(Param); + Os << "Call argument"; + if (!paramName.empty()) { + Os << " for parameter "; + printQuotedQualifiedName(Os, Param); + } + Os << " is uncounted and unsafe."; + + const SourceLocation SrcLocToReport = + isa(CallArg) ? Param->getDefaultArg()->getExprLoc() + : CallArg->getSourceRange().getBegin(); + + PathDiagnosticLocation BSLoc(SrcLocToReport, BR->getSourceManager()); + auto Report = std::make_unique(Bug, Os.str(), BSLoc); + Report->addRange(CallArg->getSourceRange()); + BR->emitReport(std::move(Report)); + } +}; +} // namespace + +void ento::registerUncountedCallArgsChecker(CheckerManager &Mgr) { + Mgr.registerChecker(); +} + +bool ento::shouldRegisterUncountedCallArgsChecker(const CheckerManager &) { + return true; +} diff --git a/clang/test/Analysis/Checkers/WebKit/call-args.cpp b/clang/test/Analysis/Checkers/WebKit/call-args.cpp new file mode 100644 --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/call-args.cpp @@ -0,0 +1,344 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s + +#include "mock-types.h" + +RefCountable* provide() { return nullptr; } +void consume_refcntbl(RefCountable*) {} + +namespace simple { + void foo() { + consume_refcntbl(provide()); + // expected-warning@-1{{Call argument is uncounted and unsafe}} + } +} + +namespace multi_arg { + void consume_refcntbl(int, RefCountable* foo, bool) {} + void foo() { + consume_refcntbl(42, provide(), true); + // expected-warning@-1{{Call argument for parameter 'foo' is uncounted and unsafe}} + } +} + +namespace ref_counted { + Ref provide_ref_counted() { return Ref{}; } + void consume_ref_counted(Ref) {} + + void foo() { + consume_refcntbl(provide_ref_counted().get()); + // no warning + } +} + +namespace methods { + struct Consumer { + void consume_ptr(RefCountable* ptr) {} + void consume_ref(const RefCountable& ref) {} + }; + + void foo() { + Consumer c; + + c.consume_ptr(provide()); + // expected-warning@-1{{Call argument for parameter 'ptr' is uncounted and unsafe}} + c.consume_ref(*provide()); + // expected-warning@-1{{Call argument for parameter 'ref' is uncounted and unsafe}} + } + + void foo2() { + struct Consumer { + void consume(RefCountable*) { } + void whatever() { + consume(provide()); + // expected-warning@-1{{Call argument is uncounted and unsafe}} + } + }; + } + + void foo3() { + struct Consumer { + void consume(RefCountable*) { } + void whatever() { + this->consume(provide()); + // expected-warning@-1{{Call argument is uncounted and unsafe}} + } + }; + } +} + +namespace casts { + RefCountable* downcast(RefCountable*) { return nullptr; } + + void foo() { + consume_refcntbl(provide()); + // expected-warning@-1{{Call argument is uncounted and unsafe}} + + consume_refcntbl(static_cast(provide())); + // expected-warning@-1{{Call argument is uncounted and unsafe}} + + consume_refcntbl(dynamic_cast(provide())); + // expected-warning@-1{{Call argument is uncounted and unsafe}} + + consume_refcntbl(const_cast(provide())); + // expected-warning@-1{{Call argument is uncounted and unsafe}} + + consume_refcntbl(reinterpret_cast(provide())); + // expected-warning@-1{{Call argument is uncounted and unsafe}} + + consume_refcntbl(downcast(provide())); + // expected-warning@-1{{Call argument is uncounted and unsafe}} + + consume_refcntbl( + static_cast( + downcast( + static_cast( + provide() + ) + ) + ) + ); + // expected-warning@-8{{Call argument is uncounted and unsafe}} + } +} + +namespace null_ptr { + void foo_ref() { + consume_refcntbl(nullptr); + consume_refcntbl(0); + } +} + +namespace ref_counted_lookalike { + struct Decoy { + RefCountable* get() { return nullptr; } + }; + + void foo() { + Decoy D; + + consume_refcntbl(D.get()); + // expected-warning@-1{{Call argument is uncounted and unsafe}} + } +} + +namespace Ref_to_reference_conversion_operator { + template struct Ref { + Ref() = default; + Ref(T*) { } + T* get() { return nullptr; } + operator T& () { return t; } + T t; + }; + + void consume_ref(RefCountable&) {} + + void foo() { + Ref bar; + consume_ref(bar); + } +} + +namespace param_formarding_function { + void consume_ref_countable_ref(RefCountable&) {} + void consume_ref_countable_ptr(RefCountable*) {} + + namespace ptr { + void foo(RefCountable* param) { + consume_ref_countable_ptr(param); + } + } + + namespace ref { + void foo(RefCountable& param) { + consume_ref_countable_ref(param); + } + } + + namespace ref_deref_operators { + void foo_ref(RefCountable& param) { + consume_ref_countable_ptr(¶m); + } + + void foo_ptr(RefCountable* param) { + consume_ref_countable_ref(*param); + } + } + + namespace casts { + + RefCountable* downcast(RefCountable*) { return nullptr; } + + template + T* bitwise_cast(T*) { return nullptr; } + + void foo(RefCountable* param) { + consume_ref_countable_ptr(downcast(param)); + consume_ref_countable_ptr(bitwise_cast(param)); + } + } +} + +namespace param_formarding_lambda { + auto consume_ref_countable_ref = [](RefCountable&) {}; + auto consume_ref_countable_ptr = [](RefCountable*) {}; + + namespace ptr { + void foo(RefCountable* param) { + consume_ref_countable_ptr(param); + } + } + + namespace ref { + void foo(RefCountable& param) { + consume_ref_countable_ref(param); + } + } + + namespace ref_deref_operators { + void foo_ref(RefCountable& param) { + consume_ref_countable_ptr(¶m); + } + + void foo_ptr(RefCountable* param) { + consume_ref_countable_ref(*param); + } + } + + namespace casts { + + RefCountable* downcast(RefCountable*) { return nullptr; } + + template + T* bitwise_cast(T*) { return nullptr; } + + void foo(RefCountable* param) { + consume_ref_countable_ptr(downcast(param)); + consume_ref_countable_ptr(bitwise_cast(param)); + } + } +} + +namespace param_forwarding_method { + struct methodclass { + void consume_ref_countable_ref(RefCountable&) {}; + static void consume_ref_countable_ptr(RefCountable*) {}; + }; + + namespace ptr { + void foo(RefCountable* param) { + methodclass::consume_ref_countable_ptr(param); + } + } + + namespace ref { + void foo(RefCountable& param) { + methodclass mc; + mc.consume_ref_countable_ref(param); + } + } + + namespace ref_deref_operators { + void foo_ref(RefCountable& param) { + methodclass::consume_ref_countable_ptr(¶m); + } + + void foo_ptr(RefCountable* param) { + methodclass mc; + mc.consume_ref_countable_ref(*param); + } + } + + namespace casts { + + RefCountable* downcast(RefCountable*) { return nullptr; } + + template + T* bitwise_cast(T*) { return nullptr; } + + void foo(RefCountable* param) { + methodclass::consume_ref_countable_ptr(downcast(param)); + methodclass::consume_ref_countable_ptr(bitwise_cast(param)); + } + } +} + +namespace make_ref { + void makeRef(RefCountable*) {} + void makeRefPtr(RefCountable*) {} + void makeWeakPtr(RefCountable*) {} + void makeWeakPtr(RefCountable&) {} + + void foo() { + makeRef(provide()); + makeRefPtr(provide()); + RefPtr a(provide()); + Ref b(provide()); + makeWeakPtr(provide()); + makeWeakPtr(*provide()); + } +} + +namespace downcast { + void consume_ref_countable(RefCountable*) {} + RefCountable* downcast(RefCountable*) { return nullptr; } + + void foo() { + RefPtr bar; + consume_ref_countable( downcast(bar.get()) ); + } +} + +namespace string_impl { + struct String { + RefCountable* impl() { return nullptr; } + }; + + struct AtomString { + RefCountable rc; + RefCountable& impl() { return rc; } + }; + + void consume_ptr(RefCountable*) {} + void consume_ref(RefCountable&) {} + + namespace simple { + void foo() { + String s; + AtomString as; + consume_ptr(s.impl()); + consume_ref(as.impl()); + } + } +} + +namespace default_arg { + RefCountable* global; + + void function_with_default_arg(RefCountable* param = global) {} + // expected-warning@-1{{Call argument for parameter 'param' is uncounted and unsafe}} + + void foo() { + function_with_default_arg(); + } +} + +namespace cxx_member_operator_call { + // The hidden this-pointer argument without a corresponding parameter caused couple bugs in parameter <-> argument attribution. + struct Foo { + Foo& operator+(RefCountable* bad) { return *this; } + friend Foo& operator-(Foo& lhs, RefCountable* bad) { return lhs; } + void operator()(RefCountable* bad) { } + }; + + RefCountable* global; + + void foo() { + Foo f; + f + global; + // expected-warning@-1{{Call argument for parameter 'bad' is uncounted and unsafe}} + f - global; + // expected-warning@-1{{Call argument for parameter 'bad' is uncounted and unsafe}} + f(global); + // expected-warning@-1{{Call argument for parameter 'bad' is uncounted and unsafe}} + } +}