Index: include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- include/clang/StaticAnalyzer/Checkers/Checkers.td +++ include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -268,6 +268,11 @@ HelpText<"Check virtual function calls during construction or destruction">, DescFile<"VirtualCallChecker.cpp">; +def MisusedMovedObjectChecker: Checker<"MisusedMovedObject">, + HelpText<"Method calls on a moved-from object and copying a moved-from " + "object will be reported">, + DescFile<"MisusedMovedObject.cpp">; + } // end: "alpha.cplusplus" Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -36,6 +36,7 @@ FixedAddressChecker.cpp GenericTaintChecker.cpp IdenticalExprChecker.cpp + MisusedMovedObjectChecker.cpp IvarInvalidationChecker.cpp LLVMConventionsChecker.cpp LocalizationChecker.cpp Index: lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp =================================================================== --- /dev/null +++ lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp @@ -0,0 +1,494 @@ +//== MisusedMovedObjectChecker --------------*- C++ -*--==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This defines checker which checks for potential misuses of a moved-from +// object. That means method calls on the object or copying it in moved-from +// state. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/AST/ExprCXX.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" + +using namespace clang; +using namespace ento; + +namespace { + +struct RegionState { +private: + enum Kind { Moved, Reported } K; + RegionState(Kind InK, const Stmt *S) : K(InK), MoveStmt(S) {} + const Stmt *MoveStmt; + +public: + bool isReported() const { return K == Reported; } + bool isMoved() const { return K == Moved; } + + static RegionState getReported(const Stmt *S = nullptr) { + return RegionState(Reported, S); + } + static RegionState getMoved(const Stmt *S = nullptr) { + return RegionState(Moved, S); + } + + bool operator==(const RegionState &X) const { + return K == X.K && MoveStmt == X.MoveStmt; + } + void Profile(llvm::FoldingSetNodeID &ID) const { + ID.AddInteger(K); + ID.AddPointer(MoveStmt); + } + const Stmt *getMoveStmt() const { return MoveStmt; } +}; + +class MisusedMovedObjectChecker + : public Checker { +public: + void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const; + void checkPreCall(const CallEvent &MC, CheckerContext &C) const; + void checkPostCall(const CallEvent &MC, CheckerContext &C) const; + void checkEndFunction(CheckerContext &C) const; + ProgramStateRef checkPointerEscape(ProgramStateRef State, + const InvalidatedSymbols &Escaped, + const CallEvent *Call, + PointerEscapeKind Kind) const; + +private: + class MovedBugVisitor : public BugReporterVisitorImpl { + public: + MovedBugVisitor(const MemRegion *R) : Region(R), Found(false) {} + + void Profile(llvm::FoldingSetNodeID &ID) const override { + static int X = 0; + ID.AddPointer(&X); + ID.AddPointer(Region); + } + + PathDiagnosticPiece *VisitNode(const ExplodedNode *N, + const ExplodedNode *PrevN, + BugReporterContext &BRC, + BugReport &BR) override; + + private: + // The tracked region. + const MemRegion *Region; + bool Found; + }; + + mutable std::unique_ptr BT; + ExplodedNode *reportBug(const MemRegion *Region, const CallEvent &Call, + CheckerContext &C, bool isCopy) const; + bool isInMoveSafeContext(const LocationContext *LC) const; + bool isStateResetMethod(const CXXMethodDecl *MethodDec) const; + bool isMoveSafeMethod(const CXXMethodDecl *MethodDec) const; + const ExplodedNode *getMoveLocation(const ExplodedNode *N, + const MemRegion *Region, + CheckerContext &C) const; +}; +} // end anonymous namespace + +REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, RegionState) + +// If a region is removed all of the subregions needs to be removed too. +static ProgramStateRef removeFromState(ProgramStateRef State, + const MemRegion *Region) { + if (!Region) + return State; + // Note: The isSubRegionOf is not reflexive. + State = State->remove(Region); + for (auto &E : State->get()) { + if (E.first->isSubRegionOf(Region)) + State = State->remove(E.first); + } + return State; +} + +PathDiagnosticPiece *MisusedMovedObjectChecker::MovedBugVisitor::VisitNode( + const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC, + BugReport &BR) { + // We need only the last move of the reported object's region. + // The visitor walks the ExplodedGraph backwards. + if (Found) + return nullptr; + ProgramStateRef State = N->getState(); + ProgramStateRef StatePrev = PrevN->getState(); + const RegionState *TrackedObject = State->get(Region); + const RegionState *TrackedObjectPrev = + StatePrev->get(Region); + if (!TrackedObject) + return nullptr; + if (TrackedObjectPrev && TrackedObject) + return nullptr; + + // Retrieve the associated statement. + const Stmt *S = TrackedObject->getMoveStmt(); + if (!S) + S = PathDiagnosticLocation::getStmt(N); + if (!S) + return nullptr; + Found = true; + + std::string ObjectName; + if (const auto DecReg = Region->getAs()) { + const auto *RegionDecl = dyn_cast(DecReg->getDecl()); + ObjectName = RegionDecl->getNameAsString() + " "; + } + std::string InfoText = ObjectName + "became moved-from here"; + + // Generate the extra diagnostic. + PathDiagnosticLocation Pos(S, BRC.getSourceManager(), + N->getLocationContext()); + return new PathDiagnosticEventPiece(Pos, InfoText, true, nullptr); +} + +const ExplodedNode *MisusedMovedObjectChecker::getMoveLocation( + const ExplodedNode *N, const MemRegion *Region, CheckerContext &C) const { + // Walk the ExplodedGraph backwards and find the first node that referred to + // the tracked region. + const ExplodedNode *MoveNode = N; + + while (N) { + ProgramStateRef State = N->getState(); + if (!State->get(Region)) + break; + MoveNode = N; + N = N->pred_empty() ? nullptr : *(N->pred_begin()); + } + return MoveNode; +} + +ExplodedNode *MisusedMovedObjectChecker::reportBug(const MemRegion *Region, + const CallEvent &Call, + CheckerContext &C, + bool isCopy = false) const { + if (ExplodedNode *N = C.generateNonFatalErrorNode()) { + if (!BT) + BT.reset(new BugType(this, "Unspecified usage of a 'moved-from' object", + "CXXMoveSemantics")); + + // Uniqueing report to the same object. + PathDiagnosticLocation LocUsedForUniqueing; + const ExplodedNode *MoveNode = getMoveLocation(N, Region, C); + + if (const Stmt *MoveStmt = PathDiagnosticLocation::getStmt(MoveNode)) + LocUsedForUniqueing = PathDiagnosticLocation::createBegin( + MoveStmt, C.getSourceManager(), MoveNode->getLocationContext()); + + // Creating the error message. + std::string ErrorMessage; + if (isCopy) + ErrorMessage = "copying a 'moved-from' object"; + else + ErrorMessage = "method call on a 'moved-from' object"; + if (const auto DecReg = Region->getAs()) { + const auto *RegionDecl = dyn_cast(DecReg->getDecl()); + ErrorMessage += ", named: " + RegionDecl->getNameAsString(); + } + + auto R = + llvm::make_unique(*BT, ErrorMessage, N, LocUsedForUniqueing, + MoveNode->getLocationContext()->getDecl()); + R->addVisitor(llvm::make_unique(Region)); + C.emitReport(std::move(R)); + return N; + } + return nullptr; +} + +// Removing the function parameters' MemRegion from the state. This is needed +// for PODs where the trivial destructor does not even created nor executed. +void MisusedMovedObjectChecker::checkEndFunction(CheckerContext &C) const { + auto State = C.getState(); + TrackedRegionMapTy Objects = State->get(); + if (Objects.isEmpty()) + return; + + auto LC = C.getLocationContext(); + + const auto LD = dyn_cast_or_null(LC->getDecl()); + if (!LD) + return; + llvm::SmallSet InvalidRegions; + + for (auto Param : LD->parameters()) { + auto Type = Param->getType().getTypePtrOrNull(); + if (!Type) + continue; + if (!Type->isPointerType() && !Type->isReferenceType()) { + InvalidRegions.insert(State->getLValue(Param, LC).getAsRegion()); + } + } + + if (InvalidRegions.empty()) + return; + + for (const auto &E : State->get()) { + if (InvalidRegions.count(E.first->getBaseRegion())) + State = State->remove(E.first); + } + + C.addTransition(State); +} + +void MisusedMovedObjectChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + const auto *AFC = dyn_cast(&Call); + if (!AFC) + return; + + ProgramStateRef State = C.getState(); + const auto FunDecl = AFC->getDecl(); + if (!FunDecl) + return; + + // Whenever returning from a function with an unknown definition invalidate + // the objects' region which was passed by reference or as a pointer. + ExplodedNode *N = nullptr; + if (!FunDecl->getBody()) { + unsigned NumOfArgs = FunDecl->getNumParams(); + unsigned Idx = 0; + for (FunctionDecl::param_const_iterator I = FunDecl->param_begin(), + E = FunDecl->param_end(); + I != E && Idx < NumOfArgs; ++I, ++Idx) { + + ParmVarDecl *Param = *I; + if (Param->isParameterPack()) + break; + const Type *Tp = Param->getType().getTypePtrOrNull(); + if (!Tp) + continue; + + if ((Tp->isPointerType() || Tp->isReferenceType()) && + !Tp->getPointeeType().isConstQualified()) { + State = removeFromState(State, AFC->getArgSVal(Idx).getAsRegion()); + } + } + N = C.addTransition(State); + } + + // In system header we assume the object is not misused. + if (auto LocationDecl = Call.getLocationContext()->getDecl()) { + if (C.getSourceManager().isInSystemHeader(LocationDecl->getLocation())) + return; + } + const auto MethodDecl = dyn_cast_or_null(FunDecl); + if (!MethodDecl) + return; + + const auto *ConstructorDecl = dyn_cast(MethodDecl); + + const auto *CC = dyn_cast_or_null(&Call); + // Check if an object became moved-from. + // Object can become moved from after a call to move assignement operator or + // move constructor . + if (ConstructorDecl && !ConstructorDecl->isMoveConstructor()) + return; + + if (!ConstructorDecl && !MethodDecl->isMoveAssignmentOperator()) + return; + + const auto ArgRegion = AFC->getArgSVal(0).getAsRegion(); + if (!ArgRegion) + return; + + // Skip moving the object to itself. + if (CC && CC->getCXXThisVal().getAsRegion() == ArgRegion) + return; + if (const auto *IC = dyn_cast(AFC)) + if (IC->getCXXThisVal().getAsRegion() == ArgRegion) + return; + + const MemRegion *BaseRegion = ArgRegion->getBaseRegion(); + // Skip temp objects because of their short lifetime. + if (BaseRegion->getAs() || + AFC->getArgExpr(0)->isRValue()) + return; + // If it has already been reported do not need to modify the state. + if (State->get(ArgRegion)) + return; + // Mark object as moved-from. + State = State->set( + ArgRegion, RegionState::getMoved(AFC->getOriginExpr())); + + C.addTransition(State, N); +} + +bool MisusedMovedObjectChecker::isMoveSafeMethod( + const CXXMethodDecl *MethodDec) const { + // We abandon the cases where bool/void/void* conversion happens. + if (const auto *ConversionDec = + dyn_cast_or_null(MethodDec)) { + const Type *Tp = ConversionDec->getConversionType().getTypePtrOrNull(); + if (!Tp) + return false; + if (Tp->isBooleanType() || Tp->isVoidType() || Tp->isVoidPointerType()) + return true; + } + + return false; +} + +bool MisusedMovedObjectChecker::isStateResetMethod( + const CXXMethodDecl *MethodDec) const { + if (MethodDec && MethodDec->getDeclName().isIdentifier()) { + StringRef MethodName; + MethodName = MethodDec->getName(); + if (MethodName == "reset" || MethodName == "clear" || + MethodName == "destroy") + return true; + } + return false; +} + +// Don't report an error inside a move related operation. +// We assume that the programmer knows what she does. +bool MisusedMovedObjectChecker::isInMoveSafeContext( + const LocationContext *LC) const { + do { + StringRef MethodName; + const auto *CtxDec = LC->getDecl(); + auto *CtorDec = dyn_cast_or_null(CtxDec); + auto *DtorDec = dyn_cast_or_null(CtxDec); + auto *MethodDec = dyn_cast_or_null(CtxDec); + if (DtorDec || (CtorDec && CtorDec->isMoveConstructor()) || + (MethodDec && MethodDec->isOverloadedOperator() && + MethodDec->getOverloadedOperator() == OO_Equal) || + isStateResetMethod(MethodDec) || isMoveSafeMethod(MethodDec)) + return true; + } while ((LC = LC->getParent())); + return false; +} + +void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call, + CheckerContext &C) const { + ProgramStateRef State = C.getState(); + const LocationContext *LC = C.getLocationContext(); + ExplodedNode *N = nullptr; + + // Remove the MemRegions from the map on which a ctor/dtor call or assignement + // happened. + + // Checking constructor calls. + if (const auto *CC = dyn_cast(&Call)) { + State = removeFromState(State, CC->getCXXThisVal().getAsRegion()); + auto CtorDec = CC->getDecl(); + // Check for copying a moved-from object and report the bug. + if (CtorDec && CtorDec->isCopyOrMoveConstructor()) { + const MemRegion *ArgRegion = CC->getArgSVal(0).getAsRegion(); + const RegionState *ArgState = State->get(ArgRegion); + if (ArgState && ArgState->isMoved()) { + if (!isInMoveSafeContext(LC)) { + N = reportBug(ArgRegion, Call, C, /*isCopy=*/true); + State = State->set(ArgRegion, + RegionState::getReported()); + } + } + } + C.addTransition(State, N); + return; + } + + const auto IC = dyn_cast(&Call); + if (!IC) + return; + // In case of destructor call we do not track the object anymore. + const MemRegion *ThisRegion = IC->getCXXThisVal().getAsRegion(); + if (dyn_cast_or_null(Call.getDecl())) { + State = removeFromState(State, IC->getCXXThisVal().getAsRegion()); + C.addTransition(State); + return; + } + + const auto MethodDecl = dyn_cast_or_null(IC->getDecl()); + if (!MethodDecl) + return; + // Checking assignement operators. + bool OperatorEq = MethodDecl->isOverloadedOperator() && + MethodDecl->getOverloadedOperator() == OO_Equal; + // Remove the tracked object for every assignement operator, but report bug + // only for move or copy assignement's argument. + if (OperatorEq) { + State = removeFromState(State, ThisRegion); + if (MethodDecl->isCopyAssignmentOperator() || + MethodDecl->isMoveAssignmentOperator()) { + const RegionState *ArgState = + State->get(IC->getArgSVal(0).getAsRegion()); + if (ArgState && ArgState->isMoved() && !isInMoveSafeContext(LC)) { + const MemRegion *ArgRegion = IC->getArgSVal(0).getAsRegion(); + N = reportBug(ArgRegion, Call, C, /*isCopy=*/true); + State = + State->set(ArgRegion, RegionState::getReported()); + } + } + C.addTransition(State, N); + return; + } + + // The remaining part is check only for method call on a moved-from object. + if (isMoveSafeMethod(MethodDecl)) + return; + + if (isStateResetMethod(MethodDecl)) { + State = State->remove(ThisRegion); + C.addTransition(State); + return; + } + + // If it is already reported then we dont report the bug again. + const RegionState *ThisState = State->get(ThisRegion); + if (!(ThisState && ThisState->isMoved())) + return; + + if (isInMoveSafeContext(LC)) + return; + + N = reportBug(ThisRegion, Call, C); + State = State->set(ThisRegion, RegionState::getReported()); + C.addTransition(State, N); +} + +void MisusedMovedObjectChecker::checkDeadSymbols(SymbolReaper &SymReaper, + CheckerContext &C) const { + ProgramStateRef State = C.getState(); + TrackedRegionMapTy TrackedRegions = State->get(); + for (TrackedRegionMapTy::value_type E : TrackedRegions) { + const MemRegion *Region = E.first; + bool IsRegDead = !SymReaper.isLiveRegion(Region); + + // Remove the dead regions from the region map. + if (IsRegDead) { + State = State->remove(Region); + } + } + C.addTransition(State); +} + +ProgramStateRef MisusedMovedObjectChecker::checkPointerEscape( + ProgramStateRef State, const InvalidatedSymbols &Escaped, + const CallEvent *Call, PointerEscapeKind Kind) const { + for (InvalidatedSymbols::const_iterator I = Escaped.begin(), + E = Escaped.end(); + I != E; ++I) { + SymbolRef Sym = *I; + const auto *Region = Sym->getOriginRegion(); + State = removeFromState(State, Region); + } + return State; +} + +void ento::registerMisusedMovedObjectChecker(CheckerManager &mgr) { + mgr.registerChecker(); +} Index: test/Analysis/MisusedMovedObject.cpp =================================================================== --- /dev/null +++ test/Analysis/MisusedMovedObject.cpp @@ -0,0 +1,135 @@ +// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.MisusedMovedObject -std=c++11 -verify %s + +namespace std { + +template +struct remove_reference; + +template +struct remove_reference { typedef _Tp type; }; + +template +struct remove_reference<_Tp &> { typedef _Tp type; }; + +template +struct remove_reference<_Tp &&> { typedef _Tp type; }; + +template +typename remove_reference<_Tp>::type &&move(_Tp &&__t) { + return static_cast::type &&>(__t); +} + +template +_Tp &&forward(typename remove_reference<_Tp>::type &__t) noexcept { + return static_cast<_Tp &&>(__t); +} + +} // namespace std + +class B { +public: + B() = default; + B(const B &) = default; + B(B &&) = default; + void operator=(B &&b) { + return; + } + void foo() { return; } +}; + +class A { + int i; + double d; + B b; + +public: + A(int ii = 0, double dd = 0) : d(dd), i(ii) {} + void moveconstruct(A &&other) { + b = std::move(other.b); + d = other.d; + i = other.i; + return; + } + + static A get() { + A v(12, 13); + return v; + } + A(A *a) { + moveconstruct(std::move(*a)); + } + A(const A &other) : i(other.i), d(other.d), b(other.b) {} + A(A &&other) : i(other.i), d(other.d), b(std::move(other.b)) { + } + A(A &&other, char *k) { + moveconstruct(std::move(other)); + } + void operator=(A &&other) { + moveconstruct(std::move(other)); + return; + } + int foo() const { return 42; } + int bar() const { return 43; } + void reset() {} +}; + +int bignum(); + +void leftRefCall(A &a) { + a.foo(); +} +void rightRefCall(A &&a) { + a.foo(); +} +void copyOrMoveCall(const A a) { + a.foo(); +} + +void looptest() { + A Var; + for (int i = 0; i < 2; i++) { + rightRefCall(std::move(Var)); + } + for (int i = 0; i < 2; i++) { + leftRefCall(Var); + } + for (int i = 0; i < 2; i++) { + copyOrMoveCall(Var); + } + for (int i = 0; i < 2; i++) { + copyOrMoveCall(std::move(Var)); // expected-warning{{copying a 'moved-from' object, named: Var}} + } +} + +void uniqueTest() { + A a(42, 42.0); + A b = std::move(a); + if (bignum()) { + a.foo(); // expected-warning{{method call on a 'moved-from' object, named: a}} + } else { + a.bar(); + } + a.bar(); +} + +void PtrAndArrayTest() { + A *Ptr = new A(1, 1.5); + A Arr[10]; + Arr[2] = std::move(*Ptr); + (*Ptr).foo(); // expected-warning{{method call on a 'moved-from' object}} + Ptr = &Arr[1]; + Arr[3] = std::move(Arr[1]); + Ptr->foo(); // expected-warning{{method call on a 'moved-from' object}} + Arr[3] = std::move(Arr[2]); + Arr[2].foo(); // expected-warning{{method call on a 'moved-from' object}} + Arr[2] = std::move(Arr[3]); + Arr[2].foo(); // no warning, since new value is assigned +} + +void tempTest() { + A a = A::get(); + A::get().foo(); + for (int i = 0; i < bignum(); i++) { + A::get().foo(); + } +}