From 190e2cfd7459b5acc30f12702137ce33913b85b4 Mon Sep 17 00:00:00 2001 From: Benjamin Kramer Date: Tue, 8 Jul 2014 14:32:17 +0000 Subject: [PATCH] [clang-tidy] Add a little checker for Twine locals in LLVM. Those often cause use after free bugs and should be generally avoided. Technically it is safe to have a Twine with >=2 components in a variable but I don't think it is a good pattern to follow. The almost trivial checker comes with elaborated fix-it hints that turn the Twine into a std::string if necessary and otherwise fall back to the original type if the Twine is created from a single value. llvm-svn: 212535 --- .../clang-tidy/llvm/CMakeLists.txt | 1 + .../clang-tidy/llvm/LLVMTidyModule.cpp | 4 ++ .../clang-tidy/llvm/TwineLocalCheck.cpp | 64 +++++++++++++++++++ .../clang-tidy/llvm/TwineLocalCheck.h | 31 +++++++++ .../test/clang-tidy/llvm-twine-local.cpp | 35 ++++++++++ 5 files changed, 135 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/llvm/TwineLocalCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/llvm/TwineLocalCheck.h create mode 100644 clang-tools-extra/test/clang-tidy/llvm-twine-local.cpp diff --git a/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt b/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt index e3314d8eef39..d405874d3474 100644 --- a/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt @@ -4,6 +4,7 @@ add_clang_library(clangTidyLLVMModule IncludeOrderCheck.cpp LLVMTidyModule.cpp NamespaceCommentCheck.cpp + TwineLocalCheck.cpp LINK_LIBS clangAST diff --git a/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp b/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp index 31a114eeebdd..0586525228b7 100644 --- a/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp @@ -12,6 +12,7 @@ #include "../ClangTidyModuleRegistry.h" #include "IncludeOrderCheck.h" #include "NamespaceCommentCheck.h" +#include "TwineLocalCheck.h" namespace clang { namespace tidy { @@ -24,6 +25,9 @@ public: CheckFactories.addCheckFactory( "llvm-namespace-comment", new ClangTidyCheckFactory()); + CheckFactories.addCheckFactory( + "llvm-twine-local", + new ClangTidyCheckFactory()); } }; diff --git a/clang-tools-extra/clang-tidy/llvm/TwineLocalCheck.cpp b/clang-tools-extra/clang-tidy/llvm/TwineLocalCheck.cpp new file mode 100644 index 000000000000..f30628e23b6f --- /dev/null +++ b/clang-tools-extra/clang-tidy/llvm/TwineLocalCheck.cpp @@ -0,0 +1,64 @@ +//===--- TwineLocalCheck.cpp - clang-tidy ---------------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "TwineLocalCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Lex/Lexer.h" +#include "llvm/Support/raw_ostream.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { + +TwineLocalCheck::TwineLocalCheck() {} + +void TwineLocalCheck::registerMatchers(MatchFinder *Finder) { + auto TwineType = + qualType(hasDeclaration(recordDecl(hasName("::llvm::Twine")))); + Finder->addMatcher(varDecl(hasType(TwineType)).bind("variable"), this); +} + +void TwineLocalCheck::check(const MatchFinder::MatchResult &Result) { + const VarDecl *VD = Result.Nodes.getNodeAs("variable"); + auto Diag = diag(VD->getLocation(), + "twine variables are prone to use after free bugs"); + + // If this VarDecl has an initializer try to fix it. + if (VD->hasInit()) { + // Peel away implicit constructors and casts so we can see the actual type + // of the initializer. + const Expr *C = VD->getInit(); + while (isa(C)) + C = cast(C)->getArg(0)->IgnoreParenImpCasts(); + + SourceRange TypeRange = + VD->getTypeSourceInfo()->getTypeLoc().getSourceRange(); + + // A real Twine, turn it into a std::string. + if (VD->getType()->getCanonicalTypeUnqualified() == + C->getType()->getCanonicalTypeUnqualified()) { + SourceLocation EndLoc = Lexer::getLocForEndOfToken( + VD->getInit()->getLocEnd(), 0, *Result.SourceManager, + Result.Context->getLangOpts()); + Diag << FixItHint::CreateReplacement(TypeRange, "std::string") + << FixItHint::CreateInsertion(VD->getInit()->getLocStart(), "(") + << FixItHint::CreateInsertion(EndLoc, ").str()"); + } else { + // Just an implicit conversion. Insert the real type. + Diag << FixItHint::CreateReplacement( + TypeRange, + C->getType().getAsString(Result.Context->getPrintingPolicy())); + } + } +} + +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/llvm/TwineLocalCheck.h b/clang-tools-extra/clang-tidy/llvm/TwineLocalCheck.h new file mode 100644 index 000000000000..f538f81430c9 --- /dev/null +++ b/clang-tools-extra/clang-tidy/llvm/TwineLocalCheck.h @@ -0,0 +1,31 @@ +//===--- TwineLocalCheck.h - clang-tidy -------------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_TWINE_LOCAL_CHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_TWINE_LOCAL_CHECK_H + +#include "../ClangTidy.h" +#include "llvm/Support/Regex.h" + +namespace clang { +namespace tidy { + +/// \brief Looks for local Twine variables which are prone to use after frees +/// and should be generally avoided. +class TwineLocalCheck : public ClangTidyCheck { +public: + TwineLocalCheck(); + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_TWINE_LOCAL_CHECK_H diff --git a/clang-tools-extra/test/clang-tidy/llvm-twine-local.cpp b/clang-tools-extra/test/clang-tidy/llvm-twine-local.cpp new file mode 100644 index 000000000000..0f134a5500a6 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/llvm-twine-local.cpp @@ -0,0 +1,35 @@ +// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp +// RUN: clang-tidy %t.cpp -checks='-*,llvm-twine-local' -fix -- > %t.msg 2>&1 +// RUN: FileCheck -input-file=%t.cpp %s +// RUN: FileCheck -input-file=%t.msg -check-prefix=CHECK-MESSAGES %s + +namespace llvm { +class Twine { +public: + Twine(const char *); + Twine(int); + Twine &operator+(const Twine &); +}; +} + +using namespace llvm; + +void foo(const Twine &x); + +static Twine Moo = Twine("bark") + "bah"; +// CHECK-MASSAGES: twine variables are prone to use after free bugs +// CHECK-MESSAGES: note: FIX-IT applied suggested code changes +// CHECK: static std::string Moo = (Twine("bark") + "bah").str(); + +int main() { + const Twine t = Twine("a") + "b" + Twine(42); +// CHECK-MASSAGES: twine variables are prone to use after free bugs +// CHECK-MESSAGES: note: FIX-IT applied suggested code changes +// CHECK: std::string t = (Twine("a") + "b" + Twine(42)).str(); + foo(Twine("a") + "b"); + + Twine Prefix = false ? "__INT_FAST" : "__UINT_FAST"; +// CHECK-MASSAGES: twine variables are prone to use after free bugs +// CHECK-MESSAGES: note: FIX-IT applied suggested code changes +// CHECK: const char * Prefix = false ? "__INT_FAST" : "__UINT_FAST"; +}