From: Devin Coughlin Date: Mon, 18 Jul 2016 17:23:30 +0000 (+0000) Subject: [analyzer] Add checker modeling potential C++ self-assignment X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=2cc3f4d704218646d2b4845f8535aa982f318f16;p=clang [analyzer] Add checker modeling potential C++ self-assignment This checker checks copy and move assignment operators whether they are protected against self-assignment. Since C++ core guidelines discourages explicit checking for `&rhs==this` in general we take a different approach: in top-frame analysis we branch the exploded graph for two cases, where &rhs==this and &rhs!=this and let existing checkers (e.g. unix.Malloc) do the rest of the work. It is important that we check all copy and move assignment operator in top frame even if we checked them already since self-assignments may happen undetected even in the same translation unit (e.g. using random indices for an array what may or may not be the same). A patch by Ádám Balogh! Differential Revision: https://reviews.llvm.org/D19311 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@275820 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Checkers/Checkers.td b/include/clang/StaticAnalyzer/Checkers/Checkers.td index 785e064e72..edf82a775e 100644 --- a/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -247,6 +247,10 @@ def NewDeleteLeaksChecker : Checker<"NewDeleteLeaks">, HelpText<"Check for memory leaks. Traces memory managed by new/delete.">, DescFile<"MallocChecker.cpp">; +def CXXSelfAssignmentChecker : Checker<"SelfAssignment">, + HelpText<"Checks C++ copy and move assignment operators for self assignment">, + DescFile<"CXXSelfAssignmentChecker.cpp">; + } // end: "cplusplus" let ParentPackage = CplusplusAlpha in { diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h index c954bbfad8..cb785f344c 100644 --- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h +++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h @@ -331,6 +331,22 @@ public: BugReport &BR) override; }; +class CXXSelfAssignmentBRVisitor final + : public BugReporterVisitorImpl { + + bool Satisfied; + +public: + CXXSelfAssignmentBRVisitor() : Satisfied(false) {} + + void Profile(llvm::FoldingSetNodeID &ID) const override {} + + PathDiagnosticPiece *VisitNode(const ExplodedNode *Succ, + const ExplodedNode *Pred, + BugReporterContext &BRC, + BugReport &BR) override; +}; + namespace bugreporter { /// Attempts to add visitors to trace a null or undefined value back to its diff --git a/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 62ccc3cb49..3510c510c7 100644 --- a/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -22,6 +22,7 @@ add_clang_library(clangStaticAnalyzerCheckers CheckerDocumentation.cpp ChrootChecker.cpp ClangCheckers.cpp + CXXSelfAssignmentChecker.cpp DeadStoresChecker.cpp DebugCheckers.cpp DereferenceChecker.cpp diff --git a/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp b/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp new file mode 100644 index 0000000000..7631322d25 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp @@ -0,0 +1,62 @@ +//=== CXXSelfAssignmentChecker.cpp -----------------------------*- C++ -*--===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This file defines CXXSelfAssignmentChecker, which tests all custom defined +// copy and move assignment operators for the case of self assignment, thus +// where the parameter refers to the same location where the this pointer +// points to. The checker itself does not do any checks at all, but it +// causes the analyzer to check every copy and move assignment operator twice: +// once for when 'this' aliases with the parameter and once for when it may not. +// It is the task of the other enabled checkers to find the bugs in these two +// different cases. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" + +using namespace clang; +using namespace ento; + +namespace { + +class CXXSelfAssignmentChecker : public Checker { +public: + CXXSelfAssignmentChecker(); + void checkBeginFunction(CheckerContext &C) const; +}; +} + +CXXSelfAssignmentChecker::CXXSelfAssignmentChecker() {} + +void CXXSelfAssignmentChecker::checkBeginFunction(CheckerContext &C) const { + if (!C.inTopFrame()) + return; + const auto *LCtx = C.getLocationContext(); + const auto *MD = dyn_cast(LCtx->getDecl()); + if (!MD) + return; + if (!MD->isCopyAssignmentOperator() && !MD->isMoveAssignmentOperator()) + return; + auto &State = C.getState(); + auto &SVB = C.getSValBuilder(); + auto ThisVal = + State->getSVal(SVB.getCXXThis(MD, LCtx->getCurrentStackFrame())); + auto Param = SVB.makeLoc(State->getRegion(MD->getParamDecl(0), LCtx)); + auto ParamVal = State->getSVal(Param); + ProgramStateRef SelfAssignState = State->bindLoc(Param, ThisVal); + C.addTransition(SelfAssignState); + ProgramStateRef NonSelfAssignState = State->bindLoc(Param, ParamVal); + C.addTransition(NonSelfAssignState); +} + +void ento::registerCXXSelfAssignmentChecker(CheckerManager &Mgr) { + Mgr.registerChecker(); +} diff --git a/lib/StaticAnalyzer/Core/BugReporter.cpp b/lib/StaticAnalyzer/Core/BugReporter.cpp index 488126b008..e04aa395d4 100644 --- a/lib/StaticAnalyzer/Core/BugReporter.cpp +++ b/lib/StaticAnalyzer/Core/BugReporter.cpp @@ -3104,6 +3104,7 @@ bool GRBugReporter::generatePathDiagnostic(PathDiagnostic& PD, R->addVisitor(llvm::make_unique()); R->addVisitor(llvm::make_unique()); R->addVisitor(llvm::make_unique()); + R->addVisitor(llvm::make_unique()); BugReport::VisitorList visitors; unsigned origReportConfigToken, finalReportConfigToken; diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 0e505463bb..5e1a482802 100644 --- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -1693,3 +1693,53 @@ UndefOrNullArgVisitor::VisitNode(const ExplodedNode *N, } return nullptr; } + +PathDiagnosticPiece * +CXXSelfAssignmentBRVisitor::VisitNode(const ExplodedNode *Succ, + const ExplodedNode *Pred, + BugReporterContext &BRC, BugReport &BR) { + if (Satisfied) + return nullptr; + + auto Edge = Succ->getLocation().getAs(); + if (!Edge.hasValue()) + return nullptr; + + auto Tag = Edge->getTag(); + if (!Tag) + return nullptr; + + if (Tag->getTagDescription() != "cplusplus.SelfAssignment") + return nullptr; + + Satisfied = true; + + const auto *Met = + dyn_cast(Succ->getCodeDecl().getAsFunction()); + assert(Met && "Not a C++ method."); + assert((Met->isCopyAssignmentOperator() || Met->isMoveAssignmentOperator()) && + "Not a copy/move assignment operator."); + + const auto *LCtx = Edge->getLocationContext(); + + const auto &State = Succ->getState(); + auto &SVB = State->getStateManager().getSValBuilder(); + + const auto Param = + State->getSVal(State->getRegion(Met->getParamDecl(0), LCtx)); + const auto This = + State->getSVal(SVB.getCXXThis(Met, LCtx->getCurrentStackFrame())); + + auto L = PathDiagnosticLocation::create(Met, BRC.getSourceManager()); + + if (!L.isValid() || !L.asLocation().isValid()) + return nullptr; + + const auto Msg = "Assuming " + Met->getParamDecl(0)->getName() + + ((Param == This) ? " == " : " != ") + "*this"; + + auto *Piece = new PathDiagnosticEventPiece(L, Msg.str()); + Piece->addRange(Met->getSourceRange()); + + return Piece; +} diff --git a/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp b/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp index 8ac229fc65..01324d0edd 100644 --- a/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp +++ b/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp @@ -431,6 +431,13 @@ static bool shouldSkipFunction(const Decl *D, // Count naming convention errors more aggressively. if (isa(D)) return false; + // We also want to reanalyze all C++ copy and move assignment operators to + // separately check the two cases where 'this' aliases with the parameter and + // where it may not. (cplusplus.SelfAssignmentChecker) + if (const auto *MD = dyn_cast(D)) { + if (MD->isCopyAssignmentOperator() || MD->isMoveAssignmentOperator()) + return false; + } // Otherwise, if we visited the function before, do not reanalyze it. return Visited.count(D); @@ -442,9 +449,7 @@ AnalysisConsumer::getInliningModeForFunction(const Decl *D, // We want to reanalyze all ObjC methods as top level to report Retain // Count naming convention errors more aggressively. But we should tune down // inlining when reanalyzing an already inlined function. - if (Visited.count(D)) { - assert(isa(D) && - "We are only reanalyzing ObjCMethods."); + if (Visited.count(D) && isa(D)) { const ObjCMethodDecl *ObjCM = cast(D); if (ObjCM->getMethodFamily() != OMF_init) return ExprEngine::Inline_Minimal; diff --git a/test/Analysis/self-assign.cpp b/test/Analysis/self-assign.cpp new file mode 100644 index 0000000000..74fb0fea24 --- /dev/null +++ b/test/Analysis/self-assign.cpp @@ -0,0 +1,89 @@ +// RUN: %clang_cc1 -std=c++11 -analyze -analyzer-checker=core,cplusplus,unix.Malloc,debug.ExprInspection %s -verify -analyzer-output=text + +extern "C" char *strdup(const char* s); +extern "C" void free(void* ptr); + +namespace std { +template struct remove_reference { typedef T type; }; +template struct remove_reference { typedef T type; }; +template struct remove_reference { typedef T type; }; +template typename remove_reference::type&& move(T&& t); +} + +void clang_analyzer_eval(int); + +class StringUsed { +public: + StringUsed(const char *s = "") : str(strdup(s)) {} + StringUsed(const StringUsed &rhs) : str(strdup(rhs.str)) {} + ~StringUsed(); + StringUsed& operator=(const StringUsed &rhs); + StringUsed& operator=(StringUsed &&rhs); + operator const char*() const; +private: + char *str; +}; + +StringUsed::~StringUsed() { + free(str); +} + +StringUsed& StringUsed::operator=(const StringUsed &rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}} + clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}} + free(str); // expected-note{{Memory is released}} + str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}} expected-note{{Use of memory after it is freed}} + return *this; +} + +StringUsed& StringUsed::operator=(StringUsed &&rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}} + clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}} + str = rhs.str; + rhs.str = nullptr; // FIXME: An improved leak checker should warn here + return *this; +} + +StringUsed::operator const char*() const { + return str; +} + +class StringUnused { +public: + StringUnused(const char *s = "") : str(strdup(s)) {} + StringUnused(const StringUnused &rhs) : str(strdup(rhs.str)) {} + ~StringUnused(); + StringUnused& operator=(const StringUnused &rhs); + StringUnused& operator=(StringUnused &&rhs); + operator const char*() const; +private: + char *str; +}; + +StringUnused::~StringUnused() { + free(str); +} + +StringUnused& StringUnused::operator=(const StringUnused &rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}} + clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}} + free(str); // expected-note{{Memory is released}} + str = strdup(rhs.str); // expected-warning{{Use of memory after it is freed}} expected-note{{Use of memory after it is freed}} + return *this; +} + +StringUnused& StringUnused::operator=(StringUnused &&rhs) { // expected-note{{Assuming rhs == *this}} expected-note{{Assuming rhs != *this}} + clang_analyzer_eval(*this == rhs); // expected-warning{{TRUE}} expected-warning{{UNKNOWN}} expected-note{{TRUE}} expected-note{{UNKNOWN}} + str = rhs.str; + rhs.str = nullptr; // FIXME: An improved leak checker should warn here + return *this; +} + +StringUnused::operator const char*() const { + return str; +} + + +int main() { + StringUsed s1 ("test"), s2; + s2 = s1; + s2 = std::move(s1); + return 0; +}