Index: include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- include/clang/StaticAnalyzer/Checkers/Checkers.td +++ include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -247,6 +247,10 @@ 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 { Index: include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h =================================================================== --- include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h +++ include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h @@ -331,6 +331,22 @@ 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 Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -22,6 +22,7 @@ CheckerDocumentation.cpp ChrootChecker.cpp ClangCheckers.cpp + CXXSelfAssignmentChecker.cpp DeadStoresChecker.cpp DebugCheckers.cpp DereferenceChecker.cpp Index: lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp =================================================================== --- /dev/null +++ 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(); +} Index: lib/StaticAnalyzer/Core/BugReporter.cpp =================================================================== --- lib/StaticAnalyzer/Core/BugReporter.cpp +++ lib/StaticAnalyzer/Core/BugReporter.cpp @@ -3104,6 +3104,7 @@ 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; Index: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp =================================================================== --- lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -1693,3 +1693,56 @@ } 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; + + SmallString<256> Buf; + llvm::raw_svector_ostream Out(Buf); + + Out << "Assuming " << Met->getParamDecl(0)->getName() << + ((Param == This) ? " == " : " != ") << "*this"; + + auto *Piece = new PathDiagnosticEventPiece(L, Out.str()); + Piece->addRange(Met->getSourceRange()); + + return Piece; +} Index: lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp =================================================================== --- lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp +++ lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp @@ -426,6 +426,13 @@ // 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); @@ -437,9 +444,7 @@ // 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; Index: test/Analysis/self-assign.cpp =================================================================== --- /dev/null +++ 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; +}