Index: clang/docs/analyzer/checkers.rst =================================================================== --- clang/docs/analyzer/checkers.rst +++ clang/docs/analyzer/checkers.rst @@ -1403,7 +1403,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 @@ -1418,6 +1418,63 @@ // ... }; +.. _webkit-WebKitUncountedCallArgsChecker: + +webkit.WebKitUncountedCallArgsChecker +""""""""""""""""""""""""""""""""""""" +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. + +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. + +Whitelist: + +- 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(null, 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 ``*`` + .. _alpha-checkers: Experimental Checkers Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1517,4 +1517,8 @@ def WebKitNoUncountedMemberChecker : Checker<"WebKitNoUncountedMemberChecker">, HelpText<"Check for no uncounted member variables.">, Documentation; + +def WebKitUncountedCallArgsChecker : Checker<"WebKitUncountedCallArgsChecker">, + HelpText<"Check uncounted call arguments.">, + Documentation; } // end webkit Index: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -121,8 +121,10 @@ ValistChecker.cpp VirtualCallChecker.cpp WebKit/NoUncountedMembersChecker.cpp + WebKit/ASTUtils.cpp WebKit/PtrTypesSemantics.cpp WebKit/RefCntblBaseVirtualDtorChecker.cpp + WebKit/UncountedCallArgsChecker.cpp LINK_LIBS clangAST Index: clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h =================================================================== --- clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h +++ 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.WebKitUncountedCallArgsChecker 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 Index: clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp =================================================================== --- /dev/null +++ clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp @@ -0,0 +1,91 @@ +//=======- 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}; + } + } + 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 Index: clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp =================================================================== --- /dev/null +++ clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp @@ -0,0 +1,197 @@ +//=======- 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/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> { +private: + BugType Bug; + mutable BugReporter *BR; + +public: + UncountedCallArgsChecker() + : Bug(this, + "Uncounted call argument for a raw pointer/reference parameter", + "WebKit coding guidelines") {} + + 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 call operators (e. g. lambda or + // std::function). + unsigned ParamIdx = 0; + if (auto operatorCall = dyn_cast(CE)) { + ParamIdx = operatorCall->getOperator() == OO_Call && + 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() && ParamIdx < CE->getNumArgs(); ++P, ++ParamIdx) { + // 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(ParamIdx); + + 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."; + + PathDiagnosticLocation BSLoc(CallArg->getSourceRange().getBegin(), + BR->getSourceManager()); + auto Report = std::make_unique(Bug, Os.str(), BSLoc); + Report->addRange(CallArg->getSourceRange()); + BR->emitReport(std::move(Report)); + } +}; +} // namespace + +void ento::registerWebKitUncountedCallArgsChecker(CheckerManager &Mgr) { + Mgr.registerChecker(); +} + +bool ento::shouldRegisterWebKitUncountedCallArgsChecker(const LangOptions &LO) { + return true; +} Index: clang/test/Analysis/Checkers/WebKit/call-args.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/Checkers/WebKit/call-args.cpp @@ -0,0 +1,312 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.WebKitUncountedCallArgsChecker -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()); + } + } +}