diff options
Diffstat (limited to 'lib/StaticAnalyzer/Checkers')
24 files changed, 1074 insertions, 260 deletions
diff --git a/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp b/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp index e6592a285e47..90d5c0e36a47 100644 --- a/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp @@ -24,16 +24,29 @@ using namespace ento; namespace { -class AnalysisOrderChecker : public Checker< check::PreStmt<CastExpr>, - check::PostStmt<CastExpr>, - check::PreStmt<ArraySubscriptExpr>, - check::PostStmt<ArraySubscriptExpr>> { - bool isCallbackEnabled(CheckerContext &C, StringRef CallbackName) const { - AnalyzerOptions &Opts = C.getAnalysisManager().getAnalyzerOptions(); +class AnalysisOrderChecker + : public Checker<check::PreStmt<CastExpr>, + check::PostStmt<CastExpr>, + check::PreStmt<ArraySubscriptExpr>, + check::PostStmt<ArraySubscriptExpr>, + check::Bind, + check::RegionChanges> { + bool isCallbackEnabled(AnalyzerOptions &Opts, StringRef CallbackName) const { return Opts.getBooleanOption("*", false, this) || Opts.getBooleanOption(CallbackName, false, this); } + bool isCallbackEnabled(CheckerContext &C, StringRef CallbackName) const { + AnalyzerOptions &Opts = C.getAnalysisManager().getAnalyzerOptions(); + return isCallbackEnabled(Opts, CallbackName); + } + + bool isCallbackEnabled(ProgramStateRef State, StringRef CallbackName) const { + AnalyzerOptions &Opts = State->getStateManager().getOwningEngine() + ->getAnalysisManager().getAnalyzerOptions(); + return isCallbackEnabled(Opts, CallbackName); + } + public: void checkPreStmt(const CastExpr *CE, CheckerContext &C) const { if (isCallbackEnabled(C, "PreStmtCastExpr")) @@ -47,17 +60,35 @@ public: << ")\n"; } - void checkPreStmt(const ArraySubscriptExpr *SubExpr, CheckerContext &C) const { + void checkPreStmt(const ArraySubscriptExpr *SubExpr, + CheckerContext &C) const { if (isCallbackEnabled(C, "PreStmtArraySubscriptExpr")) llvm::errs() << "PreStmt<ArraySubscriptExpr>\n"; } - void checkPostStmt(const ArraySubscriptExpr *SubExpr, CheckerContext &C) const { + void checkPostStmt(const ArraySubscriptExpr *SubExpr, + CheckerContext &C) const { if (isCallbackEnabled(C, "PostStmtArraySubscriptExpr")) llvm::errs() << "PostStmt<ArraySubscriptExpr>\n"; } + + void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &C) const { + if (isCallbackEnabled(C, "Bind")) + llvm::errs() << "Bind\n"; + } + + ProgramStateRef + checkRegionChanges(ProgramStateRef State, + const InvalidatedSymbols *Invalidated, + ArrayRef<const MemRegion *> ExplicitRegions, + ArrayRef<const MemRegion *> Regions, + const LocationContext *LCtx, const CallEvent *Call) const { + if (isCallbackEnabled(State, "RegionChanges")) + llvm::errs() << "RegionChanges\n"; + return State; + } }; -} +} // end anonymous namespace //===----------------------------------------------------------------------===// // Registration. diff --git a/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp index 082a4873217b..d19630eeef77 100644 --- a/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp @@ -29,7 +29,9 @@ namespace { class BlockInCriticalSectionChecker : public Checker<check::PostCall, check::PreCall> { - CallDescription LockFn, UnlockFn, SleepFn, GetcFn, FgetsFn, ReadFn, RecvFn; + CallDescription LockFn, UnlockFn, SleepFn, GetcFn, FgetsFn, ReadFn, RecvFn, + PthreadLockFn, PthreadTryLockFn, PthreadUnlockFn, + MtxLock, MtxTimedLock, MtxTryLock, MtxUnlock; std::unique_ptr<BugType> BlockInCritSectionBugType; @@ -40,6 +42,10 @@ class BlockInCriticalSectionChecker : public Checker<check::PostCall, public: BlockInCriticalSectionChecker(); + bool isBlockingFunction(const CallEvent &Call) const; + bool isLockFunction(const CallEvent &Call) const; + bool isUnlockFunction(const CallEvent &Call) const; + void checkPreCall(const CallEvent &Call, CheckerContext &C) const; /// Process unlock. @@ -55,34 +61,69 @@ REGISTER_TRAIT_WITH_PROGRAMSTATE(MutexCounter, unsigned) BlockInCriticalSectionChecker::BlockInCriticalSectionChecker() : LockFn("lock"), UnlockFn("unlock"), SleepFn("sleep"), GetcFn("getc"), - FgetsFn("fgets"), ReadFn("read"), RecvFn("recv") { + FgetsFn("fgets"), ReadFn("read"), RecvFn("recv"), + PthreadLockFn("pthread_mutex_lock"), + PthreadTryLockFn("pthread_mutex_trylock"), + PthreadUnlockFn("pthread_mutex_unlock"), + MtxLock("mtx_lock"), + MtxTimedLock("mtx_timedlock"), + MtxTryLock("mtx_trylock"), + MtxUnlock("mtx_unlock") { // Initialize the bug type. BlockInCritSectionBugType.reset( new BugType(this, "Call to blocking function in critical section", "Blocking Error")); } +bool BlockInCriticalSectionChecker::isBlockingFunction(const CallEvent &Call) const { + if (Call.isCalled(SleepFn) + || Call.isCalled(GetcFn) + || Call.isCalled(FgetsFn) + || Call.isCalled(ReadFn) + || Call.isCalled(RecvFn)) { + return true; + } + return false; +} + +bool BlockInCriticalSectionChecker::isLockFunction(const CallEvent &Call) const { + if (Call.isCalled(LockFn) + || Call.isCalled(PthreadLockFn) + || Call.isCalled(PthreadTryLockFn) + || Call.isCalled(MtxLock) + || Call.isCalled(MtxTimedLock) + || Call.isCalled(MtxTryLock)) { + return true; + } + return false; +} + +bool BlockInCriticalSectionChecker::isUnlockFunction(const CallEvent &Call) const { + if (Call.isCalled(UnlockFn) + || Call.isCalled(PthreadUnlockFn) + || Call.isCalled(MtxUnlock)) { + return true; + } + return false; +} + void BlockInCriticalSectionChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { } void BlockInCriticalSectionChecker::checkPostCall(const CallEvent &Call, CheckerContext &C) const { - if (!Call.isCalled(LockFn) - && !Call.isCalled(SleepFn) - && !Call.isCalled(GetcFn) - && !Call.isCalled(FgetsFn) - && !Call.isCalled(ReadFn) - && !Call.isCalled(RecvFn) - && !Call.isCalled(UnlockFn)) + if (!isBlockingFunction(Call) + && !isLockFunction(Call) + && !isUnlockFunction(Call)) return; ProgramStateRef State = C.getState(); unsigned mutexCount = State->get<MutexCounter>(); - if (Call.isCalled(UnlockFn) && mutexCount > 0) { + if (isUnlockFunction(Call) && mutexCount > 0) { State = State->set<MutexCounter>(--mutexCount); C.addTransition(State); - } else if (Call.isCalled(LockFn)) { + } else if (isLockFunction(Call)) { State = State->set<MutexCounter>(++mutexCount); C.addTransition(State); } else if (mutexCount > 0) { @@ -97,8 +138,11 @@ void BlockInCriticalSectionChecker::reportBlockInCritSection( if (!ErrNode) return; - auto R = llvm::make_unique<BugReport>(*BlockInCritSectionBugType, - "A blocking function %s is called inside a critical section.", ErrNode); + std::string msg; + llvm::raw_string_ostream os(msg); + os << "Call to blocking function '" << Call.getCalleeIdentifier()->getName() + << "' inside of critical section"; + auto R = llvm::make_unique<BugReport>(*BlockInCritSectionBugType, os.str(), ErrNode); R->addRange(Call.getSourceRange()); R->markInteresting(BlockDescSym); C.emitReport(std::move(R)); diff --git a/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 05505ec38600..60d60bc074eb 100644 --- a/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -48,6 +48,7 @@ add_clang_library(clangStaticAnalyzerCheckers MallocChecker.cpp MallocOverflowSecurityChecker.cpp MallocSizeofChecker.cpp + MisusedMovedObjectChecker.cpp MPI-Checker/MPIBugReporter.cpp MPI-Checker/MPIChecker.cpp MPI-Checker/MPIFunctionClassifier.cpp diff --git a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 238032c895f6..32e3ce9270aa 100644 --- a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -68,6 +68,7 @@ public: const InvalidatedSymbols *, ArrayRef<const MemRegion *> ExplicitRegions, ArrayRef<const MemRegion *> Regions, + const LocationContext *LCtx, const CallEvent *Call) const; typedef void (CStringChecker::*FnCheck)(CheckerContext &, @@ -1943,8 +1944,12 @@ void CStringChecker::evalStrsep(CheckerContext &C, const CallExpr *CE) const { // Overwrite the search string pointer. The new value is either an address // further along in the same string, or NULL if there are no more tokens. State = State->bindLoc(*SearchStrLoc, - SVB.conjureSymbolVal(getTag(), CE, LCtx, CharPtrTy, - C.blockCount())); + SVB.conjureSymbolVal(getTag(), + CE, + LCtx, + CharPtrTy, + C.blockCount()), + LCtx); } else { assert(SearchStrVal.isUnknown()); // Conjure a symbolic value. It's the best we can do. @@ -2116,6 +2121,7 @@ CStringChecker::checkRegionChanges(ProgramStateRef state, const InvalidatedSymbols *, ArrayRef<const MemRegion *> ExplicitRegions, ArrayRef<const MemRegion *> Regions, + const LocationContext *LCtx, const CallEvent *Call) const { CStringLengthTy Entries = state->get<CStringLength>(); if (Entries.isEmpty()) diff --git a/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp index 3db19946a300..391b843ff3db 100644 --- a/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp @@ -36,25 +36,24 @@ class WalkAST: public StmtVisitor<WalkAST> { AnalysisDeclContext* AC; /// Check if two expressions refer to the same declaration. - inline bool sameDecl(const Expr *A1, const Expr *A2) { - if (const DeclRefExpr *D1 = dyn_cast<DeclRefExpr>(A1->IgnoreParenCasts())) - if (const DeclRefExpr *D2 = dyn_cast<DeclRefExpr>(A2->IgnoreParenCasts())) + bool sameDecl(const Expr *A1, const Expr *A2) { + if (const auto *D1 = dyn_cast<DeclRefExpr>(A1->IgnoreParenCasts())) + if (const auto *D2 = dyn_cast<DeclRefExpr>(A2->IgnoreParenCasts())) return D1->getDecl() == D2->getDecl(); return false; } /// Check if the expression E is a sizeof(WithArg). - inline bool isSizeof(const Expr *E, const Expr *WithArg) { - if (const UnaryExprOrTypeTraitExpr *UE = - dyn_cast<UnaryExprOrTypeTraitExpr>(E)) - if (UE->getKind() == UETT_SizeOf) + bool isSizeof(const Expr *E, const Expr *WithArg) { + if (const auto *UE = dyn_cast<UnaryExprOrTypeTraitExpr>(E)) + if (UE->getKind() == UETT_SizeOf && !UE->isArgumentType()) return sameDecl(UE->getArgumentExpr(), WithArg); return false; } /// Check if the expression E is a strlen(WithArg). - inline bool isStrlen(const Expr *E, const Expr *WithArg) { - if (const CallExpr *CE = dyn_cast<CallExpr>(E)) { + bool isStrlen(const Expr *E, const Expr *WithArg) { + if (const auto *CE = dyn_cast<CallExpr>(E)) { const FunctionDecl *FD = CE->getDirectCallee(); if (!FD) return false; @@ -65,14 +64,14 @@ class WalkAST: public StmtVisitor<WalkAST> { } /// Check if the expression is an integer literal with value 1. - inline bool isOne(const Expr *E) { - if (const IntegerLiteral *IL = dyn_cast<IntegerLiteral>(E)) + bool isOne(const Expr *E) { + if (const auto *IL = dyn_cast<IntegerLiteral>(E)) return (IL->getValue().isIntN(1)); return false; } - inline StringRef getPrintableName(const Expr *E) { - if (const DeclRefExpr *D = dyn_cast<DeclRefExpr>(E->IgnoreParenCasts())) + StringRef getPrintableName(const Expr *E) { + if (const auto *D = dyn_cast<DeclRefExpr>(E->IgnoreParenCasts())) return D->getDecl()->getName(); return StringRef(); } @@ -82,8 +81,8 @@ class WalkAST: public StmtVisitor<WalkAST> { bool containsBadStrncatPattern(const CallExpr *CE); public: - WalkAST(const CheckerBase *checker, BugReporter &br, AnalysisDeclContext *ac) - : Checker(checker), BR(br), AC(ac) {} + WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC) + : Checker(Checker), BR(BR), AC(AC) {} // Statement visitor methods. void VisitChildren(Stmt *S); @@ -108,8 +107,7 @@ bool WalkAST::containsBadStrncatPattern(const CallExpr *CE) { const Expr *LenArg = CE->getArg(2); // Identify wrong size expressions, which are commonly used instead. - if (const BinaryOperator *BE = - dyn_cast<BinaryOperator>(LenArg->IgnoreParenCasts())) { + if (const auto *BE = dyn_cast<BinaryOperator>(LenArg->IgnoreParenCasts())) { // - sizeof(dst) - strlen(dst) if (BE->getOpcode() == BO_Sub) { const Expr *L = BE->getLHS(); diff --git a/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp b/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp index 7631322d255b..668e772fe1b3 100644 --- a/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp @@ -51,9 +51,9 @@ void CXXSelfAssignmentChecker::checkBeginFunction(CheckerContext &C) const { State->getSVal(SVB.getCXXThis(MD, LCtx->getCurrentStackFrame())); auto Param = SVB.makeLoc(State->getRegion(MD->getParamDecl(0), LCtx)); auto ParamVal = State->getSVal(Param); - ProgramStateRef SelfAssignState = State->bindLoc(Param, ThisVal); + ProgramStateRef SelfAssignState = State->bindLoc(Param, ThisVal, LCtx); C.addTransition(SelfAssignState); - ProgramStateRef NonSelfAssignState = State->bindLoc(Param, ParamVal); + ProgramStateRef NonSelfAssignState = State->bindLoc(Param, ParamVal, LCtx); C.addTransition(NonSelfAssignState); } diff --git a/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp b/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp index f474857a1bf4..07285d27ed9e 100644 --- a/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp @@ -21,6 +21,7 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" #include "llvm/ADT/SmallString.h" +#include "llvm/ADT/StringExtras.h" #include "llvm/Support/raw_ostream.h" using namespace clang; @@ -71,7 +72,7 @@ public: private: bool PreVisitProcessArg(CheckerContext &C, SVal V, SourceRange ArgRange, - const Expr *ArgEx, bool IsFirstArgument, + const Expr *ArgEx, int ArgumentNumber, bool CheckUninitFields, const CallEvent &Call, std::unique_ptr<BugType> &BT, const ParmVarDecl *ParamDecl) const; @@ -89,9 +90,10 @@ private: BT.reset(new BuiltinBug(this, desc)); } bool uninitRefOrPointer(CheckerContext &C, const SVal &V, - SourceRange ArgRange, - const Expr *ArgEx, std::unique_ptr<BugType> &BT, - const ParmVarDecl *ParamDecl, const char *BD) const; + SourceRange ArgRange, const Expr *ArgEx, + std::unique_ptr<BugType> &BT, + const ParmVarDecl *ParamDecl, const char *BD, + int ArgumentNumber) const; }; } // end anonymous namespace @@ -111,38 +113,45 @@ void CallAndMessageChecker::emitBadCall(BugType *BT, CheckerContext &C, C.emitReport(std::move(R)); } -static StringRef describeUninitializedArgumentInCall(const CallEvent &Call, - bool IsFirstArgument) { +static void describeUninitializedArgumentInCall(const CallEvent &Call, + int ArgumentNumber, + llvm::raw_svector_ostream &Os) { switch (Call.getKind()) { case CE_ObjCMessage: { const ObjCMethodCall &Msg = cast<ObjCMethodCall>(Call); switch (Msg.getMessageKind()) { case OCM_Message: - return "Argument in message expression is an uninitialized value"; + Os << (ArgumentNumber + 1) << llvm::getOrdinalSuffix(ArgumentNumber + 1) + << " argument in message expression is an uninitialized value"; + return; case OCM_PropertyAccess: assert(Msg.isSetter() && "Getters have no args"); - return "Argument for property setter is an uninitialized value"; + Os << "Argument for property setter is an uninitialized value"; + return; case OCM_Subscript: - if (Msg.isSetter() && IsFirstArgument) - return "Argument for subscript setter is an uninitialized value"; - return "Subscript index is an uninitialized value"; + if (Msg.isSetter() && (ArgumentNumber == 0)) + Os << "Argument for subscript setter is an uninitialized value"; + else + Os << "Subscript index is an uninitialized value"; + return; } llvm_unreachable("Unknown message kind."); } case CE_Block: - return "Block call argument is an uninitialized value"; + Os << (ArgumentNumber + 1) << llvm::getOrdinalSuffix(ArgumentNumber + 1) + << " block call argument is an uninitialized value"; + return; default: - return "Function call argument is an uninitialized value"; + Os << (ArgumentNumber + 1) << llvm::getOrdinalSuffix(ArgumentNumber + 1) + << " function call argument is an uninitialized value"; + return; } } -bool CallAndMessageChecker::uninitRefOrPointer(CheckerContext &C, - const SVal &V, - SourceRange ArgRange, - const Expr *ArgEx, - std::unique_ptr<BugType> &BT, - const ParmVarDecl *ParamDecl, - const char *BD) const { +bool CallAndMessageChecker::uninitRefOrPointer( + CheckerContext &C, const SVal &V, SourceRange ArgRange, const Expr *ArgEx, + std::unique_ptr<BugType> &BT, const ParmVarDecl *ParamDecl, const char *BD, + int ArgumentNumber) const { if (!Filter.Check_CallAndMessageUnInitRefArg) return false; @@ -153,12 +162,15 @@ bool CallAndMessageChecker::uninitRefOrPointer(CheckerContext &C, // If parameter is declared as pointer to const in function declaration, // then check if corresponding argument in function call is // pointing to undefined symbol value (uninitialized memory). - StringRef Message; + SmallString<200> Buf; + llvm::raw_svector_ostream Os(Buf); if (ParamDecl->getType()->isPointerType()) { - Message = "Function call argument is a pointer to uninitialized value"; + Os << (ArgumentNumber + 1) << llvm::getOrdinalSuffix(ArgumentNumber + 1) + << " function call argument is a pointer to uninitialized value"; } else if (ParamDecl->getType()->isReferenceType()) { - Message = "Function call argument is an uninitialized value"; + Os << (ArgumentNumber + 1) << llvm::getOrdinalSuffix(ArgumentNumber + 1) + << " function call argument is an uninitialized value"; } else return false; @@ -171,7 +183,7 @@ bool CallAndMessageChecker::uninitRefOrPointer(CheckerContext &C, if (PSV.isUndef()) { if (ExplodedNode *N = C.generateErrorNode()) { LazyInit_BT(BD, BT); - auto R = llvm::make_unique<BugReport>(*BT, Message, N); + auto R = llvm::make_unique<BugReport>(*BT, Os.str(), N); R->addRange(ArgRange); if (ArgEx) { bugreporter::trackNullOrUndefValue(N, ArgEx, *R); @@ -188,7 +200,7 @@ bool CallAndMessageChecker::PreVisitProcessArg(CheckerContext &C, SVal V, SourceRange ArgRange, const Expr *ArgEx, - bool IsFirstArgument, + int ArgumentNumber, bool CheckUninitFields, const CallEvent &Call, std::unique_ptr<BugType> &BT, @@ -196,17 +208,19 @@ bool CallAndMessageChecker::PreVisitProcessArg(CheckerContext &C, ) const { const char *BD = "Uninitialized argument value"; - if (uninitRefOrPointer(C, V, ArgRange, ArgEx, BT, ParamDecl, BD)) + if (uninitRefOrPointer(C, V, ArgRange, ArgEx, BT, ParamDecl, BD, + ArgumentNumber)) return true; if (V.isUndef()) { if (ExplodedNode *N = C.generateErrorNode()) { LazyInit_BT(BD, BT); - // Generate a report for this bug. - StringRef Desc = - describeUninitializedArgumentInCall(Call, IsFirstArgument); - auto R = llvm::make_unique<BugReport>(*BT, Desc, N); + SmallString<200> Buf; + llvm::raw_svector_ostream Os(Buf); + describeUninitializedArgumentInCall(Call, ArgumentNumber, Os); + auto R = llvm::make_unique<BugReport>(*BT, Os.str(), N); + R->addRange(ArgRange); if (ArgEx) bugreporter::trackNullOrUndefValue(N, ArgEx, *R); @@ -435,7 +449,7 @@ void CallAndMessageChecker::checkPreCall(const CallEvent &Call, if(FD && i < FD->getNumParams()) ParamDecl = FD->getParamDecl(i); if (PreVisitProcessArg(C, Call.getArgSVal(i), Call.getArgSourceRange(i), - Call.getArgExpr(i), /*IsFirstArgument=*/i == 0, + Call.getArgExpr(i), i, checkUninitFields, Call, *BT, ParamDecl)) return; } diff --git a/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp b/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp index 16a475ae9dd2..65e81315f095 100644 --- a/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp @@ -84,6 +84,10 @@ bool CastToStructVisitor::VisitCastExpr(const CastExpr *CE) { if (!VD || VD->getType()->isReferenceType()) return true; + if (ToPointeeTy->isIncompleteType() || + OrigPointeeTy->isIncompleteType()) + return true; + // Warn when there is widening cast. unsigned ToWidth = Ctx.getTypeInfo(ToPointeeTy).Width; unsigned OrigWidth = Ctx.getTypeInfo(OrigPointeeTy).Width; diff --git a/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp b/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp index 86764c939dcd..95b6c4d3775d 100644 --- a/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp +++ b/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp @@ -231,14 +231,6 @@ public: /// check::LiveSymbols void checkLiveSymbols(ProgramStateRef State, SymbolReaper &SR) const {} - /// \brief Called to determine if the checker currently needs to know if when - /// contents of any regions change. - /// - /// Since it is not necessarily cheap to compute which regions are being - /// changed, this allows the analyzer core to skip the more expensive - /// #checkRegionChanges when no checkers are tracking any state. - bool wantsRegionChangeUpdate(ProgramStateRef St) const { return true; } - /// \brief Called when the contents of one or more regions change. /// /// This can occur in many different ways: an explicit bind, a blanket @@ -255,18 +247,18 @@ public: /// by this change. For a simple bind, this list will be the same as /// \p ExplicitRegions, since a bind does not affect the contents of /// anything accessible through the base region. + /// \param LCtx LocationContext that is useful for getting various contextual + /// info, like callstack, CFG etc. /// \param Call The opaque call triggering this invalidation. Will be 0 if the /// change was not triggered by a call. /// - /// Note that this callback will not be invoked unless - /// #wantsRegionChangeUpdate returns \c true. - /// /// check::RegionChanges ProgramStateRef checkRegionChanges(ProgramStateRef State, const InvalidatedSymbols *Invalidated, ArrayRef<const MemRegion *> ExplicitRegions, ArrayRef<const MemRegion *> Regions, + const LocationContext *LCtx, const CallEvent *Call) const { return State; } diff --git a/lib/StaticAnalyzer/Checkers/CloneChecker.cpp b/lib/StaticAnalyzer/Checkers/CloneChecker.cpp index 6fa5732d10cb..1885b0e39203 100644 --- a/lib/StaticAnalyzer/Checkers/CloneChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CloneChecker.cpp @@ -38,14 +38,15 @@ public: void checkEndOfTranslationUnit(const TranslationUnitDecl *TU, AnalysisManager &Mgr, BugReporter &BR) const; - /// \brief Reports all clones to the user. + /// Reports all clones to the user. void reportClones(BugReporter &BR, AnalysisManager &Mgr, - int MinComplexity) const; + std::vector<CloneDetector::CloneGroup> &CloneGroups) const; - /// \brief Reports only suspicious clones to the user along with informaton - /// that explain why they are suspicious. - void reportSuspiciousClones(BugReporter &BR, AnalysisManager &Mgr, - int MinComplexity) const; + /// Reports only suspicious clones to the user along with informaton + /// that explain why they are suspicious. + void reportSuspiciousClones( + BugReporter &BR, AnalysisManager &Mgr, + std::vector<CloneDetector::CloneGroup> &CloneGroups) const; }; } // end anonymous namespace @@ -72,11 +73,30 @@ void CloneChecker::checkEndOfTranslationUnit(const TranslationUnitDecl *TU, bool ReportNormalClones = Mgr.getAnalyzerOptions().getBooleanOption( "ReportNormalClones", true, this); + // Let the CloneDetector create a list of clones from all the analyzed + // statements. We don't filter for matching variable patterns at this point + // because reportSuspiciousClones() wants to search them for errors. + std::vector<CloneDetector::CloneGroup> AllCloneGroups; + + Detector.findClones(AllCloneGroups, RecursiveCloneTypeIIConstraint(), + MinComplexityConstraint(MinComplexity), + MinGroupSizeConstraint(2), OnlyLargestCloneConstraint()); + if (ReportSuspiciousClones) - reportSuspiciousClones(BR, Mgr, MinComplexity); + reportSuspiciousClones(BR, Mgr, AllCloneGroups); + + // We are done for this translation unit unless we also need to report normal + // clones. + if (!ReportNormalClones) + return; - if (ReportNormalClones) - reportClones(BR, Mgr, MinComplexity); + // Now that the suspicious clone detector has checked for pattern errors, + // we also filter all clones who don't have matching patterns + CloneDetector::constrainClones(AllCloneGroups, + MatchingVariablePatternConstraint(), + MinGroupSizeConstraint(2)); + + reportClones(BR, Mgr, AllCloneGroups); } static PathDiagnosticLocation makeLocation(const StmtSequence &S, @@ -87,37 +107,55 @@ static PathDiagnosticLocation makeLocation(const StmtSequence &S, Mgr.getAnalysisDeclContext(ACtx.getTranslationUnitDecl())); } -void CloneChecker::reportClones(BugReporter &BR, AnalysisManager &Mgr, - int MinComplexity) const { - - std::vector<CloneDetector::CloneGroup> CloneGroups; - Detector.findClones(CloneGroups, MinComplexity); +void CloneChecker::reportClones( + BugReporter &BR, AnalysisManager &Mgr, + std::vector<CloneDetector::CloneGroup> &CloneGroups) const { if (!BT_Exact) BT_Exact.reset(new BugType(this, "Exact code clone", "Code clone")); - for (CloneDetector::CloneGroup &Group : CloneGroups) { + for (const CloneDetector::CloneGroup &Group : CloneGroups) { // We group the clones by printing the first as a warning and all others // as a note. - auto R = llvm::make_unique<BugReport>( - *BT_Exact, "Duplicate code detected", - makeLocation(Group.Sequences.front(), Mgr)); - R->addRange(Group.Sequences.front().getSourceRange()); - - for (unsigned i = 1; i < Group.Sequences.size(); ++i) - R->addNote("Similar code here", - makeLocation(Group.Sequences[i], Mgr), - Group.Sequences[i].getSourceRange()); + auto R = llvm::make_unique<BugReport>(*BT_Exact, "Duplicate code detected", + makeLocation(Group.front(), Mgr)); + R->addRange(Group.front().getSourceRange()); + + for (unsigned i = 1; i < Group.size(); ++i) + R->addNote("Similar code here", makeLocation(Group[i], Mgr), + Group[i].getSourceRange()); BR.emitReport(std::move(R)); } } -void CloneChecker::reportSuspiciousClones(BugReporter &BR, - AnalysisManager &Mgr, - int MinComplexity) const { - - std::vector<CloneDetector::SuspiciousClonePair> Clones; - Detector.findSuspiciousClones(Clones, MinComplexity); +void CloneChecker::reportSuspiciousClones( + BugReporter &BR, AnalysisManager &Mgr, + std::vector<CloneDetector::CloneGroup> &CloneGroups) const { + std::vector<VariablePattern::SuspiciousClonePair> Pairs; + + for (const CloneDetector::CloneGroup &Group : CloneGroups) { + for (unsigned i = 0; i < Group.size(); ++i) { + VariablePattern PatternA(Group[i]); + + for (unsigned j = i + 1; j < Group.size(); ++j) { + VariablePattern PatternB(Group[j]); + + VariablePattern::SuspiciousClonePair ClonePair; + // For now, we only report clones which break the variable pattern just + // once because multiple differences in a pattern are an indicator that + // those differences are maybe intended (e.g. because it's actually a + // different algorithm). + // FIXME: In very big clones even multiple variables can be unintended, + // so replacing this number with a percentage could better handle such + // cases. On the other hand it could increase the false-positive rate + // for all clones if the percentage is too high. + if (PatternA.countPatternDifferences(PatternB, &ClonePair) == 1) { + Pairs.push_back(ClonePair); + break; + } + } + } + } if (!BT_Suspicious) BT_Suspicious.reset( @@ -128,7 +166,7 @@ void CloneChecker::reportSuspiciousClones(BugReporter &BR, AnalysisDeclContext *ADC = Mgr.getAnalysisDeclContext(ACtx.getTranslationUnitDecl()); - for (CloneDetector::SuspiciousClonePair &Pair : Clones) { + for (VariablePattern::SuspiciousClonePair &Pair : Pairs) { // FIXME: We are ignoring the suggestions currently, because they are // only 50% accurate (even if the second suggestion is unavailable), // which may confuse the user. diff --git a/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp b/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp index 2bb9e858731c..ea894c81011c 100644 --- a/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp @@ -41,7 +41,8 @@ private: mutable std::unique_ptr<BuiltinBug> BT; // Is there loss of precision - bool isLossOfPrecision(const ImplicitCastExpr *Cast, CheckerContext &C) const; + bool isLossOfPrecision(const ImplicitCastExpr *Cast, QualType DestType, + CheckerContext &C) const; // Is there loss of sign bool isLossOfSign(const ImplicitCastExpr *Cast, CheckerContext &C) const; @@ -73,16 +74,30 @@ void ConversionChecker::checkPreStmt(const ImplicitCastExpr *Cast, // Loss of sign/precision in binary operation. if (const auto *B = dyn_cast<BinaryOperator>(Parent)) { BinaryOperator::Opcode Opc = B->getOpcode(); - if (Opc == BO_Assign || Opc == BO_AddAssign || Opc == BO_SubAssign || - Opc == BO_MulAssign) { + if (Opc == BO_Assign) { LossOfSign = isLossOfSign(Cast, C); - LossOfPrecision = isLossOfPrecision(Cast, C); + LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C); + } else if (Opc == BO_AddAssign || Opc == BO_SubAssign) { + // No loss of sign. + LossOfPrecision = isLossOfPrecision(Cast, B->getLHS()->getType(), C); + } else if (Opc == BO_MulAssign) { + LossOfSign = isLossOfSign(Cast, C); + LossOfPrecision = isLossOfPrecision(Cast, B->getLHS()->getType(), C); + } else if (Opc == BO_DivAssign || Opc == BO_RemAssign) { + LossOfSign = isLossOfSign(Cast, C); + // No loss of precision. + } else if (Opc == BO_AndAssign) { + LossOfSign = isLossOfSign(Cast, C); + // No loss of precision. + } else if (Opc == BO_OrAssign || Opc == BO_XorAssign) { + LossOfSign = isLossOfSign(Cast, C); + LossOfPrecision = isLossOfPrecision(Cast, B->getLHS()->getType(), C); } else if (B->isRelationalOp() || B->isMultiplicativeOp()) { LossOfSign = isLossOfSign(Cast, C); } } else if (isa<DeclStmt>(Parent)) { LossOfSign = isLossOfSign(Cast, C); - LossOfPrecision = isLossOfPrecision(Cast, C); + LossOfPrecision = isLossOfPrecision(Cast, Cast->getType(), C); } if (LossOfSign || LossOfPrecision) { @@ -113,6 +128,13 @@ 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; @@ -153,22 +175,22 @@ static bool isNegative(CheckerContext &C, const Expr *E) { } bool ConversionChecker::isLossOfPrecision(const ImplicitCastExpr *Cast, - CheckerContext &C) const { + QualType DestType, + CheckerContext &C) const { // Don't warn about explicit loss of precision. if (Cast->isEvaluatable(C.getASTContext())) return false; - QualType CastType = Cast->getType(); QualType SubType = Cast->IgnoreParenImpCasts()->getType(); - if (!CastType->isIntegerType() || !SubType->isIntegerType()) + if (!DestType->isIntegerType() || !SubType->isIntegerType()) return false; - if (C.getASTContext().getIntWidth(CastType) >= + if (C.getASTContext().getIntWidth(DestType) >= C.getASTContext().getIntWidth(SubType)) return false; - unsigned W = C.getASTContext().getIntWidth(CastType); + unsigned W = C.getASTContext().getIntWidth(DestType); if (W == 1 || W >= 64U) return false; diff --git a/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp b/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp index 2d5cb60edf7d..32040e71163d 100644 --- a/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp @@ -13,6 +13,7 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" #include "clang/StaticAnalyzer/Checkers/SValExplainer.h" #include "llvm/ADT/StringSwitch.h" +#include "llvm/Support/ScopedPrinter.h" using namespace clang; using namespace ento; @@ -71,8 +72,8 @@ bool ExprInspectionChecker::evalCall(const CallExpr *CE, &ExprInspectionChecker::analyzerWarnIfReached) .Case("clang_analyzer_warnOnDeadSymbol", &ExprInspectionChecker::analyzerWarnOnDeadSymbol) - .Case("clang_analyzer_explain", &ExprInspectionChecker::analyzerExplain) - .Case("clang_analyzer_dump", &ExprInspectionChecker::analyzerDump) + .StartsWith("clang_analyzer_explain", &ExprInspectionChecker::analyzerExplain) + .StartsWith("clang_analyzer_dump", &ExprInspectionChecker::analyzerDump) .Case("clang_analyzer_getExtent", &ExprInspectionChecker::analyzerGetExtent) .Case("clang_analyzer_printState", &ExprInspectionChecker::analyzerPrintState) @@ -269,7 +270,7 @@ void ExprInspectionChecker::checkEndAnalysis(ExplodedGraph &G, BugReporter &BR, unsigned NumTimesReached = Item.second.NumTimesReached; ExplodedNode *N = Item.second.ExampleNode; - reportBug(std::to_string(NumTimesReached), BR, N); + reportBug(llvm::to_string(NumTimesReached), BR, N); } } diff --git a/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp b/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp index 8c8acc637f1f..b1a54e77951b 100644 --- a/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp @@ -65,6 +65,18 @@ private: /// and thus, is tainted. static bool isStdin(const Expr *E, CheckerContext &C); + /// This is called from getPointedToSymbol() to resolve symbol references for + /// the region underlying a LazyCompoundVal. This is the default binding + /// for the LCV, which could be a conjured symbol from a function call that + /// initialized the region. It only returns the conjured symbol if the LCV + /// covers the entire region, e.g. we avoid false positives by not returning + /// a default bindingc for an entire struct if the symbol for only a single + /// field or element within it is requested. + // TODO: Return an appropriate symbol for sub-fields/elements of an LCV so + // that they are also appropriately tainted. + static SymbolRef getLCVSymbol(CheckerContext &C, + nonloc::LazyCompoundVal &LCV); + /// \brief Given a pointer argument, get the symbol of the value it contains /// (points to). static SymbolRef getPointedToSymbol(CheckerContext &C, const Expr *Arg); @@ -101,6 +113,22 @@ private: bool generateReportIfTainted(const Expr *E, const char Msg[], CheckerContext &C) const; + /// The bug visitor prints a diagnostic message at the location where a given + /// variable was tainted. + class TaintBugVisitor + : public BugReporterVisitorImpl<TaintBugVisitor> { + private: + const SVal V; + + public: + TaintBugVisitor(const SVal V) : V(V) {} + void Profile(llvm::FoldingSetNodeID &ID) const override { ID.Add(V); } + + std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N, + const ExplodedNode *PrevN, + BugReporterContext &BRC, + BugReport &BR) override; + }; typedef SmallVector<unsigned, 2> ArgVector; @@ -194,6 +222,28 @@ const char GenericTaintChecker::MsgTaintedBufferSize[] = /// points to data, which should be tainted on return. REGISTER_SET_WITH_PROGRAMSTATE(TaintArgsOnPostVisit, unsigned) +std::shared_ptr<PathDiagnosticPiece> +GenericTaintChecker::TaintBugVisitor::VisitNode(const ExplodedNode *N, + const ExplodedNode *PrevN, BugReporterContext &BRC, BugReport &BR) { + + // Find the ExplodedNode where the taint was first introduced + if (!N->getState()->isTainted(V) || PrevN->getState()->isTainted(V)) + return nullptr; + + const Stmt *S = PathDiagnosticLocation::getStmt(N); + if (!S) + return nullptr; + + const LocationContext *NCtx = N->getLocationContext(); + PathDiagnosticLocation L = + PathDiagnosticLocation::createBegin(S, BRC.getSourceManager(), NCtx); + if (!L.isValid() || !L.asLocation().isValid()) + return nullptr; + + return std::make_shared<PathDiagnosticEventPiece>( + L, "Taint originated here"); +} + GenericTaintChecker::TaintPropagationRule GenericTaintChecker::TaintPropagationRule::getTaintPropagationRule( const FunctionDecl *FDecl, @@ -423,6 +473,27 @@ bool GenericTaintChecker::checkPre(const CallExpr *CE, CheckerContext &C) const{ return false; } +SymbolRef GenericTaintChecker::getLCVSymbol(CheckerContext &C, + nonloc::LazyCompoundVal &LCV) { + StoreManager &StoreMgr = C.getStoreManager(); + + // getLCVSymbol() is reached in a PostStmt so we can always expect a default + // binding to exist if one is present. + if (Optional<SVal> binding = StoreMgr.getDefaultBinding(LCV)) { + SymbolRef Sym = binding->getAsSymbol(); + if (!Sym) + return nullptr; + + // If the LCV covers an entire base region return the default conjured symbol. + if (LCV.getRegion() == LCV.getRegion()->getBaseRegion()) + return Sym; + } + + // Otherwise, return a nullptr as there's not yet a functional way to taint + // sub-regions of LCVs. + return nullptr; +} + SymbolRef GenericTaintChecker::getPointedToSymbol(CheckerContext &C, const Expr* Arg) { ProgramStateRef State = C.getState(); @@ -438,6 +509,10 @@ SymbolRef GenericTaintChecker::getPointedToSymbol(CheckerContext &C, dyn_cast<PointerType>(Arg->getType().getCanonicalType().getTypePtr()); SVal Val = State->getSVal(*AddrLoc, ArgTy ? ArgTy->getPointeeType(): QualType()); + + if (auto LCV = Val.getAs<nonloc::LazyCompoundVal>()) + return getLCVSymbol(C, *LCV); + return Val.getAsSymbol(); } @@ -635,8 +710,13 @@ bool GenericTaintChecker::generateReportIfTainted(const Expr *E, // Check for taint. ProgramStateRef State = C.getState(); - if (!State->isTainted(getPointedToSymbol(C, E)) && - !State->isTainted(E, C.getLocationContext())) + const SymbolRef PointedToSym = getPointedToSymbol(C, E); + SVal TaintedSVal; + if (State->isTainted(PointedToSym)) + TaintedSVal = nonloc::SymbolVal(PointedToSym); + else if (State->isTainted(E, C.getLocationContext())) + TaintedSVal = C.getSVal(E); + else return false; // Generate diagnostic. @@ -644,6 +724,7 @@ bool GenericTaintChecker::generateReportIfTainted(const Expr *E, initBugType(); auto report = llvm::make_unique<BugReport>(*BT, Msg, N); report->addRange(E->getSourceRange()); + report->addVisitor(llvm::make_unique<TaintBugVisitor>(TaintedSVal)); C.emitReport(std::move(report)); return true; } diff --git a/lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp b/lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp index 531054aa7887..f0f7c98c9640 100644 --- a/lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/IteratorPastEndChecker.cpp @@ -415,8 +415,6 @@ bool IteratorPastEndChecker::evalCall(const CallExpr *CE, return evalFindFirstOf(C, CE); } else if (FD->getIdentifier() == II_find_if) { return evalFindIf(C, CE); - } else if (FD->getIdentifier() == II_find_if) { - return evalFindIf(C, CE); } else if (FD->getIdentifier() == II_find_if_not) { return evalFindIfNot(C, CE); } else if (FD->getIdentifier() == II_upper_bound) { diff --git a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp index c667b9e67d4b..696cf39473d5 100644 --- a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp @@ -153,9 +153,9 @@ void MPIChecker::allRegionsUsedByWait( MemRegionManager *const RegionManager = MR->getMemRegionManager(); if (FuncClassifier->isMPI_Waitall(CE.getCalleeIdentifier())) { - const MemRegion *SuperRegion{nullptr}; + const SubRegion *SuperRegion{nullptr}; if (const ElementRegion *const ER = MR->getAs<ElementRegion>()) { - SuperRegion = ER->getSuperRegion(); + SuperRegion = cast<SubRegion>(ER->getSuperRegion()); } // A single request is passed to MPI_Waitall. diff --git a/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp b/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp index f1aa16391db1..f8473dbd7647 100644 --- a/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp @@ -28,7 +28,8 @@ using namespace ento; namespace { class MacOSKeychainAPIChecker : public Checker<check::PreStmt<CallExpr>, check::PostStmt<CallExpr>, - check::DeadSymbols> { + check::DeadSymbols, + eval::Assume> { mutable std::unique_ptr<BugType> BT; public: @@ -57,6 +58,10 @@ public: void checkPreStmt(const CallExpr *S, CheckerContext &C) const; void checkPostStmt(const CallExpr *S, CheckerContext &C) const; void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const; + ProgramStateRef evalAssume(ProgramStateRef state, SVal Cond, + bool Assumption) const; + void printState(raw_ostream &Out, ProgramStateRef State, + const char *NL, const char *Sep) const; private: typedef std::pair<SymbolRef, const AllocationState*> AllocationPair; @@ -106,19 +111,6 @@ private: std::unique_ptr<BugReport> generateAllocatedDataNotReleasedReport( const AllocationPair &AP, ExplodedNode *N, CheckerContext &C) const; - /// Check if RetSym evaluates to an error value in the current state. - bool definitelyReturnedError(SymbolRef RetSym, - ProgramStateRef State, - SValBuilder &Builder, - bool noError = false) const; - - /// Check if RetSym evaluates to a NoErr value in the current state. - bool definitelyDidnotReturnError(SymbolRef RetSym, - ProgramStateRef State, - SValBuilder &Builder) const { - return definitelyReturnedError(RetSym, State, Builder, true); - } - /// Mark an AllocationPair interesting for diagnostic reporting. void markInteresting(BugReport *R, const AllocationPair &AP) const { R->markInteresting(AP.first); @@ -221,24 +213,6 @@ static SymbolRef getAsPointeeSymbol(const Expr *Expr, return nullptr; } -// When checking for error code, we need to consider the following cases: -// 1) noErr / [0] -// 2) someErr / [1, inf] -// 3) unknown -// If noError, returns true iff (1). -// If !noError, returns true iff (2). -bool MacOSKeychainAPIChecker::definitelyReturnedError(SymbolRef RetSym, - ProgramStateRef State, - SValBuilder &Builder, - bool noError) const { - DefinedOrUnknownSVal NoErrVal = Builder.makeIntVal(NoErr, - Builder.getSymbolManager().getType(RetSym)); - DefinedOrUnknownSVal NoErr = Builder.evalEQ(State, NoErrVal, - nonloc::SymbolVal(RetSym)); - ProgramStateRef ErrState = State->assume(NoErr, noError); - return ErrState == State; -} - // Report deallocator mismatch. Remove the region from tracking - reporting a // missing free error after this one is redundant. void MacOSKeychainAPIChecker:: @@ -289,27 +263,25 @@ void MacOSKeychainAPIChecker::checkPreStmt(const CallExpr *CE, const Expr *ArgExpr = CE->getArg(paramIdx); if (SymbolRef V = getAsPointeeSymbol(ArgExpr, C)) if (const AllocationState *AS = State->get<AllocatedData>(V)) { - if (!definitelyReturnedError(AS->Region, State, C.getSValBuilder())) { - // Remove the value from the state. The new symbol will be added for - // tracking when the second allocator is processed in checkPostStmt(). - State = State->remove<AllocatedData>(V); - ExplodedNode *N = C.generateNonFatalErrorNode(State); - if (!N) - return; - initBugType(); - SmallString<128> sbuf; - llvm::raw_svector_ostream os(sbuf); - unsigned int DIdx = FunctionsToTrack[AS->AllocatorIdx].DeallocatorIdx; - os << "Allocated data should be released before another call to " - << "the allocator: missing a call to '" - << FunctionsToTrack[DIdx].Name - << "'."; - auto Report = llvm::make_unique<BugReport>(*BT, os.str(), N); - Report->addVisitor(llvm::make_unique<SecKeychainBugVisitor>(V)); - Report->addRange(ArgExpr->getSourceRange()); - Report->markInteresting(AS->Region); - C.emitReport(std::move(Report)); - } + // Remove the value from the state. The new symbol will be added for + // tracking when the second allocator is processed in checkPostStmt(). + State = State->remove<AllocatedData>(V); + ExplodedNode *N = C.generateNonFatalErrorNode(State); + if (!N) + return; + initBugType(); + SmallString<128> sbuf; + llvm::raw_svector_ostream os(sbuf); + unsigned int DIdx = FunctionsToTrack[AS->AllocatorIdx].DeallocatorIdx; + os << "Allocated data should be released before another call to " + << "the allocator: missing a call to '" + << FunctionsToTrack[DIdx].Name + << "'."; + auto Report = llvm::make_unique<BugReport>(*BT, os.str(), N); + Report->addVisitor(llvm::make_unique<SecKeychainBugVisitor>(V)); + Report->addRange(ArgExpr->getSourceRange()); + Report->markInteresting(AS->Region); + C.emitReport(std::move(Report)); } return; } @@ -344,13 +316,12 @@ void MacOSKeychainAPIChecker::checkPreStmt(const CallExpr *CE, // Is the argument to the call being tracked? const AllocationState *AS = State->get<AllocatedData>(ArgSM); - if (!AS && FunctionsToTrack[idx].Kind != ValidAPI) { + if (!AS) return; - } - // If trying to free data which has not been allocated yet, report as a bug. - // TODO: We might want a more precise diagnostic for double free + + // TODO: We might want to report double free here. // (that would involve tracking all the freed symbols in the checker state). - if (!AS || RegionArgIsBad) { + if (RegionArgIsBad) { // It is possible that this is a false positive - the argument might // have entered as an enclosing function parameter. if (isEnclosingFunctionParam(ArgExpr)) @@ -418,23 +389,6 @@ void MacOSKeychainAPIChecker::checkPreStmt(const CallExpr *CE, return; } - // If the buffer can be null and the return status can be an error, - // report a bad call to free. - if (State->assume(ArgSVal.castAs<DefinedSVal>(), false) && - !definitelyDidnotReturnError(AS->Region, State, C.getSValBuilder())) { - ExplodedNode *N = C.generateNonFatalErrorNode(State); - if (!N) - return; - initBugType(); - auto Report = llvm::make_unique<BugReport>( - *BT, "Only call free if a valid (non-NULL) buffer was returned.", N); - Report->addVisitor(llvm::make_unique<SecKeychainBugVisitor>(ArgSM)); - Report->addRange(ArgExpr->getSourceRange()); - Report->markInteresting(AS->Region); - C.emitReport(std::move(Report)); - return; - } - C.addTransition(State); } @@ -540,27 +494,63 @@ MacOSKeychainAPIChecker::generateAllocatedDataNotReleasedReport( return Report; } +/// If the return symbol is assumed to be error, remove the allocated info +/// from consideration. +ProgramStateRef MacOSKeychainAPIChecker::evalAssume(ProgramStateRef State, + SVal Cond, + bool Assumption) const { + AllocatedDataTy AMap = State->get<AllocatedData>(); + if (AMap.isEmpty()) + return State; + + auto *CondBSE = dyn_cast_or_null<BinarySymExpr>(Cond.getAsSymExpr()); + if (!CondBSE) + return State; + BinaryOperator::Opcode OpCode = CondBSE->getOpcode(); + if (OpCode != BO_EQ && OpCode != BO_NE) + return State; + + // Match for a restricted set of patterns for cmparison of error codes. + // Note, the comparisons of type '0 == st' are transformed into SymIntExpr. + SymbolRef ReturnSymbol = nullptr; + if (auto *SIE = dyn_cast<SymIntExpr>(CondBSE)) { + const llvm::APInt &RHS = SIE->getRHS(); + bool ErrorIsReturned = (OpCode == BO_EQ && RHS != NoErr) || + (OpCode == BO_NE && RHS == NoErr); + if (!Assumption) + ErrorIsReturned = !ErrorIsReturned; + if (ErrorIsReturned) + ReturnSymbol = SIE->getLHS(); + } + + if (ReturnSymbol) + for (auto I = AMap.begin(), E = AMap.end(); I != E; ++I) { + if (ReturnSymbol == I->second.Region) + State = State->remove<AllocatedData>(I->first); + } + + return State; +} + void MacOSKeychainAPIChecker::checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const { ProgramStateRef State = C.getState(); - AllocatedDataTy ASet = State->get<AllocatedData>(); - if (ASet.isEmpty()) + AllocatedDataTy AMap = State->get<AllocatedData>(); + if (AMap.isEmpty()) return; bool Changed = false; AllocationPairVec Errors; - for (AllocatedDataTy::iterator I = ASet.begin(), E = ASet.end(); I != E; ++I) { - if (SR.isLive(I->first)) + for (auto I = AMap.begin(), E = AMap.end(); I != E; ++I) { + if (!SR.isDead(I->first)) continue; Changed = true; State = State->remove<AllocatedData>(I->first); - // If the allocated symbol is null or if the allocation call might have - // returned an error, do not report. + // If the allocated symbol is null do not report. ConstraintManager &CMgr = State->getConstraintManager(); ConditionTruthVal AllocFailed = CMgr.isNull(State, I.getKey()); - if (AllocFailed.isConstrainedTrue() || - definitelyReturnedError(I->second.Region, State, C.getSValBuilder())) + if (AllocFailed.isConstrainedTrue()) continue; Errors.push_back(std::make_pair(I->first, &I->second)); } @@ -612,6 +602,22 @@ MacOSKeychainAPIChecker::SecKeychainBugVisitor::VisitNode( "Data is allocated here."); } +void MacOSKeychainAPIChecker::printState(raw_ostream &Out, + ProgramStateRef State, + const char *NL, + const char *Sep) const { + + AllocatedDataTy AMap = State->get<AllocatedData>(); + + if (!AMap.isEmpty()) { + Out << Sep << "KeychainAPIChecker :" << NL; + for (auto I = AMap.begin(), E = AMap.end(); I != E; ++I) { + I.getKey()->dumpToStream(Out); + } + } +} + + void ento::registerMacOSKeychainAPIChecker(CheckerManager &mgr) { mgr.registerChecker<MacOSKeychainAPIChecker>(); } diff --git a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 8e839a1d28fd..6e9b7fefa3d0 100644 --- a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -174,7 +174,10 @@ public: II_valloc(nullptr), II_reallocf(nullptr), II_strndup(nullptr), II_strdup(nullptr), II_win_strdup(nullptr), II_kmalloc(nullptr), II_if_nameindex(nullptr), II_if_freenameindex(nullptr), - II_wcsdup(nullptr), II_win_wcsdup(nullptr) {} + II_wcsdup(nullptr), II_win_wcsdup(nullptr), II_g_malloc(nullptr), + II_g_malloc0(nullptr), II_g_realloc(nullptr), II_g_try_malloc(nullptr), + II_g_try_malloc0(nullptr), II_g_try_realloc(nullptr), + II_g_free(nullptr), II_g_memdup(nullptr) {} /// In pessimistic mode, the checker assumes that it does not know which /// functions might free the memory. @@ -236,7 +239,9 @@ private: *II_realloc, *II_calloc, *II_valloc, *II_reallocf, *II_strndup, *II_strdup, *II_win_strdup, *II_kmalloc, *II_if_nameindex, *II_if_freenameindex, *II_wcsdup, - *II_win_wcsdup; + *II_win_wcsdup, *II_g_malloc, *II_g_malloc0, + *II_g_realloc, *II_g_try_malloc, *II_g_try_malloc0, + *II_g_try_realloc, *II_g_free, *II_g_memdup; mutable Optional<uint64_t> KernelZeroFlagVal; void initIdentifierInfo(ASTContext &C) const; @@ -554,6 +559,16 @@ void MallocChecker::initIdentifierInfo(ASTContext &Ctx) const { II_win_strdup = &Ctx.Idents.get("_strdup"); II_win_wcsdup = &Ctx.Idents.get("_wcsdup"); II_win_alloca = &Ctx.Idents.get("_alloca"); + + // Glib + II_g_malloc = &Ctx.Idents.get("g_malloc"); + II_g_malloc0 = &Ctx.Idents.get("g_malloc0"); + II_g_realloc = &Ctx.Idents.get("g_realloc"); + II_g_try_malloc = &Ctx.Idents.get("g_try_malloc"); + II_g_try_malloc0 = &Ctx.Idents.get("g_try_malloc0"); + II_g_try_realloc = &Ctx.Idents.get("g_try_realloc"); + II_g_free = &Ctx.Idents.get("g_free"); + II_g_memdup = &Ctx.Idents.get("g_memdup"); } bool MallocChecker::isMemFunction(const FunctionDecl *FD, ASTContext &C) const { @@ -589,7 +604,8 @@ bool MallocChecker::isCMemFunction(const FunctionDecl *FD, initIdentifierInfo(C); if (Family == AF_Malloc && CheckFree) { - if (FunI == II_free || FunI == II_realloc || FunI == II_reallocf) + if (FunI == II_free || FunI == II_realloc || FunI == II_reallocf || + FunI == II_g_free) return true; } @@ -597,7 +613,11 @@ bool MallocChecker::isCMemFunction(const FunctionDecl *FD, if (FunI == II_malloc || FunI == II_realloc || FunI == II_reallocf || FunI == II_calloc || FunI == II_valloc || FunI == II_strdup || FunI == II_win_strdup || FunI == II_strndup || FunI == II_wcsdup || - FunI == II_win_wcsdup || FunI == II_kmalloc) + FunI == II_win_wcsdup || FunI == II_kmalloc || + FunI == II_g_malloc || FunI == II_g_malloc0 || + FunI == II_g_realloc || FunI == II_g_try_malloc || + FunI == II_g_try_malloc0 || FunI == II_g_try_realloc || + FunI == II_g_memdup) return true; } @@ -762,7 +782,7 @@ void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const { initIdentifierInfo(C.getASTContext()); IdentifierInfo *FunI = FD->getIdentifier(); - if (FunI == II_malloc) { + if (FunI == II_malloc || FunI == II_g_malloc || FunI == II_g_try_malloc) { if (CE->getNumArgs() < 1) return; if (CE->getNumArgs() < 3) { @@ -791,7 +811,8 @@ void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const { return; State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State); State = ProcessZeroAllocation(C, CE, 0, State); - } else if (FunI == II_realloc) { + } else if (FunI == II_realloc || FunI == II_g_realloc || + FunI == II_g_try_realloc) { State = ReallocMem(C, CE, false, State); State = ProcessZeroAllocation(C, CE, 1, State); } else if (FunI == II_reallocf) { @@ -801,7 +822,7 @@ void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const { State = CallocMem(C, CE, State); State = ProcessZeroAllocation(C, CE, 0, State); State = ProcessZeroAllocation(C, CE, 1, State); - } else if (FunI == II_free) { + } else if (FunI == II_free || FunI == II_g_free) { State = FreeMemAux(C, CE, State, 0, false, ReleasedAllocatedMemory); } else if (FunI == II_strdup || FunI == II_win_strdup || FunI == II_wcsdup || FunI == II_win_wcsdup) { @@ -841,6 +862,18 @@ void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const { AF_IfNameIndex); } else if (FunI == II_if_freenameindex) { State = FreeMemAux(C, CE, State, 0, false, ReleasedAllocatedMemory); + } else if (FunI == II_g_malloc0 || FunI == II_g_try_malloc0) { + if (CE->getNumArgs() < 1) + return; + SValBuilder &svalBuilder = C.getSValBuilder(); + SVal zeroVal = svalBuilder.makeZeroVal(svalBuilder.getContext().CharTy); + State = MallocMemAux(C, CE, CE->getArg(0), zeroVal, State); + State = ProcessZeroAllocation(C, CE, 0, State); + } else if (FunI == II_g_memdup) { + if (CE->getNumArgs() < 2) + return; + State = MallocMemAux(C, CE, CE->getArg(1), UndefinedVal(), State); + State = ProcessZeroAllocation(C, CE, 1, State); } } @@ -1154,7 +1187,7 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C, State = State->BindExpr(CE, C.getLocationContext(), RetVal); // Fill the region with the initialization value. - State = State->bindDefault(RetVal, Init); + State = State->bindDefault(RetVal, Init, LCtx); // Set the region's extent equal to the Size parameter. const SymbolicRegion *R = diff --git a/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp b/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp new file mode 100644 index 000000000000..decc552e1213 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp @@ -0,0 +1,481 @@ +// MisusedMovedObjectChecker.cpp - Check use of moved-from objects. - C++ -===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This defines checker which checks for potential misuses of a moved-from +// object. That means method calls on the object or copying it in moved-from +// state. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/AST/ExprCXX.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" + +using namespace clang; +using namespace ento; + +namespace { + +struct RegionState { +private: + enum Kind { Moved, Reported } K; + RegionState(Kind InK) : K(InK) {} + +public: + bool isReported() const { return K == Reported; } + bool isMoved() const { return K == Moved; } + + static RegionState getReported() { return RegionState(Reported); } + static RegionState getMoved() { return RegionState(Moved); } + + bool operator==(const RegionState &X) const { return K == X.K; } + void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddInteger(K); } +}; + +class MisusedMovedObjectChecker + : public Checker<check::PreCall, check::PostCall, check::EndFunction, + check::DeadSymbols, check::RegionChanges> { +public: + void checkEndFunction(CheckerContext &C) const; + void checkPreCall(const CallEvent &MC, CheckerContext &C) const; + void checkPostCall(const CallEvent &MC, CheckerContext &C) const; + void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const; + ProgramStateRef + checkRegionChanges(ProgramStateRef State, + const InvalidatedSymbols *Invalidated, + ArrayRef<const MemRegion *> ExplicitRegions, + ArrayRef<const MemRegion *> Regions, + const LocationContext *LCtx, const CallEvent *Call) const; + +private: + class MovedBugVisitor : public BugReporterVisitorImpl<MovedBugVisitor> { + public: + MovedBugVisitor(const MemRegion *R) : Region(R), Found(false) {} + + void Profile(llvm::FoldingSetNodeID &ID) const override { + static int X = 0; + ID.AddPointer(&X); + ID.AddPointer(Region); + } + + std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N, + const ExplodedNode *PrevN, + BugReporterContext &BRC, + BugReport &BR) override; + + private: + // The tracked region. + const MemRegion *Region; + bool Found; + }; + + mutable std::unique_ptr<BugType> BT; + ExplodedNode *reportBug(const MemRegion *Region, const CallEvent &Call, + CheckerContext &C, bool isCopy) const; + bool isInMoveSafeContext(const LocationContext *LC) const; + bool isStateResetMethod(const CXXMethodDecl *MethodDec) const; + bool isMoveSafeMethod(const CXXMethodDecl *MethodDec) const; + const ExplodedNode *getMoveLocation(const ExplodedNode *N, + const MemRegion *Region, + CheckerContext &C) const; +}; +} // end anonymous namespace + +REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, RegionState) + +// If a region is removed all of the subregions needs to be removed too. +static ProgramStateRef removeFromState(ProgramStateRef State, + const MemRegion *Region) { + if (!Region) + return State; + // Note: The isSubRegionOf function is not reflexive. + State = State->remove<TrackedRegionMap>(Region); + for (auto &E : State->get<TrackedRegionMap>()) { + if (E.first->isSubRegionOf(Region)) + State = State->remove<TrackedRegionMap>(E.first); + } + return State; +} + +static bool isAnyBaseRegionReported(ProgramStateRef State, + const MemRegion *Region) { + for (auto &E : State->get<TrackedRegionMap>()) { + if (Region->isSubRegionOf(E.first) && E.second.isReported()) + return true; + } + return false; +} + +std::shared_ptr<PathDiagnosticPiece> +MisusedMovedObjectChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N, + const ExplodedNode *PrevN, + BugReporterContext &BRC, + BugReport &BR) { + // We need only the last move of the reported object's region. + // The visitor walks the ExplodedGraph backwards. + if (Found) + return nullptr; + ProgramStateRef State = N->getState(); + ProgramStateRef StatePrev = PrevN->getState(); + const RegionState *TrackedObject = State->get<TrackedRegionMap>(Region); + const RegionState *TrackedObjectPrev = + StatePrev->get<TrackedRegionMap>(Region); + if (!TrackedObject) + return nullptr; + if (TrackedObjectPrev && TrackedObject) + return nullptr; + + // Retrieve the associated statement. + const Stmt *S = PathDiagnosticLocation::getStmt(N); + if (!S) + return nullptr; + Found = true; + + std::string ObjectName; + if (const auto DecReg = Region->getAs<DeclRegion>()) { + const auto *RegionDecl = dyn_cast<NamedDecl>(DecReg->getDecl()); + ObjectName = RegionDecl->getNameAsString(); + } + std::string InfoText; + if (ObjectName != "") + InfoText = "'" + ObjectName + "' became 'moved-from' here"; + else + InfoText = "Became 'moved-from' here"; + + // Generate the extra diagnostic. + PathDiagnosticLocation Pos(S, BRC.getSourceManager(), + N->getLocationContext()); + return std::make_shared<PathDiagnosticEventPiece>(Pos, InfoText, true); +} + +const ExplodedNode *MisusedMovedObjectChecker::getMoveLocation( + const ExplodedNode *N, const MemRegion *Region, CheckerContext &C) const { + // Walk the ExplodedGraph backwards and find the first node that referred to + // the tracked region. + const ExplodedNode *MoveNode = N; + + while (N) { + ProgramStateRef State = N->getState(); + if (!State->get<TrackedRegionMap>(Region)) + break; + MoveNode = N; + N = N->pred_empty() ? nullptr : *(N->pred_begin()); + } + return MoveNode; +} + +ExplodedNode *MisusedMovedObjectChecker::reportBug(const MemRegion *Region, + const CallEvent &Call, + CheckerContext &C, + bool isCopy = false) const { + if (ExplodedNode *N = C.generateNonFatalErrorNode()) { + if (!BT) + BT.reset(new BugType(this, "Usage of a 'moved-from' object", + "C++ move semantics")); + + // Uniqueing report to the same object. + PathDiagnosticLocation LocUsedForUniqueing; + const ExplodedNode *MoveNode = getMoveLocation(N, Region, C); + + if (const Stmt *MoveStmt = PathDiagnosticLocation::getStmt(MoveNode)) + LocUsedForUniqueing = PathDiagnosticLocation::createBegin( + MoveStmt, C.getSourceManager(), MoveNode->getLocationContext()); + + // Creating the error message. + std::string ErrorMessage; + if (isCopy) + ErrorMessage = "Copying a 'moved-from' object"; + else + ErrorMessage = "Method call on a 'moved-from' object"; + if (const auto DecReg = Region->getAs<DeclRegion>()) { + const auto *RegionDecl = dyn_cast<NamedDecl>(DecReg->getDecl()); + ErrorMessage += " '" + RegionDecl->getNameAsString() + "'"; + } + + auto R = + llvm::make_unique<BugReport>(*BT, ErrorMessage, N, LocUsedForUniqueing, + MoveNode->getLocationContext()->getDecl()); + R->addVisitor(llvm::make_unique<MovedBugVisitor>(Region)); + C.emitReport(std::move(R)); + return N; + } + return nullptr; +} + +// Removing the function parameters' MemRegion from the state. This is needed +// for PODs where the trivial destructor does not even created nor executed. +void MisusedMovedObjectChecker::checkEndFunction(CheckerContext &C) const { + auto State = C.getState(); + TrackedRegionMapTy Objects = State->get<TrackedRegionMap>(); + if (Objects.isEmpty()) + return; + + auto LC = C.getLocationContext(); + + const auto LD = dyn_cast_or_null<FunctionDecl>(LC->getDecl()); + if (!LD) + return; + llvm::SmallSet<const MemRegion *, 8> InvalidRegions; + + for (auto Param : LD->parameters()) { + auto Type = Param->getType().getTypePtrOrNull(); + if (!Type) + continue; + if (!Type->isPointerType() && !Type->isReferenceType()) { + InvalidRegions.insert(State->getLValue(Param, LC).getAsRegion()); + } + } + + if (InvalidRegions.empty()) + return; + + for (const auto &E : State->get<TrackedRegionMap>()) { + if (InvalidRegions.count(E.first->getBaseRegion())) + State = State->remove<TrackedRegionMap>(E.first); + } + + C.addTransition(State); +} + +void MisusedMovedObjectChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + const auto *AFC = dyn_cast<AnyFunctionCall>(&Call); + if (!AFC) + return; + + ProgramStateRef State = C.getState(); + const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(AFC->getDecl()); + if (!MethodDecl) + return; + + const auto *ConstructorDecl = dyn_cast<CXXConstructorDecl>(MethodDecl); + + const auto *CC = dyn_cast_or_null<CXXConstructorCall>(&Call); + // Check if an object became moved-from. + // Object can become moved from after a call to move assignment operator or + // move constructor . + if (ConstructorDecl && !ConstructorDecl->isMoveConstructor()) + return; + + if (!ConstructorDecl && !MethodDecl->isMoveAssignmentOperator()) + return; + + const auto ArgRegion = AFC->getArgSVal(0).getAsRegion(); + if (!ArgRegion) + return; + + // Skip moving the object to itself. + if (CC && CC->getCXXThisVal().getAsRegion() == ArgRegion) + return; + if (const auto *IC = dyn_cast<CXXInstanceCall>(AFC)) + if (IC->getCXXThisVal().getAsRegion() == ArgRegion) + return; + + const MemRegion *BaseRegion = ArgRegion->getBaseRegion(); + // Skip temp objects because of their short lifetime. + if (BaseRegion->getAs<CXXTempObjectRegion>() || + AFC->getArgExpr(0)->isRValue()) + return; + // If it has already been reported do not need to modify the state. + + if (State->get<TrackedRegionMap>(ArgRegion)) + return; + // Mark object as moved-from. + State = State->set<TrackedRegionMap>(ArgRegion, RegionState::getMoved()); + C.addTransition(State); +} + +bool MisusedMovedObjectChecker::isMoveSafeMethod( + const CXXMethodDecl *MethodDec) const { + // We abandon the cases where bool/void/void* conversion happens. + if (const auto *ConversionDec = + dyn_cast_or_null<CXXConversionDecl>(MethodDec)) { + const Type *Tp = ConversionDec->getConversionType().getTypePtrOrNull(); + if (!Tp) + return false; + if (Tp->isBooleanType() || Tp->isVoidType() || Tp->isVoidPointerType()) + return true; + } + // Function call `empty` can be skipped. + if (MethodDec && MethodDec->getDeclName().isIdentifier() && + (MethodDec->getName().lower() == "empty" || + MethodDec->getName().lower() == "isempty")) + return true; + + return false; +} + +bool MisusedMovedObjectChecker::isStateResetMethod( + const CXXMethodDecl *MethodDec) const { + if (MethodDec && MethodDec->getDeclName().isIdentifier()) { + std::string MethodName = MethodDec->getName().lower(); + if (MethodName == "reset" || MethodName == "clear" || + MethodName == "destroy") + return true; + } + return false; +} + +// Don't report an error inside a move related operation. +// We assume that the programmer knows what she does. +bool MisusedMovedObjectChecker::isInMoveSafeContext( + const LocationContext *LC) const { + do { + const auto *CtxDec = LC->getDecl(); + auto *CtorDec = dyn_cast_or_null<CXXConstructorDecl>(CtxDec); + auto *DtorDec = dyn_cast_or_null<CXXDestructorDecl>(CtxDec); + auto *MethodDec = dyn_cast_or_null<CXXMethodDecl>(CtxDec); + if (DtorDec || (CtorDec && CtorDec->isCopyOrMoveConstructor()) || + (MethodDec && MethodDec->isOverloadedOperator() && + MethodDec->getOverloadedOperator() == OO_Equal) || + isStateResetMethod(MethodDec) || isMoveSafeMethod(MethodDec)) + return true; + } while ((LC = LC->getParent())); + return false; +} + +void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call, + CheckerContext &C) const { + ProgramStateRef State = C.getState(); + const LocationContext *LC = C.getLocationContext(); + ExplodedNode *N = nullptr; + + // Remove the MemRegions from the map on which a ctor/dtor call or assignement + // happened. + + // Checking constructor calls. + if (const auto *CC = dyn_cast<CXXConstructorCall>(&Call)) { + State = removeFromState(State, CC->getCXXThisVal().getAsRegion()); + auto CtorDec = CC->getDecl(); + // Check for copying a moved-from object and report the bug. + if (CtorDec && CtorDec->isCopyOrMoveConstructor()) { + const MemRegion *ArgRegion = CC->getArgSVal(0).getAsRegion(); + const RegionState *ArgState = State->get<TrackedRegionMap>(ArgRegion); + if (ArgState && ArgState->isMoved()) { + if (!isInMoveSafeContext(LC)) { + N = reportBug(ArgRegion, Call, C, /*isCopy=*/true); + State = State->set<TrackedRegionMap>(ArgRegion, + RegionState::getReported()); + } + } + } + C.addTransition(State, N); + return; + } + + const auto IC = dyn_cast<CXXInstanceCall>(&Call); + if (!IC) + return; + // In case of destructor call we do not track the object anymore. + const MemRegion *ThisRegion = IC->getCXXThisVal().getAsRegion(); + if (dyn_cast_or_null<CXXDestructorDecl>(Call.getDecl())) { + State = removeFromState(State, IC->getCXXThisVal().getAsRegion()); + C.addTransition(State); + return; + } + + const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(IC->getDecl()); + if (!MethodDecl) + return; + // Checking assignment operators. + bool OperatorEq = MethodDecl->isOverloadedOperator() && + MethodDecl->getOverloadedOperator() == OO_Equal; + // Remove the tracked object for every assignment operator, but report bug + // only for move or copy assignment's argument. + if (OperatorEq) { + State = removeFromState(State, ThisRegion); + if (MethodDecl->isCopyAssignmentOperator() || + MethodDecl->isMoveAssignmentOperator()) { + const RegionState *ArgState = + 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); + State = + State->set<TrackedRegionMap>(ArgRegion, RegionState::getReported()); + } + } + C.addTransition(State, N); + return; + } + + // The remaining part is check only for method call on a moved-from object. + if (isMoveSafeMethod(MethodDecl)) + return; + + if (isStateResetMethod(MethodDecl)) { + State = State->remove<TrackedRegionMap>(ThisRegion); + C.addTransition(State); + return; + } + + // If it is already reported then we dont 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 + if (isAnyBaseRegionReported(State, ThisRegion)) + return; + + if (isInMoveSafeContext(LC)) + return; + + N = reportBug(ThisRegion, Call, C); + State = State->set<TrackedRegionMap>(ThisRegion, RegionState::getReported()); + C.addTransition(State, N); +} + +void MisusedMovedObjectChecker::checkDeadSymbols(SymbolReaper &SymReaper, + CheckerContext &C) const { + ProgramStateRef State = C.getState(); + TrackedRegionMapTy TrackedRegions = State->get<TrackedRegionMap>(); + for (TrackedRegionMapTy::value_type E : TrackedRegions) { + const MemRegion *Region = E.first; + bool IsRegDead = !SymReaper.isLiveRegion(Region); + + // Remove the dead regions from the region map. + if (IsRegDead) { + State = State->remove<TrackedRegionMap>(Region); + } + } + C.addTransition(State); +} + +ProgramStateRef MisusedMovedObjectChecker::checkRegionChanges( + ProgramStateRef State, const InvalidatedSymbols *Invalidated, + ArrayRef<const MemRegion *> ExplicitRegions, + ArrayRef<const MemRegion *> Regions, const LocationContext *LCtx, + const CallEvent *Call) const { + // In case of an InstanceCall don't remove the ThisRegion from the GDM since + // it is handled in checkPreCall and checkPostCall. + const MemRegion *ThisRegion = nullptr; + if (const auto *IC = dyn_cast_or_null<CXXInstanceCall>(Call)) { + ThisRegion = IC->getCXXThisVal().getAsRegion(); + } + + for (ArrayRef<const MemRegion *>::iterator I = ExplicitRegions.begin(), + E = ExplicitRegions.end(); + I != E; ++I) { + const auto *Region = *I; + if (ThisRegion != Region) { + State = removeFromState(State, Region); + } + } + + return State; +} + +void ento::registerMisusedMovedObjectChecker(CheckerManager &mgr) { + mgr.registerChecker<MisusedMovedObjectChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp b/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp index 1f82ab94af82..6d05159e51b0 100644 --- a/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp @@ -73,7 +73,7 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call, for (unsigned idx = 0; idx < NumArgs; ++idx) { // Check if the parameter is a reference. We want to report when reference - // to a null pointer is passed as a paramter. + // to a null pointer is passed as a parameter. bool haveRefTypeParam = false; if (TyI != TyE) { haveRefTypeParam = (*TyI)->isReferenceType(); diff --git a/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp index c14a87c9d2a4..21527d8c347a 100644 --- a/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp @@ -49,7 +49,7 @@ namespace { enum class Nullability : char { Contradicted, // Tracked nullability is contradicted by an explicit cast. Do // not report any nullability related issue for this symbol. - // This nullability is propagated agressively to avoid false + // This nullability is propagated aggressively to avoid false // positive results. See the comment on getMostNullable method. Nullable, Unspecified, @@ -57,7 +57,7 @@ enum class Nullability : char { }; /// Returns the most nullable nullability. This is used for message expressions -/// like [reciever method], where the nullability of this expression is either +/// like [receiver method], where the nullability of this expression is either /// the nullability of the receiver or the nullability of the return type of the /// method, depending on which is more nullable. Contradicted is considered to /// be the most nullable, to avoid false positive results. diff --git a/lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp index b9857e51f3ea..dfd2c9afe7fb 100644 --- a/lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp @@ -58,8 +58,7 @@ void ObjCPropertyChecker::checkCopyMutable(const ObjCPropertyDecl *D, if (const ObjCInterfaceDecl *IntD = dyn_cast<ObjCInterfaceDecl>(D->getDeclContext())) { ImplD = IntD->getImplementation(); - } else { - const ObjCCategoryDecl *CatD = cast<ObjCCategoryDecl>(D->getDeclContext()); + } else if (auto *CatD = dyn_cast<ObjCCategoryDecl>(D->getDeclContext())) { ImplD = CatD->getClassInterface()->getImplementation(); } diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp index eb101e12af25..3f6ae6222ce0 100644 --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp @@ -2661,6 +2661,7 @@ public: const InvalidatedSymbols *invalidated, ArrayRef<const MemRegion *> ExplicitRegions, ArrayRef<const MemRegion *> Regions, + const LocationContext* LCtx, const CallEvent *Call) const; void checkPreStmt(const ReturnStmt *S, CheckerContext &C) const; @@ -3647,7 +3648,7 @@ void RetainCountChecker::checkBind(SVal loc, SVal val, const Stmt *S, // same state. SVal StoredVal = state->getSVal(regionLoc->getRegion()); if (StoredVal != val) - escapes = (state == (state->bindLoc(*regionLoc, val))); + escapes = (state == (state->bindLoc(*regionLoc, val, C.getLocationContext()))); } if (!escapes) { // Case 4: We do not currently model what happens when a symbol is @@ -3714,10 +3715,11 @@ ProgramStateRef RetainCountChecker::evalAssume(ProgramStateRef state, ProgramStateRef RetainCountChecker::checkRegionChanges(ProgramStateRef state, - const InvalidatedSymbols *invalidated, - ArrayRef<const MemRegion *> ExplicitRegions, - ArrayRef<const MemRegion *> Regions, - const CallEvent *Call) const { + const InvalidatedSymbols *invalidated, + ArrayRef<const MemRegion *> ExplicitRegions, + ArrayRef<const MemRegion *> Regions, + const LocationContext *LCtx, + const CallEvent *Call) const { if (!invalidated) return state; diff --git a/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp index 38d2aa6d8f9d..f3c2ffc58662 100644 --- a/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp @@ -35,6 +35,30 @@ public: }; } // end anonymous namespace +static bool isArrayIndexOutOfBounds(CheckerContext &C, const Expr *Ex) { + ProgramStateRef state = C.getState(); + const LocationContext *LCtx = C.getLocationContext(); + + if (!isa<ArraySubscriptExpr>(Ex)) + return false; + + SVal Loc = state->getSVal(Ex, LCtx); + if (!Loc.isValid()) + return false; + + const MemRegion *MR = Loc.castAs<loc::MemRegionVal>().getRegion(); + const ElementRegion *ER = dyn_cast<ElementRegion>(MR); + if (!ER) + return false; + + DefinedOrUnknownSVal Idx = ER->getIndex().castAs<DefinedOrUnknownSVal>(); + DefinedOrUnknownSVal NumElements = C.getStoreManager().getSizeInElements( + state, ER->getSuperRegion(), ER->getValueType()); + ProgramStateRef StInBound = state->assumeInBound(Idx, NumElements, true); + ProgramStateRef StOutBound = state->assumeInBound(Idx, NumElements, false); + return StOutBound && !StInBound; +} + void UndefResultChecker::checkPostStmt(const BinaryOperator *B, CheckerContext &C) const { ProgramStateRef state = C.getState(); @@ -77,6 +101,8 @@ void UndefResultChecker::checkPostStmt(const BinaryOperator *B, << " operand of '" << BinaryOperator::getOpcodeStr(B->getOpcode()) << "' is a garbage value"; + if (isArrayIndexOutOfBounds(C, Ex)) + OS << " due to array index out of bounds"; } else { // Neither operand was undefined, but the result is undefined. diff --git a/lib/StaticAnalyzer/Checkers/ValistChecker.cpp b/lib/StaticAnalyzer/Checkers/ValistChecker.cpp index 0b7a4865ddc2..d12ba6258073 100644 --- a/lib/StaticAnalyzer/Checkers/ValistChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ValistChecker.cpp @@ -54,11 +54,11 @@ public: void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const; private: - const MemRegion *getVAListAsRegion(SVal SV, CheckerContext &C) const; + const MemRegion *getVAListAsRegion(SVal SV, const Expr *VAExpr, + bool &IsSymbolic, CheckerContext &C) const; StringRef getVariableNameFromRegion(const MemRegion *Reg) const; const ExplodedNode *getStartCallSite(const ExplodedNode *N, - const MemRegion *Reg, - CheckerContext &C) const; + const MemRegion *Reg) const; void reportUninitializedAccess(const MemRegion *VAList, StringRef Msg, CheckerContext &C) const; @@ -138,14 +138,21 @@ void ValistChecker::checkPreCall(const CallEvent &Call, for (auto FuncInfo : VAListAccepters) { if (!Call.isCalled(FuncInfo.Func)) continue; + bool Symbolic; const MemRegion *VAList = - getVAListAsRegion(Call.getArgSVal(FuncInfo.VAListPos), C); + getVAListAsRegion(Call.getArgSVal(FuncInfo.VAListPos), + Call.getArgExpr(FuncInfo.VAListPos), Symbolic, C); if (!VAList) return; if (C.getState()->contains<InitializedVALists>(VAList)) return; + // We did not see va_start call, but the source of the region is unknown. + // Be conservative and assume the best. + if (Symbolic) + return; + SmallString<80> Errmsg("Function '"); Errmsg += FuncInfo.Func.getFunctionName(); Errmsg += "' is called with an uninitialized va_list argument"; @@ -155,13 +162,41 @@ void ValistChecker::checkPreCall(const CallEvent &Call, } } +const MemRegion *ValistChecker::getVAListAsRegion(SVal SV, const Expr *E, + bool &IsSymbolic, + CheckerContext &C) const { + const MemRegion *Reg = SV.getAsRegion(); + if (!Reg) + return nullptr; + // TODO: In the future this should be abstracted away by the analyzer. + bool VaListModelledAsArray = false; + if (const auto *Cast = dyn_cast<CastExpr>(E)) { + QualType Ty = Cast->getType(); + VaListModelledAsArray = + Ty->isPointerType() && Ty->getPointeeType()->isRecordType(); + } + if (const auto *DeclReg = Reg->getAs<DeclRegion>()) { + if (isa<ParmVarDecl>(DeclReg->getDecl())) + Reg = C.getState()->getSVal(SV.castAs<Loc>()).getAsRegion(); + } + IsSymbolic = Reg && Reg->getAs<SymbolicRegion>(); + // Some VarRegion based VA lists reach here as ElementRegions. + const auto *EReg = dyn_cast_or_null<ElementRegion>(Reg); + return (EReg && VaListModelledAsArray) ? EReg->getSuperRegion() : Reg; +} + void ValistChecker::checkPreStmt(const VAArgExpr *VAA, CheckerContext &C) const { ProgramStateRef State = C.getState(); - SVal VAListSVal = State->getSVal(VAA->getSubExpr(), C.getLocationContext()); - const MemRegion *VAList = getVAListAsRegion(VAListSVal, C); + const Expr *VASubExpr = VAA->getSubExpr(); + SVal VAListSVal = State->getSVal(VASubExpr, C.getLocationContext()); + bool Symbolic; + const MemRegion *VAList = + getVAListAsRegion(VAListSVal, VASubExpr, Symbolic, C); if (!VAList) return; + if (Symbolic) + return; if (!State->contains<InitializedVALists>(VAList)) reportUninitializedAccess( VAList, "va_arg() is called on an uninitialized va_list", C); @@ -183,22 +218,13 @@ void ValistChecker::checkDeadSymbols(SymbolReaper &SR, N); } -const MemRegion *ValistChecker::getVAListAsRegion(SVal SV, - CheckerContext &C) const { - const MemRegion *Reg = SV.getAsRegion(); - const auto *TReg = dyn_cast_or_null<TypedValueRegion>(Reg); - // Some VarRegion based VLAs reach here as ElementRegions. - const auto *EReg = dyn_cast_or_null<ElementRegion>(TReg); - return EReg ? EReg->getSuperRegion() : TReg; -} - // This function traverses the exploded graph backwards and finds the node where // the va_list is initialized. That node is used for uniquing the bug paths. // It is not likely that there are several different va_lists that belongs to // different stack frames, so that case is not yet handled. -const ExplodedNode *ValistChecker::getStartCallSite(const ExplodedNode *N, - const MemRegion *Reg, - CheckerContext &C) const { +const ExplodedNode * +ValistChecker::getStartCallSite(const ExplodedNode *N, + const MemRegion *Reg) const { const LocationContext *LeakContext = N->getLocationContext(); const ExplodedNode *StartCallNode = N; @@ -252,7 +278,7 @@ void ValistChecker::reportLeakedVALists(const RegionVector &LeakedVALists, BT_leakedvalist->setSuppressOnSink(true); } - const ExplodedNode *StartNode = getStartCallSite(N, Reg, C); + const ExplodedNode *StartNode = getStartCallSite(N, Reg); PathDiagnosticLocation LocUsedForUniqueing; if (const Stmt *StartCallStmt = PathDiagnosticLocation::getStmt(StartNode)) @@ -278,13 +304,17 @@ void ValistChecker::reportLeakedVALists(const RegionVector &LeakedVALists, void ValistChecker::checkVAListStartCall(const CallEvent &Call, CheckerContext &C, bool IsCopy) const { - const MemRegion *VAList = getVAListAsRegion(Call.getArgSVal(0), C); - ProgramStateRef State = C.getState(); + bool Symbolic; + const MemRegion *VAList = + getVAListAsRegion(Call.getArgSVal(0), Call.getArgExpr(0), Symbolic, C); if (!VAList) return; + ProgramStateRef State = C.getState(); + if (IsCopy) { - const MemRegion *Arg2 = getVAListAsRegion(Call.getArgSVal(1), C); + const MemRegion *Arg2 = + getVAListAsRegion(Call.getArgSVal(1), Call.getArgExpr(1), Symbolic, C); if (Arg2) { if (ChecksEnabled[CK_CopyToSelf] && VAList == Arg2) { RegionVector LeakedVALists{VAList}; @@ -292,7 +322,7 @@ void ValistChecker::checkVAListStartCall(const CallEvent &Call, reportLeakedVALists(LeakedVALists, "va_list", " is copied onto itself", C, N, true); return; - } else if (!State->contains<InitializedVALists>(Arg2)) { + } else if (!State->contains<InitializedVALists>(Arg2) && !Symbolic) { if (State->contains<InitializedVALists>(VAList)) { State = State->remove<InitializedVALists>(VAList); RegionVector LeakedVALists{VAList}; @@ -321,10 +351,17 @@ void ValistChecker::checkVAListStartCall(const CallEvent &Call, void ValistChecker::checkVAListEndCall(const CallEvent &Call, CheckerContext &C) const { - const MemRegion *VAList = getVAListAsRegion(Call.getArgSVal(0), C); + bool Symbolic; + const MemRegion *VAList = + getVAListAsRegion(Call.getArgSVal(0), Call.getArgExpr(0), Symbolic, C); if (!VAList) return; + // We did not see va_start call, but the source of the region is unknown. + // Be conservative and assume the best. + if (Symbolic) + return; + if (!C.getState()->contains<InitializedVALists>(VAList)) { reportUninitializedAccess( VAList, "va_end() is called on an uninitialized va_list", C); |