diff options
Diffstat (limited to 'lib/StaticAnalyzer/Checkers')
69 files changed, 3551 insertions, 725 deletions
diff --git a/lib/StaticAnalyzer/Checkers/AllocationDiagnostics.h b/lib/StaticAnalyzer/Checkers/AllocationDiagnostics.h index 048418ef62db..62b7fab0739a 100644 --- a/lib/StaticAnalyzer/Checkers/AllocationDiagnostics.h +++ b/lib/StaticAnalyzer/Checkers/AllocationDiagnostics.h @@ -18,7 +18,7 @@ namespace clang { namespace ento { -/// \brief Returns true if leak diagnostics should directly reference +/// Returns true if leak diagnostics should directly reference /// the allocatin site (where possible). /// /// The default is false. diff --git a/lib/StaticAnalyzer/Checkers/AllocationState.h b/lib/StaticAnalyzer/Checkers/AllocationState.h new file mode 100644 index 000000000000..a6908bd7a651 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/AllocationState.h @@ -0,0 +1,34 @@ +//===--- AllocationState.h ------------------------------------- *- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_ALLOCATIONSTATE_H +#define LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_ALLOCATIONSTATE_H + +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" + +namespace clang { +namespace ento { + +namespace allocation_state { + +ProgramStateRef markReleased(ProgramStateRef State, SymbolRef Sym, + const Expr *Origin); + +/// This function provides an additional visitor that augments the bug report +/// with information relevant to memory errors caused by the misuse of +/// AF_InnerBuffer symbols. +std::unique_ptr<BugReporterVisitor> getInnerPointerBRVisitor(SymbolRef Sym); + +} // end namespace allocation_state + +} // end namespace ento +} // end namespace clang + +#endif diff --git a/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp b/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp index 90d5c0e36a47..e4cdc500de6a 100644 --- a/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp @@ -15,8 +15,10 @@ //===----------------------------------------------------------------------===// #include "ClangSACheckers.h" +#include "clang/AST/ExprCXX.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" using namespace clang; @@ -29,8 +31,17 @@ class AnalysisOrderChecker check::PostStmt<CastExpr>, check::PreStmt<ArraySubscriptExpr>, check::PostStmt<ArraySubscriptExpr>, + check::PreStmt<CXXNewExpr>, + check::PostStmt<CXXNewExpr>, + check::PreStmt<OffsetOfExpr>, + check::PostStmt<OffsetOfExpr>, + check::PreCall, + check::PostCall, + check::NewAllocator, check::Bind, - check::RegionChanges> { + check::RegionChanges, + check::LiveSymbols> { + bool isCallbackEnabled(AnalyzerOptions &Opts, StringRef CallbackName) const { return Opts.getBooleanOption("*", false, this) || Opts.getBooleanOption(CallbackName, false, this); @@ -72,11 +83,60 @@ public: llvm::errs() << "PostStmt<ArraySubscriptExpr>\n"; } + void checkPreStmt(const CXXNewExpr *NE, CheckerContext &C) const { + if (isCallbackEnabled(C, "PreStmtCXXNewExpr")) + llvm::errs() << "PreStmt<CXXNewExpr>\n"; + } + + void checkPostStmt(const CXXNewExpr *NE, CheckerContext &C) const { + if (isCallbackEnabled(C, "PostStmtCXXNewExpr")) + llvm::errs() << "PostStmt<CXXNewExpr>\n"; + } + + void checkPreStmt(const OffsetOfExpr *OOE, CheckerContext &C) const { + if (isCallbackEnabled(C, "PreStmtOffsetOfExpr")) + llvm::errs() << "PreStmt<OffsetOfExpr>\n"; + } + + void checkPostStmt(const OffsetOfExpr *OOE, CheckerContext &C) const { + if (isCallbackEnabled(C, "PostStmtOffsetOfExpr")) + llvm::errs() << "PostStmt<OffsetOfExpr>\n"; + } + + void checkPreCall(const CallEvent &Call, CheckerContext &C) const { + if (isCallbackEnabled(C, "PreCall")) { + llvm::errs() << "PreCall"; + if (const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(Call.getDecl())) + llvm::errs() << " (" << ND->getQualifiedNameAsString() << ')'; + llvm::errs() << '\n'; + } + } + + void checkPostCall(const CallEvent &Call, CheckerContext &C) const { + if (isCallbackEnabled(C, "PostCall")) { + llvm::errs() << "PostCall"; + if (const NamedDecl *ND = dyn_cast_or_null<NamedDecl>(Call.getDecl())) + llvm::errs() << " (" << ND->getQualifiedNameAsString() << ')'; + llvm::errs() << '\n'; + } + } + + void checkNewAllocator(const CXXNewExpr *CNE, SVal Target, + CheckerContext &C) const { + if (isCallbackEnabled(C, "NewAllocator")) + llvm::errs() << "NewAllocator\n"; + } + void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &C) const { if (isCallbackEnabled(C, "Bind")) llvm::errs() << "Bind\n"; } + void checkLiveSymbols(ProgramStateRef State, SymbolReaper &SymReaper) const { + if (isCallbackEnabled(State, "LiveSymbols")) + llvm::errs() << "LiveSymbols\n"; + } + ProgramStateRef checkRegionChanges(ProgramStateRef State, const InvalidatedSymbols *Invalidated, diff --git a/lib/StaticAnalyzer/Checkers/AnalyzerStatsChecker.cpp b/lib/StaticAnalyzer/Checkers/AnalyzerStatsChecker.cpp index 64c30e7a82c1..aadc6bac8d00 100644 --- a/lib/StaticAnalyzer/Checkers/AnalyzerStatsChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/AnalyzerStatsChecker.cpp @@ -122,6 +122,8 @@ void AnalyzerStatsChecker::checkEndAnalysis(ExplodedGraph &G, E = CE.blocks_exhausted_end(); I != E; ++I) { const BlockEdge &BE = I->first; const CFGBlock *Exit = BE.getDst(); + if (Exit->empty()) + continue; const CFGElement &CE = Exit->front(); if (Optional<CFGStmt> CS = CE.getAs<CFGStmt>()) { SmallString<128> bufI; diff --git a/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp b/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp index b944f90539d4..933380d494a4 100644 --- a/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp +++ b/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp @@ -33,8 +33,8 @@ class ArrayBoundCheckerV2 : enum OOB_Kind { OOB_Precedes, OOB_Excedes, OOB_Tainted }; - void reportOOB(CheckerContext &C, ProgramStateRef errorState, - OOB_Kind kind) const; + void reportOOB(CheckerContext &C, ProgramStateRef errorState, OOB_Kind kind, + std::unique_ptr<BugReporterVisitor> Visitor = nullptr) const; public: void checkLocation(SVal l, bool isLoad, const Stmt*S, @@ -125,7 +125,6 @@ void ArrayBoundCheckerV2::checkLocation(SVal location, bool isLoad, // have some flexibility in defining the base region, we can achieve // various levels of conservatism in our buffer overflow checking. ProgramStateRef state = checkerContext.getState(); - ProgramStateRef originalState = state; SValBuilder &svalBuilder = checkerContext.getSValBuilder(); const RegionRawOffsetV2 &rawOffset = @@ -205,8 +204,10 @@ void ArrayBoundCheckerV2::checkLocation(SVal location, bool isLoad, // If we are under constrained and the index variables are tainted, report. if (state_exceedsUpperBound && state_withinUpperBound) { - if (state->isTainted(rawOffset.getByteOffset())) { - reportOOB(checkerContext, state_exceedsUpperBound, OOB_Tainted); + SVal ByteOffset = rawOffset.getByteOffset(); + if (state->isTainted(ByteOffset)) { + reportOOB(checkerContext, state_exceedsUpperBound, OOB_Tainted, + llvm::make_unique<TaintBugVisitor>(ByteOffset)); return; } } else if (state_exceedsUpperBound) { @@ -222,13 +223,12 @@ void ArrayBoundCheckerV2::checkLocation(SVal location, bool isLoad, } while (false); - if (state != originalState) - checkerContext.addTransition(state); + checkerContext.addTransition(state); } -void ArrayBoundCheckerV2::reportOOB(CheckerContext &checkerContext, - ProgramStateRef errorState, - OOB_Kind kind) const { +void ArrayBoundCheckerV2::reportOOB( + CheckerContext &checkerContext, ProgramStateRef errorState, OOB_Kind kind, + std::unique_ptr<BugReporterVisitor> Visitor) const { ExplodedNode *errorNode = checkerContext.generateErrorNode(errorState); if (!errorNode) @@ -255,8 +255,9 @@ void ArrayBoundCheckerV2::reportOOB(CheckerContext &checkerContext, break; } - checkerContext.emitReport( - llvm::make_unique<BugReport>(*BT, os.str(), errorNode)); + auto BR = llvm::make_unique<BugReport>(*BT, os.str(), errorNode); + BR->addVisitor(std::move(Visitor)); + checkerContext.emitReport(std::move(BR)); } #ifndef NDEBUG diff --git a/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp b/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp index 371187747f03..7d6358acbbac 100644 --- a/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp +++ b/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp @@ -436,8 +436,7 @@ void CFNumberChecker::checkPreStmt(const CallExpr *CE, return; // Get the value of the "theType" argument. - const LocationContext *LCtx = C.getLocationContext(); - SVal TheTypeVal = state->getSVal(CE->getArg(1), LCtx); + SVal TheTypeVal = C.getSVal(CE->getArg(1)); // FIXME: We really should allow ranges of valid theType values, and // bifurcate the state appropriately. @@ -457,7 +456,7 @@ void CFNumberChecker::checkPreStmt(const CallExpr *CE, // Look at the value of the integer being passed by reference. Essentially // we want to catch cases where the value passed in is not equal to the // size of the type being created. - SVal TheValueExpr = state->getSVal(CE->getArg(2), LCtx); + SVal TheValueExpr = C.getSVal(CE->getArg(2)); // FIXME: Eventually we should handle arbitrary locations. We can do this // by having an enhanced memory model that does low-level typing. @@ -571,7 +570,7 @@ void CFRetainReleaseChecker::checkPreStmt(const CallExpr *CE, // Get the argument's value. const Expr *Arg = CE->getArg(0); - SVal ArgVal = state->getSVal(Arg, C.getLocationContext()); + SVal ArgVal = C.getSVal(Arg); Optional<DefinedSVal> DefArgVal = ArgVal.getAs<DefinedSVal>(); if (!DefArgVal) return; @@ -977,8 +976,7 @@ assumeCollectionNonEmpty(CheckerContext &C, ProgramStateRef State, if (!State) return nullptr; - SymbolRef CollectionS = - State->getSVal(FCS->getCollection(), C.getLocationContext()).getAsSymbol(); + SymbolRef CollectionS = C.getSVal(FCS->getCollection()).getAsSymbol(); return assumeCollectionNonEmpty(C, State, CollectionS, Assumption); } @@ -1166,7 +1164,7 @@ void ObjCLoopChecker::checkDeadSymbols(SymbolReaper &SymReaper, namespace { /// \class ObjCNonNilReturnValueChecker -/// \brief The checker restricts the return values of APIs known to +/// The checker restricts the return values of APIs known to /// never (or almost never) return 'nil'. class ObjCNonNilReturnValueChecker : public Checker<check::PostObjCMessage, @@ -1206,7 +1204,7 @@ ProgramStateRef ObjCNonNilReturnValueChecker::assumeExprIsNonNull(const Expr *NonNullExpr, ProgramStateRef State, CheckerContext &C) const { - SVal Val = State->getSVal(NonNullExpr, C.getLocationContext()); + SVal Val = C.getSVal(NonNullExpr); if (Optional<DefinedOrUnknownSVal> DV = Val.getAs<DefinedOrUnknownSVal>()) return State->assume(*DV, true); return State; diff --git a/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp b/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp index 097d4198800d..0e781d08e24c 100644 --- a/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp @@ -43,7 +43,7 @@ bool BuiltinFunctionChecker::evalCall(const CallExpr *CE, case Builtin::BI__builtin_assume: { assert (CE->arg_begin() != CE->arg_end()); - SVal ArgSVal = state->getSVal(CE->getArg(0), LCtx); + SVal ArgSVal = C.getSVal(CE->getArg(0)); if (ArgSVal.isUndef()) return true; // Return true to model purity. @@ -68,7 +68,7 @@ bool BuiltinFunctionChecker::evalCall(const CallExpr *CE, // __builtin_addressof is going from a reference to a pointer, but those // are represented the same way in the analyzer. assert (CE->arg_begin() != CE->arg_end()); - SVal X = state->getSVal(*(CE->arg_begin()), LCtx); + SVal X = C.getSVal(*(CE->arg_begin())); C.addTransition(state->BindExpr(CE, LCtx, X)); return true; } @@ -83,8 +83,7 @@ bool BuiltinFunctionChecker::evalCall(const CallExpr *CE, // Set the extent of the region in bytes. This enables us to use the // SVal of the argument directly. If we save the extent in bits, we // cannot represent values like symbol*8. - DefinedOrUnknownSVal Size = - state->getSVal(*(CE->arg_begin()), LCtx).castAs<DefinedOrUnknownSVal>(); + auto Size = C.getSVal(*(CE->arg_begin())).castAs<DefinedOrUnknownSVal>(); SValBuilder& svalBuilder = C.getSValBuilder(); DefinedOrUnknownSVal Extent = R->getExtent(svalBuilder); @@ -97,7 +96,8 @@ bool BuiltinFunctionChecker::evalCall(const CallExpr *CE, return true; } - case Builtin::BI__builtin_object_size: { + case Builtin::BI__builtin_object_size: + case Builtin::BI__builtin_constant_p: { // This must be resolvable at compile time, so we defer to the constant // evaluator for a value. SVal V = UnknownVal(); diff --git a/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 7ab9c6114eae..5bb4770b5675 100644 --- a/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -37,9 +37,11 @@ add_clang_library(clangStaticAnalyzerCheckers DynamicTypeChecker.cpp ExprInspectionChecker.cpp FixedAddressChecker.cpp + GCDAntipatternChecker.cpp GenericTaintChecker.cpp GTestChecker.cpp IdenticalExprChecker.cpp + InnerPointerChecker.cpp IteratorChecker.cpp IvarInvalidationChecker.cpp LLVMConventionsChecker.cpp @@ -49,6 +51,7 @@ add_clang_library(clangStaticAnalyzerCheckers MallocChecker.cpp MallocOverflowSecurityChecker.cpp MallocSizeofChecker.cpp + MmapWriteExecChecker.cpp MisusedMovedObjectChecker.cpp MPI-Checker/MPIBugReporter.cpp MPI-Checker/MPIChecker.cpp @@ -61,6 +64,7 @@ add_clang_library(clangStaticAnalyzerCheckers NullabilityChecker.cpp NumberObjectConversionChecker.cpp ObjCAtSyncChecker.cpp + ObjCAutoreleaseWriteChecker.cpp ObjCContainersASTChecker.cpp ObjCContainersChecker.cpp ObjCMissingSuperCallChecker.cpp @@ -75,6 +79,7 @@ add_clang_library(clangStaticAnalyzerCheckers RetainCountChecker.cpp ReturnPointerRangeChecker.cpp ReturnUndefChecker.cpp + RunLoopAutoreleaseLeakChecker.cpp SimpleStreamChecker.cpp StackAddrEscapeChecker.cpp StdLibraryFunctionsChecker.cpp @@ -82,11 +87,13 @@ add_clang_library(clangStaticAnalyzerCheckers TaintTesterChecker.cpp TestAfterDivZeroChecker.cpp TraversalChecker.cpp + TrustNonnullChecker.cpp UndefBranchChecker.cpp UndefCapturedBlockVarChecker.cpp UndefResultChecker.cpp UndefinedArraySubscriptChecker.cpp UndefinedAssignmentChecker.cpp + UninitializedObjectChecker.cpp UnixAPIChecker.cpp UnreachableCodeChecker.cpp VforkChecker.cpp diff --git a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 28ad7e9e5071..278452ec994a 100644 --- a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -97,14 +97,17 @@ public: void evalStrcpy(CheckerContext &C, const CallExpr *CE) const; void evalStrncpy(CheckerContext &C, const CallExpr *CE) const; void evalStpcpy(CheckerContext &C, const CallExpr *CE) const; + void evalStrlcpy(CheckerContext &C, const CallExpr *CE) const; void evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, bool returnEnd, bool isBounded, - bool isAppending) const; + bool isAppending, + bool returnPtr = true) const; void evalStrcat(CheckerContext &C, const CallExpr *CE) const; void evalStrncat(CheckerContext &C, const CallExpr *CE) const; + void evalStrlcat(CheckerContext &C, const CallExpr *CE) const; void evalStrcmp(CheckerContext &C, const CallExpr *CE) const; void evalStrncmp(CheckerContext &C, const CallExpr *CE) const; @@ -155,6 +158,10 @@ public: static bool SummarizeRegion(raw_ostream &os, ASTContext &Ctx, const MemRegion *MR); + static bool memsetAux(const Expr *DstBuffer, const Expr *CharE, + const Expr *Size, CheckerContext &C, + ProgramStateRef &State); + // Re-usable checks ProgramStateRef checkNonNull(CheckerContext &C, ProgramStateRef state, @@ -194,6 +201,14 @@ public: const Stmt *First, const Stmt *Second) const; + void emitNullArgBug(CheckerContext &C, ProgramStateRef State, const Stmt *S, + StringRef WarningMsg) const; + void emitOutOfBoundsBug(CheckerContext &C, ProgramStateRef State, + const Stmt *S, StringRef WarningMsg) const; + void emitNotCStringBug(CheckerContext &C, ProgramStateRef State, + const Stmt *S, StringRef WarningMsg) const; + void emitAdditionOverflowBug(CheckerContext &C, ProgramStateRef State) const; + ProgramStateRef checkAdditionOverflow(CheckerContext &C, ProgramStateRef state, NonLoc left, @@ -239,30 +254,14 @@ ProgramStateRef CStringChecker::checkNonNull(CheckerContext &C, std::tie(stateNull, stateNonNull) = assumeZero(C, state, l, S->getType()); if (stateNull && !stateNonNull) { - if (!Filter.CheckCStringNullArg) - return nullptr; - - ExplodedNode *N = C.generateErrorNode(stateNull); - if (!N) - return nullptr; - - if (!BT_Null) - BT_Null.reset(new BuiltinBug( - Filter.CheckNameCStringNullArg, categories::UnixAPI, - "Null pointer argument in call to byte string function")); - - SmallString<80> buf; - llvm::raw_svector_ostream os(buf); - assert(CurrentFunctionDescription); - os << "Null pointer argument in call to " << CurrentFunctionDescription; - - // Generate a report for this bug. - BuiltinBug *BT = static_cast<BuiltinBug*>(BT_Null.get()); - auto report = llvm::make_unique<BugReport>(*BT, os.str(), N); + if (Filter.CheckCStringNullArg) { + SmallString<80> buf; + llvm::raw_svector_ostream os(buf); + assert(CurrentFunctionDescription); + os << "Null pointer argument in call to " << CurrentFunctionDescription; - report->addRange(S->getSourceRange()); - bugreporter::trackNullOrUndefValue(N, S, *report); - C.emitReport(std::move(report)); + emitNullArgBug(C, stateNull, S, os.str()); + } return nullptr; } @@ -305,21 +304,14 @@ ProgramStateRef CStringChecker::CheckLocation(CheckerContext &C, ProgramStateRef StInBound = state->assumeInBound(Idx, Size, true); ProgramStateRef StOutBound = state->assumeInBound(Idx, Size, false); if (StOutBound && !StInBound) { - ExplodedNode *N = C.generateErrorNode(StOutBound); - if (!N) + // These checks are either enabled by the CString out-of-bounds checker + // explicitly or implicitly by the Malloc checker. + // In the latter case we only do modeling but do not emit warning. + if (!Filter.CheckCStringOutOfBounds) return nullptr; - - if (!BT_Bounds) { - BT_Bounds.reset(new BuiltinBug( - Filter.CheckNameCStringOutOfBounds, "Out-of-bound array access", - "Byte string function accesses out-of-bound array element")); - } - BuiltinBug *BT = static_cast<BuiltinBug*>(BT_Bounds.get()); - - // Generate a report for this bug. - std::unique_ptr<BugReport> report; + // Emit a bug report. if (warningMsg) { - report = llvm::make_unique<BugReport>(*BT, warningMsg, N); + emitOutOfBoundsBug(C, StOutBound, S, warningMsg); } else { assert(CurrentFunctionDescription); assert(CurrentFunctionDescription[0] != '\0'); @@ -329,15 +321,8 @@ ProgramStateRef CStringChecker::CheckLocation(CheckerContext &C, os << toUppercase(CurrentFunctionDescription[0]) << &CurrentFunctionDescription[1] << " accesses out-of-bound array element"; - report = llvm::make_unique<BugReport>(*BT, os.str(), N); + emitOutOfBoundsBug(C, StOutBound, S, os.str()); } - - // FIXME: It would be nice to eventually make this diagnostic more clear, - // e.g., by referencing the original declaration or by saying *why* this - // reference is outside the range. - - report->addRange(S->getSourceRange()); - C.emitReport(std::move(report)); return nullptr; } @@ -366,7 +351,7 @@ ProgramStateRef CStringChecker::CheckBufferAccess(CheckerContext &C, QualType PtrTy = Ctx.getPointerType(Ctx.CharTy); // Check that the first buffer is non-null. - SVal BufVal = state->getSVal(FirstBuf, LCtx); + SVal BufVal = C.getSVal(FirstBuf); state = checkNonNull(C, state, FirstBuf, BufVal); if (!state) return nullptr; @@ -378,15 +363,17 @@ ProgramStateRef CStringChecker::CheckBufferAccess(CheckerContext &C, // Get the access length and make sure it is known. // FIXME: This assumes the caller has already checked that the access length // is positive. And that it's unsigned. - SVal LengthVal = state->getSVal(Size, LCtx); + SVal LengthVal = C.getSVal(Size); Optional<NonLoc> Length = LengthVal.getAs<NonLoc>(); if (!Length) return state; // Compute the offset of the last element to be accessed: size-1. NonLoc One = svalBuilder.makeIntVal(1, sizeTy).castAs<NonLoc>(); - NonLoc LastOffset = svalBuilder - .evalBinOpNN(state, BO_Sub, *Length, One, sizeTy).castAs<NonLoc>(); + SVal Offset = svalBuilder.evalBinOpNN(state, BO_Sub, *Length, One, sizeTy); + if (Offset.isUnknown()) + return nullptr; + NonLoc LastOffset = Offset.castAs<NonLoc>(); // Check that the first buffer is sufficiently long. SVal BufStart = svalBuilder.evalCast(BufVal, PtrTy, FirstBuf->getType()); @@ -555,6 +542,79 @@ void CStringChecker::emitOverlapBug(CheckerContext &C, ProgramStateRef state, C.emitReport(std::move(report)); } +void CStringChecker::emitNullArgBug(CheckerContext &C, ProgramStateRef State, + const Stmt *S, StringRef WarningMsg) const { + if (ExplodedNode *N = C.generateErrorNode(State)) { + if (!BT_Null) + BT_Null.reset(new BuiltinBug( + Filter.CheckNameCStringNullArg, categories::UnixAPI, + "Null pointer argument in call to byte string function")); + + BuiltinBug *BT = static_cast<BuiltinBug *>(BT_Null.get()); + auto Report = llvm::make_unique<BugReport>(*BT, WarningMsg, N); + bugreporter::trackNullOrUndefValue(N, S, *Report); + C.emitReport(std::move(Report)); + } +} + +void CStringChecker::emitOutOfBoundsBug(CheckerContext &C, + ProgramStateRef State, const Stmt *S, + StringRef WarningMsg) const { + if (ExplodedNode *N = C.generateErrorNode(State)) { + if (!BT_Bounds) + BT_Bounds.reset(new BuiltinBug( + Filter.CheckCStringOutOfBounds ? Filter.CheckNameCStringOutOfBounds + : Filter.CheckNameCStringNullArg, + "Out-of-bound array access", + "Byte string function accesses out-of-bound array element")); + + BuiltinBug *BT = static_cast<BuiltinBug *>(BT_Bounds.get()); + + // FIXME: It would be nice to eventually make this diagnostic more clear, + // e.g., by referencing the original declaration or by saying *why* this + // reference is outside the range. + auto Report = llvm::make_unique<BugReport>(*BT, WarningMsg, N); + Report->addRange(S->getSourceRange()); + C.emitReport(std::move(Report)); + } +} + +void CStringChecker::emitNotCStringBug(CheckerContext &C, ProgramStateRef State, + const Stmt *S, + StringRef WarningMsg) const { + if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) { + if (!BT_NotCString) + BT_NotCString.reset(new BuiltinBug( + Filter.CheckNameCStringNotNullTerm, categories::UnixAPI, + "Argument is not a null-terminated string.")); + + auto Report = llvm::make_unique<BugReport>(*BT_NotCString, WarningMsg, N); + + Report->addRange(S->getSourceRange()); + C.emitReport(std::move(Report)); + } +} + +void CStringChecker::emitAdditionOverflowBug(CheckerContext &C, + ProgramStateRef State) const { + if (ExplodedNode *N = C.generateErrorNode(State)) { + if (!BT_NotCString) + BT_NotCString.reset( + new BuiltinBug(Filter.CheckNameCStringOutOfBounds, "API", + "Sum of expressions causes overflow.")); + + // This isn't a great error message, but this should never occur in real + // code anyway -- you'd have to create a buffer longer than a size_t can + // represent, which is sort of a contradiction. + const char *WarningMsg = + "This expression will create a string whose length is too big to " + "be represented as a size_t"; + + auto Report = llvm::make_unique<BugReport>(*BT_NotCString, WarningMsg, N); + C.emitReport(std::move(Report)); + } +} + ProgramStateRef CStringChecker::checkAdditionOverflow(CheckerContext &C, ProgramStateRef state, NonLoc left, @@ -598,26 +658,7 @@ ProgramStateRef CStringChecker::checkAdditionOverflow(CheckerContext &C, if (stateOverflow && !stateOkay) { // We have an overflow. Emit a bug report. - ExplodedNode *N = C.generateErrorNode(stateOverflow); - if (!N) - return nullptr; - - if (!BT_AdditionOverflow) - BT_AdditionOverflow.reset( - new BuiltinBug(Filter.CheckNameCStringOutOfBounds, "API", - "Sum of expressions causes overflow")); - - // This isn't a great error message, but this should never occur in real - // code anyway -- you'd have to create a buffer longer than a size_t can - // represent, which is sort of a contradiction. - const char *warning = - "This expression will create a string whose length is too big to " - "be represented as a size_t"; - - // Generate a report for this bug. - C.emitReport( - llvm::make_unique<BugReport>(*BT_AdditionOverflow, warning, N)); - + emitAdditionOverflowBug(C, stateOverflow); return nullptr; } @@ -717,15 +758,7 @@ SVal CStringChecker::getCStringLength(CheckerContext &C, ProgramStateRef &state, // C string. In the context of locations, the only time we can issue such // a warning is for labels. if (Optional<loc::GotoLabel> Label = Buf.getAs<loc::GotoLabel>()) { - if (!Filter.CheckCStringNotNullTerm) - return UndefinedVal(); - - if (ExplodedNode *N = C.generateNonFatalErrorNode(state)) { - if (!BT_NotCString) - BT_NotCString.reset(new BuiltinBug( - Filter.CheckNameCStringNotNullTerm, categories::UnixAPI, - "Argument is not a null-terminated string.")); - + if (Filter.CheckCStringNotNullTerm) { SmallString<120> buf; llvm::raw_svector_ostream os(buf); assert(CurrentFunctionDescription); @@ -733,14 +766,9 @@ SVal CStringChecker::getCStringLength(CheckerContext &C, ProgramStateRef &state, << " is the address of the label '" << Label->getLabel()->getName() << "', which is not a null-terminated string"; - // Generate a report for this bug. - auto report = llvm::make_unique<BugReport>(*BT_NotCString, os.str(), N); - - report->addRange(Ex->getSourceRange()); - C.emitReport(std::move(report)); + emitNotCStringBug(C, state, Ex, os.str()); } return UndefinedVal(); - } // If it's not a region and not a label, give up. @@ -777,15 +805,7 @@ SVal CStringChecker::getCStringLength(CheckerContext &C, ProgramStateRef &state, // Other regions (mostly non-data) can't have a reliable C string length. // In this case, an error is emitted and UndefinedVal is returned. // The caller should always be prepared to handle this case. - if (!Filter.CheckCStringNotNullTerm) - return UndefinedVal(); - - if (ExplodedNode *N = C.generateNonFatalErrorNode(state)) { - if (!BT_NotCString) - BT_NotCString.reset(new BuiltinBug( - Filter.CheckNameCStringNotNullTerm, categories::UnixAPI, - "Argument is not a null-terminated string.")); - + if (Filter.CheckCStringNotNullTerm) { SmallString<120> buf; llvm::raw_svector_ostream os(buf); @@ -797,13 +817,8 @@ SVal CStringChecker::getCStringLength(CheckerContext &C, ProgramStateRef &state, else os << "not a null-terminated string"; - // Generate a report for this bug. - auto report = llvm::make_unique<BugReport>(*BT_NotCString, os.str(), N); - - report->addRange(Ex->getSourceRange()); - C.emitReport(std::move(report)); + emitNotCStringBug(C, state, Ex, os.str()); } - return UndefinedVal(); } } @@ -852,9 +867,10 @@ bool CStringChecker::IsFirstBufInBound(CheckerContext &C, // Compute the offset of the last element to be accessed: size-1. NonLoc One = svalBuilder.makeIntVal(1, sizeTy).castAs<NonLoc>(); - NonLoc LastOffset = - svalBuilder.evalBinOpNN(state, BO_Sub, *Length, One, sizeTy) - .castAs<NonLoc>(); + SVal Offset = svalBuilder.evalBinOpNN(state, BO_Sub, *Length, One, sizeTy); + if (Offset.isUnknown()) + return true; // cf top comment + NonLoc LastOffset = Offset.castAs<NonLoc>(); // Check that the first buffer is sufficiently long. SVal BufStart = svalBuilder.evalCast(BufVal, PtrTy, FirstBuf->getType()); @@ -987,6 +1003,95 @@ bool CStringChecker::SummarizeRegion(raw_ostream &os, ASTContext &Ctx, } } +bool CStringChecker::memsetAux(const Expr *DstBuffer, const Expr *CharE, + 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) + return false; + + // We're about to model memset by producing a "default binding" in the Store. + // Our current implementation - RegionStore - doesn't support default bindings + // that don't cover the whole base region. So we should first get the offset + // and the base region to figure out whether the offset of buffer is 0. + RegionOffset Offset = MR->getAsOffset(); + const MemRegion *BR = Offset.getRegion(); + + Optional<NonLoc> SizeNL = SizeVal.getAs<NonLoc>(); + if (!SizeNL) + return false; + + SValBuilder &svalBuilder = C.getSValBuilder(); + ASTContext &Ctx = C.getASTContext(); + + // void *memset(void *dest, int ch, size_t count); + // For now we can only handle the case of offset is 0 and concrete char value. + if (Offset.isValid() && !Offset.hasSymbolicOffset() && + Offset.getOffset() == 0) { + // Get the base region's extent. + auto *SubReg = cast<SubRegion>(BR); + DefinedOrUnknownSVal Extent = SubReg->getExtent(svalBuilder); + + ProgramStateRef StateWholeReg, StateNotWholeReg; + std::tie(StateWholeReg, StateNotWholeReg) = + State->assume(svalBuilder.evalEQ(State, Extent, *SizeNL)); + + // With the semantic of 'memset()', we should convert the CharVal to + // unsigned char. + CharVal = svalBuilder.evalCast(CharVal, Ctx.UnsignedCharTy, Ctx.IntTy); + + ProgramStateRef StateNullChar, StateNonNullChar; + std::tie(StateNullChar, StateNonNullChar) = + assumeZero(C, State, CharVal, Ctx.UnsignedCharTy); + + if (StateWholeReg && !StateNotWholeReg && StateNullChar && + !StateNonNullChar) { + // If the 'memset()' acts on the whole region of destination buffer and + // the value of the second argument of 'memset()' is zero, bind the second + // argument's value to the destination buffer with 'default binding'. + // FIXME: Since there is no perfect way to bind the non-zero character, we + // can only deal with zero value here. In the future, we need to deal with + // the binding of non-zero value in the case of whole region. + State = State->bindDefaultZero(svalBuilder.makeLoc(BR), + C.getLocationContext()); + } else { + // If the destination buffer's extent is not equal to the value of + // third argument, just invalidate buffer. + State = InvalidateBuffer(C, State, DstBuffer, MemVal, + /*IsSourceBuffer*/ false, Size); + } + + if (StateNullChar && !StateNonNullChar) { + // If the value of the second argument of 'memset()' is zero, set the + // string length of destination buffer to 0 directly. + State = setCStringLength(State, MR, + svalBuilder.makeZeroVal(Ctx.getSizeType())); + } else if (!StateNullChar && StateNonNullChar) { + SVal NewStrLen = svalBuilder.getMetadataSymbolVal( + CStringChecker::getTag(), MR, DstBuffer, Ctx.getSizeType(), + C.getLocationContext(), C.blockCount()); + + // If the value of second argument is not zero, then the string length + // is at least the size argument. + SVal NewStrLenGESize = svalBuilder.evalBinOp( + State, BO_GE, NewStrLen, SizeVal, svalBuilder.getConditionType()); + + State = setCStringLength( + State->assume(NewStrLenGESize.castAs<DefinedOrUnknownSVal>(), true), + MR, NewStrLen); + } + } else { + // If the offset is not zero and char value is not concrete, we can do + // nothing but invalidate the buffer. + State = InvalidateBuffer(C, State, DstBuffer, MemVal, + /*IsSourceBuffer*/ false, Size); + } + return true; +} + //===----------------------------------------------------------------------===// // evaluation of individual function calls. //===----------------------------------------------------------------------===// @@ -1384,6 +1489,18 @@ void CStringChecker::evalStpcpy(CheckerContext &C, const CallExpr *CE) const { /* isAppending = */ false); } +void CStringChecker::evalStrlcpy(CheckerContext &C, const CallExpr *CE) const { + if (CE->getNumArgs() < 3) + return; + + // char *strlcpy(char *dst, const char *src, size_t n); + evalStrcpyCommon(C, CE, + /* returnEnd = */ true, + /* isBounded = */ true, + /* isAppending = */ false, + /* returnPtr = */ false); +} + void CStringChecker::evalStrcat(CheckerContext &C, const CallExpr *CE) const { if (CE->getNumArgs() < 2) return; @@ -1406,9 +1523,21 @@ void CStringChecker::evalStrncat(CheckerContext &C, const CallExpr *CE) const { /* isAppending = */ true); } +void CStringChecker::evalStrlcat(CheckerContext &C, const CallExpr *CE) const { + if (CE->getNumArgs() < 3) + return; + + //char *strlcat(char *s1, const char *s2, size_t n); + evalStrcpyCommon(C, CE, + /* returnEnd = */ false, + /* isBounded = */ true, + /* isAppending = */ true, + /* returnPtr = */ false); +} + void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, bool returnEnd, bool isBounded, - bool isAppending) const { + bool isAppending, bool returnPtr) const { CurrentFunctionDescription = "string copy function"; ProgramStateRef state = C.getState(); const LocationContext *LCtx = C.getLocationContext(); @@ -1446,6 +1575,11 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, SVal maxLastElementIndex = UnknownVal(); const char *boundWarning = nullptr; + state = CheckOverlap(C, state, isBounded ? CE->getArg(2) : CE->getArg(1), Dst, srcExpr); + + if (!state) + return; + // If the function is strncpy, strncat, etc... it is bounded. if (isBounded) { // Get the max number of characters to copy. @@ -1518,7 +1652,11 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, // If the size is known to be zero, we're done. if (StateZeroSize && !StateNonZeroSize) { - StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, DstVal); + if (returnPtr) { + StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, DstVal); + } else { + StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, *lenValNL); + } C.addTransition(StateZeroSize); return; } @@ -1649,16 +1787,22 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, finalStrLength = amountCopied; } - // The final result of the function will either be a pointer past the last - // copied element, or a pointer to the start of the destination buffer. - SVal Result = (returnEnd ? UnknownVal() : DstVal); + SVal Result; + + if (returnPtr) { + // The final result of the function will either be a pointer past the last + // copied element, or a pointer to the start of the destination buffer. + Result = (returnEnd ? UnknownVal() : DstVal); + } else { + Result = finalStrLength; + } assert(state); // If the destination is a MemRegion, try to check for a buffer overflow and // record the new string length. if (Optional<loc::MemRegionVal> dstRegVal = - DstVal.getAs<loc::MemRegionVal>()) { + DstVal.getAs<loc::MemRegionVal>()) { QualType ptrTy = Dst->getType(); // If we have an exact value on a bounded copy, use that to check for @@ -1666,9 +1810,9 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, if (boundWarning) { if (Optional<NonLoc> maxLastNL = maxLastElementIndex.getAs<NonLoc>()) { SVal maxLastElement = svalBuilder.evalBinOpLN(state, BO_Add, *dstRegVal, - *maxLastNL, ptrTy); + *maxLastNL, ptrTy); state = CheckLocation(C, state, CE->getArg(2), maxLastElement, - boundWarning); + boundWarning); if (!state) return; } @@ -1677,7 +1821,7 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, // Then, if the final length is known... if (Optional<NonLoc> knownStrLength = finalStrLength.getAs<NonLoc>()) { SVal lastElement = svalBuilder.evalBinOpLN(state, BO_Add, *dstRegVal, - *knownStrLength, ptrTy); + *knownStrLength, ptrTy); // ...and we haven't checked the bound, we'll check the actual copy. if (!boundWarning) { @@ -1689,7 +1833,7 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, } // If this is a stpcpy-style copy, the last element is the return value. - if (returnEnd) + if (returnPtr && returnEnd) Result = lastElement; } @@ -1701,12 +1845,12 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, // This would probably remove any existing bindings past the end of the // string, but that's still an improvement over blank invalidation. state = InvalidateBuffer(C, state, Dst, *dstRegVal, - /*IsSourceBuffer*/false, nullptr); + /*IsSourceBuffer*/false, nullptr); // Invalidate the source (const-invalidation without const-pointer-escaping // the address of the top-level region). state = InvalidateBuffer(C, state, srcExpr, srcVal, /*IsSourceBuffer*/true, - nullptr); + nullptr); // Set the C string length of the destination, if we know it. if (isBounded && !isAppending) { @@ -1722,12 +1866,13 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, assert(state); - // If this is a stpcpy-style copy, but we were unable to check for a buffer - // overflow, we still need a result. Conjure a return value. - if (returnEnd && Result.isUnknown()) { - Result = svalBuilder.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount()); + if (returnPtr) { + // If this is a stpcpy-style copy, but we were unable to check for a buffer + // overflow, we still need a result. Conjure a return value. + if (returnEnd && Result.isUnknown()) { + Result = svalBuilder.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount()); + } } - // Set the return value. state = state->BindExpr(CE, LCtx, Result); C.addTransition(state); @@ -1750,7 +1895,7 @@ void CStringChecker::evalStrncmp(CheckerContext &C, const CallExpr *CE) const { } void CStringChecker::evalStrcasecmp(CheckerContext &C, - const CallExpr *CE) const { + const CallExpr *CE) const { if (CE->getNumArgs() < 2) return; @@ -1759,7 +1904,7 @@ void CStringChecker::evalStrcasecmp(CheckerContext &C, } void CStringChecker::evalStrncasecmp(CheckerContext &C, - const CallExpr *CE) const { + const CallExpr *CE) const { if (CE->getNumArgs() < 3) return; @@ -1768,7 +1913,7 @@ void CStringChecker::evalStrncasecmp(CheckerContext &C, } void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE, - bool isBounded, bool ignoreCase) const { + bool isBounded, bool ignoreCase) const { CurrentFunctionDescription = "string comparison function"; ProgramStateRef state = C.getState(); const LocationContext *LCtx = C.getLocationContext(); @@ -1813,7 +1958,7 @@ void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE, // and we only need to check one size. if (StSameBuf) { StSameBuf = StSameBuf->BindExpr(CE, LCtx, - svalBuilder.makeZeroVal(CE->getType())); + svalBuilder.makeZeroVal(CE->getType())); C.addTransition(StSameBuf); // If the two arguments are GUARANTEED to be the same, we're done! @@ -1832,7 +1977,7 @@ void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE, const StringLiteral *s2StrLiteral = getCStringLiteral(C, state, s2, s2Val); bool canComputeResult = false; SVal resultVal = svalBuilder.conjureSymbolVal(nullptr, CE, LCtx, - C.blockCount()); + C.blockCount()); if (s1StrLiteral && s2StrLiteral) { StringRef s1StrRef = s1StrLiteral->getString(); @@ -1867,7 +2012,7 @@ void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE, // Use StringRef's comparison methods to compute the actual result. int compareRes = ignoreCase ? s1StrRef.compare_lower(s2StrRef) - : s1StrRef.compare(s2StrRef); + : s1StrRef.compare(s2StrRef); // The strcmp function returns an integer greater than, equal to, or less // than zero, [c11, p7.24.4.2]. @@ -1881,7 +2026,7 @@ void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE, BinaryOperatorKind op = (compareRes == 1) ? BO_GT : BO_LT; SVal compareWithZero = svalBuilder.evalBinOp(state, op, resultVal, zeroVal, - svalBuilder.getConditionType()); + svalBuilder.getConditionType()); DefinedSVal compareWithZeroVal = compareWithZero.castAs<DefinedSVal>(); state = state->assume(compareWithZeroVal, true); } @@ -1933,17 +2078,17 @@ void CStringChecker::evalStrsep(CheckerContext &C, const CallExpr *CE) const { // Invalidate the search string, representing the change of one delimiter // character to NUL. State = InvalidateBuffer(C, State, SearchStrPtr, Result, - /*IsSourceBuffer*/false, nullptr); + /*IsSourceBuffer*/false, nullptr); // Overwrite the search string pointer. The new value is either an address // further along in the same string, or NULL if there are no more tokens. State = State->bindLoc(*SearchStrLoc, - SVB.conjureSymbolVal(getTag(), - CE, - LCtx, - CharPtrTy, - C.blockCount()), - LCtx); + SVB.conjureSymbolVal(getTag(), + CE, + LCtx, + CharPtrTy, + C.blockCount()), + LCtx); } else { assert(SearchStrVal.isUnknown()); // Conjure a symbolic value. It's the best we can do. @@ -1961,12 +2106,12 @@ void CStringChecker::evalStdCopy(CheckerContext &C, const CallExpr *CE) const { } void CStringChecker::evalStdCopyBackward(CheckerContext &C, - const CallExpr *CE) const { + const CallExpr *CE) const { evalStdCopyCommon(C, CE); } void CStringChecker::evalStdCopyCommon(CheckerContext &C, - const CallExpr *CE) const { + const CallExpr *CE) const { if (CE->getNumArgs() < 3) return; @@ -1983,7 +2128,7 @@ void CStringChecker::evalStdCopyCommon(CheckerContext &C, const Expr *Dst = CE->getArg(2); SVal DstVal = State->getSVal(Dst, LCtx); State = InvalidateBuffer(C, State, Dst, DstVal, /*IsSource=*/false, - /*Size=*/nullptr); + /*Size=*/nullptr); SValBuilder &SVB = C.getSValBuilder(); @@ -2000,6 +2145,7 @@ void CStringChecker::evalMemset(CheckerContext &C, const CallExpr *CE) const { CurrentFunctionDescription = "memory set function"; const Expr *Mem = CE->getArg(0); + const Expr *CharE = CE->getArg(1); const Expr *Size = CE->getArg(2); ProgramStateRef State = C.getState(); @@ -2032,9 +2178,11 @@ void CStringChecker::evalMemset(CheckerContext &C, const CallExpr *CE) const { State = CheckBufferAccess(C, State, Size, Mem); if (!State) return; - State = InvalidateBuffer(C, State, Mem, C.getSVal(Mem), - /*IsSourceBuffer*/false, Size); - if (!State) + + // 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)) return; State = State->BindExpr(CE, LCtx, MemVal); @@ -2082,10 +2230,14 @@ bool CStringChecker::evalCall(const CallExpr *CE, CheckerContext &C) const { evalFunction = &CStringChecker::evalStrncpy; else if (C.isCLibraryFunction(FDecl, "stpcpy")) evalFunction = &CStringChecker::evalStpcpy; + else if (C.isCLibraryFunction(FDecl, "strlcpy")) + evalFunction = &CStringChecker::evalStrlcpy; else if (C.isCLibraryFunction(FDecl, "strcat")) evalFunction = &CStringChecker::evalStrcat; else if (C.isCLibraryFunction(FDecl, "strncat")) evalFunction = &CStringChecker::evalStrncat; + else if (C.isCLibraryFunction(FDecl, "strlcat")) + evalFunction = &CStringChecker::evalStrlcat; else if (C.isCLibraryFunction(FDecl, "strlen")) evalFunction = &CStringChecker::evalstrLength; else if (C.isCLibraryFunction(FDecl, "strnlen")) @@ -2149,10 +2301,10 @@ void CStringChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const { if (!MR) continue; - SVal StrVal = state->getSVal(Init, C.getLocationContext()); + SVal StrVal = C.getSVal(Init); assert(StrVal.isValid() && "Initializer string is unknown or undefined"); DefinedOrUnknownSVal strLength = - getCStringLength(C, state, Init, StrVal).castAs<DefinedOrUnknownSVal>(); + getCStringLength(C, state, Init, StrVal).castAs<DefinedOrUnknownSVal>(); state = state->set<CStringLength>(MR, strLength); } @@ -2162,11 +2314,11 @@ void CStringChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const { ProgramStateRef CStringChecker::checkRegionChanges(ProgramStateRef state, - const InvalidatedSymbols *, - ArrayRef<const MemRegion *> ExplicitRegions, - ArrayRef<const MemRegion *> Regions, - const LocationContext *LCtx, - const CallEvent *Call) const { + const InvalidatedSymbols *, + ArrayRef<const MemRegion *> ExplicitRegions, + ArrayRef<const MemRegion *> Regions, + const LocationContext *LCtx, + const CallEvent *Call) const { CStringLengthTy Entries = state->get<CStringLength>(); if (Entries.isEmpty()) return state; @@ -2176,7 +2328,7 @@ CStringChecker::checkRegionChanges(ProgramStateRef state, // First build sets for the changed regions and their super-regions. for (ArrayRef<const MemRegion *>::iterator - I = Regions.begin(), E = Regions.end(); I != E; ++I) { + I = Regions.begin(), E = Regions.end(); I != E; ++I) { const MemRegion *MR = *I; Invalidated.insert(MR); @@ -2191,7 +2343,7 @@ CStringChecker::checkRegionChanges(ProgramStateRef state, // Then loop over the entries in the current state. for (CStringLengthTy::iterator I = Entries.begin(), - E = Entries.end(); I != E; ++I) { + E = Entries.end(); I != E; ++I) { const MemRegion *MR = I.getKey(); // Is this entry for a super-region of a changed region? @@ -2215,22 +2367,22 @@ CStringChecker::checkRegionChanges(ProgramStateRef state, } void CStringChecker::checkLiveSymbols(ProgramStateRef state, - SymbolReaper &SR) const { + SymbolReaper &SR) const { // Mark all symbols in our string length map as valid. CStringLengthTy Entries = state->get<CStringLength>(); for (CStringLengthTy::iterator I = Entries.begin(), E = Entries.end(); - I != E; ++I) { + I != E; ++I) { SVal Len = I.getData(); for (SymExpr::symbol_iterator si = Len.symbol_begin(), - se = Len.symbol_end(); si != se; ++si) + se = Len.symbol_end(); si != se; ++si) SR.markInUse(*si); } } void CStringChecker::checkDeadSymbols(SymbolReaper &SR, - CheckerContext &C) const { + CheckerContext &C) const { if (!SR.hasDeadSymbols()) return; @@ -2241,7 +2393,7 @@ void CStringChecker::checkDeadSymbols(SymbolReaper &SR, CStringLengthTy::Factory &F = state->get_context<CStringLength>(); for (CStringLengthTy::iterator I = Entries.begin(), E = Entries.end(); - I != E; ++I) { + I != E; ++I) { SVal Len = I.getData(); if (SymbolRef Sym = Len.getAsSymbol()) { if (SR.isDead(Sym)) @@ -2260,11 +2412,11 @@ void CStringChecker::checkDeadSymbols(SymbolReaper &SR, checker->Filter.CheckName##name = mgr.getCurrentCheckName(); \ } -REGISTER_CHECKER(CStringNullArg) -REGISTER_CHECKER(CStringOutOfBounds) -REGISTER_CHECKER(CStringBufferOverlap) + REGISTER_CHECKER(CStringNullArg) + REGISTER_CHECKER(CStringOutOfBounds) + REGISTER_CHECKER(CStringBufferOverlap) REGISTER_CHECKER(CStringNotNullTerm) -void ento::registerCStringCheckerBasic(CheckerManager &Mgr) { - registerCStringNullArg(Mgr); -} + void ento::registerCStringCheckerBasic(CheckerManager &Mgr) { + Mgr.registerChecker<CStringChecker>(); + } diff --git a/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp index 4b5e97b69295..8b4aa857e775 100644 --- a/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp @@ -80,6 +80,18 @@ class WalkAST: public StmtVisitor<WalkAST> { /// of bytes to copy. bool containsBadStrncatPattern(const CallExpr *CE); + /// Identify erroneous patterns in the last argument to strlcpy - the number + /// of bytes to copy. + /// The bad pattern checked is when the size is known + /// to be larger than the destination can handle. + /// char dst[2]; + /// size_t cpy = 4; + /// strlcpy(dst, "abcd", sizeof("abcd") - 1); + /// strlcpy(dst, "abcd", 4); + /// strlcpy(dst + 3, "abcd", 2); + /// strlcpy(dst, "abcd", cpy); + bool containsBadStrlcpyPattern(const CallExpr *CE); + public: WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC) : Checker(Checker), BR(BR), AC(AC) {} @@ -130,6 +142,54 @@ bool WalkAST::containsBadStrncatPattern(const CallExpr *CE) { return false; } +bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) { + if (CE->getNumArgs() != 3) + return false; + const Expr *DstArg = CE->getArg(0); + const Expr *LenArg = CE->getArg(2); + + const auto *DstArgDecl = dyn_cast<DeclRefExpr>(DstArg->IgnoreParenImpCasts()); + const auto *LenArgDecl = dyn_cast<DeclRefExpr>(LenArg->IgnoreParenLValueCasts()); + uint64_t DstOff = 0; + // - size_t dstlen = sizeof(dst) + if (LenArgDecl) { + const auto *LenArgVal = dyn_cast<VarDecl>(LenArgDecl->getDecl()); + if (LenArgVal->getInit()) + LenArg = LenArgVal->getInit(); + } + + // - integral value + // We try to figure out if the last argument is possibly longer + // than the destination can possibly handle if its size can be defined. + if (const auto *IL = dyn_cast<IntegerLiteral>(LenArg->IgnoreParenImpCasts())) { + uint64_t ILRawVal = IL->getValue().getZExtValue(); + + // Case when there is pointer arithmetic on the destination buffer + // especially when we offset from the base decreasing the + // buffer length accordingly. + if (!DstArgDecl) { + if (const auto *BE = dyn_cast<BinaryOperator>(DstArg->IgnoreParenImpCasts())) { + DstArgDecl = dyn_cast<DeclRefExpr>(BE->getLHS()->IgnoreParenImpCasts()); + if (BE->getOpcode() == BO_Add) { + if ((IL = dyn_cast<IntegerLiteral>(BE->getRHS()->IgnoreParenImpCasts()))) { + DstOff = IL->getValue().getZExtValue(); + } + } + } + } + if (DstArgDecl) { + if (const auto *Buffer = dyn_cast<ConstantArrayType>(DstArgDecl->getType())) { + ASTContext &C = BR.getContext(); + uint64_t BufferLen = C.getTypeSize(Buffer) / 8; + if ((BufferLen - DstOff) < ILRawVal) + return true; + } + } + } + + return false; +} + void WalkAST::VisitCallExpr(CallExpr *CE) { const FunctionDecl *FD = CE->getDirectCallee(); if (!FD) @@ -159,6 +219,25 @@ void WalkAST::VisitCallExpr(CallExpr *CE) { "C String API", os.str(), Loc, LenArg->getSourceRange()); } + } else if (CheckerContext::isCLibraryFunction(FD, "strlcpy")) { + if (containsBadStrlcpyPattern(CE)) { + const Expr *DstArg = CE->getArg(0); + const Expr *LenArg = CE->getArg(2); + PathDiagnosticLocation Loc = + PathDiagnosticLocation::createBegin(LenArg, BR.getSourceManager(), AC); + + StringRef DstName = getPrintableName(DstArg); + + SmallString<256> S; + llvm::raw_svector_ostream os(S); + os << "The third argument is larger than the size of the input buffer. "; + if (!DstName.empty()) + os << "Replace with the value 'sizeof(" << DstName << ")` or lower"; + + BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument", + "C String API", os.str(), Loc, + LenArg->getSourceRange()); + } } // Recurse and check children. diff --git a/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp b/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp index 668e772fe1b3..d1d37c75dfcc 100644 --- a/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp @@ -48,7 +48,7 @@ void CXXSelfAssignmentChecker::checkBeginFunction(CheckerContext &C) const { auto &State = C.getState(); auto &SVB = C.getSValBuilder(); auto ThisVal = - State->getSVal(SVB.getCXXThis(MD, LCtx->getCurrentStackFrame())); + State->getSVal(SVB.getCXXThis(MD, LCtx->getStackFrame())); auto Param = SVB.makeLoc(State->getRegion(MD->getParamDecl(0), LCtx)); auto ParamVal = State->getSVal(Param); ProgramStateRef SelfAssignState = State->bindLoc(Param, ThisVal, LCtx); diff --git a/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp b/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp index 3e178152d925..059553b21995 100644 --- a/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp @@ -101,7 +101,7 @@ void CastSizeChecker::checkPreStmt(const CastExpr *CE,CheckerContext &C) const { return; ProgramStateRef state = C.getState(); - const MemRegion *R = state->getSVal(E, C.getLocationContext()).getAsRegion(); + const MemRegion *R = C.getSVal(E).getAsRegion(); if (!R) return; diff --git a/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp b/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp index 65e81315f095..00e903355720 100644 --- a/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp @@ -78,7 +78,7 @@ bool CastToStructVisitor::VisitCastExpr(const CastExpr *CE) { // Don't warn for references const ValueDecl *VD = nullptr; if (const auto *SE = dyn_cast<DeclRefExpr>(U->getSubExpr())) - VD = dyn_cast<ValueDecl>(SE->getDecl()); + VD = SE->getDecl(); else if (const auto *SE = dyn_cast<MemberExpr>(U->getSubExpr())) VD = SE->getMemberDecl(); if (!VD || VD->getType()->isReferenceType()) diff --git a/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp b/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp index 2818c9d9fd4a..f4d2e32cef11 100644 --- a/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp +++ b/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp @@ -126,7 +126,7 @@ public: const CallEvent *Call, PointerEscapeKind Kind) const; void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const; - void checkEndFunction(CheckerContext &Ctx) const; + void checkEndFunction(const ReturnStmt *RS, CheckerContext &Ctx) const; private: void diagnoseMissingReleases(CheckerContext &C) const; @@ -398,7 +398,7 @@ void ObjCDeallocChecker::checkPostObjCMessage( /// Check for missing releases even when -dealloc does not call /// '[super dealloc]'. void ObjCDeallocChecker::checkEndFunction( - CheckerContext &C) const { + const ReturnStmt *RS, CheckerContext &C) const { diagnoseMissingReleases(C); } @@ -535,7 +535,7 @@ void ObjCDeallocChecker::diagnoseMissingReleases(CheckerContext &C) const { continue; // Prevents diagnosing multiple times for the same instance variable - // at, for example, both a return and at the end of of the function. + // at, for example, both a return and at the end of the function. NewUnreleased = F.remove(NewUnreleased, IvarSymbol); if (State->getStateManager() @@ -645,7 +645,7 @@ ObjCDeallocChecker::findPropertyOnDeallocatingInstance( bool ObjCDeallocChecker::diagnoseExtraRelease(SymbolRef ReleasedValue, const ObjCMethodCall &M, CheckerContext &C) const { - // Try to get the region from which the the released value was loaded. + // Try to get the region from which the released value was loaded. // Note that, unlike diagnosing for missing releases, here we don't track // values that must not be released in the state. This is because even if // these values escape, it is still an error under the rules of MRR to diff --git a/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp b/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp index 6dbacad7f2ea..202233acffab 100644 --- a/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp +++ b/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp @@ -37,6 +37,9 @@ static bool isArc4RandomAvailable(const ASTContext &Ctx) { namespace { struct ChecksFilter { + DefaultBool check_bcmp; + DefaultBool check_bcopy; + DefaultBool check_bzero; DefaultBool check_gets; DefaultBool check_getpw; DefaultBool check_mktemp; @@ -47,6 +50,9 @@ struct ChecksFilter { DefaultBool check_FloatLoopCounter; DefaultBool check_UncheckedReturn; + CheckName checkName_bcmp; + CheckName checkName_bcopy; + CheckName checkName_bzero; CheckName checkName_gets; CheckName checkName_getpw; CheckName checkName_mktemp; @@ -89,6 +95,9 @@ public: // Checker-specific methods. void checkLoopConditionForFloat(const ForStmt *FS); + void checkCall_bcmp(const CallExpr *CE, const FunctionDecl *FD); + void checkCall_bcopy(const CallExpr *CE, const FunctionDecl *FD); + void checkCall_bzero(const CallExpr *CE, const FunctionDecl *FD); void checkCall_gets(const CallExpr *CE, const FunctionDecl *FD); void checkCall_getpw(const CallExpr *CE, const FunctionDecl *FD); void checkCall_mktemp(const CallExpr *CE, const FunctionDecl *FD); @@ -129,6 +138,9 @@ void WalkAST::VisitCallExpr(CallExpr *CE) { // Set the evaluation function by switching on the callee name. FnCheck evalFunction = llvm::StringSwitch<FnCheck>(Name) + .Case("bcmp", &WalkAST::checkCall_bcmp) + .Case("bcopy", &WalkAST::checkCall_bcopy) + .Case("bzero", &WalkAST::checkCall_bzero) .Case("gets", &WalkAST::checkCall_gets) .Case("getpw", &WalkAST::checkCall_getpw) .Case("mktemp", &WalkAST::checkCall_mktemp) @@ -296,6 +308,132 @@ void WalkAST::checkLoopConditionForFloat(const ForStmt *FS) { } //===----------------------------------------------------------------------===// +// Check: Any use of bcmp. +// CWE-477: Use of Obsolete Functions +// bcmp was deprecated in POSIX.1-2008 +//===----------------------------------------------------------------------===// + +void WalkAST::checkCall_bcmp(const CallExpr *CE, const FunctionDecl *FD) { + if (!filter.check_bcmp) + return; + + const FunctionProtoType *FPT = FD->getType()->getAs<FunctionProtoType>(); + if (!FPT) + return; + + // Verify that the function takes three arguments. + if (FPT->getNumParams() != 3) + return; + + for (int i = 0; i < 2; i++) { + // Verify the first and second argument type is void*. + const PointerType *PT = FPT->getParamType(i)->getAs<PointerType>(); + if (!PT) + return; + + if (PT->getPointeeType().getUnqualifiedType() != BR.getContext().VoidTy) + return; + } + + // Verify the third argument type is integer. + if (!FPT->getParamType(2)->isIntegralOrUnscopedEnumerationType()) + return; + + // Issue a warning. + PathDiagnosticLocation CELoc = + PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC); + BR.EmitBasicReport(AC->getDecl(), filter.checkName_bcmp, + "Use of deprecated function in call to 'bcmp()'", + "Security", + "The bcmp() function is obsoleted by memcmp().", + CELoc, CE->getCallee()->getSourceRange()); +} + +//===----------------------------------------------------------------------===// +// Check: Any use of bcopy. +// CWE-477: Use of Obsolete Functions +// bcopy was deprecated in POSIX.1-2008 +//===----------------------------------------------------------------------===// + +void WalkAST::checkCall_bcopy(const CallExpr *CE, const FunctionDecl *FD) { + if (!filter.check_bcopy) + return; + + const FunctionProtoType *FPT = FD->getType()->getAs<FunctionProtoType>(); + if (!FPT) + return; + + // Verify that the function takes three arguments. + if (FPT->getNumParams() != 3) + return; + + for (int i = 0; i < 2; i++) { + // Verify the first and second argument type is void*. + const PointerType *PT = FPT->getParamType(i)->getAs<PointerType>(); + if (!PT) + return; + + if (PT->getPointeeType().getUnqualifiedType() != BR.getContext().VoidTy) + return; + } + + // Verify the third argument type is integer. + if (!FPT->getParamType(2)->isIntegralOrUnscopedEnumerationType()) + return; + + // Issue a warning. + PathDiagnosticLocation CELoc = + PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC); + BR.EmitBasicReport(AC->getDecl(), filter.checkName_bcopy, + "Use of deprecated function in call to 'bcopy()'", + "Security", + "The bcopy() function is obsoleted by memcpy() " + "or memmove().", + CELoc, CE->getCallee()->getSourceRange()); +} + +//===----------------------------------------------------------------------===// +// Check: Any use of bzero. +// CWE-477: Use of Obsolete Functions +// bzero was deprecated in POSIX.1-2008 +//===----------------------------------------------------------------------===// + +void WalkAST::checkCall_bzero(const CallExpr *CE, const FunctionDecl *FD) { + if (!filter.check_bzero) + return; + + const FunctionProtoType *FPT = FD->getType()->getAs<FunctionProtoType>(); + if (!FPT) + return; + + // Verify that the function takes two arguments. + if (FPT->getNumParams() != 2) + return; + + // Verify the first argument type is void*. + const PointerType *PT = FPT->getParamType(0)->getAs<PointerType>(); + if (!PT) + return; + + if (PT->getPointeeType().getUnqualifiedType() != BR.getContext().VoidTy) + return; + + // Verify the second argument type is integer. + if (!FPT->getParamType(1)->isIntegralOrUnscopedEnumerationType()) + return; + + // Issue a warning. + PathDiagnosticLocation CELoc = + PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC); + BR.EmitBasicReport(AC->getDecl(), filter.checkName_bzero, + "Use of deprecated function in call to 'bzero()'", + "Security", + "The bzero() function is obsoleted by memset().", + CELoc, CE->getCallee()->getSourceRange()); +} + + +//===----------------------------------------------------------------------===// // Check: Any use of 'gets' is insecure. // Originally: <rdar://problem/6335715> // Implements (part of): 300-BSI (buildsecurityin.us-cert.gov) @@ -510,6 +648,17 @@ void WalkAST::checkCall_strcpy(const CallExpr *CE, const FunctionDecl *FD) { if (!checkCall_strCommon(CE, FD)) return; + const auto *Target = CE->getArg(0)->IgnoreImpCasts(), + *Source = CE->getArg(1)->IgnoreImpCasts(); + if (const auto *DeclRef = dyn_cast<DeclRefExpr>(Target)) + if (const auto *Array = dyn_cast<ConstantArrayType>(DeclRef->getType())) { + uint64_t ArraySize = BR.getContext().getTypeSize(Array) / 8; + if (const auto *String = dyn_cast<StringLiteral>(Source)) { + if (ArraySize >= String->getLength() + 1) + return; + } + } + // Issue a warning. PathDiagnosticLocation CELoc = PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC); @@ -764,6 +913,9 @@ public: checker->filter.checkName_##name = mgr.getCurrentCheckName(); \ } +REGISTER_CHECKER(bcmp) +REGISTER_CHECKER(bcopy) +REGISTER_CHECKER(bzero) REGISTER_CHECKER(gets) REGISTER_CHECKER(getpw) REGISTER_CHECKER(mkstemp) diff --git a/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp b/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp index 95b6c4d3775d..7862a4c25681 100644 --- a/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp +++ b/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp @@ -42,6 +42,7 @@ class CheckerDocumentation : public Checker< check::PreStmt<ReturnStmt>, check::PreCall, check::PostCall, check::BranchCondition, + check::NewAllocator, check::Location, check::Bind, check::DeadSymbols, @@ -58,7 +59,7 @@ class CheckerDocumentation : public Checker< check::PreStmt<ReturnStmt>, check::Event<ImplicitNullDerefEvent>, check::ASTDecl<FunctionDecl> > { public: - /// \brief Pre-visit the Statement. + /// Pre-visit the Statement. /// /// The method will be called before the analyzer core processes the /// statement. The notification is performed for every explored CFGElement, @@ -71,7 +72,7 @@ public: /// check::PreStmt<ReturnStmt> void checkPreStmt(const ReturnStmt *DS, CheckerContext &C) const {} - /// \brief Post-visit the Statement. + /// Post-visit the Statement. /// /// The method will be called after the analyzer core processes the /// statement. The notification is performed for every explored CFGElement, @@ -81,7 +82,7 @@ public: /// check::PostStmt<DeclStmt> void checkPostStmt(const DeclStmt *DS, CheckerContext &C) const; - /// \brief Pre-visit the Objective C message. + /// Pre-visit the Objective C message. /// /// This will be called before the analyzer core processes the method call. /// This is called for any action which produces an Objective-C message send, @@ -90,13 +91,13 @@ public: /// check::PreObjCMessage void checkPreObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const {} - /// \brief Post-visit the Objective C message. + /// Post-visit the Objective C message. /// \sa checkPreObjCMessage() /// /// check::PostObjCMessage void checkPostObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const {} - /// \brief Visit an Objective-C message whose receiver is nil. + /// Visit an Objective-C message whose receiver is nil. /// /// This will be called when the analyzer core processes a method call whose /// receiver is definitely nil. In this case, check{Pre/Post}ObjCMessage and @@ -105,7 +106,7 @@ public: /// check::ObjCMessageNil void checkObjCMessageNil(const ObjCMethodCall &M, CheckerContext &C) const {} - /// \brief Pre-visit an abstract "call" event. + /// Pre-visit an abstract "call" event. /// /// This is used for checkers that want to check arguments or attributed /// behavior for functions and methods no matter how they are being invoked. @@ -117,16 +118,32 @@ public: /// check::PreCall void checkPreCall(const CallEvent &Call, CheckerContext &C) const {} - /// \brief Post-visit an abstract "call" event. + /// Post-visit an abstract "call" event. /// \sa checkPreObjCMessage() /// /// check::PostCall void checkPostCall(const CallEvent &Call, CheckerContext &C) const {} - /// \brief Pre-visit of the condition statement of a branch (such as IfStmt). + /// Pre-visit of the condition statement of a branch (such as IfStmt). void checkBranchCondition(const Stmt *Condition, CheckerContext &Ctx) const {} - /// \brief Called on a load from and a store to a location. + /// Post-visit the C++ operator new's allocation call. + /// + /// Execution of C++ operator new consists of the following phases: (1) call + /// default or overridden operator new() to allocate memory (2) cast the + /// return value of operator new() from void pointer type to class pointer + /// type, (3) assuming that the value is non-null, call the object's + /// constructor over this pointer, (4) declare that the value of the + /// new-expression is this pointer. This callback is called between steps + /// (2) and (3). Post-call for the allocator is called after step (1). + /// Pre-statement for the new-expression is called on step (4) when the value + /// of the expression is evaluated. + /// \param NE The C++ new-expression that triggered the allocation. + /// \param Target The allocated region, casted to the class type. + void checkNewAllocator(const CXXNewExpr *NE, SVal Target, + CheckerContext &) const {} + + /// Called on a load from and a store to a location. /// /// The method will be called each time a location (pointer) value is /// accessed. @@ -138,7 +155,7 @@ public: void checkLocation(SVal Loc, bool IsLoad, const Stmt *S, CheckerContext &) const {} - /// \brief Called on binding of a value to a location. + /// Called on binding of a value to a location. /// /// \param Loc The value of the location (pointer). /// \param Val The value which will be stored at the location Loc. @@ -147,7 +164,7 @@ public: /// check::Bind void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &) const {} - /// \brief Called whenever a symbol becomes dead. + /// Called whenever a symbol becomes dead. /// /// This callback should be used by the checkers to aggressively clean /// up/reduce the checker state, which is important for reducing the overall @@ -164,20 +181,20 @@ public: void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const {} - /// \brief Called when the analyzer core starts analyzing a function, + /// Called when the analyzer core starts analyzing a function, /// regardless of whether it is analyzed at the top level or is inlined. /// /// check::BeginFunction void checkBeginFunction(CheckerContext &Ctx) const {} - /// \brief Called when the analyzer core reaches the end of a + /// Called when the analyzer core reaches the end of a /// function being analyzed regardless of whether it is analyzed at the top /// level or is inlined. /// /// check::EndFunction - void checkEndFunction(CheckerContext &Ctx) const {} + void checkEndFunction(const ReturnStmt *RS, CheckerContext &Ctx) const {} - /// \brief Called after all the paths in the ExplodedGraph reach end of path + /// Called after all the paths in the ExplodedGraph reach end of path /// - the symbolic execution graph is fully explored. /// /// This callback should be used in cases when a checker needs to have a @@ -190,14 +207,14 @@ public: BugReporter &BR, ExprEngine &Eng) const {} - /// \brief Called after analysis of a TranslationUnit is complete. + /// Called after analysis of a TranslationUnit is complete. /// /// check::EndOfTranslationUnit void checkEndOfTranslationUnit(const TranslationUnitDecl *TU, AnalysisManager &Mgr, BugReporter &BR) const {} - /// \brief Evaluates function call. + /// Evaluates function call. /// /// The analysis core threats all function calls in the same way. However, some /// functions have special meaning, which should be reflected in the program @@ -212,7 +229,7 @@ public: /// eval::Call bool evalCall(const CallExpr *CE, CheckerContext &C) const { return true; } - /// \brief Handles assumptions on symbolic values. + /// Handles assumptions on symbolic values. /// /// This method is called when a symbolic expression is assumed to be true or /// false. For example, the assumptions are performed when evaluating a @@ -231,7 +248,7 @@ public: /// check::LiveSymbols void checkLiveSymbols(ProgramStateRef State, SymbolReaper &SR) const {} - /// \brief Called when the contents of one or more regions change. + /// Called when the contents of one or more regions change. /// /// This can occur in many different ways: an explicit bind, a blanket /// invalidation of the region contents, or by passing a region to a function @@ -263,7 +280,7 @@ public: return State; } - /// \brief Called when pointers escape. + /// Called when pointers escape. /// /// This notifies the checkers about pointer escape, which occurs whenever /// the analyzer cannot track the symbol any more. For example, as a @@ -283,7 +300,7 @@ public: return State; } - /// \brief Called when const pointers escape. + /// Called when const pointers escape. /// /// Note: in most cases checkPointerEscape callback is sufficient. /// \sa checkPointerEscape @@ -297,7 +314,7 @@ public: /// check::Event<ImplicitNullDerefEvent> void checkEvent(ImplicitNullDerefEvent Event) const {} - /// \brief Check every declaration in the AST. + /// Check every declaration in the AST. /// /// An AST traversal callback, which should only be used when the checker is /// not path sensitive. It will be called for every Declaration in the AST and diff --git a/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp index 9e9939ae25c0..b38992b0e030 100644 --- a/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp @@ -105,7 +105,7 @@ void ChrootChecker::Chdir(CheckerContext &C, const CallExpr *CE) const { // After chdir("/"), enter the jail, set the enum value JAIL_ENTERED. const Expr *ArgExpr = CE->getArg(0); - SVal ArgVal = state->getSVal(ArgExpr, C.getLocationContext()); + SVal ArgVal = C.getSVal(ArgExpr); if (const MemRegion *R = ArgVal.getAsRegion()) { R = R->StripCasts(); @@ -132,7 +132,7 @@ void ChrootChecker::checkPreStmt(const CallExpr *CE, CheckerContext &C) const { if (!II_chdir) II_chdir = &Ctx.Idents.get("chdir"); - // Ingnore chroot and chdir. + // Ignore chroot and chdir. if (FD->getIdentifier() == II_chroot || FD->getIdentifier() == II_chdir) return; diff --git a/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp b/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp index e04e2ab2c320..d3489282ab62 100644 --- a/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp @@ -39,7 +39,7 @@ class DeleteWithNonVirtualDtorChecker : public Checker<check::PreStmt<CXXDeleteExpr>> { mutable std::unique_ptr<BugType> BT; - class DeleteBugVisitor : public BugReporterVisitorImpl<DeleteBugVisitor> { + class DeleteBugVisitor : public BugReporterVisitor { public: DeleteBugVisitor() : Satisfied(false) {} void Profile(llvm::FoldingSetNodeID &ID) const override { @@ -110,8 +110,6 @@ DeleteWithNonVirtualDtorChecker::DeleteBugVisitor::VisitNode( if (Satisfied) return nullptr; - ProgramStateRef State = N->getState(); - const LocationContext *LC = N->getLocationContext(); const Stmt *S = PathDiagnosticLocation::getStmt(N); if (!S) return nullptr; @@ -128,7 +126,7 @@ DeleteWithNonVirtualDtorChecker::DeleteBugVisitor::VisitNode( } // Region associated with the current cast expression. - const MemRegion *M = State->getSVal(CastE, LC).getAsRegion(); + const MemRegion *M = N->getSVal(CastE).getAsRegion(); if (!M) return nullptr; diff --git a/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp b/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp index 598502305633..bc39c92ea970 100644 --- a/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp @@ -24,22 +24,23 @@ using namespace ento; namespace { class DivZeroChecker : public Checker< check::PreStmt<BinaryOperator> > { mutable std::unique_ptr<BuiltinBug> BT; - void reportBug(const char *Msg, - ProgramStateRef StateZero, - CheckerContext &C) const ; + void reportBug(const char *Msg, ProgramStateRef StateZero, CheckerContext &C, + std::unique_ptr<BugReporterVisitor> Visitor = nullptr) const; + public: void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const; }; } // end anonymous namespace -void DivZeroChecker::reportBug(const char *Msg, - ProgramStateRef StateZero, - CheckerContext &C) const { +void DivZeroChecker::reportBug( + const char *Msg, ProgramStateRef StateZero, CheckerContext &C, + std::unique_ptr<BugReporterVisitor> Visitor) const { if (ExplodedNode *N = C.generateErrorNode(StateZero)) { if (!BT) BT.reset(new BuiltinBug(this, "Division by zero")); auto R = llvm::make_unique<BugReport>(*BT, Msg, N); + R->addVisitor(std::move(Visitor)); bugreporter::trackNullOrUndefValue(N, bugreporter::GetDenomExpr(N), *R); C.emitReport(std::move(R)); } @@ -57,7 +58,7 @@ void DivZeroChecker::checkPreStmt(const BinaryOperator *B, if (!B->getRHS()->getType()->isScalarType()) return; - SVal Denom = C.getState()->getSVal(B->getRHS(), C.getLocationContext()); + SVal Denom = C.getSVal(B->getRHS()); Optional<DefinedSVal> DV = Denom.getAs<DefinedSVal>(); // Divide-by-undefined handled in the generic checking for uses of @@ -78,7 +79,8 @@ void DivZeroChecker::checkPreStmt(const BinaryOperator *B, bool TaintedD = C.getState()->isTainted(*DV); if ((stateNotZero && stateZero && TaintedD)) { - reportBug("Division by a tainted value, possibly zero", stateZero, C); + reportBug("Division by a tainted value, possibly zero", stateZero, C, + llvm::make_unique<TaintBugVisitor>(*DV)); return; } diff --git a/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp b/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp index 109897be2931..4e4d81cd6714 100644 --- a/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp @@ -38,8 +38,7 @@ class DynamicTypeChecker : public Checker<check::PostStmt<ImplicitCastExpr>> { new BugType(this, "Dynamic and static type mismatch", "Type Error")); } - class DynamicTypeBugVisitor - : public BugReporterVisitorImpl<DynamicTypeBugVisitor> { + class DynamicTypeBugVisitor : public BugReporterVisitor { public: DynamicTypeBugVisitor(const MemRegion *Reg) : Reg(Reg) {} diff --git a/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp b/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp index db9179e018a1..126e57645a43 100644 --- a/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp +++ b/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp @@ -59,7 +59,7 @@ class DynamicTypePropagation: const ObjCObjectType *getObjectTypeForAllocAndNew(const ObjCMessageExpr *MsgE, CheckerContext &C) const; - /// \brief Return a better dynamic type if one can be derived from the cast. + /// Return a better dynamic type if one can be derived from the cast. const ObjCObjectPointerType *getBetterObjCType(const Expr *CastE, CheckerContext &C) const; @@ -74,7 +74,7 @@ class DynamicTypePropagation: new BugType(this, "Generics", categories::CoreFoundationObjectiveC)); } - class GenericsBugVisitor : public BugReporterVisitorImpl<GenericsBugVisitor> { + class GenericsBugVisitor : public BugReporterVisitor { public: GenericsBugVisitor(SymbolRef S) : Sym(S) {} @@ -562,7 +562,7 @@ void DynamicTypePropagation::checkPostStmt(const CastExpr *CE, DestObjectPtrType->isUnspecialized()) return; - SymbolRef Sym = State->getSVal(CE, C.getLocationContext()).getAsSymbol(); + SymbolRef Sym = C.getSVal(CE).getAsSymbol(); if (!Sym) return; @@ -631,7 +631,7 @@ static const Expr *stripCastsAndSugar(const Expr *E) { } static bool isObjCTypeParamDependent(QualType Type) { - // It is illegal to typedef parameterized types inside an interface. Therfore + // It is illegal to typedef parameterized types inside an interface. Therefore // an Objective-C type can only be dependent on a type parameter when the type // parameter structurally present in the type itself. class IsObjCTypeParamDependentTypeVisitor diff --git a/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp b/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp index 0005ec470d20..8de653c10f7e 100644 --- a/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp @@ -149,7 +149,7 @@ void ExprInspectionChecker::analyzerEval(const CallExpr *CE, // A specific instantiation of an inlined function may have more constrained // values than can generally be assumed. Skip the check. - if (LC->getCurrentStackFrame()->getParent() != nullptr) + if (LC->getStackFrame()->getParent() != nullptr) return; reportBug(getArgumentValueString(CE, C), C); @@ -178,7 +178,7 @@ void ExprInspectionChecker::analyzerCheckInlined(const CallExpr *CE, // when we are analyzing it as an inlined function. This means that // clang_analyzer_checkInlined(true) should always print TRUE, but // clang_analyzer_checkInlined(false) should never actually print anything. - if (LC->getCurrentStackFrame()->getParent() == nullptr) + if (LC->getStackFrame()->getParent() == nullptr) return; reportBug(getArgumentValueString(CE, C), C); diff --git a/lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp b/lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp index 3fe89f96a43b..059203fca730 100644 --- a/lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp @@ -44,8 +44,7 @@ void FixedAddressChecker::checkPreStmt(const BinaryOperator *B, if (!T->isPointerType()) return; - ProgramStateRef state = C.getState(); - SVal RV = state->getSVal(B->getRHS(), C.getLocationContext()); + SVal RV = C.getSVal(B->getRHS()); if (!RV.isConstant() || RV.isZeroConstant()) return; diff --git a/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp b/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp new file mode 100644 index 000000000000..5cb51b01f044 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/GCDAntipatternChecker.cpp @@ -0,0 +1,229 @@ +//===- GCDAntipatternChecker.cpp ---------------------------------*- C++ -*-==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This file defines GCDAntipatternChecker which checks against a common +// antipattern when synchronous API is emulated from asynchronous callbacks +// using a semaphore: +// +// dispatch_semaphore_t sema = dispatch_semaphore_create(0); +// +// AnyCFunctionCall(^{ +// // code… +// dispatch_semaphore_signal(sema); +// }) +// dispatch_semaphore_wait(sema, *) +// +// Such code is a common performance problem, due to inability of GCD to +// properly handle QoS when a combination of queues and semaphores is used. +// Good code would either use asynchronous API (when available), or perform +// the necessary action in asynchronous callback. +// +// Currently, the check is performed using a simple heuristical AST pattern +// matching. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h" +#include "llvm/Support/Debug.h" + +using namespace clang; +using namespace ento; +using namespace ast_matchers; + +namespace { + +// ID of a node at which the diagnostic would be emitted. +const char *WarnAtNode = "waitcall"; + +class GCDAntipatternChecker : public Checker<check::ASTCodeBody> { +public: + void checkASTCodeBody(const Decl *D, + AnalysisManager &AM, + BugReporter &BR) const; +}; + +auto callsName(const char *FunctionName) + -> decltype(callee(functionDecl())) { + return callee(functionDecl(hasName(FunctionName))); +} + +auto equalsBoundArgDecl(int ArgIdx, const char *DeclName) + -> decltype(hasArgument(0, expr())) { + return hasArgument(ArgIdx, ignoringParenCasts(declRefExpr( + to(varDecl(equalsBoundNode(DeclName)))))); +} + +auto bindAssignmentToDecl(const char *DeclName) -> decltype(hasLHS(expr())) { + return hasLHS(ignoringParenImpCasts( + declRefExpr(to(varDecl().bind(DeclName))))); +} + +/// The pattern is very common in tests, and it is OK to use it there. +/// We have to heuristics for detecting tests: method name starts with "test" +/// (used in XCTest), and a class name contains "mock" or "test" (used in +/// helpers which are not tests themselves, but used exclusively in tests). +static bool isTest(const Decl *D) { + if (const auto* ND = dyn_cast<NamedDecl>(D)) { + std::string DeclName = ND->getNameAsString(); + if (StringRef(DeclName).startswith("test")) + return true; + } + if (const auto *OD = dyn_cast<ObjCMethodDecl>(D)) { + if (const auto *CD = dyn_cast<ObjCContainerDecl>(OD->getParent())) { + std::string ContainerName = CD->getNameAsString(); + StringRef CN(ContainerName); + if (CN.contains_lower("test") || CN.contains_lower("mock")) + return true; + } + } + return false; +} + +static auto findGCDAntiPatternWithSemaphore() -> decltype(compoundStmt()) { + + const char *SemaphoreBinding = "semaphore_name"; + auto SemaphoreCreateM = callExpr(allOf( + callsName("dispatch_semaphore_create"), + hasArgument(0, ignoringParenCasts(integerLiteral(equals(0)))))); + + auto SemaphoreBindingM = anyOf( + forEachDescendant( + varDecl(hasDescendant(SemaphoreCreateM)).bind(SemaphoreBinding)), + forEachDescendant(binaryOperator(bindAssignmentToDecl(SemaphoreBinding), + hasRHS(SemaphoreCreateM)))); + + auto HasBlockArgumentM = hasAnyArgument(hasType( + hasCanonicalType(blockPointerType()) + )); + + auto ArgCallsSignalM = hasAnyArgument(stmt(hasDescendant(callExpr( + allOf( + callsName("dispatch_semaphore_signal"), + equalsBoundArgDecl(0, SemaphoreBinding) + ))))); + + auto HasBlockAndCallsSignalM = allOf(HasBlockArgumentM, ArgCallsSignalM); + + auto HasBlockCallingSignalM = + forEachDescendant( + stmt(anyOf( + callExpr(HasBlockAndCallsSignalM), + objcMessageExpr(HasBlockAndCallsSignalM) + ))); + + auto SemaphoreWaitM = forEachDescendant( + callExpr( + allOf( + callsName("dispatch_semaphore_wait"), + equalsBoundArgDecl(0, SemaphoreBinding) + ) + ).bind(WarnAtNode)); + + return compoundStmt( + SemaphoreBindingM, HasBlockCallingSignalM, SemaphoreWaitM); +} + +static auto findGCDAntiPatternWithGroup() -> decltype(compoundStmt()) { + + const char *GroupBinding = "group_name"; + auto DispatchGroupCreateM = callExpr(callsName("dispatch_group_create")); + + auto GroupBindingM = anyOf( + forEachDescendant( + varDecl(hasDescendant(DispatchGroupCreateM)).bind(GroupBinding)), + forEachDescendant(binaryOperator(bindAssignmentToDecl(GroupBinding), + hasRHS(DispatchGroupCreateM)))); + + auto GroupEnterM = forEachDescendant( + stmt(callExpr(allOf(callsName("dispatch_group_enter"), + equalsBoundArgDecl(0, GroupBinding))))); + + auto HasBlockArgumentM = hasAnyArgument(hasType( + hasCanonicalType(blockPointerType()) + )); + + auto ArgCallsSignalM = hasAnyArgument(stmt(hasDescendant(callExpr( + allOf( + callsName("dispatch_group_leave"), + equalsBoundArgDecl(0, GroupBinding) + ))))); + + auto HasBlockAndCallsLeaveM = allOf(HasBlockArgumentM, ArgCallsSignalM); + + auto AcceptsBlockM = + forEachDescendant( + stmt(anyOf( + callExpr(HasBlockAndCallsLeaveM), + objcMessageExpr(HasBlockAndCallsLeaveM) + ))); + + auto GroupWaitM = forEachDescendant( + callExpr( + allOf( + callsName("dispatch_group_wait"), + equalsBoundArgDecl(0, GroupBinding) + ) + ).bind(WarnAtNode)); + + return compoundStmt(GroupBindingM, GroupEnterM, AcceptsBlockM, GroupWaitM); +} + +static void emitDiagnostics(const BoundNodes &Nodes, + const char* Type, + BugReporter &BR, + AnalysisDeclContext *ADC, + const GCDAntipatternChecker *Checker) { + const auto *SW = Nodes.getNodeAs<CallExpr>(WarnAtNode); + assert(SW); + + std::string Diagnostics; + llvm::raw_string_ostream OS(Diagnostics); + OS << "Waiting on a callback using a " << Type << " creates useless threads " + << "and is subject to priority inversion; consider " + << "using a synchronous API or changing the caller to be asynchronous"; + + BR.EmitBasicReport( + ADC->getDecl(), + Checker, + /*Name=*/"GCD performance anti-pattern", + /*Category=*/"Performance", + OS.str(), + PathDiagnosticLocation::createBegin(SW, BR.getSourceManager(), ADC), + SW->getSourceRange()); +} + +void GCDAntipatternChecker::checkASTCodeBody(const Decl *D, + AnalysisManager &AM, + BugReporter &BR) const { + if (isTest(D)) + return; + + AnalysisDeclContext *ADC = AM.getAnalysisDeclContext(D); + + auto SemaphoreMatcherM = findGCDAntiPatternWithSemaphore(); + auto Matches = match(SemaphoreMatcherM, *D->getBody(), AM.getASTContext()); + for (BoundNodes Match : Matches) + emitDiagnostics(Match, "semaphore", BR, ADC, this); + + auto GroupMatcherM = findGCDAntiPatternWithGroup(); + Matches = match(GroupMatcherM, *D->getBody(), AM.getASTContext()); + for (BoundNodes Match : Matches) + emitDiagnostics(Match, "group", BR, ADC, this); +} + +} + +void ento::registerGCDAntipattern(CheckerManager &Mgr) { + Mgr.registerChecker<GCDAntipatternChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/GTestChecker.cpp b/lib/StaticAnalyzer/Checkers/GTestChecker.cpp index f0be41b293e4..3ef95e673b87 100644 --- a/lib/StaticAnalyzer/Checkers/GTestChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/GTestChecker.cpp @@ -161,7 +161,7 @@ void GTestChecker::modelAssertionResultCopyConstructor( const CXXConstructorCall *Call, CheckerContext &C) const { assert(Call->getNumArgs() == 1); - // The first parameter of the the copy constructor must be the other + // The first parameter of the copy constructor must be the other // instance to initialize this instances fields from. SVal OtherVal = Call->getArgSVal(0); SVal ThisVal = Call->getCXXThisVal(); diff --git a/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp b/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp index 43966656cd8d..899586745a0b 100644 --- a/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp @@ -48,24 +48,24 @@ private: BT.reset(new BugType(this, "Use of Untrusted Data", "Untrusted Data")); } - /// \brief Catch taint related bugs. Check if tainted data is passed to a + /// Catch taint related bugs. Check if tainted data is passed to a /// system call etc. bool checkPre(const CallExpr *CE, CheckerContext &C) const; - /// \brief Add taint sources on a pre-visit. + /// Add taint sources on a pre-visit. void addSourcesPre(const CallExpr *CE, CheckerContext &C) const; - /// \brief Propagate taint generated at pre-visit. + /// Propagate taint generated at pre-visit. bool propagateFromPre(const CallExpr *CE, CheckerContext &C) const; - /// \brief Add taint sources on a post visit. + /// Add taint sources on a post visit. void addSourcesPost(const CallExpr *CE, CheckerContext &C) const; /// Check if the region the expression evaluates to is the standard input, /// and thus, is tainted. static bool isStdin(const Expr *E, CheckerContext &C); - /// \brief Given a pointer argument, return the value it points to. + /// Given a pointer argument, return the value it points to. static Optional<SVal> getPointedToSVal(CheckerContext &C, const Expr *Arg); /// Functions defining the attack surface. @@ -100,26 +100,9 @@ private: bool generateReportIfTainted(const Expr *E, const char Msg[], CheckerContext &C) const; - /// The bug visitor prints a diagnostic message at the location where a given - /// variable was tainted. - class TaintBugVisitor - : public BugReporterVisitorImpl<TaintBugVisitor> { - private: - const SVal V; - - public: - TaintBugVisitor(const SVal V) : V(V) {} - void Profile(llvm::FoldingSetNodeID &ID) const override { ID.Add(V); } - - std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N, - const ExplodedNode *PrevN, - BugReporterContext &BRC, - BugReport &BR) override; - }; - typedef SmallVector<unsigned, 2> ArgVector; - /// \brief A struct used to specify taint propagation rules for a function. + /// A struct used to specify taint propagation rules for a function. /// /// If any of the possible taint source arguments is tainted, all of the /// destination arguments should also be tainted. Use InvalidArgIndex in the @@ -183,7 +166,7 @@ private: return (V && State->isTainted(*V)); } - /// \brief Pre-process a function which propagates taint according to the + /// Pre-process a function which propagates taint according to the /// taint rule. ProgramStateRef process(const CallExpr *CE, CheckerContext &C) const; @@ -214,28 +197,6 @@ const char GenericTaintChecker::MsgTaintedBufferSize[] = /// points to data, which should be tainted on return. REGISTER_SET_WITH_PROGRAMSTATE(TaintArgsOnPostVisit, unsigned) -std::shared_ptr<PathDiagnosticPiece> -GenericTaintChecker::TaintBugVisitor::VisitNode(const ExplodedNode *N, - const ExplodedNode *PrevN, BugReporterContext &BRC, BugReport &BR) { - - // Find the ExplodedNode where the taint was first introduced - if (!N->getState()->isTainted(V) || PrevN->getState()->isTainted(V)) - return nullptr; - - const Stmt *S = PathDiagnosticLocation::getStmt(N); - if (!S) - return nullptr; - - const LocationContext *NCtx = N->getLocationContext(); - PathDiagnosticLocation L = - PathDiagnosticLocation::createBegin(S, BRC.getSourceManager(), NCtx); - if (!L.isValid() || !L.asLocation().isValid()) - return nullptr; - - return std::make_shared<PathDiagnosticEventPiece>( - L, "Taint originated here"); -} - GenericTaintChecker::TaintPropagationRule GenericTaintChecker::TaintPropagationRule::getTaintPropagationRule( const FunctionDecl *FDecl, @@ -468,7 +429,7 @@ bool GenericTaintChecker::checkPre(const CallExpr *CE, CheckerContext &C) const{ Optional<SVal> GenericTaintChecker::getPointedToSVal(CheckerContext &C, const Expr *Arg) { ProgramStateRef State = C.getState(); - SVal AddrVal = State->getSVal(Arg->IgnoreParens(), C.getLocationContext()); + SVal AddrVal = C.getSVal(Arg->IgnoreParens()); if (AddrVal.isUnknownOrUndef()) return None; @@ -621,7 +582,7 @@ ProgramStateRef GenericTaintChecker::postRetTaint(const CallExpr *CE, bool GenericTaintChecker::isStdin(const Expr *E, CheckerContext &C) { ProgramStateRef State = C.getState(); - SVal Val = State->getSVal(E, C.getLocationContext()); + SVal Val = C.getSVal(E); // stdin is a pointer, so it would be a region. const MemRegion *MemReg = Val.getAsRegion(); @@ -646,7 +607,8 @@ bool GenericTaintChecker::isStdin(const Expr *E, CheckerContext &C) { if ((D->getName().find("stdin") != StringRef::npos) && D->isExternC()) if (const PointerType * PtrTy = dyn_cast<PointerType>(D->getType().getTypePtr())) - if (PtrTy->getPointeeType() == C.getASTContext().getFILEType()) + if (PtrTy->getPointeeType().getCanonicalType() == + C.getASTContext().getFILEType().getCanonicalType()) return true; } return false; diff --git a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp index cf57b8dca063..f102ca96a5c1 100644 --- a/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp @@ -8,7 +8,7 @@ //===----------------------------------------------------------------------===// /// /// \file -/// \brief This defines IdenticalExprChecker, a check that warns about +/// This defines IdenticalExprChecker, a check that warns about /// unintended use of identical expressions. /// /// It checks for use of identical expressions with comparison operators and @@ -296,7 +296,7 @@ bool FindIdenticalExprVisitor::VisitConditionalOperator( return true; } -/// \brief Determines whether two statement trees are identical regarding +/// Determines whether two statement trees are identical regarding /// operators and symbols. /// /// Exceptions: expressions containing macros or functions with possible side diff --git a/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp b/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp new file mode 100644 index 000000000000..ed877ab34518 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp @@ -0,0 +1,252 @@ +//=== InnerPointerChecker.cpp -------------------------------------*- C++ -*--// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This file defines a check that marks a raw pointer to a C++ container's +// inner buffer released when the object is destroyed. This information can +// be used by MallocChecker to detect use-after-free problems. +// +//===----------------------------------------------------------------------===// + +#include "AllocationState.h" +#include "ClangSACheckers.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" + +using namespace clang; +using namespace ento; + +using PtrSet = llvm::ImmutableSet<SymbolRef>; + +// Associate container objects with a set of raw pointer symbols. +REGISTER_MAP_WITH_PROGRAMSTATE(RawPtrMap, const MemRegion *, PtrSet) + +// This is a trick to gain access to PtrSet's Factory. +namespace clang { +namespace ento { +template <> +struct ProgramStateTrait<PtrSet> : public ProgramStatePartialTrait<PtrSet> { + static void *GDMIndex() { + static int Index = 0; + return &Index; + } +}; +} // end namespace ento +} // end namespace clang + +namespace { + +class InnerPointerChecker + : public Checker<check::DeadSymbols, check::PostCall> { + + CallDescription AppendFn, AssignFn, ClearFn, CStrFn, DataFn, EraseFn, + InsertFn, PopBackFn, PushBackFn, ReplaceFn, ReserveFn, ResizeFn, + ShrinkToFitFn, SwapFn; + +public: + class InnerPointerBRVisitor : public BugReporterVisitor { + SymbolRef PtrToBuf; + + public: + InnerPointerBRVisitor(SymbolRef Sym) : PtrToBuf(Sym) {} + + static void *getTag() { + static int Tag = 0; + return &Tag; + } + + void Profile(llvm::FoldingSetNodeID &ID) const override { + ID.AddPointer(getTag()); + } + + std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N, + const ExplodedNode *PrevN, + BugReporterContext &BRC, + BugReport &BR) override; + + // FIXME: Scan the map once in the visitor's constructor and do a direct + // lookup by region. + bool isSymbolTracked(ProgramStateRef State, SymbolRef Sym) { + RawPtrMapTy Map = State->get<RawPtrMap>(); + for (const auto Entry : Map) { + if (Entry.second.contains(Sym)) + return true; + } + return false; + } + }; + + InnerPointerChecker() + : AppendFn("append"), AssignFn("assign"), ClearFn("clear"), + CStrFn("c_str"), DataFn("data"), EraseFn("erase"), InsertFn("insert"), + PopBackFn("pop_back"), PushBackFn("push_back"), ReplaceFn("replace"), + ReserveFn("reserve"), ResizeFn("resize"), + ShrinkToFitFn("shrink_to_fit"), SwapFn("swap") {} + + /// Check whether the function called on the container object is a + /// member function that potentially invalidates pointers referring + /// to the objects's internal buffer. + bool mayInvalidateBuffer(const CallEvent &Call) const; + + /// Record the connection between the symbol returned by c_str() and the + /// corresponding string object region in the ProgramState. Mark the symbol + /// released if the string object is destroyed. + void checkPostCall(const CallEvent &Call, CheckerContext &C) const; + + /// Clean up the ProgramState map. + void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const; +}; + +} // end anonymous namespace + +// [string.require] +// +// "References, pointers, and iterators referring to the elements of a +// basic_string sequence may be invalidated by the following uses of that +// basic_string object: +// +// -- TODO: As an argument to any standard library function taking a reference +// to non-const basic_string as an argument. For example, as an argument to +// non-member functions swap(), operator>>(), and getline(), or as an argument +// to basic_string::swap(). +// +// -- Calling non-const member functions, except operator[], at, front, back, +// begin, rbegin, end, and rend." +// +bool InnerPointerChecker::mayInvalidateBuffer(const CallEvent &Call) const { + if (const auto *MemOpCall = dyn_cast<CXXMemberOperatorCall>(&Call)) { + OverloadedOperatorKind Opc = MemOpCall->getOriginExpr()->getOperator(); + if (Opc == OO_Equal || Opc == OO_PlusEqual) + return true; + return false; + } + return (isa<CXXDestructorCall>(Call) || Call.isCalled(AppendFn) || + Call.isCalled(AssignFn) || Call.isCalled(ClearFn) || + Call.isCalled(EraseFn) || Call.isCalled(InsertFn) || + Call.isCalled(PopBackFn) || Call.isCalled(PushBackFn) || + Call.isCalled(ReplaceFn) || Call.isCalled(ReserveFn) || + Call.isCalled(ResizeFn) || Call.isCalled(ShrinkToFitFn) || + Call.isCalled(SwapFn)); +} + +void InnerPointerChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + const auto *ICall = dyn_cast<CXXInstanceCall>(&Call); + if (!ICall) + return; + + SVal Obj = ICall->getCXXThisVal(); + const auto *ObjRegion = dyn_cast_or_null<TypedValueRegion>(Obj.getAsRegion()); + if (!ObjRegion) + return; + + auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl(); + if (TypeDecl->getName() != "basic_string") + return; + + ProgramStateRef State = C.getState(); + + if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) { + SVal RawPtr = Call.getReturnValue(); + if (SymbolRef Sym = RawPtr.getAsSymbol(/*IncludeBaseRegions=*/true)) { + // Start tracking this raw pointer by adding it to the set of symbols + // associated with this container object in the program state map. + PtrSet::Factory &F = State->getStateManager().get_context<PtrSet>(); + const PtrSet *SetPtr = State->get<RawPtrMap>(ObjRegion); + PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet(); + assert(C.wasInlined || !Set.contains(Sym)); + Set = F.add(Set, Sym); + State = State->set<RawPtrMap>(ObjRegion, Set); + C.addTransition(State); + } + return; + } + + if (mayInvalidateBuffer(Call)) { + if (const PtrSet *PS = State->get<RawPtrMap>(ObjRegion)) { + // Mark all pointer symbols associated with the deleted object released. + const Expr *Origin = Call.getOriginExpr(); + for (const auto Symbol : *PS) { + // NOTE: `Origin` may be null, and will be stored so in the symbol's + // `RefState` in MallocChecker's `RegionState` program state map. + State = allocation_state::markReleased(State, Symbol, Origin); + } + State = State->remove<RawPtrMap>(ObjRegion); + C.addTransition(State); + return; + } + } +} + +void InnerPointerChecker::checkDeadSymbols(SymbolReaper &SymReaper, + CheckerContext &C) const { + ProgramStateRef State = C.getState(); + PtrSet::Factory &F = State->getStateManager().get_context<PtrSet>(); + RawPtrMapTy RPM = State->get<RawPtrMap>(); + for (const auto Entry : RPM) { + if (!SymReaper.isLiveRegion(Entry.first)) { + // Due to incomplete destructor support, some dead regions might + // remain in the program state map. Clean them up. + State = State->remove<RawPtrMap>(Entry.first); + } + if (const PtrSet *OldSet = State->get<RawPtrMap>(Entry.first)) { + PtrSet CleanedUpSet = *OldSet; + for (const auto Symbol : Entry.second) { + if (!SymReaper.isLive(Symbol)) + CleanedUpSet = F.remove(CleanedUpSet, Symbol); + } + State = CleanedUpSet.isEmpty() + ? State->remove<RawPtrMap>(Entry.first) + : State->set<RawPtrMap>(Entry.first, CleanedUpSet); + } + } + C.addTransition(State); +} + +std::shared_ptr<PathDiagnosticPiece> +InnerPointerChecker::InnerPointerBRVisitor::VisitNode(const ExplodedNode *N, + const ExplodedNode *PrevN, + BugReporterContext &BRC, + BugReport &BR) { + + if (!isSymbolTracked(N->getState(), PtrToBuf) || + isSymbolTracked(PrevN->getState(), PtrToBuf)) + return nullptr; + + const Stmt *S = PathDiagnosticLocation::getStmt(N); + if (!S) + return nullptr; + + SmallString<256> Buf; + llvm::raw_svector_ostream OS(Buf); + OS << "Dangling inner pointer obtained here"; + PathDiagnosticLocation Pos(S, BRC.getSourceManager(), + N->getLocationContext()); + return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true, + nullptr); +} + +namespace clang { +namespace ento { +namespace allocation_state { + +std::unique_ptr<BugReporterVisitor> getInnerPointerBRVisitor(SymbolRef Sym) { + return llvm::make_unique<InnerPointerChecker::InnerPointerBRVisitor>(Sym); +} + +} // end namespace allocation_state +} // end namespace ento +} // end namespace clang + +void ento::registerInnerPointerChecker(CheckerManager &Mgr) { + registerNewDeleteChecker(Mgr); + Mgr.registerChecker<InnerPointerChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp b/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp index 0f9b749506fa..56c250cd1678 100644 --- a/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp @@ -46,19 +46,25 @@ // use setter and getters functions which separate the three cases. To store // them we use a pointer union of symbol and memory region. // -// The checker works the following way: We record the past-end iterator for -// all containers whenever their `.end()` is called. Since the Constraint -// Manager cannot handle SVals we need to take over its role. We post-check -// equality and non-equality comparisons and propagate the position of the -// iterator to the other side of the comparison if it is past-end and we are in -// the 'equal' branch (true-branch for `==` and false-branch for `!=`). +// The checker works the following way: We record the begin and the +// past-end iterator for all containers whenever their `.begin()` and `.end()` +// are called. Since the Constraint Manager cannot handle such SVals we need +// to take over its role. We post-check equality and non-equality comparisons +// and record that the two sides are equal if we are in the 'equal' branch +// (true-branch for `==` and false-branch for `!=`). // // In case of type-I or type-II iterators we get a concrete integer as a result // of the comparison (1 or 0) but in case of type-III we only get a Symbol. In // this latter case we record the symbol and reload it in evalAssume() and do // the propagation there. We also handle (maybe double) negated comparisons -// which are represented in the form of (x == 0 or x !=0 ) where x is the +// which are represented in the form of (x == 0 or x != 0) where x is the // comparison itself. +// +// Since `SimpleConstraintManager` cannot handle complex symbolic expressions +// we only use expressions of the format S, S+n or S-n for iterator positions +// where S is a conjured symbol and n is an unsigned concrete integer. When +// making an assumption e.g. `S1 + n == S2 + m` we store `S1 - S2 == m - n` as +// a constraint which we later retrieve when doing an actual comparison. #include "ClangSACheckers.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" @@ -80,7 +86,7 @@ private: const MemRegion *Cont; // Abstract offset - SymbolRef Offset; + const SymbolRef Offset; IteratorPosition(const MemRegion *C, SymbolRef Of) : Cont(C), Offset(Of) {} @@ -113,31 +119,39 @@ public: typedef llvm::PointerUnion<const MemRegion *, SymbolRef> RegionOrSymbol; -// Structure to record the symbolic end position of a container +// Structure to record the symbolic begin and end position of a container struct ContainerData { private: - SymbolRef End; + const SymbolRef Begin, End; - ContainerData(SymbolRef E) : End(E) {} + ContainerData(SymbolRef B, SymbolRef E) : Begin(B), End(E) {} public: + static ContainerData fromBegin(SymbolRef B) { + return ContainerData(B, nullptr); + } + static ContainerData fromEnd(SymbolRef E) { - return ContainerData(E); + return ContainerData(nullptr, E); } + SymbolRef getBegin() const { return Begin; } SymbolRef getEnd() const { return End; } - ContainerData newEnd(SymbolRef E) const { return ContainerData(E); } + ContainerData newBegin(SymbolRef B) const { return ContainerData(B, End); } + + ContainerData newEnd(SymbolRef E) const { return ContainerData(Begin, E); } bool operator==(const ContainerData &X) const { - return End == X.End; + return Begin == X.Begin && End == X.End; } bool operator!=(const ContainerData &X) const { - return End != X.End; + return Begin != X.Begin || End != X.End; } void Profile(llvm::FoldingSetNodeID &ID) const { + ID.Add(Begin); ID.Add(End); } }; @@ -167,8 +181,9 @@ public: class IteratorChecker : public Checker<check::PreCall, check::PostCall, + check::PreStmt<CXXOperatorCallExpr>, check::PostStmt<MaterializeTemporaryExpr>, - check::DeadSymbols, + check::LiveSymbols, check::DeadSymbols, eval::Assume> { std::unique_ptr<BugType> OutOfRangeBugType; @@ -176,10 +191,22 @@ class IteratorChecker void handleComparison(CheckerContext &C, const SVal &RetVal, const SVal &LVal, const SVal &RVal, OverloadedOperatorKind Op) const; void verifyDereference(CheckerContext &C, const SVal &Val) const; + void handleIncrement(CheckerContext &C, const SVal &RetVal, const SVal &Iter, + bool Postfix) const; + void handleDecrement(CheckerContext &C, const SVal &RetVal, const SVal &Iter, + bool Postfix) const; + void handleRandomIncrOrDecr(CheckerContext &C, OverloadedOperatorKind Op, + const SVal &RetVal, const SVal &LHS, + const SVal &RHS) const; + void handleBegin(CheckerContext &C, const Expr *CE, const SVal &RetVal, + const SVal &Cont) const; void handleEnd(CheckerContext &C, const Expr *CE, const SVal &RetVal, const SVal &Cont) const; void assignToContainer(CheckerContext &C, const Expr *CE, const SVal &RetVal, const MemRegion *Cont) const; + void verifyRandomIncrOrDecr(CheckerContext &C, OverloadedOperatorKind Op, + const SVal &RetVal, const SVal &LHS, + const SVal &RHS) const; void reportOutOfRangeBug(const StringRef &Message, const SVal &Val, CheckerContext &C, ExplodedNode *ErrNode) const; @@ -196,8 +223,10 @@ public: void checkPreCall(const CallEvent &Call, CheckerContext &C) const; void checkPostCall(const CallEvent &Call, CheckerContext &C) const; + void checkPreStmt(const CXXOperatorCallExpr *COCE, CheckerContext &C) const; void checkPostStmt(const MaterializeTemporaryExpr *MTE, CheckerContext &C) const; + void checkLiveSymbols(ProgramStateRef State, SymbolReaper &SR) const; void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const; ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond, bool Assumption) const; @@ -217,9 +246,13 @@ namespace { bool isIteratorType(const QualType &Type); bool isIterator(const CXXRecordDecl *CRD); +bool isBeginCall(const FunctionDecl *Func); bool isEndCall(const FunctionDecl *Func); bool isSimpleComparisonOperator(OverloadedOperatorKind OK); bool isDereferenceOperator(OverloadedOperatorKind OK); +bool isIncrementOperator(OverloadedOperatorKind OK); +bool isDecrementOperator(OverloadedOperatorKind OK); +bool isRandomIncrOrDecrOperator(OverloadedOperatorKind OK); BinaryOperator::Opcode getOpcode(const SymExpr *SE); const RegionOrSymbol getRegionOrSymbol(const SVal &Val); const ProgramStateRef processComparison(ProgramStateRef State, @@ -230,7 +263,11 @@ const ProgramStateRef saveComparison(ProgramStateRef State, const SVal &RVal, bool Eq); const IteratorComparison *loadComparison(ProgramStateRef State, const SymExpr *Condition); +SymbolRef getContainerBegin(ProgramStateRef State, const MemRegion *Cont); SymbolRef getContainerEnd(ProgramStateRef State, const MemRegion *Cont); +ProgramStateRef createContainerBegin(ProgramStateRef State, + const MemRegion *Cont, + const SymbolRef Sym); ProgramStateRef createContainerEnd(ProgramStateRef State, const MemRegion *Cont, const SymbolRef Sym); const IteratorPosition *getIteratorPosition(ProgramStateRef State, @@ -255,6 +292,7 @@ const ContainerData *getContainerData(ProgramStateRef State, ProgramStateRef setContainerData(ProgramStateRef State, const MemRegion *Cont, const ContainerData &CData); bool isOutOfRange(ProgramStateRef State, const IteratorPosition &Pos); +bool isZero(ProgramStateRef State, const NonLoc &Val); } // namespace IteratorChecker::IteratorChecker() { @@ -272,6 +310,22 @@ void IteratorChecker::checkPreCall(const CallEvent &Call, if (Func->isOverloadedOperator()) { 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)); + } + } else { + if (Call.getNumArgs() >= 2) { + verifyRandomIncrOrDecr(C, Func->getOverloadedOperator(), + Call.getReturnValue(), Call.getArgSVal(0), + Call.getArgSVal(1)); + } + } + } else if (ChecksEnabled[CK_IteratorRangeChecker] && isDereferenceOperator(Func->getOverloadedOperator())) { // Check for dereference of out-of-range iterators if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) { @@ -300,6 +354,36 @@ void IteratorChecker::checkPostCall(const CallEvent &Call, handleComparison(C, Call.getReturnValue(), Call.getArgSVal(0), Call.getArgSVal(1), Op); } + } else if (isRandomIncrOrDecrOperator(Func->getOverloadedOperator())) { + if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) { + if (Call.getNumArgs() >= 1) { + handleRandomIncrOrDecr(C, Func->getOverloadedOperator(), + Call.getReturnValue(), + InstCall->getCXXThisVal(), Call.getArgSVal(0)); + } + } else { + if (Call.getNumArgs() >= 2) { + handleRandomIncrOrDecr(C, Func->getOverloadedOperator(), + Call.getReturnValue(), Call.getArgSVal(0), + Call.getArgSVal(1)); + } + } + } else if (isIncrementOperator(Func->getOverloadedOperator())) { + if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) { + handleIncrement(C, Call.getReturnValue(), InstCall->getCXXThisVal(), + Call.getNumArgs()); + } else { + handleIncrement(C, Call.getReturnValue(), Call.getArgSVal(0), + Call.getNumArgs()); + } + } else if (isDecrementOperator(Func->getOverloadedOperator())) { + if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) { + handleDecrement(C, Call.getReturnValue(), InstCall->getCXXThisVal(), + Call.getNumArgs()); + } else { + handleDecrement(C, Call.getReturnValue(), Call.getArgSVal(0), + Call.getNumArgs()); + } } } else { const auto *OrigExpr = Call.getOriginExpr(); @@ -315,6 +399,11 @@ void IteratorChecker::checkPostCall(const CallEvent &Call, return; if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) { + if (isBeginCall(Func)) { + handleBegin(C, OrigExpr, Call.getReturnValue(), + InstCall->getCXXThisVal()); + return; + } if (isEndCall(Func)) { handleEnd(C, OrigExpr, Call.getReturnValue(), InstCall->getCXXThisVal()); @@ -351,19 +440,80 @@ void IteratorChecker::checkPostCall(const CallEvent &Call, } } +void IteratorChecker::checkPreStmt(const CXXOperatorCallExpr *COCE, + CheckerContext &C) const { + const auto *ThisExpr = COCE->getArg(0); + + auto State = C.getState(); + const auto *LCtx = C.getLocationContext(); + + const auto CurrentThis = State->getSVal(ThisExpr, LCtx); + if (const auto *Reg = CurrentThis.getAsRegion()) { + if (!Reg->getAs<CXXTempObjectRegion>()) + return; + const auto OldState = C.getPredecessor()->getFirstPred()->getState(); + const auto OldThis = OldState->getSVal(ThisExpr, LCtx); + // FIXME: This solution is unreliable. It may happen that another checker + // subscribes to the pre-statement check of `CXXOperatorCallExpr` + // and adds a transition before us. The proper fix is to make the + // CFG provide a `ConstructionContext` for the `CXXOperatorCallExpr`, + // which would turn the corresponding `CFGStmt` element into a + // `CFGCXXRecordTypedCall` element, which will allow `ExprEngine` to + // foresee that the `begin()`/`end()` call constructs the object + // directly in the temporary region that `CXXOperatorCallExpr` takes + // as its implicit object argument. + const auto *Pos = getIteratorPosition(OldState, OldThis); + if (!Pos) + return; + State = setIteratorPosition(State, CurrentThis, *Pos); + C.addTransition(State); + } +} + void IteratorChecker::checkPostStmt(const MaterializeTemporaryExpr *MTE, CheckerContext &C) const { /* Transfer iterator state to temporary objects */ auto State = C.getState(); - const auto *LCtx = C.getLocationContext(); const auto *Pos = - getIteratorPosition(State, State->getSVal(MTE->GetTemporaryExpr(), LCtx)); + getIteratorPosition(State, C.getSVal(MTE->GetTemporaryExpr())); if (!Pos) return; - State = setIteratorPosition(State, State->getSVal(MTE, LCtx), *Pos); + State = setIteratorPosition(State, C.getSVal(MTE), *Pos); C.addTransition(State); } +void IteratorChecker::checkLiveSymbols(ProgramStateRef State, + SymbolReaper &SR) const { + // Keep symbolic expressions of iterator positions, container begins and ends + // alive + auto RegionMap = State->get<IteratorRegionMap>(); + for (const auto Reg : RegionMap) { + const auto Offset = Reg.second.getOffset(); + for (auto i = Offset->symbol_begin(); i != Offset->symbol_end(); ++i) + if (isa<SymbolData>(*i)) + SR.markLive(*i); + } + + auto SymbolMap = State->get<IteratorSymbolMap>(); + for (const auto Sym : SymbolMap) { + const auto Offset = Sym.second.getOffset(); + for (auto i = Offset->symbol_begin(); i != Offset->symbol_end(); ++i) + if (isa<SymbolData>(*i)) + SR.markLive(*i); + } + + auto ContMap = State->get<ContainerMap>(); + for (const auto Cont : ContMap) { + const auto CData = Cont.second; + if (CData.getBegin()) { + SR.markLive(CData.getBegin()); + } + if (CData.getEnd()) { + SR.markLive(CData.getEnd()); + } + } +} + void IteratorChecker::checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const { // Cleanup @@ -471,13 +621,209 @@ void IteratorChecker::verifyDereference(CheckerContext &C, static CheckerProgramPointTag Tag("IteratorRangeChecker", "IteratorOutOfRange"); auto *N = C.generateNonFatalErrorNode(State, &Tag); - if (!N) { + if (!N) return; - } reportOutOfRangeBug("Iterator accessed outside of its range.", Val, C, N); } } +void IteratorChecker::handleIncrement(CheckerContext &C, const SVal &RetVal, + const SVal &Iter, bool Postfix) const { + // Increment the symbolic expressions which represents the position of the + // iterator + auto State = C.getState(); + const auto *Pos = getIteratorPosition(State, Iter); + 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); + State = setIteratorPosition(State, Iter, NewPos); + State = setIteratorPosition(State, RetVal, Postfix ? *Pos : NewPos); + C.addTransition(State); + } +} + +void IteratorChecker::handleDecrement(CheckerContext &C, const SVal &RetVal, + const SVal &Iter, bool Postfix) const { + // Decrement the symbolic expressions which represents the position of the + // iterator + auto State = C.getState(); + const auto *Pos = getIteratorPosition(State, Iter); + 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); + State = setIteratorPosition(State, Iter, NewPos); + State = setIteratorPosition(State, RetVal, Postfix ? *Pos : NewPos); + C.addTransition(State); + } +} + +// This function tells the analyzer's engine that symbols produced by our +// checker, most notably iterator positions, are relatively small. +// A distance between items in the container should not be very large. +// By assuming that it is within around 1/8 of the address space, +// we can help the analyzer perform operations on these symbols +// without being afraid of integer overflows. +// FIXME: Should we provide it as an API, so that all checkers could use it? +static ProgramStateRef assumeNoOverflow(ProgramStateRef State, SymbolRef Sym, + long Scale) { + SValBuilder &SVB = State->getStateManager().getSValBuilder(); + BasicValueFactory &BV = SVB.getBasicValueFactory(); + + QualType T = Sym->getType(); + assert(T->isSignedIntegerOrEnumerationType()); + APSIntType AT = BV.getAPSIntType(T); + + ProgramStateRef NewState = State; + + llvm::APSInt Max = AT.getMaxValue() / AT.getValue(Scale); + SVal IsCappedFromAbove = + SVB.evalBinOpNN(State, BO_LE, nonloc::SymbolVal(Sym), + nonloc::ConcreteInt(Max), SVB.getConditionType()); + if (auto DV = IsCappedFromAbove.getAs<DefinedSVal>()) { + NewState = NewState->assume(*DV, true); + if (!NewState) + return State; + } + + llvm::APSInt Min = -Max; + SVal IsCappedFromBelow = + SVB.evalBinOpNN(State, BO_GE, nonloc::SymbolVal(Sym), + nonloc::ConcreteInt(Min), SVB.getConditionType()); + if (auto DV = IsCappedFromBelow.getAs<DefinedSVal>()) { + NewState = NewState->assume(*DV, true); + if (!NewState) + return State; + } + + return NewState; +} + +void IteratorChecker::handleRandomIncrOrDecr(CheckerContext &C, + OverloadedOperatorKind Op, + const SVal &RetVal, + const SVal &LHS, + const SVal &RHS) const { + // Increment or decrement the symbolic expressions which represents the + // position of the iterator + auto State = C.getState(); + const auto *Pos = getIteratorPosition(State, LHS); + if (!Pos) + return; + + const auto *value = &RHS; + if (auto loc = RHS.getAs<Loc>()) { + const auto val = State->getRawSVal(*loc); + 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); + C.addTransition(State); +} + +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)) + return; + + auto value = RHS; + if (auto loc = RHS.getAs<Loc>()) { + value = State->getRawSVal(*loc); + } + + // Incremention or decremention by 0 is never bug + if (isZero(State, value.castAs<NonLoc>())) + 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) + 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)) { + auto *N = C.generateNonFatalErrorNode(State); + if (!N) + return; + reportOutOfRangeBug("Iterator accessed past its end.", LHS, C, N); + } +} + +void IteratorChecker::handleBegin(CheckerContext &C, const Expr *CE, + const SVal &RetVal, const SVal &Cont) const { + const auto *ContReg = Cont.getAsRegion(); + if (!ContReg) + return; + + while (const auto *CBOR = ContReg->getAs<CXXBaseObjectRegion>()) { + ContReg = CBOR->getSuperRegion(); + } + + // If the container already has a begin symbol then use it. Otherwise first + // create a new one. + auto State = C.getState(); + auto BeginSym = getContainerBegin(State, ContReg); + if (!BeginSym) { + auto &SymMgr = C.getSymbolManager(); + BeginSym = SymMgr.conjureSymbol(CE, C.getLocationContext(), + C.getASTContext().LongTy, C.blockCount()); + State = assumeNoOverflow(State, BeginSym, 4); + State = createContainerBegin(State, ContReg, BeginSym); + } + State = setIteratorPosition(State, RetVal, + IteratorPosition::getPosition(ContReg, BeginSym)); + C.addTransition(State); +} + void IteratorChecker::handleEnd(CheckerContext &C, const Expr *CE, const SVal &RetVal, const SVal &Cont) const { const auto *ContReg = Cont.getAsRegion(); @@ -496,6 +842,7 @@ void IteratorChecker::handleEnd(CheckerContext &C, const Expr *CE, auto &SymMgr = C.getSymbolManager(); EndSym = SymMgr.conjureSymbol(CE, C.getLocationContext(), C.getASTContext().LongTy, C.blockCount()); + State = assumeNoOverflow(State, EndSym, 4); State = createContainerEnd(State, ContReg, EndSym); } State = setIteratorPosition(State, RetVal, @@ -514,6 +861,7 @@ void IteratorChecker::assignToContainer(CheckerContext &C, const Expr *CE, auto &SymMgr = C.getSymbolManager(); auto Sym = SymMgr.conjureSymbol(CE, C.getLocationContext(), C.getASTContext().LongTy, C.blockCount()); + State = assumeNoOverflow(State, Sym, 4); State = setIteratorPosition(State, RetVal, IteratorPosition::getPosition(Cont, Sym)); C.addTransition(State); @@ -529,9 +877,12 @@ void IteratorChecker::reportOutOfRangeBug(const StringRef &Message, namespace { +bool isLess(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2); bool isGreaterOrEqual(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, + BinaryOperator::Opcode Opc); bool isIteratorType(const QualType &Type) { if (Type->isPointerType()) @@ -585,6 +936,13 @@ bool isIterator(const CXXRecordDecl *CRD) { HasPostIncrOp && HasDerefOp; } +bool isBeginCall(const FunctionDecl *Func) { + const auto *IdInfo = Func->getIdentifier(); + if (!IdInfo) + return false; + return IdInfo->getName().endswith_lower("begin"); +} + bool isEndCall(const FunctionDecl *Func) { const auto *IdInfo = Func->getIdentifier(); if (!IdInfo) @@ -601,11 +959,24 @@ bool isDereferenceOperator(OverloadedOperatorKind OK) { OK == OO_Subscript; } +bool isIncrementOperator(OverloadedOperatorKind OK) { + return OK == OO_PlusPlus; +} + +bool isDecrementOperator(OverloadedOperatorKind OK) { + return OK == OO_MinusMinus; +} + +bool isRandomIncrOrDecrOperator(OverloadedOperatorKind OK) { + return OK == OO_Plus || OK == OO_PlusEqual || OK == OO_Minus || + OK == OO_MinusEqual; +} + BinaryOperator::Opcode getOpcode(const SymExpr *SE) { if (const auto *BSE = dyn_cast<BinarySymExpr>(SE)) { return BSE->getOpcode(); } else if (const auto *SC = dyn_cast<SymbolConjured>(SE)) { - const auto *COE = dyn_cast<CXXOperatorCallExpr>(SC->getStmt()); + const auto *COE = dyn_cast_or_null<CXXOperatorCallExpr>(SC->getStmt()); if (!COE) return BO_Comma; // Extremal value, neither EQ nor NE if (COE->getOperator() == OO_EqualEqual) { @@ -660,6 +1031,14 @@ const IteratorComparison *loadComparison(ProgramStateRef State, return State->get<IteratorComparisonMap>(Condition); } +SymbolRef getContainerBegin(ProgramStateRef State, const MemRegion *Cont) { + const auto *CDataPtr = getContainerData(State, Cont); + if (!CDataPtr) + return nullptr; + + return CDataPtr->getBegin(); +} + SymbolRef getContainerEnd(ProgramStateRef State, const MemRegion *Cont) { const auto *CDataPtr = getContainerData(State, Cont); if (!CDataPtr) @@ -668,6 +1047,22 @@ SymbolRef getContainerEnd(ProgramStateRef State, const MemRegion *Cont) { return CDataPtr->getEnd(); } +ProgramStateRef createContainerBegin(ProgramStateRef State, + const MemRegion *Cont, + const SymbolRef Sym) { + // Only create if it does not exist + const auto *CDataPtr = getContainerData(State, Cont); + if (CDataPtr) { + if (CDataPtr->getBegin()) { + return State; + } + const auto CData = CDataPtr->newBegin(Sym); + return setContainerData(State, Cont, CData); + } + const auto CData = ContainerData::fromBegin(Sym); + return setContainerData(State, Cont, CData); +} + ProgramStateRef createContainerEnd(ProgramStateRef State, const MemRegion *Cont, const SymbolRef Sym) { // Only create if it does not exist @@ -675,14 +1070,12 @@ ProgramStateRef createContainerEnd(ProgramStateRef State, const MemRegion *Cont, if (CDataPtr) { if (CDataPtr->getEnd()) { return State; - } else { - const auto CData = CDataPtr->newEnd(Sym); - return setContainerData(State, Cont, CData); } - } else { - const auto CData = ContainerData::fromEnd(Sym); + const auto CData = CDataPtr->newEnd(Sym); return setContainerData(State, Cont, CData); } + const auto CData = ContainerData::fromEnd(Sym); + return setContainerData(State, Cont, CData); } const ContainerData *getContainerData(ProgramStateRef State, @@ -767,17 +1160,39 @@ ProgramStateRef relateIteratorPositions(ProgramStateRef State, const IteratorPosition &Pos1, const IteratorPosition &Pos2, bool Equal) { - // Try to compare them and get a defined value auto &SVB = State->getStateManager().getSValBuilder(); + + // FIXME: This code should be reworked as follows: + // 1. Subtract the operands using evalBinOp(). + // 2. Assume that the result doesn't overflow. + // 3. Compare the result to 0. + // 4. Assume the result of the comparison. const auto comparison = SVB.evalBinOp(State, BO_EQ, nonloc::SymbolVal(Pos1.getOffset()), - nonloc::SymbolVal(Pos2.getOffset()), SVB.getConditionType()) - .getAs<DefinedSVal>(); - if (comparison) { - return State->assume(*comparison, Equal); + nonloc::SymbolVal(Pos2.getOffset()), + SVB.getConditionType()); + + assert(comparison.getAs<DefinedSVal>() && + "Symbol comparison must be a `DefinedSVal`"); + + auto NewState = State->assume(comparison.castAs<DefinedSVal>(), Equal); + if (const auto CompSym = comparison.getAsSymbol()) { + assert(isa<SymIntExpr>(CompSym) && + "Symbol comparison must be a `SymIntExpr`"); + assert(BinaryOperator::isComparisonOp( + cast<SymIntExpr>(CompSym)->getOpcode()) && + "Symbol comparison must be a comparison"); + return assumeNoOverflow(NewState, cast<SymIntExpr>(CompSym)->getLHS(), 2); } - return State; + return NewState; +} + +bool isZero(ProgramStateRef State, const NonLoc &Val) { + auto &BVF = State->getBasicVals(); + return compare(State, Val, + nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(0))), + BO_EQ); } bool isOutOfRange(ProgramStateRef State, const IteratorPosition &Pos) { @@ -789,6 +1204,13 @@ bool isOutOfRange(ProgramStateRef State, const IteratorPosition &Pos) { // Out of range means less than the begin symbol or greater or equal to the // end symbol. + const auto Beg = CData->getBegin(); + if (Beg) { + if (isLess(State, Pos.getOffset(), Beg)) { + return true; + } + } + const auto End = CData->getEnd(); if (End) { if (isGreaterOrEqual(State, Pos.getOffset(), End)) { @@ -799,25 +1221,30 @@ bool isOutOfRange(ProgramStateRef State, const IteratorPosition &Pos) { return false; } +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 compare(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2, BinaryOperator::Opcode Opc) { - auto &SMgr = State->getStateManager(); - auto &SVB = SMgr.getSValBuilder(); + return compare(State, nonloc::SymbolVal(Sym1), nonloc::SymbolVal(Sym2), Opc); +} + +bool compare(ProgramStateRef State, NonLoc NL1, NonLoc NL2, + BinaryOperator::Opcode Opc) { + auto &SVB = State->getStateManager().getSValBuilder(); const auto comparison = - SVB.evalBinOp(State, Opc, nonloc::SymbolVal(Sym1), - nonloc::SymbolVal(Sym2), SVB.getConditionType()) - .getAs<DefinedSVal>(); + SVB.evalBinOp(State, Opc, NL1, NL2, SVB.getConditionType()); - if(comparison) { - return !!State->assume(*comparison, true); - } + assert(comparison.getAs<DefinedSVal>() && + "Symbol comparison must be a `DefinedSVal`"); - return false; + return !State->assume(comparison.castAs<DefinedSVal>(), false); } } // namespace @@ -831,3 +1258,4 @@ bool compare(ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2, } REGISTER_CHECKER(IteratorRangeChecker) + diff --git a/lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp b/lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp index 8076ca09591f..2fb627184eb9 100644 --- a/lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp @@ -20,7 +20,7 @@ // been called on them. An invalidation method should either invalidate all // the ivars or call another invalidation method (on self). // -// Partial invalidor annotation allows to addess cases when ivars are +// Partial invalidor annotation allows to address cases when ivars are // invalidated by other methods, which might or might not be called from // the invalidation method. The checker checks that each invalidation // method and all the partial methods cumulatively invalidate all ivars. @@ -402,13 +402,13 @@ visit(const ObjCImplementationDecl *ImplD) const { // Find the setter and the getter. const ObjCMethodDecl *SetterD = PD->getSetterMethodDecl(); if (SetterD) { - SetterD = cast<ObjCMethodDecl>(SetterD->getCanonicalDecl()); + SetterD = SetterD->getCanonicalDecl(); PropSetterToIvarMap[SetterD] = ID; } const ObjCMethodDecl *GetterD = PD->getGetterMethodDecl(); if (GetterD) { - GetterD = cast<ObjCMethodDecl>(GetterD->getCanonicalDecl()); + GetterD = GetterD->getCanonicalDecl(); PropGetterToIvarMap[GetterD] = ID; } } @@ -606,7 +606,7 @@ void IvarInvalidationCheckerImpl::MethodCrawler::checkObjCMessageExpr( const ObjCMessageExpr *ME) { const ObjCMethodDecl *MD = ME->getMethodDecl(); if (MD) { - MD = cast<ObjCMethodDecl>(MD->getCanonicalDecl()); + MD = MD->getCanonicalDecl(); MethToIvarMapTy::const_iterator IvI = PropertyGetterToIvarMap.find(MD); if (IvI != PropertyGetterToIvarMap.end()) markInvalidated(IvI->second); @@ -630,7 +630,7 @@ void IvarInvalidationCheckerImpl::MethodCrawler::checkObjCPropertyRefExpr( if (PA->isImplicitProperty()) { const ObjCMethodDecl *MD = PA->getImplicitPropertySetter(); if (MD) { - MD = cast<ObjCMethodDecl>(MD->getCanonicalDecl()); + MD = MD->getCanonicalDecl(); MethToIvarMapTy::const_iterator IvI =PropertyGetterToIvarMap.find(MD); if (IvI != PropertyGetterToIvarMap.end()) markInvalidated(IvI->second); @@ -702,7 +702,7 @@ void IvarInvalidationCheckerImpl::MethodCrawler::VisitObjCMessageExpr( // Check if we call a setter and set the property to 'nil'. if (MD && (ME->getNumArgs() == 1) && isZero(ME->getArg(0))) { - MD = cast<ObjCMethodDecl>(MD->getCanonicalDecl()); + MD = MD->getCanonicalDecl(); MethToIvarMapTy::const_iterator IvI = PropertySetterToIvarMap.find(MD); if (IvI != PropertySetterToIvarMap.end()) { markInvalidated(IvI->second); diff --git a/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp b/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp index 655ce33390c9..849b1193c042 100644 --- a/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp @@ -113,8 +113,7 @@ NonLocalizedStringChecker::NonLocalizedStringChecker() { } namespace { -class NonLocalizedStringBRVisitor final - : public BugReporterVisitorImpl<NonLocalizedStringBRVisitor> { +class NonLocalizedStringBRVisitor final : public BugReporterVisitor { const MemRegion *NonLocalizedString; bool Satisfied; @@ -1017,8 +1016,7 @@ NonLocalizedStringBRVisitor::VisitNode(const ExplodedNode *Succ, if (!LiteralExpr) return nullptr; - ProgramStateRef State = Succ->getState(); - SVal LiteralSVal = State->getSVal(LiteralExpr, Succ->getLocationContext()); + SVal LiteralSVal = Succ->getSVal(LiteralExpr); if (LiteralSVal.getAsRegion() != NonLocalizedString) return nullptr; @@ -1108,7 +1106,7 @@ void EmptyLocalizationContextChecker::checkASTDecl( void EmptyLocalizationContextChecker::MethodCrawler::VisitObjCMessageExpr( const ObjCMessageExpr *ME) { - // FIXME: We may be able to use PPCallbacks to check for empy context + // FIXME: We may be able to use PPCallbacks to check for empty context // comments as part of preprocessing and avoid this re-lexing hack. const ObjCInterfaceDecl *OD = ME->getReceiverInterface(); if (!OD) @@ -1389,7 +1387,7 @@ void PluralMisuseChecker::MethodCrawler::reportPluralMisuseError( // Generate the bug report. BR.EmitBasicReport(AC->getDecl(), Checker, "Plural Misuse", "Localizability Issue (Apple)", - "Plural cases are not supported accross all languages. " + "Plural cases are not supported across all languages. " "Use a .stringsdict file instead", PathDiagnosticLocation(S, BR.getSourceManager(), AC)); } diff --git a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h index 0ee91cca4793..40eb0631d7c5 100644 --- a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h +++ b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h @@ -78,7 +78,7 @@ private: /// Bug visitor class to find the node where the request region was previously /// used in order to include it into the BugReport path. - class RequestNodeVisitor : public BugReporterVisitorImpl<RequestNodeVisitor> { + class RequestNodeVisitor : public BugReporterVisitor { public: RequestNodeVisitor(const MemRegion *const MemoryRegion, const std::string &ErrText) diff --git a/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp b/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp index f8473dbd7647..b8ef6701c0df 100644 --- a/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp @@ -120,8 +120,7 @@ private: /// The bug visitor which allows us to print extra diagnostics along the /// BugReport path. For example, showing the allocation site of the leaked /// region. - class SecKeychainBugVisitor - : public BugReporterVisitorImpl<SecKeychainBugVisitor> { + class SecKeychainBugVisitor : public BugReporterVisitor { protected: // The allocated region symbol tracked by the main analysis. SymbolRef Sym; @@ -202,7 +201,7 @@ static bool isBadDeallocationArgument(const MemRegion *Arg) { static SymbolRef getAsPointeeSymbol(const Expr *Expr, CheckerContext &C) { ProgramStateRef State = C.getState(); - SVal ArgV = State->getSVal(Expr, C.getLocationContext()); + SVal ArgV = C.getSVal(Expr); if (Optional<loc::MemRegionVal> X = ArgV.getAs<loc::MemRegionVal>()) { StoreManager& SM = C.getStoreManager(); @@ -297,7 +296,7 @@ void MacOSKeychainAPIChecker::checkPreStmt(const CallExpr *CE, // Check the argument to the deallocator. const Expr *ArgExpr = CE->getArg(paramIdx); - SVal ArgSVal = State->getSVal(ArgExpr, C.getLocationContext()); + SVal ArgSVal = C.getSVal(ArgExpr); // Undef is reported by another checker. if (ArgSVal.isUndef()) @@ -426,8 +425,7 @@ void MacOSKeychainAPIChecker::checkPostStmt(const CallExpr *CE, // allocated value symbol, since our diagnostics depend on the value // returned by the call. Ex: Data should only be freed if noErr was // returned during allocation.) - SymbolRef RetStatusSymbol = - State->getSVal(CE, C.getLocationContext()).getAsSymbol(); + SymbolRef RetStatusSymbol = C.getSVal(CE).getAsSymbol(); C.getSymbolManager().addSymbolDependency(V, RetStatusSymbol); // Track the allocated value in the checker state. diff --git a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 851114004b96..8f07f413e81f 100644 --- a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -30,6 +30,7 @@ #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallString.h" #include "llvm/ADT/StringExtras.h" +#include "AllocationState.h" #include <climits> #include <utility> @@ -45,7 +46,8 @@ enum AllocationFamily { AF_CXXNew, AF_CXXNewArray, AF_IfNameIndex, - AF_Alloca + AF_Alloca, + AF_InnerBuffer }; class RefState { @@ -134,10 +136,10 @@ enum ReallocPairKind { }; /// \class ReallocPair -/// \brief Stores information about the symbol being reallocated by a call to +/// Stores information about the symbol being reallocated by a call to /// 'realloc' to allow modeling failed reallocation later in the path. struct ReallocPair { - // \brief The symbol which realloc reallocated. + // The symbol which realloc reallocated. SymbolRef ReallocatedSym; ReallocPairKind Kind; @@ -162,6 +164,7 @@ class MallocChecker : public Checker<check::DeadSymbols, check::PreCall, check::PostStmt<CallExpr>, check::PostStmt<CXXNewExpr>, + check::NewAllocator, check::PreStmt<CXXDeleteExpr>, check::PostStmt<BlockExpr>, check::PostObjCMessage, @@ -207,6 +210,8 @@ public: void checkPreCall(const CallEvent &Call, CheckerContext &C) const; void checkPostStmt(const CallExpr *CE, CheckerContext &C) const; void checkPostStmt(const CXXNewExpr *NE, CheckerContext &C) const; + void checkNewAllocator(const CXXNewExpr *NE, SVal Target, + CheckerContext &C) const; void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const; void checkPostObjCMessage(const ObjCMethodCall &Call, CheckerContext &C) const; void checkPostStmt(const BlockExpr *BE, CheckerContext &C) const; @@ -253,20 +258,20 @@ private: void initIdentifierInfo(ASTContext &C) const; - /// \brief Determine family of a deallocation expression. + /// Determine family of a deallocation expression. AllocationFamily getAllocationFamily(CheckerContext &C, const Stmt *S) const; - /// \brief Print names of allocators and deallocators. + /// Print names of allocators and deallocators. /// /// \returns true on success. bool printAllocDeallocName(raw_ostream &os, CheckerContext &C, const Expr *E) const; - /// \brief Print expected name of an allocator based on the deallocator's + /// Print expected name of an allocator based on the deallocator's /// family derived from the DeallocExpr. void printExpectedAllocName(raw_ostream &os, CheckerContext &C, const Expr *DeallocExpr) const; - /// \brief Print expected name of a deallocator based on the allocator's + /// Print expected name of a deallocator based on the allocator's /// family. void printExpectedDeallocName(raw_ostream &os, AllocationFamily Family) const; @@ -281,10 +286,18 @@ private: bool isStandardNewDelete(const FunctionDecl *FD, ASTContext &C) const; ///@} - /// \brief Perform a zero-allocation check. + /// Process C++ operator new()'s allocation, which is the part of C++ + /// new-expression that goes before the constructor. + void processNewAllocation(const CXXNewExpr *NE, CheckerContext &C, + SVal Target) const; + + /// Perform a zero-allocation check. + /// The optional \p RetVal parameter specifies the newly allocated pointer + /// value; if unspecified, the value of expression \p E is used. ProgramStateRef ProcessZeroAllocation(CheckerContext &C, const Expr *E, const unsigned AllocationSizeArg, - ProgramStateRef State) const; + ProgramStateRef State, + Optional<SVal> RetVal = None) const; ProgramStateRef MallocMemReturnsAttr(CheckerContext &C, const CallExpr *CE, @@ -300,7 +313,7 @@ private: AllocationFamily Family = AF_Malloc); static ProgramStateRef addExtentSize(CheckerContext &C, const CXXNewExpr *NE, - ProgramStateRef State); + ProgramStateRef State, SVal Target); // Check if this malloc() for special flags. At present that means M_ZERO or // __GFP_ZERO (in which case, treat it like calloc). @@ -309,9 +322,12 @@ private: const ProgramStateRef &State) const; /// Update the RefState to reflect the new memory allocation. + /// The optional \p RetVal parameter specifies the newly allocated pointer + /// value; if unspecified, the value of expression \p E is used. static ProgramStateRef MallocUpdateRefState(CheckerContext &C, const Expr *E, ProgramStateRef State, - AllocationFamily Family = AF_Malloc); + AllocationFamily Family = AF_Malloc, + Optional<SVal> RetVal = None); ProgramStateRef FreeMemAttr(CheckerContext &C, const CallExpr *CE, const OwnershipAttr* Att, @@ -337,7 +353,7 @@ private: static ProgramStateRef CallocMem(CheckerContext &C, const CallExpr *CE, ProgramStateRef State); - ///\brief Check if the memory associated with this symbol was released. + ///Check if the memory associated with this symbol was released. bool isReleased(SymbolRef Sym, CheckerContext &C) const; bool checkUseAfterFree(SymbolRef Sym, CheckerContext &C, const Stmt *S) const; @@ -415,8 +431,7 @@ private: /// The bug visitor which allows us to print extra diagnostics along the /// BugReport path. For example, showing the allocation site of the leaked /// region. - class MallocBugVisitor final - : public BugReporterVisitorImpl<MallocBugVisitor> { + class MallocBugVisitor final : public BugReporterVisitor { protected: enum NotificationMode { Normal, @@ -432,15 +447,24 @@ private: // A symbol from when the primary region should have been reallocated. SymbolRef FailedReallocSymbol; + // A C++ destructor stack frame in which memory was released. Used for + // miscellaneous false positive suppression. + const StackFrameContext *ReleaseDestructorLC; + bool IsLeak; public: MallocBugVisitor(SymbolRef S, bool isLeak = false) - : Sym(S), Mode(Normal), FailedReallocSymbol(nullptr), IsLeak(isLeak) {} + : Sym(S), Mode(Normal), FailedReallocSymbol(nullptr), + ReleaseDestructorLC(nullptr), IsLeak(isLeak) {} + + static void *getTag() { + static int Tag = 0; + return &Tag; + } void Profile(llvm::FoldingSetNodeID &ID) const override { - static int X = 0; - ID.AddPointer(&X); + ID.AddPointer(getTag()); ID.AddPointer(Sym); } @@ -456,8 +480,13 @@ private: inline bool isReleased(const RefState *S, const RefState *SPrev, const Stmt *Stmt) { // Did not track -> released. Other state (allocated) -> released. - return (Stmt && (isa<CallExpr>(Stmt) || isa<CXXDeleteExpr>(Stmt)) && - (S && S->isReleased()) && (!SPrev || !SPrev->isReleased())); + // The statement associated with the release might be missing. + bool IsReleased = (S && S->isReleased()) && + (!SPrev || !SPrev->isReleased()); + assert(!IsReleased || + (Stmt && (isa<CallExpr>(Stmt) || isa<CXXDeleteExpr>(Stmt))) || + (!Stmt && S->getAllocationFamily() == AF_InnerBuffer)); + return IsReleased; } inline bool isRelinquished(const RefState *S, const RefState *SPrev, @@ -486,7 +515,7 @@ private: BugReporterContext &BRC, BugReport &BR) override; - std::unique_ptr<PathDiagnosticPiece> + std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC, const ExplodedNode *EndPathNode, BugReport &BR) override { if (!IsLeak) @@ -496,7 +525,7 @@ private: PathDiagnosticLocation::createEndOfPath(EndPathNode, BRC.getSourceManager()); // Do not add the statement itself as a range in case of leak. - return llvm::make_unique<PathDiagnosticEventPiece>(L, BR.getDescription(), + return std::make_shared<PathDiagnosticEventPiece>(L, BR.getDescription(), false); } @@ -758,7 +787,7 @@ llvm::Optional<ProgramStateRef> MallocChecker::performKernelMalloc( return None; const Expr *FlagsEx = CE->getArg(CE->getNumArgs() - 1); - const SVal V = State->getSVal(FlagsEx, C.getLocationContext()); + const SVal V = C.getSVal(FlagsEx); if (!V.getAs<NonLoc>()) { // The case where 'V' can be a location can only be due to a bad header, // so in this case bail out. @@ -949,13 +978,15 @@ void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const { } // Performs a 0-sized allocations check. -ProgramStateRef MallocChecker::ProcessZeroAllocation(CheckerContext &C, - const Expr *E, - const unsigned AllocationSizeArg, - ProgramStateRef State) const { +ProgramStateRef MallocChecker::ProcessZeroAllocation( + CheckerContext &C, const Expr *E, const unsigned AllocationSizeArg, + ProgramStateRef State, Optional<SVal> RetVal) const { if (!State) return nullptr; + if (!RetVal) + RetVal = C.getSVal(E); + const Expr *Arg = nullptr; if (const CallExpr *CE = dyn_cast<CallExpr>(E)) { @@ -972,8 +1003,7 @@ ProgramStateRef MallocChecker::ProcessZeroAllocation(CheckerContext &C, assert(Arg); - Optional<DefinedSVal> DefArgVal = - State->getSVal(Arg, C.getLocationContext()).getAs<DefinedSVal>(); + Optional<DefinedSVal> DefArgVal = C.getSVal(Arg).getAs<DefinedSVal>(); if (!DefArgVal) return State; @@ -988,8 +1018,7 @@ ProgramStateRef MallocChecker::ProcessZeroAllocation(CheckerContext &C, State->assume(SvalBuilder.evalEQ(State, *DefArgVal, Zero)); if (TrueState && !FalseState) { - SVal retVal = State->getSVal(E, C.getLocationContext()); - SymbolRef Sym = retVal.getAsLocSymbol(); + SymbolRef Sym = RetVal->getAsLocSymbol(); if (!Sym) return State; @@ -1050,9 +1079,9 @@ static bool treatUnusedNewEscaped(const CXXNewExpr *NE) { return false; } -void MallocChecker::checkPostStmt(const CXXNewExpr *NE, - CheckerContext &C) const { - +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) @@ -1072,37 +1101,48 @@ void MallocChecker::checkPostStmt(const CXXNewExpr *NE, // MallocUpdateRefState() instead of MallocMemAux() which breakes the // existing binding. State = MallocUpdateRefState(C, NE, State, NE->isArray() ? AF_CXXNewArray - : AF_CXXNew); - State = addExtentSize(C, NE, State); - State = ProcessZeroAllocation(C, NE, 0, State); + : AF_CXXNew, Target); + State = addExtentSize(C, NE, State, Target); + State = ProcessZeroAllocation(C, NE, 0, State, Target); C.addTransition(State); } +void MallocChecker::checkPostStmt(const CXXNewExpr *NE, + CheckerContext &C) const { + if (!C.getAnalysisManager().getAnalyzerOptions().mayInlineCXXAllocator()) + processNewAllocation(NE, C, C.getSVal(NE)); +} + +void MallocChecker::checkNewAllocator(const CXXNewExpr *NE, SVal Target, + CheckerContext &C) const { + if (!C.wasInlined) + processNewAllocation(NE, C, Target); +} + // Sets the extent value of the MemRegion allocated by // new expression NE to its size in Bytes. // ProgramStateRef MallocChecker::addExtentSize(CheckerContext &C, const CXXNewExpr *NE, - ProgramStateRef State) { + ProgramStateRef State, + SVal Target) { if (!State) return nullptr; SValBuilder &svalBuilder = C.getSValBuilder(); SVal ElementCount; - const LocationContext *LCtx = C.getLocationContext(); const SubRegion *Region; if (NE->isArray()) { const Expr *SizeExpr = NE->getArraySize(); - ElementCount = State->getSVal(SizeExpr, C.getLocationContext()); + ElementCount = C.getSVal(SizeExpr); // Store the extent size for the (symbolic)region // containing the elements. - Region = (State->getSVal(NE, LCtx)) - .getAsRegion() + Region = Target.getAsRegion() ->getAs<SubRegion>() - ->getSuperRegion() + ->StripCasts() ->getAs<SubRegion>(); } else { ElementCount = svalBuilder.makeIntVal(1, true); - Region = (State->getSVal(NE, LCtx)).getAsRegion()->getAs<SubRegion>(); + Region = Target.getAsRegion()->getAs<SubRegion>(); } assert(Region); @@ -1199,7 +1239,8 @@ MallocChecker::MallocMemReturnsAttr(CheckerContext &C, const CallExpr *CE, OwnershipAttr::args_iterator I = Att->args_begin(), E = Att->args_end(); if (I != E) { - return MallocMemAux(C, CE, CE->getArg(*I), UndefinedVal(), State); + return MallocMemAux(C, CE, CE->getArg(I->getASTIndex()), UndefinedVal(), + State); } return MallocMemAux(C, CE, UnknownVal(), UndefinedVal(), State); } @@ -1212,8 +1253,7 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C, if (!State) return nullptr; - return MallocMemAux(C, CE, State->getSVal(SizeEx, C.getLocationContext()), - Init, State, Family); + return MallocMemAux(C, CE, C.getSVal(SizeEx), Init, State, Family); } ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C, @@ -1239,7 +1279,7 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C, State = State->BindExpr(CE, C.getLocationContext(), RetVal); // Fill the region with the initialization value. - State = State->bindDefault(RetVal, Init, LCtx); + State = State->bindDefaultInitial(RetVal, Init, LCtx); // Set the region's extent equal to the Size parameter. const SymbolicRegion *R = @@ -1263,18 +1303,22 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C, ProgramStateRef MallocChecker::MallocUpdateRefState(CheckerContext &C, const Expr *E, ProgramStateRef State, - AllocationFamily Family) { + AllocationFamily Family, + Optional<SVal> RetVal) { if (!State) return nullptr; // Get the return value. - SVal retVal = State->getSVal(E, C.getLocationContext()); + if (!RetVal) + RetVal = C.getSVal(E); // We expect the malloc functions to return a pointer. - if (!retVal.getAs<Loc>()) + if (!RetVal->getAs<Loc>()) return nullptr; - SymbolRef Sym = retVal.getAsLocSymbol(); + SymbolRef Sym = RetVal->getAsLocSymbol(); + // This is a return value of a function that was not inlined, such as malloc() + // or new(). We've checked that in the caller. Therefore, it must be a symbol. assert(Sym); // Set the symbol's state to Allocated. @@ -1294,9 +1338,9 @@ ProgramStateRef MallocChecker::FreeMemAttr(CheckerContext &C, bool ReleasedAllocated = false; for (const auto &Arg : Att->args()) { - ProgramStateRef StateI = FreeMemAux(C, CE, State, Arg, - Att->getOwnKind() == OwnershipAttr::Holds, - ReleasedAllocated); + ProgramStateRef StateI = FreeMemAux( + C, CE, State, Arg.getASTIndex(), + Att->getOwnKind() == OwnershipAttr::Holds, ReleasedAllocated); if (StateI) State = StateI; } @@ -1429,6 +1473,7 @@ void MallocChecker::printExpectedAllocName(raw_ostream &os, CheckerContext &C, case AF_CXXNew: os << "'new'"; return; case AF_CXXNewArray: os << "'new[]'"; return; case AF_IfNameIndex: os << "'if_nameindex()'"; return; + case AF_InnerBuffer: os << "container-specific allocator"; return; case AF_Alloca: case AF_None: llvm_unreachable("not a deallocation expression"); } @@ -1441,6 +1486,7 @@ void MallocChecker::printExpectedDeallocName(raw_ostream &os, case AF_CXXNew: os << "'delete'"; return; case AF_CXXNewArray: os << "'delete[]'"; return; case AF_IfNameIndex: os << "'if_freenameindex()'"; return; + case AF_InnerBuffer: os << "container-specific deallocator"; return; case AF_Alloca: case AF_None: llvm_unreachable("suspicious argument"); } @@ -1457,7 +1503,7 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C, if (!State) return nullptr; - SVal ArgVal = State->getSVal(ArgExpr, C.getLocationContext()); + SVal ArgVal = C.getSVal(ArgExpr); if (!ArgVal.getAs<DefinedOrUnknownSVal>()) return nullptr; DefinedOrUnknownSVal location = ArgVal.castAs<DefinedOrUnknownSVal>(); @@ -1615,7 +1661,9 @@ MallocChecker::getCheckIfTracked(AllocationFamily Family, return Optional<MallocChecker::CheckKind>(); } case AF_CXXNew: - case AF_CXXNewArray: { + case AF_CXXNewArray: + // FIXME: Add new CheckKind for AF_InnerBuffer. + case AF_InnerBuffer: { if (IsALeakCheck) { if (ChecksEnabled[CK_NewDeleteLeaksChecker]) return CK_NewDeleteLeaksChecker; @@ -1945,6 +1993,11 @@ void MallocChecker::ReportUseAfterFree(CheckerContext &C, SourceRange Range, R->markInteresting(Sym); R->addRange(Range); R->addVisitor(llvm::make_unique<MallocBugVisitor>(Sym)); + + const RefState *RS = C.getState()->get<RegionState>(Sym); + if (RS->getAllocationFamily() == AF_InnerBuffer) + R->addVisitor(allocation_state::getInnerPointerBRVisitor(Sym)); + C.emitReport(std::move(R)); } } @@ -2047,8 +2100,8 @@ void MallocChecker::ReportFunctionPointerFree(CheckerContext &C, SVal ArgVal, if (ExplodedNode *N = C.generateErrorNode()) { if (!BT_BadFree[*CheckKind]) - BT_BadFree[*CheckKind].reset( - new BugType(CheckNames[*CheckKind], "Bad free", "Memory Error")); + BT_BadFree[*CheckKind].reset(new BugType( + CheckNames[*CheckKind], "Bad free", categories::MemoryError)); SmallString<100> Buf; llvm::raw_svector_ostream Os(Buf); @@ -2084,8 +2137,7 @@ ProgramStateRef MallocChecker::ReallocMemAux(CheckerContext &C, return nullptr; const Expr *arg0Expr = CE->getArg(0); - const LocationContext *LCtx = C.getLocationContext(); - SVal Arg0Val = State->getSVal(arg0Expr, LCtx); + SVal Arg0Val = C.getSVal(arg0Expr); if (!Arg0Val.getAs<DefinedOrUnknownSVal>()) return nullptr; DefinedOrUnknownSVal arg0Val = Arg0Val.castAs<DefinedOrUnknownSVal>(); @@ -2099,7 +2151,7 @@ ProgramStateRef MallocChecker::ReallocMemAux(CheckerContext &C, const Expr *Arg1 = CE->getArg(1); // Get the value of the size argument. - SVal TotalSize = State->getSVal(Arg1, LCtx); + SVal TotalSize = C.getSVal(Arg1); if (SuffixWithN) TotalSize = evalMulForBufferSize(C, Arg1, CE->getArg(2)); if (!TotalSize.getAs<DefinedOrUnknownSVal>()) @@ -2133,7 +2185,7 @@ ProgramStateRef MallocChecker::ReallocMemAux(CheckerContext &C, // Get the from and to pointer symbols as in toPtr = realloc(fromPtr, size). assert(!PrtIsNull); SymbolRef FromPtr = arg0Val.getAsSymbol(); - SVal RetVal = State->getSVal(CE, LCtx); + SVal RetVal = C.getSVal(CE); SymbolRef ToPtr = RetVal.getAsSymbol(); if (!FromPtr || !ToPtr) return nullptr; @@ -2216,7 +2268,7 @@ MallocChecker::getAllocationSite(const ExplodedNode *N, SymbolRef Sym, // Do not show local variables belonging to a function other than // where the error is reported. if (!VR || - (VR->getStackFrame() == LeakContext->getCurrentStackFrame())) + (VR->getStackFrame() == LeakContext->getStackFrame())) ReferenceRegion = MR; } } @@ -2406,7 +2458,7 @@ void MallocChecker::checkPreStmt(const ReturnStmt *S, CheckerContext &C) const { // Check if we are returning a symbol. ProgramStateRef State = C.getState(); - SVal RetVal = State->getSVal(E, C.getLocationContext()); + SVal RetVal = C.getSVal(E); SymbolRef Sym = RetVal.getAsSymbol(); if (!Sym) // If we are returning a field of the allocated struct or an array element, @@ -2436,8 +2488,7 @@ void MallocChecker::checkPostStmt(const BlockExpr *BE, ProgramStateRef state = C.getState(); const BlockDataRegion *R = - cast<BlockDataRegion>(state->getSVal(BE, - C.getLocationContext()).getAsRegion()); + cast<BlockDataRegion>(C.getSVal(BE).getAsRegion()); BlockDataRegion::referenced_vars_iterator I = R->referenced_vars_begin(), E = R->referenced_vars_end(); @@ -2793,36 +2844,133 @@ static SymbolRef findFailedReallocSymbol(ProgramStateRef currState, return nullptr; } +static bool isReferenceCountingPointerDestructor(const CXXDestructorDecl *DD) { + if (const IdentifierInfo *II = DD->getParent()->getIdentifier()) { + StringRef N = II->getName(); + if (N.contains_lower("ptr") || N.contains_lower("pointer")) { + if (N.contains_lower("ref") || N.contains_lower("cnt") || + N.contains_lower("intrusive") || N.contains_lower("shared")) { + return true; + } + } + } + return false; +} + std::shared_ptr<PathDiagnosticPiece> MallocChecker::MallocBugVisitor::VisitNode( const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC, BugReport &BR) { + ProgramStateRef state = N->getState(); ProgramStateRef statePrev = PrevN->getState(); const RefState *RS = state->get<RegionState>(Sym); const RefState *RSPrev = statePrev->get<RegionState>(Sym); - if (!RS) - return nullptr; const Stmt *S = PathDiagnosticLocation::getStmt(N); - if (!S) + // When dealing with containers, we sometimes want to give a note + // even if the statement is missing. + if (!S && (!RS || RS->getAllocationFamily() != AF_InnerBuffer)) return nullptr; + const LocationContext *CurrentLC = N->getLocationContext(); + + // If we find an atomic fetch_add or fetch_sub within the destructor in which + // the pointer was released (before the release), this is likely a destructor + // of a shared pointer. + // Because we don't model atomics, and also because we don't know that the + // original reference count is positive, we should not report use-after-frees + // on objects deleted in such destructors. This can probably be improved + // through better shared pointer modeling. + if (ReleaseDestructorLC) { + if (const auto *AE = dyn_cast<AtomicExpr>(S)) { + AtomicExpr::AtomicOp Op = AE->getOp(); + if (Op == AtomicExpr::AO__c11_atomic_fetch_add || + Op == AtomicExpr::AO__c11_atomic_fetch_sub) { + if (ReleaseDestructorLC == CurrentLC || + ReleaseDestructorLC->isParentOf(CurrentLC)) { + BR.markInvalid(getTag(), S); + } + } + } + } + // FIXME: We will eventually need to handle non-statement-based events // (__attribute__((cleanup))). // Find out if this is an interesting point and what is the kind. - const char *Msg = nullptr; + StringRef Msg; StackHintGeneratorForSymbol *StackHint = nullptr; + SmallString<256> Buf; + llvm::raw_svector_ostream OS(Buf); + if (Mode == Normal) { if (isAllocated(RS, RSPrev, S)) { Msg = "Memory is allocated"; StackHint = new StackHintGeneratorForSymbol(Sym, "Returned allocated memory"); } else if (isReleased(RS, RSPrev, S)) { - Msg = "Memory is released"; + const auto Family = RS->getAllocationFamily(); + switch (Family) { + case AF_Alloca: + case AF_Malloc: + case AF_CXXNew: + case AF_CXXNewArray: + case AF_IfNameIndex: + Msg = "Memory is released"; + break; + case AF_InnerBuffer: { + OS << "Inner pointer invalidated by call to "; + if (N->getLocation().getKind() == ProgramPoint::PostImplicitCallKind) { + OS << "destructor"; + } else { + OS << "'"; + const Stmt *S = RS->getStmt(); + if (const auto *MemCallE = dyn_cast<CXXMemberCallExpr>(S)) { + OS << MemCallE->getMethodDecl()->getNameAsString(); + } else if (const auto *OpCallE = dyn_cast<CXXOperatorCallExpr>(S)) { + OS << OpCallE->getDirectCallee()->getNameAsString(); + } + OS << "'"; + } + Msg = OS.str(); + break; + } + case AF_None: + llvm_unreachable("Unhandled allocation family!"); + } StackHint = new StackHintGeneratorForSymbol(Sym, "Returning; memory was released"); + + // See if we're releasing memory while inlining a destructor + // (or one of its callees). This turns on various common + // false positive suppressions. + bool FoundAnyDestructor = false; + for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) { + if (const auto *DD = dyn_cast<CXXDestructorDecl>(LC->getDecl())) { + if (isReferenceCountingPointerDestructor(DD)) { + // This immediately looks like a reference-counting destructor. + // We're bad at guessing the original reference count of the object, + // so suppress the report for now. + BR.markInvalid(getTag(), DD); + } else if (!FoundAnyDestructor) { + assert(!ReleaseDestructorLC && + "There can be only one release point!"); + // Suspect that it's a reference counting pointer destructor. + // On one of the next nodes might find out that it has atomic + // reference counting operations within it (see the code above), + // and if so, we'd conclude that it likely is a reference counting + // pointer destructor. + ReleaseDestructorLC = LC->getStackFrame(); + // It is unlikely that releasing memory is delegated to a destructor + // inside a destructor of a shared pointer, because it's fairly hard + // to pass the information that the pointer indeed needs to be + // released into it. So we're only interested in the innermost + // destructor. + FoundAnyDestructor = true; + } + } + } } else if (isRelinquished(RS, RSPrev, S)) { Msg = "Memory ownership is transferred"; StackHint = new StackHintGeneratorForSymbol(Sym, ""); @@ -2856,13 +3004,24 @@ std::shared_ptr<PathDiagnosticPiece> MallocChecker::MallocBugVisitor::VisitNode( } } - if (!Msg) + if (Msg.empty()) return nullptr; assert(StackHint); // Generate the extra diagnostic. - PathDiagnosticLocation Pos(S, BRC.getSourceManager(), - N->getLocationContext()); + PathDiagnosticLocation Pos; + if (!S) { + assert(RS->getAllocationFamily() == AF_InnerBuffer); + auto PostImplCall = N->getLocation().getAs<PostImplicitCall>(); + if (!PostImplCall) + return nullptr; + Pos = PathDiagnosticLocation(PostImplCall->getLocation(), + BRC.getSourceManager()); + } else { + Pos = PathDiagnosticLocation(S, BRC.getSourceManager(), + N->getLocationContext()); + } + return std::make_shared<PathDiagnosticEventPiece>(Pos, Msg, true, StackHint); } @@ -2890,6 +3049,20 @@ void MallocChecker::printState(raw_ostream &Out, ProgramStateRef State, } } +namespace clang { +namespace ento { +namespace allocation_state { + +ProgramStateRef +markReleased(ProgramStateRef State, SymbolRef Sym, const Expr *Origin) { + AllocationFamily Family = AF_InnerBuffer; + return State->set<RegionState>(Sym, RefState::getReleased(Family, Origin)); +} + +} // end namespace allocation_state +} // end namespace ento +} // end namespace clang + void ento::registerNewDeleteLeaksChecker(CheckerManager &mgr) { registerCStringCheckerBasic(mgr); MallocChecker *checker = mgr.registerChecker<MallocChecker>(); @@ -2900,8 +3073,13 @@ void ento::registerNewDeleteLeaksChecker(CheckerManager &mgr) { mgr.getCurrentCheckName(); // We currently treat NewDeleteLeaks checker as a subchecker of NewDelete // checker. - if (!checker->ChecksEnabled[MallocChecker::CK_NewDeleteChecker]) + if (!checker->ChecksEnabled[MallocChecker::CK_NewDeleteChecker]) { checker->ChecksEnabled[MallocChecker::CK_NewDeleteChecker] = true; + // FIXME: This does not set the correct name, but without this workaround + // no name will be set at all. + checker->CheckNames[MallocChecker::CK_NewDeleteChecker] = + mgr.getCurrentCheckName(); + } } #define REGISTER_CHECKER(name) \ diff --git a/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp b/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp index 497978f07815..19c1d077afa1 100644 --- a/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MisusedMovedObjectChecker.cpp @@ -46,7 +46,7 @@ class MisusedMovedObjectChecker : public Checker<check::PreCall, check::PostCall, check::EndFunction, check::DeadSymbols, check::RegionChanges> { public: - void checkEndFunction(CheckerContext &C) const; + void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const; void checkPreCall(const CallEvent &MC, CheckerContext &C) const; void checkPostCall(const CallEvent &MC, CheckerContext &C) const; void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const; @@ -61,7 +61,7 @@ public: private: enum MisuseKind {MK_FunCall, MK_Copy, MK_Move}; - class MovedBugVisitor : public BugReporterVisitorImpl<MovedBugVisitor> { + class MovedBugVisitor : public BugReporterVisitor { public: MovedBugVisitor(const MemRegion *R) : Region(R), Found(false) {} @@ -101,8 +101,6 @@ static ProgramStateRef removeFromState(ProgramStateRef State, const MemRegion *Region) { if (!Region) return State; - // Note: The isSubRegionOf function is not reflexive. - State = State->remove<TrackedRegionMap>(Region); for (auto &E : State->get<TrackedRegionMap>()) { if (E.first->isSubRegionOf(Region)) State = State->remove<TrackedRegionMap>(E.first); @@ -224,7 +222,8 @@ ExplodedNode *MisusedMovedObjectChecker::reportBug(const MemRegion *Region, // Removing the function parameters' MemRegion from the state. This is needed // for PODs where the trivial destructor does not even created nor executed. -void MisusedMovedObjectChecker::checkEndFunction(CheckerContext &C) const { +void MisusedMovedObjectChecker::checkEndFunction(const ReturnStmt *RS, + CheckerContext &C) const { auto State = C.getState(); TrackedRegionMapTy Objects = State->get<TrackedRegionMap>(); if (Objects.isEmpty()) diff --git a/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp b/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp new file mode 100644 index 000000000000..5060b0e0a6e0 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp @@ -0,0 +1,88 @@ +// MmapWriteExecChecker.cpp - Check for the prot argument -----------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This checker tests the 3rd argument of mmap's calls to check if +// it is writable and executable in the same time. It's somehow +// an optional checker since for example in JIT libraries it is pretty common. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" + +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" + +using namespace clang; +using namespace ento; +using llvm::APSInt; + +namespace { +class MmapWriteExecChecker : public Checker<check::PreCall> { + CallDescription MmapFn; + CallDescription MprotectFn; + static int ProtWrite; + static int ProtExec; + static int ProtRead; + mutable std::unique_ptr<BugType> BT; +public: + MmapWriteExecChecker() : MmapFn("mmap", 6), MprotectFn("mprotect", 3) {} + void checkPreCall(const CallEvent &Call, CheckerContext &C) const; + int ProtExecOv; + int ProtReadOv; +}; +} + +int MmapWriteExecChecker::ProtWrite = 0x02; +int MmapWriteExecChecker::ProtExec = 0x04; +int MmapWriteExecChecker::ProtRead = 0x01; + +void MmapWriteExecChecker::checkPreCall(const CallEvent &Call, + CheckerContext &C) const { + if (Call.isCalled(MmapFn) || Call.isCalled(MprotectFn)) { + SVal ProtVal = Call.getArgSVal(2); + Optional<nonloc::ConcreteInt> ProtLoc = ProtVal.getAs<nonloc::ConcreteInt>(); + int64_t Prot = ProtLoc->getValue().getSExtValue(); + if (ProtExecOv != ProtExec) + ProtExec = ProtExecOv; + if (ProtReadOv != ProtRead) + ProtRead = ProtReadOv; + + // Wrong settings + if (ProtRead == ProtExec) + return; + + if ((Prot & (ProtWrite | ProtExec)) == (ProtWrite | ProtExec)) { + if (!BT) + BT.reset(new BugType(this, "W^X check fails, Write Exec prot flags set", "Security")); + + ExplodedNode *N = C.generateNonFatalErrorNode(); + if (!N) + return; + + auto Report = llvm::make_unique<BugReport>( + *BT, "Both PROT_WRITE and PROT_EXEC flags are set. This can " + "lead to exploitable memory regions, which could be overwritten " + "with malicious code", N); + Report->addRange(Call.getArgSourceRange(2)); + C.emitReport(std::move(Report)); + } + } +} + +void ento::registerMmapWriteExecChecker(CheckerManager &mgr) { + MmapWriteExecChecker *Mwec = + mgr.registerChecker<MmapWriteExecChecker>(); + Mwec->ProtExecOv = + mgr.getAnalyzerOptions().getOptionAsInteger("MmapProtExec", 0x04, Mwec); + Mwec->ProtReadOv = + mgr.getAnalyzerOptions().getOptionAsInteger("MmapProtRead", 0x01, Mwec); +} diff --git a/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp b/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp index 559c75d7a5b0..2bd68b625c1f 100644 --- a/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp @@ -186,8 +186,7 @@ static void setFlag(ProgramStateRef state, SVal val, CheckerContext &C) { } static QualType parameterTypeFromSVal(SVal val, CheckerContext &C) { - const StackFrameContext * - SFC = C.getLocationContext()->getCurrentStackFrame(); + const StackFrameContext * SFC = C.getStackFrame(); if (Optional<loc::MemRegionVal> X = val.getAs<loc::MemRegionVal>()) { const MemRegion* R = X->getRegion(); if (const VarRegion *VR = R->getAs<VarRegion>()) diff --git a/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp b/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp index 6d05159e51b0..01d2c0491b85 100644 --- a/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp @@ -44,13 +44,9 @@ public: }; } // end anonymous namespace -void NonNullParamChecker::checkPreCall(const CallEvent &Call, - CheckerContext &C) const { +/// \return Bitvector marking non-null attributes. +static llvm::SmallBitVector getNonNullAttrs(const CallEvent &Call) { const Decl *FD = Call.getDecl(); - if (!FD) - return; - - // Merge all non-null attributes unsigned NumArgs = Call.getNumArgs(); llvm::SmallBitVector AttrNonNull(NumArgs); for (const auto *NonNull : FD->specific_attrs<NonNullAttr>()) { @@ -58,49 +54,54 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call, AttrNonNull.set(0, NumArgs); break; } - for (unsigned Val : NonNull->args()) { - if (Val >= NumArgs) + for (const ParamIdx &Idx : NonNull->args()) { + unsigned IdxAST = Idx.getASTIndex(); + if (IdxAST >= NumArgs) continue; - AttrNonNull.set(Val); + AttrNonNull.set(IdxAST); } } + return AttrNonNull; +} - ProgramStateRef state = C.getState(); +void NonNullParamChecker::checkPreCall(const CallEvent &Call, + CheckerContext &C) const { + if (!Call.getDecl()) + return; + + llvm::SmallBitVector AttrNonNull = getNonNullAttrs(Call); + unsigned NumArgs = Call.getNumArgs(); - CallEvent::param_type_iterator TyI = Call.param_type_begin(), - TyE = Call.param_type_end(); + ProgramStateRef state = C.getState(); + ArrayRef<ParmVarDecl*> parms = Call.parameters(); for (unsigned idx = 0; idx < NumArgs; ++idx) { + // For vararg functions, a corresponding parameter decl may not exist. + bool HasParam = idx < parms.size(); // Check if the parameter is a reference. We want to report when reference // to a null pointer is passed as a parameter. - bool haveRefTypeParam = false; - if (TyI != TyE) { - haveRefTypeParam = (*TyI)->isReferenceType(); - TyI++; - } - + bool haveRefTypeParam = + HasParam ? parms[idx]->getType()->isReferenceType() : false; bool haveAttrNonNull = AttrNonNull[idx]; - if (!haveAttrNonNull) { - // Check if the parameter is also marked 'nonnull'. - ArrayRef<ParmVarDecl*> parms = Call.parameters(); - if (idx < parms.size()) - haveAttrNonNull = parms[idx]->hasAttr<NonNullAttr>(); - } - if (!haveRefTypeParam && !haveAttrNonNull) + // Check if the parameter is also marked 'nonnull'. + if (!haveAttrNonNull && HasParam) + haveAttrNonNull = parms[idx]->hasAttr<NonNullAttr>(); + + if (!haveAttrNonNull && !haveRefTypeParam) continue; // If the value is unknown or undefined, we can't perform this check. const Expr *ArgE = Call.getArgExpr(idx); SVal V = Call.getArgSVal(idx); - Optional<DefinedSVal> DV = V.getAs<DefinedSVal>(); + auto DV = V.getAs<DefinedSVal>(); if (!DV) continue; - // Process the case when the argument is not a location. assert(!haveRefTypeParam || DV->getAs<Loc>()); + // Process the case when the argument is not a location. if (haveAttrNonNull && !DV->getAs<Loc>()) { // If the argument is a union type, we want to handle a potential // transparent_union GCC extension. @@ -112,66 +113,63 @@ void NonNullParamChecker::checkPreCall(const CallEvent &Call, if (!UT || !UT->getDecl()->hasAttr<TransparentUnionAttr>()) continue; - if (Optional<nonloc::CompoundVal> CSV = - DV->getAs<nonloc::CompoundVal>()) { - nonloc::CompoundVal::iterator CSV_I = CSV->begin(); - assert(CSV_I != CSV->end()); - V = *CSV_I; - DV = V.getAs<DefinedSVal>(); - assert(++CSV_I == CSV->end()); - // FIXME: Handle (some_union){ some_other_union_val }, which turns into - // a LazyCompoundVal inside a CompoundVal. - if (!V.getAs<Loc>()) - continue; - // Retrieve the corresponding expression. - if (const CompoundLiteralExpr *CE = dyn_cast<CompoundLiteralExpr>(ArgE)) - if (const InitListExpr *IE = - dyn_cast<InitListExpr>(CE->getInitializer())) - ArgE = dyn_cast<Expr>(*(IE->begin())); - - } else { - // FIXME: Handle LazyCompoundVals? + auto CSV = DV->getAs<nonloc::CompoundVal>(); + + // FIXME: Handle LazyCompoundVals? + if (!CSV) continue; - } + + V = *(CSV->begin()); + DV = V.getAs<DefinedSVal>(); + assert(++CSV->begin() == CSV->end()); + // FIXME: Handle (some_union){ some_other_union_val }, which turns into + // a LazyCompoundVal inside a CompoundVal. + if (!V.getAs<Loc>()) + continue; + + // Retrieve the corresponding expression. + if (const auto *CE = dyn_cast<CompoundLiteralExpr>(ArgE)) + if (const auto *IE = dyn_cast<InitListExpr>(CE->getInitializer())) + ArgE = dyn_cast<Expr>(*(IE->begin())); } ConstraintManager &CM = C.getConstraintManager(); ProgramStateRef stateNotNull, stateNull; std::tie(stateNotNull, stateNull) = CM.assumeDual(state, *DV); - if (stateNull) { - if (!stateNotNull) { - // Generate an error node. Check for a null node in case - // we cache out. - if (ExplodedNode *errorNode = C.generateErrorNode(stateNull)) { - - std::unique_ptr<BugReport> R; - if (haveAttrNonNull) - R = genReportNullAttrNonNull(errorNode, ArgE); - else if (haveRefTypeParam) - R = genReportReferenceToNullPointer(errorNode, ArgE); - - // Highlight the range of the argument that was null. - R->addRange(Call.getArgSourceRange(idx)); - - // Emit the bug report. - C.emitReport(std::move(R)); - } - - // Always return. Either we cached out or we just emitted an error. - return; + // Generate an error node. Check for a null node in case + // we cache out. + if (stateNull && !stateNotNull) { + if (ExplodedNode *errorNode = C.generateErrorNode(stateNull)) { + + std::unique_ptr<BugReport> R; + if (haveAttrNonNull) + R = genReportNullAttrNonNull(errorNode, ArgE); + else if (haveRefTypeParam) + R = genReportReferenceToNullPointer(errorNode, ArgE); + + // Highlight the range of the argument that was null. + R->addRange(Call.getArgSourceRange(idx)); + + // Emit the bug report. + C.emitReport(std::move(R)); } + + // Always return. Either we cached out or we just emitted an error. + return; + } + + if (stateNull) { if (ExplodedNode *N = C.generateSink(stateNull, C.getPredecessor())) { ImplicitNullDerefEvent event = { - V, false, N, &C.getBugReporter(), - /*IsDirectDereference=*/haveRefTypeParam}; + V, false, N, &C.getBugReporter(), + /*IsDirectDereference=*/haveRefTypeParam}; dispatchEvent(event); } } // If a pointer value passed the check we should assume that it is // indeed not null from this point forward. - assert(stateNotNull); state = stateNotNull; } diff --git a/lib/StaticAnalyzer/Checkers/NonnullGlobalConstantsChecker.cpp b/lib/StaticAnalyzer/Checkers/NonnullGlobalConstantsChecker.cpp index 0b4ecb41d20f..6f3180eb839a 100644 --- a/lib/StaticAnalyzer/Checkers/NonnullGlobalConstantsChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/NonnullGlobalConstantsChecker.cpp @@ -54,7 +54,7 @@ private: } // namespace -/// Lazily initialize cache for required identifier informations. +/// Lazily initialize cache for required identifier information. void NonnullGlobalConstantsChecker::initIdentifierInfo(ASTContext &Ctx) const { if (NSStringII) return; @@ -73,9 +73,9 @@ void NonnullGlobalConstantsChecker::checkLocation(SVal location, bool isLoad, return; ProgramStateRef State = C.getState(); - SVal V = State->getSVal(location.castAs<Loc>()); if (isGlobalConstString(location)) { + SVal V = State->getSVal(location.castAs<Loc>()); Optional<DefinedOrUnknownSVal> Constr = V.getAs<DefinedOrUnknownSVal>(); if (Constr) { diff --git a/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp index fa9a317683ba..7d1ca61c97a9 100644 --- a/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp @@ -30,6 +30,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/CheckerHelpers.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" @@ -40,21 +41,6 @@ using namespace clang; using namespace ento; namespace { -// Do not reorder! The getMostNullable method relies on the order. -// Optimization: Most pointers expected to be unspecified. When a symbol has an -// unspecified or nonnull type non of the rules would indicate any problem for -// that symbol. For this reason only nullable and contradicted nullability are -// stored for a symbol. When a symbol is already contradicted, it can not be -// casted back to nullable. -enum class Nullability : char { - Contradicted, // Tracked nullability is contradicted by an explicit cast. Do - // not report any nullability related issue for this symbol. - // This nullability is propagated aggressively to avoid false - // positive results. See the comment on getMostNullable method. - Nullable, - Unspecified, - Nonnull -}; /// Returns the most nullable nullability. This is used for message expressions /// like [receiver method], where the nullability of this expression is either @@ -142,8 +128,7 @@ public: DefaultBool NeedTracking; private: - class NullabilityBugVisitor - : public BugReporterVisitorImpl<NullabilityBugVisitor> { + class NullabilityBugVisitor : public BugReporterVisitor { public: NullabilityBugVisitor(const MemRegion *M) : Region(M) {} @@ -265,7 +250,7 @@ REGISTER_MAP_WITH_PROGRAMSTATE(NullabilityMap, const MemRegion *, // initial direct violation has been discovered, and (3) warning after a direct // violation that has been implicitly or explicitly suppressed (for // example, with a cast of NULL to _Nonnull). In essence, once an invariant -// violation is detected on a path, this checker will be esentially turned off +// violation is detected on a path, this checker will be essentially turned off // for the rest of the analysis // // The analyzer takes this approach (rather than generating a sink node) to @@ -345,17 +330,6 @@ NullabilityChecker::NullabilityBugVisitor::VisitNode(const ExplodedNode *N, nullptr); } -static Nullability getNullabilityAnnotation(QualType Type) { - const auto *AttrType = Type->getAs<AttributedType>(); - if (!AttrType) - return Nullability::Unspecified; - if (AttrType->getAttrKind() == AttributedType::attr_nullable) - return Nullability::Nullable; - else if (AttrType->getAttrKind() == AttributedType::attr_nonnull) - return Nullability::Nonnull; - return Nullability::Unspecified; -} - /// Returns true when the value stored at the given location is null /// and the passed in type is nonnnull. static bool checkValueAtLValForInvariantViolation(ProgramStateRef State, @@ -560,8 +534,7 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S, if (State->get<InvariantViolated>()) return; - auto RetSVal = - State->getSVal(S, C.getLocationContext()).getAs<DefinedOrUnknownSVal>(); + auto RetSVal = C.getSVal(S).getAs<DefinedOrUnknownSVal>(); if (!RetSVal) return; @@ -873,7 +846,7 @@ void NullabilityChecker::checkPostObjCMessage(const ObjCMethodCall &M, // are either item retrieval related or not interesting nullability wise. // Using this fact, to keep the code easier to read just ignore the return // value of every instance method of dictionaries. - if (M.isInstanceMessage() && Name.find("Dictionary") != StringRef::npos) { + if (M.isInstanceMessage() && Name.contains("Dictionary")) { State = State->set<NullabilityMap>(ReturnRegion, Nullability::Contradicted); C.addTransition(State); @@ -881,7 +854,7 @@ void NullabilityChecker::checkPostObjCMessage(const ObjCMethodCall &M, } // For similar reasons ignore some methods of Cocoa arrays. StringRef FirstSelectorSlot = M.getSelector().getNameForSlot(0); - if (Name.find("Array") != StringRef::npos && + if (Name.contains("Array") && (FirstSelectorSlot == "firstObject" || FirstSelectorSlot == "lastObject")) { State = @@ -894,7 +867,7 @@ void NullabilityChecker::checkPostObjCMessage(const ObjCMethodCall &M, // encodings are used. Using lossless encodings is so frequent that ignoring // this class of methods reduced the emitted diagnostics by about 30% on // some projects (and all of that was false positives). - if (Name.find("String") != StringRef::npos) { + if (Name.contains("String")) { for (auto Param : M.parameters()) { if (Param->getName() == "encoding") { State = State->set<NullabilityMap>(ReturnRegion, @@ -977,8 +950,7 @@ void NullabilityChecker::checkPostStmt(const ExplicitCastExpr *CE, if (DestNullability == Nullability::Unspecified) return; - auto RegionSVal = - State->getSVal(CE, C.getLocationContext()).getAs<DefinedOrUnknownSVal>(); + auto RegionSVal = C.getSVal(CE).getAs<DefinedOrUnknownSVal>(); const MemRegion *Region = getTrackRegion(*RegionSVal); if (!Region) return; diff --git a/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp b/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp index 40e379cb2efc..d1749cfdbe27 100644 --- a/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp @@ -270,8 +270,10 @@ void NumberObjectConversionChecker::checkASTCodeBody(const Decl *D, hasRHS(SuspiciousNumberObjectExprM))); auto ConversionThroughBranchingM = - ifStmt(hasCondition(SuspiciousNumberObjectExprM)) - .bind("pedantic"); + ifStmt(allOf( + hasCondition(SuspiciousNumberObjectExprM), + unless(hasConditionVariableStatement(declStmt()) + ))).bind("pedantic"); auto ConversionThroughCallM = callExpr(hasAnyArgument(allOf(hasType(SuspiciousScalarTypeM), diff --git a/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp index cbaa5c23592d..b7339fe79f69 100644 --- a/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp @@ -39,7 +39,7 @@ void ObjCAtSyncChecker::checkPreStmt(const ObjCAtSynchronizedStmt *S, const Expr *Ex = S->getSynchExpr(); ProgramStateRef state = C.getState(); - SVal V = state->getSVal(Ex, C.getLocationContext()); + SVal V = C.getSVal(Ex); // Uninitialized value used for the mutex? if (V.getAs<UndefinedVal>()) { diff --git a/lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp new file mode 100644 index 000000000000..81bcda51b8f8 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/ObjCAutoreleaseWriteChecker.cpp @@ -0,0 +1,209 @@ +//===- ObjCAutoreleaseWriteChecker.cpp ----------------------------*- C++ -*-==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This file defines ObjCAutoreleaseWriteChecker which warns against writes +// into autoreleased out parameters which cause crashes. +// An example of a problematic write is a write to {@code error} in the example +// below: +// +// - (BOOL) mymethod:(NSError *__autoreleasing *)error list:(NSArray*) list { +// [list enumerateObjectsUsingBlock:^(id obj, NSUInteger idx, BOOL *stop) { +// NSString *myString = obj; +// if ([myString isEqualToString:@"error"] && error) +// *error = [NSError errorWithDomain:@"MyDomain" code:-1]; +// }]; +// return false; +// } +// +// Such code will crash on read from `*error` due to the autorelease pool +// in `enumerateObjectsUsingBlock` implementation freeing the error object +// on exit from the function. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h" +#include "llvm/ADT/Twine.h" + +using namespace clang; +using namespace ento; +using namespace ast_matchers; + +namespace { + +const char *ProblematicWriteBind = "problematicwrite"; +const char *CapturedBind = "capturedbind"; +const char *ParamBind = "parambind"; +const char *IsMethodBind = "ismethodbind"; + +class ObjCAutoreleaseWriteChecker : public Checker<check::ASTCodeBody> { +public: + void checkASTCodeBody(const Decl *D, + AnalysisManager &AM, + BugReporter &BR) const; +private: + std::vector<std::string> SelectorsWithAutoreleasingPool = { + // Common to NSArray, NSSet, NSOrderedSet + "enumerateObjectsUsingBlock:", + "enumerateObjectsWithOptions:usingBlock:", + + // Common to NSArray and NSOrderedSet + "enumerateObjectsAtIndexes:options:usingBlock:", + "indexOfObjectAtIndexes:options:passingTest:", + "indexesOfObjectsAtIndexes:options:passingTest:", + "indexOfObjectPassingTest:", + "indexOfObjectWithOptions:passingTest:", + "indexesOfObjectsPassingTest:", + "indexesOfObjectsWithOptions:passingTest:", + + // NSDictionary + "enumerateKeysAndObjectsUsingBlock:", + "enumerateKeysAndObjectsWithOptions:usingBlock:", + "keysOfEntriesPassingTest:", + "keysOfEntriesWithOptions:passingTest:", + + // NSSet + "objectsPassingTest:", + "objectsWithOptions:passingTest:", + "enumerateIndexPathsWithOptions:usingBlock:", + + // NSIndexSet + "enumerateIndexesWithOptions:usingBlock:", + "enumerateIndexesUsingBlock:", + "enumerateIndexesInRange:options:usingBlock:", + "enumerateRangesUsingBlock:", + "enumerateRangesWithOptions:usingBlock:", + "enumerateRangesInRange:options:usingBlock:", + "indexPassingTest:", + "indexesPassingTest:", + "indexWithOptions:passingTest:", + "indexesWithOptions:passingTest:", + "indexInRange:options:passingTest:", + "indexesInRange:options:passingTest:" + }; + + std::vector<std::string> FunctionsWithAutoreleasingPool = { + "dispatch_async", "dispatch_group_async", "dispatch_barrier_async"}; +}; +} + +static inline std::vector<llvm::StringRef> toRefs(std::vector<std::string> V) { + return std::vector<llvm::StringRef>(V.begin(), V.end()); +} + +static auto callsNames(std::vector<std::string> FunctionNames) + -> decltype(callee(functionDecl())) { + return callee(functionDecl(hasAnyName(toRefs(FunctionNames)))); +} + +static void emitDiagnostics(BoundNodes &Match, const Decl *D, BugReporter &BR, + AnalysisManager &AM, + const ObjCAutoreleaseWriteChecker *Checker) { + AnalysisDeclContext *ADC = AM.getAnalysisDeclContext(D); + + const auto *PVD = Match.getNodeAs<ParmVarDecl>(ParamBind); + QualType Ty = PVD->getType(); + if (Ty->getPointeeType().getObjCLifetime() != Qualifiers::OCL_Autoreleasing) + return; + const char *ActionMsg = "Write to"; + const auto *MarkedStmt = Match.getNodeAs<Expr>(ProblematicWriteBind); + bool IsCapture = false; + + // Prefer to warn on write, but if not available, warn on capture. + if (!MarkedStmt) { + MarkedStmt = Match.getNodeAs<Expr>(CapturedBind); + assert(MarkedStmt); + ActionMsg = "Capture of"; + IsCapture = true; + } + + SourceRange Range = MarkedStmt->getSourceRange(); + PathDiagnosticLocation Location = PathDiagnosticLocation::createBegin( + MarkedStmt, BR.getSourceManager(), ADC); + bool IsMethod = Match.getNodeAs<ObjCMethodDecl>(IsMethodBind) != nullptr; + const char *Name = IsMethod ? "method" : "function"; + + BR.EmitBasicReport( + ADC->getDecl(), Checker, + /*Name=*/(llvm::Twine(ActionMsg) + + " autoreleasing out parameter inside autorelease pool").str(), + /*Category=*/"Memory", + (llvm::Twine(ActionMsg) + " autoreleasing out parameter " + + (IsCapture ? "'" + PVD->getName() + "'" + " " : "") + "inside " + + "autorelease pool that may exit before " + Name + " returns; consider " + "writing first to a strong local variable declared outside of the block") + .str(), + Location, + Range); +} + +void ObjCAutoreleaseWriteChecker::checkASTCodeBody(const Decl *D, + AnalysisManager &AM, + BugReporter &BR) const { + + auto DoublePointerParamM = + parmVarDecl(hasType(hasCanonicalType(pointerType( + pointee(hasCanonicalType(objcObjectPointerType())))))) + .bind(ParamBind); + + auto ReferencedParamM = + declRefExpr(to(parmVarDecl(DoublePointerParamM))).bind(CapturedBind); + + // Write into a binded object, e.g. *ParamBind = X. + auto WritesIntoM = binaryOperator( + hasLHS(unaryOperator( + hasOperatorName("*"), + hasUnaryOperand( + ignoringParenImpCasts(ReferencedParamM)) + )), + hasOperatorName("=") + ).bind(ProblematicWriteBind); + + auto ArgumentCaptureM = hasAnyArgument( + ignoringParenImpCasts(ReferencedParamM)); + auto CapturedInParamM = stmt(anyOf( + callExpr(ArgumentCaptureM), + objcMessageExpr(ArgumentCaptureM))); + + // WritesIntoM happens inside a block passed as an argument. + auto WritesOrCapturesInBlockM = hasAnyArgument(allOf( + hasType(hasCanonicalType(blockPointerType())), + forEachDescendant( + stmt(anyOf(WritesIntoM, CapturedInParamM)) + ))); + + auto BlockPassedToMarkedFuncM = stmt(anyOf( + callExpr(allOf( + callsNames(FunctionsWithAutoreleasingPool), WritesOrCapturesInBlockM)), + objcMessageExpr(allOf( + hasAnySelector(toRefs(SelectorsWithAutoreleasingPool)), + WritesOrCapturesInBlockM)) + )); + + auto HasParamAndWritesInMarkedFuncM = allOf( + hasAnyParameter(DoublePointerParamM), + forEachDescendant(BlockPassedToMarkedFuncM)); + + auto MatcherM = decl(anyOf( + objcMethodDecl(HasParamAndWritesInMarkedFuncM).bind(IsMethodBind), + functionDecl(HasParamAndWritesInMarkedFuncM), + blockDecl(HasParamAndWritesInMarkedFuncM))); + + auto Matches = match(MatcherM, *D, AM.getASTContext()); + for (BoundNodes Match : Matches) + emitDiagnostics(Match, D, BR, AM, this); +} + +void ento::registerAutoreleaseWriteChecker(CheckerManager &Mgr) { + Mgr.registerChecker<ObjCAutoreleaseWriteChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp index 58ebf72660b6..fb05ca630b45 100644 --- a/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp @@ -39,7 +39,7 @@ class ObjCContainersChecker : public Checker< check::PreStmt<CallExpr>, } inline SymbolRef getArraySym(const Expr *E, CheckerContext &C) const { - SVal ArrayRef = C.getState()->getSVal(E, C.getLocationContext()); + SVal ArrayRef = C.getSVal(E); SymbolRef ArraySym = ArrayRef.getAsSymbol(); return ArraySym; } @@ -66,13 +66,13 @@ REGISTER_MAP_WITH_PROGRAMSTATE(ArraySizeMap, SymbolRef, DefinedSVal) void ObjCContainersChecker::addSizeInfo(const Expr *Array, const Expr *Size, CheckerContext &C) const { ProgramStateRef State = C.getState(); - SVal SizeV = State->getSVal(Size, C.getLocationContext()); + SVal SizeV = C.getSVal(Size); // Undefined is reported by another checker. if (SizeV.isUnknownOrUndef()) return; // Get the ArrayRef symbol. - SVal ArrayRef = State->getSVal(Array, C.getLocationContext()); + SVal ArrayRef = C.getSVal(Array); SymbolRef ArraySym = ArrayRef.getAsSymbol(); if (!ArraySym) return; @@ -128,7 +128,7 @@ void ObjCContainersChecker::checkPreStmt(const CallExpr *CE, // Get the index. const Expr *IdxExpr = CE->getArg(1); - SVal IdxVal = State->getSVal(IdxExpr, C.getLocationContext()); + SVal IdxVal = C.getSVal(IdxExpr); if (IdxVal.isUnknownOrUndef()) return; DefinedSVal Idx = IdxVal.castAs<DefinedSVal>(); diff --git a/lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp index 32a1adb587bf..d01c6ae6e093 100644 --- a/lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ObjCMissingSuperCallChecker.cpp @@ -81,7 +81,7 @@ private: } -/// \brief Determine whether the given class has a superclass that we want +/// Determine whether the given class has a superclass that we want /// to check. The name of the found superclass is stored in SuperclassName. /// /// \param D The declaration to check for superclasses. diff --git a/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp index ffa3a2700616..629520437369 100644 --- a/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp @@ -86,11 +86,11 @@ public: namespace { enum SelfFlagEnum { - /// \brief No flag set. + /// No flag set. SelfFlag_None = 0x0, - /// \brief Value came from 'self'. + /// Value came from 'self'. SelfFlag_Self = 0x1, - /// \brief Value came from the result of an initializer (e.g. [super init]). + /// Value came from the result of an initializer (e.g. [super init]). SelfFlag_InitRes = 0x2 }; } @@ -98,7 +98,7 @@ enum SelfFlagEnum { REGISTER_MAP_WITH_PROGRAMSTATE(SelfFlag, SymbolRef, unsigned) REGISTER_TRAIT_WITH_PROGRAMSTATE(CalledInit, bool) -/// \brief A call receiving a reference to 'self' invalidates the object that +/// A call receiving a reference to 'self' invalidates the object that /// 'self' contains. This keeps the "self flags" assigned to the 'self' /// object before the call so we can assign them to the new object that 'self' /// points to after the call. @@ -128,11 +128,11 @@ static bool hasSelfFlag(SVal val, SelfFlagEnum flag, CheckerContext &C) { return getSelfFlags(val, C) & flag; } -/// \brief Returns true of the value of the expression is the object that 'self' +/// Returns true of the value of the expression is the object that 'self' /// points to and is an object that did not come from the result of calling /// an initializer. static bool isInvalidSelf(const Expr *E, CheckerContext &C) { - SVal exprVal = C.getState()->getSVal(E, C.getLocationContext()); + SVal exprVal = C.getSVal(E); if (!hasSelfFlag(exprVal, SelfFlag_Self, C)) return false; // value did not come from 'self'. if (hasSelfFlag(exprVal, SelfFlag_InitRes, C)) @@ -183,7 +183,7 @@ void ObjCSelfInitChecker::checkPostObjCMessage(const ObjCMethodCall &Msg, // value out when we return from this method. state = state->set<CalledInit>(true); - SVal V = state->getSVal(Msg.getOriginExpr(), C.getLocationContext()); + SVal V = C.getSVal(Msg.getOriginExpr()); addSelfFlag(state, V, SelfFlag_InitRes, C); return; } @@ -407,7 +407,7 @@ static bool shouldRunOnFunctionOrMethod(const NamedDecl *ND) { return ID != nullptr; } -/// \brief Returns true if the location is 'self'. +/// Returns true if the location is 'self'. static bool isSelfVar(SVal location, CheckerContext &C) { AnalysisDeclContext *analCtx = C.getCurrentAnalysisDeclContext(); if (!analCtx->getSelfDecl()) diff --git a/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp index 69b19a785938..fcba3b33f3e0 100644 --- a/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp @@ -62,9 +62,7 @@ private: REGISTER_SET_WITH_PROGRAMSTATE(CalledSuperDealloc, SymbolRef) namespace { -class SuperDeallocBRVisitor final - : public BugReporterVisitorImpl<SuperDeallocBRVisitor> { - +class SuperDeallocBRVisitor final : public BugReporterVisitor { SymbolRef ReceiverSymbol; bool Satisfied; diff --git a/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp b/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp index 6c0c53dd64cb..f69f3492edb1 100644 --- a/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp @@ -67,7 +67,7 @@ public: visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD)); } - /// \brief Look for records of overly padded types. If padding * + /// Look for records of overly padded types. If padding * /// PadMultiplier exceeds AllowedPad, then generate a report. /// PadMultiplier is used to share code with the array padding /// checker. @@ -97,7 +97,7 @@ public: reportRecord(RD, BaselinePad, OptimalPad, OptimalFieldsOrder); } - /// \brief Look for arrays of overly padded types. If the padding of the + /// Look for arrays of overly padded types. If the padding of the /// array type exceeds AllowedPad, then generate a report. void visitVariable(const VarDecl *VD) const { const ArrayType *ArrTy = VD->getType()->getAsArrayTypeUnsafe(); @@ -237,7 +237,7 @@ public: }; std::transform(RD->field_begin(), RD->field_end(), std::back_inserter(Fields), GatherSizesAndAlignments); - std::sort(Fields.begin(), Fields.end()); + llvm::sort(Fields.begin(), Fields.end()); // This lets us skip over vptrs and non-virtual bases, // so that we can just worry about the fields in our object. // Note that this does cause us to miss some cases where we diff --git a/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp b/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp index 8caf6df4d970..63f82b275ba2 100644 --- a/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp @@ -154,8 +154,7 @@ void PointerArithChecker::reportPointerArithMisuse(const Expr *E, return; ProgramStateRef State = C.getState(); - const MemRegion *Region = - State->getSVal(E, C.getLocationContext()).getAsRegion(); + const MemRegion *Region = C.getSVal(E).getAsRegion(); if (!Region) return; if (PointedNeeded) @@ -227,7 +226,7 @@ void PointerArithChecker::checkPostStmt(const CallExpr *CE, if (AllocFunctions.count(FunI) == 0) return; - SVal SV = State->getSVal(CE, C.getLocationContext()); + SVal SV = C.getSVal(CE); const MemRegion *Region = SV.getAsRegion(); if (!Region) return; @@ -248,7 +247,7 @@ void PointerArithChecker::checkPostStmt(const CXXNewExpr *NE, AllocKind Kind = getKindOfNewOp(NE, FD); ProgramStateRef State = C.getState(); - SVal AllocedVal = State->getSVal(NE, C.getLocationContext()); + SVal AllocedVal = C.getSVal(NE); const MemRegion *Region = AllocedVal.getAsRegion(); if (!Region) return; @@ -263,7 +262,7 @@ void PointerArithChecker::checkPostStmt(const CastExpr *CE, const Expr *CastedExpr = CE->getSubExpr(); ProgramStateRef State = C.getState(); - SVal CastedVal = State->getSVal(CastedExpr, C.getLocationContext()); + SVal CastedVal = C.getSVal(CastedExpr); const MemRegion *Region = CastedVal.getAsRegion(); if (!Region) @@ -281,7 +280,7 @@ void PointerArithChecker::checkPreStmt(const CastExpr *CE, const Expr *CastedExpr = CE->getSubExpr(); ProgramStateRef State = C.getState(); - SVal CastedVal = State->getSVal(CastedExpr, C.getLocationContext()); + SVal CastedVal = C.getSVal(CastedExpr); const MemRegion *Region = CastedVal.getAsRegion(); if (!Region) @@ -304,12 +303,15 @@ void PointerArithChecker::checkPreStmt(const UnaryOperator *UOp, void PointerArithChecker::checkPreStmt(const ArraySubscriptExpr *SubsExpr, CheckerContext &C) const { - ProgramStateRef State = C.getState(); - SVal Idx = State->getSVal(SubsExpr->getIdx(), C.getLocationContext()); + SVal Idx = C.getSVal(SubsExpr->getIdx()); // Indexing with 0 is OK. if (Idx.isZeroConstant()) return; + + // Indexing vector-type expressions is also OK. + if (SubsExpr->getBase()->getType()->isVectorType()) + return; reportPointerArithMisuse(SubsExpr->getBase(), C); } @@ -324,14 +326,14 @@ void PointerArithChecker::checkPreStmt(const BinaryOperator *BOp, ProgramStateRef State = C.getState(); if (Rhs->getType()->isIntegerType() && Lhs->getType()->isPointerType()) { - SVal RHSVal = State->getSVal(Rhs, C.getLocationContext()); + SVal RHSVal = C.getSVal(Rhs); if (State->isNull(RHSVal).isConstrainedTrue()) return; reportPointerArithMisuse(Lhs, C, !BOp->isAdditiveOp()); } // The int += ptr; case is not valid C++. if (Lhs->getType()->isIntegerType() && Rhs->getType()->isPointerType()) { - SVal LHSVal = State->getSVal(Lhs, C.getLocationContext()); + SVal LHSVal = C.getSVal(Lhs); if (State->isNull(LHSVal).isConstrainedTrue()) return; reportPointerArithMisuse(Rhs, C); diff --git a/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp b/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp index 2d33ebc2610d..9aa5348e4c34 100644 --- a/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp @@ -39,10 +39,8 @@ void PointerSubChecker::checkPreStmt(const BinaryOperator *B, if (B->getOpcode() != BO_Sub) return; - ProgramStateRef state = C.getState(); - const LocationContext *LCtx = C.getLocationContext(); - SVal LV = state->getSVal(B->getLHS(), LCtx); - SVal RV = state->getSVal(B->getRHS(), LCtx); + SVal LV = C.getSVal(B->getLHS()); + SVal RV = C.getSVal(B->getRHS()); const MemRegion *LR = LV.getAsRegion(); const MemRegion *RR = RV.getAsRegion(); diff --git a/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp b/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp index dab29be1c8fb..10ab952e069b 100644 --- a/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp @@ -109,8 +109,6 @@ REGISTER_MAP_WITH_PROGRAMSTATE(DestroyRetVal, const MemRegion *, SymbolRef) void PthreadLockChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const { - ProgramStateRef state = C.getState(); - const LocationContext *LCtx = C.getLocationContext(); StringRef FName = C.getCalleeName(CE); if (FName.empty()) return; @@ -121,34 +119,31 @@ void PthreadLockChecker::checkPostStmt(const CallExpr *CE, if (FName == "pthread_mutex_lock" || FName == "pthread_rwlock_rdlock" || FName == "pthread_rwlock_wrlock") - AcquireLock(C, CE, state->getSVal(CE->getArg(0), LCtx), - false, PthreadSemantics); + AcquireLock(C, CE, C.getSVal(CE->getArg(0)), false, PthreadSemantics); else if (FName == "lck_mtx_lock" || FName == "lck_rw_lock_exclusive" || FName == "lck_rw_lock_shared") - AcquireLock(C, CE, state->getSVal(CE->getArg(0), LCtx), - false, XNUSemantics); + AcquireLock(C, CE, C.getSVal(CE->getArg(0)), false, XNUSemantics); else if (FName == "pthread_mutex_trylock" || FName == "pthread_rwlock_tryrdlock" || FName == "pthread_rwlock_trywrlock") - AcquireLock(C, CE, state->getSVal(CE->getArg(0), LCtx), + AcquireLock(C, CE, C.getSVal(CE->getArg(0)), true, PthreadSemantics); else if (FName == "lck_mtx_try_lock" || FName == "lck_rw_try_lock_exclusive" || FName == "lck_rw_try_lock_shared") - AcquireLock(C, CE, state->getSVal(CE->getArg(0), LCtx), - true, XNUSemantics); + AcquireLock(C, CE, C.getSVal(CE->getArg(0)), true, XNUSemantics); else if (FName == "pthread_mutex_unlock" || FName == "pthread_rwlock_unlock" || FName == "lck_mtx_unlock" || FName == "lck_rw_done") - ReleaseLock(C, CE, state->getSVal(CE->getArg(0), LCtx)); + ReleaseLock(C, CE, C.getSVal(CE->getArg(0))); else if (FName == "pthread_mutex_destroy") - DestroyLock(C, CE, state->getSVal(CE->getArg(0), LCtx), PthreadSemantics); + DestroyLock(C, CE, C.getSVal(CE->getArg(0)), PthreadSemantics); else if (FName == "lck_mtx_destroy") - DestroyLock(C, CE, state->getSVal(CE->getArg(0), LCtx), XNUSemantics); + DestroyLock(C, CE, C.getSVal(CE->getArg(0)), XNUSemantics); else if (FName == "pthread_mutex_init") - InitLock(C, CE, state->getSVal(CE->getArg(0), LCtx)); + InitLock(C, CE, C.getSVal(CE->getArg(0))); } // When a lock is destroyed, in some semantics(like PthreadSemantics) we are not @@ -232,7 +227,7 @@ void PthreadLockChecker::AcquireLock(CheckerContext &C, const CallExpr *CE, if (sym) state = resolvePossiblyDestroyedMutex(state, lockR, sym); - SVal X = state->getSVal(CE, C.getLocationContext()); + SVal X = C.getSVal(CE); if (X.isUnknownOrUndef()) return; diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp index e47494a3e90b..2c1e139330d6 100644 --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp @@ -555,7 +555,7 @@ public: } const RetainSummary *find(IdentifierInfo* II, Selector S) { - // FIXME: Class method lookup. Right now we dont' have a good way + // FIXME: Class method lookup. Right now we don't have a good way // of going between IdentifierInfo* and the class hierarchy. MapTy::iterator I = M.find(ObjCSummaryKey(II, S)); @@ -883,21 +883,22 @@ RetainSummaryManager::getPersistentSummary(const RetainSummary &OldSumm) { //===----------------------------------------------------------------------===// static bool isRetain(const FunctionDecl *FD, StringRef FName) { - return FName.endswith("Retain"); + return FName.startswith_lower("retain") || FName.endswith_lower("retain"); } static bool isRelease(const FunctionDecl *FD, StringRef FName) { - return FName.endswith("Release"); + return FName.startswith_lower("release") || FName.endswith_lower("release"); } static bool isAutorelease(const FunctionDecl *FD, StringRef FName) { - return FName.endswith("Autorelease"); + return FName.startswith_lower("autorelease") || + FName.endswith_lower("autorelease"); } static bool isMakeCollectable(const FunctionDecl *FD, StringRef FName) { // FIXME: Remove FunctionDecl parameter. // FIXME: Is it really okay if MakeCollectable isn't a suffix? - return FName.find("MakeCollectable") != StringRef::npos; + return FName.find_lower("MakeCollectable") != StringRef::npos; } static ArgEffect getStopTrackingHardEquivalent(ArgEffect E) { @@ -1787,8 +1788,7 @@ namespace { //===---------===// // Bug Reports. // //===---------===// - - class CFRefReportVisitor : public BugReporterVisitorImpl<CFRefReportVisitor> { + class CFRefReportVisitor : public BugReporterVisitor { protected: SymbolRef Sym; const SummaryLogTy &SummaryLog; @@ -1809,7 +1809,7 @@ namespace { BugReporterContext &BRC, BugReport &BR) override; - std::unique_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC, + std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC, const ExplodedNode *N, BugReport &BR) override; }; @@ -1820,18 +1820,9 @@ namespace { const SummaryLogTy &log) : CFRefReportVisitor(sym, GCEnabled, log) {} - std::unique_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC, + std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC, const ExplodedNode *N, BugReport &BR) override; - - std::unique_ptr<BugReporterVisitor> clone() const override { - // The curiously-recurring template pattern only works for one level of - // subclassing. Rather than make a new template base for - // CFRefReportVisitor, we simply override clone() to do the right thing. - // This could be trouble someday if BugReporterVisitorImpl is ever - // used for something else besides a convenient implementation of clone(). - return llvm::make_unique<CFRefLeakReportVisitor>(*this); - } }; class CFRefReport : public BugReport { @@ -1929,6 +1920,14 @@ static bool isNumericLiteralExpression(const Expr *E) { isa<CXXBoolLiteralExpr>(E); } +static Optional<std::string> describeRegion(const MemRegion *MR) { + if (const auto *VR = dyn_cast_or_null<VarRegion>(MR)) + return std::string(VR->getDecl()->getName()); + // Once we support more storage locations for bindings, + // this would need to be improved. + return None; +} + /// Returns true if this stack frame is for an Objective-C method that is a /// property getter or setter whose body has been synthesized by the analyzer. static bool isSynthesizedAccessor(const StackFrameContext *SFC) { @@ -1969,8 +1968,8 @@ CFRefReportVisitor::VisitNode(const ExplodedNode *N, const ExplodedNode *PrevN, const Stmt *S = N->getLocation().castAs<StmtPoint>().getStmt(); if (isa<ObjCIvarRefExpr>(S) && - isSynthesizedAccessor(LCtx->getCurrentStackFrame())) { - S = LCtx->getCurrentStackFrame()->getCallSite(); + isSynthesizedAccessor(LCtx->getStackFrame())) { + S = LCtx->getStackFrame()->getCallSite(); } if (isa<ObjCArrayLiteral>(S)) { @@ -2298,7 +2297,7 @@ GetAllocationSite(ProgramStateManager& StateMgr, const ExplodedNode *N, 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->getCurrentStackFrame()) + if (!VR || VR->getStackFrame() == LeakContext->getStackFrame()) FirstBinding = R; } @@ -2356,14 +2355,14 @@ GetAllocationSite(ProgramStateManager& StateMgr, const ExplodedNode *N, InterestingMethodContext); } -std::unique_ptr<PathDiagnosticPiece> +std::shared_ptr<PathDiagnosticPiece> CFRefReportVisitor::getEndPath(BugReporterContext &BRC, const ExplodedNode *EndN, BugReport &BR) { BR.markInteresting(Sym); return BugReporterVisitor::getDefaultEndPath(BRC, EndN, BR); } -std::unique_ptr<PathDiagnosticPiece> +std::shared_ptr<PathDiagnosticPiece> CFRefLeakReportVisitor::getEndPath(BugReporterContext &BRC, const ExplodedNode *EndN, BugReport &BR) { @@ -2393,9 +2392,9 @@ CFRefLeakReportVisitor::getEndPath(BugReporterContext &BRC, os << "Object leaked: "; - if (FirstBinding) { - os << "object allocated and stored into '" - << FirstBinding->getString() << '\''; + Optional<std::string> RegionDescription = describeRegion(FirstBinding); + if (RegionDescription) { + os << "object allocated and stored into '" << *RegionDescription << '\''; } else os << "allocated object"; @@ -2450,7 +2449,7 @@ CFRefLeakReportVisitor::getEndPath(BugReporterContext &BRC, os << " is not referenced later in this execution path and has a retain " "count of +" << RV->getCount(); - return llvm::make_unique<PathDiagnosticEventPiece>(L, os.str()); + return std::make_shared<PathDiagnosticEventPiece>(L, os.str()); } void CFRefLeakReport::deriveParamLocation(CheckerContext &Ctx, SymbolRef sym) { @@ -2513,7 +2512,8 @@ void CFRefLeakReport::deriveAllocLocation(CheckerContext &Ctx,SymbolRef sym) { UniqueingDecl = AllocNode->getLocationContext()->getDecl(); } -void CFRefLeakReport::createDescription(CheckerContext &Ctx, bool GCEnabled, bool IncludeAllocationLine) { +void CFRefLeakReport::createDescription(CheckerContext &Ctx, bool GCEnabled, + bool IncludeAllocationLine) { assert(Location.isValid() && UniqueingDecl && UniqueingLocation.isValid()); Description.clear(); llvm::raw_string_ostream os(Description); @@ -2522,8 +2522,9 @@ void CFRefLeakReport::createDescription(CheckerContext &Ctx, bool GCEnabled, boo os << "(when using garbage collection) "; os << "of an object"; - if (AllocBinding) { - os << " stored into '" << AllocBinding->getString() << '\''; + Optional<std::string> RegionDescription = describeRegion(AllocBinding); + if (RegionDescription) { + os << " stored into '" << *RegionDescription << '\''; if (IncludeAllocationLine) { FullSourceLoc SL(AllocStmt->getLocStart(), Ctx.getSourceManager()); os << " (allocated on line " << SL.getSpellingLineNumber() << ")"; @@ -2742,7 +2743,7 @@ public: void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const; void checkBeginFunction(CheckerContext &C) const; - void checkEndFunction(CheckerContext &C) const; + void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const; ProgramStateRef updateSymbol(ProgramStateRef state, SymbolRef sym, RefVal V, ArgEffect E, RefVal::Kind &hasErr, @@ -2799,9 +2800,7 @@ void RetainCountChecker::checkPostStmt(const BlockExpr *BE, return; ProgramStateRef state = C.getState(); - const BlockDataRegion *R = - cast<BlockDataRegion>(state->getSVal(BE, - C.getLocationContext()).getAsRegion()); + auto *R = cast<BlockDataRegion>(C.getSVal(BE).getAsRegion()); BlockDataRegion::referenced_vars_iterator I = R->referenced_vars_begin(), E = R->referenced_vars_end(); @@ -2851,7 +2850,7 @@ void RetainCountChecker::checkPostStmt(const CastExpr *CE, } ProgramStateRef state = C.getState(); - SymbolRef Sym = state->getSVal(CE, C.getLocationContext()).getAsLocSymbol(); + SymbolRef Sym = C.getSVal(CE).getAsLocSymbol(); if (!Sym) return; const RefVal* T = getRefBinding(state, Sym); @@ -2874,7 +2873,7 @@ void RetainCountChecker::processObjCLiterals(CheckerContext &C, ProgramStateRef state = C.getState(); const ExplodedNode *pred = C.getPredecessor(); for (const Stmt *Child : Ex->children()) { - SVal V = state->getSVal(Child, pred->getLocationContext()); + SVal V = pred->getSVal(Child); if (SymbolRef sym = V.getAsSymbol()) if (const RefVal* T = getRefBinding(state, sym)) { RefVal::Kind hasErr = (RefVal::Kind) 0; @@ -2913,10 +2912,9 @@ void RetainCountChecker::checkPostStmt(const ObjCDictionaryLiteral *DL, void RetainCountChecker::checkPostStmt(const ObjCBoxedExpr *Ex, CheckerContext &C) const { const ExplodedNode *Pred = C.getPredecessor(); - const LocationContext *LCtx = Pred->getLocationContext(); ProgramStateRef State = Pred->getState(); - if (SymbolRef Sym = State->getSVal(Ex, LCtx).getAsSymbol()) { + if (SymbolRef Sym = Pred->getSVal(Ex).getAsSymbol()) { QualType ResultTy = Ex->getType(); State = setRefBinding(State, Sym, RefVal::makeNotOwned(RetEffect::ObjC, ResultTy)); @@ -3993,7 +3991,8 @@ void RetainCountChecker::checkBeginFunction(CheckerContext &Ctx) const { Ctx.addTransition(state); } -void RetainCountChecker::checkEndFunction(CheckerContext &Ctx) const { +void RetainCountChecker::checkEndFunction(const ReturnStmt *RS, + CheckerContext &Ctx) const { ProgramStateRef state = Ctx.getState(); RefBindingsTy B = state->get<RefBindings>(); ExplodedNode *Pred = Ctx.getPredecessor(); diff --git a/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp b/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp index 19fa0fb193cc..1952715a9b7c 100644 --- a/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp @@ -40,7 +40,7 @@ void ReturnPointerRangeChecker::checkPreStmt(const ReturnStmt *RS, if (!RetE) return; - SVal V = state->getSVal(RetE, C.getLocationContext()); + SVal V = C.getSVal(RetE); const MemRegion *R = V.getAsRegion(); const ElementRegion *ER = dyn_cast_or_null<ElementRegion>(R); diff --git a/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp b/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp new file mode 100644 index 000000000000..64b61a0213d2 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/RunLoopAutoreleaseLeakChecker.cpp @@ -0,0 +1,217 @@ +//=- RunLoopAutoreleaseLeakChecker.cpp --------------------------*- C++ -*-==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +// +//===----------------------------------------------------------------------===// +// +// A checker for detecting leaks resulting from allocating temporary +// autoreleased objects before starting the main run loop. +// +// Checks for two antipatterns: +// 1. ObjCMessageExpr followed by [[NSRunLoop mainRunLoop] run] in the same +// autorelease pool. +// 2. ObjCMessageExpr followed by [[NSRunLoop mainRunLoop] run] in no +// autorelease pool. +// +// Any temporary objects autoreleased in code called in those expressions +// will not be deallocated until the program exits, and are effectively leaks. +// +//===----------------------------------------------------------------------===// +// + +#include "ClangSACheckers.h" +#include "clang/AST/Decl.h" +#include "clang/AST/DeclObjC.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h" + +using namespace clang; +using namespace ento; +using namespace ast_matchers; + +namespace { + +const char * RunLoopBind = "NSRunLoopM"; +const char * RunLoopRunBind = "RunLoopRunM"; +const char * OtherMsgBind = "OtherMessageSentM"; +const char * AutoreleasePoolBind = "AutoreleasePoolM"; + +class RunLoopAutoreleaseLeakChecker : public Checker< + check::ASTCodeBody> { + +public: + void checkASTCodeBody(const Decl *D, + AnalysisManager &AM, + BugReporter &BR) const; + +}; + +} // 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) { + for (const Stmt *C : Parent->children()) { + if (C == A) + return true; + + 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 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(); +} + +static void emitDiagnostics(BoundNodes &Match, + const Decl *D, + BugReporter &BR, + AnalysisManager &AM, + const RunLoopAutoreleaseLeakChecker *Checker) { + + assert(D->hasBody()); + const Stmt *DeclBody = D->getBody(); + + AnalysisDeclContext *ADC = AM.getAnalysisDeclContext(D); + + const auto *ME = Match.getNodeAs<ObjCMessageExpr>(OtherMsgBind); + assert(ME); + + const auto *AP = + Match.getNodeAs<ObjCAutoreleasePoolStmt>(AutoreleasePoolBind); + bool HasAutoreleasePool = (AP != nullptr); + + const auto *RL = Match.getNodeAs<ObjCMessageExpr>(RunLoopBind); + const auto *RLR = Match.getNodeAs<Stmt>(RunLoopRunBind); + assert(RLR && "Run loop launch not found"); + + assert(ME != RLR); + if (HasAutoreleasePool && seenBefore(AP, RLR, ME)) + return; + + if (!HasAutoreleasePool && seenBefore(DeclBody, RLR, ME)) + return; + + PathDiagnosticLocation Location = PathDiagnosticLocation::createBegin( + ME, BR.getSourceManager(), ADC); + SourceRange Range = ME->getSourceRange(); + + BR.EmitBasicReport(ADC->getDecl(), Checker, + /*Name=*/"Memory leak inside autorelease pool", + /*Category=*/"Memory", + /*Name=*/ + (Twine("Temporary objects allocated in the") + + " autorelease pool " + + (HasAutoreleasePool ? "" : "of last resort ") + + "followed by the launch of " + + (RL ? "main run loop " : "xpc_main ") + + "may never get released; consider moving them to a " + "separate autorelease pool") + .str(), + Location, Range); +} + +static StatementMatcher getRunLoopRunM(StatementMatcher Extra = anything()) { + StatementMatcher MainRunLoopM = + objcMessageExpr(hasSelector("mainRunLoop"), + hasReceiverType(asString("NSRunLoop")), + Extra) + .bind(RunLoopBind); + + StatementMatcher MainRunLoopRunM = objcMessageExpr(hasSelector("run"), + hasReceiver(MainRunLoopM), + Extra).bind(RunLoopRunBind); + + StatementMatcher XPCRunM = + callExpr(callee(functionDecl(hasName("xpc_main")))).bind(RunLoopRunBind); + return anyOf(MainRunLoopRunM, XPCRunM); +} + +static StatementMatcher getOtherMessageSentM(StatementMatcher Extra = anything()) { + return objcMessageExpr(unless(anyOf(equalsBoundNode(RunLoopBind), + equalsBoundNode(RunLoopRunBind))), + Extra) + .bind(OtherMsgBind); +} + +static void +checkTempObjectsInSamePool(const Decl *D, AnalysisManager &AM, BugReporter &BR, + const RunLoopAutoreleaseLeakChecker *Chkr) { + StatementMatcher RunLoopRunM = getRunLoopRunM(); + StatementMatcher OtherMessageSentM = getOtherMessageSentM(); + + StatementMatcher RunLoopInAutorelease = + autoreleasePoolStmt( + hasDescendant(RunLoopRunM), + hasDescendant(OtherMessageSentM)).bind(AutoreleasePoolBind); + + DeclarationMatcher GroupM = decl(hasDescendant(RunLoopInAutorelease)); + + auto Matches = match(GroupM, *D, AM.getASTContext()); + for (BoundNodes Match : Matches) + emitDiagnostics(Match, D, BR, AM, Chkr); +} + +static void +checkTempObjectsInNoPool(const Decl *D, AnalysisManager &AM, BugReporter &BR, + const RunLoopAutoreleaseLeakChecker *Chkr) { + + auto NoPoolM = unless(hasAncestor(autoreleasePoolStmt())); + + StatementMatcher RunLoopRunM = getRunLoopRunM(NoPoolM); + StatementMatcher OtherMessageSentM = getOtherMessageSentM(NoPoolM); + + DeclarationMatcher GroupM = functionDecl( + isMain(), + hasDescendant(RunLoopRunM), + hasDescendant(OtherMessageSentM) + ); + + auto Matches = match(GroupM, *D, AM.getASTContext()); + + for (BoundNodes Match : Matches) + emitDiagnostics(Match, D, BR, AM, Chkr); + +} + +void RunLoopAutoreleaseLeakChecker::checkASTCodeBody(const Decl *D, + AnalysisManager &AM, + BugReporter &BR) const { + checkTempObjectsInSamePool(D, AM, BR, this); + checkTempObjectsInNoPool(D, AM, BR, this); +} + +void ento::registerRunLoopAutoreleaseLeakChecker(CheckerManager &mgr) { + mgr.registerChecker<RunLoopAutoreleaseLeakChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp index 25975628c553..feae9e59b343 100644 --- a/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp @@ -47,7 +47,7 @@ public: void checkPreCall(const CallEvent &Call, CheckerContext &C) const; void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const; - void checkEndFunction(CheckerContext &Ctx) const; + void checkEndFunction(const ReturnStmt *RS, CheckerContext &Ctx) const; private: void checkReturnedBlockCaptures(const BlockDataRegion &B, @@ -120,7 +120,7 @@ bool StackAddrEscapeChecker::isArcManagedBlock(const MemRegion *R, bool StackAddrEscapeChecker::isNotInCurrentFrame(const MemRegion *R, CheckerContext &C) { const StackSpaceRegion *S = cast<StackSpaceRegion>(R->getMemorySpace()); - return S->getStackFrame() != C.getLocationContext()->getCurrentStackFrame(); + return S->getStackFrame() != C.getStackFrame(); } bool StackAddrEscapeChecker::isSemaphoreCaptured(const BlockDecl &B) const { @@ -255,8 +255,7 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, return; RetE = RetE->IgnoreParens(); - const LocationContext *LCtx = C.getLocationContext(); - SVal V = C.getState()->getSVal(RetE, LCtx); + SVal V = C.getSVal(RetE); const MemRegion *R = V.getAsRegion(); if (!R) return; @@ -288,7 +287,8 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS, EmitStackError(C, R, RetE); } -void StackAddrEscapeChecker::checkEndFunction(CheckerContext &Ctx) const { +void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS, + CheckerContext &Ctx) const { if (!ChecksEnabled[CK_StackAddrEscapeChecker]) return; @@ -304,8 +304,7 @@ void StackAddrEscapeChecker::checkEndFunction(CheckerContext &Ctx) const { public: SmallVector<std::pair<const MemRegion *, const MemRegion *>, 10> V; - CallBack(CheckerContext &CC) - : Ctx(CC), CurSFC(CC.getLocationContext()->getCurrentStackFrame()) {} + CallBack(CheckerContext &CC) : Ctx(CC), CurSFC(CC.getStackFrame()) {} bool HandleBinding(StoreManager &SMgr, Store S, const MemRegion *Region, SVal Val) override { diff --git a/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index 915514b42133..d77975559e3f 100644 --- a/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -242,22 +242,19 @@ void StreamChecker::Fclose(CheckerContext &C, const CallExpr *CE) const { void StreamChecker::Fread(CheckerContext &C, const CallExpr *CE) const { ProgramStateRef state = C.getState(); - if (!CheckNullStream(state->getSVal(CE->getArg(3), C.getLocationContext()), - state, C)) + if (!CheckNullStream(C.getSVal(CE->getArg(3)), state, C)) return; } void StreamChecker::Fwrite(CheckerContext &C, const CallExpr *CE) const { ProgramStateRef state = C.getState(); - if (!CheckNullStream(state->getSVal(CE->getArg(3), C.getLocationContext()), - state, C)) + if (!CheckNullStream(C.getSVal(CE->getArg(3)), state, C)) return; } void StreamChecker::Fseek(CheckerContext &C, const CallExpr *CE) const { ProgramStateRef state = C.getState(); - if (!(state = CheckNullStream(state->getSVal(CE->getArg(0), - C.getLocationContext()), state, C))) + if (!(state = CheckNullStream(C.getSVal(CE->getArg(0)), state, C))) return; // Check the legality of the 'whence' argument of 'fseek'. SVal Whence = state->getSVal(CE->getArg(2), C.getLocationContext()); @@ -283,57 +280,49 @@ void StreamChecker::Fseek(CheckerContext &C, const CallExpr *CE) const { void StreamChecker::Ftell(CheckerContext &C, const CallExpr *CE) const { ProgramStateRef state = C.getState(); - if (!CheckNullStream(state->getSVal(CE->getArg(0), C.getLocationContext()), - state, C)) + if (!CheckNullStream(C.getSVal(CE->getArg(0)), state, C)) return; } void StreamChecker::Rewind(CheckerContext &C, const CallExpr *CE) const { ProgramStateRef state = C.getState(); - if (!CheckNullStream(state->getSVal(CE->getArg(0), C.getLocationContext()), - state, C)) + if (!CheckNullStream(C.getSVal(CE->getArg(0)), state, C)) return; } void StreamChecker::Fgetpos(CheckerContext &C, const CallExpr *CE) const { ProgramStateRef state = C.getState(); - if (!CheckNullStream(state->getSVal(CE->getArg(0), C.getLocationContext()), - state, C)) + if (!CheckNullStream(C.getSVal(CE->getArg(0)), state, C)) return; } void StreamChecker::Fsetpos(CheckerContext &C, const CallExpr *CE) const { ProgramStateRef state = C.getState(); - if (!CheckNullStream(state->getSVal(CE->getArg(0), C.getLocationContext()), - state, C)) + if (!CheckNullStream(C.getSVal(CE->getArg(0)), state, C)) return; } void StreamChecker::Clearerr(CheckerContext &C, const CallExpr *CE) const { ProgramStateRef state = C.getState(); - if (!CheckNullStream(state->getSVal(CE->getArg(0), C.getLocationContext()), - state, C)) + if (!CheckNullStream(C.getSVal(CE->getArg(0)), state, C)) return; } void StreamChecker::Feof(CheckerContext &C, const CallExpr *CE) const { ProgramStateRef state = C.getState(); - if (!CheckNullStream(state->getSVal(CE->getArg(0), C.getLocationContext()), - state, C)) + if (!CheckNullStream(C.getSVal(CE->getArg(0)), state, C)) return; } void StreamChecker::Ferror(CheckerContext &C, const CallExpr *CE) const { ProgramStateRef state = C.getState(); - if (!CheckNullStream(state->getSVal(CE->getArg(0), C.getLocationContext()), - state, C)) + if (!CheckNullStream(C.getSVal(CE->getArg(0)), state, C)) return; } void StreamChecker::Fileno(CheckerContext &C, const CallExpr *CE) const { ProgramStateRef state = C.getState(); - if (!CheckNullStream(state->getSVal(CE->getArg(0), C.getLocationContext()), - state, C)) + if (!CheckNullStream(C.getSVal(CE->getArg(0)), state, C)) return; } @@ -363,8 +352,7 @@ ProgramStateRef StreamChecker::CheckNullStream(SVal SV, ProgramStateRef state, ProgramStateRef StreamChecker::CheckDoubleClose(const CallExpr *CE, ProgramStateRef state, CheckerContext &C) const { - SymbolRef Sym = - state->getSVal(CE->getArg(0), C.getLocationContext()).getAsSymbol(); + SymbolRef Sym = C.getSVal(CE->getArg(0)).getAsSymbol(); if (!Sym) return state; diff --git a/lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp b/lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp index 5268bbf5562e..f4c0edbab3f0 100644 --- a/lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp @@ -55,7 +55,7 @@ public: } }; -class DivisionBRVisitor : public BugReporterVisitorImpl<DivisionBRVisitor> { +class DivisionBRVisitor : public BugReporterVisitor { private: SymbolRef ZeroSymbol; const StackFrameContext *SFC; @@ -85,7 +85,7 @@ class TestAfterDivZeroChecker public: void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const; void checkBranchCondition(const Stmt *Condition, CheckerContext &C) const; - void checkEndFunction(CheckerContext &C) const; + void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const; void setDivZeroMap(SVal Var, CheckerContext &C) const; bool hasDivZeroMap(SVal Var, const CheckerContext &C) const; bool isZero(SVal S, CheckerContext &C) const; @@ -114,8 +114,7 @@ DivisionBRVisitor::VisitNode(const ExplodedNode *Succ, const ExplodedNode *Pred, if (!E) return nullptr; - ProgramStateRef State = Succ->getState(); - SVal S = State->getSVal(E, Succ->getLocationContext()); + SVal S = Succ->getSVal(E); if (ZeroSymbol == S.getAsSymbol() && SFC == Succ->getStackFrame()) { Satisfied = true; @@ -181,7 +180,8 @@ void TestAfterDivZeroChecker::reportBug(SVal Val, CheckerContext &C) const { } } -void TestAfterDivZeroChecker::checkEndFunction(CheckerContext &C) const { +void TestAfterDivZeroChecker::checkEndFunction(const ReturnStmt *RS, + CheckerContext &C) const { ProgramStateRef State = C.getState(); DivZeroMapTy DivZeroes = State->get<DivZeroMap>(); diff --git a/lib/StaticAnalyzer/Checkers/TraversalChecker.cpp b/lib/StaticAnalyzer/Checkers/TraversalChecker.cpp index 8ad962875b06..ee185b813611 100644 --- a/lib/StaticAnalyzer/Checkers/TraversalChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/TraversalChecker.cpp @@ -30,7 +30,7 @@ class TraversalDumper : public Checker< check::BranchCondition, public: void checkBranchCondition(const Stmt *Condition, CheckerContext &C) const; void checkBeginFunction(CheckerContext &C) const; - void checkEndFunction(CheckerContext &C) const; + void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const; }; } @@ -56,7 +56,8 @@ void TraversalDumper::checkBeginFunction(CheckerContext &C) const { llvm::outs() << "--BEGIN FUNCTION--\n"; } -void TraversalDumper::checkEndFunction(CheckerContext &C) const { +void TraversalDumper::checkEndFunction(const ReturnStmt *RS, + CheckerContext &C) const { llvm::outs() << "--END FUNCTION--\n"; } diff --git a/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp b/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp new file mode 100644 index 000000000000..f3d68014224d --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/TrustNonnullChecker.cpp @@ -0,0 +1,90 @@ +//== TrustNonnullChecker.cpp - Checker for trusting annotations -*- C++ -*--==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This checker adds an assumption that methods annotated with _Nonnull +// which come from system headers actually return a non-null pointer. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" + +using namespace clang; +using namespace ento; + +namespace { + +class TrustNonnullChecker : public Checker<check::PostCall> { +private: + /// \returns Whether we trust the result of the method call to be + /// a non-null pointer. + bool isNonNullPtr(const CallEvent &Call, CheckerContext &C) const { + QualType ExprRetType = Call.getResultType(); + if (!ExprRetType->isAnyPointerType()) + return false; + + if (getNullabilityAnnotation(ExprRetType) == Nullability::Nonnull) + return true; + + // The logic for ObjC instance method calls is more complicated, + // as the return value is nil when the receiver is nil. + if (!isa<ObjCMethodCall>(&Call)) + return false; + + const auto *MCall = cast<ObjCMethodCall>(&Call); + const ObjCMethodDecl *MD = MCall->getDecl(); + + // Distrust protocols. + if (isa<ObjCProtocolDecl>(MD->getDeclContext())) + return false; + + QualType DeclRetType = MD->getReturnType(); + if (getNullabilityAnnotation(DeclRetType) != Nullability::Nonnull) + return false; + + // For class messages it is sufficient for the declaration to be + // annotated _Nonnull. + if (!MCall->isInstanceMessage()) + return true; + + // Alternatively, the analyzer could know that the receiver is not null. + SVal Receiver = MCall->getReceiverSVal(); + ConditionTruthVal TV = C.getState()->isNonNull(Receiver); + if (TV.isConstrainedTrue()) + return true; + + return false; + } + +public: + void checkPostCall(const CallEvent &Call, CheckerContext &C) const { + // Only trust annotations for system headers for non-protocols. + if (!Call.isInSystemHeader()) + return; + + ProgramStateRef State = C.getState(); + + if (isNonNullPtr(Call, C)) + if (auto L = Call.getReturnValue().getAs<Loc>()) + State = State->assume(*L, /*Assumption=*/true); + + C.addTransition(State); + } +}; + +} // end empty namespace + + +void ento::registerTrustNonnullChecker(CheckerManager &Mgr) { + Mgr.registerChecker<TrustNonnullChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp index 0a274292aa39..934ee63318fa 100644 --- a/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp @@ -59,7 +59,7 @@ public: void UndefBranchChecker::checkBranchCondition(const Stmt *Condition, CheckerContext &Ctx) const { - SVal X = Ctx.getState()->getSVal(Condition, Ctx.getLocationContext()); + SVal X = Ctx.getSVal(Condition); if (X.isUndef()) { // Generate a sink node, which implicitly marks both outgoing branches as // infeasible. diff --git a/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp index 17fe8610da06..6a93c10c7644 100644 --- a/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp @@ -55,9 +55,7 @@ UndefCapturedBlockVarChecker::checkPostStmt(const BlockExpr *BE, return; ProgramStateRef state = C.getState(); - const BlockDataRegion *R = - cast<BlockDataRegion>(state->getSVal(BE, - C.getLocationContext()).getAsRegion()); + auto *R = cast<BlockDataRegion>(C.getSVal(BE).getAsRegion()); BlockDataRegion::referenced_vars_iterator I = R->referenced_vars_begin(), E = R->referenced_vars_end(); diff --git a/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp index 172ce346f1ba..b9a93bedca2e 100644 --- a/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp @@ -37,12 +37,11 @@ public: static bool isArrayIndexOutOfBounds(CheckerContext &C, const Expr *Ex) { ProgramStateRef state = C.getState(); - const LocationContext *LCtx = C.getLocationContext(); if (!isa<ArraySubscriptExpr>(Ex)) return false; - SVal Loc = state->getSVal(Ex, LCtx); + SVal Loc = C.getSVal(Ex); if (!Loc.isValid()) return false; @@ -64,11 +63,18 @@ static bool isShiftOverflow(const BinaryOperator *B, CheckerContext &C) { B->getRHS(), C.getASTContext().getIntWidth(B->getLHS()->getType())); } +static bool isLeftShiftResultUnrepresentable(const BinaryOperator *B, + CheckerContext &C) { + SValBuilder &SB = C.getSValBuilder(); + ProgramStateRef State = C.getState(); + const llvm::APSInt *LHS = SB.getKnownValue(State, C.getSVal(B->getLHS())); + const llvm::APSInt *RHS = SB.getKnownValue(State, C.getSVal(B->getRHS())); + return (unsigned)RHS->getZExtValue() > LHS->countLeadingZeros(); +} + void UndefResultChecker::checkPostStmt(const BinaryOperator *B, CheckerContext &C) const { - ProgramStateRef state = C.getState(); - const LocationContext *LCtx = C.getLocationContext(); - if (state->getSVal(B, LCtx).isUndef()) { + if (C.getSVal(B).isUndef()) { // Do not report assignments of uninitialized values inside swap functions. // This should allow to swap partially uninitialized structs @@ -92,11 +98,11 @@ void UndefResultChecker::checkPostStmt(const BinaryOperator *B, const Expr *Ex = nullptr; bool isLeft = true; - if (state->getSVal(B->getLHS(), LCtx).isUndef()) { + if (C.getSVal(B->getLHS()).isUndef()) { Ex = B->getLHS()->IgnoreParenCasts(); isLeft = true; } - else if (state->getSVal(B->getRHS(), LCtx).isUndef()) { + else if (C.getSVal(B->getRHS()).isUndef()) { Ex = B->getRHS()->IgnoreParenCasts(); isLeft = false; } @@ -141,6 +147,19 @@ void UndefResultChecker::checkPostStmt(const BinaryOperator *B, C.isNegative(B->getLHS())) { OS << "The result of the left shift is undefined because the left " "operand is negative"; + } else if (B->getOpcode() == BinaryOperatorKind::BO_Shl && + isLeftShiftResultUnrepresentable(B, C)) { + ProgramStateRef State = C.getState(); + SValBuilder &SB = C.getSValBuilder(); + const llvm::APSInt *LHS = + SB.getKnownValue(State, C.getSVal(B->getLHS())); + const llvm::APSInt *RHS = + SB.getKnownValue(State, C.getSVal(B->getRHS())); + OS << "The result of the left shift is undefined due to shifting \'" + << LHS->getSExtValue() << "\' by \'" << RHS->getZExtValue() + << "\', which is unrepresentable in the unsigned version of " + << "the return type \'" << B->getLHS()->getType().getAsString() + << "\'"; } else { OS << "The result of the '" << BinaryOperator::getOpcodeStr(B->getOpcode()) diff --git a/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp index c3dcf1fac197..2ef6855ba6b7 100644 --- a/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp @@ -51,17 +51,20 @@ void UndefinedAssignmentChecker::checkBind(SVal location, SVal val, if (!N) return; - const char *str = "Assigned value is garbage or undefined"; - + static const char *const DefaultMsg = + "Assigned value is garbage or undefined"; if (!BT) - BT.reset(new BuiltinBug(this, str)); + BT.reset(new BuiltinBug(this, DefaultMsg)); // Generate a report for this bug. + llvm::SmallString<128> Str; + llvm::raw_svector_ostream OS(Str); + const Expr *ex = nullptr; while (StoreE) { if (const UnaryOperator *U = dyn_cast<UnaryOperator>(StoreE)) { - str = "The expression is an uninitialized value. " + OS << "The expression is an uninitialized value. " "The computed value will also be garbage"; ex = U->getSubExpr(); @@ -70,9 +73,8 @@ void UndefinedAssignmentChecker::checkBind(SVal location, SVal val, if (const BinaryOperator *B = dyn_cast<BinaryOperator>(StoreE)) { if (B->isCompoundAssignmentOp()) { - ProgramStateRef state = C.getState(); - if (state->getSVal(B->getLHS(), C.getLocationContext()).isUndef()) { - str = "The left expression of the compound assignment is an " + if (C.getSVal(B->getLHS()).isUndef()) { + OS << "The left expression of the compound assignment is an " "uninitialized value. The computed value will also be garbage"; ex = B->getLHS(); break; @@ -88,10 +90,26 @@ void UndefinedAssignmentChecker::checkBind(SVal location, SVal val, ex = VD->getInit(); } + if (const auto *CD = + dyn_cast<CXXConstructorDecl>(C.getStackFrame()->getDecl())) { + if (CD->isImplicit()) { + for (auto I : CD->inits()) { + if (I->getInit()->IgnoreImpCasts() == StoreE) { + OS << "Value assigned to field '" << I->getMember()->getName() + << "' in implicit constructor is garbage or undefined"; + break; + } + } + } + } + break; } - auto R = llvm::make_unique<BugReport>(*BT, str, N); + if (OS.str().empty()) + OS << DefaultMsg; + + auto R = llvm::make_unique<BugReport>(*BT, OS.str(), N); if (ex) { R->addRange(ex->getSourceRange()); bugreporter::trackNullOrUndefValue(N, ex, *R); diff --git a/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp b/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp new file mode 100644 index 000000000000..398228a9d887 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp @@ -0,0 +1,688 @@ +//===----- UninitializedObjectChecker.cpp ------------------------*- C++ -*-==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This file defines a checker that reports uninitialized fields in objects +// created after a constructor call. +// +// This checker has two options: +// - "Pedantic" (boolean). If its not set or is set to false, the checker +// won't emit warnings for objects that don't have at least one initialized +// field. This may be set with +// +// `-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 +// per constructor call, and listing the uninitialized fields that belongs +// to it in notes. Defaults to false. +// +// `-analyzer-config alpha.cplusplus.UninitializedObject:NotesAsWarnings=true`. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include <algorithm> + +using namespace clang; +using namespace clang::ento; + +namespace { + +class UninitializedObjectChecker : public Checker<check::EndFunction> { + std::unique_ptr<BuiltinBug> BT_uninitField; + +public: + // These fields will be initialized when registering the checker. + bool IsPedantic; + bool ShouldConvertNotesToWarnings; + + UninitializedObjectChecker() + : BT_uninitField(new BuiltinBug(this, "Uninitialized fields")) {} + void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const; +}; + +/// Represents a field chain. A field chain is a vector of fields where the +/// first element of the chain is the object under checking (not stored), and +/// every other element is a field, and the element that precedes it is the +/// object that contains it. +/// +/// Note that this class is immutable, and new fields may only be added through +/// constructor calls. +class FieldChainInfo { + using FieldChain = llvm::ImmutableList<const FieldRegion *>; + + FieldChain Chain; + + const bool IsDereferenced = false; + +public: + FieldChainInfo() = default; + + FieldChainInfo(const FieldChainInfo &Other, const bool IsDereferenced) + : Chain(Other.Chain), IsDereferenced(IsDereferenced) {} + + FieldChainInfo(const FieldChainInfo &Other, const FieldRegion *FR, + const bool IsDereferenced = false); + + bool contains(const FieldRegion *FR) const { return Chain.contains(FR); } + bool isPointer() const; + + /// If this is a fieldchain whose last element is an uninitialized region of a + /// pointer type, `IsDereferenced` will store whether the pointer itself or + /// the pointee is uninitialized. + bool isDereferenced() const; + const FieldDecl *getEndOfChain() const; + void print(llvm::raw_ostream &Out) const; + +private: + /// Prints every element except the last to `Out`. Since ImmutableLists store + /// elements in reverse order, and have no reverse iterators, we use a + /// recursive function to print the fieldchain correctly. The last element in + /// the chain is to be printed by `print`. + static void printTail(llvm::raw_ostream &Out, + const llvm::ImmutableListImpl<const FieldRegion *> *L); + friend struct FieldChainInfoComparator; +}; + +struct FieldChainInfoComparator { + bool operator()(const FieldChainInfo &lhs, const FieldChainInfo &rhs) const { + assert(!lhs.Chain.isEmpty() && !rhs.Chain.isEmpty() && + "Attempted to store an empty fieldchain!"); + return *lhs.Chain.begin() < *rhs.Chain.begin(); + } +}; + +using UninitFieldSet = std::set<FieldChainInfo, FieldChainInfoComparator>; + +/// Searches for and stores uninitialized fields in a non-union object. +class FindUninitializedFields { + ProgramStateRef State; + const TypedValueRegion *const ObjectR; + + const bool IsPedantic; + bool IsAnyFieldInitialized = false; + + UninitFieldSet UninitFields; + +public: + FindUninitializedFields(ProgramStateRef State, + const TypedValueRegion *const R, bool IsPedantic); + const UninitFieldSet &getUninitFields(); + +private: + /// Adds a FieldChainInfo object to UninitFields. Return true if an insertion + /// took place. + bool addFieldToUninits(FieldChainInfo LocalChain); + + // For the purposes of this checker, we'll regard the object under checking as + // a directed tree, where + // * the root is the object under checking + // * every node is an object that is + // - a union + // - a non-union record + // - a pointer/reference + // - an array + // - of a primitive type, which we'll define later in a helper function. + // * the parent of each node is the object that contains it + // * every leaf is an array, a primitive object, a nullptr or an undefined + // pointer. + // + // Example: + // + // struct A { + // struct B { + // int x, y = 0; + // }; + // B b; + // int *iptr = new int; + // B* bptr; + // + // A() {} + // }; + // + // The directed tree: + // + // ->x + // / + // ->b--->y + // / + // A-->iptr->(int value) + // \ + // ->bptr + // + // From this we'll construct a vector of fieldchains, where each fieldchain + // represents an uninitialized field. An uninitialized field may be a + // primitive object, a pointer, a pointee or a union without a single + // initialized field. + // In the above example, for the default constructor call we'll end up with + // these fieldchains: + // + // this->b.x + // this->iptr (pointee uninit) + // this->bptr (pointer uninit) + // + // We'll traverse each node of the above graph with the appropiate one of + // these methods: + + /// This method checks a region of a union object, and returns true if no + /// field is initialized within the region. + bool isUnionUninit(const TypedValueRegion *R); + + /// This method checks a region of a non-union object, and returns true if + /// an uninitialized field is found within the region. + bool isNonUnionUninit(const TypedValueRegion *R, FieldChainInfo LocalChain); + + /// This method checks a region of a pointer or reference object, and returns + /// true if the ptr/ref object itself or any field within the pointee's region + /// is uninitialized. + bool isPointerOrReferenceUninit(const FieldRegion *FR, + FieldChainInfo LocalChain); + + /// This method returns true if the value of a primitive object is + /// uninitialized. + bool isPrimitiveUninit(const SVal &V); + + // Note that we don't have a method for arrays -- the elements of an array are + // often left uninitialized intentionally even when it is of a C++ record + // type, so we'll assume that an array is always initialized. + // TODO: Add a support for nonloc::LocAsInteger. +}; + +} // end of anonymous namespace + +// Static variable instantionations. + +static llvm::ImmutableListFactory<const FieldRegion *> Factory; + +// Utility function declarations. + +/// Returns the object that was constructed by CtorDecl, or None if that isn't +/// possible. +static Optional<nonloc::LazyCompoundVal> +getObjectVal(const CXXConstructorDecl *CtorDecl, CheckerContext &Context); + +/// Checks whether the constructor under checking is called by another +/// constructor. +static bool isCalledByConstructor(const CheckerContext &Context); + +/// Returns whether FD can be (transitively) dereferenced to a void pointer type +/// (void*, void**, ...). The type of the region behind a void pointer isn't +/// known, and thus FD can not be analyzed. +static bool isVoidPointer(const FieldDecl *FD); + +/// Returns true if T is a primitive type. We defined this type so that for +/// objects that we'd only like analyze as much as checking whether their +/// value is undefined or not, such as ints and doubles, can be analyzed with +/// ease. This also helps ensuring that every special field type is handled +/// correctly. +static bool isPrimitiveType(const QualType &T) { + return T->isBuiltinType() || T->isEnumeralType() || T->isMemberPointerType(); +} + +/// Constructs a note message for a given FieldChainInfo object. +static void printNoteMessage(llvm::raw_ostream &Out, + const FieldChainInfo &Chain); + +/// Returns with Field's name. This is a helper function to get the correct name +/// even if Field is a captured lambda variable. +static StringRef getVariableName(const FieldDecl *Field); + +//===----------------------------------------------------------------------===// +// Methods for UninitializedObjectChecker. +//===----------------------------------------------------------------------===// + +void UninitializedObjectChecker::checkEndFunction( + const ReturnStmt *RS, CheckerContext &Context) const { + + const auto *CtorDecl = dyn_cast_or_null<CXXConstructorDecl>( + Context.getLocationContext()->getDecl()); + if (!CtorDecl) + return; + + if (!CtorDecl->isUserProvided()) + return; + + if (CtorDecl->getParent()->isUnion()) + return; + + // This avoids essentially the same error being reported multiple times. + if (isCalledByConstructor(Context)) + return; + + Optional<nonloc::LazyCompoundVal> Object = getObjectVal(CtorDecl, Context); + if (!Object) + return; + + FindUninitializedFields F(Context.getState(), Object->getRegion(), + IsPedantic); + + const UninitFieldSet &UninitFields = F.getUninitFields(); + + if (UninitFields.empty()) + return; + + // There are uninitialized fields in the record. + + ExplodedNode *Node = Context.generateNonFatalErrorNode(Context.getState()); + if (!Node) + return; + + PathDiagnosticLocation LocUsedForUniqueing; + const Stmt *CallSite = Context.getStackFrame()->getCallSite(); + if (CallSite) + LocUsedForUniqueing = PathDiagnosticLocation::createBegin( + CallSite, Context.getSourceManager(), Node->getLocationContext()); + + // For Plist consumers that don't support notes just yet, we'll convert notes + // to warnings. + if (ShouldConvertNotesToWarnings) { + for (const auto &Chain : UninitFields) { + SmallString<100> WarningBuf; + llvm::raw_svector_ostream WarningOS(WarningBuf); + + printNoteMessage(WarningOS, Chain); + + auto Report = llvm::make_unique<BugReport>( + *BT_uninitField, WarningOS.str(), Node, LocUsedForUniqueing, + Node->getLocationContext()->getDecl()); + Context.emitReport(std::move(Report)); + } + return; + } + + SmallString<100> WarningBuf; + llvm::raw_svector_ostream WarningOS(WarningBuf); + WarningOS << UninitFields.size() << " uninitialized field" + << (UninitFields.size() == 1 ? "" : "s") + << " at the end of the constructor call"; + + auto Report = llvm::make_unique<BugReport>( + *BT_uninitField, WarningOS.str(), Node, LocUsedForUniqueing, + Node->getLocationContext()->getDecl()); + + for (const auto &Chain : UninitFields) { + SmallString<200> NoteBuf; + llvm::raw_svector_ostream NoteOS(NoteBuf); + + printNoteMessage(NoteOS, Chain); + + Report->addNote(NoteOS.str(), + PathDiagnosticLocation::create(Chain.getEndOfChain(), + Context.getSourceManager())); + } + Context.emitReport(std::move(Report)); +} + +//===----------------------------------------------------------------------===// +// Methods for FindUninitializedFields. +//===----------------------------------------------------------------------===// + +FindUninitializedFields::FindUninitializedFields( + ProgramStateRef State, const TypedValueRegion *const R, bool IsPedantic) + : State(State), ObjectR(R), IsPedantic(IsPedantic) {} + +const UninitFieldSet &FindUninitializedFields::getUninitFields() { + isNonUnionUninit(ObjectR, FieldChainInfo()); + + if (!IsPedantic && !IsAnyFieldInitialized) + UninitFields.clear(); + + return UninitFields; +} + +bool FindUninitializedFields::addFieldToUninits(FieldChainInfo Chain) { + if (State->getStateManager().getContext().getSourceManager().isInSystemHeader( + Chain.getEndOfChain()->getLocation())) + return false; + + return UninitFields.insert(Chain).second; +} + +bool FindUninitializedFields::isNonUnionUninit(const TypedValueRegion *R, + FieldChainInfo LocalChain) { + assert(R->getValueType()->isRecordType() && + !R->getValueType()->isUnionType() && + "This method only checks non-union record objects!"); + + const RecordDecl *RD = + R->getValueType()->getAs<RecordType>()->getDecl()->getDefinition(); + assert(RD && "Referred record has no definition"); + + bool ContainsUninitField = false; + + // Are all of this non-union's fields initialized? + for (const FieldDecl *I : RD->fields()) { + + const auto FieldVal = + State->getLValue(I, loc::MemRegionVal(R)).castAs<loc::MemRegionVal>(); + const auto *FR = FieldVal.getRegionAs<FieldRegion>(); + QualType T = I->getType(); + + // If LocalChain already contains FR, then we encountered a cyclic + // reference. In this case, region FR is already under checking at an + // earlier node in the directed tree. + if (LocalChain.contains(FR)) + return false; + + if (T->isStructureOrClassType()) { + if (isNonUnionUninit(FR, {LocalChain, FR})) + ContainsUninitField = true; + continue; + } + + if (T->isUnionType()) { + if (isUnionUninit(FR)) { + if (addFieldToUninits({LocalChain, FR})) + ContainsUninitField = true; + } else + IsAnyFieldInitialized = true; + continue; + } + + if (T->isArrayType()) { + IsAnyFieldInitialized = true; + continue; + } + + if (T->isPointerType() || T->isReferenceType()) { + if (isPointerOrReferenceUninit(FR, LocalChain)) + ContainsUninitField = true; + continue; + } + + if (isPrimitiveType(T)) { + SVal V = State->getSVal(FieldVal); + + if (isPrimitiveUninit(V)) { + if (addFieldToUninits({LocalChain, FR})) + ContainsUninitField = true; + } + continue; + } + + llvm_unreachable("All cases are handled!"); + } + + // Checking bases. + // FIXME: As of now, because of `isCalledByConstructor`, objects whose type + // is a descendant of another type will emit warnings for uninitalized + // inherited members. + // This is not the only way to analyze bases of an object -- if we didn't + // filter them out, and didn't analyze the bases, this checker would run for + // each base of the object in order of base initailization and in theory would + // find every uninitalized field. This approach could also make handling + // diamond inheritances more easily. + // + // This rule (that a descendant type's cunstructor is responsible for + // initializing inherited data members) is not obvious, and should it should + // be. + const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD); + if (!CXXRD) + return ContainsUninitField; + + for (const CXXBaseSpecifier &BaseSpec : CXXRD->bases()) { + const auto *BaseRegion = State->getLValue(BaseSpec, R) + .castAs<loc::MemRegionVal>() + .getRegionAs<TypedValueRegion>(); + + if (isNonUnionUninit(BaseRegion, LocalChain)) + ContainsUninitField = true; + } + + return ContainsUninitField; +} + +bool FindUninitializedFields::isUnionUninit(const TypedValueRegion *R) { + assert(R->getValueType()->isUnionType() && + "This method only checks union objects!"); + // TODO: Implement support for union fields. + return false; +} + +// Note that pointers/references don't contain fields themselves, so in this +// function we won't add anything to LocalChain. +bool FindUninitializedFields::isPointerOrReferenceUninit( + const FieldRegion *FR, FieldChainInfo LocalChain) { + + assert((FR->getDecl()->getType()->isPointerType() || + FR->getDecl()->getType()->isReferenceType()) && + "This method only checks pointer/reference objects!"); + + SVal V = State->getSVal(FR); + + if (V.isUnknown() || V.isZeroConstant()) { + IsAnyFieldInitialized = true; + return false; + } + + if (V.isUndef()) { + return addFieldToUninits({LocalChain, FR}); + } + + const FieldDecl *FD = FR->getDecl(); + + // TODO: The dynamic type of a void pointer may be retrieved with + // `getDynamicTypeInfo`. + if (isVoidPointer(FD)) { + IsAnyFieldInitialized = true; + return false; + } + + assert(V.getAs<Loc>() && "V should be Loc at this point!"); + + // At this point the pointer itself is initialized and points to a valid + // location, we'll now check the pointee. + SVal DerefdV = State->getSVal(V.castAs<Loc>()); + + // TODO: Dereferencing should be done according to the dynamic type. + while (Optional<Loc> L = DerefdV.getAs<Loc>()) { + DerefdV = State->getSVal(*L); + } + + // If V is a pointer pointing to a record type. + if (Optional<nonloc::LazyCompoundVal> RecordV = + DerefdV.getAs<nonloc::LazyCompoundVal>()) { + + const TypedValueRegion *R = RecordV->getRegion(); + + // We can't reason about symbolic regions, assume its initialized. + // Note that this also avoids a potential infinite recursion, because + // constructors for list-like classes are checked without being called, and + // the Static Analyzer will construct a symbolic region for Node *next; or + // similar code snippets. + if (R->getSymbolicBase()) { + IsAnyFieldInitialized = true; + return false; + } + + const QualType T = R->getValueType(); + + if (T->isStructureOrClassType()) + return isNonUnionUninit(R, {LocalChain, FR}); + + if (T->isUnionType()) { + if (isUnionUninit(R)) { + return addFieldToUninits({LocalChain, FR, /*IsDereferenced*/ true}); + } else { + IsAnyFieldInitialized = true; + return false; + } + } + + if (T->isArrayType()) { + IsAnyFieldInitialized = true; + return false; + } + + llvm_unreachable("All cases are handled!"); + } + + // TODO: If possible, it should be asserted that the DerefdV at this point is + // primitive. + + if (isPrimitiveUninit(DerefdV)) + return addFieldToUninits({LocalChain, FR, /*IsDereferenced*/ true}); + + IsAnyFieldInitialized = true; + return false; +} + +bool FindUninitializedFields::isPrimitiveUninit(const SVal &V) { + if (V.isUndef()) + return true; + + IsAnyFieldInitialized = true; + return false; +} + +//===----------------------------------------------------------------------===// +// Methods for FieldChainInfo. +//===----------------------------------------------------------------------===// + +FieldChainInfo::FieldChainInfo(const FieldChainInfo &Other, + const FieldRegion *FR, const bool IsDereferenced) + : FieldChainInfo(Other, IsDereferenced) { + assert(!contains(FR) && "Can't add a field that is already a part of the " + "fieldchain! Is this a cyclic reference?"); + Chain = Factory.add(FR, Other.Chain); +} + +bool FieldChainInfo::isPointer() const { + assert(!Chain.isEmpty() && "Empty fieldchain!"); + return (*Chain.begin())->getDecl()->getType()->isPointerType(); +} + +bool FieldChainInfo::isDereferenced() const { + assert(isPointer() && "Only pointers may or may not be dereferenced!"); + return IsDereferenced; +} + +const FieldDecl *FieldChainInfo::getEndOfChain() const { + assert(!Chain.isEmpty() && "Empty fieldchain!"); + return (*Chain.begin())->getDecl(); +} + +// TODO: This function constructs an incorrect fieldchain string in the +// following case: +// +// struct Base { int x; }; +// struct D1 : Base {}; struct D2 : Base {}; +// +// struct MostDerived : D1, D2 { +// MostDerived() {} +// } +// +// A call to MostDerived::MostDerived() will cause two notes that say +// "uninitialized field 'this->x'", but we can't refer to 'x' directly, +// we need an explicit namespace resolution whether the uninit field was +// 'D1::x' or 'D2::x'. +void FieldChainInfo::print(llvm::raw_ostream &Out) const { + if (Chain.isEmpty()) + return; + + const llvm::ImmutableListImpl<const FieldRegion *> *L = + Chain.getInternalPointer(); + printTail(Out, L->getTail()); + Out << getVariableName(L->getHead()->getDecl()); +} + +void FieldChainInfo::printTail( + llvm::raw_ostream &Out, + const llvm::ImmutableListImpl<const FieldRegion *> *L) { + if (!L) + return; + + printTail(Out, L->getTail()); + const FieldDecl *Field = L->getHead()->getDecl(); + Out << getVariableName(Field); + Out << (Field->getType()->isPointerType() ? "->" : "."); +} + +//===----------------------------------------------------------------------===// +// Utility functions. +//===----------------------------------------------------------------------===// + +static bool isVoidPointer(const FieldDecl *FD) { + QualType T = FD->getType(); + + while (!T.isNull()) { + if (T->isVoidPointerType()) + return true; + T = T->getPointeeType(); + } + return false; +} + +static Optional<nonloc::LazyCompoundVal> +getObjectVal(const CXXConstructorDecl *CtorDecl, CheckerContext &Context) { + + Loc ThisLoc = Context.getSValBuilder().getCXXThis(CtorDecl->getParent(), + 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>()); + + return Object.getAs<nonloc::LazyCompoundVal>(); +} + +// TODO: We should also check that if the constructor was called by another +// constructor, whether those two are in any relation to one another. In it's +// current state, this introduces some false negatives. +static bool isCalledByConstructor(const CheckerContext &Context) { + const LocationContext *LC = Context.getLocationContext()->getParent(); + + while (LC) { + if (isa<CXXConstructorDecl>(LC->getDecl())) + return true; + + LC = LC->getParent(); + } + return false; +} + +static void printNoteMessage(llvm::raw_ostream &Out, + const FieldChainInfo &Chain) { + if (Chain.isPointer()) { + if (Chain.isDereferenced()) + Out << "uninitialized pointee 'this->"; + else + Out << "uninitialized pointer 'this->"; + } else + Out << "uninitialized field 'this->"; + Chain.print(Out); + Out << "'"; +} + +static StringRef getVariableName(const FieldDecl *Field) { + // If Field is a captured lambda variable, Field->getName() will return with + // an empty string. We can however acquire it's name from the lambda's + // captures. + const auto *CXXParent = dyn_cast<CXXRecordDecl>(Field->getParent()); + + if (CXXParent && CXXParent->isLambda()) { + assert(CXXParent->captures_begin()); + auto It = CXXParent->captures_begin() + Field->getFieldIndex(); + return It->getCapturedVar()->getName(); + } + + return Field->getName(); +} + +void ento::registerUninitializedObjectChecker(CheckerManager &Mgr) { + auto Chk = Mgr.registerChecker<UninitializedObjectChecker>(); + Chk->IsPedantic = Mgr.getAnalyzerOptions().getBooleanOption( + "Pedantic", /*DefaultVal*/ false, Chk); + Chk->ShouldConvertNotesToWarnings = Mgr.getAnalyzerOptions().getBooleanOption( + "NotesAsWarnings", /*DefaultVal*/ false, Chk); +} diff --git a/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp b/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp index 7f9a00ff876d..a6b50dc37740 100644 --- a/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp @@ -194,7 +194,7 @@ void UnixAPIChecker::CheckOpenVariant(CheckerContext &C, // Now check if oflags has O_CREAT set. const Expr *oflagsEx = CE->getArg(FlagsArgIndex); - const SVal V = state->getSVal(oflagsEx, C.getLocationContext()); + const SVal V = C.getSVal(oflagsEx); if (!V.getAs<NonLoc>()) { // The case where 'V' can be a location can only be due to a bad header, // so in this case bail out. @@ -248,8 +248,7 @@ void UnixAPIChecker::CheckPthreadOnce(CheckerContext &C, // Check if the first argument is stack allocated. If so, issue a warning // because that's likely to be bad news. ProgramStateRef state = C.getState(); - const MemRegion *R = - state->getSVal(CE->getArg(0), C.getLocationContext()).getAsRegion(); + const MemRegion *R = C.getSVal(CE->getArg(0)).getAsRegion(); if (!R || !isa<StackSpaceRegion>(R->getMemorySpace())) return; @@ -336,7 +335,7 @@ void UnixAPIChecker::BasicAllocationCheck(CheckerContext &C, ProgramStateRef state = C.getState(); ProgramStateRef trueState = nullptr, falseState = nullptr; const Expr *arg = CE->getArg(sizeArg); - SVal argVal = state->getSVal(arg, C.getLocationContext()); + SVal argVal = C.getSVal(arg); if (argVal.isUnknownOrUndef()) return; @@ -364,7 +363,7 @@ void UnixAPIChecker::CheckCallocZero(CheckerContext &C, unsigned int i; for (i = 0; i < nArgs; i++) { const Expr *arg = CE->getArg(i); - SVal argVal = state->getSVal(arg, C.getLocationContext()); + SVal argVal = C.getSVal(arg); if (argVal.isUnknownOrUndef()) { if (i == 0) continue; diff --git a/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp b/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp index 6f21e868b174..dbd12cc9b65a 100644 --- a/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp @@ -132,7 +132,8 @@ void UnreachableCodeChecker::checkEndAnalysis(ExplodedGraph &G, ci != ce; ++ci) { if (Optional<CFGStmt> S = (*ci).getAs<CFGStmt>()) if (const CallExpr *CE = dyn_cast<CallExpr>(S->getStmt())) { - if (CE->getBuiltinCallee() == Builtin::BI__builtin_unreachable) { + if (CE->getBuiltinCallee() == Builtin::BI__builtin_unreachable || + CE->isBuiltinAssumeFalse(Eng.getContext())) { foundUnreachable = true; break; } diff --git a/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp b/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp index 40217bdee892..2584f2011819 100644 --- a/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp @@ -32,19 +32,18 @@ class VLASizeChecker : public Checker< check::PreStmt<DeclStmt> > { mutable std::unique_ptr<BugType> BT; enum VLASize_Kind { VLA_Garbage, VLA_Zero, VLA_Tainted, VLA_Negative }; - void reportBug(VLASize_Kind Kind, - const Expr *SizeE, - ProgramStateRef State, - CheckerContext &C) const; + void reportBug(VLASize_Kind Kind, const Expr *SizeE, ProgramStateRef State, + CheckerContext &C, + std::unique_ptr<BugReporterVisitor> Visitor = nullptr) const; + public: void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const; }; } // end anonymous namespace -void VLASizeChecker::reportBug(VLASize_Kind Kind, - const Expr *SizeE, - ProgramStateRef State, - CheckerContext &C) const { +void VLASizeChecker::reportBug( + VLASize_Kind Kind, const Expr *SizeE, ProgramStateRef State, + CheckerContext &C, std::unique_ptr<BugReporterVisitor> Visitor) const { // Generate an error node. ExplodedNode *N = C.generateErrorNode(State); if (!N) @@ -73,6 +72,7 @@ void VLASizeChecker::reportBug(VLASize_Kind Kind, } auto report = llvm::make_unique<BugReport>(*BT, os.str(), N); + report->addVisitor(std::move(Visitor)); report->addRange(SizeE->getSourceRange()); bugreporter::trackNullOrUndefValue(N, SizeE, *report); C.emitReport(std::move(report)); @@ -94,7 +94,7 @@ void VLASizeChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const { // FIXME: Handle multi-dimensional VLAs. const Expr *SE = VLA->getSizeExpr(); ProgramStateRef state = C.getState(); - SVal sizeV = state->getSVal(SE, C.getLocationContext()); + SVal sizeV = C.getSVal(SE); if (sizeV.isUndef()) { reportBug(VLA_Garbage, SE, state, C); @@ -108,7 +108,8 @@ void VLASizeChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const { // Check if the size is tainted. if (state->isTainted(sizeV)) { - reportBug(VLA_Tainted, SE, nullptr, C); + reportBug(VLA_Tainted, SE, nullptr, C, + llvm::make_unique<TaintBugVisitor>(sizeV)); return; } diff --git a/lib/StaticAnalyzer/Checkers/ValistChecker.cpp b/lib/StaticAnalyzer/Checkers/ValistChecker.cpp index 06c4ef71d80b..bd657340fcfb 100644 --- a/lib/StaticAnalyzer/Checkers/ValistChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ValistChecker.cpp @@ -56,7 +56,6 @@ public: private: const MemRegion *getVAListAsRegion(SVal SV, const Expr *VAExpr, bool &IsSymbolic, CheckerContext &C) const; - StringRef getVariableNameFromRegion(const MemRegion *Reg) const; const ExplodedNode *getStartCallSite(const ExplodedNode *N, const MemRegion *Reg) const; @@ -64,13 +63,13 @@ private: CheckerContext &C) const; void reportLeakedVALists(const RegionVector &LeakedVALists, StringRef Msg1, StringRef Msg2, CheckerContext &C, ExplodedNode *N, - bool ForceReport = false) const; + bool ReportUninit = false) const; void checkVAListStartCall(const CallEvent &Call, CheckerContext &C, bool IsCopy) const; void checkVAListEndCall(const CallEvent &Call, CheckerContext &C) const; - class ValistBugVisitor : public BugReporterVisitorImpl<ValistBugVisitor> { + class ValistBugVisitor : public BugReporterVisitor { public: ValistBugVisitor(const MemRegion *Reg, bool IsLeak = false) : Reg(Reg), IsLeak(IsLeak) {} @@ -79,7 +78,7 @@ private: ID.AddPointer(&X); ID.AddPointer(Reg); } - std::unique_ptr<PathDiagnosticPiece> + std::shared_ptr<PathDiagnosticPiece> getEndPath(BugReporterContext &BRC, const ExplodedNode *EndPathNode, BugReport &BR) override { if (!IsLeak) @@ -88,8 +87,7 @@ private: PathDiagnosticLocation L = PathDiagnosticLocation::createEndOfPath( EndPathNode, BRC.getSourceManager()); // Do not add the statement itself as a range in case of leak. - return llvm::make_unique<PathDiagnosticEventPiece>(L, BR.getDescription(), - false); + return std::make_shared<PathDiagnosticEventPiece>(L, BR.getDescription(), false); } std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N, const ExplodedNode *PrevN, @@ -189,7 +187,7 @@ void ValistChecker::checkPreStmt(const VAArgExpr *VAA, CheckerContext &C) const { ProgramStateRef State = C.getState(); const Expr *VASubExpr = VAA->getSubExpr(); - SVal VAListSVal = State->getSVal(VASubExpr, C.getLocationContext()); + SVal VAListSVal = C.getSVal(VASubExpr); bool Symbolic; const MemRegion *VAList = getVAListAsRegion(VAListSVal, VASubExpr, Symbolic, C); @@ -267,15 +265,19 @@ void ValistChecker::reportUninitializedAccess(const MemRegion *VAList, void ValistChecker::reportLeakedVALists(const RegionVector &LeakedVALists, StringRef Msg1, StringRef Msg2, CheckerContext &C, ExplodedNode *N, - bool ForceReport) const { + bool ReportUninit) const { if (!(ChecksEnabled[CK_Unterminated] || - (ChecksEnabled[CK_Uninitialized] && ForceReport))) + (ChecksEnabled[CK_Uninitialized] && ReportUninit))) return; for (auto Reg : LeakedVALists) { if (!BT_leakedvalist) { - BT_leakedvalist.reset(new BugType(CheckNames[CK_Unterminated], - "Leaked va_list", - categories::MemoryError)); + // FIXME: maybe creating a new check name for this type of bug is a better + // solution. + BT_leakedvalist.reset( + new BugType(CheckNames[CK_Unterminated].getName().empty() + ? CheckNames[CK_Uninitialized] + : CheckNames[CK_Unterminated], + "Leaked va_list", categories::MemoryError)); BT_leakedvalist->setSuppressOnSink(true); } @@ -375,7 +377,7 @@ void ValistChecker::checkVAListEndCall(const CallEvent &Call, std::shared_ptr<PathDiagnosticPiece> ValistChecker::ValistBugVisitor::VisitNode( const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC, - BugReport &BR) { + BugReport &) { ProgramStateRef State = N->getState(); ProgramStateRef StatePrev = PrevN->getState(); diff --git a/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp b/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp index c5010f53785a..5b602468cdd4 100644 --- a/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp @@ -48,7 +48,7 @@ public: DefaultBool IsPureOnly; void checkBeginFunction(CheckerContext &C) const; - void checkEndFunction(CheckerContext &C) const; + void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const; void checkPreCall(const CallEvent &Call, CheckerContext &C) const; private: @@ -57,7 +57,7 @@ private: void reportBug(StringRef Msg, bool PureError, const MemRegion *Reg, CheckerContext &C) const; - class VirtualBugVisitor : public BugReporterVisitorImpl<VirtualBugVisitor> { + class VirtualBugVisitor : public BugReporterVisitor { private: const MemRegion *ObjectRegion; bool Found; @@ -108,7 +108,7 @@ VirtualCallChecker::VirtualBugVisitor::VisitNode(const ExplodedNode *N, if (!MD) return nullptr; auto ThiSVal = - State->getSVal(SVB.getCXXThis(MD, LCtx->getCurrentStackFrame())); + State->getSVal(SVB.getCXXThis(MD, LCtx->getStackFrame())); const MemRegion *Reg = ThiSVal.castAs<loc::MemRegionVal>().getRegion(); if (!Reg) return nullptr; @@ -167,7 +167,8 @@ void VirtualCallChecker::checkBeginFunction(CheckerContext &C) const { } // The EndFunction callback when leave a constructor or a destructor. -void VirtualCallChecker::checkEndFunction(CheckerContext &C) const { +void VirtualCallChecker::checkEndFunction(const ReturnStmt *RS, + CheckerContext &C) const { registerCtorDtorCallInState(false, C); } @@ -230,7 +231,7 @@ void VirtualCallChecker::registerCtorDtorCallInState(bool IsBeginFunction, // Enter a constructor, set the corresponding memregion be true. if (isa<CXXConstructorDecl>(MD)) { auto ThiSVal = - State->getSVal(SVB.getCXXThis(MD, LCtx->getCurrentStackFrame())); + State->getSVal(SVB.getCXXThis(MD, LCtx->getStackFrame())); const MemRegion *Reg = ThiSVal.getAsRegion(); if (IsBeginFunction) State = State->set<CtorDtorMap>(Reg, ObjectState::CtorCalled); @@ -244,7 +245,7 @@ void VirtualCallChecker::registerCtorDtorCallInState(bool IsBeginFunction, // Enter a Destructor, set the corresponding memregion be true. if (isa<CXXDestructorDecl>(MD)) { auto ThiSVal = - State->getSVal(SVB.getCXXThis(MD, LCtx->getCurrentStackFrame())); + State->getSVal(SVB.getCXXThis(MD, LCtx->getStackFrame())); const MemRegion *Reg = ThiSVal.getAsRegion(); if (IsBeginFunction) State = State->set<CtorDtorMap>(Reg, ObjectState::DtorCalled); |