diff options
Diffstat (limited to 'lib/StaticAnalyzer/Checkers')
26 files changed, 1125 insertions, 540 deletions
diff --git a/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp b/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp index 848c2662019a..b944f90539d4 100644 --- a/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp +++ b/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp @@ -259,6 +259,7 @@ void ArrayBoundCheckerV2::reportOOB(CheckerContext &checkerContext, llvm::make_unique<BugReport>(*BT, os.str(), errorNode)); } +#ifndef NDEBUG LLVM_DUMP_METHOD void RegionRawOffsetV2::dump() const { dumpToStream(llvm::errs()); } @@ -266,7 +267,7 @@ LLVM_DUMP_METHOD void RegionRawOffsetV2::dump() const { void RegionRawOffsetV2::dumpToStream(raw_ostream &os) const { os << "raw_offset_v2{" << getRegion() << ',' << getByteOffset() << '}'; } - +#endif // Lazily computes a value to be used by 'computeOffset'. If 'val' // is unknown or undefined, we lazily substitute '0'. Otherwise, diff --git a/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp index d19630eeef77..c31f2794df6a 100644 --- a/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp @@ -26,15 +26,22 @@ using namespace ento; namespace { -class BlockInCriticalSectionChecker : public Checker<check::PostCall, - check::PreCall> { +class BlockInCriticalSectionChecker : public Checker<check::PostCall> { + + mutable IdentifierInfo *IILockGuard, *IIUniqueLock; CallDescription LockFn, UnlockFn, SleepFn, GetcFn, FgetsFn, ReadFn, RecvFn, PthreadLockFn, PthreadTryLockFn, PthreadUnlockFn, MtxLock, MtxTimedLock, MtxTryLock, MtxUnlock; + StringRef ClassLockGuard, ClassUniqueLock; + + mutable bool IdentifierInfoInitialized; + std::unique_ptr<BugType> BlockInCritSectionBugType; + void initIdentifierInfo(ASTContext &Ctx) const; + void reportBlockInCritSection(SymbolRef FileDescSym, const CallEvent &call, CheckerContext &C) const; @@ -46,13 +53,10 @@ public: bool isLockFunction(const CallEvent &Call) const; bool isUnlockFunction(const CallEvent &Call) const; - void checkPreCall(const CallEvent &Call, CheckerContext &C) const; - /// Process unlock. /// Process lock. /// Process blocking functions (sleep, getc, fgets, read, recv) void checkPostCall(const CallEvent &Call, CheckerContext &C) const; - }; } // end anonymous namespace @@ -60,7 +64,8 @@ public: REGISTER_TRAIT_WITH_PROGRAMSTATE(MutexCounter, unsigned) BlockInCriticalSectionChecker::BlockInCriticalSectionChecker() - : LockFn("lock"), UnlockFn("unlock"), SleepFn("sleep"), GetcFn("getc"), + : IILockGuard(nullptr), IIUniqueLock(nullptr), + LockFn("lock"), UnlockFn("unlock"), SleepFn("sleep"), GetcFn("getc"), FgetsFn("fgets"), ReadFn("read"), RecvFn("recv"), PthreadLockFn("pthread_mutex_lock"), PthreadTryLockFn("pthread_mutex_trylock"), @@ -68,13 +73,29 @@ BlockInCriticalSectionChecker::BlockInCriticalSectionChecker() MtxLock("mtx_lock"), MtxTimedLock("mtx_timedlock"), MtxTryLock("mtx_trylock"), - MtxUnlock("mtx_unlock") { + MtxUnlock("mtx_unlock"), + ClassLockGuard("lock_guard"), + ClassUniqueLock("unique_lock"), + IdentifierInfoInitialized(false) { // Initialize the bug type. BlockInCritSectionBugType.reset( new BugType(this, "Call to blocking function in critical section", "Blocking Error")); } +void BlockInCriticalSectionChecker::initIdentifierInfo(ASTContext &Ctx) const { + if (!IdentifierInfoInitialized) { + /* In case of checking C code, or when the corresponding headers are not + * included, we might end up query the identifier table every time when this + * function is called instead of early returning it. To avoid this, a bool + * variable (IdentifierInfoInitialized) is used and the function will be run + * only once. */ + IILockGuard = &Ctx.Idents.get(ClassLockGuard); + IIUniqueLock = &Ctx.Idents.get(ClassUniqueLock); + IdentifierInfoInitialized = true; + } +} + bool BlockInCriticalSectionChecker::isBlockingFunction(const CallEvent &Call) const { if (Call.isCalled(SleepFn) || Call.isCalled(GetcFn) @@ -87,6 +108,12 @@ bool BlockInCriticalSectionChecker::isBlockingFunction(const CallEvent &Call) co } bool BlockInCriticalSectionChecker::isLockFunction(const CallEvent &Call) const { + if (const auto *Ctor = dyn_cast<CXXConstructorCall>(&Call)) { + auto IdentifierInfo = Ctor->getDecl()->getParent()->getIdentifier(); + if (IdentifierInfo == IILockGuard || IdentifierInfo == IIUniqueLock) + return true; + } + if (Call.isCalled(LockFn) || Call.isCalled(PthreadLockFn) || Call.isCalled(PthreadTryLockFn) @@ -99,6 +126,13 @@ bool BlockInCriticalSectionChecker::isLockFunction(const CallEvent &Call) const } bool BlockInCriticalSectionChecker::isUnlockFunction(const CallEvent &Call) const { + if (const auto *Dtor = dyn_cast<CXXDestructorCall>(&Call)) { + const auto *DRecordDecl = dyn_cast<CXXRecordDecl>(Dtor->getDecl()->getParent()); + auto IdentifierInfo = DRecordDecl->getIdentifier(); + if (IdentifierInfo == IILockGuard || IdentifierInfo == IIUniqueLock) + return true; + } + if (Call.isCalled(UnlockFn) || Call.isCalled(PthreadUnlockFn) || Call.isCalled(MtxUnlock)) { @@ -107,12 +141,10 @@ bool BlockInCriticalSectionChecker::isUnlockFunction(const CallEvent &Call) cons return false; } -void BlockInCriticalSectionChecker::checkPreCall(const CallEvent &Call, - CheckerContext &C) const { -} - void BlockInCriticalSectionChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const { + initIdentifierInfo(C.getASTContext()); + if (!isBlockingFunction(Call) && !isLockFunction(Call) && !isUnlockFunction(Call)) diff --git a/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 2759240dd276..7ab9c6114eae 100644 --- a/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -29,6 +29,7 @@ add_clang_library(clangStaticAnalyzerCheckers CXXSelfAssignmentChecker.cpp DeadStoresChecker.cpp DebugCheckers.cpp + DeleteWithNonVirtualDtorChecker.cpp DereferenceChecker.cpp DirectIvarAssignment.cpp DivZeroChecker.cpp @@ -56,6 +57,7 @@ add_clang_library(clangStaticAnalyzerCheckers NSErrorChecker.cpp NoReturnFunctionChecker.cpp NonNullParamChecker.cpp + NonnullGlobalConstantsChecker.cpp NullabilityChecker.cpp NumberObjectConversionChecker.cpp ObjCAtSyncChecker.cpp diff --git a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 77c24629d71e..28ad7e9e5071 100644 --- a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -289,8 +289,8 @@ ProgramStateRef CStringChecker::CheckLocation(CheckerContext &C, if (!ER) return state; - assert(ER->getValueType() == C.getASTContext().CharTy && - "CheckLocation should only be called with char* ElementRegions"); + if (ER->getValueType() != C.getASTContext().CharTy) + return state; // Get the size of the array. const SubRegion *superReg = cast<SubRegion>(ER->getSuperRegion()); @@ -874,6 +874,8 @@ bool CStringChecker::IsFirstBufInBound(CheckerContext &C, if (!ER) return true; // cf top comment. + // FIXME: Does this crash when a non-standard definition + // of a library function is encountered? assert(ER->getValueType() == C.getASTContext().CharTy && "IsFirstBufInBound should only be called with char* ElementRegions"); @@ -1050,31 +1052,22 @@ void CStringChecker::evalCopyCommon(CheckerContext &C, // If this is mempcpy, get the byte after the last byte copied and // bind the expr. if (IsMempcpy) { - loc::MemRegionVal destRegVal = destVal.castAs<loc::MemRegionVal>(); - - // Get the length to copy. - if (Optional<NonLoc> lenValNonLoc = sizeVal.getAs<NonLoc>()) { - // Get the byte after the last byte copied. - SValBuilder &SvalBuilder = C.getSValBuilder(); - ASTContext &Ctx = SvalBuilder.getContext(); - QualType CharPtrTy = Ctx.getPointerType(Ctx.CharTy); - loc::MemRegionVal DestRegCharVal = SvalBuilder.evalCast(destRegVal, - CharPtrTy, Dest->getType()).castAs<loc::MemRegionVal>(); - SVal lastElement = C.getSValBuilder().evalBinOpLN(state, BO_Add, - DestRegCharVal, - *lenValNonLoc, - Dest->getType()); - - // The byte after the last byte copied is the return value. - state = state->BindExpr(CE, LCtx, lastElement); - } else { - // If we don't know how much we copied, we can at least - // conjure a return value for later. - SVal result = C.getSValBuilder().conjureSymbolVal(nullptr, CE, LCtx, + // Get the byte after the last byte copied. + SValBuilder &SvalBuilder = C.getSValBuilder(); + ASTContext &Ctx = SvalBuilder.getContext(); + QualType CharPtrTy = Ctx.getPointerType(Ctx.CharTy); + SVal DestRegCharVal = + SvalBuilder.evalCast(destVal, CharPtrTy, Dest->getType()); + SVal lastElement = C.getSValBuilder().evalBinOp( + state, BO_Add, DestRegCharVal, sizeVal, Dest->getType()); + // If we don't know how much we copied, we can at least + // conjure a return value for later. + if (lastElement.isUnknown()) + lastElement = C.getSValBuilder().conjureSymbolVal(nullptr, CE, LCtx, C.blockCount()); - state = state->BindExpr(CE, LCtx, result); - } + // The byte after the last byte copied is the return value. + state = state->BindExpr(CE, LCtx, lastElement); } else { // All other copies return the destination buffer. // (Well, bcopy() has a void return type, but this won't hurt.) diff --git a/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp index 391b843ff3db..4b5e97b69295 100644 --- a/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp @@ -16,7 +16,7 @@ #include "clang/AST/Expr.h" #include "clang/AST/OperationKinds.h" #include "clang/AST/StmtVisitor.h" -#include "clang/Analysis/AnalysisContext.h" +#include "clang/Analysis/AnalysisDeclContext.h" #include "clang/Basic/TargetInfo.h" #include "clang/Basic/TypeTraits.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" diff --git a/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp b/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp index 07285d27ed9e..20a46843e23e 100644 --- a/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp @@ -179,7 +179,7 @@ bool CallAndMessageChecker::uninitRefOrPointer( if (const MemRegion *SValMemRegion = V.getAsRegion()) { const ProgramStateRef State = C.getState(); - const SVal PSV = State->getSVal(SValMemRegion); + const SVal PSV = State->getSVal(SValMemRegion, C.getASTContext().CharTy); if (PSV.isUndef()) { if (ExplodedNode *N = C.generateErrorNode()) { LazyInit_BT(BD, BT); diff --git a/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp b/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp index 60f16188bcf8..6dbacad7f2ea 100644 --- a/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp +++ b/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp @@ -13,7 +13,7 @@ #include "ClangSACheckers.h" #include "clang/AST/StmtVisitor.h" -#include "clang/Analysis/AnalysisContext.h" +#include "clang/Analysis/AnalysisDeclContext.h" #include "clang/Basic/TargetInfo.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/Checker.h" @@ -32,7 +32,6 @@ static bool isArc4RandomAvailable(const ASTContext &Ctx) { T.getOS() == llvm::Triple::FreeBSD || T.getOS() == llvm::Triple::NetBSD || T.getOS() == llvm::Triple::OpenBSD || - T.getOS() == llvm::Triple::Bitrig || T.getOS() == llvm::Triple::DragonFly; } diff --git a/lib/StaticAnalyzer/Checkers/CloneChecker.cpp b/lib/StaticAnalyzer/Checkers/CloneChecker.cpp index 83955c586b68..ee517ed97770 100644 --- a/lib/StaticAnalyzer/Checkers/CloneChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CloneChecker.cpp @@ -64,7 +64,7 @@ void CloneChecker::checkEndOfTranslationUnit(const TranslationUnitDecl *TU, // the CloneDetector. The only thing left to do is to report the found clones. int MinComplexity = Mgr.getAnalyzerOptions().getOptionAsInteger( - "MinimumCloneComplexity", 10, this); + "MinimumCloneComplexity", 50, this); assert(MinComplexity >= 0); bool ReportSuspiciousClones = Mgr.getAnalyzerOptions().getBooleanOption( @@ -81,11 +81,11 @@ void CloneChecker::checkEndOfTranslationUnit(const TranslationUnitDecl *TU, // because reportSuspiciousClones() wants to search them for errors. std::vector<CloneDetector::CloneGroup> AllCloneGroups; - Detector.findClones(AllCloneGroups, - FilenamePatternConstraint(IgnoredFilesPattern), - RecursiveCloneTypeIIConstraint(), - MinComplexityConstraint(MinComplexity), - MinGroupSizeConstraint(2), OnlyLargestCloneConstraint()); + Detector.findClones( + AllCloneGroups, FilenamePatternConstraint(IgnoredFilesPattern), + RecursiveCloneTypeIIHashConstraint(), MinGroupSizeConstraint(2), + MinComplexityConstraint(MinComplexity), + RecursiveCloneTypeIIVerifyConstraint(), OnlyLargestCloneConstraint()); if (ReportSuspiciousClones) reportSuspiciousClones(BR, Mgr, AllCloneGroups); diff --git a/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp b/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp index ea894c81011c..17ec2c288777 100644 --- a/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp @@ -123,57 +123,6 @@ void ConversionChecker::reportBug(ExplodedNode *N, CheckerContext &C, C.emitReport(std::move(R)); } -// Is E value greater or equal than Val? -static bool isGreaterEqual(CheckerContext &C, const Expr *E, - unsigned long long Val) { - ProgramStateRef State = C.getState(); - SVal EVal = C.getSVal(E); - if (EVal.isUnknownOrUndef()) - return false; - if (!EVal.getAs<NonLoc>() && EVal.getAs<Loc>()) { - ProgramStateManager &Mgr = C.getStateManager(); - EVal = - Mgr.getStoreManager().getBinding(State->getStore(), EVal.castAs<Loc>()); - } - if (EVal.isUnknownOrUndef() || !EVal.getAs<NonLoc>()) - return false; - - SValBuilder &Bldr = C.getSValBuilder(); - DefinedSVal V = Bldr.makeIntVal(Val, C.getASTContext().LongLongTy); - - // Is DefinedEVal greater or equal with V? - SVal GE = Bldr.evalBinOp(State, BO_GE, EVal, V, Bldr.getConditionType()); - if (GE.isUnknownOrUndef()) - return false; - ConstraintManager &CM = C.getConstraintManager(); - ProgramStateRef StGE, StLT; - std::tie(StGE, StLT) = CM.assumeDual(State, GE.castAs<DefinedSVal>()); - return StGE && !StLT; -} - -// Is E value negative? -static bool isNegative(CheckerContext &C, const Expr *E) { - ProgramStateRef State = C.getState(); - SVal EVal = State->getSVal(E, C.getLocationContext()); - if (EVal.isUnknownOrUndef() || !EVal.getAs<NonLoc>()) - return false; - DefinedSVal DefinedEVal = EVal.castAs<DefinedSVal>(); - - SValBuilder &Bldr = C.getSValBuilder(); - DefinedSVal V = Bldr.makeIntVal(0, false); - - SVal LT = - Bldr.evalBinOp(State, BO_LT, DefinedEVal, V, Bldr.getConditionType()); - - // Is E value greater than MaxVal? - ConstraintManager &CM = C.getConstraintManager(); - ProgramStateRef StNegative, StPositive; - std::tie(StNegative, StPositive) = - CM.assumeDual(State, LT.castAs<DefinedSVal>()); - - return StNegative && !StPositive; -} - bool ConversionChecker::isLossOfPrecision(const ImplicitCastExpr *Cast, QualType DestType, CheckerContext &C) const { @@ -195,18 +144,18 @@ bool ConversionChecker::isLossOfPrecision(const ImplicitCastExpr *Cast, return false; unsigned long long MaxVal = 1ULL << W; - return isGreaterEqual(C, Cast->getSubExpr(), MaxVal); + return C.isGreaterOrEqual(Cast->getSubExpr(), MaxVal); } bool ConversionChecker::isLossOfSign(const ImplicitCastExpr *Cast, - CheckerContext &C) const { + CheckerContext &C) const { QualType CastType = Cast->getType(); QualType SubType = Cast->IgnoreParenImpCasts()->getType(); if (!CastType->isUnsignedIntegerType() || !SubType->isSignedIntegerType()) return false; - return isNegative(C, Cast->getSubExpr()); + return C.isNegative(Cast->getSubExpr()); } void ento::registerConversionChecker(CheckerManager &mgr) { diff --git a/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp b/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp index 2eef1688d4c4..810a33ed404d 100644 --- a/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp +++ b/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp @@ -16,7 +16,6 @@ #include "clang/Analysis/Analyses/LiveVariables.h" #include "clang/Analysis/CallGraph.h" #include "clang/StaticAnalyzer/Core/Checker.h" -#include "clang/StaticAnalyzer/Core/IssueHash.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" @@ -213,35 +212,3 @@ void ento::registerExplodedGraphViewer(CheckerManager &mgr) { mgr.registerChecker<ExplodedGraphViewer>(); } -//===----------------------------------------------------------------------===// -// DumpBugHash -//===----------------------------------------------------------------------===// - -namespace { -class BugHashDumper : public Checker<check::PostStmt<Stmt>> { -public: - mutable std::unique_ptr<BugType> BT; - - void checkPostStmt(const Stmt *S, CheckerContext &C) const { - if (!BT) - BT.reset(new BugType(this, "Dump hash components", "debug")); - - ExplodedNode *N = C.generateNonFatalErrorNode(); - if (!N) - return; - - const LangOptions &Opts = C.getLangOpts(); - const SourceManager &SM = C.getSourceManager(); - FullSourceLoc FL(S->getLocStart(), SM); - std::string HashContent = - GetIssueString(SM, FL, getCheckName().getName(), BT->getCategory(), - C.getLocationContext()->getDecl(), Opts); - - C.emitReport(llvm::make_unique<BugReport>(*BT, HashContent, N)); - } -}; -} - -void ento::registerBugHashDumper(CheckerManager &mgr) { - mgr.registerChecker<BugHashDumper>(); -} diff --git a/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp b/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp new file mode 100644 index 000000000000..e04e2ab2c320 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp @@ -0,0 +1,153 @@ +//===-- DeleteWithNonVirtualDtorChecker.cpp -----------------------*- C++ -*--// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// Defines a checker for the OOP52-CPP CERT rule: Do not delete a polymorphic +// object without a virtual destructor. +// +// Diagnostic flags -Wnon-virtual-dtor and -Wdelete-non-virtual-dtor report if +// an object with a virtual function but a non-virtual destructor exists or is +// deleted, respectively. +// +// This check exceeds them by comparing the dynamic and static types of the +// object at the point of destruction and only warns if it happens through a +// pointer to a base type without a virtual destructor. The check places a note +// at the last point where the conversion from derived to base happened. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.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" +#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" + +using namespace clang; +using namespace ento; + +namespace { +class DeleteWithNonVirtualDtorChecker + : public Checker<check::PreStmt<CXXDeleteExpr>> { + mutable std::unique_ptr<BugType> BT; + + class DeleteBugVisitor : public BugReporterVisitorImpl<DeleteBugVisitor> { + public: + DeleteBugVisitor() : Satisfied(false) {} + void Profile(llvm::FoldingSetNodeID &ID) const override { + static int X = 0; + ID.AddPointer(&X); + } + std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N, + const ExplodedNode *PrevN, + BugReporterContext &BRC, + BugReport &BR) override; + + private: + bool Satisfied; + }; + +public: + void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const; +}; +} // end anonymous namespace + +void DeleteWithNonVirtualDtorChecker::checkPreStmt(const CXXDeleteExpr *DE, + CheckerContext &C) const { + const Expr *DeletedObj = DE->getArgument(); + const MemRegion *MR = C.getSVal(DeletedObj).getAsRegion(); + if (!MR) + return; + + const auto *BaseClassRegion = MR->getAs<TypedValueRegion>(); + const auto *DerivedClassRegion = MR->getBaseRegion()->getAs<SymbolicRegion>(); + if (!BaseClassRegion || !DerivedClassRegion) + return; + + const auto *BaseClass = BaseClassRegion->getValueType()->getAsCXXRecordDecl(); + const auto *DerivedClass = + DerivedClassRegion->getSymbol()->getType()->getPointeeCXXRecordDecl(); + if (!BaseClass || !DerivedClass) + return; + + if (!BaseClass->hasDefinition() || !DerivedClass->hasDefinition()) + return; + + if (BaseClass->getDestructor()->isVirtual()) + return; + + if (!DerivedClass->isDerivedFrom(BaseClass)) + return; + + if (!BT) + BT.reset(new BugType(this, + "Destruction of a polymorphic object with no " + "virtual destructor", + "Logic error")); + + ExplodedNode *N = C.generateNonFatalErrorNode(); + auto R = llvm::make_unique<BugReport>(*BT, BT->getName(), N); + + // Mark region of problematic base class for later use in the BugVisitor. + R->markInteresting(BaseClassRegion); + R->addVisitor(llvm::make_unique<DeleteBugVisitor>()); + C.emitReport(std::move(R)); +} + +std::shared_ptr<PathDiagnosticPiece> +DeleteWithNonVirtualDtorChecker::DeleteBugVisitor::VisitNode( + const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC, + BugReport &BR) { + // Stop traversal after the first conversion was found on a path. + if (Satisfied) + return nullptr; + + ProgramStateRef State = N->getState(); + const LocationContext *LC = N->getLocationContext(); + const Stmt *S = PathDiagnosticLocation::getStmt(N); + if (!S) + return nullptr; + + const auto *CastE = dyn_cast<CastExpr>(S); + if (!CastE) + return nullptr; + + // Only interested in DerivedToBase implicit casts. + // Explicit casts can have different CastKinds. + if (const auto *ImplCastE = dyn_cast<ImplicitCastExpr>(CastE)) { + if (ImplCastE->getCastKind() != CK_DerivedToBase) + return nullptr; + } + + // Region associated with the current cast expression. + const MemRegion *M = State->getSVal(CastE, LC).getAsRegion(); + if (!M) + return nullptr; + + // Check if target region was marked as problematic previously. + if (!BR.isInteresting(M)) + return nullptr; + + // Stop traversal on this path. + Satisfied = true; + + SmallString<256> Buf; + llvm::raw_svector_ostream OS(Buf); + OS << "Conversion from derived to base happened here"; + PathDiagnosticLocation Pos(S, BRC.getSourceManager(), + N->getLocationContext()); + return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true, + nullptr); +} + +void ento::registerDeleteWithNonVirtualDtorChecker(CheckerManager &mgr) { + mgr.registerChecker<DeleteWithNonVirtualDtorChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp b/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp index 0891ea85a714..23b43759a34b 100644 --- a/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp +++ b/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp @@ -546,8 +546,6 @@ void DynamicTypePropagation::checkPostStmt(const CastExpr *CE, OrigObjectPtrType = OrigObjectPtrType->stripObjCKindOfTypeAndQuals(ASTCtxt); DestObjectPtrType = DestObjectPtrType->stripObjCKindOfTypeAndQuals(ASTCtxt); - // TODO: erase tracked information when there is a cast to unrelated type - // and everything is unspecialized statically. if (OrigObjectPtrType->isUnspecialized() && DestObjectPtrType->isUnspecialized()) return; @@ -556,29 +554,31 @@ void DynamicTypePropagation::checkPostStmt(const CastExpr *CE, if (!Sym) return; - // Check which assignments are legal. - bool OrigToDest = - ASTCtxt.canAssignObjCInterfaces(DestObjectPtrType, OrigObjectPtrType); - bool DestToOrig = - ASTCtxt.canAssignObjCInterfaces(OrigObjectPtrType, DestObjectPtrType); const ObjCObjectPointerType *const *TrackedType = State->get<MostSpecializedTypeArgsMap>(Sym); - // Downcasts and upcasts handled in an uniform way regardless of being - // explicit. Explicit casts however can happen between mismatched types. - if (isa<ExplicitCastExpr>(CE) && !OrigToDest && !DestToOrig) { - // Mismatched types. If the DestType specialized, store it. Forget the - // tracked type otherwise. - if (DestObjectPtrType->isSpecialized()) { - State = State->set<MostSpecializedTypeArgsMap>(Sym, DestObjectPtrType); - C.addTransition(State, AfterTypeProp); - } else if (TrackedType) { + if (isa<ExplicitCastExpr>(CE)) { + // Treat explicit casts as an indication from the programmer that the + // Objective-C type system is not rich enough to express the needed + // invariant. In such cases, forget any existing information inferred + // about the type arguments. We don't assume the casted-to specialized + // type here because the invariant the programmer specifies in the cast + // may only hold at this particular program point and not later ones. + // We don't want a suppressing cast to require a cascade of casts down the + // line. + if (TrackedType) { State = State->remove<MostSpecializedTypeArgsMap>(Sym); C.addTransition(State, AfterTypeProp); } return; } + // Check which assignments are legal. + bool OrigToDest = + ASTCtxt.canAssignObjCInterfaces(DestObjectPtrType, OrigObjectPtrType); + bool DestToOrig = + ASTCtxt.canAssignObjCInterfaces(OrigObjectPtrType, DestObjectPtrType); + // The tracked type should be the sub or super class of the static destination // type. When an (implicit) upcast or a downcast happens according to static // types, and there is no subtyping relationship between the tracked and the diff --git a/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp b/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp index 32040e71163d..0005ec470d20 100644 --- a/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp @@ -8,10 +8,11 @@ //===----------------------------------------------------------------------===// #include "ClangSACheckers.h" +#include "clang/StaticAnalyzer/Checkers/SValExplainer.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/IssueHash.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" -#include "clang/StaticAnalyzer/Checkers/SValExplainer.h" #include "llvm/ADT/StringSwitch.h" #include "llvm/Support/ScopedPrinter.h" @@ -41,6 +42,7 @@ class ExprInspectionChecker : public Checker<eval::Call, check::DeadSymbols, void analyzerExplain(const CallExpr *CE, CheckerContext &C) const; void analyzerPrintState(const CallExpr *CE, CheckerContext &C) const; void analyzerGetExtent(const CallExpr *CE, CheckerContext &C) const; + void analyzerHashDump(const CallExpr *CE, CheckerContext &C) const; typedef void (ExprInspectionChecker::*FnCheck)(const CallExpr *, CheckerContext &C) const; @@ -79,6 +81,7 @@ bool ExprInspectionChecker::evalCall(const CallExpr *CE, &ExprInspectionChecker::analyzerPrintState) .Case("clang_analyzer_numTimesReached", &ExprInspectionChecker::analyzerNumTimesReached) + .Case("clang_analyzer_hashDump", &ExprInspectionChecker::analyzerHashDump) .Default(nullptr); if (!Handler) @@ -272,6 +275,7 @@ void ExprInspectionChecker::checkEndAnalysis(ExplodedGraph &G, BugReporter &BR, reportBug(llvm::to_string(NumTimesReached), BR, N); } + ReachedStats.clear(); } void ExprInspectionChecker::analyzerCrash(const CallExpr *CE, @@ -279,7 +283,18 @@ void ExprInspectionChecker::analyzerCrash(const CallExpr *CE, LLVM_BUILTIN_TRAP; } +void ExprInspectionChecker::analyzerHashDump(const CallExpr *CE, + CheckerContext &C) const { + const LangOptions &Opts = C.getLangOpts(); + const SourceManager &SM = C.getSourceManager(); + FullSourceLoc FL(CE->getArg(0)->getLocStart(), SM); + std::string HashContent = + GetIssueString(SM, FL, getCheckName().getName(), "Category", + C.getLocationContext()->getDecl(), Opts); + + reportBug(HashContent, C); +} + void ento::registerExprInspectionChecker(CheckerManager &Mgr) { Mgr.registerChecker<ExprInspectionChecker>(); } - diff --git a/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp b/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp index 883c6a663291..43966656cd8d 100644 --- a/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp @@ -466,7 +466,7 @@ bool GenericTaintChecker::checkPre(const CallExpr *CE, CheckerContext &C) const{ } Optional<SVal> GenericTaintChecker::getPointedToSVal(CheckerContext &C, - const Expr* Arg) { + const Expr *Arg) { ProgramStateRef State = C.getState(); SVal AddrVal = State->getSVal(Arg->IgnoreParens(), C.getLocationContext()); if (AddrVal.isUnknownOrUndef()) @@ -476,9 +476,18 @@ Optional<SVal> GenericTaintChecker::getPointedToSVal(CheckerContext &C, if (!AddrLoc) return None; - const PointerType *ArgTy = - dyn_cast<PointerType>(Arg->getType().getCanonicalType().getTypePtr()); - return State->getSVal(*AddrLoc, ArgTy ? ArgTy->getPointeeType(): QualType()); + QualType ArgTy = Arg->getType().getCanonicalType(); + if (!ArgTy->isPointerType()) + return None; + + QualType ValTy = ArgTy->getPointeeType(); + + // Do not dereference void pointers. Treat them as byte pointers instead. + // FIXME: we might want to consider more than just the first byte. + if (ValTy->isVoidType()) + ValTy = C.getASTContext().CharTy; + + return State->getSVal(*AddrLoc, ValTy); } ProgramStateRef diff --git a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp index 0c3bff5b63b8..cf57b8dca063 100644 --- a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp @@ -255,7 +255,10 @@ void FindIdenticalExprVisitor::checkComparisonOp(const BinaryOperator *B) { PathDiagnosticLocation ELoc = PathDiagnosticLocation::createOperatorLoc(B, BR.getSourceManager()); StringRef Message; - if (((Op == BO_EQ) || (Op == BO_LE) || (Op == BO_GE))) + if (Op == BO_Cmp) + Message = "comparison of identical expressions always evaluates to " + "'equal'"; + else if (((Op == BO_EQ) || (Op == BO_LE) || (Op == BO_GE))) Message = "comparison of identical expressions always evaluates to true"; else Message = "comparison of identical expressions always evaluates to false"; diff --git a/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp b/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp index decc552e1213..497978f07815 100644 --- a/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp @@ -56,8 +56,11 @@ public: ArrayRef<const MemRegion *> ExplicitRegions, ArrayRef<const MemRegion *> Regions, const LocationContext *LCtx, const CallEvent *Call) const; + void printState(raw_ostream &Out, ProgramStateRef State, + const char *NL, const char *Sep) const override; private: + enum MisuseKind {MK_FunCall, MK_Copy, MK_Move}; class MovedBugVisitor : public BugReporterVisitorImpl<MovedBugVisitor> { public: MovedBugVisitor(const MemRegion *R) : Region(R), Found(false) {} @@ -81,7 +84,7 @@ private: mutable std::unique_ptr<BugType> BT; ExplodedNode *reportBug(const MemRegion *Region, const CallEvent &Call, - CheckerContext &C, bool isCopy) const; + CheckerContext &C, MisuseKind MK) const; bool isInMoveSafeContext(const LocationContext *LC) const; bool isStateResetMethod(const CXXMethodDecl *MethodDec) const; bool isMoveSafeMethod(const CXXMethodDecl *MethodDec) const; @@ -177,7 +180,7 @@ const ExplodedNode *MisusedMovedObjectChecker::getMoveLocation( ExplodedNode *MisusedMovedObjectChecker::reportBug(const MemRegion *Region, const CallEvent &Call, CheckerContext &C, - bool isCopy = false) const { + MisuseKind MK) const { if (ExplodedNode *N = C.generateNonFatalErrorNode()) { if (!BT) BT.reset(new BugType(this, "Usage of a 'moved-from' object", @@ -193,10 +196,17 @@ ExplodedNode *MisusedMovedObjectChecker::reportBug(const MemRegion *Region, // Creating the error message. std::string ErrorMessage; - if (isCopy) - ErrorMessage = "Copying a 'moved-from' object"; - else - ErrorMessage = "Method call on a 'moved-from' object"; + switch(MK) { + case MK_FunCall: + ErrorMessage = "Method call on a 'moved-from' object"; + break; + case MK_Copy: + ErrorMessage = "Copying a 'moved-from' object"; + break; + case MK_Move: + ErrorMessage = "Moving a 'moved-from' object"; + break; + } if (const auto DecReg = Region->getAs<DeclRegion>()) { const auto *RegionDecl = dyn_cast<NamedDecl>(DecReg->getDecl()); ErrorMessage += " '" + RegionDecl->getNameAsString() + "'"; @@ -350,7 +360,7 @@ void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call, const LocationContext *LC = C.getLocationContext(); ExplodedNode *N = nullptr; - // Remove the MemRegions from the map on which a ctor/dtor call or assignement + // Remove the MemRegions from the map on which a ctor/dtor call or assignment // happened. // Checking constructor calls. @@ -363,7 +373,10 @@ void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call, const RegionState *ArgState = State->get<TrackedRegionMap>(ArgRegion); if (ArgState && ArgState->isMoved()) { if (!isInMoveSafeContext(LC)) { - N = reportBug(ArgRegion, Call, C, /*isCopy=*/true); + if(CtorDec->isMoveConstructor()) + N = reportBug(ArgRegion, Call, C, MK_Move); + else + N = reportBug(ArgRegion, Call, C, MK_Copy); State = State->set<TrackedRegionMap>(ArgRegion, RegionState::getReported()); } @@ -378,8 +391,11 @@ void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call, return; // In case of destructor call we do not track the object anymore. const MemRegion *ThisRegion = IC->getCXXThisVal().getAsRegion(); + if (!ThisRegion) + return; + if (dyn_cast_or_null<CXXDestructorDecl>(Call.getDecl())) { - State = removeFromState(State, IC->getCXXThisVal().getAsRegion()); + State = removeFromState(State, ThisRegion); C.addTransition(State); return; } @@ -400,7 +416,10 @@ void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call, State->get<TrackedRegionMap>(IC->getArgSVal(0).getAsRegion()); if (ArgState && ArgState->isMoved() && !isInMoveSafeContext(LC)) { const MemRegion *ArgRegion = IC->getArgSVal(0).getAsRegion(); - N = reportBug(ArgRegion, Call, C, /*isCopy=*/true); + if(MethodDecl->isMoveAssignmentOperator()) + N = reportBug(ArgRegion, Call, C, MK_Move); + else + N = reportBug(ArgRegion, Call, C, MK_Copy); State = State->set<TrackedRegionMap>(ArgRegion, RegionState::getReported()); } @@ -410,28 +429,35 @@ void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call, } // The remaining part is check only for method call on a moved-from object. + + // We want to investigate the whole object, not only sub-object of a parent + // class in which the encountered method defined. + while (const CXXBaseObjectRegion *BR = + dyn_cast<CXXBaseObjectRegion>(ThisRegion)) + ThisRegion = BR->getSuperRegion(); + if (isMoveSafeMethod(MethodDecl)) return; if (isStateResetMethod(MethodDecl)) { - State = State->remove<TrackedRegionMap>(ThisRegion); + State = removeFromState(State, ThisRegion); C.addTransition(State); return; } - // If it is already reported then we dont report the bug again. + // If it is already reported then we don't report the bug again. const RegionState *ThisState = State->get<TrackedRegionMap>(ThisRegion); if (!(ThisState && ThisState->isMoved())) return; - // Dont report it in case if any base region is already reported + // Don't report it in case if any base region is already reported if (isAnyBaseRegionReported(State, ThisRegion)) return; if (isInMoveSafeContext(LC)) return; - N = reportBug(ThisRegion, Call, C); + N = reportBug(ThisRegion, Call, C, MK_FunCall); State = State->set<TrackedRegionMap>(ThisRegion, RegionState::getReported()); C.addTransition(State, N); } @@ -476,6 +502,25 @@ ProgramStateRef MisusedMovedObjectChecker::checkRegionChanges( return State; } +void MisusedMovedObjectChecker::printState(raw_ostream &Out, + ProgramStateRef State, + const char *NL, + const char *Sep) const { + + TrackedRegionMapTy RS = State->get<TrackedRegionMap>(); + + if (!RS.isEmpty()) { + Out << Sep << "Moved-from objects :" << NL; + for (auto I: RS) { + I.first->dumpToStream(Out); + if (I.second.isMoved()) + Out << ": moved"; + else + Out << ": moved and reported"; + Out << NL; + } + } +} void ento::registerMisusedMovedObjectChecker(CheckerManager &mgr) { mgr.registerChecker<MisusedMovedObjectChecker>(); } diff --git a/lib/StaticAnalyzer/Checkers/NonnullGlobalConstantsChecker.cpp b/lib/StaticAnalyzer/Checkers/NonnullGlobalConstantsChecker.cpp new file mode 100644 index 000000000000..0b4ecb41d20f --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/NonnullGlobalConstantsChecker.cpp @@ -0,0 +1,140 @@ +//==- NonnullGlobalConstantsChecker.cpp ---------------------------*- C++ -*--// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This checker adds an assumption that constant globals of certain types* are +// non-null, as otherwise they generally do not convey any useful information. +// The assumption is useful, as many framework use e. g. global const strings, +// and the analyzer might not be able to infer the global value if the +// definition is in a separate translation unit. +// The following types (and their typedef aliases) are considered to be +// non-null: +// - `char* const` +// - `const CFStringRef` from CoreFoundation +// - `NSString* const` from Foundation +// - `CFBooleanRef` from Foundation +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.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/CheckerContext.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h" + +using namespace clang; +using namespace ento; + +namespace { + +class NonnullGlobalConstantsChecker : public Checker<check::Location> { + mutable IdentifierInfo *NSStringII = nullptr; + mutable IdentifierInfo *CFStringRefII = nullptr; + mutable IdentifierInfo *CFBooleanRefII = nullptr; + +public: + NonnullGlobalConstantsChecker() {} + + void checkLocation(SVal l, bool isLoad, const Stmt *S, + CheckerContext &C) const; + +private: + void initIdentifierInfo(ASTContext &Ctx) const; + + bool isGlobalConstString(SVal V) const; + + bool isNonnullType(QualType Ty) const; +}; + +} // namespace + +/// Lazily initialize cache for required identifier informations. +void NonnullGlobalConstantsChecker::initIdentifierInfo(ASTContext &Ctx) const { + if (NSStringII) + return; + + NSStringII = &Ctx.Idents.get("NSString"); + CFStringRefII = &Ctx.Idents.get("CFStringRef"); + CFBooleanRefII = &Ctx.Idents.get("CFBooleanRef"); +} + +/// Add an assumption that const string-like globals are non-null. +void NonnullGlobalConstantsChecker::checkLocation(SVal location, bool isLoad, + const Stmt *S, + CheckerContext &C) const { + initIdentifierInfo(C.getASTContext()); + if (!isLoad || !location.isValid()) + return; + + ProgramStateRef State = C.getState(); + SVal V = State->getSVal(location.castAs<Loc>()); + + if (isGlobalConstString(location)) { + Optional<DefinedOrUnknownSVal> Constr = V.getAs<DefinedOrUnknownSVal>(); + + if (Constr) { + + // Assume that the variable is non-null. + ProgramStateRef OutputState = State->assume(*Constr, true); + C.addTransition(OutputState); + } + } +} + +/// \param V loaded lvalue. +/// \return whether {@code val} is a string-like const global. +bool NonnullGlobalConstantsChecker::isGlobalConstString(SVal V) const { + Optional<loc::MemRegionVal> RegionVal = V.getAs<loc::MemRegionVal>(); + if (!RegionVal) + return false; + auto *Region = dyn_cast<VarRegion>(RegionVal->getAsRegion()); + if (!Region) + return false; + const VarDecl *Decl = Region->getDecl(); + + if (!Decl->hasGlobalStorage()) + return false; + + QualType Ty = Decl->getType(); + bool HasConst = Ty.isConstQualified(); + if (isNonnullType(Ty) && HasConst) + return true; + + // Look through the typedefs. + while (auto *T = dyn_cast<TypedefType>(Ty)) { + Ty = T->getDecl()->getUnderlyingType(); + + // It is sufficient for any intermediate typedef + // to be classified const. + HasConst = HasConst || Ty.isConstQualified(); + if (isNonnullType(Ty) && HasConst) + return true; + } + return false; +} + +/// \return whether {@code type} is extremely unlikely to be null +bool NonnullGlobalConstantsChecker::isNonnullType(QualType Ty) const { + + if (Ty->isPointerType() && Ty->getPointeeType()->isCharType()) + return true; + + if (auto *T = dyn_cast<ObjCObjectPointerType>(Ty)) { + return T->getInterfaceDecl() && + T->getInterfaceDecl()->getIdentifier() == NSStringII; + } else if (auto *T = dyn_cast<TypedefType>(Ty)) { + IdentifierInfo* II = T->getDecl()->getIdentifier(); + return II == CFStringRefII || II == CFBooleanRefII; + } + return false; +} + +void ento::registerNonnullGlobalConstantsChecker(CheckerManager &Mgr) { + Mgr.registerChecker<NonnullGlobalConstantsChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/ObjCContainersASTChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCContainersASTChecker.cpp index b10ec848ee46..e4737fcee7fb 100644 --- a/lib/StaticAnalyzer/Checkers/ObjCContainersASTChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ObjCContainersASTChecker.cpp @@ -13,7 +13,7 @@ //===----------------------------------------------------------------------===// #include "ClangSACheckers.h" #include "clang/AST/StmtVisitor.h" -#include "clang/Analysis/AnalysisContext.h" +#include "clang/Analysis/AnalysisDeclContext.h" #include "clang/Basic/TargetInfo.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/Checker.h" diff --git a/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp index 075ff09dcbfa..69b19a785938 100644 --- a/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp @@ -107,8 +107,6 @@ void ObjCSuperDeallocChecker::checkPreObjCMessage(const ObjCMethodCall &M, } reportUseAfterDealloc(ReceiverSymbol, Desc, M.getOriginExpr(), C); - - return; } void ObjCSuperDeallocChecker::checkPreCall(const CallEvent &Call, diff --git a/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp b/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp index 0e3a649e88f7..dab29be1c8fb 100644 --- a/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp @@ -81,6 +81,8 @@ class PthreadLockChecker public: void checkPostStmt(const CallExpr *CE, CheckerContext &C) const; void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const; + void printState(raw_ostream &Out, ProgramStateRef State, + const char *NL, const char *Sep) const override; void AcquireLock(CheckerContext &C, const CallExpr *CE, SVal lock, bool isTryLock, enum LockingSemantics semantics) const; @@ -184,6 +186,39 @@ ProgramStateRef PthreadLockChecker::resolvePossiblyDestroyedMutex( return state; } +void PthreadLockChecker::printState(raw_ostream &Out, ProgramStateRef State, + const char *NL, const char *Sep) const { + LockMapTy LM = State->get<LockMap>(); + if (!LM.isEmpty()) { + Out << Sep << "Mutex states:" << NL; + for (auto I : LM) { + I.first->dumpToStream(Out); + if (I.second.isLocked()) + Out << ": locked"; + else if (I.second.isUnlocked()) + Out << ": unlocked"; + else if (I.second.isDestroyed()) + Out << ": destroyed"; + else if (I.second.isUntouchedAndPossiblyDestroyed()) + Out << ": not tracked, possibly destroyed"; + else if (I.second.isUnlockedAndPossiblyDestroyed()) + Out << ": unlocked, possibly destroyed"; + Out << NL; + } + } + + LockSetTy LS = State->get<LockSet>(); + if (!LS.isEmpty()) { + Out << Sep << "Mutex lock order:" << NL; + for (auto I: LS) { + I->dumpToStream(Out); + Out << NL; + } + } + + // TODO: Dump destroyed mutex symbols? +} + void PthreadLockChecker::AcquireLock(CheckerContext &C, const CallExpr *CE, SVal lock, bool isTryLock, enum LockingSemantics semantics) const { diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp index 21ccf21515b3..e47494a3e90b 100644 --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp @@ -462,6 +462,7 @@ private: ArgEffect getDefaultArgEffect() const { return DefaultArgEffect; } friend class RetainSummaryManager; + friend class RetainCountChecker; }; } // end anonymous namespace @@ -1061,6 +1062,7 @@ RetainSummaryManager::getFunctionSummary(const FunctionDecl *FD) { // Inspect the result type. QualType RetTy = FT->getReturnType(); + std::string RetTyName = RetTy.getAsString(); // FIXME: This should all be refactored into a chain of "summary lookup" // filters. @@ -1080,12 +1082,14 @@ RetainSummaryManager::getFunctionSummary(const FunctionDecl *FD) { AllowAnnotations = false; } else if (FName == "CFPlugInInstanceCreate") { S = getPersistentSummary(RetEffect::MakeNoRet()); - } else if (FName == "IOBSDNameMatching" || + } else if (FName == "IORegistryEntrySearchCFProperty" + || (RetTyName == "CFMutableDictionaryRef" && ( + FName == "IOBSDNameMatching" || FName == "IOServiceMatching" || FName == "IOServiceNameMatching" || - FName == "IORegistryEntrySearchCFProperty" || FName == "IORegistryEntryIDMatching" || - FName == "IOOpenFirmwarePathMatching") { + FName == "IOOpenFirmwarePathMatching" + ))) { // Part of <rdar://problem/6961230>. (IOKit) // This should be addressed using a API table. S = getPersistentSummary(RetEffect::MakeOwned(RetEffect::CF), @@ -1166,6 +1170,11 @@ RetainSummaryManager::getFunctionSummary(const FunctionDecl *FD) { if (cocoa::isRefType(RetTy, "CF", FName)) { if (isRetain(FD, FName)) { S = getUnarySummary(FT, cfretain); + // CFRetain isn't supposed to be annotated. However, this may as well + // be a user-made "safe" CFRetain function that is incorrectly + // annotated as cf_returns_retained due to lack of better options. + // We want to ignore such annotation. + AllowAnnotations = false; } else if (isAutorelease(FD, FName)) { S = getUnarySummary(FT, cfautorelease); // The headers use cf_consumed, but we can fully model CFAutorelease @@ -1192,10 +1201,10 @@ RetainSummaryManager::getFunctionSummary(const FunctionDecl *FD) { break; } - // For the Disk Arbitration API (DiskArbitration/DADisk.h) - if (cocoa::isRefType(RetTy, "DADisk") || - cocoa::isRefType(RetTy, "DADissenter") || - cocoa::isRefType(RetTy, "DASessionRef")) { + // For all other CF-style types, use the Create/Get + // rule for summaries but don't support Retain functions + // with framework-specific prefixes. + if (coreFoundation::isCFObjectRef(RetTy)) { S = getCFCreateGetRuleSummary(FD); break; } @@ -1210,7 +1219,8 @@ RetainSummaryManager::getFunctionSummary(const FunctionDecl *FD) { // Check for release functions, the only kind of functions that we care // about that don't return a pointer type. - if (FName[0] == 'C' && (FName[1] == 'F' || FName[1] == 'G')) { + if (FName.size() >= 2 && + FName[0] == 'C' && (FName[1] == 'F' || FName[1] == 'G')) { // Test for 'CGCF'. FName = FName.substr(FName.startswith("CGCF") ? 4 : 2); @@ -1319,6 +1329,13 @@ static bool isTrustedReferenceCountImplementation(const FunctionDecl *FD) { return hasRCAnnotation(FD, "rc_ownership_trusted_implementation"); } +static bool isGeneralizedObjectRef(QualType Ty) { + if (Ty.getAsString().substr(0, 4) == "isl_") + return true; + else + return false; +} + //===----------------------------------------------------------------------===// // Summary creation for Selectors. //===----------------------------------------------------------------------===// @@ -1340,6 +1357,8 @@ RetainSummaryManager::getRetEffectFromAnnotations(QualType RetTy, if (D->hasAttr<CFReturnsRetainedAttr>()) return RetEffect::MakeOwned(RetEffect::CF); + else if (hasRCAnnotation(D, "rc_ownership_returns_retained")) + return RetEffect::MakeOwned(RetEffect::Generalized); if (D->hasAttr<CFReturnsNotRetainedAttr>()) return RetEffect::MakeNotOwned(RetEffect::CF); @@ -1363,9 +1382,11 @@ RetainSummaryManager::updateSummaryFromAnnotations(const RetainSummary *&Summ, const ParmVarDecl *pd = *pi; if (pd->hasAttr<NSConsumedAttr>()) Template->addArg(AF, parm_idx, DecRefMsg); - else if (pd->hasAttr<CFConsumedAttr>()) + else if (pd->hasAttr<CFConsumedAttr>() || + hasRCAnnotation(pd, "rc_ownership_consumed")) Template->addArg(AF, parm_idx, DecRef); - else if (pd->hasAttr<CFReturnsRetainedAttr>()) { + else if (pd->hasAttr<CFReturnsRetainedAttr>() || + hasRCAnnotation(pd, "rc_ownership_returns_retained")) { QualType PointeeTy = pd->getType()->getPointeeType(); if (!PointeeTy.isNull()) if (coreFoundation::isCFObjectRef(PointeeTy)) @@ -1844,6 +1865,15 @@ namespace { class CFRefLeakReport : public CFRefReport { const MemRegion* AllocBinding; + const Stmt *AllocStmt; + + // Finds the function declaration where a leak warning for the parameter 'sym' should be raised. + void deriveParamLocation(CheckerContext &Ctx, SymbolRef sym); + // Finds the location where a leak warning for 'sym' should be raised. + void deriveAllocLocation(CheckerContext &Ctx, SymbolRef sym); + // Produces description of a leak warning which is printed on the console. + void createDescription(CheckerContext &Ctx, bool GCEnabled, bool IncludeAllocationLine); + public: CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts, bool GCEnabled, const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym, @@ -1999,17 +2029,15 @@ CFRefReportVisitor::VisitNode(const ExplodedNode *N, const ExplodedNode *PrevN, } if (CurrV.getObjKind() == RetEffect::CF) { - if (Sym->getType().isNull()) { - os << " returns a Core Foundation object with a "; - } else { - os << " returns a Core Foundation object of type " - << Sym->getType().getAsString() << " with a "; - } - } - else { + os << " returns a Core Foundation object of type " + << Sym->getType().getAsString() << " with a "; + } else if (CurrV.getObjKind() == RetEffect::Generalized) { + os << " returns an object of type " << Sym->getType().getAsString() + << " with a "; + } else { assert (CurrV.getObjKind() == RetEffect::ObjC); QualType T = Sym->getType(); - if (T.isNull() || !isa<ObjCObjectPointerType>(T)) { + if (!isa<ObjCObjectPointerType>(T)) { os << " returns an Objective-C object with a "; } else { const ObjCObjectPointerType *PT = cast<ObjCObjectPointerType>(T); @@ -2425,13 +2453,25 @@ CFRefLeakReportVisitor::getEndPath(BugReporterContext &BRC, return llvm::make_unique<PathDiagnosticEventPiece>(L, os.str()); } -CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts, - bool GCEnabled, const SummaryLogTy &Log, - ExplodedNode *n, SymbolRef sym, - CheckerContext &Ctx, - bool IncludeAllocationLine) - : CFRefReport(D, LOpts, GCEnabled, Log, n, sym, false) { +void CFRefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) { + const SourceManager& SMgr = Ctx.getSourceManager(); + if (!sym->getOriginRegion()) + return; + + auto *Region = dyn_cast<DeclRegion>(sym->getOriginRegion()); + if (Region) { + const Decl *PDecl = Region->getDecl(); + if (PDecl && isa<ParmVarDecl>(PDecl)) { + PathDiagnosticLocation ParamLocation = PathDiagnosticLocation::create(PDecl, SMgr); + Location = ParamLocation; + UniqueingLocation = ParamLocation; + UniqueingDecl = Ctx.getLocationContext()->getDecl(); + } + } +} + +void CFRefLeakReport::deriveAllocLocation(CheckerContext &Ctx,SymbolRef sym) { // Most bug reports are cached at the location where they occurred. // With leaks, we want to unique them by the location where they were // allocated, and only report a single path. To do this, we need to find @@ -2455,8 +2495,12 @@ CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts, // FIXME: This will crash the analyzer if an allocation comes from an // implicit call (ex: a destructor call). // (Currently there are no such allocations in Cocoa, though.) - const Stmt *AllocStmt = PathDiagnosticLocation::getStmt(AllocNode); - assert(AllocStmt && "Cannot find allocation statement"); + AllocStmt = PathDiagnosticLocation::getStmt(AllocNode); + + if (!AllocStmt) { + AllocBinding = nullptr; + return; + } PathDiagnosticLocation AllocLocation = PathDiagnosticLocation::createBegin(AllocStmt, SMgr, @@ -2467,8 +2511,10 @@ CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts, // leaks should be uniqued on the allocation site. UniqueingLocation = AllocLocation; UniqueingDecl = AllocNode->getLocationContext()->getDecl(); +} - // Fill in the description of the bug. +void CFRefLeakReport::createDescription(CheckerContext &Ctx, bool GCEnabled, bool IncludeAllocationLine) { + assert(Location.isValid() && UniqueingDecl && UniqueingLocation.isValid()); Description.clear(); llvm::raw_string_ostream os(Description); os << "Potential leak "; @@ -2483,6 +2529,20 @@ CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts, os << " (allocated on line " << SL.getSpellingLineNumber() << ")"; } } +} + +CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts, + bool GCEnabled, const SummaryLogTy &Log, + ExplodedNode *n, SymbolRef sym, + CheckerContext &Ctx, + bool IncludeAllocationLine) + : CFRefReport(D, LOpts, GCEnabled, Log, n, sym, false) { + + deriveAllocLocation(Ctx, sym); + if (!AllocBinding) + deriveParamLocation(Ctx, sym); + + createDescription(Ctx, GCEnabled, IncludeAllocationLine); addVisitor(llvm::make_unique<CFRefLeakReportVisitor>(sym, GCEnabled, Log)); } @@ -2496,6 +2556,7 @@ class RetainCountChecker : public Checker< check::Bind, check::DeadSymbols, check::EndAnalysis, + check::BeginFunction, check::EndFunction, check::PostStmt<BlockExpr>, check::PostStmt<CastExpr>, @@ -2680,6 +2741,7 @@ public: SymbolRef Sym, ProgramStateRef state) const; void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const; + void checkBeginFunction(CheckerContext &C) const; void checkEndFunction(CheckerContext &C) const; ProgramStateRef updateSymbol(ProgramStateRef state, SymbolRef sym, @@ -3901,6 +3963,36 @@ RetainCountChecker::processLeaks(ProgramStateRef state, return N; } +void RetainCountChecker::checkBeginFunction(CheckerContext &Ctx) const { + if (!Ctx.inTopFrame()) + return; + + const LocationContext *LCtx = Ctx.getLocationContext(); + const FunctionDecl *FD = dyn_cast<FunctionDecl>(LCtx->getDecl()); + + if (!FD || isTrustedReferenceCountImplementation(FD)) + return; + + ProgramStateRef state = Ctx.getState(); + + const RetainSummary *FunctionSummary = getSummaryManager(Ctx).getFunctionSummary(FD); + ArgEffects CalleeSideArgEffects = FunctionSummary->getArgEffects(); + + for (unsigned idx = 0, e = FD->getNumParams(); idx != e; ++idx) { + const ParmVarDecl *Param = FD->getParamDecl(idx); + SymbolRef Sym = state->getSVal(state->getRegion(Param, LCtx)).getAsSymbol(); + + QualType Ty = Param->getType(); + const ArgEffect *AE = CalleeSideArgEffects.lookup(idx); + if (AE && *AE == DecRef && isGeneralizedObjectRef(Ty)) + state = setRefBinding(state, Sym, RefVal::makeOwned(RetEffect::ObjKind::Generalized, Ty)); + else if (isGeneralizedObjectRef(Ty)) + state = setRefBinding(state, Sym, RefVal::makeNotOwned(RetEffect::ObjKind::Generalized, Ty)); + } + + Ctx.addTransition(state); +} + void RetainCountChecker::checkEndFunction(CheckerContext &Ctx) const { ProgramStateRef state = Ctx.getState(); RefBindingsTy B = state->get<RefBindings>(); diff --git a/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index 556274d0edb6..25975628c553 100644 --- a/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -18,6 +18,7 @@ #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" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" #include "llvm/ADT/SmallString.h" @@ -26,85 +27,139 @@ using namespace clang; using namespace ento; namespace { -class StackAddrEscapeChecker : public Checker< check::PreStmt<ReturnStmt>, - check::EndFunction > { +class StackAddrEscapeChecker + : public Checker<check::PreCall, check::PreStmt<ReturnStmt>, + check::EndFunction> { + mutable IdentifierInfo *dispatch_semaphore_tII; mutable std::unique_ptr<BuiltinBug> BT_stackleak; mutable std::unique_ptr<BuiltinBug> BT_returnstack; + mutable std::unique_ptr<BuiltinBug> BT_capturedstackasync; + mutable std::unique_ptr<BuiltinBug> BT_capturedstackret; public: + enum CheckKind { + CK_StackAddrEscapeChecker, + CK_StackAddrAsyncEscapeChecker, + CK_NumCheckKinds + }; + + DefaultBool ChecksEnabled[CK_NumCheckKinds]; + + void checkPreCall(const CallEvent &Call, CheckerContext &C) const; void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const; void checkEndFunction(CheckerContext &Ctx) const; + private: + void checkReturnedBlockCaptures(const BlockDataRegion &B, + CheckerContext &C) const; + void checkAsyncExecutedBlockCaptures(const BlockDataRegion &B, + CheckerContext &C) const; void EmitStackError(CheckerContext &C, const MemRegion *R, const Expr *RetE) const; + bool isSemaphoreCaptured(const BlockDecl &B) const; static SourceRange genName(raw_ostream &os, const MemRegion *R, ASTContext &Ctx); + static SmallVector<const MemRegion *, 4> + getCapturedStackRegions(const BlockDataRegion &B, CheckerContext &C); + static bool isArcManagedBlock(const MemRegion *R, CheckerContext &C); + static bool isNotInCurrentFrame(const MemRegion *R, CheckerContext &C); }; -} +} // namespace SourceRange StackAddrEscapeChecker::genName(raw_ostream &os, const MemRegion *R, ASTContext &Ctx) { - // Get the base region, stripping away fields and elements. + // Get the base region, stripping away fields and elements. R = R->getBaseRegion(); SourceManager &SM = Ctx.getSourceManager(); SourceRange range; os << "Address of "; // Check if the region is a compound literal. - if (const CompoundLiteralRegion* CR = dyn_cast<CompoundLiteralRegion>(R)) { + if (const auto *CR = dyn_cast<CompoundLiteralRegion>(R)) { const CompoundLiteralExpr *CL = CR->getLiteralExpr(); os << "stack memory associated with a compound literal " "declared on line " - << SM.getExpansionLineNumber(CL->getLocStart()) - << " returned to caller"; + << SM.getExpansionLineNumber(CL->getLocStart()) << " returned to caller"; range = CL->getSourceRange(); - } - else if (const AllocaRegion* AR = dyn_cast<AllocaRegion>(R)) { + } else if (const auto *AR = dyn_cast<AllocaRegion>(R)) { const Expr *ARE = AR->getExpr(); SourceLocation L = ARE->getLocStart(); range = ARE->getSourceRange(); os << "stack memory allocated by call to alloca() on line " << SM.getExpansionLineNumber(L); - } - else if (const BlockDataRegion *BR = dyn_cast<BlockDataRegion>(R)) { + } else if (const auto *BR = dyn_cast<BlockDataRegion>(R)) { const BlockDecl *BD = BR->getCodeRegion()->getDecl(); SourceLocation L = BD->getLocStart(); range = BD->getSourceRange(); os << "stack-allocated block declared on line " << SM.getExpansionLineNumber(L); - } - else if (const VarRegion *VR = dyn_cast<VarRegion>(R)) { - os << "stack memory associated with local variable '" - << VR->getString() << '\''; + } else if (const auto *VR = dyn_cast<VarRegion>(R)) { + os << "stack memory associated with local variable '" << VR->getString() + << '\''; range = VR->getDecl()->getSourceRange(); - } - else if (const CXXTempObjectRegion *TOR = dyn_cast<CXXTempObjectRegion>(R)) { + } else if (const auto *TOR = dyn_cast<CXXTempObjectRegion>(R)) { QualType Ty = TOR->getValueType().getLocalUnqualifiedType(); os << "stack memory associated with temporary object of type '"; Ty.print(os, Ctx.getPrintingPolicy()); os << "'"; range = TOR->getExpr()->getSourceRange(); - } - else { + } else { llvm_unreachable("Invalid region in ReturnStackAddressChecker."); } return range; } -void StackAddrEscapeChecker::EmitStackError(CheckerContext &C, const MemRegion *R, - const Expr *RetE) const { - ExplodedNode *N = C.generateErrorNode(); +bool StackAddrEscapeChecker::isArcManagedBlock(const MemRegion *R, + CheckerContext &C) { + assert(R && "MemRegion should not be null"); + return C.getASTContext().getLangOpts().ObjCAutoRefCount && + isa<BlockDataRegion>(R); +} +bool StackAddrEscapeChecker::isNotInCurrentFrame(const MemRegion *R, + CheckerContext &C) { + const StackSpaceRegion *S = cast<StackSpaceRegion>(R->getMemorySpace()); + return S->getStackFrame() != C.getLocationContext()->getCurrentStackFrame(); +} + +bool StackAddrEscapeChecker::isSemaphoreCaptured(const BlockDecl &B) const { + if (!dispatch_semaphore_tII) + dispatch_semaphore_tII = &B.getASTContext().Idents.get("dispatch_semaphore_t"); + for (const auto &C : B.captures()) { + const auto *T = C.getVariable()->getType()->getAs<TypedefType>(); + if (T && T->getDecl()->getIdentifier() == dispatch_semaphore_tII) + return true; + } + return false; +} + +SmallVector<const MemRegion *, 4> +StackAddrEscapeChecker::getCapturedStackRegions(const BlockDataRegion &B, + CheckerContext &C) { + SmallVector<const MemRegion *, 4> Regions; + BlockDataRegion::referenced_vars_iterator I = B.referenced_vars_begin(); + BlockDataRegion::referenced_vars_iterator E = B.referenced_vars_end(); + for (; I != E; ++I) { + SVal Val = C.getState()->getSVal(I.getCapturedRegion()); + const MemRegion *Region = Val.getAsRegion(); + if (Region && isa<StackSpaceRegion>(Region->getMemorySpace())) + Regions.push_back(Region); + } + return Regions; +} + +void StackAddrEscapeChecker::EmitStackError(CheckerContext &C, + const MemRegion *R, + const Expr *RetE) const { + ExplodedNode *N = C.generateNonFatalErrorNode(); if (!N) return; - if (!BT_returnstack) - BT_returnstack.reset( - new BuiltinBug(this, "Return of address to stack-allocated memory")); - + BT_returnstack = llvm::make_unique<BuiltinBug>( + this, "Return of address to stack-allocated memory"); // Generate a report for this bug. - SmallString<512> buf; + SmallString<128> buf; llvm::raw_svector_ostream os(buf); SourceRange range = genName(os, R, C.getASTContext()); os << " returned to caller"; @@ -112,12 +167,88 @@ void StackAddrEscapeChecker::EmitStackError(CheckerContext &C, const MemRegion * report->addRange(RetE->getSourceRange()); if (range.isValid()) report->addRange(range); - C.emitReport(std::move(report)); } +void StackAddrEscapeChecker::checkAsyncExecutedBlockCaptures( + const BlockDataRegion &B, CheckerContext &C) const { + // There is a not-too-uncommon idiom + // where a block passed to dispatch_async captures a semaphore + // and then the thread (which called dispatch_async) is blocked on waiting + // for the completion of the execution of the block + // via dispatch_semaphore_wait. To avoid false-positives (for now) + // we ignore all the blocks which have captured + // a variable of the type "dispatch_semaphore_t". + if (isSemaphoreCaptured(*B.getDecl())) + return; + for (const MemRegion *Region : getCapturedStackRegions(B, C)) { + // The block passed to dispatch_async may capture another block + // created on the stack. However, there is no leak in this situaton, + // no matter if ARC or no ARC is enabled: + // dispatch_async copies the passed "outer" block (via Block_copy) + // and if the block has captured another "inner" block, + // the "inner" block will be copied as well. + if (isa<BlockDataRegion>(Region)) + continue; + ExplodedNode *N = C.generateNonFatalErrorNode(); + if (!N) + continue; + if (!BT_capturedstackasync) + BT_capturedstackasync = llvm::make_unique<BuiltinBug>( + this, "Address of stack-allocated memory is captured"); + SmallString<128> Buf; + llvm::raw_svector_ostream Out(Buf); + SourceRange Range = genName(Out, Region, C.getASTContext()); + Out << " is captured by an asynchronously-executed block"; + auto Report = + llvm::make_unique<BugReport>(*BT_capturedstackasync, Out.str(), N); + if (Range.isValid()) + Report->addRange(Range); + C.emitReport(std::move(Report)); + } +} + +void StackAddrEscapeChecker::checkReturnedBlockCaptures( + const BlockDataRegion &B, CheckerContext &C) const { + for (const MemRegion *Region : getCapturedStackRegions(B, C)) { + if (isArcManagedBlock(Region, C) || isNotInCurrentFrame(Region, C)) + continue; + ExplodedNode *N = C.generateNonFatalErrorNode(); + if (!N) + continue; + if (!BT_capturedstackret) + BT_capturedstackret = llvm::make_unique<BuiltinBug>( + this, "Address of stack-allocated memory is captured"); + SmallString<128> Buf; + llvm::raw_svector_ostream Out(Buf); + SourceRange Range = genName(Out, Region, C.getASTContext()); + Out << " is captured by a returned block"; + auto Report = + llvm::make_unique<BugReport>(*BT_capturedstackret, Out.str(), N); + if (Range.isValid()) + Report->addRange(Range); + C.emitReport(std::move(Report)); + } +} + +void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call, + CheckerContext &C) const { + if (!ChecksEnabled[CK_StackAddrAsyncEscapeChecker]) + return; + if (!Call.isGlobalCFunction("dispatch_after") && + !Call.isGlobalCFunction("dispatch_async")) + return; + for (unsigned Idx = 0, NumArgs = Call.getNumArgs(); Idx < NumArgs; ++Idx) { + if (const BlockDataRegion *B = dyn_cast_or_null<BlockDataRegion>( + Call.getArgSVal(Idx).getAsRegion())) + checkAsyncExecutedBlockCaptures(*B, C); + } +} + void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const { + if (!ChecksEnabled[CK_StackAddrEscapeChecker]) + return; const Expr *RetE = RS->getRetValue(); if (!RetE) @@ -127,25 +258,14 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, const LocationContext *LCtx = C.getLocationContext(); SVal V = C.getState()->getSVal(RetE, LCtx); const MemRegion *R = V.getAsRegion(); - if (!R) return; - const StackSpaceRegion *SS = - dyn_cast_or_null<StackSpaceRegion>(R->getMemorySpace()); - - if (!SS) - return; - - // Return stack memory in an ancestor stack frame is fine. - const StackFrameContext *CurFrame = LCtx->getCurrentStackFrame(); - const StackFrameContext *MemFrame = SS->getStackFrame(); - if (MemFrame != CurFrame) - return; + if (const BlockDataRegion *B = dyn_cast<BlockDataRegion>(R)) + checkReturnedBlockCaptures(*B, C); - // Automatic reference counting automatically copies blocks. - if (C.getASTContext().getLangOpts().ObjCAutoRefCount && - isa<BlockDataRegion>(R)) + if (!isa<StackSpaceRegion>(R->getMemorySpace()) || + isNotInCurrentFrame(R, C) || isArcManagedBlock(R, C)) return; // Returning a record by value is fine. (In this case, the returned @@ -169,7 +289,10 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, } void StackAddrEscapeChecker::checkEndFunction(CheckerContext &Ctx) const { - ProgramStateRef state = Ctx.getState(); + if (!ChecksEnabled[CK_StackAddrEscapeChecker]) + return; + + ProgramStateRef State = Ctx.getState(); // Iterate over all bindings to global variables and see if it contains // a memory region in the stack space. @@ -177,82 +300,73 @@ void StackAddrEscapeChecker::checkEndFunction(CheckerContext &Ctx) const { private: CheckerContext &Ctx; const StackFrameContext *CurSFC; - public: - SmallVector<std::pair<const MemRegion*, const MemRegion*>, 10> V; - CallBack(CheckerContext &CC) : - Ctx(CC), - CurSFC(CC.getLocationContext()->getCurrentStackFrame()) - {} + public: + SmallVector<std::pair<const MemRegion *, const MemRegion *>, 10> V; - bool HandleBinding(StoreManager &SMgr, Store store, - const MemRegion *region, SVal val) override { + CallBack(CheckerContext &CC) + : Ctx(CC), CurSFC(CC.getLocationContext()->getCurrentStackFrame()) {} - if (!isa<GlobalsSpaceRegion>(region->getMemorySpace())) - return true; + bool HandleBinding(StoreManager &SMgr, Store S, const MemRegion *Region, + SVal Val) override { - const MemRegion *vR = val.getAsRegion(); - if (!vR) + if (!isa<GlobalsSpaceRegion>(Region->getMemorySpace())) return true; - - // Under automated retain release, it is okay to assign a block - // directly to a global variable. - if (Ctx.getASTContext().getLangOpts().ObjCAutoRefCount && - isa<BlockDataRegion>(vR)) - return true; - - if (const StackSpaceRegion *SSR = - dyn_cast<StackSpaceRegion>(vR->getMemorySpace())) { - // If the global variable holds a location in the current stack frame, - // record the binding to emit a warning. - if (SSR->getStackFrame() == CurSFC) - V.push_back(std::make_pair(region, vR)); - } - + const MemRegion *VR = Val.getAsRegion(); + if (VR && isa<StackSpaceRegion>(VR->getMemorySpace()) && + !isArcManagedBlock(VR, Ctx) && !isNotInCurrentFrame(VR, Ctx)) + V.emplace_back(Region, VR); return true; } }; - CallBack cb(Ctx); - state->getStateManager().getStoreManager().iterBindings(state->getStore(),cb); + CallBack Cb(Ctx); + State->getStateManager().getStoreManager().iterBindings(State->getStore(), + Cb); - if (cb.V.empty()) + if (Cb.V.empty()) return; // Generate an error node. - ExplodedNode *N = Ctx.generateNonFatalErrorNode(state); + ExplodedNode *N = Ctx.generateNonFatalErrorNode(State); if (!N) return; if (!BT_stackleak) - BT_stackleak.reset( - new BuiltinBug(this, "Stack address stored into global variable", - "Stack address was saved into a global variable. " - "This is dangerous because the address will become " - "invalid after returning from the function")); + BT_stackleak = llvm::make_unique<BuiltinBug>( + this, "Stack address stored into global variable", + "Stack address was saved into a global variable. " + "This is dangerous because the address will become " + "invalid after returning from the function"); - for (unsigned i = 0, e = cb.V.size(); i != e; ++i) { + for (const auto &P : Cb.V) { // Generate a report for this bug. - SmallString<512> buf; - llvm::raw_svector_ostream os(buf); - SourceRange range = genName(os, cb.V[i].second, Ctx.getASTContext()); - os << " is still referred to by the "; - if (isa<StaticGlobalSpaceRegion>(cb.V[i].first->getMemorySpace())) - os << "static"; + SmallString<128> Buf; + llvm::raw_svector_ostream Out(Buf); + SourceRange Range = genName(Out, P.second, Ctx.getASTContext()); + Out << " is still referred to by the "; + if (isa<StaticGlobalSpaceRegion>(P.first->getMemorySpace())) + Out << "static"; else - os << "global"; - os << " variable '"; - const VarRegion *VR = cast<VarRegion>(cb.V[i].first->getBaseRegion()); - os << *VR->getDecl() - << "' upon returning to the caller. This will be a dangling reference"; - auto report = llvm::make_unique<BugReport>(*BT_stackleak, os.str(), N); - if (range.isValid()) - report->addRange(range); - - Ctx.emitReport(std::move(report)); + Out << "global"; + Out << " variable '"; + const VarRegion *VR = cast<VarRegion>(P.first->getBaseRegion()); + Out << *VR->getDecl() + << "' upon returning to the caller. This will be a dangling reference"; + auto Report = llvm::make_unique<BugReport>(*BT_stackleak, Out.str(), N); + if (Range.isValid()) + Report->addRange(Range); + + Ctx.emitReport(std::move(Report)); } } -void ento::registerStackAddrEscapeChecker(CheckerManager &mgr) { - mgr.registerChecker<StackAddrEscapeChecker>(); -} +#define REGISTER_CHECKER(name) \ + void ento::register##name(CheckerManager &Mgr) { \ + StackAddrEscapeChecker *Chk = \ + Mgr.registerChecker<StackAddrEscapeChecker>(); \ + Chk->ChecksEnabled[StackAddrEscapeChecker::CK_##name] = true; \ + } + +REGISTER_CHECKER(StackAddrEscapeChecker) +REGISTER_CHECKER(StackAddrAsyncEscapeChecker) diff --git a/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp index f3c2ffc58662..172ce346f1ba 100644 --- a/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp @@ -59,6 +59,11 @@ static bool isArrayIndexOutOfBounds(CheckerContext &C, const Expr *Ex) { return StOutBound && !StInBound; } +static bool isShiftOverflow(const BinaryOperator *B, CheckerContext &C) { + return C.isGreaterOrEqual( + B->getRHS(), C.getASTContext().getIntWidth(B->getLHS()->getType())); +} + void UndefResultChecker::checkPostStmt(const BinaryOperator *B, CheckerContext &C) const { ProgramStateRef state = C.getState(); @@ -97,18 +102,50 @@ void UndefResultChecker::checkPostStmt(const BinaryOperator *B, } if (Ex) { - OS << "The " << (isLeft ? "left" : "right") - << " operand of '" + OS << "The " << (isLeft ? "left" : "right") << " operand of '" << BinaryOperator::getOpcodeStr(B->getOpcode()) << "' is a garbage value"; if (isArrayIndexOutOfBounds(C, Ex)) OS << " due to array index out of bounds"; - } - else { + } else { // Neither operand was undefined, but the result is undefined. - OS << "The result of the '" - << BinaryOperator::getOpcodeStr(B->getOpcode()) - << "' expression is undefined"; + if ((B->getOpcode() == BinaryOperatorKind::BO_Shl || + B->getOpcode() == BinaryOperatorKind::BO_Shr) && + C.isNegative(B->getRHS())) { + OS << "The result of the " + << ((B->getOpcode() == BinaryOperatorKind::BO_Shl) ? "left" + : "right") + << " shift is undefined because the right operand is negative"; + } else if ((B->getOpcode() == BinaryOperatorKind::BO_Shl || + B->getOpcode() == BinaryOperatorKind::BO_Shr) && + isShiftOverflow(B, C)) { + + OS << "The result of the " + << ((B->getOpcode() == BinaryOperatorKind::BO_Shl) ? "left" + : "right") + << " shift is undefined due to shifting by "; + + SValBuilder &SB = C.getSValBuilder(); + const llvm::APSInt *I = + SB.getKnownValue(C.getState(), C.getSVal(B->getRHS())); + if (!I) + OS << "a value that is"; + else if (I->isUnsigned()) + OS << '\'' << I->getZExtValue() << "\', which is"; + else + OS << '\'' << I->getSExtValue() << "\', which is"; + + OS << " greater or equal to the width of type '" + << B->getLHS()->getType().getAsString() << "'."; + } else if (B->getOpcode() == BinaryOperatorKind::BO_Shl && + C.isNegative(B->getLHS())) { + OS << "The result of the left shift is undefined because the left " + "operand is negative"; + } else { + OS << "The result of the '" + << BinaryOperator::getOpcodeStr(B->getOpcode()) + << "' expression is undefined"; + } } auto report = llvm::make_unique<BugReport>(*BT, OS.str(), N); if (Ex) { diff --git a/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp index 7a31efc8cef8..c3dcf1fac197 100644 --- a/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp @@ -60,6 +60,14 @@ void UndefinedAssignmentChecker::checkBind(SVal location, SVal val, const Expr *ex = nullptr; while (StoreE) { + if (const UnaryOperator *U = dyn_cast<UnaryOperator>(StoreE)) { + str = "The expression is an uninitialized value. " + "The computed value will also be garbage"; + + ex = U->getSubExpr(); + break; + } + if (const BinaryOperator *B = dyn_cast<BinaryOperator>(StoreE)) { if (B->isCompoundAssignmentOp()) { ProgramStateRef state = C.getState(); diff --git a/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp b/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp index ccd8e9a18b00..6f21e868b174 100644 --- a/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp @@ -112,7 +112,7 @@ void UnreachableCodeChecker::checkEndAnalysis(ExplodedGraph &G, continue; // Check for false positives - if (CB->size() > 0 && isInvalidPath(CB, *PM)) + if (isInvalidPath(CB, *PM)) continue; // It is good practice to always have a "default" label in a "switch", even diff --git a/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp b/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp index b47762b915ce..c5010f53785a 100644 --- a/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp @@ -14,279 +14,272 @@ #include "ClangSACheckers.h" #include "clang/AST/DeclCXX.h" -#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" -#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h" -#include "llvm/ADT/SmallString.h" -#include "llvm/Support/SaveAndRestore.h" -#include "llvm/Support/raw_ostream.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h" using namespace clang; using namespace ento; namespace { - -class WalkAST : public StmtVisitor<WalkAST> { - const CheckerBase *Checker; - BugReporter &BR; - AnalysisDeclContext *AC; - - /// The root constructor or destructor whose callees are being analyzed. - const CXXMethodDecl *RootMethod = nullptr; - - /// Whether the checker should walk into bodies of called functions. - /// Controlled by the "Interprocedural" analyzer-config option. - bool IsInterprocedural = false; - - /// Whether the checker should only warn for calls to pure virtual functions - /// (which is undefined behavior) or for all virtual functions (which may - /// may result in unexpected behavior). - bool ReportPureOnly = false; - - typedef const CallExpr * WorkListUnit; - typedef SmallVector<WorkListUnit, 20> DFSWorkList; - - /// A vector representing the worklist which has a chain of CallExprs. - DFSWorkList WList; - - // PreVisited : A CallExpr to this FunctionDecl is in the worklist, but the - // body has not been visited yet. - // PostVisited : A CallExpr to this FunctionDecl is in the worklist, and the - // body has been visited. - enum Kind { NotVisited, - PreVisited, /**< A CallExpr to this FunctionDecl is in the - worklist, but the body has not yet been - visited. */ - PostVisited /**< A CallExpr to this FunctionDecl is in the - worklist, and the body has been visited. */ - }; - - /// A DenseMap that records visited states of FunctionDecls. - llvm::DenseMap<const FunctionDecl *, Kind> VisitedFunctions; - - /// The CallExpr whose body is currently being visited. This is used for - /// generating bug reports. This is null while visiting the body of a - /// constructor or destructor. - const CallExpr *visitingCallExpr; - -public: - WalkAST(const CheckerBase *checker, BugReporter &br, AnalysisDeclContext *ac, - const CXXMethodDecl *rootMethod, bool isInterprocedural, - bool reportPureOnly) - : Checker(checker), BR(br), AC(ac), RootMethod(rootMethod), - IsInterprocedural(isInterprocedural), ReportPureOnly(reportPureOnly), - visitingCallExpr(nullptr) { - // Walking should always start from either a constructor or a destructor. - assert(isa<CXXConstructorDecl>(rootMethod) || - isa<CXXDestructorDecl>(rootMethod)); - } - - bool hasWork() const { return !WList.empty(); } - - /// This method adds a CallExpr to the worklist and marks the callee as - /// being PreVisited. - void Enqueue(WorkListUnit WLUnit) { - const FunctionDecl *FD = WLUnit->getDirectCallee(); - if (!FD || !FD->getBody()) - return; - Kind &K = VisitedFunctions[FD]; - if (K != NotVisited) - return; - K = PreVisited; - WList.push_back(WLUnit); +enum class ObjectState : bool { CtorCalled, DtorCalled }; +} // end namespace + // FIXME: Ascending over StackFrameContext maybe another method. + +namespace llvm { +template <> struct FoldingSetTrait<ObjectState> { + static inline void Profile(ObjectState X, FoldingSetNodeID &ID) { + ID.AddInteger(static_cast<int>(X)); } +}; +} // end namespace llvm - /// This method returns an item from the worklist without removing it. - WorkListUnit Dequeue() { - assert(!WList.empty()); - return WList.back(); - } +namespace { +class VirtualCallChecker + : public Checker<check::BeginFunction, check::EndFunction, check::PreCall> { + mutable std::unique_ptr<BugType> BT; - void Execute() { - while (hasWork()) { - WorkListUnit WLUnit = Dequeue(); - const FunctionDecl *FD = WLUnit->getDirectCallee(); - assert(FD && FD->getBody()); - - if (VisitedFunctions[FD] == PreVisited) { - // If the callee is PreVisited, walk its body. - // Visit the body. - SaveAndRestore<const CallExpr *> SaveCall(visitingCallExpr, WLUnit); - Visit(FD->getBody()); - - // Mark the function as being PostVisited to indicate we have - // scanned the body. - VisitedFunctions[FD] = PostVisited; - continue; - } - - // Otherwise, the callee is PostVisited. - // Remove it from the worklist. - assert(VisitedFunctions[FD] == PostVisited); - WList.pop_back(); +public: + // The flag to determine if pure virtual functions should be issued only. + DefaultBool IsPureOnly; + + void checkBeginFunction(CheckerContext &C) const; + void checkEndFunction(CheckerContext &C) const; + void checkPreCall(const CallEvent &Call, CheckerContext &C) const; + +private: + void registerCtorDtorCallInState(bool IsBeginFunction, + CheckerContext &C) const; + void reportBug(StringRef Msg, bool PureError, const MemRegion *Reg, + CheckerContext &C) const; + + class VirtualBugVisitor : public BugReporterVisitorImpl<VirtualBugVisitor> { + private: + const MemRegion *ObjectRegion; + bool Found; + + public: + VirtualBugVisitor(const MemRegion *R) : ObjectRegion(R), Found(false) {} + + void Profile(llvm::FoldingSetNodeID &ID) const override { + static int X = 0; + ID.AddPointer(&X); + ID.AddPointer(ObjectRegion); } - } - - // Stmt visitor methods. - void VisitCallExpr(CallExpr *CE); - void VisitCXXMemberCallExpr(CallExpr *CE); - void VisitStmt(Stmt *S) { VisitChildren(S); } - void VisitChildren(Stmt *S); - - void ReportVirtualCall(const CallExpr *CE, bool isPure); + std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N, + const ExplodedNode *PrevN, + BugReporterContext &BRC, + BugReport &BR) override; + }; }; -} // end anonymous namespace - -//===----------------------------------------------------------------------===// -// AST walking. -//===----------------------------------------------------------------------===// - -void WalkAST::VisitChildren(Stmt *S) { - for (Stmt *Child : S->children()) - if (Child) - Visit(Child); -} - -void WalkAST::VisitCallExpr(CallExpr *CE) { - VisitChildren(CE); - if (IsInterprocedural) - Enqueue(CE); +} // end namespace + +// GDM (generic data map) to the memregion of this for the ctor and dtor. +REGISTER_MAP_WITH_PROGRAMSTATE(CtorDtorMap, const MemRegion *, ObjectState) + +std::shared_ptr<PathDiagnosticPiece> +VirtualCallChecker::VirtualBugVisitor::VisitNode(const ExplodedNode *N, + const ExplodedNode *PrevN, + BugReporterContext &BRC, + BugReport &BR) { + // We need the last ctor/dtor which call the virtual function. + // The visitor walks the ExplodedGraph backwards. + if (Found) + return nullptr; + + ProgramStateRef State = N->getState(); + const LocationContext *LCtx = N->getLocationContext(); + const CXXConstructorDecl *CD = + dyn_cast_or_null<CXXConstructorDecl>(LCtx->getDecl()); + const CXXDestructorDecl *DD = + dyn_cast_or_null<CXXDestructorDecl>(LCtx->getDecl()); + + if (!CD && !DD) + return nullptr; + + ProgramStateManager &PSM = State->getStateManager(); + auto &SVB = PSM.getSValBuilder(); + const auto *MD = dyn_cast<CXXMethodDecl>(LCtx->getDecl()); + if (!MD) + return nullptr; + auto ThiSVal = + State->getSVal(SVB.getCXXThis(MD, LCtx->getCurrentStackFrame())); + const MemRegion *Reg = ThiSVal.castAs<loc::MemRegionVal>().getRegion(); + if (!Reg) + return nullptr; + if (Reg != ObjectRegion) + return nullptr; + + const Stmt *S = PathDiagnosticLocation::getStmt(N); + if (!S) + return nullptr; + Found = true; + + std::string InfoText; + if (CD) + InfoText = "This constructor of an object of type '" + + CD->getNameAsString() + + "' has not returned when the virtual method was called"; + else + InfoText = "This destructor of an object of type '" + + DD->getNameAsString() + + "' has not returned when the virtual method was called"; + + // Generate the extra diagnostic. + PathDiagnosticLocation Pos(S, BRC.getSourceManager(), + N->getLocationContext()); + return std::make_shared<PathDiagnosticEventPiece>(Pos, InfoText, true); } -void WalkAST::VisitCXXMemberCallExpr(CallExpr *CE) { - VisitChildren(CE); - bool callIsNonVirtual = false; +// The function to check if a callexpr is a virtual function. +static bool isVirtualCall(const CallExpr *CE) { + bool CallIsNonVirtual = false; - // Several situations to elide for checking. - if (MemberExpr *CME = dyn_cast<MemberExpr>(CE->getCallee())) { - // If the member access is fully qualified (i.e., X::F), then treat - // this as a non-virtual call and do not warn. + if (const MemberExpr *CME = dyn_cast<MemberExpr>(CE->getCallee())) { + // The member access is fully qualified (i.e., X::F). + // Treat this as a non-virtual call and do not warn. if (CME->getQualifier()) - callIsNonVirtual = true; + CallIsNonVirtual = true; - if (Expr *base = CME->getBase()->IgnoreImpCasts()) { - // Elide analyzing the call entirely if the base pointer is not 'this'. - if (!isa<CXXThisExpr>(base)) - return; - - // If the most derived class is marked final, we know that now subclass - // can override this member. - if (base->getBestDynamicClassType()->hasAttr<FinalAttr>()) - callIsNonVirtual = true; + if (const Expr *Base = CME->getBase()) { + // The most derived class is marked final. + if (Base->getBestDynamicClassType()->hasAttr<FinalAttr>()) + CallIsNonVirtual = true; } } - // Get the callee. const CXXMethodDecl *MD = dyn_cast_or_null<CXXMethodDecl>(CE->getDirectCallee()); - if (MD && MD->isVirtual() && !callIsNonVirtual && !MD->hasAttr<FinalAttr>() && + if (MD && MD->isVirtual() && !CallIsNonVirtual && !MD->hasAttr<FinalAttr>() && !MD->getParent()->hasAttr<FinalAttr>()) - ReportVirtualCall(CE, MD->isPure()); + return true; + return false; +} + +// The BeginFunction callback when enter a constructor or a destructor. +void VirtualCallChecker::checkBeginFunction(CheckerContext &C) const { + registerCtorDtorCallInState(true, C); +} - if (IsInterprocedural) - Enqueue(CE); +// The EndFunction callback when leave a constructor or a destructor. +void VirtualCallChecker::checkEndFunction(CheckerContext &C) const { + registerCtorDtorCallInState(false, C); } -void WalkAST::ReportVirtualCall(const CallExpr *CE, bool isPure) { - if (ReportPureOnly && !isPure) +void VirtualCallChecker::checkPreCall(const CallEvent &Call, + CheckerContext &C) const { + const auto MC = dyn_cast<CXXMemberCall>(&Call); + if (!MC) return; - SmallString<100> buf; - llvm::raw_svector_ostream os(buf); - - // FIXME: The interprocedural diagnostic experience here is not good. - // Ultimately this checker should be re-written to be path sensitive. - // For now, only diagnose intraprocedurally, by default. - if (IsInterprocedural) { - os << "Call Path : "; - // Name of current visiting CallExpr. - os << *CE->getDirectCallee(); - - // Name of the CallExpr whose body is current being walked. - if (visitingCallExpr) - os << " <-- " << *visitingCallExpr->getDirectCallee(); - // Names of FunctionDecls in worklist with state PostVisited. - for (SmallVectorImpl<const CallExpr *>::iterator I = WList.end(), - E = WList.begin(); I != E; --I) { - const FunctionDecl *FD = (*(I-1))->getDirectCallee(); - assert(FD); - if (VisitedFunctions[FD] == PostVisited) - os << " <-- " << *FD; - } + const CXXMethodDecl *MD = dyn_cast_or_null<CXXMethodDecl>(Call.getDecl()); + if (!MD) + return; + ProgramStateRef State = C.getState(); + const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr()); - os << "\n"; + if (IsPureOnly && !MD->isPure()) + return; + if (!isVirtualCall(CE)) + return; + + const MemRegion *Reg = MC->getCXXThisVal().getAsRegion(); + const ObjectState *ObState = State->get<CtorDtorMap>(Reg); + if (!ObState) + return; + // Check if a virtual method is called. + // The GDM of constructor and destructor should be true. + if (*ObState == ObjectState::CtorCalled) { + if (IsPureOnly && MD->isPure()) + reportBug("Call to pure virtual function during construction", true, Reg, + C); + else if (!MD->isPure()) + reportBug("Call to virtual function during construction", false, Reg, C); + else + reportBug("Call to pure virtual function during construction", false, Reg, + C); } - PathDiagnosticLocation CELoc = - PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC); - SourceRange R = CE->getCallee()->getSourceRange(); + if (*ObState == ObjectState::DtorCalled) { + if (IsPureOnly && MD->isPure()) + reportBug("Call to pure virtual function during destruction", true, Reg, + C); + else if (!MD->isPure()) + reportBug("Call to virtual function during destruction", false, Reg, C); + else + reportBug("Call to pure virtual function during construction", false, Reg, + C); + } +} - os << "Call to "; - if (isPure) - os << "pure "; +void VirtualCallChecker::registerCtorDtorCallInState(bool IsBeginFunction, + CheckerContext &C) const { + const auto *LCtx = C.getLocationContext(); + const auto *MD = dyn_cast_or_null<CXXMethodDecl>(LCtx->getDecl()); + if (!MD) + return; - os << "virtual function during "; + ProgramStateRef State = C.getState(); + auto &SVB = C.getSValBuilder(); - if (isa<CXXConstructorDecl>(RootMethod)) - os << "construction "; - else - os << "destruction "; + // Enter a constructor, set the corresponding memregion be true. + if (isa<CXXConstructorDecl>(MD)) { + auto ThiSVal = + State->getSVal(SVB.getCXXThis(MD, LCtx->getCurrentStackFrame())); + const MemRegion *Reg = ThiSVal.getAsRegion(); + if (IsBeginFunction) + State = State->set<CtorDtorMap>(Reg, ObjectState::CtorCalled); + else + State = State->remove<CtorDtorMap>(Reg); - if (isPure) - os << "has undefined behavior"; - else - os << "will not dispatch to derived class"; + C.addTransition(State); + return; + } - BR.EmitBasicReport(AC->getDecl(), Checker, - "Call to virtual function during construction or " - "destruction", - "C++ Object Lifecycle", os.str(), CELoc, R); + // Enter a Destructor, set the corresponding memregion be true. + if (isa<CXXDestructorDecl>(MD)) { + auto ThiSVal = + State->getSVal(SVB.getCXXThis(MD, LCtx->getCurrentStackFrame())); + const MemRegion *Reg = ThiSVal.getAsRegion(); + if (IsBeginFunction) + State = State->set<CtorDtorMap>(Reg, ObjectState::DtorCalled); + else + State = State->remove<CtorDtorMap>(Reg); + + C.addTransition(State); + return; + } } -//===----------------------------------------------------------------------===// -// VirtualCallChecker -//===----------------------------------------------------------------------===// - -namespace { -class VirtualCallChecker : public Checker<check::ASTDecl<CXXRecordDecl> > { -public: - DefaultBool isInterprocedural; - DefaultBool isPureOnly; - - void checkASTDecl(const CXXRecordDecl *RD, AnalysisManager& mgr, - BugReporter &BR) const { - AnalysisDeclContext *ADC = mgr.getAnalysisDeclContext(RD); - - // Check the constructors. - for (const auto *I : RD->ctors()) { - if (!I->isCopyOrMoveConstructor()) - if (Stmt *Body = I->getBody()) { - WalkAST walker(this, BR, ADC, I, isInterprocedural, isPureOnly); - walker.Visit(Body); - walker.Execute(); - } - } +void VirtualCallChecker::reportBug(StringRef Msg, bool IsSink, + const MemRegion *Reg, + CheckerContext &C) const { + ExplodedNode *N; + if (IsSink) + N = C.generateErrorNode(); + else + N = C.generateNonFatalErrorNode(); - // Check the destructor. - if (CXXDestructorDecl *DD = RD->getDestructor()) - if (Stmt *Body = DD->getBody()) { - WalkAST walker(this, BR, ADC, DD, isInterprocedural, isPureOnly); - walker.Visit(Body); - walker.Execute(); - } - } -}; + if (!N) + return; + if (!BT) + BT.reset(new BugType( + this, "Call to virtual function during construction or destruction", + "C++ Object Lifecycle")); + + auto Reporter = llvm::make_unique<BugReport>(*BT, Msg, N); + Reporter->addVisitor(llvm::make_unique<VirtualBugVisitor>(Reg)); + C.emitReport(std::move(Reporter)); } void ento::registerVirtualCallChecker(CheckerManager &mgr) { VirtualCallChecker *checker = mgr.registerChecker<VirtualCallChecker>(); - checker->isInterprocedural = - mgr.getAnalyzerOptions().getBooleanOption("Interprocedural", false, - checker); - checker->isPureOnly = - mgr.getAnalyzerOptions().getBooleanOption("PureOnly", false, - checker); + checker->IsPureOnly = + mgr.getAnalyzerOptions().getBooleanOption("PureOnly", false, checker); } |