From 4689eec07ae36cc0bff1dd9f4bceda577a706dc1 Mon Sep 17 00:00:00 2001 From: Michael Layzell Date: Mon, 16 Jan 2017 19:11:41 -0500 Subject: [PATCH] Bug 1331434 - Part 1: Add an analysis to require a return after calls to annotated functions, r=ehsan MozReview-Commit-ID: 7NqXap8FdSn --- build/clang-plugin/Checks.inc | 1 + build/clang-plugin/ChecksIncludes.inc | 1 + build/clang-plugin/CustomMatchers.h | 5 + .../MustReturnFromCallerChecker.cpp | 105 ++++++++++ .../MustReturnFromCallerChecker.h | 26 +++ build/clang-plugin/RecurseGuard.h | 63 ++++++ build/clang-plugin/StmtToBlockMap.h | 86 ++++++++ build/clang-plugin/moz.build | 1 + build/clang-plugin/plugin.h | 1 + .../tests/TestMustReturnFromCaller.cpp | 183 ++++++++++++++++++ build/clang-plugin/tests/moz.build | 1 + mfbt/Attributes.h | 21 +- 12 files changed, 491 insertions(+), 3 deletions(-) create mode 100644 build/clang-plugin/MustReturnFromCallerChecker.cpp create mode 100644 build/clang-plugin/MustReturnFromCallerChecker.h create mode 100644 build/clang-plugin/RecurseGuard.h create mode 100644 build/clang-plugin/StmtToBlockMap.h create mode 100644 build/clang-plugin/tests/TestMustReturnFromCaller.cpp diff --git a/build/clang-plugin/Checks.inc b/build/clang-plugin/Checks.inc index ee6010120f68..2d90cbd92dda 100644 --- a/build/clang-plugin/Checks.inc +++ b/build/clang-plugin/Checks.inc @@ -10,6 +10,7 @@ CHECK(ExplicitImplicitChecker, "implicit-constructor") CHECK(ExplicitOperatorBoolChecker, "explicit-operator-bool") CHECK(KungFuDeathGripChecker, "kungfu-death-grip") CHECK(MustOverrideChecker, "must-override") +CHECK(MustReturnFromCallerChecker, "must-return-from-caller") CHECK(MustUseChecker, "must-use") CHECK(NaNExprChecker, "nan-expr") CHECK(NeedsNoVTableTypeChecker, "needs-no-vtable-type") diff --git a/build/clang-plugin/ChecksIncludes.inc b/build/clang-plugin/ChecksIncludes.inc index ec6bf561bd3f..56aa20c95bb6 100644 --- a/build/clang-plugin/ChecksIncludes.inc +++ b/build/clang-plugin/ChecksIncludes.inc @@ -11,6 +11,7 @@ #include "ExplicitOperatorBoolChecker.h" #include "KungFuDeathGripChecker.h" #include "MustOverrideChecker.h" +#include "MustReturnFromCallerChecker.h" #include "MustUseChecker.h" #include "NaNExprChecker.h" #include "NeedsNoVTableTypeChecker.h" diff --git a/build/clang-plugin/CustomMatchers.h b/build/clang-plugin/CustomMatchers.h index 456b06a7b89d..3206da380847 100644 --- a/build/clang-plugin/CustomMatchers.h +++ b/build/clang-plugin/CustomMatchers.h @@ -231,6 +231,11 @@ AST_MATCHER(CXXMethodDecl, isNonVirtual) { const CXXMethodDecl *Decl = Node.getCanonicalDecl(); return Decl && !Decl->isVirtual(); } + +AST_MATCHER(FunctionDecl, isMozMustReturnFromCaller) { + const FunctionDecl *Decl = Node.getCanonicalDecl(); + return Decl && hasCustomAnnotation(Decl, "moz_must_return_from_caller"); +} } } diff --git a/build/clang-plugin/MustReturnFromCallerChecker.cpp b/build/clang-plugin/MustReturnFromCallerChecker.cpp new file mode 100644 index 000000000000..d43204ad7dee --- /dev/null +++ b/build/clang-plugin/MustReturnFromCallerChecker.cpp @@ -0,0 +1,105 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include "MustReturnFromCallerChecker.h" +#include "CustomMatchers.h" + +void MustReturnFromCallerChecker::registerMatchers(MatchFinder* AstMatcher) { + // Look for a call to a MOZ_MUST_RETURN_FROM_CALLER function + AstMatcher->addMatcher(callExpr(callee(functionDecl(isMozMustReturnFromCaller())), + anyOf(hasAncestor(lambdaExpr().bind("containing-lambda")), + hasAncestor(functionDecl().bind("containing-func")))).bind("call"), + this); +} + +void MustReturnFromCallerChecker::check( + const MatchFinder::MatchResult& Result) { + const auto *ContainingLambda = + Result.Nodes.getNodeAs("containing-lambda"); + const auto *ContainingFunc = + Result.Nodes.getNodeAs("containing-func"); + const auto *Call = Result.Nodes.getNodeAs("call"); + + Stmt *Body = nullptr; + if (ContainingLambda) { + Body = ContainingLambda->getBody(); + } else if (ContainingFunc) { + Body = ContainingFunc->getBody(); + } else { + return; + } + assert(Body && "Should have a body by this point"); + + // Generate the CFG for the enclosing function or decl. + CFG::BuildOptions Options; + std::unique_ptr TheCFG = + CFG::buildCFG(nullptr, Body, Result.Context, Options); + if (!TheCFG) { + return; + } + + // Determine which block in the CFG we want to look at the successors of. + StmtToBlockMap BlockMap(TheCFG.get(), Result.Context); + size_t CallIndex; + const auto *Block = BlockMap.blockContainingStmt(Call, &CallIndex); + assert(Block && "This statement should be within the CFG!"); + + if (!immediatelyReturns(Block, Result.Context, CallIndex + 1)) { + diag(Call->getLocStart(), + "You must immediately return after calling this function", + DiagnosticIDs::Error); + } +} + +bool +MustReturnFromCallerChecker::immediatelyReturns(RecurseGuard Block, + ASTContext *TheContext, + size_t FromIdx) { + if (Block.isRepeat()) { + return false; + } + + for (size_t I = FromIdx; I < Block->size(); ++I) { + Optional S = (*Block)[I].getAs(); + if (!S) { + continue; + } + + auto AfterTrivials = IgnoreTrivials(S->getStmt()); + + // If we are looking at a ConstructExpr, a DeclRefExpr or a MemberExpr it's + // OK to use them after a call to a MOZ_MUST_RETURN_FROM_CALLER function. + // It is also, of course, OK to look at a ReturnStmt. + if (isa(AfterTrivials) || + isa(AfterTrivials) || + isa(AfterTrivials) || + isa(AfterTrivials)) { + continue; + } + + // It's also OK to call any function or method which is annotated with + // MOZ_MAY_CALL_AFTER_MUST_RETURN. We consider all CXXConversionDecls + // to be MOZ_MAY_CALL_AFTER_MUST_RETURN (like operator T*()). + if (auto CE = dyn_cast(AfterTrivials)) { + auto Callee = CE->getDirectCallee(); + if (Callee && hasCustomAnnotation(Callee, "moz_may_call_after_must_return")) { + continue; + } + + if (Callee && isa(Callee)) { + continue; + } + } + + // Otherwise, this expression is problematic. + return false; + } + + for (auto Succ = Block->succ_begin(); Succ != Block->succ_end(); ++Succ) { + if (!immediatelyReturns(Block.recurse(*Succ), TheContext, 0)) { + return false; + } + } + return true; +} diff --git a/build/clang-plugin/MustReturnFromCallerChecker.h b/build/clang-plugin/MustReturnFromCallerChecker.h new file mode 100644 index 000000000000..11dbdc5a34a5 --- /dev/null +++ b/build/clang-plugin/MustReturnFromCallerChecker.h @@ -0,0 +1,26 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#ifndef MustReturnFromCallerChecker_h__ +#define MustReturnFromCallerChecker_h__ + +#include "plugin.h" +#include "Utils.h" +#include "RecurseGuard.h" +#include "StmtToBlockMap.h" + +class MustReturnFromCallerChecker : public BaseCheck { +public: + MustReturnFromCallerChecker(StringRef CheckName, + ContextType *Context = nullptr) + : BaseCheck(CheckName, Context) {} + void registerMatchers(MatchFinder* AstMatcher) override; + void check(const MatchFinder::MatchResult &Result) override; +private: + bool immediatelyReturns(RecurseGuard Block, + ASTContext *TheContext, + size_t FromIdx); +}; + +#endif diff --git a/build/clang-plugin/RecurseGuard.h b/build/clang-plugin/RecurseGuard.h new file mode 100644 index 000000000000..1f1b359b94f0 --- /dev/null +++ b/build/clang-plugin/RecurseGuard.h @@ -0,0 +1,63 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#ifndef RecurseGuard_h__ +#define RecurseGuard_h__ + +#include "Utils.h" + +// This class acts as a tracker for avoiding infinite recursion when traversing +// chains in CFGs etc. +// +// Constructing a RecurseGuard sets up a shared backing store which tracks the +// currently observed objects. Whenever recursing, use RecurseGuard.recurse(T) +// to construct another RecurseGuard with the same backing store. +// +// The RecurseGuard object will unregister its object when it is destroyed, and +// has a method `isRepeat()` which will return `true` if the item was already +// seen. +template +class RecurseGuard { +public: + RecurseGuard(T Thing) : Thing(Thing), Set(new DenseSet()), Repeat(false) { + Set->insert(Thing); + } + RecurseGuard(T Thing, std::shared_ptr>& Set) + : Thing(Thing), Set(Set), Repeat(false) { + Repeat = !Set->insert(Thing).second; + } + RecurseGuard(const RecurseGuard &) = delete; + RecurseGuard(RecurseGuard && Other) + : Thing(Other.Thing), Set(Other.Set), Repeat(Other.Repeat) { + Other.Repeat = true; + } + ~RecurseGuard() { + if (!Repeat) { + Set->erase(Thing); + } + } + + bool isRepeat() { return Repeat; } + + T get() { return Thing; } + + operator T() { + return Thing; + } + + T operator ->() { + return Thing; + } + + RecurseGuard recurse(T NewThing) { + return RecurseGuard(NewThing, Set); + } + +private: + T Thing; + std::shared_ptr> Set; + bool Repeat; +}; + +#endif // RecurseGuard_h__ diff --git a/build/clang-plugin/StmtToBlockMap.h b/build/clang-plugin/StmtToBlockMap.h new file mode 100644 index 000000000000..7124ef8fbad9 --- /dev/null +++ b/build/clang-plugin/StmtToBlockMap.h @@ -0,0 +1,86 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#ifndef StmtToBlockMap_h__ +#define StmtToBlockMap_h__ + +#include "Utils.h" + +// This method is copied from clang-tidy's ExprSequence.cpp. +// +// Returns the Stmt nodes that are parents of 'S', skipping any potential +// intermediate non-Stmt nodes. +// +// In almost all cases, this function returns a single parent or no parents at +// all. +inline SmallVector getParentStmts(const Stmt *S, + ASTContext *Context) { + SmallVector Result; + + ASTContext::DynTypedNodeList Parents = Context->getParents(*S); + + SmallVector NodesToProcess(Parents.begin(), + Parents.end()); + + while (!NodesToProcess.empty()) { + ast_type_traits::DynTypedNode Node = NodesToProcess.back(); + NodesToProcess.pop_back(); + + if (const auto *S = Node.get()) { + Result.push_back(S); + } else { + Parents = Context->getParents(Node); + NodesToProcess.append(Parents.begin(), Parents.end()); + } + } + + return Result; +} + +// This class is a modified version of the class from clang-tidy's ExprSequence.cpp +// +// Maps `Stmt`s to the `CFGBlock` that contains them. Some `Stmt`s may be +// contained in more than one `CFGBlock`; in this case, they are mapped to the +// innermost block (i.e. the one that is furthest from the root of the tree). +// An optional outparameter provides the index into the block where the `Stmt` +// was found. +class StmtToBlockMap { +public: + // Initializes the map for the given `CFG`. + StmtToBlockMap(const CFG *TheCFG, ASTContext *TheContext) : Context(TheContext) { + for (const auto *B : *TheCFG) { + for (size_t I = 0; I < B->size(); ++I) { + if (Optional S = (*B)[I].getAs()) { + Map[S->getStmt()] = std::make_pair(B, I); + } + } + } + } + + // Returns the block that S is contained in. Some `Stmt`s may be contained + // in more than one `CFGBlock`; in this case, this function returns the + // innermost block (i.e. the one that is furthest from the root of the tree). + // + // The optional outparameter `Index` is set to the index into the block where + // the `Stmt` was found. + const CFGBlock *blockContainingStmt(const Stmt *S, size_t *Index = nullptr) const { + while (!Map.count(S)) { + SmallVector Parents = getParentStmts(S, Context); + if (Parents.empty()) + return nullptr; + S = Parents[0]; + } + + const auto &E = Map.lookup(S); + if (Index) *Index = E.second; + return E.first; + } + +private: + ASTContext *Context; + + llvm::DenseMap> Map; +}; + +#endif // StmtToBlockMap_h__ diff --git a/build/clang-plugin/moz.build b/build/clang-plugin/moz.build index b3548bf1bbda..36ed11a94d7b 100644 --- a/build/clang-plugin/moz.build +++ b/build/clang-plugin/moz.build @@ -16,6 +16,7 @@ UNIFIED_SOURCES += [ 'KungFuDeathGripChecker.cpp', 'MozCheckAction.cpp', 'MustOverrideChecker.cpp', + 'MustReturnFromCallerChecker.cpp', 'MustUseChecker.cpp', 'NaNExprChecker.cpp', 'NeedsNoVTableTypeChecker.cpp', diff --git a/build/clang-plugin/plugin.h b/build/clang-plugin/plugin.h index 9fc547d3487e..83e806dd1c5a 100644 --- a/build/clang-plugin/plugin.h +++ b/build/clang-plugin/plugin.h @@ -5,6 +5,7 @@ #ifndef plugin_h__ #define plugin_h__ +#include "clang/Analysis/CFG.h" #include "clang/AST/ASTConsumer.h" #include "clang/AST/ASTContext.h" #include "clang/AST/RecursiveASTVisitor.h" diff --git a/build/clang-plugin/tests/TestMustReturnFromCaller.cpp b/build/clang-plugin/tests/TestMustReturnFromCaller.cpp new file mode 100644 index 000000000000..5467d4ff95ff --- /dev/null +++ b/build/clang-plugin/tests/TestMustReturnFromCaller.cpp @@ -0,0 +1,183 @@ +#include + +#define MOZ_MUST_RETURN_FROM_CALLER __attribute__((annotate("moz_must_return_from_caller"))) +#define MOZ_MAY_CALL_AFTER_MUST_RETURN __attribute__((annotate("moz_may_call_after_must_return"))) + +void MOZ_MUST_RETURN_FROM_CALLER Throw() {} +void DoAnythingElse(); +int MakeAnInt(); +int MOZ_MAY_CALL_AFTER_MUST_RETURN SafeMakeInt(); +bool Condition(); + +class Foo { +public: + __attribute__((annotate("moz_implicit"))) Foo(std::nullptr_t); + Foo(); +}; + +void a1() { + Throw(); +} + +int a2() { + Throw(); // expected-error {{You must immediately return after calling this function}} + return MakeAnInt(); +} + +int a3() { + Throw(); + return 5; +} + +int a4() { + Throw(); // expected-error {{You must immediately return after calling this function}} + return Condition() ? MakeAnInt() : MakeAnInt(); +} + +void a5() { + Throw(); // expected-error {{You must immediately return after calling this function}} + DoAnythingElse(); +} + +int a6() { + Throw(); // expected-error {{You must immediately return after calling this function}} + DoAnythingElse(); + return MakeAnInt(); +} + +int a7() { + Throw(); // expected-error {{You must immediately return after calling this function}} + DoAnythingElse(); + return Condition() ? MakeAnInt() : MakeAnInt(); +} + +int a8() { + Throw(); + return SafeMakeInt(); +} + +int a9() { + if (Condition()) { + Throw(); + } + return SafeMakeInt(); +} + +void b1() { + if (Condition()) { + Throw(); + } +} + +int b2() { + if (Condition()) { + Throw(); // expected-error {{You must immediately return after calling this function}} + } + return MakeAnInt(); +} + +int b3() { + if (Condition()) { + Throw(); + } + return 5; +} + +int b4() { + if (Condition()) { + Throw(); // expected-error {{You must immediately return after calling this function}} + } + return Condition() ? MakeAnInt() : MakeAnInt(); +} + +void b5() { + if (Condition()) { + Throw(); // expected-error {{You must immediately return after calling this function}} + } + DoAnythingElse(); +} + +void b6() { + if (Condition()) { + Throw(); // expected-error {{You must immediately return after calling this function}} + DoAnythingElse(); + } +} + +void b7() { + if (Condition()) { + Throw(); + return; + } + DoAnythingElse(); +} + +void b8() { + if (Condition()) { + Throw(); // expected-error {{You must immediately return after calling this function}} + DoAnythingElse(); + return; + } + DoAnythingElse(); +} + +void b9() { + while (Condition()) { + Throw(); // expected-error {{You must immediately return after calling this function}} + } +} + +void b10() { + while (Condition()) { + Throw(); + return; + } +} + +void b11() { + Throw(); // expected-error {{You must immediately return after calling this function}} + if (Condition()) { + return; + } else { + return; + } +} + +void b12() { + switch (MakeAnInt()) { + case 1: + break; + default: + Throw(); + return; + } +} + +void b13() { + if (Condition()) { + Throw(); + } + return; +} + +Foo b14() { + if (Condition()) { + Throw(); + return nullptr; + } + return nullptr; +} + +Foo b15() { + if (Condition()) { + Throw(); + } + return nullptr; +} + +Foo b16() { + if (Condition()) { + Throw(); + } + return Foo(); +} diff --git a/build/clang-plugin/tests/moz.build b/build/clang-plugin/tests/moz.build index 1230a2aae1fa..c914d661fd5f 100644 --- a/build/clang-plugin/tests/moz.build +++ b/build/clang-plugin/tests/moz.build @@ -18,6 +18,7 @@ SOURCES += [ 'TestKungFuDeathGrip.cpp', 'TestMultipleAnnotations.cpp', 'TestMustOverride.cpp', + 'TestMustReturnFromCaller.cpp', 'TestMustUse.cpp', 'TestNANTestingExpr.cpp', 'TestNANTestingExprC.c', diff --git a/mfbt/Attributes.h b/mfbt/Attributes.h index b7fd72413ca7..cde4af526e42 100644 --- a/mfbt/Attributes.h +++ b/mfbt/Attributes.h @@ -510,9 +510,18 @@ * be used with types which are intended to be implicitly constructed into other * other types before being assigned to variables. * MOZ_REQUIRED_BASE_METHOD: Applies to virtual class method declarations. - * Sometimes derived classes override methods that need to be called by their - * overridden counterparts. This marker indicates that the marked method must - * be called by the method that it overrides. + * Sometimes derived classes override methods that need to be called by their + * overridden counterparts. This marker indicates that the marked method must + * be called by the method that it overrides. + * MOZ_MUST_RETURN_FROM_CALLER: Applies to function or method declarations. + * Callers of the annotated function/method must return from that function + * within the calling block using an explicit `return` statement. + * Only calls to Constructors, references to local and member variables, + * and calls to functions or methods marked as MOZ_MAY_CALL_AFTER_MUST_RETURN + * may be made after the MUST_RETURN_FROM_CALLER call. + * MOZ_MAY_CALL_AFTER_MUST_RETURN: Applies to function or method declarations. + * Calls to these methods may be made in functions after calls a + * MOZ_MUST_RETURN_FROM_CALLER function or method. */ #ifdef MOZ_CLANG_PLUGIN # define MOZ_MUST_OVERRIDE __attribute__((annotate("moz_must_override"))) @@ -550,6 +559,10 @@ __attribute__((annotate("moz_non_param"))) # define MOZ_REQUIRED_BASE_METHOD \ __attribute__((annotate("moz_required_base_method"))) +# define MOZ_MUST_RETURN_FROM_CALLER \ + __attribute__((annotate("moz_must_return_from_caller"))) +# define MOZ_MAY_CALL_AFTER_MUST_RETURN \ + __attribute__((annotate("moz_may_call_after_must_return"))) /* * It turns out that clang doesn't like void func() __attribute__ {} without a * warning, so use pragmas to disable the warning. This code won't work on GCC @@ -586,6 +599,8 @@ # define MOZ_NON_PARAM /* nothing */ # define MOZ_NON_AUTOABLE /* nothing */ # define MOZ_REQUIRED_BASE_METHOD /* nothing */ +# define MOZ_MUST_RETURN_FROM_CALLER /* nothing */ +# define MOZ_MAY_CALL_AFTER_MUST_RETURN /* nothing */ #endif /* MOZ_CLANG_PLUGIN */ #define MOZ_RAII MOZ_NON_TEMPORARY_CLASS MOZ_STACK_CLASS