From edb71ee4a0f935896b237b554ce8891204e7a71b Mon Sep 17 00:00:00 2001 From: "dcheng@chromium.org" Date: Mon, 18 Aug 2014 08:16:04 +0000 Subject: [PATCH] Add logic for catching unsafe scoped_refptr conversions to T* in returns The tool doesn't attempt to handle all potential unsafe conversions. It only attempts to catch a conservative subset of the conversions that are definitely unsafe, such as returning a local scoped_refptr or temporary. If it encounters something that isn't simple to resolve as safe/unsafe (for example, a reference may be bound to a variable with local storage, which is unsafe, or it may be bound to a member, which is probably safe), it prefers to skip rewriting anything at all, so it can be manually resolved. BUG=110610 R=rsleevi@chromium.org Review URL: https://codereview.chromium.org/472923002 Cr-Commit-Position: refs/heads/master@{#290225} git-svn-id: svn://svn.chromium.org/chrome/trunk/src@290225 0039d316-1c4b-4281-b951-d872f2087c98 --- .../RewriteScopedRefptr.cpp | 163 +++++++++++++----- .../tests/local-returned-as-raw-expected.cc | 11 +- .../tests/local-returned-as-raw-original.cc | 7 +- .../ref-to-local-returned-as-raw-expected.cc | 18 ++ .../ref-to-local-returned-as-raw-original.cc | 18 ++ .../tests/temp-returned-as-raw-expected.cc | 22 +++ .../tests/temp-returned-as-raw-original.cc | 22 +++ 7 files changed, 207 insertions(+), 54 deletions(-) create mode 100644 tools/clang/rewrite_scoped_refptr/tests/ref-to-local-returned-as-raw-expected.cc create mode 100644 tools/clang/rewrite_scoped_refptr/tests/ref-to-local-returned-as-raw-original.cc create mode 100644 tools/clang/rewrite_scoped_refptr/tests/temp-returned-as-raw-expected.cc create mode 100644 tools/clang/rewrite_scoped_refptr/tests/temp-returned-as-raw-original.cc diff --git a/tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp b/tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp index 1365aaf8d1a15f..a5394bf188ebf9 100644 --- a/tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp +++ b/tools/clang/rewrite_scoped_refptr/RewriteScopedRefptr.cpp @@ -61,6 +61,24 @@ bool NeedsParens(const clang::Expr* expr) { return false; } +Replacement RewriteRawPtrToScopedRefptr(const MatchFinder::MatchResult& result, + clang::SourceLocation begin, + clang::SourceLocation end) { + clang::CharSourceRange range = clang::CharSourceRange::getTokenRange( + result.SourceManager->getSpellingLoc(begin), + result.SourceManager->getSpellingLoc(end)); + + std::string text = clang::Lexer::getSourceText( + range, *result.SourceManager, result.Context->getLangOpts()); + text.erase(text.rfind('*')); + + std::string replacement_text("scoped_refptr<"); + replacement_text += text; + replacement_text += ">"; + + return Replacement(*result.SourceManager, range, replacement_text); +} + class GetRewriterCallback : public MatchFinder::MatchCallback { public: explicit GetRewriterCallback(Replacements* replacements) @@ -166,34 +184,62 @@ class VarRewriterCallback : public MatchFinder::MatchCallback { }; void VarRewriterCallback::run(const MatchFinder::MatchResult& result) { - const clang::CXXMemberCallExpr* const implicit_call = - result.Nodes.getNodeAs("call"); const clang::DeclaratorDecl* const var_decl = result.Nodes.getNodeAs("var"); - if (!implicit_call || !var_decl) + if (!var_decl) return; const clang::TypeSourceInfo* tsi = var_decl->getTypeSourceInfo(); - clang::CharSourceRange range = clang::CharSourceRange::getTokenRange( - result.SourceManager->getSpellingLoc(tsi->getTypeLoc().getBeginLoc()), - result.SourceManager->getSpellingLoc(tsi->getTypeLoc().getEndLoc())); - if (!range.isValid()) - return; + // TODO(dcheng): This mishandles a case where a variable has multiple + // declarations, e.g.: + // + // in .h: + // Foo* my_global_magical_foo; + // + // in .cc: + // Foo* my_global_magical_foo = CreateFoo(); + // + // In this case, it will only rewrite the .cc definition. Oh well. This should + // be rare enough that these cases can be manually handled, since the style + // guide prohibits globals of non-POD type. + replacements_->insert(RewriteRawPtrToScopedRefptr( + result, tsi->getTypeLoc().getBeginLoc(), tsi->getTypeLoc().getEndLoc())); +} - std::string text = clang::Lexer::getSourceText( - range, *result.SourceManager, result.Context->getLangOpts()); - if (text.empty()) +class FunctionRewriterCallback : public MatchFinder::MatchCallback { + public: + explicit FunctionRewriterCallback(Replacements* replacements) + : replacements_(replacements) {} + virtual void run(const MatchFinder::MatchResult& result) override; + + private: + Replacements* const replacements_; +}; + +void FunctionRewriterCallback::run(const MatchFinder::MatchResult& result) { + const clang::FunctionDecl* const function_decl = + result.Nodes.getNodeAs("fn"); + + if (!function_decl) return; - text.erase(text.rfind('*')); - std::string replacement_text("scoped_refptr<"); - replacement_text += text; - replacement_text += ">"; + // If matched against an implicit conversion to a DeclRefExpr, make sure the + // referenced declaration is of class type, e.g. the tool skips trying to + // chase pointers/references to determine if the pointee is a scoped_refptr + // with local storage. Instead, let a human manually handle those cases. + const clang::VarDecl* const var_decl = + result.Nodes.getNodeAs("var"); + if (var_decl && !var_decl->getTypeSourceInfo()->getType()->isClassType()) { + return; + } - replacements_->insert( - Replacement(*result.SourceManager, range, replacement_text)); + for (clang::FunctionDecl* f : function_decl->redecls()) { + clang::SourceRange range = f->getReturnTypeSourceRange(); + replacements_->insert( + RewriteRawPtrToScopedRefptr(result, range.getBegin(), range.getEnd())); + } } } // namespace @@ -211,26 +257,44 @@ int main(int argc, const char* argv[]) { // Finds all calls to conversion operator member function. This catches calls // to "operator T*", "operator Testable", and "operator bool" equally. - auto base_matcher = memberCallExpr( - thisPointerType(recordDecl(isSameOrDerivedFrom("::scoped_refptr"), - isTemplateInstantiation())), - callee(conversionDecl())); - - // The heuristic for whether or not a conversion is 'unsafe'. An unsafe - // conversion is one where a temporary scoped_refptr is converted to + auto base_matcher = + id("call", + memberCallExpr( + thisPointerType(recordDecl(isSameOrDerivedFrom("::scoped_refptr"), + isTemplateInstantiation())), + callee(conversionDecl()), + on(id("arg", expr())))); + + // The heuristic for whether or not converting a temporary is 'unsafe'. An + // unsafe conversion is one where a temporary scoped_refptr is converted to // another type. The matcher provides an exception for a temporary // scoped_refptr that is the result of an operator call. In this case, assume // that it's the result of an iterator dereference, and the container itself // retains the necessary reference, since this is a common idiom to see in // loop bodies. - auto is_unsafe_conversion = - bindTemporaryExpr(unless(has(operatorCallExpr()))); - - auto safe_conversion_matcher = memberCallExpr( - base_matcher, on(id("arg", expr(unless(is_unsafe_conversion))))); - - auto unsafe_conversion_matcher = - memberCallExpr(base_matcher, on(id("arg", is_unsafe_conversion))); + auto is_unsafe_temporary_conversion = + on(bindTemporaryExpr(unless(has(operatorCallExpr())))); + + // Returning a scoped_refptr as a T* is considered unsafe if either are + // true: + // - The scoped_refptr is a temporary. + // - The scoped_refptr has local lifetime. + auto returned_as_raw_ptr = hasParent( + returnStmt(hasAncestor(id("fn", functionDecl(returns(pointerType())))))); + // This matcher intentionally matches more than it should. For example, this + // will match: + // scoped_refptr& foo = some_other_foo; + // return foo; + // The matcher callback filters out VarDecls that aren't a scoped_refptr, + // so those cases can be manually handled. + auto is_local_variable = + on(declRefExpr(to(id("var", varDecl(hasLocalStorage()))))); + auto is_unsafe_return = + anyOf(allOf(hasParent(implicitCastExpr(returned_as_raw_ptr)), + is_local_variable), + allOf(hasParent(implicitCastExpr( + hasParent(exprWithCleanups(returned_as_raw_ptr)))), + is_unsafe_temporary_conversion)); // This catches both user-defined conversions (eg: "operator bool") and // standard conversion sequence (C++03 13.3.3.1.1), such as converting a @@ -243,31 +307,38 @@ int main(int argc, const char* argv[]) { auto bool_conversion_matcher = hasParent( expr(anyOf(implicit_to_bool, expr(hasParent(implicit_to_bool))))); - // Find all calls to an operator overload that do NOT (ultimately) result in - // being cast to a bool - eg: where it's being converted to T* and rewrite - // them to add a call to get(). + // Find all calls to an operator overload that are 'safe'. // // All bool conversions will be handled with the Testable trick, but that // can only be used once "operator T*" is removed, since otherwise it leaves // the call ambiguous. GetRewriterCallback get_callback(&replacements); - match_finder.addMatcher(id("call", safe_conversion_matcher), &get_callback); + match_finder.addMatcher( + memberCallExpr( + base_matcher, + unless(anyOf(is_unsafe_temporary_conversion, is_unsafe_return))), + &get_callback); // Find temporary scoped_refptr's being unsafely assigned to a T*. VarRewriterCallback var_callback(&replacements); + auto initialized_with_temporary = ignoringImpCasts(exprWithCleanups( + has(memberCallExpr(base_matcher, is_unsafe_temporary_conversion)))); + match_finder.addMatcher(id("var", + varDecl(hasInitializer(initialized_with_temporary), + hasType(pointerType()))), + &var_callback); match_finder.addMatcher( - id("var", - varDecl(hasInitializer(ignoringImpCasts(exprWithCleanups( - has(id("call", unsafe_conversion_matcher))))), - hasType(pointerType()))), - &var_callback); - match_finder.addMatcher( - constructorDecl(forEachConstructorInitializer(allOf( - withInitializer(ignoringImpCasts( - exprWithCleanups(has(id("call", unsafe_conversion_matcher))))), - forField(id("var", fieldDecl(hasType(pointerType()))))))), + constructorDecl(forEachConstructorInitializer( + allOf(withInitializer(initialized_with_temporary), + forField(id("var", fieldDecl(hasType(pointerType()))))))), &var_callback); + // Rewrite functions that unsafely turn a scoped_refptr into a T* when + // returning a value. + FunctionRewriterCallback fn_callback(&replacements); + match_finder.addMatcher(memberCallExpr(base_matcher, is_unsafe_return), + &fn_callback); + std::unique_ptr factory = clang::tooling::newFrontendActionFactory(&match_finder); int result = tool.run(factory.get()); diff --git a/tools/clang/rewrite_scoped_refptr/tests/local-returned-as-raw-expected.cc b/tools/clang/rewrite_scoped_refptr/tests/local-returned-as-raw-expected.cc index 340f636358736d..30b0f8341907b1 100644 --- a/tools/clang/rewrite_scoped_refptr/tests/local-returned-as-raw-expected.cc +++ b/tools/clang/rewrite_scoped_refptr/tests/local-returned-as-raw-expected.cc @@ -8,10 +8,11 @@ struct Foo { int dummy; }; -// Case 1: An example of an unsafe conversion, where the object is freed by -// the time the function returns. -Foo* GetBuggyFoo() { +// An example of an unsafe conversion, where the object is freed by the time the +// function returns. +scoped_refptr GetBuggyFoo(); + +scoped_refptr GetBuggyFoo() { scoped_refptr unsafe(new Foo); - // FIXME: The tool should rewrite the return type of the function. - return unsafe.get(); + return unsafe; } diff --git a/tools/clang/rewrite_scoped_refptr/tests/local-returned-as-raw-original.cc b/tools/clang/rewrite_scoped_refptr/tests/local-returned-as-raw-original.cc index 8c4631b4d77d75..7103cab8b5d7e3 100644 --- a/tools/clang/rewrite_scoped_refptr/tests/local-returned-as-raw-original.cc +++ b/tools/clang/rewrite_scoped_refptr/tests/local-returned-as-raw-original.cc @@ -8,10 +8,11 @@ struct Foo { int dummy; }; -// Case 1: An example of an unsafe conversion, where the object is freed by -// the time the function returns. +// An example of an unsafe conversion, where the object is freed by the time the +// function returns. +Foo* GetBuggyFoo(); + Foo* GetBuggyFoo() { scoped_refptr unsafe(new Foo); - // FIXME: The tool should rewrite the return type of the function. return unsafe; } diff --git a/tools/clang/rewrite_scoped_refptr/tests/ref-to-local-returned-as-raw-expected.cc b/tools/clang/rewrite_scoped_refptr/tests/ref-to-local-returned-as-raw-expected.cc new file mode 100644 index 00000000000000..86081203adf43a --- /dev/null +++ b/tools/clang/rewrite_scoped_refptr/tests/ref-to-local-returned-as-raw-expected.cc @@ -0,0 +1,18 @@ +// Copyright (c) 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/memory/ref_counted.h" + +struct Foo : public base::RefCounted { + int dummy; +}; + +// An example of an unsafe conversion, since the reference is bound to a +// scoped_refptr with local storage. The tool should ignore this, since it +// should prefer letting a human manually resolve trickier cases like this. +Foo* TestFunction() { + scoped_refptr a; + scoped_refptr& b = a; + return b; +} diff --git a/tools/clang/rewrite_scoped_refptr/tests/ref-to-local-returned-as-raw-original.cc b/tools/clang/rewrite_scoped_refptr/tests/ref-to-local-returned-as-raw-original.cc new file mode 100644 index 00000000000000..86081203adf43a --- /dev/null +++ b/tools/clang/rewrite_scoped_refptr/tests/ref-to-local-returned-as-raw-original.cc @@ -0,0 +1,18 @@ +// Copyright (c) 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/memory/ref_counted.h" + +struct Foo : public base::RefCounted { + int dummy; +}; + +// An example of an unsafe conversion, since the reference is bound to a +// scoped_refptr with local storage. The tool should ignore this, since it +// should prefer letting a human manually resolve trickier cases like this. +Foo* TestFunction() { + scoped_refptr a; + scoped_refptr& b = a; + return b; +} diff --git a/tools/clang/rewrite_scoped_refptr/tests/temp-returned-as-raw-expected.cc b/tools/clang/rewrite_scoped_refptr/tests/temp-returned-as-raw-expected.cc new file mode 100644 index 00000000000000..1987bbb3586c3f --- /dev/null +++ b/tools/clang/rewrite_scoped_refptr/tests/temp-returned-as-raw-expected.cc @@ -0,0 +1,22 @@ +// Copyright (c) 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/memory/ref_counted.h" + +struct Foo : public base::RefCounted { + int dummy; +}; + +class Bar { + scoped_refptr TestFunction(); +}; + +scoped_refptr CreateFoo(); + +// An example of an unsafe conversion--the scoped_refptr will be destroyed by +// the time function returns, since it's a temporary, so the returned raw +// pointer may point to a deleted object. +scoped_refptr Bar::TestFunction() { + return CreateFoo(); +} diff --git a/tools/clang/rewrite_scoped_refptr/tests/temp-returned-as-raw-original.cc b/tools/clang/rewrite_scoped_refptr/tests/temp-returned-as-raw-original.cc new file mode 100644 index 00000000000000..e0fd791d95a670 --- /dev/null +++ b/tools/clang/rewrite_scoped_refptr/tests/temp-returned-as-raw-original.cc @@ -0,0 +1,22 @@ +// Copyright (c) 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/memory/ref_counted.h" + +struct Foo : public base::RefCounted { + int dummy; +}; + +class Bar { + Foo* TestFunction(); +}; + +scoped_refptr CreateFoo(); + +// An example of an unsafe conversion--the scoped_refptr will be destroyed by +// the time function returns, since it's a temporary, so the returned raw +// pointer may point to a deleted object. +Foo* Bar::TestFunction() { + return CreateFoo(); +}