diff options
| author | Jordan Rupprecht <rupprecht@google.com> | 2018-12-19 02:24:12 +0000 |
|---|---|---|
| committer | Jordan Rupprecht <rupprecht@google.com> | 2018-12-19 02:24:12 +0000 |
| commit | 55c8788102d8fd203270fabd6513247b2d7fbd87 (patch) | |
| tree | 63ab727404da1afaca89c6578f37f135a50922e7 /lib/StaticAnalyzer/Checkers | |
| parent | 9fa0a1f211b7c9a402767bc895a87481cf347230 (diff) | |
| parent | 46efdf2ccc2a80aefebf8433dbf9c7c959f6e629 (diff) | |
Creating branches/google/stable and tags/google/stable/2018-12-18 from r349201
git-svn-id: https://llvm.org/svn/llvm-project/cfe/branches/google/stable@349597 91177308-0d34-0410-b5e6-96231b3b80d8
Diffstat (limited to 'lib/StaticAnalyzer/Checkers')
55 files changed, 1601 insertions, 1009 deletions
diff --git a/lib/StaticAnalyzer/Checkers/AllocationDiagnostics.cpp b/lib/StaticAnalyzer/Checkers/AllocationDiagnostics.cpp deleted file mode 100644 index 3dec8a58c9..0000000000 --- a/lib/StaticAnalyzer/Checkers/AllocationDiagnostics.cpp +++ /dev/null @@ -1,24 +0,0 @@ -//=- AllocationDiagnostics.cpp - Config options for allocation diags *- C++ -*-// -// -// The LLVM Compiler Infrastructure -// -// This file is distributed under the University of Illinois Open Source -// License. See LICENSE.TXT for details. -// -//===----------------------------------------------------------------------===// -// -// Declares the configuration functions for leaks/allocation diagnostics. -// -//===-------------------------- - -#include "AllocationDiagnostics.h" - -namespace clang { -namespace ento { - -bool shouldIncludeAllocationSiteInLeakDiagnostics(AnalyzerOptions &AOpts) { - return AOpts.getBooleanOption("leak-diagnostics-reference-allocation", - false); -} - -}} diff --git a/lib/StaticAnalyzer/Checkers/AllocationDiagnostics.h b/lib/StaticAnalyzer/Checkers/AllocationDiagnostics.h deleted file mode 100644 index 62b7fab073..0000000000 --- a/lib/StaticAnalyzer/Checkers/AllocationDiagnostics.h +++ /dev/null @@ -1,31 +0,0 @@ -//=--- AllocationDiagnostics.h - Config options for allocation diags *- C++ -*-// -// -// The LLVM Compiler Infrastructure -// -// This file is distributed under the University of Illinois Open Source -// License. See LICENSE.TXT for details. -// -//===----------------------------------------------------------------------===// -// -// Declares the configuration functions for leaks/allocation diagnostics. -// -//===----------------------------------------------------------------------===// - -#ifndef LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_ALLOCATIONDIAGNOSTICS_H -#define LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_ALLOCATIONDIAGNOSTICS_H - -#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h" - -namespace clang { namespace ento { - -/// Returns true if leak diagnostics should directly reference -/// the allocatin site (where possible). -/// -/// The default is false. -/// -bool shouldIncludeAllocationSiteInLeakDiagnostics(AnalyzerOptions &AOpts); - -}} - -#endif - diff --git a/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp b/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp index cc306329dc..f9cf97e508 100644 --- a/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp @@ -45,8 +45,8 @@ class AnalysisOrderChecker check::LiveSymbols> { bool isCallbackEnabled(AnalyzerOptions &Opts, StringRef CallbackName) const { - return Opts.getBooleanOption("*", false, this) || - Opts.getBooleanOption(CallbackName, false, this); + return Opts.getCheckerBooleanOption("*", false, this) || + Opts.getCheckerBooleanOption(CallbackName, false, this); } bool isCallbackEnabled(CheckerContext &C, StringRef CallbackName) const { diff --git a/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp b/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp index a21e10d948..d7f305aea9 100644 --- a/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp +++ b/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp @@ -214,7 +214,7 @@ void NilArgChecker::generateBugReport(ExplodedNode *N, auto R = llvm::make_unique<BugReport>(*BT, Msg, N); R->addRange(Range); - bugreporter::trackNullOrUndefValue(N, E, *R); + bugreporter::trackExpressionValue(N, E, *R); C.emitReport(std::move(R)); } @@ -578,7 +578,7 @@ void CFRetainReleaseChecker::checkPreCall(const CallEvent &Call, auto report = llvm::make_unique<BugReport>(BT, OS.str(), N); report->addRange(Call.getArgSourceRange(0)); - bugreporter::trackNullOrUndefValue(N, Call.getArgExpr(0), *report); + bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *report); C.emitReport(std::move(report)); return; } @@ -800,7 +800,7 @@ void VariadicMethodTypeChecker::checkPreObjCMessage(const ObjCMethodCall &msg, //===----------------------------------------------------------------------===// // The map from container symbol to the container count symbol. -// We currently will remember the last countainer count symbol encountered. +// We currently will remember the last container count symbol encountered. REGISTER_MAP_WITH_PROGRAMSTATE(ContainerCountMap, SymbolRef, SymbolRef) REGISTER_MAP_WITH_PROGRAMSTATE(ContainerNonEmptyMap, SymbolRef, bool) diff --git a/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp b/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp index 0e781d08e2..3541b7f269 100644 --- a/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp @@ -101,9 +101,10 @@ bool BuiltinFunctionChecker::evalCall(const CallExpr *CE, // This must be resolvable at compile time, so we defer to the constant // evaluator for a value. SVal V = UnknownVal(); - llvm::APSInt Result; - if (CE->EvaluateAsInt(Result, C.getASTContext(), Expr::SE_NoSideEffects)) { + Expr::EvalResult EVResult; + if (CE->EvaluateAsInt(EVResult, C.getASTContext(), Expr::SE_NoSideEffects)) { // Make sure the result has the correct type. + llvm::APSInt Result = EVResult.Val.getInt(); SValBuilder &SVB = C.getSValBuilder(); BasicValueFactory &BVF = SVB.getBasicValueFactory(); BVF.getAPSIntType(CE->getType()).apply(Result); diff --git a/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 306813b880..47dbac634a 100644 --- a/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -3,7 +3,6 @@ set(LLVM_LINK_COMPONENTS ) add_clang_library(clangStaticAnalyzerCheckers - AllocationDiagnostics.cpp AnalysisOrderChecker.cpp AnalyzerStatsChecker.cpp ArrayBoundChecker.cpp @@ -35,6 +34,7 @@ add_clang_library(clangStaticAnalyzerCheckers DivZeroChecker.cpp DynamicTypePropagation.cpp DynamicTypeChecker.cpp + EnumCastOutOfRangeChecker.cpp ExprInspectionChecker.cpp FixedAddressChecker.cpp GCDAntipatternChecker.cpp @@ -52,7 +52,7 @@ add_clang_library(clangStaticAnalyzerCheckers MallocOverflowSecurityChecker.cpp MallocSizeofChecker.cpp MmapWriteExecChecker.cpp - MisusedMovedObjectChecker.cpp + MoveChecker.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 12a576e5d8..ed68df93be 100644 --- a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -124,6 +124,7 @@ public: void evalStdCopyBackward(CheckerContext &C, const CallExpr *CE) const; void evalStdCopyCommon(CheckerContext &C, const CallExpr *CE) const; void evalMemset(CheckerContext &C, const CallExpr *CE) const; + void evalBzero(CheckerContext &C, const CallExpr *CE) const; // Utility methods std::pair<ProgramStateRef , ProgramStateRef > @@ -158,7 +159,7 @@ public: static bool SummarizeRegion(raw_ostream &os, ASTContext &Ctx, const MemRegion *MR); - static bool memsetAux(const Expr *DstBuffer, const Expr *CharE, + static bool memsetAux(const Expr *DstBuffer, SVal CharE, const Expr *Size, CheckerContext &C, ProgramStateRef &State); @@ -553,7 +554,8 @@ void CStringChecker::emitNullArgBug(CheckerContext &C, ProgramStateRef State, BuiltinBug *BT = static_cast<BuiltinBug *>(BT_Null.get()); auto Report = llvm::make_unique<BugReport>(*BT, WarningMsg, N); Report->addRange(S->getSourceRange()); - bugreporter::trackNullOrUndefValue(N, S, *Report); + if (const auto *Ex = dyn_cast<Expr>(S)) + bugreporter::trackExpressionValue(N, Ex, *Report); C.emitReport(std::move(Report)); } } @@ -1004,11 +1006,10 @@ bool CStringChecker::SummarizeRegion(raw_ostream &os, ASTContext &Ctx, } } -bool CStringChecker::memsetAux(const Expr *DstBuffer, const Expr *CharE, +bool CStringChecker::memsetAux(const Expr *DstBuffer, SVal CharVal, const Expr *Size, CheckerContext &C, ProgramStateRef &State) { SVal MemVal = C.getSVal(DstBuffer); - SVal CharVal = C.getSVal(CharE); SVal SizeVal = C.getSVal(Size); const MemRegion *MR = MemVal.getAsRegion(); if (!MR) @@ -2183,13 +2184,59 @@ void CStringChecker::evalMemset(CheckerContext &C, const CallExpr *CE) const { // According to the values of the arguments, bind the value of the second // argument to the destination buffer and set string length, or just // invalidate the destination buffer. - if (!memsetAux(Mem, CharE, Size, C, State)) + if (!memsetAux(Mem, C.getSVal(CharE), Size, C, State)) return; State = State->BindExpr(CE, LCtx, MemVal); C.addTransition(State); } +void CStringChecker::evalBzero(CheckerContext &C, const CallExpr *CE) const { + if (CE->getNumArgs() != 2) + return; + + CurrentFunctionDescription = "memory clearance function"; + + const Expr *Mem = CE->getArg(0); + const Expr *Size = CE->getArg(1); + SVal Zero = C.getSValBuilder().makeZeroVal(C.getASTContext().IntTy); + + ProgramStateRef State = C.getState(); + + // See if the size argument is zero. + SVal SizeVal = C.getSVal(Size); + QualType SizeTy = Size->getType(); + + ProgramStateRef StateZeroSize, StateNonZeroSize; + std::tie(StateZeroSize, StateNonZeroSize) = + assumeZero(C, State, SizeVal, SizeTy); + + // If the size is zero, there won't be any actual memory access, + // In this case we just return. + if (StateZeroSize && !StateNonZeroSize) { + C.addTransition(StateZeroSize); + return; + } + + // Get the value of the memory area. + SVal MemVal = C.getSVal(Mem); + + // Ensure the memory area is not null. + // If it is NULL there will be a NULL pointer dereference. + State = checkNonNull(C, StateNonZeroSize, Mem, MemVal); + if (!State) + return; + + State = CheckBufferAccess(C, State, Size, Mem); + if (!State) + return; + + if (!memsetAux(Mem, Zero, Size, C, State)) + return; + + C.addTransition(State); +} + static bool isCPPStdLibraryFunction(const FunctionDecl *FD, StringRef Name) { IdentifierInfo *II = FD->getIdentifier(); if (!II) @@ -2223,7 +2270,8 @@ bool CStringChecker::evalCall(const CallExpr *CE, CheckerContext &C) const { evalFunction = &CStringChecker::evalMemcmp; else if (C.isCLibraryFunction(FDecl, "memmove")) evalFunction = &CStringChecker::evalMemmove; - else if (C.isCLibraryFunction(FDecl, "memset")) + else if (C.isCLibraryFunction(FDecl, "memset") || + C.isCLibraryFunction(FDecl, "explicit_memset")) evalFunction = &CStringChecker::evalMemset; else if (C.isCLibraryFunction(FDecl, "strcpy")) evalFunction = &CStringChecker::evalStrcpy; @@ -2261,6 +2309,9 @@ bool CStringChecker::evalCall(const CallExpr *CE, CheckerContext &C) const { evalFunction = &CStringChecker::evalStdCopy; else if (isCPPStdLibraryFunction(FDecl, "copy_backward")) evalFunction = &CStringChecker::evalStdCopyBackward; + else if (C.isCLibraryFunction(FDecl, "bzero") || + C.isCLibraryFunction(FDecl, "explicit_bzero")) + evalFunction = &CStringChecker::evalBzero; // If the callee isn't a string function, let another checker handle it. if (!evalFunction) @@ -2384,9 +2435,6 @@ void CStringChecker::checkLiveSymbols(ProgramStateRef state, void CStringChecker::checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const { - if (!SR.hasDeadSymbols()) - return; - ProgramStateRef state = C.getState(); CStringLengthTy Entries = state->get<CStringLength>(); if (Entries.isEmpty()) diff --git a/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp b/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp index 1f9bdeb059..400b719cfd 100644 --- a/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp @@ -108,7 +108,7 @@ void CallAndMessageChecker::emitBadCall(BugType *BT, CheckerContext &C, R->addRange(BadE->getSourceRange()); if (BadE->isGLValue()) BadE = bugreporter::getDerefExpr(BadE); - bugreporter::trackNullOrUndefValue(N, BadE, *R); + bugreporter::trackExpressionValue(N, BadE, *R); } C.emitReport(std::move(R)); } @@ -185,9 +185,9 @@ bool CallAndMessageChecker::uninitRefOrPointer( LazyInit_BT(BD, BT); auto R = llvm::make_unique<BugReport>(*BT, Os.str(), N); R->addRange(ArgRange); - if (ArgEx) { - bugreporter::trackNullOrUndefValue(N, ArgEx, *R); - } + if (ArgEx) + bugreporter::trackExpressionValue(N, ArgEx, *R); + C.emitReport(std::move(R)); } return true; @@ -196,6 +196,7 @@ bool CallAndMessageChecker::uninitRefOrPointer( return false; } +namespace { class FindUninitializedField { public: SmallVector<const FieldDecl *, 10> FieldChain; @@ -234,6 +235,7 @@ public: return false; } }; +} // namespace bool CallAndMessageChecker::PreVisitProcessArg(CheckerContext &C, SVal V, @@ -262,7 +264,7 @@ bool CallAndMessageChecker::PreVisitProcessArg(CheckerContext &C, R->addRange(ArgRange); if (ArgEx) - bugreporter::trackNullOrUndefValue(N, ArgEx, *R); + bugreporter::trackExpressionValue(N, ArgEx, *R); C.emitReport(std::move(R)); } return true; @@ -305,7 +307,7 @@ bool CallAndMessageChecker::PreVisitProcessArg(CheckerContext &C, R->addRange(ArgRange); if (ArgEx) - bugreporter::trackNullOrUndefValue(N, ArgEx, *R); + bugreporter::trackExpressionValue(N, ArgEx, *R); // FIXME: enhance track back for uninitialized value for arbitrary // memregions C.emitReport(std::move(R)); @@ -365,7 +367,7 @@ void CallAndMessageChecker::checkPreStmt(const CXXDeleteExpr *DE, Desc = "Argument to 'delete' is uninitialized"; BugType *BT = BT_cxx_delete_undef.get(); auto R = llvm::make_unique<BugReport>(*BT, Desc, N); - bugreporter::trackNullOrUndefValue(N, DE, *R); + bugreporter::trackExpressionValue(N, DE, *R); C.emitReport(std::move(R)); return; } @@ -494,7 +496,7 @@ void CallAndMessageChecker::checkPreObjCMessage(const ObjCMethodCall &msg, // FIXME: getTrackNullOrUndefValueVisitor can't handle "super" yet. if (const Expr *ReceiverE = ME->getInstanceReceiver()) - bugreporter::trackNullOrUndefValue(N, ReceiverE, *R); + bugreporter::trackExpressionValue(N, ReceiverE, *R); C.emitReport(std::move(R)); } return; @@ -535,7 +537,7 @@ void CallAndMessageChecker::emitNilReceiverBug(CheckerContext &C, report->addRange(ME->getReceiverRange()); // FIXME: This won't track "self" in messages to super. if (const Expr *receiver = ME->getInstanceReceiver()) { - bugreporter::trackNullOrUndefValue(N, receiver, *report); + bugreporter::trackExpressionValue(N, receiver, *report); } C.emitReport(std::move(report)); } diff --git a/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp b/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp index 202233acff..87d7d90ee2 100644 --- a/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp +++ b/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp @@ -188,7 +188,7 @@ void WalkAST::VisitForStmt(ForStmt *FS) { } //===----------------------------------------------------------------------===// -// Check: floating poing variable used as loop counter. +// Check: floating point variable used as loop counter. // Originally: <rdar://problem/6336718> // Implements: CERT security coding advisory FLP-30. //===----------------------------------------------------------------------===// @@ -597,9 +597,10 @@ void WalkAST::checkCall_mkstemp(const CallExpr *CE, const FunctionDecl *FD) { unsigned suffix = 0; if (ArgSuffix.second >= 0) { const Expr *suffixEx = CE->getArg((unsigned)ArgSuffix.second); - llvm::APSInt Result; - if (!suffixEx->EvaluateAsInt(Result, BR.getContext())) + Expr::EvalResult EVResult; + if (!suffixEx->EvaluateAsInt(EVResult, BR.getContext())) return; + llvm::APSInt Result = EVResult.Val.getInt(); // FIXME: Issue a warning. if (Result.isNegative()) return; diff --git a/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp b/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp index 7862a4c256..e5f2937300 100644 --- a/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp +++ b/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp @@ -169,7 +169,7 @@ public: /// This callback should be used by the checkers to aggressively clean /// up/reduce the checker state, which is important for reducing the overall /// memory usage. Specifically, if a checker keeps symbol specific information - /// in the sate, it can and should be dropped after the symbol becomes dead. + /// in the state, it can and should be dropped after the symbol becomes dead. /// In addition, reporting a bug as soon as the checker becomes dead leads to /// more precise diagnostics. (For example, one should report that a malloced /// variable is not freed right after it goes out of scope.) diff --git a/lib/StaticAnalyzer/Checkers/ClangCheckers.cpp b/lib/StaticAnalyzer/Checkers/ClangCheckers.cpp index fb9e366c3d..d12e421d31 100644 --- a/lib/StaticAnalyzer/Checkers/ClangCheckers.cpp +++ b/lib/StaticAnalyzer/Checkers/ClangCheckers.cpp @@ -25,7 +25,7 @@ using namespace ento; void ento::registerBuiltinCheckers(CheckerRegistry ®istry) { #define GET_CHECKERS -#define CHECKER(FULLNAME,CLASS,DESCFILE,HELPTEXT,GROUPINDEX,HIDDEN) \ +#define CHECKER(FULLNAME, CLASS, HELPTEXT) \ registry.addChecker(register##CLASS, FULLNAME, HELPTEXT); #include "clang/StaticAnalyzer/Checkers/Checkers.inc" #undef GET_CHECKERS diff --git a/lib/StaticAnalyzer/Checkers/ClangSACheckers.h b/lib/StaticAnalyzer/Checkers/ClangSACheckers.h index d6e96f27a7..cd42cd6cd3 100644 --- a/lib/StaticAnalyzer/Checkers/ClangSACheckers.h +++ b/lib/StaticAnalyzer/Checkers/ClangSACheckers.h @@ -24,7 +24,7 @@ class CheckerManager; class CheckerRegistry; #define GET_CHECKERS -#define CHECKER(FULLNAME,CLASS,CXXFILE,HELPTEXT,GROUPINDEX,HIDDEN) \ +#define CHECKER(FULLNAME, CLASS, HELPTEXT) \ void register##CLASS(CheckerManager &mgr); #include "clang/StaticAnalyzer/Checkers/Checkers.inc" #undef CHECKER diff --git a/lib/StaticAnalyzer/Checkers/CloneChecker.cpp b/lib/StaticAnalyzer/Checkers/CloneChecker.cpp index ee517ed977..427b9c4ad2 100644 --- a/lib/StaticAnalyzer/Checkers/CloneChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CloneChecker.cpp @@ -42,7 +42,7 @@ public: void reportClones(BugReporter &BR, AnalysisManager &Mgr, std::vector<CloneDetector::CloneGroup> &CloneGroups) const; - /// Reports only suspicious clones to the user along with informaton + /// Reports only suspicious clones to the user along with information /// that explain why they are suspicious. void reportSuspiciousClones( BugReporter &BR, AnalysisManager &Mgr, @@ -63,18 +63,18 @@ void CloneChecker::checkEndOfTranslationUnit(const TranslationUnitDecl *TU, // At this point, every statement in the translation unit has been analyzed by // the CloneDetector. The only thing left to do is to report the found clones. - int MinComplexity = Mgr.getAnalyzerOptions().getOptionAsInteger( + int MinComplexity = Mgr.getAnalyzerOptions().getCheckerIntegerOption( "MinimumCloneComplexity", 50, this); assert(MinComplexity >= 0); - bool ReportSuspiciousClones = Mgr.getAnalyzerOptions().getBooleanOption( - "ReportSuspiciousClones", true, this); + bool ReportSuspiciousClones = Mgr.getAnalyzerOptions() + .getCheckerBooleanOption("ReportSuspiciousClones", true, this); - bool ReportNormalClones = Mgr.getAnalyzerOptions().getBooleanOption( + bool ReportNormalClones = Mgr.getAnalyzerOptions().getCheckerBooleanOption( "ReportNormalClones", true, this); - StringRef IgnoredFilesPattern = Mgr.getAnalyzerOptions().getOptionAsString( - "IgnoredFilesPattern", "", this); + StringRef IgnoredFilesPattern = Mgr.getAnalyzerOptions() + .getCheckerStringOption("IgnoredFilesPattern", "", 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 diff --git a/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp b/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp index 17ec2c2887..208f94451c 100644 --- a/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp @@ -14,8 +14,10 @@ // of expressions. A warning is reported when: // * a negative value is implicitly converted to an unsigned value in an // assignment, comparison or multiplication. -// * assignment / initialization when source value is greater than the max -// value of target +// * assignment / initialization when the source value is greater than the max +// value of the target integer type +// * assignment / initialization when the source integer is above the range +// where the target floating point type can represent all integers // // Many compilers and tools have similar checks that are based on semantic // analysis. Those checks are sound but have poor precision. ConversionChecker @@ -28,6 +30,9 @@ #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/CheckerManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "llvm/ADT/APFloat.h" + +#include <climits> using namespace clang; using namespace ento; @@ -40,11 +45,9 @@ public: private: mutable std::unique_ptr<BuiltinBug> BT; - // Is there loss of precision bool isLossOfPrecision(const ImplicitCastExpr *Cast, QualType DestType, CheckerContext &C) const; - // Is there loss of sign bool isLossOfSign(const ImplicitCastExpr *Cast, CheckerContext &C) const; void reportBug(ExplodedNode *N, CheckerContext &C, const char Msg[]) const; @@ -132,19 +135,51 @@ bool ConversionChecker::isLossOfPrecision(const ImplicitCastExpr *Cast, QualType SubType = Cast->IgnoreParenImpCasts()->getType(); - if (!DestType->isIntegerType() || !SubType->isIntegerType()) + if (!DestType->isRealType() || !SubType->isIntegerType()) return false; - if (C.getASTContext().getIntWidth(DestType) >= - C.getASTContext().getIntWidth(SubType)) + const bool isFloat = DestType->isFloatingType(); + + const auto &AC = C.getASTContext(); + + // We will find the largest RepresentsUntilExp value such that the DestType + // can exactly represent all nonnegative integers below 2^RepresentsUntilExp. + unsigned RepresentsUntilExp; + + if (isFloat) { + const llvm::fltSemantics &Sema = AC.getFloatTypeSemantics(DestType); + RepresentsUntilExp = llvm::APFloat::semanticsPrecision(Sema); + } else { + RepresentsUntilExp = AC.getIntWidth(DestType); + if (RepresentsUntilExp == 1) { + // This is just casting a number to bool, probably not a bug. + return false; + } + if (DestType->isSignedIntegerType()) + RepresentsUntilExp--; + } + + if (RepresentsUntilExp >= sizeof(unsigned long long) * CHAR_BIT) { + // Avoid overflow in our later calculations. return false; + } + + unsigned CorrectedSrcWidth = AC.getIntWidth(SubType); + if (SubType->isSignedIntegerType()) + CorrectedSrcWidth--; - unsigned W = C.getASTContext().getIntWidth(DestType); - if (W == 1 || W >= 64U) + if (RepresentsUntilExp >= CorrectedSrcWidth) { + // Simple case: the destination can store all values of the source type. return false; + } - unsigned long long MaxVal = 1ULL << W; + unsigned long long MaxVal = 1ULL << RepresentsUntilExp; + if (isFloat) { + // If this is a floating point type, it can also represent MaxVal exactly. + MaxVal++; + } return C.isGreaterOrEqual(Cast->getSubExpr(), MaxVal); + // TODO: maybe also check negative values with too large magnitude. } bool ConversionChecker::isLossOfSign(const ImplicitCastExpr *Cast, diff --git a/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp b/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp index 76a9a7c567..7446eadf34 100644 --- a/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp @@ -329,9 +329,8 @@ public: return; if (const Expr *E = V->getInit()) { - while (const ExprWithCleanups *exprClean = - dyn_cast<ExprWithCleanups>(E)) - E = exprClean->getSubExpr(); + while (const FullExpr *FE = dyn_cast<FullExpr>(E)) + E = FE->getSubExpr(); // Look through transitive assignments, e.g.: // int x = y = 0; diff --git a/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp b/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp index 810a33ed40..60027f4a8b 100644 --- a/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp +++ b/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp @@ -182,7 +182,9 @@ public: llvm::errs() << "[config]\n"; for (unsigned I = 0, E = Keys.size(); I != E; ++I) - llvm::errs() << Keys[I]->getKey() << " = " << Keys[I]->second << '\n'; + llvm::errs() << Keys[I]->getKey() << " = " + << (Keys[I]->second.empty() ? "\"\"" : Keys[I]->second) + << '\n'; llvm::errs() << "[stats]\n" << "num-entries = " << Keys.size() << '\n'; } diff --git a/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp b/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp index 152b937bb0..368d5ce357 100644 --- a/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp @@ -111,6 +111,12 @@ static bool suppressReport(const Expr *E) { return E->getType().getQualifiers().hasAddressSpace(); } +static bool isDeclRefExprToReference(const Expr *E) { + if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) + return DRE->getDecl()->getType()->isReferenceType(); + return false; +} + void DereferenceChecker::reportBug(ProgramStateRef State, const Stmt *S, CheckerContext &C) const { // Generate an error node. @@ -154,7 +160,7 @@ void DereferenceChecker::reportBug(ProgramStateRef State, const Stmt *S, } case Stmt::MemberExprClass: { const MemberExpr *M = cast<MemberExpr>(S); - if (M->isArrow() || bugreporter::isDeclRefExprToReference(M->getBase())) { + if (M->isArrow() || isDeclRefExprToReference(M->getBase())) { os << "Access to field '" << M->getMemberNameInfo() << "' results in a dereference of a null pointer"; AddDerefSource(os, Ranges, M->getBase()->IgnoreParenCasts(), @@ -177,7 +183,7 @@ void DereferenceChecker::reportBug(ProgramStateRef State, const Stmt *S, auto report = llvm::make_unique<BugReport>( *BT_null, buf.empty() ? BT_null->getDescription() : StringRef(buf), N); - bugreporter::trackNullOrUndefValue(N, bugreporter::getDerefExpr(S), *report); + bugreporter::trackExpressionValue(N, bugreporter::getDerefExpr(S), *report); for (SmallVectorImpl<SourceRange>::iterator I = Ranges.begin(), E = Ranges.end(); I!=E; ++I) @@ -197,8 +203,7 @@ void DereferenceChecker::checkLocation(SVal l, bool isLoad, const Stmt* S, auto report = llvm::make_unique<BugReport>(*BT_undef, BT_undef->getDescription(), N); - bugreporter::trackNullOrUndefValue(N, bugreporter::getDerefExpr(S), - *report); + bugreporter::trackExpressionValue(N, bugreporter::getDerefExpr(S), *report); C.emitReport(std::move(report)); } return; diff --git a/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp b/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp index bc39c92ea9..5e10fa99fb 100644 --- a/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp @@ -32,6 +32,13 @@ public: }; } // end anonymous namespace +static const Expr *getDenomExpr(const ExplodedNode *N) { + const Stmt *S = N->getLocationAs<PreStmt>()->getStmt(); + if (const auto *BE = dyn_cast<BinaryOperator>(S)) + return BE->getRHS(); + return nullptr; +} + void DivZeroChecker::reportBug( const char *Msg, ProgramStateRef StateZero, CheckerContext &C, std::unique_ptr<BugReporterVisitor> Visitor) const { @@ -41,7 +48,7 @@ void DivZeroChecker::reportBug( auto R = llvm::make_unique<BugReport>(*BT, Msg, N); R->addVisitor(std::move(Visitor)); - bugreporter::trackNullOrUndefValue(N, bugreporter::GetDenomExpr(N), *R); + bugreporter::trackExpressionValue(N, getDenomExpr(N), *R); C.emitReport(std::move(R)); } } diff --git a/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp b/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp index b5a3c7187f..f83a0ec075 100644 --- a/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp +++ b/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp @@ -123,11 +123,6 @@ void DynamicTypePropagation::checkDeadSymbols(SymbolReaper &SR, } } - if (!SR.hasDeadSymbols()) { - C.addTransition(State); - return; - } - MostSpecializedTypeArgsMapTy TyArgMap = State->get<MostSpecializedTypeArgsMap>(); for (MostSpecializedTypeArgsMapTy::iterator I = TyArgMap.begin(), diff --git a/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp b/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp new file mode 100644 index 0000000000..f3a35daf07 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp @@ -0,0 +1,128 @@ +//===- EnumCastOutOfRangeChecker.cpp ---------------------------*- C++ -*--===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// The EnumCastOutOfRangeChecker is responsible for checking integer to +// enumeration casts that could result in undefined values. This could happen +// if the value that we cast from is out of the value range of the enumeration. +// Reference: +// [ISO/IEC 14882-2014] ISO/IEC 14882-2014. +// Programming Languages — C++, Fourth Edition. 2014. +// C++ Standard, [dcl.enum], in paragraph 8, which defines the range of an enum +// C++ Standard, [expr.static.cast], paragraph 10, which defines the behaviour +// of casting an integer value that is out of range +// SEI CERT C++ Coding Standard, INT50-CPP. Do not cast to an out-of-range +// enumeration value +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" + +using namespace clang; +using namespace ento; + +namespace { +// This evaluator checks two SVals for equality. The first SVal is provided via +// the constructor, the second is the parameter of the overloaded () operator. +// It uses the in-built ConstraintManager to resolve the equlity to possible or +// not possible ProgramStates. +class ConstraintBasedEQEvaluator { + const DefinedOrUnknownSVal CompareValue; + const ProgramStateRef PS; + SValBuilder &SVB; + +public: + ConstraintBasedEQEvaluator(CheckerContext &C, + const DefinedOrUnknownSVal CompareValue) + : CompareValue(CompareValue), PS(C.getState()), SVB(C.getSValBuilder()) {} + + bool operator()(const llvm::APSInt &EnumDeclInitValue) { + DefinedOrUnknownSVal EnumDeclValue = SVB.makeIntVal(EnumDeclInitValue); + DefinedOrUnknownSVal ElemEqualsValueToCast = + SVB.evalEQ(PS, EnumDeclValue, CompareValue); + + return static_cast<bool>(PS->assume(ElemEqualsValueToCast, true)); + } +}; + +// This checker checks CastExpr statements. +// If the value provided to the cast is one of the values the enumeration can +// represent, the said value matches the enumeration. If the checker can +// establish the impossibility of matching it gives a warning. +// Being conservative, it does not warn if there is slight possibility the +// value can be matching. +class EnumCastOutOfRangeChecker : public Checker<check::PreStmt<CastExpr>> { + mutable std::unique_ptr<BuiltinBug> EnumValueCastOutOfRange; + void reportWarning(CheckerContext &C) const; + +public: + void checkPreStmt(const CastExpr *CE, CheckerContext &C) const; +}; + +using EnumValueVector = llvm::SmallVector<llvm::APSInt, 6>; + +// Collects all of the values an enum can represent (as SVals). +EnumValueVector getDeclValuesForEnum(const EnumDecl *ED) { + EnumValueVector DeclValues( + std::distance(ED->enumerator_begin(), ED->enumerator_end())); + llvm::transform(ED->enumerators(), DeclValues.begin(), + [](const EnumConstantDecl *D) { return D->getInitVal(); }); + return DeclValues; +} +} // namespace + +void EnumCastOutOfRangeChecker::reportWarning(CheckerContext &C) const { + if (const ExplodedNode *N = C.generateNonFatalErrorNode()) { + if (!EnumValueCastOutOfRange) + EnumValueCastOutOfRange.reset( + new BuiltinBug(this, "Enum cast out of range", + "The value provided to the cast expression is not in " + "the valid range of values for the enum")); + C.emitReport(llvm::make_unique<BugReport>( + *EnumValueCastOutOfRange, EnumValueCastOutOfRange->getDescription(), + N)); + } +} + +void EnumCastOutOfRangeChecker::checkPreStmt(const CastExpr *CE, + CheckerContext &C) const { + // Get the value of the expression to cast. + const llvm::Optional<DefinedOrUnknownSVal> ValueToCast = + C.getSVal(CE->getSubExpr()).getAs<DefinedOrUnknownSVal>(); + + // If the value cannot be reasoned about (not even a DefinedOrUnknownSVal), + // don't analyze further. + if (!ValueToCast) + return; + + const QualType T = CE->getType(); + // Check whether the cast type is an enum. + if (!T->isEnumeralType()) + return; + + // If the cast is an enum, get its declaration. + // If the isEnumeralType() returned true, then the declaration must exist + // even if it is a stub declaration. It is up to the getDeclValuesForEnum() + // function to handle this. + const EnumDecl *ED = T->castAs<EnumType>()->getDecl(); + + EnumValueVector DeclValues = getDeclValuesForEnum(ED); + // Check if any of the enum values possibly match. + bool PossibleValueMatch = llvm::any_of( + DeclValues, ConstraintBasedEQEvaluator(C, *ValueToCast)); + + // If there is no value that can possibly match any of the enum values, then + // warn. + if (!PossibleValueMatch) + reportWarning(C); +} + +void ento::registerEnumCastOutOfRangeChecker(CheckerManager &mgr) { + mgr.registerChecker<EnumCastOutOfRangeChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp b/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp index 7c53b2d21a..0752dba49c 100644 --- a/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp @@ -337,6 +337,7 @@ void ExprInspectionChecker::analyzerDenote(const CallExpr *CE, C.addTransition(C.getState()->set<DenotedSymbols>(Sym, E)); } +namespace { class SymbolExpressor : public SymExprVisitor<SymbolExpressor, Optional<std::string>> { ProgramStateRef State; @@ -369,6 +370,7 @@ public: return None; } }; +} // namespace void ExprInspectionChecker::analyzerExpress(const CallExpr *CE, CheckerContext &C) const { diff --git a/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp b/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp index 7a71751342..0b8f677a4a 100644 --- a/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp @@ -238,14 +238,17 @@ class IteratorChecker void handleEraseAfter(CheckerContext &C, const SVal &Iter) const; void handleEraseAfter(CheckerContext &C, const SVal &Iter1, const SVal &Iter2) const; + void verifyIncrement(CheckerContext &C, const SVal &Iter) const; + void verifyDecrement(CheckerContext &C, const SVal &Iter) const; void verifyRandomIncrOrDecr(CheckerContext &C, OverloadedOperatorKind Op, - const SVal &RetVal, const SVal &LHS, - const SVal &RHS) const; + const SVal &LHS, const SVal &RHS) const; void verifyMatch(CheckerContext &C, const SVal &Iter, const MemRegion *Cont) const; void verifyMatch(CheckerContext &C, const SVal &Iter1, const SVal &Iter2) const; - + IteratorPosition advancePosition(CheckerContext &C, OverloadedOperatorKind Op, + const IteratorPosition &Pos, + const SVal &Distance) const; void reportOutOfRangeBug(const StringRef &Message, const SVal &Val, CheckerContext &C, ExplodedNode *ErrNode) const; void reportMismatchedBug(const StringRef &Message, const SVal &Val1, @@ -388,7 +391,9 @@ ProgramStateRef setContainerData(ProgramStateRef State, const MemRegion *Cont, bool hasLiveIterators(ProgramStateRef State, const MemRegion *Cont); bool isBoundThroughLazyCompoundVal(const Environment &Env, const MemRegion *Reg); -bool isOutOfRange(ProgramStateRef State, const IteratorPosition &Pos); +bool isPastTheEnd(ProgramStateRef State, const IteratorPosition &Pos); +bool isAheadOfRange(ProgramStateRef State, const IteratorPosition &Pos); +bool isBehindPastTheEnd(ProgramStateRef State, const IteratorPosition &Pos); bool isZero(ProgramStateRef State, const NonLoc &Val); } // namespace @@ -422,29 +427,46 @@ void IteratorChecker::checkPreCall(const CallEvent &Call, verifyAccess(C, Call.getArgSVal(0)); } } - if (ChecksEnabled[CK_IteratorRangeChecker] && - isRandomIncrOrDecrOperator(Func->getOverloadedOperator())) { - if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) { - // Check for out-of-range incrementions and decrementions - if (Call.getNumArgs() >= 1) { - verifyRandomIncrOrDecr(C, Func->getOverloadedOperator(), - Call.getReturnValue(), - InstCall->getCXXThisVal(), Call.getArgSVal(0)); + if (ChecksEnabled[CK_IteratorRangeChecker]) { + if (isIncrementOperator(Func->getOverloadedOperator())) { + // Check for out-of-range incrementions + if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) { + verifyIncrement(C, InstCall->getCXXThisVal()); + } else { + if (Call.getNumArgs() >= 1) { + verifyIncrement(C, Call.getArgSVal(0)); + } } - } else { - if (Call.getNumArgs() >= 2) { - verifyRandomIncrOrDecr(C, Func->getOverloadedOperator(), - Call.getReturnValue(), Call.getArgSVal(0), - Call.getArgSVal(1)); + } else if (isDecrementOperator(Func->getOverloadedOperator())) { + // Check for out-of-range decrementions + if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) { + verifyDecrement(C, InstCall->getCXXThisVal()); + } else { + if (Call.getNumArgs() >= 1) { + verifyDecrement(C, Call.getArgSVal(0)); + } + } + } else if (isRandomIncrOrDecrOperator(Func->getOverloadedOperator())) { + if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) { + // Check for out-of-range incrementions and decrementions + if (Call.getNumArgs() >= 1) { + verifyRandomIncrOrDecr(C, Func->getOverloadedOperator(), + InstCall->getCXXThisVal(), + Call.getArgSVal(0)); + } + } else { + if (Call.getNumArgs() >= 2) { + verifyRandomIncrOrDecr(C, Func->getOverloadedOperator(), + Call.getArgSVal(0), Call.getArgSVal(1)); + } + } + } else if (isDereferenceOperator(Func->getOverloadedOperator())) { + // Check for dereference of out-of-range iterators + if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) { + verifyDereference(C, InstCall->getCXXThisVal()); + } else { + verifyDereference(C, Call.getArgSVal(0)); } - } - } else if (ChecksEnabled[CK_IteratorRangeChecker] && - isDereferenceOperator(Func->getOverloadedOperator())) { - // Check for dereference of out-of-range iterators - if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) { - verifyDereference(C, InstCall->getCXXThisVal()); - } else { - verifyDereference(C, Call.getArgSVal(0)); } } else if (ChecksEnabled[CK_MismatchedIteratorChecker] && isComparisonOperator(Func->getOverloadedOperator())) { @@ -529,7 +551,7 @@ void IteratorChecker::checkPreCall(const CallEvent &Call, // // In this case the first two arguments to f() must be iterators must belong // to the same container and the last to also to the same container but - // not neccessarily to the same as the first two. + // not necessarily to the same as the first two. if (!ChecksEnabled[CK_MismatchedIteratorChecker]) return; @@ -895,11 +917,12 @@ void IteratorChecker::verifyDereference(CheckerContext &C, const SVal &Val) const { auto State = C.getState(); const auto *Pos = getIteratorPosition(State, Val); - if (Pos && isOutOfRange(State, *Pos)) { + if (Pos && isPastTheEnd(State, *Pos)) { auto *N = C.generateNonFatalErrorNode(State); if (!N) return; - reportOutOfRangeBug("Iterator accessed outside of its range.", Val, C, N); + reportOutOfRangeBug("Past-the-end iterator dereferenced.", Val, C, N); + return; } } @@ -924,14 +947,9 @@ void IteratorChecker::handleIncrement(CheckerContext &C, const SVal &RetVal, if (Pos) { auto &SymMgr = C.getSymbolManager(); auto &BVF = SymMgr.getBasicVals(); - auto &SVB = C.getSValBuilder(); - const auto OldOffset = Pos->getOffset(); - auto NewOffset = - SVB.evalBinOp(State, BO_Add, - nonloc::SymbolVal(OldOffset), - nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(1))), - SymMgr.getType(OldOffset)).getAsSymbol(); - auto NewPos = Pos->setTo(NewOffset); + const auto NewPos = + advancePosition(C, OO_Plus, *Pos, + nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(1)))); State = setIteratorPosition(State, Iter, NewPos); State = setIteratorPosition(State, RetVal, Postfix ? *Pos : NewPos); C.addTransition(State); @@ -947,14 +965,9 @@ void IteratorChecker::handleDecrement(CheckerContext &C, const SVal &RetVal, if (Pos) { auto &SymMgr = C.getSymbolManager(); auto &BVF = SymMgr.getBasicVals(); - auto &SVB = C.getSValBuilder(); - const auto OldOffset = Pos->getOffset(); - auto NewOffset = - SVB.evalBinOp(State, BO_Sub, - nonloc::SymbolVal(OldOffset), - nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(1))), - SymMgr.getType(OldOffset)).getAsSymbol(); - auto NewPos = Pos->setTo(NewOffset); + const auto NewPos = + advancePosition(C, OO_Minus, *Pos, + nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(1)))); State = setIteratorPosition(State, Iter, NewPos); State = setIteratorPosition(State, RetVal, Postfix ? *Pos : NewPos); C.addTransition(State); @@ -1020,78 +1033,71 @@ void IteratorChecker::handleRandomIncrOrDecr(CheckerContext &C, value = &val; } - auto &SymMgr = C.getSymbolManager(); - auto &SVB = C.getSValBuilder(); - auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub; - const auto OldOffset = Pos->getOffset(); - SymbolRef NewOffset; - if (const auto intValue = value->getAs<nonloc::ConcreteInt>()) { - // For concrete integers we can calculate the new position - NewOffset = SVB.evalBinOp(State, BinOp, nonloc::SymbolVal(OldOffset), - *intValue, - SymMgr.getType(OldOffset)).getAsSymbol(); - } else { - // For other symbols create a new symbol to keep expressions simple - const auto &LCtx = C.getLocationContext(); - NewOffset = SymMgr.conjureSymbol(nullptr, LCtx, SymMgr.getType(OldOffset), - C.blockCount()); - State = assumeNoOverflow(State, NewOffset, 4); - } - auto NewPos = Pos->setTo(NewOffset); auto &TgtVal = (Op == OO_PlusEqual || Op == OO_MinusEqual) ? LHS : RetVal; - State = setIteratorPosition(State, TgtVal, NewPos); + State = + setIteratorPosition(State, TgtVal, advancePosition(C, Op, *Pos, *value)); C.addTransition(State); } +void IteratorChecker::verifyIncrement(CheckerContext &C, + const SVal &Iter) const { + auto &BVF = C.getSValBuilder().getBasicValueFactory(); + verifyRandomIncrOrDecr(C, OO_Plus, Iter, + nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(1)))); +} + +void IteratorChecker::verifyDecrement(CheckerContext &C, + const SVal &Iter) const { + auto &BVF = C.getSValBuilder().getBasicValueFactory(); + verifyRandomIncrOrDecr(C, OO_Minus, Iter, + nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(1)))); +} + void IteratorChecker::verifyRandomIncrOrDecr(CheckerContext &C, OverloadedOperatorKind Op, - const SVal &RetVal, const SVal &LHS, const SVal &RHS) const { auto State = C.getState(); // If the iterator is initially inside its range, then the operation is valid const auto *Pos = getIteratorPosition(State, LHS); - if (!Pos || !isOutOfRange(State, *Pos)) + if (!Pos) return; - auto value = RHS; - if (auto loc = RHS.getAs<Loc>()) { - value = State->getRawSVal(*loc); + auto Value = RHS; + if (auto ValAsLoc = RHS.getAs<Loc>()) { + Value = State->getRawSVal(*ValAsLoc); } - // Incremention or decremention by 0 is never bug - if (isZero(State, value.castAs<NonLoc>())) + if (Value.isUnknown()) return; - auto &SymMgr = C.getSymbolManager(); - auto &SVB = C.getSValBuilder(); - auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub; - const auto OldOffset = Pos->getOffset(); - const auto intValue = value.getAs<nonloc::ConcreteInt>(); - if (!intValue) + // Incremention or decremention by 0 is never a bug. + if (isZero(State, Value.castAs<NonLoc>())) return; - auto NewOffset = SVB.evalBinOp(State, BinOp, nonloc::SymbolVal(OldOffset), - *intValue, - SymMgr.getType(OldOffset)).getAsSymbol(); - auto NewPos = Pos->setTo(NewOffset); - - // If out of range, the only valid operation is to step into the range - if (isOutOfRange(State, NewPos)) { + // The result may be the past-end iterator of the container, but any other + // out of range position is undefined behaviour + if (isAheadOfRange(State, advancePosition(C, Op, *Pos, Value))) { auto *N = C.generateNonFatalErrorNode(State); if (!N) return; - reportOutOfRangeBug("Iterator accessed past its end.", LHS, C, N); + reportOutOfRangeBug("Iterator decremented ahead of its valid range.", LHS, + C, N); + } + if (isBehindPastTheEnd(State, advancePosition(C, Op, *Pos, Value))) { + auto *N = C.generateNonFatalErrorNode(State); + if (!N) + return; + reportOutOfRangeBug("Iterator incremented behind the past-the-end " + "iterator.", LHS, C, N); } } void IteratorChecker::verifyMatch(CheckerContext &C, const SVal &Iter, const MemRegion *Cont) const { // Verify match between a container and the container of an iterator - while (const auto *CBOR = Cont->getAs<CXXBaseObjectRegion>()) { - Cont = CBOR->getSuperRegion(); - } + Cont = Cont->getMostDerivedObjectRegion(); auto State = C.getState(); const auto *Pos = getIteratorPosition(State, Iter); @@ -1125,9 +1131,7 @@ void IteratorChecker::handleBegin(CheckerContext &C, const Expr *CE, if (!ContReg) return; - while (const auto *CBOR = ContReg->getAs<CXXBaseObjectRegion>()) { - ContReg = CBOR->getSuperRegion(); - } + ContReg = ContReg->getMostDerivedObjectRegion(); // If the container already has a begin symbol then use it. Otherwise first // create a new one. @@ -1151,9 +1155,7 @@ void IteratorChecker::handleEnd(CheckerContext &C, const Expr *CE, if (!ContReg) return; - while (const auto *CBOR = ContReg->getAs<CXXBaseObjectRegion>()) { - ContReg = CBOR->getSuperRegion(); - } + ContReg = ContReg->getMostDerivedObjectRegion(); // If the container already has an end symbol then use it. Otherwise first // create a new one. @@ -1174,9 +1176,7 @@ void IteratorChecker::handleEnd(CheckerContext &C, const Expr *CE, void IteratorChecker::assignToContainer(CheckerContext &C, const Expr *CE, const SVal &RetVal, const MemRegion *Cont) const { - while (const auto *CBOR = Cont->getAs<CXXBaseObjectRegion>()) { - Cont = CBOR->getSuperRegion(); - } + Cont = Cont->getMostDerivedObjectRegion(); auto State = C.getState(); auto &SymMgr = C.getSymbolManager(); @@ -1194,9 +1194,7 @@ void IteratorChecker::handleAssign(CheckerContext &C, const SVal &Cont, if (!ContReg) return; - while (const auto *CBOR = ContReg->getAs<CXXBaseObjectRegion>()) { - ContReg = CBOR->getSuperRegion(); - } + ContReg = ContReg->getMostDerivedObjectRegion(); // Assignment of a new value to a container always invalidates all its // iterators @@ -1211,13 +1209,11 @@ void IteratorChecker::handleAssign(CheckerContext &C, const SVal &Cont, if (!OldCont.isUndef()) { const auto *OldContReg = OldCont.getAsRegion(); if (OldContReg) { - while (const auto *CBOR = OldContReg->getAs<CXXBaseObjectRegion>()) { - OldContReg = CBOR->getSuperRegion(); - } + OldContReg = OldContReg->getMostDerivedObjectRegion(); const auto OldCData = getContainerData(State, OldContReg); if (OldCData) { if (const auto OldEndSym = OldCData->getEnd()) { - // If we already assigned an "end" symbol to the old conainer, then + // If we already assigned an "end" symbol to the old container, then // first reassign all iterator positions to the new container which // are not past the container (thus not greater or equal to the // current "end" symbol). @@ -1273,9 +1269,7 @@ void IteratorChecker::handleClear(CheckerContext &C, const SVal &Cont) const { if (!ContReg) return; - while (const auto *CBOR = ContReg->getAs<CXXBaseObjectRegion>()) { - ContReg = CBOR->getSuperRegion(); - } + ContReg = ContReg->getMostDerivedObjectRegion(); // The clear() operation invalidates all the iterators, except the past-end // iterators of list-like containers @@ -1302,9 +1296,7 @@ void IteratorChecker::handlePushBack(CheckerContext &C, if (!ContReg) return; - while (const auto *CBOR = ContReg->getAs<CXXBaseObjectRegion>()) { - ContReg = CBOR->getSuperRegion(); - } + ContReg = ContReg->getMostDerivedObjectRegion(); // For deque-like containers invalidate all iterator positions auto State = C.getState(); @@ -1341,9 +1333,7 @@ void IteratorChecker::handlePopBack(CheckerContext &C, const SVal &Cont) const { if (!ContReg) return; - while (const auto *CBOR = ContReg->getAs<CXXBaseObjectRegion>()) { - ContReg = CBOR->getSuperRegion(); - } + ContReg = ContReg->getMostDerivedObjectRegion(); auto State = C.getState(); const auto CData = getContainerData(State, ContReg); @@ -1381,9 +1371,7 @@ void IteratorChecker::handlePushFront(CheckerContext &C, if (!ContReg) return; - while (const auto *CBOR = ContReg->getAs<CXXBaseObjectRegion>()) { - ContReg = CBOR->getSuperRegion(); - } + ContReg = ContReg->getMostDerivedObjectRegion(); // For deque-like containers invalidate all iterator positions auto State = C.getState(); @@ -1416,9 +1404,7 @@ void IteratorChecker::handlePopFront(CheckerContext &C, if (!ContReg) return; - while (const auto *CBOR = ContReg->getAs<CXXBaseObjectRegion>()) { - ContReg = CBOR->getSuperRegion(); - } + ContReg = ContReg->getMostDerivedObjectRegion(); auto State = C.getState(); const auto CData = getContainerData(State, ContReg); @@ -1566,6 +1552,35 @@ void IteratorChecker::handleEraseAfter(CheckerContext &C, const SVal &Iter1, C.addTransition(State); } +IteratorPosition IteratorChecker::advancePosition(CheckerContext &C, + OverloadedOperatorKind Op, + const IteratorPosition &Pos, + const SVal &Distance) const { + auto State = C.getState(); + auto &SymMgr = C.getSymbolManager(); + auto &SVB = C.getSValBuilder(); + + assert ((Op == OO_Plus || Op == OO_PlusEqual || + Op == OO_Minus || Op == OO_MinusEqual) && + "Advance operator must be one of +, -, += and -=."); + auto BinOp = (Op == OO_Plus || Op == OO_PlusEqual) ? BO_Add : BO_Sub; + if (const auto IntDist = Distance.getAs<nonloc::ConcreteInt>()) { + // For concrete integers we can calculate the new position + return Pos.setTo(SVB.evalBinOp(State, BinOp, + nonloc::SymbolVal(Pos.getOffset()), *IntDist, + SymMgr.getType(Pos.getOffset())) + .getAsSymbol()); + } else { + // For other symbols create a new symbol to keep expressions simple + const auto &LCtx = C.getLocationContext(); + const auto NewPosSym = SymMgr.conjureSymbol(nullptr, LCtx, + SymMgr.getType(Pos.getOffset()), + C.blockCount()); + State = assumeNoOverflow(State, NewPosSym, 4); + return Pos.setTo(NewPosSym); + } +} + void IteratorChecker::reportOutOfRangeBug(const StringRef &Message, const SVal &Val, CheckerContext &C, ExplodedNode *ErrNode) const { @@ -1605,7 +1620,8 @@ void IteratorChecker::reportInvalidatedBug(const StringRef &Message, namespace { bool isLess(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2); -bool isGreaterOrEqual(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2); +bool isGreater(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2); +bool isEqual(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2); bool compare(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2, BinaryOperator::Opcode Opc); bool compare(ProgramStateRef State, NonLoc NL1, NonLoc NL2, @@ -2015,7 +2031,8 @@ ProgramStateRef setContainerData(ProgramStateRef State, const MemRegion *Cont, const IteratorPosition *getIteratorPosition(ProgramStateRef State, const SVal &Val) { - if (const auto Reg = Val.getAsRegion()) { + if (auto Reg = Val.getAsRegion()) { + Reg = Reg->getMostDerivedObjectRegion(); return State->get<IteratorRegionMap>(Reg); } else if (const auto Sym = Val.getAsSymbol()) { return State->get<IteratorSymbolMap>(Sym); @@ -2028,7 +2045,8 @@ const IteratorPosition *getIteratorPosition(ProgramStateRef State, const IteratorPosition *getIteratorPosition(ProgramStateRef State, RegionOrSymbol RegOrSym) { if (RegOrSym.is<const MemRegion *>()) { - return State->get<IteratorRegionMap>(RegOrSym.get<const MemRegion *>()); + auto Reg = RegOrSym.get<const MemRegion *>()->getMostDerivedObjectRegion(); + return State->get<IteratorRegionMap>(Reg); } else if (RegOrSym.is<SymbolRef>()) { return State->get<IteratorSymbolMap>(RegOrSym.get<SymbolRef>()); } @@ -2037,7 +2055,8 @@ const IteratorPosition *getIteratorPosition(ProgramStateRef State, ProgramStateRef setIteratorPosition(ProgramStateRef State, const SVal &Val, const IteratorPosition &Pos) { - if (const auto Reg = Val.getAsRegion()) { + if (auto Reg = Val.getAsRegion()) { + Reg = Reg->getMostDerivedObjectRegion(); return State->set<IteratorRegionMap>(Reg, Pos); } else if (const auto Sym = Val.getAsSymbol()) { return State->set<IteratorSymbolMap>(Sym, Pos); @@ -2051,8 +2070,8 @@ ProgramStateRef setIteratorPosition(ProgramStateRef State, RegionOrSymbol RegOrSym, const IteratorPosition &Pos) { if (RegOrSym.is<const MemRegion *>()) { - return State->set<IteratorRegionMap>(RegOrSym.get<const MemRegion *>(), - Pos); + auto Reg = RegOrSym.get<const MemRegion *>()->getMostDerivedObjectRegion(); + return State->set<IteratorRegionMap>(Reg, Pos); } else if (RegOrSym.is<SymbolRef>()) { return State->set<IteratorSymbolMap>(RegOrSym.get<SymbolRef>(), Pos); } @@ -2060,7 +2079,8 @@ ProgramStateRef setIteratorPosition(ProgramStateRef State, } ProgramStateRef removeIteratorPosition(ProgramStateRef State, const SVal &Val) { - if (const auto Reg = Val.getAsRegion()) { + if (auto Reg = Val.getAsRegion()) { + Reg = Reg->getMostDerivedObjectRegion(); return State->remove<IteratorRegionMap>(Reg); } else if (const auto Sym = Val.getAsSymbol()) { return State->remove<IteratorSymbolMap>(Sym); @@ -2294,14 +2314,27 @@ bool isZero(ProgramStateRef State, const NonLoc &Val) { BO_EQ); } -bool isOutOfRange(ProgramStateRef State, const IteratorPosition &Pos) { +bool isPastTheEnd(ProgramStateRef State, const IteratorPosition &Pos) { const auto *Cont = Pos.getContainer(); const auto *CData = getContainerData(State, Cont); if (!CData) return false; - // Out of range means less than the begin symbol or greater or equal to the - // end symbol. + const auto End = CData->getEnd(); + if (End) { + if (isEqual(State, Pos.getOffset(), End)) { + return true; + } + } + + return false; +} + +bool isAheadOfRange(ProgramStateRef State, const IteratorPosition &Pos) { + const auto *Cont = Pos.getContainer(); + const auto *CData = getContainerData(State, Cont); + if (!CData) + return false; const auto Beg = CData->getBegin(); if (Beg) { @@ -2310,9 +2343,18 @@ bool isOutOfRange(ProgramStateRef State, const IteratorPosition &Pos) { } } + return false; +} + +bool isBehindPastTheEnd(ProgramStateRef State, const IteratorPosition &Pos) { + const auto *Cont = Pos.getContainer(); + const auto *CData = getContainerData(State, Cont); + if (!CData) + return false; + const auto End = CData->getEnd(); if (End) { - if (isGreaterOrEqual(State, Pos.getOffset(), End)) { + if (isGreater(State, Pos.getOffset(), End)) { return true; } } @@ -2324,8 +2366,12 @@ bool isLess(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2) { return compare(State, Sym1, Sym2, BO_LT); } -bool isGreaterOrEqual(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2) { - return compare(State, Sym1, Sym2, BO_GE); +bool isGreater(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2) { + return compare(State, Sym1, Sym2, BO_GT); +} + +bool isEqual(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2) { + return compare(State, Sym1, Sym2, BO_EQ); } bool compare(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2, diff --git a/lib/StaticAnalyzer/Checkers/LLVMConventionsChecker.cpp b/lib/StaticAnalyzer/Checkers/LLVMConventionsChecker.cpp index db4fbca36d..18618d0459 100644 --- a/lib/StaticAnalyzer/Checkers/LLVMConventionsChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/LLVMConventionsChecker.cpp @@ -32,8 +32,7 @@ static bool IsLLVMStringRef(QualType T) { if (!RT) return false; - return StringRef(QualType(RT, 0).getAsString()) == - "class StringRef"; + return StringRef(QualType(RT, 0).getAsString()) == "class StringRef"; } /// Check whether the declaration is semantically inside the top-level diff --git a/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp b/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp index 0e51cf1184..103a33d39f 100644 --- a/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp @@ -1398,7 +1398,8 @@ void ento::registerNonLocalizedStringChecker(CheckerManager &mgr) { NonLocalizedStringChecker *checker = mgr.registerChecker<NonLocalizedStringChecker>(); checker->IsAggressive = - mgr.getAnalyzerOptions().getBooleanOption("AggressiveReport", false); + mgr.getAnalyzerOptions().getCheckerBooleanOption("AggressiveReport", + false, checker); } void ento::registerEmptyLocalizationContextChecker(CheckerManager &mgr) { diff --git a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp index 696cf39473..3f89c33cde 100644 --- a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp @@ -100,9 +100,6 @@ void MPIChecker::checkUnmatchedWaits(const CallEvent &PreCallEvent, void MPIChecker::checkMissingWaits(SymbolReaper &SymReaper, CheckerContext &Ctx) const { - if (!SymReaper.hasDeadSymbols()) - return; - ProgramStateRef State = Ctx.getState(); const auto &Requests = State->get<RequestMap>(); if (Requests.isEmpty()) diff --git a/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp b/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp index cc29895e69..f5c7d52f4e 100644 --- a/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp @@ -16,6 +16,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 "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" @@ -29,6 +30,7 @@ namespace { class MacOSKeychainAPIChecker : public Checker<check::PreStmt<CallExpr>, check::PostStmt<CallExpr>, check::DeadSymbols, + check::PointerEscape, eval::Assume> { mutable std::unique_ptr<BugType> BT; @@ -58,6 +60,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 checkPointerEscape(ProgramStateRef State, + const InvalidatedSymbols &Escaped, + const CallEvent *Call, + PointerEscapeKind Kind) const; ProgramStateRef evalAssume(ProgramStateRef state, SVal Cond, bool Assumption) const; void printState(raw_ostream &Out, ProgramStateRef State, @@ -570,6 +576,44 @@ void MacOSKeychainAPIChecker::checkDeadSymbols(SymbolReaper &SR, C.addTransition(State, N); } +ProgramStateRef MacOSKeychainAPIChecker::checkPointerEscape( + ProgramStateRef State, const InvalidatedSymbols &Escaped, + const CallEvent *Call, PointerEscapeKind Kind) const { + // FIXME: This branch doesn't make any sense at all, but it is an overfitted + // replacement for a previous overfitted code that was making even less sense. + if (!Call || Call->getDecl()) + return State; + + for (auto I : State->get<AllocatedData>()) { + SymbolRef Sym = I.first; + if (Escaped.count(Sym)) + State = State->remove<AllocatedData>(Sym); + + // This checker is special. Most checkers in fact only track symbols of + // SymbolConjured type, eg. symbols returned from functions such as + // malloc(). This checker tracks symbols returned as out-parameters. + // + // When a function is evaluated conservatively, the out-parameter's pointee + // base region gets invalidated with a SymbolConjured. If the base region is + // larger than the region we're interested in, the value we're interested in + // would be SymbolDerived based on that SymbolConjured. However, such + // SymbolDerived will never be listed in the Escaped set when the base + // region is invalidated because ExprEngine doesn't know which symbols + // were derived from a given symbol, while there can be infinitely many + // valid symbols derived from any given symbol. + // + // Hence the extra boilerplate: remove the derived symbol when its parent + // symbol escapes. + // + if (const auto *SD = dyn_cast<SymbolDerived>(Sym)) { + SymbolRef ParentSym = SD->getParentSymbol(); + if (Escaped.count(ParentSym)) + State = State->remove<AllocatedData>(Sym); + } + } + return State; +} + std::shared_ptr<PathDiagnosticPiece> MacOSKeychainAPIChecker::SecKeychainBugVisitor::VisitNode( const ExplodedNode *N, BugReporterContext &BRC, BugReport &BR) { diff --git a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 8327b2ef96..8e88fadd37 100644 --- a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -712,10 +712,8 @@ bool MallocChecker::isCMemFunction(const FunctionDecl *FD, return false; } -// Tells if the callee is one of the following: -// 1) A global non-placement new/delete operator function. -// 2) A global placement operator function with the single placement argument -// of type std::nothrow_t. +// Tells if the callee is one of the builtin new/delete operators, including +// placement operators and other standard overloads. bool MallocChecker::isStandardNewDelete(const FunctionDecl *FD, ASTContext &C) const { if (!FD) @@ -726,23 +724,11 @@ bool MallocChecker::isStandardNewDelete(const FunctionDecl *FD, Kind != OO_Delete && Kind != OO_Array_Delete) return false; - // Skip all operator new/delete methods. - if (isa<CXXMethodDecl>(FD)) - return false; - - // Return true if tested operator is a standard placement nothrow operator. - if (FD->getNumParams() == 2) { - QualType T = FD->getParamDecl(1)->getType(); - if (const IdentifierInfo *II = T.getBaseTypeIdentifier()) - return II->getName().equals("nothrow_t"); - } - - // Skip placement operators. - if (FD->getNumParams() != 1 || FD->isVariadic()) - return false; - - // One of the standard new/new[]/delete/delete[] non-placement operators. - return true; + // This is standard if and only if it's not defined in a user file. + SourceLocation L = FD->getLocation(); + // If the header for operator delete is not included, it's still defined + // in an invalid source location. Check to make sure we don't crash. + return !L.isValid() || C.getSourceManager().isInSystemHeader(L); } llvm::Optional<ProgramStateRef> MallocChecker::performKernelMalloc( @@ -1087,12 +1073,6 @@ static bool treatUnusedNewEscaped(const CXXNewExpr *NE) { void MallocChecker::processNewAllocation(const CXXNewExpr *NE, CheckerContext &C, SVal Target) const { - if (NE->getNumPlacementArgs()) - for (CXXNewExpr::const_arg_iterator I = NE->placement_arg_begin(), - E = NE->placement_arg_end(); I != E; ++I) - if (SymbolRef Sym = C.getSVal(*I).getAsSymbol()) - checkUseAfterFree(Sym, C, *I); - if (!isStandardNewDelete(NE->getOperatorNew(), C.getASTContext())) return; @@ -1103,7 +1083,7 @@ void MallocChecker::processNewAllocation(const CXXNewExpr *NE, ProgramStateRef State = C.getState(); // The return value from operator new is bound to a specified initialization // value (if any) and we don't want to loose this value. So we call - // MallocUpdateRefState() instead of MallocMemAux() which breakes the + // MallocUpdateRefState() instead of MallocMemAux() which breaks the // existing binding. State = MallocUpdateRefState(C, NE, State, NE->isArray() ? AF_CXXNewArray : AF_CXXNew, Target); @@ -1114,7 +1094,7 @@ void MallocChecker::processNewAllocation(const CXXNewExpr *NE, void MallocChecker::checkPostStmt(const CXXNewExpr *NE, CheckerContext &C) const { - if (!C.getAnalysisManager().getAnalyzerOptions().mayInlineCXXAllocator()) + if (!C.getAnalysisManager().getAnalyzerOptions().MayInlineCXXAllocator) processNewAllocation(NE, C, C.getSVal(NE)); } @@ -2365,13 +2345,11 @@ void MallocChecker::reportLeak(SymbolRef Sym, ExplodedNode *N, void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const { - if (!SymReaper.hasDeadSymbols()) - return; - ProgramStateRef state = C.getState(); - RegionStateTy RS = state->get<RegionState>(); + RegionStateTy OldRS = state->get<RegionState>(); RegionStateTy::Factory &F = state->get_context<RegionState>(); + RegionStateTy RS = OldRS; SmallVector<SymbolRef, 2> Errors; for (RegionStateTy::iterator I = RS.begin(), E = RS.end(); I != E; ++I) { if (SymReaper.isDead(I->first)) { @@ -2379,10 +2357,18 @@ void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper, Errors.push_back(I->first); // Remove the dead symbol from the map. RS = F.remove(RS, I->first); - } } + if (RS == OldRS) { + // We shouldn't have touched other maps yet. + assert(state->get<ReallocPairs>() == + C.getState()->get<ReallocPairs>()); + assert(state->get<FreeReturnValue>() == + C.getState()->get<FreeReturnValue>()); + return; + } + // Cleanup the Realloc Pairs Map. ReallocPairsTy RP = state->get<ReallocPairs>(); for (ReallocPairsTy::iterator I = RP.begin(), E = RP.end(); I != E; ++I) { @@ -2438,10 +2424,6 @@ void MallocChecker::checkPreCall(const CallEvent &Call, isCMemFunction(FD, Ctx, AF_IfNameIndex, MemoryOperationKind::MOK_Free))) return; - - if (ChecksEnabled[CK_NewDeleteChecker] && - isStandardNewDelete(FD, Ctx)) - return; } // Check if the callee of a method is deleted. @@ -2539,8 +2521,7 @@ void MallocChecker::checkPostStmt(const BlockExpr *BE, } state = - state->scanReachableSymbols<StopTrackingCallback>(Regions.data(), - Regions.data() + Regions.size()).getState(); + state->scanReachableSymbols<StopTrackingCallback>(Regions).getState(); C.addTransition(state); } @@ -3109,7 +3090,7 @@ markReleased(ProgramStateRef State, SymbolRef Sym, const Expr *Origin) { void ento::registerNewDeleteLeaksChecker(CheckerManager &mgr) { registerCStringCheckerBasic(mgr); MallocChecker *checker = mgr.registerChecker<MallocChecker>(); - checker->IsOptimistic = mgr.getAnalyzerOptions().getBooleanOption( + checker->IsOptimistic = mgr.getAnalyzerOptions().getCheckerBooleanOption( "Optimistic", false, checker); checker->ChecksEnabled[MallocChecker::CK_NewDeleteLeaksChecker] = true; checker->CheckNames[MallocChecker::CK_NewDeleteLeaksChecker] = @@ -3130,7 +3111,7 @@ void ento::registerNewDeleteLeaksChecker(CheckerManager &mgr) { void ento::registerInnerPointerCheckerAux(CheckerManager &mgr) { registerCStringCheckerBasic(mgr); MallocChecker *checker = mgr.registerChecker<MallocChecker>(); - checker->IsOptimistic = mgr.getAnalyzerOptions().getBooleanOption( + checker->IsOptimistic = mgr.getAnalyzerOptions().getCheckerBooleanOption( "Optimistic", false, checker); checker->ChecksEnabled[MallocChecker::CK_InnerPointerChecker] = true; checker->CheckNames[MallocChecker::CK_InnerPointerChecker] = @@ -3141,7 +3122,7 @@ void ento::registerInnerPointerCheckerAux(CheckerManager &mgr) { void ento::register##name(CheckerManager &mgr) { \ registerCStringCheckerBasic(mgr); \ MallocChecker *checker = mgr.registerChecker<MallocChecker>(); \ - checker->IsOptimistic = mgr.getAnalyzerOptions().getBooleanOption( \ + checker->IsOptimistic = mgr.getAnalyzerOptions().getCheckerBooleanOption( \ "Optimistic", false, checker); \ checker->ChecksEnabled[MallocChecker::CK_##name] = true; \ checker->CheckNames[MallocChecker::CK_##name] = mgr.getCurrentCheckName(); \ diff --git a/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp b/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp index fc2ab1d6e3..4e45a37fd8 100644 --- a/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MallocOverflowSecurityChecker.cpp @@ -135,9 +135,9 @@ private: bool isIntZeroExpr(const Expr *E) const { if (!E->getType()->isIntegralOrEnumerationType()) return false; - llvm::APSInt Result; + Expr::EvalResult Result; if (E->EvaluateAsInt(Result, Context)) - return Result == 0; + return Result.Val.getInt() == 0; return false; } @@ -191,8 +191,11 @@ private: if (const BinaryOperator *BOp = dyn_cast<BinaryOperator>(rhse)) { if (BOp->getOpcode() == BO_Div) { const Expr *denom = BOp->getRHS()->IgnoreParenImpCasts(); - if (denom->EvaluateAsInt(denomVal, Context)) + Expr::EvalResult Result; + if (denom->EvaluateAsInt(Result, Context)) { + denomVal = Result.Val.getInt(); denomKnown = true; + } const Expr *numerator = BOp->getLHS()->IgnoreParenImpCasts(); if (numerator->isEvaluatable(Context)) numeratorKnown = true; diff --git a/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp b/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp index 5060b0e0a6..0d63cfe937 100644 --- a/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp @@ -82,7 +82,9 @@ void ento::registerMmapWriteExecChecker(CheckerManager &mgr) { MmapWriteExecChecker *Mwec = mgr.registerChecker<MmapWriteExecChecker>(); Mwec->ProtExecOv = - mgr.getAnalyzerOptions().getOptionAsInteger("MmapProtExec", 0x04, Mwec); + mgr.getAnalyzerOptions() + .getCheckerIntegerOption("MmapProtExec", 0x04, Mwec); Mwec->ProtReadOv = - mgr.getAnalyzerOptions().getOptionAsInteger("MmapProtRead", 0x01, Mwec); + mgr.getAnalyzerOptions() + .getCheckerIntegerOption("MmapProtRead", 0x01, Mwec); } diff --git a/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp b/lib/StaticAnalyzer/Checkers/MoveChecker.cpp index 83037f0444..dd83ce02e0 100644 --- a/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MoveChecker.cpp @@ -1,4 +1,4 @@ -// MisusedMovedObjectChecker.cpp - Check use of moved-from objects. - C++ -===// +// MoveChecker.cpp - Check use of moved-from objects. - C++ ---------------===// // // The LLVM Compiler Infrastructure // @@ -20,6 +20,7 @@ #include "clang/StaticAnalyzer/Core/CheckerManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "llvm/ADT/StringSet.h" using namespace clang; using namespace ento; @@ -42,8 +43,8 @@ public: void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddInteger(K); } }; -class MisusedMovedObjectChecker - : public Checker<check::PreCall, check::PostCall, check::EndFunction, +class MoveChecker + : public Checker<check::PreCall, check::PostCall, check::DeadSymbols, check::RegionChanges> { public: void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const; @@ -53,22 +54,66 @@ public: ProgramStateRef checkRegionChanges(ProgramStateRef State, const InvalidatedSymbols *Invalidated, - ArrayRef<const MemRegion *> ExplicitRegions, - ArrayRef<const MemRegion *> Regions, + ArrayRef<const MemRegion *> RequestedRegions, + ArrayRef<const MemRegion *> InvalidatedRegions, 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}; + enum MisuseKind { MK_FunCall, MK_Copy, MK_Move }; + + struct ObjectKind { + bool Local : 1; // Is this a local variable or a local rvalue reference? + bool STL : 1; // Is this an object of a standard type? + }; + + // Not all of these are entirely move-safe, but they do provide *some* + // guarantees, and it means that somebody is using them after move + // in a valid manner. + // TODO: We can still try to identify *unsafe* use after move, such as + // dereference of a moved-from smart pointer (which is guaranteed to be null). + const llvm::StringSet<> StandardMoveSafeClasses = { + "basic_filebuf", + "basic_ios", + "future", + "optional", + "packaged_task" + "promise", + "shared_future", + "shared_lock", + "shared_ptr", + "thread", + "unique_ptr", + "unique_lock", + "weak_ptr", + }; + + // Obtains ObjectKind of an object. Because class declaration cannot always + // be easily obtained from the memory region, it is supplied separately. + ObjectKind classifyObject(const MemRegion *MR, const CXXRecordDecl *RD) const; + + // Classifies the object and dumps a user-friendly description string to + // the stream. Return value is equivalent to classifyObject. + ObjectKind explainObject(llvm::raw_ostream &OS, + const MemRegion *MR, const CXXRecordDecl *RD) const; + + bool isStandardMoveSafeClass(const CXXRecordDecl *RD) const; + class MovedBugVisitor : public BugReporterVisitor { public: - MovedBugVisitor(const MemRegion *R) : Region(R), Found(false) {} + MovedBugVisitor(const MoveChecker &Chk, + const MemRegion *R, const CXXRecordDecl *RD) + : Chk(Chk), Region(R), RD(RD), Found(false) {} void Profile(llvm::FoldingSetNodeID &ID) const override { static int X = 0; ID.AddPointer(&X); ID.AddPointer(Region); + // Don't add RD because it's, in theory, uniquely determined by + // the region. In practice though, it's not always possible to obtain + // the declaration directly from the region, that's why we store it + // in the first place. } std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N, @@ -76,13 +121,22 @@ private: BugReport &BR) override; private: + const MoveChecker &Chk; // The tracked region. const MemRegion *Region; + // The class of the tracked object. + const CXXRecordDecl *RD; bool Found; }; + bool IsAggressive = false; + +public: + void setAggressiveness(bool Aggressive) { IsAggressive = Aggressive; } + +private: mutable std::unique_ptr<BugType> BT; - ExplodedNode *reportBug(const MemRegion *Region, const CallEvent &Call, + ExplodedNode *reportBug(const MemRegion *Region, const CXXRecordDecl *RD, CheckerContext &C, MisuseKind MK) const; bool isInMoveSafeContext(const LocationContext *LC) const; bool isStateResetMethod(const CXXMethodDecl *MethodDec) const; @@ -116,10 +170,19 @@ static bool isAnyBaseRegionReported(ProgramStateRef State, return false; } +static const MemRegion *unwrapRValueReferenceIndirection(const MemRegion *MR) { + if (const auto *SR = dyn_cast_or_null<SymbolicRegion>(MR)) { + SymbolRef Sym = SR->getSymbol(); + if (Sym->getType()->isRValueReferenceType()) + if (const MemRegion *OriginMR = Sym->getOriginRegion()) + return OriginMR; + } + return MR; +} + std::shared_ptr<PathDiagnosticPiece> -MisusedMovedObjectChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N, - BugReporterContext &BRC, - BugReport &) { +MoveChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N, + BugReporterContext &BRC, BugReport &BR) { // We need only the last move of the reported object's region. // The visitor walks the ExplodedGraph backwards. if (Found) @@ -140,25 +203,25 @@ MisusedMovedObjectChecker::MovedBugVisitor::VisitNode(const ExplodedNode *N, 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"; + SmallString<128> Str; + llvm::raw_svector_ostream OS(Str); + + OS << "Object"; + ObjectKind OK = Chk.explainObject(OS, Region, RD); + if (OK.STL) + OS << " is left in a valid but unspecified state after move"; else - InfoText = "Became 'moved-from' here"; + OS << " is moved"; // Generate the extra diagnostic. PathDiagnosticLocation Pos(S, BRC.getSourceManager(), N->getLocationContext()); - return std::make_shared<PathDiagnosticEventPiece>(Pos, InfoText, true); -} + return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true); + } -const ExplodedNode *MisusedMovedObjectChecker::getMoveLocation( - const ExplodedNode *N, const MemRegion *Region, CheckerContext &C) const { +const ExplodedNode *MoveChecker::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; @@ -173,13 +236,13 @@ const ExplodedNode *MisusedMovedObjectChecker::getMoveLocation( return MoveNode; } -ExplodedNode *MisusedMovedObjectChecker::reportBug(const MemRegion *Region, - const CallEvent &Call, - CheckerContext &C, - MisuseKind MK) const { +ExplodedNode *MoveChecker::reportBug(const MemRegion *Region, + const CXXRecordDecl *RD, + CheckerContext &C, + MisuseKind MK) const { if (ExplodedNode *N = C.generateNonFatalErrorNode()) { if (!BT) - BT.reset(new BugType(this, "Usage of a 'moved-from' object", + BT.reset(new BugType(this, "Use-after-move", "C++ move semantics")); // Uniqueing report to the same object. @@ -191,71 +254,37 @@ ExplodedNode *MisusedMovedObjectChecker::reportBug(const MemRegion *Region, MoveStmt, C.getSourceManager(), MoveNode->getLocationContext()); // Creating the error message. - std::string ErrorMessage; + llvm::SmallString<128> Str; + llvm::raw_svector_ostream OS(Str); switch(MK) { case MK_FunCall: - ErrorMessage = "Method call on a 'moved-from' object"; + OS << "Method called on moved-from object"; + explainObject(OS, Region, RD); break; case MK_Copy: - ErrorMessage = "Copying a 'moved-from' object"; + OS << "Moved-from object"; + explainObject(OS, Region, RD); + OS << " is copied"; break; case MK_Move: - ErrorMessage = "Moving a 'moved-from' object"; + OS << "Moved-from object"; + explainObject(OS, Region, RD); + OS << " is moved"; break; } - 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, + llvm::make_unique<BugReport>(*BT, OS.str(), N, LocUsedForUniqueing, MoveNode->getLocationContext()->getDecl()); - R->addVisitor(llvm::make_unique<MovedBugVisitor>(Region)); + R->addVisitor(llvm::make_unique<MovedBugVisitor>(*this, Region, RD)); 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(const ReturnStmt *RS, - 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 { +void MoveChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { const auto *AFC = dyn_cast<AnyFunctionCall>(&Call); if (!AFC) return; @@ -281,6 +310,20 @@ void MisusedMovedObjectChecker::checkPostCall(const CallEvent &Call, if (!ArgRegion) return; + // In non-aggressive mode, only warn on use-after-move of local variables (or + // local rvalue references) and of STL objects. The former is possible because + // local variables (or local rvalue references) are not tempting their user to + // re-use the storage. The latter is possible because STL objects are known + // to end up in a valid but unspecified state after the move and their + // state-reset methods are also known, which allows us to predict + // precisely when use-after-move is invalid. + // In aggressive mode, warn on any use-after-move because the user + // has intentionally asked us to completely eliminate use-after-move + // in his code. + ObjectKind OK = classifyObject(ArgRegion, MethodDecl->getParent()); + if (!IsAggressive && !OK.Local && !OK.STL) + return; + // Skip moving the object to itself. if (CC && CC->getCXXThisVal().getAsRegion() == ArgRegion) return; @@ -302,8 +345,7 @@ void MisusedMovedObjectChecker::checkPostCall(const CallEvent &Call, C.addTransition(State); } -bool MisusedMovedObjectChecker::isMoveSafeMethod( - const CXXMethodDecl *MethodDec) const { +bool MoveChecker::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)) { @@ -314,20 +356,23 @@ bool MisusedMovedObjectChecker::isMoveSafeMethod( return true; } // Function call `empty` can be skipped. - if (MethodDec && MethodDec->getDeclName().isIdentifier() && + return (MethodDec && MethodDec->getDeclName().isIdentifier() && (MethodDec->getName().lower() == "empty" || - MethodDec->getName().lower() == "isempty")) - return true; - - return false; + MethodDec->getName().lower() == "isempty")); } -bool MisusedMovedObjectChecker::isStateResetMethod( - const CXXMethodDecl *MethodDec) const { - if (MethodDec && MethodDec->getDeclName().isIdentifier()) { +bool MoveChecker::isStateResetMethod(const CXXMethodDecl *MethodDec) const { + if (!MethodDec) + return false; + if (MethodDec->hasAttr<ReinitializesAttr>()) + return true; + if (MethodDec->getDeclName().isIdentifier()) { std::string MethodName = MethodDec->getName().lower(); + // TODO: Some of these methods (eg., resize) are not always resetting + // the state, so we should consider looking at the arguments. if (MethodName == "reset" || MethodName == "clear" || - MethodName == "destroy") + MethodName == "destroy" || MethodName == "resize" || + MethodName == "shrink") return true; } return false; @@ -335,8 +380,7 @@ bool MisusedMovedObjectChecker::isStateResetMethod( // 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 { +bool MoveChecker::isInMoveSafeContext(const LocationContext *LC) const { do { const auto *CtxDec = LC->getDecl(); auto *CtorDec = dyn_cast_or_null<CXXConstructorDecl>(CtxDec); @@ -351,8 +395,45 @@ bool MisusedMovedObjectChecker::isInMoveSafeContext( return false; } -void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call, - CheckerContext &C) const { +bool MoveChecker::isStandardMoveSafeClass(const CXXRecordDecl *RD) const { + const IdentifierInfo *II = RD->getIdentifier(); + return II && StandardMoveSafeClasses.count(II->getName()); +} + +MoveChecker::ObjectKind +MoveChecker::classifyObject(const MemRegion *MR, + const CXXRecordDecl *RD) const { + // Local variables and local rvalue references are classified as "Local". + // For the purposes of this checker, we classify move-safe STL types + // as not-"STL" types, because that's how the checker treats them. + MR = unwrapRValueReferenceIndirection(MR); + return { + /*Local=*/ + MR && isa<VarRegion>(MR) && isa<StackSpaceRegion>(MR->getMemorySpace()), + /*STL=*/ + RD && RD->getDeclContext()->isStdNamespace() && + !isStandardMoveSafeClass(RD) + }; +} + +MoveChecker::ObjectKind +MoveChecker::explainObject(llvm::raw_ostream &OS, const MemRegion *MR, + const CXXRecordDecl *RD) const { + // We may need a leading space every time we actually explain anything, + // and we never know if we are to explain anything until we try. + if (const auto DR = + dyn_cast_or_null<DeclRegion>(unwrapRValueReferenceIndirection(MR))) { + const auto *RegionDecl = cast<NamedDecl>(DR->getDecl()); + OS << " '" << RegionDecl->getNameAsString() << "'"; + } + ObjectKind OK = classifyObject(MR, RD); + if (OK.STL) { + OS << " of type '" << RD->getQualifiedNameAsString() << "'"; + } + return OK; +} + +void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { ProgramStateRef State = C.getState(); const LocationContext *LC = C.getLocationContext(); ExplodedNode *N = nullptr; @@ -370,10 +451,11 @@ void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call, const RegionState *ArgState = State->get<TrackedRegionMap>(ArgRegion); if (ArgState && ArgState->isMoved()) { if (!isInMoveSafeContext(LC)) { + const CXXRecordDecl *RD = CtorDec->getParent(); if(CtorDec->isMoveConstructor()) - N = reportBug(ArgRegion, Call, C, MK_Move); + N = reportBug(ArgRegion, RD, C, MK_Move); else - N = reportBug(ArgRegion, Call, C, MK_Copy); + N = reportBug(ArgRegion, RD, C, MK_Copy); State = State->set<TrackedRegionMap>(ArgRegion, RegionState::getReported()); } @@ -386,20 +468,22 @@ void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call, 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 (!ThisRegion) + + // Calling a destructor on a moved object is fine. + if (isa<CXXDestructorCall>(IC)) return; - if (dyn_cast_or_null<CXXDestructorDecl>(Call.getDecl())) { - State = removeFromState(State, ThisRegion); - C.addTransition(State); + const MemRegion *ThisRegion = IC->getCXXThisVal().getAsRegion(); + if (!ThisRegion) return; - } const auto MethodDecl = dyn_cast_or_null<CXXMethodDecl>(IC->getDecl()); if (!MethodDecl) return; + + // Store class declaration as well, for bug reporting purposes. + const CXXRecordDecl *RD = MethodDecl->getParent(); + // Checking assignment operators. bool OperatorEq = MethodDecl->isOverloadedOperator() && MethodDecl->getOverloadedOperator() == OO_Equal; @@ -414,9 +498,9 @@ void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call, if (ArgState && ArgState->isMoved() && !isInMoveSafeContext(LC)) { const MemRegion *ArgRegion = IC->getArgSVal(0).getAsRegion(); if(MethodDecl->isMoveAssignmentOperator()) - N = reportBug(ArgRegion, Call, C, MK_Move); + N = reportBug(ArgRegion, RD, C, MK_Move); else - N = reportBug(ArgRegion, Call, C, MK_Copy); + N = reportBug(ArgRegion, RD, C, MK_Copy); State = State->set<TrackedRegionMap>(ArgRegion, RegionState::getReported()); } @@ -429,8 +513,7 @@ void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call, // 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)) + while (const auto *BR = dyn_cast<CXXBaseObjectRegion>(ThisRegion)) ThisRegion = BR->getSuperRegion(); if (isMoveSafeMethod(MethodDecl)) @@ -454,13 +537,13 @@ void MisusedMovedObjectChecker::checkPreCall(const CallEvent &Call, if (isInMoveSafeContext(LC)) return; - N = reportBug(ThisRegion, Call, C, MK_FunCall); + N = reportBug(ThisRegion, RD, C, MK_FunCall); State = State->set<TrackedRegionMap>(ThisRegion, RegionState::getReported()); C.addTransition(State, N); } -void MisusedMovedObjectChecker::checkDeadSymbols(SymbolReaper &SymReaper, - CheckerContext &C) const { +void MoveChecker::checkDeadSymbols(SymbolReaper &SymReaper, + CheckerContext &C) const { ProgramStateRef State = C.getState(); TrackedRegionMapTy TrackedRegions = State->get<TrackedRegionMap>(); for (TrackedRegionMapTy::value_type E : TrackedRegions) { @@ -475,34 +558,44 @@ void MisusedMovedObjectChecker::checkDeadSymbols(SymbolReaper &SymReaper, C.addTransition(State); } -ProgramStateRef MisusedMovedObjectChecker::checkRegionChanges( +ProgramStateRef MoveChecker::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); + ArrayRef<const MemRegion *> RequestedRegions, + ArrayRef<const MemRegion *> InvalidatedRegions, + const LocationContext *LCtx, const CallEvent *Call) const { + if (Call) { + // Relax invalidation upon function calls: only invalidate parameters + // that are passed directly via non-const pointers or non-const references + // or rvalue references. + // In case of an InstanceCall don't invalidate the this-region since + // it is fully handled in checkPreCall and checkPostCall. + const MemRegion *ThisRegion = nullptr; + if (const auto *IC = dyn_cast<CXXInstanceCall>(Call)) + ThisRegion = IC->getCXXThisVal().getAsRegion(); + + // Requested ("explicit") regions are the regions passed into the call + // directly, but not all of them end up being invalidated. + // But when they do, they appear in the InvalidatedRegions array as well. + for (const auto *Region : RequestedRegions) { + if (ThisRegion != Region) { + if (llvm::find(InvalidatedRegions, Region) != + std::end(InvalidatedRegions)) { + State = removeFromState(State, Region); + } + } } + } else { + // For invalidations that aren't caused by calls, assume nothing. In + // particular, direct write into an object's field invalidates the status. + for (const auto *Region : InvalidatedRegions) + State = removeFromState(State, Region->getBaseRegion()); } return State; } -void MisusedMovedObjectChecker::printState(raw_ostream &Out, - ProgramStateRef State, - const char *NL, - const char *Sep) const { +void MoveChecker::printState(raw_ostream &Out, ProgramStateRef State, + const char *NL, const char *Sep) const { TrackedRegionMapTy RS = State->get<TrackedRegionMap>(); @@ -518,6 +611,8 @@ void MisusedMovedObjectChecker::printState(raw_ostream &Out, } } } -void ento::registerMisusedMovedObjectChecker(CheckerManager &mgr) { - mgr.registerChecker<MisusedMovedObjectChecker>(); +void ento::registerMoveChecker(CheckerManager &mgr) { + MoveChecker *chk = mgr.registerChecker<MoveChecker>(); + chk->setAggressiveness(mgr.getAnalyzerOptions().getCheckerBooleanOption( + "Aggressive", false, chk)); } diff --git a/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp b/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp index 01d2c0491b..a97eab4e82 100644 --- a/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp @@ -192,7 +192,7 @@ NonNullParamChecker::genReportNullAttrNonNull(const ExplodedNode *ErrorNode, *BTAttrNonNull, "Null pointer passed as an argument to a 'nonnull' parameter", ErrorNode); if (ArgE) - bugreporter::trackNullOrUndefValue(ErrorNode, ArgE, *R); + bugreporter::trackExpressionValue(ErrorNode, ArgE, *R); return R; } @@ -208,9 +208,7 @@ std::unique_ptr<BugReport> NonNullParamChecker::genReportReferenceToNullPointer( const Expr *ArgEDeref = bugreporter::getDerefExpr(ArgE); if (!ArgEDeref) ArgEDeref = ArgE; - bugreporter::trackNullOrUndefValue(ErrorNode, - ArgEDeref, - *R); + bugreporter::trackExpressionValue(ErrorNode, ArgEDeref, *R); } return R; diff --git a/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp index 86ae9cb666..ce656d5201 100644 --- a/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp @@ -174,7 +174,8 @@ private: if (Error == ErrorKind::NilAssignedToNonnull || Error == ErrorKind::NilPassedToNonnull || Error == ErrorKind::NilReturnedToNonnull) - bugreporter::trackNullOrUndefValue(N, ValueExpr, *R); + if (const auto *Ex = dyn_cast<Expr>(ValueExpr)) + bugreporter::trackExpressionValue(N, Ex, *R); } BR.emitReport(std::move(R)); } @@ -184,7 +185,7 @@ private: const SymbolicRegion *getTrackRegion(SVal Val, bool CheckSuperRegion = false) const; - /// Returns true if the call is diagnosable in the currrent analyzer + /// Returns true if the call is diagnosable in the current analyzer /// configuration. bool isDiagnosableCall(const CallEvent &Call) const { if (NoDiagnoseCallsToSystemHeaders && Call.isInSystemHeader()) @@ -328,8 +329,8 @@ NullabilityChecker::NullabilityBugVisitor::VisitNode(const ExplodedNode *N, nullptr); } -/// Returns true when the value stored at the given location is null -/// and the passed in type is nonnnull. +/// Returns true when the value stored at the given location has been +/// constrained to null after being passed through an object of nonnnull type. static bool checkValueAtLValForInvariantViolation(ProgramStateRef State, SVal LV, QualType T) { if (getNullabilityAnnotation(T) != Nullability::Nonnull) @@ -339,9 +340,14 @@ static bool checkValueAtLValForInvariantViolation(ProgramStateRef State, if (!RegionVal) return false; - auto StoredVal = - State->getSVal(RegionVal->getRegion()).getAs<DefinedOrUnknownSVal>(); - if (!StoredVal) + // If the value was constrained to null *after* it was passed through that + // location, it could not have been a concrete pointer *when* it was passed. + // In that case we would have handled the situation when the value was + // bound to that location, by emitting (or not emitting) a report. + // Therefore we are only interested in symbolic regions that can be either + // null or non-null depending on the value of their respective symbol. + auto StoredVal = State->getSVal(*RegionVal).getAs<loc::MemRegionVal>(); + if (!StoredVal || !isa<SymbolicRegion>(StoredVal->getRegion())) return false; if (getNullConstraint(*StoredVal, State) == NullConstraint::IsNull) @@ -445,9 +451,6 @@ void NullabilityChecker::reportBugIfInvariantHolds(StringRef Msg, /// Cleaning up the program state. void NullabilityChecker::checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const { - if (!SR.hasDeadSymbols()) - return; - ProgramStateRef State = C.getState(); NullabilityMapTy Nullabilities = State->get<NullabilityMap>(); for (NullabilityMapTy::iterator I = Nullabilities.begin(), @@ -1172,10 +1175,15 @@ void NullabilityChecker::printState(raw_ostream &Out, ProgramStateRef State, NullabilityMapTy B = State->get<NullabilityMap>(); + if (State->get<InvariantViolated>()) + Out << Sep << NL + << "Nullability invariant was violated, warnings suppressed." << NL; + if (B.isEmpty()) return; - Out << Sep << NL; + if (!State->get<InvariantViolated>()) + Out << Sep << NL; for (NullabilityMapTy::iterator I = B.begin(), E = B.end(); I != E; ++I) { Out << I->first << " : "; @@ -1192,7 +1200,7 @@ void NullabilityChecker::printState(raw_ostream &Out, ProgramStateRef State, checker->NeedTracking = checker->NeedTracking || trackingRequired; \ checker->NoDiagnoseCallsToSystemHeaders = \ checker->NoDiagnoseCallsToSystemHeaders || \ - mgr.getAnalyzerOptions().getBooleanOption( \ + mgr.getAnalyzerOptions().getCheckerBooleanOption( \ "NoDiagnoseCallsToSystemHeaders", false, checker, true); \ } diff --git a/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp b/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp index 56b42239d7..f808739347 100644 --- a/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp @@ -87,9 +87,10 @@ void Callback::run(const MatchFinder::MatchResult &Result) { MacroIndicatesWeShouldSkipTheCheck = true; } if (!MacroIndicatesWeShouldSkipTheCheck) { - llvm::APSInt Result; + Expr::EvalResult EVResult; if (CheckIfNull->IgnoreParenCasts()->EvaluateAsInt( - Result, ACtx, Expr::SE_AllowSideEffects)) { + EVResult, ACtx, Expr::SE_AllowSideEffects)) { + llvm::APSInt Result = EVResult.Val.getInt(); if (Result == 0) { if (!C->Pedantic) return; @@ -346,5 +347,5 @@ void ento::registerNumberObjectConversionChecker(CheckerManager &Mgr) { NumberObjectConversionChecker *Chk = Mgr.registerChecker<NumberObjectConversionChecker>(); Chk->Pedantic = - Mgr.getAnalyzerOptions().getBooleanOption("Pedantic", false, Chk); + Mgr.getAnalyzerOptions().getCheckerBooleanOption("Pedantic", false, Chk); } diff --git a/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp index b7339fe79f..f56a795636 100644 --- a/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp @@ -49,7 +49,7 @@ void ObjCAtSyncChecker::checkPreStmt(const ObjCAtSynchronizedStmt *S, "for @synchronized")); auto report = llvm::make_unique<BugReport>(*BT_undef, BT_undef->getDescription(), N); - bugreporter::trackNullOrUndefValue(N, Ex, *report); + bugreporter::trackExpressionValue(N, Ex, *report); C.emitReport(std::move(report)); } return; @@ -73,7 +73,7 @@ void ObjCAtSyncChecker::checkPreStmt(const ObjCAtSynchronizedStmt *S, "(no synchronization will occur)")); auto report = llvm::make_unique<BugReport>(*BT_null, BT_null->getDescription(), N); - bugreporter::trackNullOrUndefValue(N, Ex, *report); + bugreporter::trackExpressionValue(N, Ex, *report); C.emitReport(std::move(report)); return; @@ -89,6 +89,6 @@ void ObjCAtSyncChecker::checkPreStmt(const ObjCAtSynchronizedStmt *S, } void ento::registerObjCAtSyncChecker(CheckerManager &mgr) { - if (mgr.getLangOpts().ObjC2) + if (mgr.getLangOpts().ObjC) mgr.registerChecker<ObjCAtSyncChecker>(); } diff --git a/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp b/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp index b1255243be..dc361ad537 100644 --- a/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp @@ -41,7 +41,8 @@ public: BugReporter &BRArg) const { BR = &BRArg; AllowedPad = - MGR.getAnalyzerOptions().getOptionAsInteger("AllowedPad", 24, this); + MGR.getAnalyzerOptions() + .getCheckerIntegerOption("AllowedPad", 24, this); assert(AllowedPad >= 0 && "AllowedPad option should be non-negative"); // The calls to checkAST* from AnalysisConsumer don't @@ -75,6 +76,20 @@ public: if (shouldSkipDecl(RD)) return; + // TODO: Figure out why we are going through declarations and not only + // definitions. + if (!(RD = RD->getDefinition())) + return; + + // This is the simplest correct case: a class with no fields and one base + // class. Other cases are more complicated because of how the base classes + // & fields might interact, so we don't bother dealing with them. + // TODO: Support other combinations of base classes and fields. + if (auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) + if (CXXRD->field_empty() && CXXRD->getNumBases() == 1) + return visitRecord(CXXRD->bases().begin()->getType()->getAsRecordDecl(), + PadMultiplier); + auto &ASTContext = RD->getASTContext(); const ASTRecordLayout &RL = ASTContext.getASTRecordLayout(RD); assert(llvm::isPowerOf2_64(RL.getAlignment().getQuantity())); @@ -112,12 +127,15 @@ public: if (RT == nullptr) return; - // TODO: Recurse into the fields and base classes to see if any - // of those have excess padding. + // TODO: Recurse into the fields to see if they have excess padding. visitRecord(RT->getDecl(), Elts); } bool shouldSkipDecl(const RecordDecl *RD) const { + // TODO: Figure out why we are going through declarations and not only + // definitions. + if (!(RD = RD->getDefinition())) + return true; auto Location = RD->getLocation(); // If the construct doesn't have a source file, then it's not something // we want to diagnose. @@ -132,13 +150,14 @@ public: // Not going to attempt to optimize unions. if (RD->isUnion()) return true; - // How do you reorder fields if you haven't got any? - if (RD->field_empty()) - return true; if (auto *CXXRD = dyn_cast<CXXRecordDecl>(RD)) { // Tail padding with base classes ends up being very complicated. - // We will skip objects with base classes for now. - if (CXXRD->getNumBases() != 0) + // We will skip objects with base classes for now, unless they do not + // have fields. + // TODO: Handle more base class scenarios. + if (!CXXRD->field_empty() && CXXRD->getNumBases() != 0) + return true; + if (CXXRD->field_empty() && CXXRD->getNumBases() != 1) return true; // Virtual bases are complicated, skipping those for now. if (CXXRD->getNumVBases() != 0) @@ -150,6 +169,10 @@ public: if (CXXRD->getTypeForDecl()->isInstantiationDependentType()) return true; } + // How do you reorder fields if you haven't got any? + else if (RD->field_empty()) + return true; + auto IsTrickyField = [](const FieldDecl *FD) -> bool { // Bitfield layout is hard. if (FD->isBitField()) @@ -323,7 +346,7 @@ public: BR->emitReport(std::move(Report)); } }; -} +} // namespace void ento::registerPaddingChecker(CheckerManager &Mgr) { Mgr.registerChecker<PaddingChecker>(); diff --git a/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp b/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp index 63f82b275b..af242845f0 100644 --- a/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp @@ -112,7 +112,7 @@ PointerArithChecker::getPointedRegion(const MemRegion *Region, } /// Checks whether a region is the part of an array. -/// In case there is a dericed to base cast above the array element, the +/// In case there is a derived to base cast above the array element, the /// Polymorphic output value is set to true. AKind output value is set to the /// allocation kind of the inspected region. const MemRegion *PointerArithChecker::getArrayRegion(const MemRegion *Region, diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp index e5d27f577d..488cf6d3eb 100644 --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp @@ -39,13 +39,76 @@ ProgramStateRef removeRefBinding(ProgramStateRef State, SymbolRef Sym) { return State->remove<RefBindings>(Sym); } +class UseAfterRelease : public CFRefBug { +public: + UseAfterRelease(const CheckerBase *checker) + : CFRefBug(checker, "Use-after-release") {} + + const char *getDescription() const override { + return "Reference-counted object is used after it is released"; + } +}; + +class BadRelease : public CFRefBug { +public: + BadRelease(const CheckerBase *checker) : CFRefBug(checker, "Bad release") {} + + const char *getDescription() const override { + return "Incorrect decrement of the reference count of an object that is " + "not owned at this point by the caller"; + } +}; + +class DeallocNotOwned : public CFRefBug { +public: + DeallocNotOwned(const CheckerBase *checker) + : CFRefBug(checker, "-dealloc sent to non-exclusively owned object") {} + + const char *getDescription() const override { + return "-dealloc sent to object that may be referenced elsewhere"; + } +}; + +class OverAutorelease : public CFRefBug { +public: + OverAutorelease(const CheckerBase *checker) + : CFRefBug(checker, "Object autoreleased too many times") {} + + const char *getDescription() const override { + return "Object autoreleased too many times"; + } +}; + +class ReturnedNotOwnedForOwned : public CFRefBug { +public: + ReturnedNotOwnedForOwned(const CheckerBase *checker) + : CFRefBug(checker, "Method should return an owned object") {} + + const char *getDescription() const override { + return "Object with a +0 retain count returned to caller where a +1 " + "(owning) retain count is expected"; + } +}; + +class Leak : public CFRefBug { +public: + Leak(const CheckerBase *checker, StringRef name) : CFRefBug(checker, name) { + // Leaks should not be reported if they are post-dominated by a sink. + setSuppressOnSink(true); + } + + const char *getDescription() const override { return ""; } + + bool isLeak() const override { return true; } +}; + } // end namespace retaincountchecker } // end namespace ento } // end namespace clang void RefVal::print(raw_ostream &Out) const { if (!T.isNull()) - Out << "Tracked " << T.getAsString() << '/'; + Out << "Tracked " << T.getAsString() << " | "; switch (getKind()) { default: llvm_unreachable("Invalid RefVal kind"); @@ -175,9 +238,7 @@ void RetainCountChecker::checkPostStmt(const BlockExpr *BE, Regions.push_back(VR); } - state = - state->scanReachableSymbols<StopTrackingCallback>(Regions.data(), - Regions.data() + Regions.size()).getState(); + state = state->scanReachableSymbols<StopTrackingCallback>(Regions).getState(); C.addTransition(state); } @@ -352,6 +413,56 @@ void RetainCountChecker::checkPostCall(const CallEvent &Call, checkSummary(*Summ, Call, C); } +void RetainCountChecker::checkEndAnalysis(ExplodedGraph &G, BugReporter &BR, + ExprEngine &Eng) const { + // FIXME: This is a hack to make sure the summary log gets cleared between + // analyses of different code bodies. + // + // Why is this necessary? Because a checker's lifetime is tied to a + // translation unit, but an ExplodedGraph's lifetime is just a code body. + // Once in a blue moon, a new ExplodedNode will have the same address as an + // old one with an associated summary, and the bug report visitor gets very + // confused. (To make things worse, the summary lifetime is currently also + // tied to a code body, so we get a crash instead of incorrect results.) + // + // Why is this a bad solution? Because if the lifetime of the ExplodedGraph + // changes, things will start going wrong again. Really the lifetime of this + // log needs to be tied to either the specific nodes in it or the entire + // ExplodedGraph, not to a specific part of the code being analyzed. + // + // (Also, having stateful local data means that the same checker can't be + // used from multiple threads, but a lot of checkers have incorrect + // assumptions about that anyway. So that wasn't a priority at the time of + // this fix.) + // + // This happens at the end of analysis, but bug reports are emitted /after/ + // this point. So we can't just clear the summary log now. Instead, we mark + // that the next time we access the summary log, it should be cleared. + + // If we never reset the summary log during /this/ code body analysis, + // there were no new summaries. There might still have been summaries from + // the /last/ analysis, so clear them out to make sure the bug report + // visitors don't get confused. + if (ShouldResetSummaryLog) + SummaryLog.clear(); + + ShouldResetSummaryLog = !SummaryLog.empty(); +} + +CFRefBug * +RetainCountChecker::getLeakWithinFunctionBug(const LangOptions &LOpts) const { + if (!leakWithinFunction) + leakWithinFunction.reset(new Leak(this, "Leak")); + return leakWithinFunction.get(); +} + +CFRefBug * +RetainCountChecker::getLeakAtReturnBug(const LangOptions &LOpts) const { + if (!leakAtReturn) + leakAtReturn.reset(new Leak(this, "Leak of returned object")); + return leakAtReturn.get(); +} + /// GetReturnType - Used to get the return type of a message expression or /// function call with the intention of affixing that type to a tracked symbol. /// While the return type can be queried directly from RetEx, when @@ -422,13 +533,6 @@ void RetainCountChecker::processSummaryOfInlined(const RetainSummary &Summ, RetEffect RE = Summ.getRetEffect(); if (SymbolRef Sym = CallOrMsg.getReturnValue().getAsSymbol()) { - if (const auto *MCall = dyn_cast<CXXMemberCall>(&CallOrMsg)) { - if (Optional<RefVal> updatedRefVal = - refValFromRetEffect(RE, MCall->getResultType())) { - state = setRefBinding(state, Sym, *updatedRefVal); - } - } - if (RE.getKind() == RetEffect::NoRetHard) state = removeRefBinding(state, Sym); } @@ -470,6 +574,25 @@ static ProgramStateRef updateOutParameter(ProgramStateRef State, return State; } +static bool isPointerToObject(QualType QT) { + QualType PT = QT->getPointeeType(); + if (!PT.isNull()) + if (PT->getAsCXXRecordDecl()) + return true; + return false; +} + +/// Whether the tracked value should be escaped on a given call. +/// OSObjects are escaped when passed to void * / etc. +static bool shouldEscapeArgumentOnCall(const CallEvent &CE, unsigned ArgIdx, + const RefVal *TrackedValue) { + if (TrackedValue->getObjKind() != RetEffect::OS) + return false; + if (ArgIdx >= CE.parameters().size()) + return false; + return !isPointerToObject(CE.parameters()[ArgIdx]->getType()); +} + void RetainCountChecker::checkSummary(const RetainSummary &Summ, const CallEvent &CallOrMsg, CheckerContext &C) const { @@ -488,6 +611,10 @@ void RetainCountChecker::checkSummary(const RetainSummary &Summ, state = updateOutParameter(state, V, Effect); } else if (SymbolRef Sym = V.getAsLocSymbol()) { if (const RefVal *T = getRefBinding(state, Sym)) { + + if (shouldEscapeArgumentOnCall(CallOrMsg, idx, T)) + Effect = StopTrackingHard; + state = updateSymbol(state, Sym, *T, Effect, hasErr, C); if (hasErr) { ErrorRange = CallOrMsg.getArgSourceRange(idx); @@ -637,7 +764,7 @@ RetainCountChecker::updateSymbol(ProgramStateRef state, SymbolRef sym, break; } - // Fall-through. + LLVM_FALLTHROUGH; case DoNothing: return state; @@ -774,40 +901,48 @@ bool RetainCountChecker::evalCall(const CallExpr *CE, CheckerContext &C) const { // annotate attribute. If it does, we will not inline it. bool hasTrustedImplementationAnnotation = false; + const LocationContext *LCtx = C.getLocationContext(); + + using BehaviorSummary = RetainSummaryManager::BehaviorSummary; + Optional<BehaviorSummary> BSmr = + SmrMgr.canEval(CE, FD, hasTrustedImplementationAnnotation); + // See if it's one of the specific functions we know how to eval. - if (!SmrMgr.canEval(CE, FD, hasTrustedImplementationAnnotation)) + if (!BSmr) return false; // Bind the return value. - const LocationContext *LCtx = C.getLocationContext(); - SVal RetVal = state->getSVal(CE->getArg(0), LCtx); - if (RetVal.isUnknown() || - (hasTrustedImplementationAnnotation && !ResultTy.isNull())) { + if (BSmr == BehaviorSummary::Identity || + BSmr == BehaviorSummary::IdentityOrZero) { + SVal RetVal = state->getSVal(CE->getArg(0), LCtx); + // If the receiver is unknown or the function has // 'rc_ownership_trusted_implementation' annotate attribute, conjure a // return value. - SValBuilder &SVB = C.getSValBuilder(); - RetVal = SVB.conjureSymbolVal(nullptr, CE, LCtx, ResultTy, C.blockCount()); - } - state = state->BindExpr(CE, LCtx, RetVal, false); - - // FIXME: This should not be necessary, but otherwise the argument seems to be - // considered alive during the next statement. - if (const MemRegion *ArgRegion = RetVal.getAsRegion()) { - // Save the refcount status of the argument. - SymbolRef Sym = RetVal.getAsLocSymbol(); - const RefVal *Binding = nullptr; - if (Sym) - Binding = getRefBinding(state, Sym); - - // Invalidate the argument region. - state = state->invalidateRegions( - ArgRegion, CE, C.blockCount(), LCtx, - /*CausesPointerEscape*/ hasTrustedImplementationAnnotation); - - // Restore the refcount status of the argument. - if (Binding) - state = setRefBinding(state, Sym, *Binding); + if (RetVal.isUnknown() || + (hasTrustedImplementationAnnotation && !ResultTy.isNull())) { + SValBuilder &SVB = C.getSValBuilder(); + RetVal = + SVB.conjureSymbolVal(nullptr, CE, LCtx, ResultTy, C.blockCount()); + } + state = state->BindExpr(CE, LCtx, RetVal, /*Invalidate=*/false); + + if (BSmr == BehaviorSummary::IdentityOrZero) { + // Add a branch where the output is zero. + ProgramStateRef NullOutputState = C.getState(); + + // Assume that output is zero on the other branch. + NullOutputState = NullOutputState->BindExpr( + CE, LCtx, C.getSValBuilder().makeNull(), /*Invalidate=*/false); + + C.addTransition(NullOutputState); + + // And on the original branch assume that both input and + // output are non-zero. + if (auto L = RetVal.getAs<DefinedOrUnknownSVal>()) + state = state->assume(*L, /*Assumption=*/true); + + } } C.addTransition(state); @@ -947,8 +1082,7 @@ ExplodedNode * RetainCountChecker::checkReturnWithRetEffect(const ReturnStmt *S, if (N) { const LangOptions &LOpts = C.getASTContext().getLangOpts(); auto R = llvm::make_unique<CFRefLeakReport>( - *getLeakAtReturnBug(LOpts), LOpts, SummaryLog, N, Sym, C, - IncludeAllocationLine); + *getLeakAtReturnBug(LOpts), LOpts, SummaryLog, N, Sym, C); C.emitReport(std::move(R)); } return N; @@ -1097,9 +1231,8 @@ RetainCountChecker::checkRegionChanges(ProgramStateRef state, WhitelistedSymbols.insert(SR->getSymbol()); } - for (InvalidatedSymbols::const_iterator I=invalidated->begin(), - E = invalidated->end(); I!=E; ++I) { - SymbolRef sym = *I; + for (SymbolRef sym : + llvm::make_range(invalidated->begin(), invalidated->end())) { if (WhitelistedSymbols.count(sym)) continue; // Remove any existing reference-count binding. @@ -1235,7 +1368,7 @@ RetainCountChecker::processLeaks(ProgramStateRef state, assert(BT && "BugType not initialized."); Ctx.emitReport(llvm::make_unique<CFRefLeakReport>( - *BT, LOpts, SummaryLog, N, *I, Ctx, IncludeAllocationLine)); + *BT, LOpts, SummaryLog, N, *I, Ctx)); } } @@ -1322,19 +1455,6 @@ void RetainCountChecker::checkEndFunction(const ReturnStmt *RS, processLeaks(state, Leaked, Ctx, Pred); } -const ProgramPointTag * -RetainCountChecker::getDeadSymbolTag(SymbolRef sym) const { - const CheckerProgramPointTag *&tag = DeadSymbolTags[sym]; - if (!tag) { - SmallString<64> buf; - llvm::raw_svector_ostream out(buf); - out << "Dead Symbol : "; - sym->dumpToStream(out); - tag = new CheckerProgramPointTag(this, out.str()); - } - return tag; -} - void RetainCountChecker::checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const { ExplodedNode *Pred = C.getPredecessor(); @@ -1344,20 +1464,18 @@ void RetainCountChecker::checkDeadSymbols(SymbolReaper &SymReaper, SmallVector<SymbolRef, 10> Leaked; // Update counts from autorelease pools - for (SymbolReaper::dead_iterator I = SymReaper.dead_begin(), - E = SymReaper.dead_end(); I != E; ++I) { - SymbolRef Sym = *I; - if (const RefVal *T = B.lookup(Sym)){ - // Use the symbol as the tag. - // FIXME: This might not be as unique as we would like. - const ProgramPointTag *Tag = getDeadSymbolTag(Sym); - state = handleAutoreleaseCounts(state, Pred, Tag, C, Sym, *T); + for (const auto &I: state->get<RefBindings>()) { + SymbolRef Sym = I.first; + if (SymReaper.isDead(Sym)) { + static CheckerProgramPointTag Tag(this, "DeadSymbolAutorelease"); + const RefVal &V = I.second; + state = handleAutoreleaseCounts(state, Pred, &Tag, C, Sym, V); if (!state) return; // Fetch the new reference count from the state, and use it to handle // this symbol. - state = handleSymbolDeath(state, *I, *getRefBinding(state, Sym), Leaked); + state = handleSymbolDeath(state, Sym, *getRefBinding(state, Sym), Leaked); } } @@ -1408,5 +1526,23 @@ void RetainCountChecker::printState(raw_ostream &Out, ProgramStateRef State, //===----------------------------------------------------------------------===// void ento::registerRetainCountChecker(CheckerManager &Mgr) { - Mgr.registerChecker<RetainCountChecker>(Mgr.getAnalyzerOptions()); + auto *Chk = Mgr.registerChecker<RetainCountChecker>(); + Chk->TrackObjCAndCFObjects = true; +} + +// FIXME: remove this, hack for backwards compatibility: +// it should be possible to enable the NS/CF retain count checker as +// osx.cocoa.RetainCount, and it should be possible to disable +// osx.OSObjectRetainCount using osx.cocoa.RetainCount:CheckOSObject=false. +static bool hasPrevCheckOSObjectOptionDisabled(AnalyzerOptions &Options) { + auto I = Options.Config.find("osx.cocoa.RetainCount:CheckOSObject"); + if (I != Options.Config.end()) + return I->getValue() == "false"; + return false; +} + +void ento::registerOSObjectRetainCountChecker(CheckerManager &Mgr) { + auto *Chk = Mgr.registerChecker<RetainCountChecker>(); + if (!hasPrevCheckOSObjectOptionDisabled(Mgr.getAnalyzerOptions())) + Chk->TrackOSObjects = true; } diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h index e8d9136ffd..0f43e8f5dd 100644 --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h @@ -16,7 +16,6 @@ #define LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_RETAINCOUNTCHECKER_H #include "../ClangSACheckers.h" -#include "../AllocationDiagnostics.h" #include "RetainCountDiagnostics.h" #include "clang/AST/Attr.h" #include "clang/AST/DeclCXX.h" @@ -45,8 +44,6 @@ #include <cstdarg> #include <utility> -using llvm::StrInStrNoCase; - namespace clang { namespace ento { namespace retaincountchecker { @@ -95,7 +92,7 @@ private: /// See the RefVal::Kind enum for possible values. unsigned RawKind : 5; - /// The kind of object being tracked (CF or ObjC), if known. + /// The kind of object being tracked (CF or ObjC or OSObject), if known. /// /// See the RetEffect::ObjKind enum for possible values. unsigned RawObjectKind : 3; @@ -268,72 +265,26 @@ class RetainCountChecker mutable std::unique_ptr<RetainSummaryManager> Summaries; mutable SummaryLogTy SummaryLog; - AnalyzerOptions &Options; mutable bool ShouldResetSummaryLog; - /// Optional setting to indicate if leak reports should include - /// the allocation line. - mutable bool IncludeAllocationLine; - public: - RetainCountChecker(AnalyzerOptions &Options) - : Options(Options), ShouldResetSummaryLog(false), - IncludeAllocationLine( - shouldIncludeAllocationSiteInLeakDiagnostics(Options)) {} - ~RetainCountChecker() override { DeleteContainerSeconds(DeadSymbolTags); } + /// Track Objective-C and CoreFoundation objects. + bool TrackObjCAndCFObjects = false; - bool shouldCheckOSObjectRetainCount() const { - return Options.getBooleanOption("CheckOSObject", false, this); - } + /// Track sublcasses of OSObject. + bool TrackOSObjects = false; + + RetainCountChecker() : ShouldResetSummaryLog(false) {} + + ~RetainCountChecker() override { DeleteContainerSeconds(DeadSymbolTags); } void checkEndAnalysis(ExplodedGraph &G, BugReporter &BR, - ExprEngine &Eng) const { - // FIXME: This is a hack to make sure the summary log gets cleared between - // analyses of different code bodies. - // - // Why is this necessary? Because a checker's lifetime is tied to a - // translation unit, but an ExplodedGraph's lifetime is just a code body. - // Once in a blue moon, a new ExplodedNode will have the same address as an - // old one with an associated summary, and the bug report visitor gets very - // confused. (To make things worse, the summary lifetime is currently also - // tied to a code body, so we get a crash instead of incorrect results.) - // - // Why is this a bad solution? Because if the lifetime of the ExplodedGraph - // changes, things will start going wrong again. Really the lifetime of this - // log needs to be tied to either the specific nodes in it or the entire - // ExplodedGraph, not to a specific part of the code being analyzed. - // - // (Also, having stateful local data means that the same checker can't be - // used from multiple threads, but a lot of checkers have incorrect - // assumptions about that anyway. So that wasn't a priority at the time of - // this fix.) - // - // This happens at the end of analysis, but bug reports are emitted /after/ - // this point. So we can't just clear the summary log now. Instead, we mark - // that the next time we access the summary log, it should be cleared. - - // If we never reset the summary log during /this/ code body analysis, - // there were no new summaries. There might still have been summaries from - // the /last/ analysis, so clear them out to make sure the bug report - // visitors don't get confused. - if (ShouldResetSummaryLog) - SummaryLog.clear(); - - ShouldResetSummaryLog = !SummaryLog.empty(); - } + ExprEngine &Eng) const; - CFRefBug *getLeakWithinFunctionBug(const LangOptions &LOpts) const { - if (!leakWithinFunction) - leakWithinFunction.reset(new Leak(this, "Leak")); - return leakWithinFunction.get(); - } + CFRefBug *getLeakWithinFunctionBug(const LangOptions &LOpts) const; - CFRefBug *getLeakAtReturnBug(const LangOptions &LOpts) const { - if (!leakAtReturn) - leakAtReturn.reset(new Leak(this, "Leak of returned object")); - return leakAtReturn.get(); - } + CFRefBug *getLeakAtReturnBug(const LangOptions &LOpts) const; RetainSummaryManager &getSummaryManager(ASTContext &Ctx) const { // FIXME: We don't support ARC being turned on and off during one analysis. @@ -341,7 +292,7 @@ public: bool ARCEnabled = (bool)Ctx.getLangOpts().ObjCAutoRefCount; if (!Summaries) { Summaries.reset(new RetainSummaryManager( - Ctx, ARCEnabled, shouldCheckOSObjectRetainCount())); + Ctx, ARCEnabled, TrackObjCAndCFObjects, TrackOSObjects)); } else { assert(Summaries->isARCEnabled() == ARCEnabled); } diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp index 0be37ff65c..9dff0be138 100644 --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp @@ -28,16 +28,302 @@ static bool isNumericLiteralExpression(const Expr *E) { isa<CXXBoolLiteralExpr>(E); } +/// If type represents a pointer to CXXRecordDecl, +/// and is not a typedef, return the decl name. +/// Otherwise, return the serialization of type. +static std::string getPrettyTypeName(QualType QT) { + QualType PT = QT->getPointeeType(); + if (!PT.isNull() && !QT->getAs<TypedefType>()) + if (const auto *RD = PT->getAsCXXRecordDecl()) + return RD->getName(); + return QT.getAsString(); +} + +/// Write information about the type state change to {@code os}, +/// return whether the note should be generated. +static bool shouldGenerateNote(llvm::raw_string_ostream &os, + const RefVal *PrevT, const RefVal &CurrV, + SmallVector<ArgEffect, 2> &AEffects) { + // Get the previous type state. + RefVal PrevV = *PrevT; + + // Specially handle -dealloc. + if (std::find(AEffects.begin(), AEffects.end(), Dealloc) != AEffects.end()) { + // Determine if the object's reference count was pushed to zero. + assert(!PrevV.hasSameState(CurrV) && "The state should have changed."); + // We may not have transitioned to 'release' if we hit an error. + // This case is handled elsewhere. + if (CurrV.getKind() == RefVal::Released) { + assert(CurrV.getCombinedCounts() == 0); + os << "Object released by directly sending the '-dealloc' message"; + return true; + } + } + + // Determine if the typestate has changed. + if (!PrevV.hasSameState(CurrV)) + switch (CurrV.getKind()) { + case RefVal::Owned: + case RefVal::NotOwned: + if (PrevV.getCount() == CurrV.getCount()) { + // Did an autorelease message get sent? + if (PrevV.getAutoreleaseCount() == CurrV.getAutoreleaseCount()) + return false; + + assert(PrevV.getAutoreleaseCount() < CurrV.getAutoreleaseCount()); + os << "Object autoreleased"; + return true; + } + + if (PrevV.getCount() > CurrV.getCount()) + os << "Reference count decremented."; + else + os << "Reference count incremented."; + + if (unsigned Count = CurrV.getCount()) + os << " The object now has a +" << Count << " retain count."; + + return true; + + case RefVal::Released: + if (CurrV.getIvarAccessHistory() == + RefVal::IvarAccessHistory::ReleasedAfterDirectAccess && + CurrV.getIvarAccessHistory() != PrevV.getIvarAccessHistory()) { + os << "Strong instance variable relinquished. "; + } + os << "Object released."; + return true; + + case RefVal::ReturnedOwned: + // Autoreleases can be applied after marking a node ReturnedOwned. + if (CurrV.getAutoreleaseCount()) + return false; + + os << "Object returned to caller as an owning reference (single " + "retain count transferred to caller)"; + return true; + + case RefVal::ReturnedNotOwned: + os << "Object returned to caller with a +0 retain count"; + return true; + + default: + return false; + } + return true; +} + +static void generateDiagnosticsForCallLike(ProgramStateRef CurrSt, + const LocationContext *LCtx, + const RefVal &CurrV, SymbolRef &Sym, + const Stmt *S, + llvm::raw_string_ostream &os) { + if (const CallExpr *CE = dyn_cast<CallExpr>(S)) { + // Get the name of the callee (if it is available) + // from the tracked SVal. + SVal X = CurrSt->getSValAsScalarOrLoc(CE->getCallee(), LCtx); + const FunctionDecl *FD = X.getAsFunctionDecl(); + + // If failed, try to get it from AST. + if (!FD) + FD = dyn_cast<FunctionDecl>(CE->getCalleeDecl()); + + if (const auto *MD = dyn_cast<CXXMethodDecl>(CE->getCalleeDecl())) { + os << "Call to method '" << MD->getQualifiedNameAsString() << '\''; + } else if (FD) { + os << "Call to function '" << FD->getQualifiedNameAsString() << '\''; + } else { + os << "function call"; + } + } else if (isa<CXXNewExpr>(S)) { + os << "Operator 'new'"; + } else { + assert(isa<ObjCMessageExpr>(S)); + CallEventManager &Mgr = CurrSt->getStateManager().getCallEventManager(); + CallEventRef<ObjCMethodCall> Call = + Mgr.getObjCMethodCall(cast<ObjCMessageExpr>(S), CurrSt, LCtx); + + switch (Call->getMessageKind()) { + case OCM_Message: + os << "Method"; + break; + case OCM_PropertyAccess: + os << "Property"; + break; + case OCM_Subscript: + os << "Subscript"; + break; + } + } + + if (CurrV.getObjKind() == RetEffect::CF) { + os << " returns a Core Foundation object of type " + << Sym->getType().getAsString() << " with a "; + } else if (CurrV.getObjKind() == RetEffect::OS) { + os << " returns an OSObject of type " << getPrettyTypeName(Sym->getType()) + << " 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 (!isa<ObjCObjectPointerType>(T)) { + os << " returns an Objective-C object with a "; + } else { + const ObjCObjectPointerType *PT = cast<ObjCObjectPointerType>(T); + os << " returns an instance of " << PT->getPointeeType().getAsString() + << " with a "; + } + } + + if (CurrV.isOwned()) { + os << "+1 retain count"; + } else { + assert(CurrV.isNotOwned()); + os << "+0 retain count"; + } +} + +namespace clang { +namespace ento { +namespace retaincountchecker { + +class CFRefReportVisitor : public BugReporterVisitor { +protected: + SymbolRef Sym; + const SummaryLogTy &SummaryLog; + +public: + CFRefReportVisitor(SymbolRef sym, const SummaryLogTy &log) + : Sym(sym), SummaryLog(log) {} + + void Profile(llvm::FoldingSetNodeID &ID) const override { + static int x = 0; + ID.AddPointer(&x); + ID.AddPointer(Sym); + } + + std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N, + BugReporterContext &BRC, + BugReport &BR) override; + + std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC, + const ExplodedNode *N, + BugReport &BR) override; +}; + +class CFRefLeakReportVisitor : public CFRefReportVisitor { +public: + CFRefLeakReportVisitor(SymbolRef sym, + const SummaryLogTy &log) + : CFRefReportVisitor(sym, log) {} + + std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC, + const ExplodedNode *N, + BugReport &BR) override; +}; + +} // end namespace retaincountchecker +} // end namespace ento +} // end namespace clang + + +/// Find the first node with the parent stack frame. +static const ExplodedNode *getCalleeNode(const ExplodedNode *Pred) { + const StackFrameContext *SC = Pred->getStackFrame(); + if (SC->inTopFrame()) + return nullptr; + const StackFrameContext *PC = SC->getParent()->getStackFrame(); + if (!PC) + return nullptr; + + const ExplodedNode *N = Pred; + while (N && N->getStackFrame() != PC) { + N = N->getFirstPred(); + } + return N; +} + + +/// Insert a diagnostic piece at function exit +/// if a function parameter is annotated as "os_consumed", +/// but it does not actually consume the reference. +static std::shared_ptr<PathDiagnosticEventPiece> +annotateConsumedSummaryMismatch(const ExplodedNode *N, + CallExitBegin &CallExitLoc, + const SourceManager &SM, + CallEventManager &CEMgr) { + + const ExplodedNode *CN = getCalleeNode(N); + if (!CN) + return nullptr; + + CallEventRef<> Call = CEMgr.getCaller(N->getStackFrame(), N->getState()); + + std::string sbuf; + llvm::raw_string_ostream os(sbuf); + ArrayRef<const ParmVarDecl *> Parameters = Call->parameters(); + for (unsigned I=0; I < Call->getNumArgs() && I < Parameters.size(); ++I) { + const ParmVarDecl *PVD = Parameters[I]; + + if (!PVD->hasAttr<OSConsumedAttr>()) + return nullptr; + + if (SymbolRef SR = Call->getArgSVal(I).getAsLocSymbol()) { + const RefVal *CountBeforeCall = getRefBinding(CN->getState(), SR); + const RefVal *CountAtExit = getRefBinding(N->getState(), SR); + + if (!CountBeforeCall || !CountAtExit) + continue; + + unsigned CountBefore = CountBeforeCall->getCount(); + unsigned CountAfter = CountAtExit->getCount(); + + bool AsExpected = CountBefore > 0 && CountAfter == CountBefore - 1; + if (!AsExpected) { + os << "Parameter '"; + PVD->getNameForDiagnostic(os, PVD->getASTContext().getPrintingPolicy(), + /*Qualified=*/false); + os << "' is marked as consuming, but the function does not consume " + << "the reference\n"; + } + } + } + + if (os.str().empty()) + return nullptr; + + // FIXME: remove the code duplication with NoStoreFuncVisitor. + PathDiagnosticLocation L; + if (const ReturnStmt *RS = CallExitLoc.getReturnStmt()) { + L = PathDiagnosticLocation::createBegin(RS, SM, N->getLocationContext()); + } else { + L = PathDiagnosticLocation( + Call->getRuntimeDefinition().getDecl()->getSourceRange().getEnd(), SM); + } + + return std::make_shared<PathDiagnosticEventPiece>(L, os.str()); +} + std::shared_ptr<PathDiagnosticPiece> CFRefReportVisitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC, BugReport &BR) { + const SourceManager &SM = BRC.getSourceManager(); + CallEventManager &CEMgr = BRC.getStateManager().getCallEventManager(); + if (auto CE = N->getLocationAs<CallExitBegin>()) { + if (auto PD = annotateConsumedSummaryMismatch(N, *CE, SM, CEMgr)) + return PD; + } + // FIXME: We will eventually need to handle non-statement-based events // (__attribute__((cleanup))). if (!N->getLocation().getAs<StmtPoint>()) return nullptr; // Check if the type state has changed. - ProgramStateRef PrevSt = N->getFirstPred()->getState(); + const ExplodedNode *PrevNode = N->getFirstPred(); + ProgramStateRef PrevSt = PrevNode->getState(); ProgramStateRef CurrSt = N->getState(); const LocationContext *LCtx = N->getLocationContext(); @@ -64,11 +350,9 @@ CFRefReportVisitor::VisitNode(const ExplodedNode *N, if (isa<ObjCArrayLiteral>(S)) { os << "NSArray literal is an object with a +0 retain count"; - } - else if (isa<ObjCDictionaryLiteral>(S)) { + } else if (isa<ObjCDictionaryLiteral>(S)) { os << "NSDictionary literal is an object with a +0 retain count"; - } - else if (const ObjCBoxedExpr *BL = dyn_cast<ObjCBoxedExpr>(S)) { + } else if (const ObjCBoxedExpr *BL = dyn_cast<ObjCBoxedExpr>(S)) { if (isNumericLiteralExpression(BL->getSubExpr())) os << "NSNumber literal is an object with a +0 retain count"; else { @@ -78,83 +362,27 @@ CFRefReportVisitor::VisitNode(const ExplodedNode *N, // We should always be able to find the boxing class interface, // but consider this future-proofing. - if (BoxClass) + if (BoxClass) { os << *BoxClass << " b"; - else + } else { os << "B"; + } os << "oxed expression produces an object with a +0 retain count"; } - } - else if (isa<ObjCIvarRefExpr>(S)) { + } else if (isa<ObjCIvarRefExpr>(S)) { os << "Object loaded from instance variable"; - } - else { - if (const CallExpr *CE = dyn_cast<CallExpr>(S)) { - // Get the name of the callee (if it is available). - SVal X = CurrSt->getSValAsScalarOrLoc(CE->getCallee(), LCtx); - if (const FunctionDecl *FD = X.getAsFunctionDecl()) - os << "Call to function '" << *FD << '\''; - else - os << "function call"; - } - else { - assert(isa<ObjCMessageExpr>(S)); - CallEventManager &Mgr = CurrSt->getStateManager().getCallEventManager(); - CallEventRef<ObjCMethodCall> Call - = Mgr.getObjCMethodCall(cast<ObjCMessageExpr>(S), CurrSt, LCtx); - - switch (Call->getMessageKind()) { - case OCM_Message: - os << "Method"; - break; - case OCM_PropertyAccess: - os << "Property"; - break; - case OCM_Subscript: - os << "Subscript"; - break; - } - } - - if (CurrV.getObjKind() == RetEffect::CF) { - os << " returns a Core Foundation object of type " - << Sym->getType().getAsString() << " with a "; - } else if (CurrV.getObjKind() == RetEffect::OS) { - os << " returns an OSObject 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 (!isa<ObjCObjectPointerType>(T)) { - os << " returns an Objective-C object with a "; - } else { - const ObjCObjectPointerType *PT = cast<ObjCObjectPointerType>(T); - os << " returns an instance of " - << PT->getPointeeType().getAsString() << " with a "; - } - } - - if (CurrV.isOwned()) { - os << "+1 retain count"; - } else { - assert (CurrV.isNotOwned()); - os << "+0 retain count"; - } + } else { + generateDiagnosticsForCallLike(CurrSt, LCtx, CurrV, Sym, S, os); } - PathDiagnosticLocation Pos(S, BRC.getSourceManager(), - N->getLocationContext()); + PathDiagnosticLocation Pos(S, SM, N->getLocationContext()); return std::make_shared<PathDiagnosticEventPiece>(Pos, os.str()); } // Gather up the effects that were performed on the object at this // program point SmallVector<ArgEffect, 2> AEffects; - const ExplodedNode *OrigNode = BRC.getNodeResolver().getOriginalNode(N); if (const RetainSummary *Summ = SummaryLog.lookup(OrigNode)) { // We only have summaries attached to nodes after evaluating CallExpr and @@ -166,8 +394,7 @@ CFRefReportVisitor::VisitNode(const ExplodedNode *N, // was ever passed as an argument. unsigned i = 0; - for (CallExpr::const_arg_iterator AI=CE->arg_begin(), AE=CE->arg_end(); - AI!=AE; ++AI, ++i) { + for (auto AI=CE->arg_begin(), AE=CE->arg_end(); AI!=AE; ++AI, ++i) { // Retrieve the value of the argument. Is it the symbol // we are interested in? @@ -188,75 +415,8 @@ CFRefReportVisitor::VisitNode(const ExplodedNode *N, } } - do { - // Get the previous type state. - RefVal PrevV = *PrevT; - - // Specially handle -dealloc. - if (std::find(AEffects.begin(), AEffects.end(), Dealloc) != - AEffects.end()) { - // Determine if the object's reference count was pushed to zero. - assert(!PrevV.hasSameState(CurrV) && "The state should have changed."); - // We may not have transitioned to 'release' if we hit an error. - // This case is handled elsewhere. - if (CurrV.getKind() == RefVal::Released) { - assert(CurrV.getCombinedCounts() == 0); - os << "Object released by directly sending the '-dealloc' message"; - break; - } - } - - // Determine if the typestate has changed. - if (!PrevV.hasSameState(CurrV)) - switch (CurrV.getKind()) { - case RefVal::Owned: - case RefVal::NotOwned: - if (PrevV.getCount() == CurrV.getCount()) { - // Did an autorelease message get sent? - if (PrevV.getAutoreleaseCount() == CurrV.getAutoreleaseCount()) - return nullptr; - - assert(PrevV.getAutoreleaseCount() < CurrV.getAutoreleaseCount()); - os << "Object autoreleased"; - break; - } - - if (PrevV.getCount() > CurrV.getCount()) - os << "Reference count decremented."; - else - os << "Reference count incremented."; - - if (unsigned Count = CurrV.getCount()) - os << " The object now has a +" << Count << " retain count."; - - break; - - case RefVal::Released: - if (CurrV.getIvarAccessHistory() == - RefVal::IvarAccessHistory::ReleasedAfterDirectAccess && - CurrV.getIvarAccessHistory() != PrevV.getIvarAccessHistory()) { - os << "Strong instance variable relinquished. "; - } - os << "Object released."; - break; - - case RefVal::ReturnedOwned: - // Autoreleases can be applied after marking a node ReturnedOwned. - if (CurrV.getAutoreleaseCount()) - return nullptr; - - os << "Object returned to caller as an owning reference (single " - "retain count transferred to caller)"; - break; - - case RefVal::ReturnedNotOwned: - os << "Object returned to caller with a +0 retain count"; - break; - - default: - return nullptr; - } - } while (0); + if (!shouldGenerateNote(os, PrevT, CurrV, AEffects)) + return nullptr; if (os.str().empty()) return nullptr; // We have nothing to say! @@ -303,9 +463,8 @@ struct AllocationInfo { }; } // end anonymous namespace -static AllocationInfo -GetAllocationSite(ProgramStateManager& StateMgr, const ExplodedNode *N, - SymbolRef Sym) { +static AllocationInfo GetAllocationSite(ProgramStateManager &StateMgr, + const ExplodedNode *N, SymbolRef Sym) { const ExplodedNode *AllocationNode = N; const ExplodedNode *AllocationNodeInCurrentOrParentContext = N; const MemRegion *FirstBinding = nullptr; @@ -327,11 +486,11 @@ GetAllocationSite(ProgramStateManager& StateMgr, const ExplodedNode *N, if (FB) { const MemRegion *R = FB.getRegion(); - const VarRegion *VR = R->getBaseRegion()->getAs<VarRegion>(); // Do not show local variables belonging to a function other than // where the error is reported. - if (!VR || VR->getStackFrame() == LeakContext->getStackFrame()) - FirstBinding = R; + if (auto MR = dyn_cast<StackSpaceRegion>(R->getMemorySpace())) + if (MR->getStackFrame() == LeakContext->getStackFrame()) + FirstBinding = R; } // AllocationNode is the last node in which the symbol was tracked. @@ -340,7 +499,7 @@ GetAllocationSite(ProgramStateManager& StateMgr, const ExplodedNode *N, // AllocationNodeInCurrentContext, is the last node in the current or // parent context in which the symbol was tracked. // - // Note that the allocation site might be in the parent conext. For example, + // Note that the allocation site might be in the parent context. For example, // the case where an allocation happens in a block that captures a reference // to it and that reference is overwritten/dropped by another call to // the block. @@ -350,9 +509,9 @@ GetAllocationSite(ProgramStateManager& StateMgr, const ExplodedNode *N, // Find the last init that was called on the given symbol and store the // init method's location context. if (!InitMethodContext) - if (Optional<CallEnter> CEP = N->getLocation().getAs<CallEnter>()) { + if (auto CEP = N->getLocation().getAs<CallEnter>()) { const Stmt *CE = CEP->getCallExpr(); - if (const ObjCMessageExpr *ME = dyn_cast_or_null<ObjCMessageExpr>(CE)) { + if (const auto *ME = dyn_cast_or_null<ObjCMessageExpr>(CE)) { const Stmt *RecExpr = ME->getInstanceReceiver(); if (RecExpr) { SVal RecV = St->getSVal(RecExpr, NContext); @@ -362,7 +521,7 @@ GetAllocationSite(ProgramStateManager& StateMgr, const ExplodedNode *N, } } - N = N->pred_empty() ? nullptr : *(N->pred_begin()); + N = N->getFirstPred(); } // If we are reporting a leak of the object that was allocated with alloc, @@ -379,9 +538,11 @@ GetAllocationSite(ProgramStateManager& StateMgr, const ExplodedNode *N, // If allocation happened in a function different from the leak node context, // do not report the binding. assert(N && "Could not find allocation node"); - if (N->getLocationContext() != LeakContext) { + + if (AllocationNodeInCurrentOrParentContext && + AllocationNodeInCurrentOrParentContext->getLocationContext() != + LeakContext) FirstBinding = nullptr; - } return AllocationInfo(AllocationNodeInCurrentOrParentContext, FirstBinding, @@ -406,8 +567,7 @@ CFRefLeakReportVisitor::getEndPath(BugReporterContext &BRC, // We are reporting a leak. Walk up the graph to get to the first node where // the symbol appeared, and also get the first VarDecl that tracked object // is stored to. - AllocationInfo AllocI = - GetAllocationSite(BRC.getStateManager(), EndN, Sym); + AllocationInfo AllocI = GetAllocationSite(BRC.getStateManager(), EndN, Sym); const MemRegion* FirstBinding = AllocI.R; BR.markInteresting(AllocI.InterestingMethodContext); @@ -428,9 +588,9 @@ CFRefLeakReportVisitor::getEndPath(BugReporterContext &BRC, Optional<std::string> RegionDescription = describeRegion(FirstBinding); if (RegionDescription) { os << "object allocated and stored into '" << *RegionDescription << '\''; + } else { + os << "allocated object of type " << getPrettyTypeName(Sym->getType()); } - else - os << "allocated object"; // Get the retain count. const RefVal* RV = getRefBinding(EndN->getState(), Sym); @@ -445,11 +605,13 @@ CFRefLeakReportVisitor::getEndPath(BugReporterContext &BRC, os << (isa<ObjCMethodDecl>(D) ? " is returned from a method " : " is returned from a function "); - if (D->hasAttr<CFReturnsNotRetainedAttr>()) + if (D->hasAttr<CFReturnsNotRetainedAttr>()) { os << "that is annotated as CF_RETURNS_NOT_RETAINED"; - else if (D->hasAttr<NSReturnsNotRetainedAttr>()) + } else if (D->hasAttr<NSReturnsNotRetainedAttr>()) { os << "that is annotated as NS_RETURNS_NOT_RETAINED"; - else { + } else if (D->hasAttr<OSReturnsNotRetainedAttr>()) { + os << "that is annotated as OS_RETURNS_NOT_RETAINED"; + } else { if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D)) { if (BRC.getASTContext().getLangOpts().ObjCAutoRefCount) { os << "managed by Automatic Reference Counting"; @@ -468,14 +630,30 @@ CFRefLeakReportVisitor::getEndPath(BugReporterContext &BRC, " Foundation"; } } - } - else + } else { os << " is not referenced later in this execution path and has a retain " "count of +" << RV->getCount(); + } return std::make_shared<PathDiagnosticEventPiece>(L, os.str()); } +CFRefReport::CFRefReport(CFRefBug &D, const LangOptions &LOpts, + const SummaryLogTy &Log, ExplodedNode *n, + SymbolRef sym, bool registerVisitor) + : BugReport(D, D.getDescription(), n), Sym(sym) { + if (registerVisitor) + addVisitor(llvm::make_unique<CFRefReportVisitor>(sym, Log)); +} + +CFRefReport::CFRefReport(CFRefBug &D, const LangOptions &LOpts, + const SummaryLogTy &Log, ExplodedNode *n, + SymbolRef sym, StringRef endText) + : BugReport(D, D.getDescription(), endText, n) { + + addVisitor(llvm::make_unique<CFRefReportVisitor>(sym, Log)); +} + void CFRefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) { const SourceManager& SMgr = Ctx.getSourceManager(); @@ -494,7 +672,8 @@ void CFRefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) { } } -void CFRefLeakReport::deriveAllocLocation(CheckerContext &Ctx,SymbolRef sym) { +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 @@ -508,7 +687,7 @@ void CFRefLeakReport::deriveAllocLocation(CheckerContext &Ctx,SymbolRef sym) { const SourceManager& SMgr = Ctx.getSourceManager(); AllocationInfo AllocI = - GetAllocationSite(Ctx.getStateManager(), getErrorNode(), sym); + GetAllocationSite(Ctx.getStateManager(), getErrorNode(), sym); AllocNode = AllocI.N; AllocBinding = AllocI.R; @@ -536,8 +715,7 @@ void CFRefLeakReport::deriveAllocLocation(CheckerContext &Ctx,SymbolRef sym) { UniqueingDecl = AllocNode->getLocationContext()->getDecl(); } -void CFRefLeakReport::createDescription(CheckerContext &Ctx, - bool IncludeAllocationLine) { +void CFRefLeakReport::createDescription(CheckerContext &Ctx) { assert(Location.isValid() && UniqueingDecl && UniqueingLocation.isValid()); Description.clear(); llvm::raw_string_ostream os(Description); @@ -546,25 +724,24 @@ void CFRefLeakReport::createDescription(CheckerContext &Ctx, Optional<std::string> RegionDescription = describeRegion(AllocBinding); if (RegionDescription) { os << " stored into '" << *RegionDescription << '\''; - if (IncludeAllocationLine) { - FullSourceLoc SL(AllocStmt->getBeginLoc(), Ctx.getSourceManager()); - os << " (allocated on line " << SL.getSpellingLineNumber() << ")"; - } + } else { + + // If we can't figure out the name, just supply the type information. + os << " of type " << getPrettyTypeName(Sym->getType()); } } CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts, const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym, - CheckerContext &Ctx, - bool IncludeAllocationLine) + CheckerContext &Ctx) : CFRefReport(D, LOpts, Log, n, sym, false) { deriveAllocLocation(Ctx, sym); if (!AllocBinding) deriveParamLocation(Ctx, sym); - createDescription(Ctx, IncludeAllocationLine); + createDescription(Ctx); addVisitor(llvm::make_unique<CFRefLeakReportVisitor>(sym, Log)); } diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h index 58abd67039..a30f62ac34 100644 --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h @@ -37,126 +37,21 @@ public: virtual bool isLeak() const { return false; } }; -class UseAfterRelease : public CFRefBug { -public: - UseAfterRelease(const CheckerBase *checker) - : CFRefBug(checker, "Use-after-release") {} - - const char *getDescription() const override { - return "Reference-counted object is used after it is released"; - } -}; - -class BadRelease : public CFRefBug { -public: - BadRelease(const CheckerBase *checker) : CFRefBug(checker, "Bad release") {} - - const char *getDescription() const override { - return "Incorrect decrement of the reference count of an object that is " - "not owned at this point by the caller"; - } -}; - -class DeallocNotOwned : public CFRefBug { -public: - DeallocNotOwned(const CheckerBase *checker) - : CFRefBug(checker, "-dealloc sent to non-exclusively owned object") {} - - const char *getDescription() const override { - return "-dealloc sent to object that may be referenced elsewhere"; - } -}; - -class OverAutorelease : public CFRefBug { -public: - OverAutorelease(const CheckerBase *checker) - : CFRefBug(checker, "Object autoreleased too many times") {} - - const char *getDescription() const override { - return "Object autoreleased too many times"; - } -}; - -class ReturnedNotOwnedForOwned : public CFRefBug { -public: - ReturnedNotOwnedForOwned(const CheckerBase *checker) - : CFRefBug(checker, "Method should return an owned object") {} - - const char *getDescription() const override { - return "Object with a +0 retain count returned to caller where a +1 " - "(owning) retain count is expected"; - } -}; - -class Leak : public CFRefBug { -public: - Leak(const CheckerBase *checker, StringRef name) : CFRefBug(checker, name) { - // Leaks should not be reported if they are post-dominated by a sink. - setSuppressOnSink(true); - } - - const char *getDescription() const override { return ""; } - - bool isLeak() const override { return true; } -}; - typedef ::llvm::DenseMap<const ExplodedNode *, const RetainSummary *> SummaryLogTy; -/// Visitors. - -class CFRefReportVisitor : public BugReporterVisitor { +class CFRefReport : public BugReport { protected: SymbolRef Sym; - const SummaryLogTy &SummaryLog; - -public: - CFRefReportVisitor(SymbolRef sym, const SummaryLogTy &log) - : Sym(sym), SummaryLog(log) {} - - void Profile(llvm::FoldingSetNodeID &ID) const override { - static int x = 0; - ID.AddPointer(&x); - ID.AddPointer(Sym); - } - - std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N, - BugReporterContext &BRC, - BugReport &BR) override; - - std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC, - const ExplodedNode *N, - BugReport &BR) override; -}; - -class CFRefLeakReportVisitor : public CFRefReportVisitor { -public: - CFRefLeakReportVisitor(SymbolRef sym, - const SummaryLogTy &log) - : CFRefReportVisitor(sym, log) {} - - std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC, - const ExplodedNode *N, - BugReport &BR) override; -}; - -class CFRefReport : public BugReport { public: CFRefReport(CFRefBug &D, const LangOptions &LOpts, const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym, - bool registerVisitor = true) - : BugReport(D, D.getDescription(), n) { - if (registerVisitor) - addVisitor(llvm::make_unique<CFRefReportVisitor>(sym, Log)); - } + bool registerVisitor = true); CFRefReport(CFRefBug &D, const LangOptions &LOpts, const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym, - StringRef endText) - : BugReport(D, D.getDescription(), endText, n) { - addVisitor(llvm::make_unique<CFRefReportVisitor>(sym, Log)); - } + StringRef endText); llvm::iterator_range<ranges_iterator> getRanges() override { const CFRefBug& BugTy = static_cast<CFRefBug&>(getBugType()); @@ -176,13 +71,12 @@ class CFRefLeakReport : public CFRefReport { // 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 IncludeAllocationLine); + void createDescription(CheckerContext &Ctx); public: CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts, const SummaryLogTy &Log, ExplodedNode *n, SymbolRef sym, - CheckerContext &Ctx, - bool IncludeAllocationLine); + CheckerContext &Ctx); PathDiagnosticLocation getLocation(const SourceManager &SM) const override { assert(Location.isValid()); diff --git a/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp b/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp index c5e826a84b..e866f06ebb 100644 --- a/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp @@ -87,7 +87,7 @@ static void emitBug(CheckerContext &C, BuiltinBug &BT, const Expr *RetE, auto Report = llvm::make_unique<BugReport>(BT, BT.getDescription(), N); Report->addRange(RetE->getSourceRange()); - bugreporter::trackNullOrUndefValue(N, TrackingE ? TrackingE : RetE, *Report); + bugreporter::trackExpressionValue(N, TrackingE ? TrackingE : RetE, *Report); C.emitReport(std::move(Report)); } diff --git a/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp b/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp index 55516a34d1..3f3477b928 100644 --- a/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp @@ -58,13 +58,12 @@ public: } // end anonymous namespace - -using TriBoolTy = Optional<bool>; -using MemoizationMapTy = llvm::DenseMap<const Stmt *, Optional<TriBoolTy>>; - -static TriBoolTy -seenBeforeRec(const Stmt *Parent, const Stmt *A, const Stmt *B, - MemoizationMapTy &Memoization) { +/// \return Whether {@code A} occurs before {@code B} in traversal of +/// {@code Parent}. +/// Conceptually a very incomplete/unsound approximation of happens-before +/// relationship (A is likely to be evaluated before B), +/// but useful enough in this case. +static bool seenBefore(const Stmt *Parent, const Stmt *A, const Stmt *B) { for (const Stmt *C : Parent->children()) { if (!C) continue; @@ -74,26 +73,9 @@ seenBeforeRec(const Stmt *Parent, const Stmt *A, const Stmt *B, if (C == B) return false; - Optional<TriBoolTy> &Cached = Memoization[C]; - if (!Cached) - Cached = seenBeforeRec(C, A, B, Memoization); - - if (Cached->hasValue()) - return Cached->getValue(); + return seenBefore(C, A, B); } - - return None; -} - -/// \return Whether {@code A} occurs before {@code B} in traversal of -/// {@code Parent}. -/// Conceptually a very incomplete/unsound approximation of happens-before -/// relationship (A is likely to be evaluated before B), -/// but useful enough in this case. -static bool seenBefore(const Stmt *Parent, const Stmt *A, const Stmt *B) { - MemoizationMapTy Memoization; - TriBoolTy Val = seenBeforeRec(Parent, A, B, Memoization); - return Val.getValue(); + return false; } static void emitDiagnostics(BoundNodes &Match, diff --git a/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index d77975559e..b383411068 100644 --- a/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -383,26 +383,26 @@ ProgramStateRef StreamChecker::CheckDoubleClose(const CallExpr *CE, void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const { + ProgramStateRef state = C.getState(); + // TODO: Clean up the state. - for (SymbolReaper::dead_iterator I = SymReaper.dead_begin(), - E = SymReaper.dead_end(); I != E; ++I) { - SymbolRef Sym = *I; - ProgramStateRef state = C.getState(); - const StreamState *SS = state->get<StreamMap>(Sym); - if (!SS) + const StreamMapTy &Map = state->get<StreamMap>(); + for (const auto &I: Map) { + SymbolRef Sym = I.first; + const StreamState &SS = I.second; + if (!SymReaper.isDead(Sym) || !SS.isOpened()) continue; - if (SS->isOpened()) { - ExplodedNode *N = C.generateErrorNode(); - if (N) { - if (!BT_ResourceLeak) - BT_ResourceLeak.reset(new BuiltinBug( - this, "Resource Leak", - "Opened File never closed. Potential Resource leak.")); - C.emitReport(llvm::make_unique<BugReport>( - *BT_ResourceLeak, BT_ResourceLeak->getDescription(), N)); - } - } + ExplodedNode *N = C.generateErrorNode(); + if (!N) + return; + + if (!BT_ResourceLeak) + BT_ResourceLeak.reset( + new BuiltinBug(this, "Resource Leak", + "Opened File never closed. Potential Resource leak.")); + C.emitReport(llvm::make_unique<BugReport>( + *BT_ResourceLeak, BT_ResourceLeak->getDescription(), N)); } } diff --git a/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp b/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp index eed1efd10e..515c98cd11 100644 --- a/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp @@ -212,20 +212,26 @@ private: /// the negation of \p Antecedent. /// Checks NonNullImplicationMap and assumes \p Antecedent otherwise. ProgramStateRef addImplication(SymbolRef Antecedent, - ProgramStateRef State, + ProgramStateRef InputState, bool Negated) const { - SValBuilder &SVB = State->getStateManager().getSValBuilder(); + if (!InputState) + return nullptr; + SValBuilder &SVB = InputState->getStateManager().getSValBuilder(); const SymbolRef *Consequent = - Negated ? State->get<NonNullImplicationMap>(Antecedent) - : State->get<NullImplicationMap>(Antecedent); + Negated ? InputState->get<NonNullImplicationMap>(Antecedent) + : InputState->get<NullImplicationMap>(Antecedent); if (!Consequent) - return State; + return InputState; SVal AntecedentV = SVB.makeSymbolVal(Antecedent); - if ((Negated && State->isNonNull(AntecedentV).isConstrainedTrue()) - || (!Negated && State->isNull(AntecedentV).isConstrainedTrue())) { + ProgramStateRef State = InputState; + + if ((Negated && InputState->isNonNull(AntecedentV).isConstrainedTrue()) + || (!Negated && InputState->isNull(AntecedentV).isConstrainedTrue())) { SVal ConsequentS = SVB.makeSymbolVal(*Consequent); - State = State->assume(ConsequentS.castAs<DefinedSVal>(), Negated); + State = InputState->assume(ConsequentS.castAs<DefinedSVal>(), Negated); + if (!State) + return nullptr; // Drop implications from the map. if (Negated) { diff --git a/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp index 934ee63318..9e75bba5eb 100644 --- a/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp @@ -98,7 +98,7 @@ void UndefBranchChecker::checkBranchCondition(const Stmt *Condition, // Emit the bug report. auto R = llvm::make_unique<BugReport>(*BT, BT->getDescription(), N); - bugreporter::trackNullOrUndefValue(N, Ex, *R); + bugreporter::trackExpressionValue(N, Ex, *R); R->addRange(Ex->getSourceRange()); Ctx.emitReport(std::move(R)); diff --git a/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp index 47faf699f9..f30f32e959 100644 --- a/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp @@ -174,10 +174,10 @@ void UndefResultChecker::checkPostStmt(const BinaryOperator *B, auto report = llvm::make_unique<BugReport>(*BT, OS.str(), N); if (Ex) { report->addRange(Ex->getSourceRange()); - bugreporter::trackNullOrUndefValue(N, Ex, *report); + bugreporter::trackExpressionValue(N, Ex, *report); } else - bugreporter::trackNullOrUndefValue(N, B, *report); + bugreporter::trackExpressionValue(N, B, *report); C.emitReport(std::move(report)); } diff --git a/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp index fe07eafd28..5a704eb41c 100644 --- a/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp @@ -55,7 +55,7 @@ UndefinedArraySubscriptChecker::checkPreStmt(const ArraySubscriptExpr *A, // Generate a report for this bug. auto R = llvm::make_unique<BugReport>(*BT, BT->getName(), N); R->addRange(A->getIdx()->getSourceRange()); - bugreporter::trackNullOrUndefValue(N, A->getIdx(), *R); + bugreporter::trackExpressionValue(N, A->getIdx(), *R); C.emitReport(std::move(R)); } diff --git a/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp index 2ef6855ba6..a0bc857c49 100644 --- a/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp @@ -112,7 +112,7 @@ void UndefinedAssignmentChecker::checkBind(SVal location, SVal val, auto R = llvm::make_unique<BugReport>(*BT, OS.str(), N); if (ex) { R->addRange(ex->getSourceRange()); - bugreporter::trackNullOrUndefValue(N, ex, *R); + bugreporter::trackExpressionValue(N, ex, *R); } C.emitReport(std::move(R)); } diff --git a/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h b/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h index d10b862ea0..c3291a21c1 100644 --- a/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h +++ b/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h @@ -21,7 +21,7 @@ // `-analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true`. // // - "NotesAsWarnings" (boolean). If set to true, the checker will emit a -// warning for each uninitalized field, as opposed to emitting one warning +// warning for each uninitialized field, as opposed to emitting one warning // per constructor call, and listing the uninitialized fields that belongs // to it in notes. Defaults to false. // @@ -215,7 +215,11 @@ public: const TypedValueRegion *const R, const UninitObjCheckerOptions &Opts); - const UninitFieldMap &getUninitFields() { return UninitFields; } + /// Returns with the modified state and a map of (uninitialized region, + /// note message) pairs. + std::pair<ProgramStateRef, const UninitFieldMap &> getResults() { + return {State, UninitFields}; + } /// Returns whether the analyzed region contains at least one initialized /// field. Note that this includes subfields as well, not just direct ones, @@ -230,7 +234,7 @@ private: // * every node is an object that is // - a union // - a non-union record - // - dereferencable (see isDereferencableType()) + // - dereferenceable (see isDereferencableType()) // - an array // - of a primitive type (see isPrimitiveType()) // * the parent of each node is the object that contains it @@ -271,7 +275,7 @@ private: // this->iptr (pointee uninit) // this->bptr (pointer uninit) // - // We'll traverse each node of the above graph with the appropiate one of + // We'll traverse each node of the above graph with the appropriate one of // these methods: /// Checks the region of a union object, and returns true if no field is @@ -296,14 +300,16 @@ private: // TODO: Add a support for nonloc::LocAsInteger. /// Processes LocalChain and attempts to insert it into UninitFields. Returns - /// true on success. + /// true on success. Also adds the head of the list and \p PointeeR (if + /// supplied) to the GDM as already analyzed objects. /// /// Since this class analyzes regions with recursion, we'll only store /// references to temporary FieldNode objects created on the stack. This means /// that after analyzing a leaf of the directed tree described above, the /// elements LocalChain references will be destructed, so we can't store it /// directly. - bool addFieldToUninits(FieldChainInfo LocalChain); + bool addFieldToUninits(FieldChainInfo LocalChain, + const MemRegion *PointeeR = nullptr); }; /// Returns true if T is a primitive type. An object of a primitive type only diff --git a/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp b/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp index 50ab7c0a0e..94f664ab93 100644 --- a/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp @@ -28,9 +28,14 @@ using namespace clang; using namespace clang::ento; +/// We'll mark fields (and pointee of fields) that are confirmed to be +/// uninitialized as already analyzed. +REGISTER_SET_WITH_PROGRAMSTATE(AnalyzedRegions, const MemRegion *) + namespace { -class UninitializedObjectChecker : public Checker<check::EndFunction> { +class UninitializedObjectChecker + : public Checker<check::EndFunction, check::DeadSymbols> { std::unique_ptr<BuiltinBug> BT_uninitField; public: @@ -39,7 +44,9 @@ public: UninitializedObjectChecker() : BT_uninitField(new BuiltinBug(this, "Uninitialized fields")) {} + void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const; + void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const; }; /// A basic field type, that is not a pointer or a reference, it's dynamic and @@ -96,12 +103,11 @@ public: // Utility function declarations. -/// Returns the object that was constructed by CtorDecl, or None if that isn't -/// possible. -// TODO: Refactor this function so that it returns the constructed object's -// region. -static Optional<nonloc::LazyCompoundVal> -getObjectVal(const CXXConstructorDecl *CtorDecl, CheckerContext &Context); +/// Returns the region that was constructed by CtorDecl, or nullptr if that +/// isn't possible. +static const TypedValueRegion * +getConstructedRegion(const CXXConstructorDecl *CtorDecl, + CheckerContext &Context); /// Checks whether the object constructed by \p Ctor will be analyzed later /// (e.g. if the object is a field of another object, in which case we'd check @@ -135,20 +141,26 @@ void UninitializedObjectChecker::checkEndFunction( if (willObjectBeAnalyzedLater(CtorDecl, Context)) return; - Optional<nonloc::LazyCompoundVal> Object = getObjectVal(CtorDecl, Context); - if (!Object) + const TypedValueRegion *R = getConstructedRegion(CtorDecl, Context); + if (!R) return; - FindUninitializedFields F(Context.getState(), Object->getRegion(), Opts); + FindUninitializedFields F(Context.getState(), R, Opts); + + std::pair<ProgramStateRef, const UninitFieldMap &> UninitInfo = + F.getResults(); - const UninitFieldMap &UninitFields = F.getUninitFields(); + ProgramStateRef UpdatedState = UninitInfo.first; + const UninitFieldMap &UninitFields = UninitInfo.second; - if (UninitFields.empty()) + if (UninitFields.empty()) { + Context.addTransition(UpdatedState); return; + } // There are uninitialized fields in the record. - ExplodedNode *Node = Context.generateNonFatalErrorNode(Context.getState()); + ExplodedNode *Node = Context.generateNonFatalErrorNode(UpdatedState); if (!Node) return; @@ -189,6 +201,15 @@ void UninitializedObjectChecker::checkEndFunction( Context.emitReport(std::move(Report)); } +void UninitializedObjectChecker::checkDeadSymbols(SymbolReaper &SR, + CheckerContext &C) const { + ProgramStateRef State = C.getState(); + for (const MemRegion *R : State->get<AnalyzedRegions>()) { + if (!SR.isLiveRegion(R)) + State = State->remove<AnalyzedRegions>(R); + } +} + //===----------------------------------------------------------------------===// // Methods for FindUninitializedFields. //===----------------------------------------------------------------------===// @@ -206,17 +227,34 @@ FindUninitializedFields::FindUninitializedFields( UninitFields.clear(); } -bool FindUninitializedFields::addFieldToUninits(FieldChainInfo Chain) { +bool FindUninitializedFields::addFieldToUninits(FieldChainInfo Chain, + const MemRegion *PointeeR) { + const FieldRegion *FR = Chain.getUninitRegion(); + + assert((PointeeR || !isDereferencableType(FR->getDecl()->getType())) && + "One must also pass the pointee region as a parameter for " + "dereferenceable fields!"); + + if (State->contains<AnalyzedRegions>(FR)) + return false; + + if (PointeeR) { + if (State->contains<AnalyzedRegions>(PointeeR)) { + return false; + } + State = State->add<AnalyzedRegions>(PointeeR); + } + + State = State->add<AnalyzedRegions>(FR); + if (State->getStateManager().getContext().getSourceManager().isInSystemHeader( - Chain.getUninitRegion()->getDecl()->getLocation())) + FR->getDecl()->getLocation())) return false; UninitFieldMap::mapped_type NoteMsgBuf; llvm::raw_svector_ostream OS(NoteMsgBuf); Chain.printNoteMsg(OS); - return UninitFields - .insert(std::make_pair(Chain.getUninitRegion(), std::move(NoteMsgBuf))) - .second; + return UninitFields.insert({FR, std::move(NoteMsgBuf)}).second; } bool FindUninitializedFields::isNonUnionUninit(const TypedValueRegion *R, @@ -400,25 +438,27 @@ static void printTail(llvm::raw_ostream &Out, // Utility functions. //===----------------------------------------------------------------------===// -static Optional<nonloc::LazyCompoundVal> -getObjectVal(const CXXConstructorDecl *CtorDecl, CheckerContext &Context) { +static const TypedValueRegion * +getConstructedRegion(const CXXConstructorDecl *CtorDecl, + CheckerContext &Context) { - Loc ThisLoc = Context.getSValBuilder().getCXXThis(CtorDecl->getParent(), + Loc ThisLoc = Context.getSValBuilder().getCXXThis(CtorDecl, Context.getStackFrame()); - // Getting the value for 'this'. - SVal This = Context.getState()->getSVal(ThisLoc); - // Getting the value for '*this'. - SVal Object = Context.getState()->getSVal(This.castAs<Loc>()); + SVal ObjectV = Context.getState()->getSVal(ThisLoc); + + auto *R = ObjectV.getAsRegion()->getAs<TypedValueRegion>(); + if (R && !R->getValueType()->getAsCXXRecordDecl()) + return nullptr; - return Object.getAs<nonloc::LazyCompoundVal>(); + return R; } static bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor, CheckerContext &Context) { - Optional<nonloc::LazyCompoundVal> CurrentObject = getObjectVal(Ctor, Context); - if (!CurrentObject) + const TypedValueRegion *CurrRegion = getConstructedRegion(Ctor, Context); + if (!CurrRegion) return false; const LocationContext *LC = Context.getLocationContext(); @@ -429,14 +469,14 @@ static bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor, if (!OtherCtor) continue; - Optional<nonloc::LazyCompoundVal> OtherObject = - getObjectVal(OtherCtor, Context); - if (!OtherObject) + const TypedValueRegion *OtherRegion = + getConstructedRegion(OtherCtor, Context); + if (!OtherRegion) continue; - // If the CurrentObject is a subregion of OtherObject, it will be analyzed - // during the analysis of OtherObject. - if (CurrentObject->getRegion()->isSubRegionOf(OtherObject->getRegion())) + // If the CurrRegion is a subregion of OtherRegion, it will be analyzed + // during the analysis of OtherRegion. + if (CurrRegion->isSubRegionOf(OtherRegion)) return true; } @@ -487,12 +527,12 @@ void ento::registerUninitializedObjectChecker(CheckerManager &Mgr) { UninitObjCheckerOptions &ChOpts = Chk->Opts; ChOpts.IsPedantic = - AnOpts.getBooleanOption("Pedantic", /*DefaultVal*/ false, Chk); + AnOpts.getCheckerBooleanOption("Pedantic", /*DefaultVal*/ false, Chk); ChOpts.ShouldConvertNotesToWarnings = - AnOpts.getBooleanOption("NotesAsWarnings", /*DefaultVal*/ false, Chk); - ChOpts.CheckPointeeInitialization = AnOpts.getBooleanOption( + AnOpts.getCheckerBooleanOption("NotesAsWarnings", /*DefaultVal*/ false, Chk); + ChOpts.CheckPointeeInitialization = AnOpts.getCheckerBooleanOption( "CheckPointeeInitialization", /*DefaultVal*/ false, Chk); ChOpts.IgnoredRecordsWithFieldPattern = - AnOpts.getOptionAsString("IgnoreRecordsWithField", + AnOpts.getCheckerStringOption("IgnoreRecordsWithField", /*DefaultVal*/ "", Chk); } diff --git a/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp b/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp index 623ba6b3ff..ae53f00b0b 100644 --- a/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp +++ b/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp @@ -89,15 +89,39 @@ public: } }; +/// Represents a Loc field that points to itself. +class CyclicLocField final : public FieldNode { + +public: + CyclicLocField(const FieldRegion *FR) : FieldNode(FR) {} + + virtual void printNoteMsg(llvm::raw_ostream &Out) const override { + Out << "object references itself "; + } + + virtual void printPrefix(llvm::raw_ostream &Out) const override {} + + virtual void printNode(llvm::raw_ostream &Out) const override { + Out << getVariableName(getDecl()); + } + + virtual void printSeparator(llvm::raw_ostream &Out) const override { + llvm_unreachable("CyclicLocField objects must be the last node of the " + "fieldchain!"); + } +}; + } // end of anonymous namespace // Utility function declarations. -/// Returns whether \p T can be (transitively) dereferenced to a void pointer -/// type (void*, void**, ...). -static bool isVoidPointer(QualType T); - -using DereferenceInfo = std::pair<const TypedValueRegion *, bool>; +struct DereferenceInfo { + const TypedValueRegion *R; + const bool NeedsCastBack; + const bool IsCyclic; + DereferenceInfo(const TypedValueRegion *R, bool NCB, bool IC) + : R(R), NeedsCastBack(NCB), IsCyclic(IC) {} +}; /// Dereferences \p FR and returns with the pointee's region, and whether it /// needs to be casted back to it's location type. If for whatever reason @@ -105,6 +129,10 @@ using DereferenceInfo = std::pair<const TypedValueRegion *, bool>; static llvm::Optional<DereferenceInfo> dereference(ProgramStateRef State, const FieldRegion *FR); +/// Returns whether \p T can be (transitively) dereferenced to a void pointer +/// type (void*, void**, ...). +static bool isVoidPointer(QualType T); + //===----------------------------------------------------------------------===// // Methods for FindUninitializedFields. //===----------------------------------------------------------------------===// @@ -116,7 +144,7 @@ bool FindUninitializedFields::isDereferencableUninit( assert((isDereferencableType(FR->getDecl()->getType()) || V.getAs<nonloc::LocAsInteger>()) && - "This method only checks dereferencable objects!"); + "This method only checks dereferenceable objects!"); if (V.isUnknown() || V.getAs<loc::ConcreteInt>()) { IsAnyFieldInitialized = true; @@ -125,7 +153,7 @@ bool FindUninitializedFields::isDereferencableUninit( if (V.isUndef()) { return addFieldToUninits( - LocalChain.add(LocField(FR, /*IsDereferenced*/ false))); + LocalChain.add(LocField(FR, /*IsDereferenced*/ false)), FR); } if (!Opts.CheckPointeeInitialization) { @@ -141,8 +169,11 @@ bool FindUninitializedFields::isDereferencableUninit( return false; } - const TypedValueRegion *R = DerefInfo->first; - const bool NeedsCastBack = DerefInfo->second; + if (DerefInfo->IsCyclic) + return addFieldToUninits(LocalChain.add(CyclicLocField(FR)), FR); + + const TypedValueRegion *R = DerefInfo->R; + const bool NeedsCastBack = DerefInfo->NeedsCastBack; QualType DynT = R->getLocationType(); QualType PointeeT = DynT->getPointeeType(); @@ -156,8 +187,9 @@ bool FindUninitializedFields::isDereferencableUninit( if (PointeeT->isUnionType()) { if (isUnionUninit(R)) { if (NeedsCastBack) - return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT))); - return addFieldToUninits(LocalChain.add(LocField(FR))); + return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT)), + R); + return addFieldToUninits(LocalChain.add(LocField(FR)), R); } else { IsAnyFieldInitialized = true; return false; @@ -177,8 +209,8 @@ bool FindUninitializedFields::isDereferencableUninit( if (isPrimitiveUninit(PointeeV)) { if (NeedsCastBack) - return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT))); - return addFieldToUninits(LocalChain.add(LocField(FR))); + return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT)), R); + return addFieldToUninits(LocalChain.add(LocField(FR)), R); } IsAnyFieldInitialized = true; @@ -189,15 +221,6 @@ bool FindUninitializedFields::isDereferencableUninit( // Utility functions. //===----------------------------------------------------------------------===// -static bool isVoidPointer(QualType T) { - while (!T.isNull()) { - if (T->isVoidPointerType()) - return true; - T = T->getPointeeType(); - } - return false; -} - static llvm::Optional<DereferenceInfo> dereference(ProgramStateRef State, const FieldRegion *FR) { @@ -229,9 +252,8 @@ static llvm::Optional<DereferenceInfo> dereference(ProgramStateRef State, return None; // We found a cyclic pointer, like int *ptr = (int *)&ptr. - // TODO: Should we report these fields too? if (!VisitedRegions.insert(R).second) - return None; + return DereferenceInfo{R, NeedsCastBack, /*IsCyclic*/ true}; DynT = R->getLocationType(); // In order to ensure that this loop terminates, we're also checking the @@ -248,5 +270,14 @@ static llvm::Optional<DereferenceInfo> dereference(ProgramStateRef State, R = R->getSuperRegion()->getAs<TypedValueRegion>(); } - return std::make_pair(R, NeedsCastBack); + return DereferenceInfo{R, NeedsCastBack, /*IsCyclic*/ false}; +} + +static bool isVoidPointer(QualType T) { + while (!T.isNull()) { + if (T->isVoidPointerType()) + return true; + T = T->getPointeeType(); + } + return false; } diff --git a/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp b/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp index a6b50dc377..baf9aa0b57 100644 --- a/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp @@ -314,7 +314,7 @@ bool UnixAPIChecker::ReportZeroByteAllocation(CheckerContext &C, auto report = llvm::make_unique<BugReport>(*BT_mallocZero, os.str(), N); report->addRange(arg->getSourceRange()); - bugreporter::trackNullOrUndefValue(N, arg, *report); + bugreporter::trackExpressionValue(N, arg, *report); C.emitReport(std::move(report)); return true; diff --git a/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp b/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp index 6c67aa4e31..f879891703 100644 --- a/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp @@ -232,7 +232,7 @@ bool UnreachableCodeChecker::isInvalidPath(const CFGBlock *CB, if (!pred) return false; - // Get the predecessor block's terminator conditon + // Get the predecessor block's terminator condition const Stmt *cond = pred->getTerminatorCondition(); //assert(cond && "CFGBlock's predecessor has a terminator condition"); diff --git a/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp b/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp index 2584f20118..58ed463476 100644 --- a/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp @@ -74,7 +74,7 @@ void VLASizeChecker::reportBug( auto report = llvm::make_unique<BugReport>(*BT, os.str(), N); report->addVisitor(std::move(Visitor)); report->addRange(SizeE->getSourceRange()); - bugreporter::trackNullOrUndefValue(N, SizeE, *report); + bugreporter::trackExpressionValue(N, SizeE, *report); C.emitReport(std::move(report)); } diff --git a/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp b/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp index cf673de6d4..902b325dec 100644 --- a/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp @@ -280,5 +280,6 @@ void ento::registerVirtualCallChecker(CheckerManager &mgr) { VirtualCallChecker *checker = mgr.registerChecker<VirtualCallChecker>(); checker->IsPureOnly = - mgr.getAnalyzerOptions().getBooleanOption("PureOnly", false, checker); + mgr.getAnalyzerOptions().getCheckerBooleanOption("PureOnly", false, + checker); } |