diff options
Diffstat (limited to 'lib/StaticAnalyzer/Checkers')
44 files changed, 3578 insertions, 414 deletions
diff --git a/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp b/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp new file mode 100644 index 0000000000000..e6592a285e477 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp @@ -0,0 +1,68 @@ +//===- AnalysisOrderChecker - Print callbacks called ------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This checker prints callbacks that are called during analysis. +// This is required to ensure that callbacks are fired in order +// and do not duplicate or get lost. +// Feel free to extend this checker with any callback you need to check. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" + +using namespace clang; +using namespace ento; + +namespace { + +class AnalysisOrderChecker : public Checker< check::PreStmt<CastExpr>, + check::PostStmt<CastExpr>, + check::PreStmt<ArraySubscriptExpr>, + check::PostStmt<ArraySubscriptExpr>> { + bool isCallbackEnabled(CheckerContext &C, StringRef CallbackName) const { + AnalyzerOptions &Opts = C.getAnalysisManager().getAnalyzerOptions(); + return Opts.getBooleanOption("*", false, this) || + Opts.getBooleanOption(CallbackName, false, this); + } + +public: + void checkPreStmt(const CastExpr *CE, CheckerContext &C) const { + if (isCallbackEnabled(C, "PreStmtCastExpr")) + llvm::errs() << "PreStmt<CastExpr> (Kind : " << CE->getCastKindName() + << ")\n"; + } + + void checkPostStmt(const CastExpr *CE, CheckerContext &C) const { + if (isCallbackEnabled(C, "PostStmtCastExpr")) + llvm::errs() << "PostStmt<CastExpr> (Kind : " << CE->getCastKindName() + << ")\n"; + } + + void checkPreStmt(const ArraySubscriptExpr *SubExpr, CheckerContext &C) const { + if (isCallbackEnabled(C, "PreStmtArraySubscriptExpr")) + llvm::errs() << "PreStmt<ArraySubscriptExpr>\n"; + } + + void checkPostStmt(const ArraySubscriptExpr *SubExpr, CheckerContext &C) const { + if (isCallbackEnabled(C, "PostStmtArraySubscriptExpr")) + llvm::errs() << "PostStmt<ArraySubscriptExpr>\n"; + } +}; +} + +//===----------------------------------------------------------------------===// +// Registration. +//===----------------------------------------------------------------------===// + +void ento::registerAnalysisOrderChecker(CheckerManager &mgr) { + mgr.registerChecker<AnalysisOrderChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp b/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp index 13f0f655b89c4..848c2662019a1 100644 --- a/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp +++ b/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp @@ -17,6 +17,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/APSIntType.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h" #include "llvm/ADT/SmallString.h" @@ -67,18 +68,47 @@ public: static SVal computeExtentBegin(SValBuilder &svalBuilder, const MemRegion *region) { - while (true) - switch (region->getKind()) { + const MemSpaceRegion *SR = region->getMemorySpace(); + if (SR->getKind() == MemRegion::UnknownSpaceRegionKind) + return UnknownVal(); + else + return svalBuilder.makeZeroArrayIndex(); +} + +// TODO: once the constraint manager is smart enough to handle non simplified +// symbolic expressions remove this function. Note that this can not be used in +// the constraint manager as is, since this does not handle overflows. It is +// safe to assume, however, that memory offsets will not overflow. +static std::pair<NonLoc, nonloc::ConcreteInt> +getSimplifiedOffsets(NonLoc offset, nonloc::ConcreteInt extent, + SValBuilder &svalBuilder) { + Optional<nonloc::SymbolVal> SymVal = offset.getAs<nonloc::SymbolVal>(); + if (SymVal && SymVal->isExpression()) { + if (const SymIntExpr *SIE = dyn_cast<SymIntExpr>(SymVal->getSymbol())) { + llvm::APSInt constant = + APSIntType(extent.getValue()).convert(SIE->getRHS()); + switch (SIE->getOpcode()) { + case BO_Mul: + // The constant should never be 0 here, since it the result of scaling + // based on the size of a type which is never 0. + if ((extent.getValue() % constant) != 0) + return std::pair<NonLoc, nonloc::ConcreteInt>(offset, extent); + else + return getSimplifiedOffsets( + nonloc::SymbolVal(SIE->getLHS()), + svalBuilder.makeIntVal(extent.getValue() / constant), + svalBuilder); + case BO_Add: + return getSimplifiedOffsets( + nonloc::SymbolVal(SIE->getLHS()), + svalBuilder.makeIntVal(extent.getValue() - constant), svalBuilder); default: - return svalBuilder.makeZeroArrayIndex(); - case MemRegion::SymbolicRegionKind: - // FIXME: improve this later by tracking symbolic lower bounds - // for symbolic regions. - return UnknownVal(); - case MemRegion::ElementRegionKind: - region = cast<SubRegion>(region)->getSuperRegion(); - continue; + break; + } } + } + + return std::pair<NonLoc, nonloc::ConcreteInt>(offset, extent); } void ArrayBoundCheckerV2::checkLocation(SVal location, bool isLoad, @@ -104,6 +134,8 @@ void ArrayBoundCheckerV2::checkLocation(SVal location, bool isLoad, if (!rawOffset.getRegion()) return; + NonLoc rawOffsetVal = rawOffset.getByteOffset(); + // CHECK LOWER BOUND: Is byteOffset < extent begin? // If so, we are doing a load/store // before the first valid offset in the memory region. @@ -111,9 +143,17 @@ void ArrayBoundCheckerV2::checkLocation(SVal location, bool isLoad, SVal extentBegin = computeExtentBegin(svalBuilder, rawOffset.getRegion()); if (Optional<NonLoc> NV = extentBegin.getAs<NonLoc>()) { - SVal lowerBound = - svalBuilder.evalBinOpNN(state, BO_LT, rawOffset.getByteOffset(), *NV, - svalBuilder.getConditionType()); + if (NV->getAs<nonloc::ConcreteInt>()) { + std::pair<NonLoc, nonloc::ConcreteInt> simplifiedOffsets = + getSimplifiedOffsets(rawOffset.getByteOffset(), + NV->castAs<nonloc::ConcreteInt>(), + svalBuilder); + rawOffsetVal = simplifiedOffsets.first; + *NV = simplifiedOffsets.second; + } + + SVal lowerBound = svalBuilder.evalBinOpNN(state, BO_LT, rawOffsetVal, *NV, + svalBuilder.getConditionType()); Optional<NonLoc> lowerBoundToCheck = lowerBound.getAs<NonLoc>(); if (!lowerBoundToCheck) @@ -142,10 +182,18 @@ void ArrayBoundCheckerV2::checkLocation(SVal location, bool isLoad, if (!extentVal.getAs<NonLoc>()) break; - SVal upperbound - = svalBuilder.evalBinOpNN(state, BO_GE, rawOffset.getByteOffset(), - extentVal.castAs<NonLoc>(), - svalBuilder.getConditionType()); + if (extentVal.getAs<nonloc::ConcreteInt>()) { + std::pair<NonLoc, nonloc::ConcreteInt> simplifiedOffsets = + getSimplifiedOffsets(rawOffset.getByteOffset(), + extentVal.castAs<nonloc::ConcreteInt>(), + svalBuilder); + rawOffsetVal = simplifiedOffsets.first; + extentVal = simplifiedOffsets.second; + } + + SVal upperbound = svalBuilder.evalBinOpNN(state, BO_GE, rawOffsetVal, + extentVal.castAs<NonLoc>(), + svalBuilder.getConditionType()); Optional<NonLoc> upperboundToCheck = upperbound.getAs<NonLoc>(); if (!upperboundToCheck) @@ -157,13 +205,13 @@ 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())) + if (state->isTainted(rawOffset.getByteOffset())) { reportOOB(checkerContext, state_exceedsUpperBound, OOB_Tainted); return; - } - - // If we are constrained enough to definitely exceed the upper bound, report. - if (state_exceedsUpperBound) { + } + } else if (state_exceedsUpperBound) { + // If we are constrained enough to definitely exceed the upper bound, + // report. assert(!state_withinUpperBound); reportOOB(checkerContext, state_exceedsUpperBound, OOB_Excedes); return; diff --git a/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp b/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp index 6239c5507a4be..1ea85d60c9e92 100644 --- a/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp +++ b/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp @@ -336,15 +336,15 @@ void NilArgChecker::checkPostStmt(const ObjCDictionaryLiteral *DL, } //===----------------------------------------------------------------------===// -// Error reporting. +// Checking for mismatched types passed to CFNumberCreate/CFNumberGetValue. //===----------------------------------------------------------------------===// namespace { -class CFNumberCreateChecker : public Checker< check::PreStmt<CallExpr> > { +class CFNumberChecker : public Checker< check::PreStmt<CallExpr> > { mutable std::unique_ptr<APIMisuse> BT; - mutable IdentifierInfo* II; + mutable IdentifierInfo *ICreate, *IGetValue; public: - CFNumberCreateChecker() : II(nullptr) {} + CFNumberChecker() : ICreate(nullptr), IGetValue(nullptr) {} void checkPreStmt(const CallExpr *CE, CheckerContext &C) const; @@ -425,7 +425,7 @@ static const char* GetCFNumberTypeStr(uint64_t i) { } #endif -void CFNumberCreateChecker::checkPreStmt(const CallExpr *CE, +void CFNumberChecker::checkPreStmt(const CallExpr *CE, CheckerContext &C) const { ProgramStateRef state = C.getState(); const FunctionDecl *FD = C.getCalleeDecl(CE); @@ -433,10 +433,12 @@ void CFNumberCreateChecker::checkPreStmt(const CallExpr *CE, return; ASTContext &Ctx = C.getASTContext(); - if (!II) - II = &Ctx.Idents.get("CFNumberCreate"); - - if (FD->getIdentifier() != II || CE->getNumArgs() != 3) + if (!ICreate) { + ICreate = &Ctx.Idents.get("CFNumberCreate"); + IGetValue = &Ctx.Idents.get("CFNumberGetValue"); + } + if (!(FD->getIdentifier() == ICreate || FD->getIdentifier() == IGetValue) || + CE->getNumArgs() != 3) return; // Get the value of the "theType" argument. @@ -450,13 +452,13 @@ void CFNumberCreateChecker::checkPreStmt(const CallExpr *CE, return; uint64_t NumberKind = V->getValue().getLimitedValue(); - Optional<uint64_t> OptTargetSize = GetCFNumberSize(Ctx, NumberKind); + Optional<uint64_t> OptCFNumberSize = GetCFNumberSize(Ctx, NumberKind); // FIXME: In some cases we can emit an error. - if (!OptTargetSize) + if (!OptCFNumberSize) return; - uint64_t TargetSize = *OptTargetSize; + uint64_t CFNumberSize = *OptCFNumberSize; // 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 @@ -481,39 +483,44 @@ void CFNumberCreateChecker::checkPreStmt(const CallExpr *CE, if (!T->isIntegralOrEnumerationType()) return; - uint64_t SourceSize = Ctx.getTypeSize(T); + uint64_t PrimitiveTypeSize = Ctx.getTypeSize(T); - // CHECK: is SourceSize == TargetSize - if (SourceSize == TargetSize) + if (PrimitiveTypeSize == CFNumberSize) return; - // Generate an error. Only generate a sink error node - // if 'SourceSize < TargetSize'; otherwise generate a non-fatal error node. - // // FIXME: We can actually create an abstract "CFNumber" object that has // the bits initialized to the provided values. - // - ExplodedNode *N = SourceSize < TargetSize ? C.generateErrorNode() - : C.generateNonFatalErrorNode(); + ExplodedNode *N = C.generateNonFatalErrorNode(); if (N) { SmallString<128> sbuf; llvm::raw_svector_ostream os(sbuf); + bool isCreate = (FD->getIdentifier() == ICreate); + + if (isCreate) { + os << (PrimitiveTypeSize == 8 ? "An " : "A ") + << PrimitiveTypeSize << "-bit integer is used to initialize a " + << "CFNumber object that represents " + << (CFNumberSize == 8 ? "an " : "a ") + << CFNumberSize << "-bit integer; "; + } else { + os << "A CFNumber object that represents " + << (CFNumberSize == 8 ? "an " : "a ") + << CFNumberSize << "-bit integer is used to initialize " + << (PrimitiveTypeSize == 8 ? "an " : "a ") + << PrimitiveTypeSize << "-bit integer; "; + } - os << (SourceSize == 8 ? "An " : "A ") - << SourceSize << " bit integer is used to initialize a CFNumber " - "object that represents " - << (TargetSize == 8 ? "an " : "a ") - << TargetSize << " bit integer. "; - - if (SourceSize < TargetSize) - os << (TargetSize - SourceSize) - << " bits of the CFNumber value will be garbage." ; + if (PrimitiveTypeSize < CFNumberSize) + os << (CFNumberSize - PrimitiveTypeSize) + << " bits of the CFNumber value will " + << (isCreate ? "be garbage." : "overwrite adjacent storage."); else - os << (SourceSize - TargetSize) - << " bits of the input integer will be lost."; + os << (PrimitiveTypeSize - CFNumberSize) + << " bits of the integer value will be " + << (isCreate ? "lost." : "garbage."); if (!BT) - BT.reset(new APIMisuse(this, "Bad use of CFNumberCreate")); + BT.reset(new APIMisuse(this, "Bad use of CFNumber APIs")); auto report = llvm::make_unique<BugReport>(*BT, os.str(), N); report->addRange(CE->getArg(2)->getSourceRange()); @@ -1272,8 +1279,8 @@ void ento::registerNilArgChecker(CheckerManager &mgr) { mgr.registerChecker<NilArgChecker>(); } -void ento::registerCFNumberCreateChecker(CheckerManager &mgr) { - mgr.registerChecker<CFNumberCreateChecker>(); +void ento::registerCFNumberChecker(CheckerManager &mgr) { + mgr.registerChecker<CFNumberChecker>(); } void ento::registerCFRetainReleaseChecker(CheckerManager &mgr) { diff --git a/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp b/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp new file mode 100644 index 0000000000000..082a4873217b7 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp @@ -0,0 +1,109 @@ +//===-- BlockInCriticalSectionChecker.cpp -----------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// Defines a checker for blocks in critical sections. This checker should find +// the calls to blocking functions (for example: sleep, getc, fgets, read, +// recv etc.) inside a critical section. When sleep(x) is called while a mutex +// is held, other threades cannot lock the same mutex. This might take some +// time, leading to bad performance or even deadlock. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.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; + +namespace { + +class BlockInCriticalSectionChecker : public Checker<check::PostCall, + check::PreCall> { + + CallDescription LockFn, UnlockFn, SleepFn, GetcFn, FgetsFn, ReadFn, RecvFn; + + std::unique_ptr<BugType> BlockInCritSectionBugType; + + void reportBlockInCritSection(SymbolRef FileDescSym, + const CallEvent &call, + CheckerContext &C) const; + +public: + BlockInCriticalSectionChecker(); + + void checkPreCall(const CallEvent &Call, CheckerContext &C) const; + + /// Process unlock. + /// Process lock. + /// Process blocking functions (sleep, getc, fgets, read, recv) + void checkPostCall(const CallEvent &Call, CheckerContext &C) const; + +}; + +} // end anonymous namespace + +REGISTER_TRAIT_WITH_PROGRAMSTATE(MutexCounter, unsigned) + +BlockInCriticalSectionChecker::BlockInCriticalSectionChecker() + : LockFn("lock"), UnlockFn("unlock"), SleepFn("sleep"), GetcFn("getc"), + FgetsFn("fgets"), ReadFn("read"), RecvFn("recv") { + // Initialize the bug type. + BlockInCritSectionBugType.reset( + new BugType(this, "Call to blocking function in critical section", + "Blocking Error")); +} + +void BlockInCriticalSectionChecker::checkPreCall(const CallEvent &Call, + CheckerContext &C) const { +} + +void BlockInCriticalSectionChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + if (!Call.isCalled(LockFn) + && !Call.isCalled(SleepFn) + && !Call.isCalled(GetcFn) + && !Call.isCalled(FgetsFn) + && !Call.isCalled(ReadFn) + && !Call.isCalled(RecvFn) + && !Call.isCalled(UnlockFn)) + return; + + ProgramStateRef State = C.getState(); + unsigned mutexCount = State->get<MutexCounter>(); + if (Call.isCalled(UnlockFn) && mutexCount > 0) { + State = State->set<MutexCounter>(--mutexCount); + C.addTransition(State); + } else if (Call.isCalled(LockFn)) { + State = State->set<MutexCounter>(++mutexCount); + C.addTransition(State); + } else if (mutexCount > 0) { + SymbolRef BlockDesc = Call.getReturnValue().getAsSymbol(); + reportBlockInCritSection(BlockDesc, Call, C); + } +} + +void BlockInCriticalSectionChecker::reportBlockInCritSection( + SymbolRef BlockDescSym, const CallEvent &Call, CheckerContext &C) const { + ExplodedNode *ErrNode = C.generateNonFatalErrorNode(); + if (!ErrNode) + return; + + auto R = llvm::make_unique<BugReport>(*BlockInCritSectionBugType, + "A blocking function %s is called inside a critical section.", ErrNode); + R->addRange(Call.getSourceRange()); + R->markInteresting(BlockDescSym); + C.emitReport(std::move(R)); +} + +void ento::registerBlockInCriticalSectionChecker(CheckerManager &mgr) { + mgr.registerChecker<BlockInCriticalSectionChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp b/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp index dab2f61229a0a..8c2aef21b3cab 100644 --- a/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp @@ -55,6 +55,7 @@ bool BuiltinFunctionChecker::evalCall(const CallExpr *CE, return true; } + case Builtin::BI__builtin_alloca_with_align: case Builtin::BI__builtin_alloca: { // FIXME: Refactor into StoreManager itself? MemRegionManager& RM = C.getStoreManager().getRegionManager(); diff --git a/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 62ccc3cb4970b..41415f0376c00 100644 --- a/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -4,10 +4,12 @@ set(LLVM_LINK_COMPONENTS add_clang_library(clangStaticAnalyzerCheckers AllocationDiagnostics.cpp + AnalysisOrderChecker.cpp AnalyzerStatsChecker.cpp ArrayBoundChecker.cpp ArrayBoundCheckerV2.cpp BasicObjCFoundationChecks.cpp + BlockInCriticalSectionChecker.cpp BoolAssignmentChecker.cpp BuiltinFunctionChecker.cpp CStringChecker.cpp @@ -22,6 +24,9 @@ add_clang_library(clangStaticAnalyzerCheckers CheckerDocumentation.cpp ChrootChecker.cpp ClangCheckers.cpp + CloneChecker.cpp + ConversionChecker.cpp + CXXSelfAssignmentChecker.cpp DeadStoresChecker.cpp DebugCheckers.cpp DereferenceChecker.cpp @@ -32,6 +37,7 @@ add_clang_library(clangStaticAnalyzerCheckers ExprInspectionChecker.cpp FixedAddressChecker.cpp GenericTaintChecker.cpp + GTestChecker.cpp IdenticalExprChecker.cpp IvarInvalidationChecker.cpp LLVMConventionsChecker.cpp @@ -49,10 +55,12 @@ add_clang_library(clangStaticAnalyzerCheckers NoReturnFunctionChecker.cpp NonNullParamChecker.cpp NullabilityChecker.cpp + NumberObjectConversionChecker.cpp ObjCAtSyncChecker.cpp ObjCContainersASTChecker.cpp ObjCContainersChecker.cpp ObjCMissingSuperCallChecker.cpp + ObjCPropertyChecker.cpp ObjCSelfInitChecker.cpp ObjCSuperDeallocChecker.cpp ObjCUnusedIVarsChecker.cpp @@ -65,6 +73,7 @@ add_clang_library(clangStaticAnalyzerCheckers ReturnUndefChecker.cpp SimpleStreamChecker.cpp StackAddrEscapeChecker.cpp + StdLibraryFunctionsChecker.cpp StreamChecker.cpp TaintTesterChecker.cpp TestAfterDivZeroChecker.cpp @@ -78,6 +87,7 @@ add_clang_library(clangStaticAnalyzerCheckers UnreachableCodeChecker.cpp VforkChecker.cpp VLASizeChecker.cpp + ValistChecker.cpp VirtualCallChecker.cpp DEPENDS @@ -85,6 +95,7 @@ add_clang_library(clangStaticAnalyzerCheckers LINK_LIBS clangAST + clangASTMatchers clangAnalysis clangBasic clangLex diff --git a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index e9512977fa6d1..238032c895f68 100644 --- a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -22,7 +22,6 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallString.h" -#include "llvm/ADT/StringSwitch.h" #include "llvm/Support/raw_ostream.h" using namespace clang; @@ -63,7 +62,6 @@ public: void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const; void checkLiveSymbols(ProgramStateRef state, SymbolReaper &SR) const; void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const; - bool wantsRegionChangeUpdate(ProgramStateRef state) const; ProgramStateRef checkRegionChanges(ProgramStateRef state, @@ -686,6 +684,7 @@ SVal CStringChecker::getCStringLengthForRegion(CheckerContext &C, QualType sizeTy = svalBuilder.getContext().getSizeType(); SVal strLength = svalBuilder.getMetadataSymbolVal(CStringChecker::getTag(), MR, Ex, sizeTy, + C.getLocationContext(), C.blockCount()); if (!hypothetical) { @@ -2112,11 +2111,6 @@ void CStringChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const { C.addTransition(state); } -bool CStringChecker::wantsRegionChangeUpdate(ProgramStateRef state) const { - CStringLengthTy Entries = state->get<CStringLength>(); - return !Entries.isEmpty(); -} - ProgramStateRef CStringChecker::checkRegionChanges(ProgramStateRef state, const InvalidatedSymbols *, diff --git a/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp b/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp new file mode 100644 index 0000000000000..7631322d255b4 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/CXXSelfAssignmentChecker.cpp @@ -0,0 +1,62 @@ +//=== CXXSelfAssignmentChecker.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 CXXSelfAssignmentChecker, which tests all custom defined +// copy and move assignment operators for the case of self assignment, thus +// where the parameter refers to the same location where the this pointer +// points to. The checker itself does not do any checks at all, but it +// causes the analyzer to check every copy and move assignment operator twice: +// once for when 'this' aliases with the parameter and once for when it may not. +// It is the task of the other enabled checkers to find the bugs in these two +// different cases. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" + +using namespace clang; +using namespace ento; + +namespace { + +class CXXSelfAssignmentChecker : public Checker<check::BeginFunction> { +public: + CXXSelfAssignmentChecker(); + void checkBeginFunction(CheckerContext &C) const; +}; +} + +CXXSelfAssignmentChecker::CXXSelfAssignmentChecker() {} + +void CXXSelfAssignmentChecker::checkBeginFunction(CheckerContext &C) const { + if (!C.inTopFrame()) + return; + const auto *LCtx = C.getLocationContext(); + const auto *MD = dyn_cast<CXXMethodDecl>(LCtx->getDecl()); + if (!MD) + return; + if (!MD->isCopyAssignmentOperator() && !MD->isMoveAssignmentOperator()) + return; + auto &State = C.getState(); + auto &SVB = C.getSValBuilder(); + auto ThisVal = + State->getSVal(SVB.getCXXThis(MD, LCtx->getCurrentStackFrame())); + auto Param = SVB.makeLoc(State->getRegion(MD->getParamDecl(0), LCtx)); + auto ParamVal = State->getSVal(Param); + ProgramStateRef SelfAssignState = State->bindLoc(Param, ThisVal); + C.addTransition(SelfAssignState); + ProgramStateRef NonSelfAssignState = State->bindLoc(Param, ParamVal); + C.addTransition(NonSelfAssignState); +} + +void ento::registerCXXSelfAssignmentChecker(CheckerManager &Mgr) { + Mgr.registerChecker<CXXSelfAssignmentChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp b/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp index 5126716fcded1..f474857a1bf43 100644 --- a/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp @@ -356,7 +356,6 @@ void CallAndMessageChecker::checkPreStmt(const CXXDeleteExpr *DE, } } - void CallAndMessageChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { ProgramStateRef State = C.getState(); @@ -389,11 +388,10 @@ void CallAndMessageChecker::checkPreCall(const CallEvent &Call, } const Decl *D = Call.getDecl(); - const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D); - if (FD) { - // If we have a declaration, we can make sure we pass enough parameters to - // the function. - unsigned Params = FD->getNumParams(); + if (D && (isa<FunctionDecl>(D) || isa<BlockDecl>(D))) { + // If we have a function or block declaration, we can make sure we pass + // enough parameters. + unsigned Params = Call.parameters().size(); if (Call.getNumArgs() < Params) { ExplodedNode *N = C.generateErrorNode(); if (!N) @@ -403,8 +401,14 @@ void CallAndMessageChecker::checkPreCall(const CallEvent &Call, SmallString<512> Str; llvm::raw_svector_ostream os(Str); - os << "Function taking " << Params << " argument" - << (Params == 1 ? "" : "s") << " is called with less (" + if (isa<FunctionDecl>(D)) { + os << "Function "; + } else { + assert(isa<BlockDecl>(D)); + os << "Block "; + } + os << "taking " << Params << " argument" + << (Params == 1 ? "" : "s") << " is called with fewer (" << Call.getNumArgs() << ")"; C.emitReport( @@ -425,6 +429,7 @@ void CallAndMessageChecker::checkPreCall(const CallEvent &Call, else BT = &BT_call_arg; + const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D); for (unsigned i = 0, e = Call.getNumArgs(); i != e; ++i) { const ParmVarDecl *ParamDecl = nullptr; if(FD && i < FD->getNumParams()) diff --git a/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp b/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp index 2337400750c71..3e178152d9251 100644 --- a/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp @@ -140,5 +140,10 @@ void CastSizeChecker::checkPreStmt(const CastExpr *CE,CheckerContext &C) const { } void ento::registerCastSizeChecker(CheckerManager &mgr) { - mgr.registerChecker<CastSizeChecker>(); + // PR31226: C++ is more complicated than what this checker currently supports. + // There are derived-to-base casts, there are different rules for 0-size + // structures, no flexible arrays, etc. + // FIXME: Disabled on C++ for now. + if (!mgr.getLangOpts().CPlusPlus) + mgr.registerChecker<CastSizeChecker>(); } diff --git a/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp b/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp index fa7841356efbd..16a475ae9dd21 100644 --- a/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CastToStructChecker.cpp @@ -1,4 +1,4 @@ -//=== CastToStructChecker.cpp - Fixed address usage checker ----*- C++ -*--===// +//=== CastToStructChecker.cpp ----------------------------------*- C++ -*--===// // // The LLVM Compiler Infrastructure // @@ -8,12 +8,13 @@ //===----------------------------------------------------------------------===// // // This files defines CastToStructChecker, a builtin checker that checks for -// cast from non-struct pointer to struct pointer. +// cast from non-struct pointer to struct pointer and widening struct data cast. // This check corresponds to CWE-588. // //===----------------------------------------------------------------------===// #include "ClangSACheckers.h" +#include "clang/AST/RecursiveASTVisitor.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/CheckerManager.h" @@ -23,18 +24,22 @@ using namespace clang; using namespace ento; namespace { -class CastToStructChecker : public Checker< check::PreStmt<CastExpr> > { - mutable std::unique_ptr<BuiltinBug> BT; +class CastToStructVisitor : public RecursiveASTVisitor<CastToStructVisitor> { + BugReporter &BR; + const CheckerBase *Checker; + AnalysisDeclContext *AC; public: - void checkPreStmt(const CastExpr *CE, CheckerContext &C) const; + explicit CastToStructVisitor(BugReporter &B, const CheckerBase *Checker, + AnalysisDeclContext *A) + : BR(B), Checker(Checker), AC(A) {} + bool VisitCastExpr(const CastExpr *CE); }; } -void CastToStructChecker::checkPreStmt(const CastExpr *CE, - CheckerContext &C) const { +bool CastToStructVisitor::VisitCastExpr(const CastExpr *CE) { const Expr *E = CE->getSubExpr(); - ASTContext &Ctx = C.getASTContext(); + ASTContext &Ctx = AC->getASTContext(); QualType OrigTy = Ctx.getCanonicalType(E->getType()); QualType ToTy = Ctx.getCanonicalType(CE->getType()); @@ -42,34 +47,72 @@ void CastToStructChecker::checkPreStmt(const CastExpr *CE, const PointerType *ToPTy = dyn_cast<PointerType>(ToTy.getTypePtr()); if (!ToPTy || !OrigPTy) - return; + return true; QualType OrigPointeeTy = OrigPTy->getPointeeType(); QualType ToPointeeTy = ToPTy->getPointeeType(); if (!ToPointeeTy->isStructureOrClassType()) - return; + return true; // We allow cast from void*. if (OrigPointeeTy->isVoidType()) - return; + return true; // Now the cast-to-type is struct pointer, the original type is not void*. if (!OrigPointeeTy->isRecordType()) { - if (ExplodedNode *N = C.generateNonFatalErrorNode()) { - if (!BT) - BT.reset( - new BuiltinBug(this, "Cast from non-struct type to struct type", - "Casting a non-structure type to a structure type " - "and accessing a field can lead to memory access " - "errors or data corruption.")); - auto R = llvm::make_unique<BugReport>(*BT, BT->getDescription(), N); - R->addRange(CE->getSourceRange()); - C.emitReport(std::move(R)); - } + SourceRange Sr[1] = {CE->getSourceRange()}; + PathDiagnosticLocation Loc(CE, BR.getSourceManager(), AC); + BR.EmitBasicReport( + AC->getDecl(), Checker, "Cast from non-struct type to struct type", + categories::LogicError, "Casting a non-structure type to a structure " + "type and accessing a field can lead to memory " + "access errors or data corruption.", + Loc, Sr); + } else { + // Don't warn when size of data is unknown. + const auto *U = dyn_cast<UnaryOperator>(E); + if (!U || U->getOpcode() != UO_AddrOf) + return true; + + // Don't warn for references + const ValueDecl *VD = nullptr; + if (const auto *SE = dyn_cast<DeclRefExpr>(U->getSubExpr())) + VD = dyn_cast<ValueDecl>(SE->getDecl()); + else if (const auto *SE = dyn_cast<MemberExpr>(U->getSubExpr())) + VD = SE->getMemberDecl(); + if (!VD || VD->getType()->isReferenceType()) + return true; + + // Warn when there is widening cast. + unsigned ToWidth = Ctx.getTypeInfo(ToPointeeTy).Width; + unsigned OrigWidth = Ctx.getTypeInfo(OrigPointeeTy).Width; + if (ToWidth <= OrigWidth) + return true; + + PathDiagnosticLocation Loc(CE, BR.getSourceManager(), AC); + BR.EmitBasicReport(AC->getDecl(), Checker, "Widening cast to struct type", + categories::LogicError, + "Casting data to a larger structure type and accessing " + "a field can lead to memory access errors or data " + "corruption.", + Loc, CE->getSourceRange()); } + + return true; } +namespace { +class CastToStructChecker : public Checker<check::ASTCodeBody> { +public: + void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr, + BugReporter &BR) const { + CastToStructVisitor Visitor(BR, this, Mgr.getAnalysisDeclContext(D)); + Visitor.TraverseDecl(const_cast<Decl *>(D)); + } +}; +} // end anonymous namespace + void ento::registerCastToStructChecker(CheckerManager &mgr) { mgr.registerChecker<CastToStructChecker>(); } diff --git a/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp b/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp index 9e863e79e41fc..2818c9d9fd4ae 100644 --- a/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp +++ b/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp @@ -34,6 +34,7 @@ #include "clang/AST/Expr.h" #include "clang/AST/ExprObjC.h" #include "clang/Basic/LangOptions.h" +#include "clang/Basic/TargetInfo.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h" @@ -173,6 +174,7 @@ private: bool classHasSeparateTeardown(const ObjCInterfaceDecl *ID) const; bool isReleasedByCIFilterDealloc(const ObjCPropertyImplDecl *PropImpl) const; + bool isNibLoadedIvarWithoutRetain(const ObjCPropertyImplDecl *PropImpl) const; }; } // End anonymous namespace. @@ -525,7 +527,7 @@ void ObjCDeallocChecker::diagnoseMissingReleases(CheckerContext &C) const { if (SelfRegion != IvarRegion->getSuperRegion()) continue; - const ObjCIvarDecl *IvarDecl = IvarRegion->getDecl(); + const ObjCIvarDecl *IvarDecl = IvarRegion->getDecl(); // Prevent an inlined call to -dealloc in a super class from warning // about the values the subclass's -dealloc should release. if (IvarDecl->getContainingInterface() != @@ -903,6 +905,9 @@ ReleaseRequirement ObjCDeallocChecker::getDeallocReleaseRequirement( if (isReleasedByCIFilterDealloc(PropImpl)) return ReleaseRequirement::MustNotReleaseDirectly; + if (isNibLoadedIvarWithoutRetain(PropImpl)) + return ReleaseRequirement::Unknown; + return ReleaseRequirement::MustRelease; case ObjCPropertyDecl::Weak: @@ -1059,6 +1064,32 @@ bool ObjCDeallocChecker::isReleasedByCIFilterDealloc( return false; } +/// Returns whether the ivar backing the property is an IBOutlet that +/// has its value set by nib loading code without retaining the value. +/// +/// On macOS, if there is no setter, the nib-loading code sets the ivar +/// directly, without retaining the value, +/// +/// On iOS and its derivatives, the nib-loading code will call +/// -setValue:forKey:, which retains the value before directly setting the ivar. +bool ObjCDeallocChecker::isNibLoadedIvarWithoutRetain( + const ObjCPropertyImplDecl *PropImpl) const { + const ObjCIvarDecl *IvarDecl = PropImpl->getPropertyIvarDecl(); + if (!IvarDecl->hasAttr<IBOutletAttr>()) + return false; + + const llvm::Triple &Target = + IvarDecl->getASTContext().getTargetInfo().getTriple(); + + if (!Target.isMacOSX()) + return false; + + if (PropImpl->getPropertyDecl()->getSetterMethodDecl()) + return false; + + return true; +} + void ento::registerObjCDeallocChecker(CheckerManager &Mgr) { const LangOptions &LangOpts = Mgr.getLangOpts(); // These checker only makes sense under MRR. diff --git a/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp b/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp index 74d05e27e8eb0..86764c939dcdc 100644 --- a/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp +++ b/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp @@ -45,6 +45,7 @@ class CheckerDocumentation : public Checker< check::PreStmt<ReturnStmt>, check::Location, check::Bind, check::DeadSymbols, + check::BeginFunction, check::EndFunction, check::EndAnalysis, check::EndOfTranslationUnit, diff --git a/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp b/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp index 14587fb5163b3..9e9939ae25c09 100644 --- a/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp @@ -19,7 +19,6 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h" -#include "llvm/ADT/ImmutableMap.h" using namespace clang; using namespace ento; diff --git a/lib/StaticAnalyzer/Checkers/CloneChecker.cpp b/lib/StaticAnalyzer/Checkers/CloneChecker.cpp new file mode 100644 index 0000000000000..6fa5732d10cba --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/CloneChecker.cpp @@ -0,0 +1,161 @@ +//===--- CloneChecker.cpp - Clone detection checker -------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +/// +/// \file +/// CloneChecker is a checker that reports clones in the current translation +/// unit. +/// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/Analysis/CloneDetection.h" +#include "clang/Basic/Diagnostic.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/AnalysisManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" + +using namespace clang; +using namespace ento; + +namespace { +class CloneChecker + : public Checker<check::ASTCodeBody, check::EndOfTranslationUnit> { + mutable CloneDetector Detector; + mutable std::unique_ptr<BugType> BT_Exact, BT_Suspicious; + +public: + void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr, + BugReporter &BR) const; + + void checkEndOfTranslationUnit(const TranslationUnitDecl *TU, + AnalysisManager &Mgr, BugReporter &BR) const; + + /// \brief Reports all clones to the user. + void reportClones(BugReporter &BR, AnalysisManager &Mgr, + int MinComplexity) const; + + /// \brief Reports only suspicious clones to the user along with informaton + /// that explain why they are suspicious. + void reportSuspiciousClones(BugReporter &BR, AnalysisManager &Mgr, + int MinComplexity) const; +}; +} // end anonymous namespace + +void CloneChecker::checkASTCodeBody(const Decl *D, AnalysisManager &Mgr, + BugReporter &BR) const { + // Every statement that should be included in the search for clones needs to + // be passed to the CloneDetector. + Detector.analyzeCodeBody(D); +} + +void CloneChecker::checkEndOfTranslationUnit(const TranslationUnitDecl *TU, + AnalysisManager &Mgr, + BugReporter &BR) const { + // At this point, every statement in the translation unit has been analyzed by + // the CloneDetector. The only thing left to do is to report the found clones. + + int MinComplexity = Mgr.getAnalyzerOptions().getOptionAsInteger( + "MinimumCloneComplexity", 10, this); + assert(MinComplexity >= 0); + + bool ReportSuspiciousClones = Mgr.getAnalyzerOptions().getBooleanOption( + "ReportSuspiciousClones", true, this); + + bool ReportNormalClones = Mgr.getAnalyzerOptions().getBooleanOption( + "ReportNormalClones", true, this); + + if (ReportSuspiciousClones) + reportSuspiciousClones(BR, Mgr, MinComplexity); + + if (ReportNormalClones) + reportClones(BR, Mgr, MinComplexity); +} + +static PathDiagnosticLocation makeLocation(const StmtSequence &S, + AnalysisManager &Mgr) { + ASTContext &ACtx = Mgr.getASTContext(); + return PathDiagnosticLocation::createBegin( + S.front(), ACtx.getSourceManager(), + Mgr.getAnalysisDeclContext(ACtx.getTranslationUnitDecl())); +} + +void CloneChecker::reportClones(BugReporter &BR, AnalysisManager &Mgr, + int MinComplexity) const { + + std::vector<CloneDetector::CloneGroup> CloneGroups; + Detector.findClones(CloneGroups, MinComplexity); + + if (!BT_Exact) + BT_Exact.reset(new BugType(this, "Exact code clone", "Code clone")); + + for (CloneDetector::CloneGroup &Group : CloneGroups) { + // We group the clones by printing the first as a warning and all others + // as a note. + auto R = llvm::make_unique<BugReport>( + *BT_Exact, "Duplicate code detected", + makeLocation(Group.Sequences.front(), Mgr)); + R->addRange(Group.Sequences.front().getSourceRange()); + + for (unsigned i = 1; i < Group.Sequences.size(); ++i) + R->addNote("Similar code here", + makeLocation(Group.Sequences[i], Mgr), + Group.Sequences[i].getSourceRange()); + BR.emitReport(std::move(R)); + } +} + +void CloneChecker::reportSuspiciousClones(BugReporter &BR, + AnalysisManager &Mgr, + int MinComplexity) const { + + std::vector<CloneDetector::SuspiciousClonePair> Clones; + Detector.findSuspiciousClones(Clones, MinComplexity); + + if (!BT_Suspicious) + BT_Suspicious.reset( + new BugType(this, "Suspicious code clone", "Code clone")); + + ASTContext &ACtx = BR.getContext(); + SourceManager &SM = ACtx.getSourceManager(); + AnalysisDeclContext *ADC = + Mgr.getAnalysisDeclContext(ACtx.getTranslationUnitDecl()); + + for (CloneDetector::SuspiciousClonePair &Pair : Clones) { + // FIXME: We are ignoring the suggestions currently, because they are + // only 50% accurate (even if the second suggestion is unavailable), + // which may confuse the user. + // Think how to perform more accurate suggestions? + + auto R = llvm::make_unique<BugReport>( + *BT_Suspicious, + "Potential copy-paste error; did you really mean to use '" + + Pair.FirstCloneInfo.Variable->getNameAsString() + "' here?", + PathDiagnosticLocation::createBegin(Pair.FirstCloneInfo.Mention, SM, + ADC)); + R->addRange(Pair.FirstCloneInfo.Mention->getSourceRange()); + + R->addNote("Similar code using '" + + Pair.SecondCloneInfo.Variable->getNameAsString() + "' here", + PathDiagnosticLocation::createBegin(Pair.SecondCloneInfo.Mention, + SM, ADC), + Pair.SecondCloneInfo.Mention->getSourceRange()); + + BR.emitReport(std::move(R)); + } +} + +//===----------------------------------------------------------------------===// +// Register CloneChecker +//===----------------------------------------------------------------------===// + +void ento::registerCloneChecker(CheckerManager &Mgr) { + Mgr.registerChecker<CloneChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp b/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp new file mode 100644 index 0000000000000..2bb9e858731c0 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp @@ -0,0 +1,192 @@ +//=== ConversionChecker.cpp -------------------------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// Check that there is no loss of sign/precision in assignments, comparisons +// and multiplications. +// +// ConversionChecker uses path sensitive analysis to determine possible values +// of expressions. A warning is reported when: +// * a negative value is implicitly converted to an unsigned value in an +// assignment, comparison or multiplication. +// * assignment / initialization when source value is greater than the max +// value of target +// +// Many compilers and tools have similar checks that are based on semantic +// analysis. Those checks are sound but have poor precision. ConversionChecker +// is an alternative to those checks. +// +//===----------------------------------------------------------------------===// +#include "ClangSACheckers.h" +#include "clang/AST/ParentMap.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" + +using namespace clang; +using namespace ento; + +namespace { +class ConversionChecker : public Checker<check::PreStmt<ImplicitCastExpr>> { +public: + void checkPreStmt(const ImplicitCastExpr *Cast, CheckerContext &C) const; + +private: + mutable std::unique_ptr<BuiltinBug> BT; + + // Is there loss of precision + bool isLossOfPrecision(const ImplicitCastExpr *Cast, CheckerContext &C) const; + + // Is there loss of sign + bool isLossOfSign(const ImplicitCastExpr *Cast, CheckerContext &C) const; + + void reportBug(ExplodedNode *N, CheckerContext &C, const char Msg[]) const; +}; +} + +void ConversionChecker::checkPreStmt(const ImplicitCastExpr *Cast, + CheckerContext &C) const { + // TODO: For now we only warn about DeclRefExpr, to avoid noise. Warn for + // calculations also. + if (!isa<DeclRefExpr>(Cast->IgnoreParenImpCasts())) + return; + + // Don't warn for loss of sign/precision in macros. + if (Cast->getExprLoc().isMacroID()) + return; + + // Get Parent. + const ParentMap &PM = C.getLocationContext()->getParentMap(); + const Stmt *Parent = PM.getParent(Cast); + if (!Parent) + return; + + bool LossOfSign = false; + bool LossOfPrecision = false; + + // Loss of sign/precision in binary operation. + if (const auto *B = dyn_cast<BinaryOperator>(Parent)) { + BinaryOperator::Opcode Opc = B->getOpcode(); + if (Opc == BO_Assign || Opc == BO_AddAssign || Opc == BO_SubAssign || + Opc == BO_MulAssign) { + LossOfSign = isLossOfSign(Cast, C); + LossOfPrecision = isLossOfPrecision(Cast, C); + } else if (B->isRelationalOp() || B->isMultiplicativeOp()) { + LossOfSign = isLossOfSign(Cast, C); + } + } else if (isa<DeclStmt>(Parent)) { + LossOfSign = isLossOfSign(Cast, C); + LossOfPrecision = isLossOfPrecision(Cast, C); + } + + if (LossOfSign || LossOfPrecision) { + // Generate an error node. + ExplodedNode *N = C.generateNonFatalErrorNode(C.getState()); + if (!N) + return; + if (LossOfSign) + reportBug(N, C, "Loss of sign in implicit conversion"); + if (LossOfPrecision) + reportBug(N, C, "Loss of precision in implicit conversion"); + } +} + +void ConversionChecker::reportBug(ExplodedNode *N, CheckerContext &C, + const char Msg[]) const { + if (!BT) + BT.reset( + new BuiltinBug(this, "Conversion", "Possible loss of sign/precision.")); + + // Generate a report for this bug. + auto R = llvm::make_unique<BugReport>(*BT, Msg, N); + C.emitReport(std::move(R)); +} + +// Is E value greater or equal than Val? +static bool isGreaterEqual(CheckerContext &C, const Expr *E, + unsigned long long Val) { + ProgramStateRef State = C.getState(); + SVal EVal = C.getSVal(E); + if (EVal.isUnknownOrUndef() || !EVal.getAs<NonLoc>()) + return false; + + SValBuilder &Bldr = C.getSValBuilder(); + DefinedSVal V = Bldr.makeIntVal(Val, C.getASTContext().LongLongTy); + + // Is DefinedEVal greater or equal with V? + SVal GE = Bldr.evalBinOp(State, BO_GE, EVal, V, Bldr.getConditionType()); + if (GE.isUnknownOrUndef()) + return false; + ConstraintManager &CM = C.getConstraintManager(); + ProgramStateRef StGE, StLT; + std::tie(StGE, StLT) = CM.assumeDual(State, GE.castAs<DefinedSVal>()); + return StGE && !StLT; +} + +// Is E value negative? +static bool isNegative(CheckerContext &C, const Expr *E) { + ProgramStateRef State = C.getState(); + SVal EVal = State->getSVal(E, C.getLocationContext()); + if (EVal.isUnknownOrUndef() || !EVal.getAs<NonLoc>()) + return false; + DefinedSVal DefinedEVal = EVal.castAs<DefinedSVal>(); + + SValBuilder &Bldr = C.getSValBuilder(); + DefinedSVal V = Bldr.makeIntVal(0, false); + + SVal LT = + Bldr.evalBinOp(State, BO_LT, DefinedEVal, V, Bldr.getConditionType()); + + // Is E value greater than MaxVal? + ConstraintManager &CM = C.getConstraintManager(); + ProgramStateRef StNegative, StPositive; + std::tie(StNegative, StPositive) = + CM.assumeDual(State, LT.castAs<DefinedSVal>()); + + return StNegative && !StPositive; +} + +bool ConversionChecker::isLossOfPrecision(const ImplicitCastExpr *Cast, + CheckerContext &C) const { + // Don't warn about explicit loss of precision. + if (Cast->isEvaluatable(C.getASTContext())) + return false; + + QualType CastType = Cast->getType(); + QualType SubType = Cast->IgnoreParenImpCasts()->getType(); + + if (!CastType->isIntegerType() || !SubType->isIntegerType()) + return false; + + if (C.getASTContext().getIntWidth(CastType) >= + C.getASTContext().getIntWidth(SubType)) + return false; + + unsigned W = C.getASTContext().getIntWidth(CastType); + if (W == 1 || W >= 64U) + return false; + + unsigned long long MaxVal = 1ULL << W; + return isGreaterEqual(C, Cast->getSubExpr(), MaxVal); +} + +bool ConversionChecker::isLossOfSign(const ImplicitCastExpr *Cast, + CheckerContext &C) const { + QualType CastType = Cast->getType(); + QualType SubType = Cast->IgnoreParenImpCasts()->getType(); + + if (!CastType->isUnsignedIntegerType() || !SubType->isSignedIntegerType()) + return false; + + return isNegative(C, Cast->getSubExpr()); +} + +void ento::registerConversionChecker(CheckerManager &mgr) { + mgr.registerChecker<ConversionChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp b/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp index 7e0cb8e933951..a37ebc506d04b 100644 --- a/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp @@ -107,12 +107,7 @@ PathDiagnosticPiece *DynamicTypeChecker::DynamicTypeBugVisitor::VisitNode( return nullptr; // Retrieve the associated statement. - const Stmt *S = nullptr; - ProgramPoint ProgLoc = N->getLocation(); - if (Optional<StmtPoint> SP = ProgLoc.getAs<StmtPoint>()) { - S = SP->getStmt(); - } - + const Stmt *S = PathDiagnosticLocation::getStmt(N); if (!S) return nullptr; diff --git a/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp b/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp index b8e43325da04c..a418c82f5a017 100644 --- a/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp +++ b/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp @@ -626,7 +626,7 @@ static bool isObjCTypeParamDependent(QualType Type) { : public RecursiveASTVisitor<IsObjCTypeParamDependentTypeVisitor> { public: IsObjCTypeParamDependentTypeVisitor() : Result(false) {} - bool VisitTypedefType(const TypedefType *Type) { + bool VisitObjCTypeParamType(const ObjCTypeParamType *Type) { if (isa<ObjCTypeParamDecl>(Type->getDecl())) { Result = true; return false; @@ -727,6 +727,37 @@ void DynamicTypePropagation::checkPreObjCMessage(const ObjCMethodCall &M, if (!Method) return; + // If the method is declared on a class that has a non-invariant + // type parameter, don't warn about parameter mismatches after performing + // substitution. This prevents warning when the programmer has purposely + // casted the receiver to a super type or unspecialized type but the analyzer + // has a more precise tracked type than the programmer intends at the call + // site. + // + // For example, consider NSArray (which has a covariant type parameter) + // and NSMutableArray (a subclass of NSArray where the type parameter is + // invariant): + // NSMutableArray *a = [[NSMutableArray<NSString *> alloc] init; + // + // [a containsObject:number]; // Safe: -containsObject is defined on NSArray. + // NSArray<NSObject *> *other = [a arrayByAddingObject:number] // Safe + // + // [a addObject:number] // Unsafe: -addObject: is defined on NSMutableArray + // + + const ObjCInterfaceDecl *Interface = Method->getClassInterface(); + if (!Interface) + return; + + ObjCTypeParamList *TypeParams = Interface->getTypeParamList(); + if (!TypeParams) + return; + + for (ObjCTypeParamDecl *TypeParam : *TypeParams) { + if (TypeParam->getVariance() != ObjCTypeParamVariance::Invariant) + return; + } + Optional<ArrayRef<QualType>> TypeArgs = (*TrackedType)->getObjCSubstitutions(Method->getDeclContext()); // This case might happen when there is an unspecialized override of a @@ -909,12 +940,7 @@ PathDiagnosticPiece *DynamicTypePropagation::GenericsBugVisitor::VisitNode( return nullptr; // Retrieve the associated statement. - const Stmt *S = nullptr; - ProgramPoint ProgLoc = N->getLocation(); - if (Optional<StmtPoint> SP = ProgLoc.getAs<StmtPoint>()) { - S = SP->getStmt(); - } - + const Stmt *S = PathDiagnosticLocation::getStmt(N); if (!S) return nullptr; diff --git a/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp b/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp index 31e9150cc15b3..2d5cb60edf7da 100644 --- a/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp @@ -18,25 +18,41 @@ using namespace clang; using namespace ento; namespace { -class ExprInspectionChecker : public Checker<eval::Call, check::DeadSymbols> { +class ExprInspectionChecker : public Checker<eval::Call, check::DeadSymbols, + check::EndAnalysis> { mutable std::unique_ptr<BugType> BT; + // These stats are per-analysis, not per-branch, hence they shouldn't + // stay inside the program state. + struct ReachedStat { + ExplodedNode *ExampleNode; + unsigned NumTimesReached; + }; + mutable llvm::DenseMap<const CallExpr *, ReachedStat> ReachedStats; + void analyzerEval(const CallExpr *CE, CheckerContext &C) const; void analyzerCheckInlined(const CallExpr *CE, CheckerContext &C) const; void analyzerWarnIfReached(const CallExpr *CE, CheckerContext &C) const; + void analyzerNumTimesReached(const CallExpr *CE, CheckerContext &C) const; void analyzerCrash(const CallExpr *CE, CheckerContext &C) const; void analyzerWarnOnDeadSymbol(const CallExpr *CE, CheckerContext &C) const; + void analyzerDump(const CallExpr *CE, CheckerContext &C) const; void analyzerExplain(const CallExpr *CE, CheckerContext &C) const; + void analyzerPrintState(const CallExpr *CE, CheckerContext &C) const; void analyzerGetExtent(const CallExpr *CE, CheckerContext &C) const; typedef void (ExprInspectionChecker::*FnCheck)(const CallExpr *, CheckerContext &C) const; - void reportBug(llvm::StringRef Msg, CheckerContext &C) const; + ExplodedNode *reportBug(llvm::StringRef Msg, CheckerContext &C) const; + ExplodedNode *reportBug(llvm::StringRef Msg, BugReporter &BR, + ExplodedNode *N) const; public: bool evalCall(const CallExpr *CE, CheckerContext &C) const; void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const; + void checkEndAnalysis(ExplodedGraph &G, BugReporter &BR, + ExprEngine &Eng) const; }; } @@ -56,7 +72,12 @@ bool ExprInspectionChecker::evalCall(const CallExpr *CE, .Case("clang_analyzer_warnOnDeadSymbol", &ExprInspectionChecker::analyzerWarnOnDeadSymbol) .Case("clang_analyzer_explain", &ExprInspectionChecker::analyzerExplain) + .Case("clang_analyzer_dump", &ExprInspectionChecker::analyzerDump) .Case("clang_analyzer_getExtent", &ExprInspectionChecker::analyzerGetExtent) + .Case("clang_analyzer_printState", + &ExprInspectionChecker::analyzerPrintState) + .Case("clang_analyzer_numTimesReached", + &ExprInspectionChecker::analyzerNumTimesReached) .Default(nullptr); if (!Handler) @@ -98,16 +119,24 @@ static const char *getArgumentValueString(const CallExpr *CE, } } -void ExprInspectionChecker::reportBug(llvm::StringRef Msg, - CheckerContext &C) const { - if (!BT) - BT.reset(new BugType(this, "Checking analyzer assumptions", "debug")); - +ExplodedNode *ExprInspectionChecker::reportBug(llvm::StringRef Msg, + CheckerContext &C) const { ExplodedNode *N = C.generateNonFatalErrorNode(); + reportBug(Msg, C.getBugReporter(), N); + return N; +} + +ExplodedNode *ExprInspectionChecker::reportBug(llvm::StringRef Msg, + BugReporter &BR, + ExplodedNode *N) const { if (!N) - return; + return nullptr; + + if (!BT) + BT.reset(new BugType(this, "Checking analyzer assumptions", "debug")); - C.emitReport(llvm::make_unique<BugReport>(*BT, Msg, N)); + BR.emitReport(llvm::make_unique<BugReport>(*BT, Msg, N)); + return N; } void ExprInspectionChecker::analyzerEval(const CallExpr *CE, @@ -127,6 +156,15 @@ void ExprInspectionChecker::analyzerWarnIfReached(const CallExpr *CE, reportBug("REACHABLE", C); } +void ExprInspectionChecker::analyzerNumTimesReached(const CallExpr *CE, + CheckerContext &C) const { + ++ReachedStats[CE].NumTimesReached; + if (!ReachedStats[CE].ExampleNode) { + // Later, in checkEndAnalysis, we'd throw a report against it. + ReachedStats[CE].ExampleNode = C.generateNonFatalErrorNode(); + } +} + void ExprInspectionChecker::analyzerCheckInlined(const CallExpr *CE, CheckerContext &C) const { const LocationContext *LC = C.getPredecessor()->getLocationContext(); @@ -144,22 +182,43 @@ void ExprInspectionChecker::analyzerCheckInlined(const CallExpr *CE, void ExprInspectionChecker::analyzerExplain(const CallExpr *CE, CheckerContext &C) const { - if (CE->getNumArgs() == 0) + if (CE->getNumArgs() == 0) { reportBug("Missing argument for explaining", C); + return; + } SVal V = C.getSVal(CE->getArg(0)); SValExplainer Ex(C.getASTContext()); reportBug(Ex.Visit(V), C); } +void ExprInspectionChecker::analyzerDump(const CallExpr *CE, + CheckerContext &C) const { + if (CE->getNumArgs() == 0) { + reportBug("Missing argument for dumping", C); + return; + } + + SVal V = C.getSVal(CE->getArg(0)); + + llvm::SmallString<32> Str; + llvm::raw_svector_ostream OS(Str); + V.dumpToStream(OS); + reportBug(OS.str(), C); +} + void ExprInspectionChecker::analyzerGetExtent(const CallExpr *CE, CheckerContext &C) const { - if (CE->getNumArgs() == 0) + if (CE->getNumArgs() == 0) { reportBug("Missing region for obtaining extent", C); + return; + } auto MR = dyn_cast_or_null<SubRegion>(C.getSVal(CE->getArg(0)).getAsRegion()); - if (!MR) + if (!MR) { reportBug("Obtaining extent of a non-region", C); + return; + } ProgramStateRef State = C.getState(); State = State->BindExpr(CE, C.getLocationContext(), @@ -167,6 +226,11 @@ void ExprInspectionChecker::analyzerGetExtent(const CallExpr *CE, C.addTransition(State); } +void ExprInspectionChecker::analyzerPrintState(const CallExpr *CE, + CheckerContext &C) const { + C.getState()->dump(); +} + void ExprInspectionChecker::analyzerWarnOnDeadSymbol(const CallExpr *CE, CheckerContext &C) const { if (CE->getNumArgs() == 0) @@ -185,15 +249,28 @@ void ExprInspectionChecker::checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const { ProgramStateRef State = C.getState(); const MarkedSymbolsTy &Syms = State->get<MarkedSymbols>(); + ExplodedNode *N = C.getPredecessor(); for (auto I = Syms.begin(), E = Syms.end(); I != E; ++I) { SymbolRef Sym = *I; if (!SymReaper.isDead(Sym)) continue; - reportBug("SYMBOL DEAD", C); + // The non-fatal error node should be the same for all reports. + if (ExplodedNode *BugNode = reportBug("SYMBOL DEAD", C)) + N = BugNode; State = State->remove<MarkedSymbols>(Sym); } - C.addTransition(State); + C.addTransition(State, N); +} + +void ExprInspectionChecker::checkEndAnalysis(ExplodedGraph &G, BugReporter &BR, + ExprEngine &Eng) const { + for (auto Item: ReachedStats) { + unsigned NumTimesReached = Item.second.NumTimesReached; + ExplodedNode *N = Item.second.ExampleNode; + + reportBug(std::to_string(NumTimesReached), BR, N); + } } void ExprInspectionChecker::analyzerCrash(const CallExpr *CE, diff --git a/lib/StaticAnalyzer/Checkers/GTestChecker.cpp b/lib/StaticAnalyzer/Checkers/GTestChecker.cpp new file mode 100644 index 0000000000000..f0be41b293e45 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/GTestChecker.cpp @@ -0,0 +1,299 @@ +//==- GTestChecker.cpp - Model gtest API --*- C++ -*-==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This checker models the behavior of un-inlined APIs from the gtest +// unit-testing library to avoid false positives when using assertions from +// that library. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/AST/Expr.h" +#include "clang/Basic/LangOptions.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" +#include "llvm/Support/raw_ostream.h" + +using namespace clang; +using namespace ento; + +// Modeling of un-inlined AssertionResult constructors +// +// The gtest unit testing API provides macros for assertions that expand +// into an if statement that calls a series of constructors and returns +// when the "assertion" is false. +// +// For example, +// +// ASSERT_TRUE(a == b) +// +// expands into: +// +// switch (0) +// case 0: +// default: +// if (const ::testing::AssertionResult gtest_ar_ = +// ::testing::AssertionResult((a == b))) +// ; +// else +// return ::testing::internal::AssertHelper( +// ::testing::TestPartResult::kFatalFailure, +// "<path to project>", +// <line number>, +// ::testing::internal::GetBoolAssertionFailureMessage( +// gtest_ar_, "a == b", "false", "true") +// .c_str()) = ::testing::Message(); +// +// where AssertionResult is defined similarly to +// +// class AssertionResult { +// public: +// AssertionResult(const AssertionResult& other); +// explicit AssertionResult(bool success) : success_(success) {} +// operator bool() const { return success_; } +// ... +// private: +// bool success_; +// }; +// +// In order for the analyzer to correctly handle this assertion, it needs to +// know that the boolean value of the expression "a == b" is stored the +// 'success_' field of the original AssertionResult temporary and propagated +// (via the copy constructor) into the 'success_' field of the object stored +// in 'gtest_ar_'. That boolean value will then be returned from the bool +// conversion method in the if statement. This guarantees that the assertion +// holds when the return path is not taken. +// +// If the success value is not properly propagated, then the eager case split +// on evaluating the expression can cause pernicious false positives +// on the non-return path: +// +// ASSERT(ptr != NULL) +// *ptr = 7; // False positive null pointer dereference here +// +// Unfortunately, the bool constructor cannot be inlined (because its +// implementation is not present in the headers) and the copy constructor is +// not inlined (because it is constructed into a temporary and the analyzer +// does not inline these since it does not yet reliably call temporary +// destructors). +// +// This checker compensates for the missing inlining by propagating the +// _success value across the bool and copy constructors so the assertion behaves +// as expected. + +namespace { +class GTestChecker : public Checker<check::PostCall> { + + mutable IdentifierInfo *AssertionResultII; + mutable IdentifierInfo *SuccessII; + +public: + GTestChecker(); + + void checkPostCall(const CallEvent &Call, CheckerContext &C) const; + +private: + void modelAssertionResultBoolConstructor(const CXXConstructorCall *Call, + bool IsRef, CheckerContext &C) const; + + void modelAssertionResultCopyConstructor(const CXXConstructorCall *Call, + CheckerContext &C) const; + + void initIdentifierInfo(ASTContext &Ctx) const; + + SVal + getAssertionResultSuccessFieldValue(const CXXRecordDecl *AssertionResultDecl, + SVal Instance, + ProgramStateRef State) const; + + static ProgramStateRef assumeValuesEqual(SVal Val1, SVal Val2, + ProgramStateRef State, + CheckerContext &C); +}; +} // End anonymous namespace. + +GTestChecker::GTestChecker() : AssertionResultII(nullptr), SuccessII(nullptr) {} + +/// Model a call to an un-inlined AssertionResult(bool) or +/// AssertionResult(bool &, ...). +/// To do so, constrain the value of the newly-constructed instance's 'success_' +/// field to be equal to the passed-in boolean value. +/// +/// \param IsRef Whether the boolean parameter is a reference or not. +void GTestChecker::modelAssertionResultBoolConstructor( + const CXXConstructorCall *Call, bool IsRef, CheckerContext &C) const { + assert(Call->getNumArgs() >= 1 && Call->getNumArgs() <= 2); + + ProgramStateRef State = C.getState(); + SVal BooleanArgVal = Call->getArgSVal(0); + if (IsRef) { + // The argument is a reference, so load from it to get the boolean value. + if (!BooleanArgVal.getAs<Loc>()) + return; + BooleanArgVal = C.getState()->getSVal(BooleanArgVal.castAs<Loc>()); + } + + SVal ThisVal = Call->getCXXThisVal(); + + SVal ThisSuccess = getAssertionResultSuccessFieldValue( + Call->getDecl()->getParent(), ThisVal, State); + + State = assumeValuesEqual(ThisSuccess, BooleanArgVal, State, C); + C.addTransition(State); +} + +/// Model a call to an un-inlined AssertionResult copy constructor: +/// +/// AssertionResult(const &AssertionResult other) +/// +/// To do so, constrain the value of the newly-constructed instance's +/// 'success_' field to be equal to the value of the pass-in instance's +/// 'success_' field. +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 + // instance to initialize this instances fields from. + SVal OtherVal = Call->getArgSVal(0); + SVal ThisVal = Call->getCXXThisVal(); + + const CXXRecordDecl *AssertResultClassDecl = Call->getDecl()->getParent(); + ProgramStateRef State = C.getState(); + + SVal ThisSuccess = getAssertionResultSuccessFieldValue(AssertResultClassDecl, + ThisVal, State); + SVal OtherSuccess = getAssertionResultSuccessFieldValue(AssertResultClassDecl, + OtherVal, State); + + State = assumeValuesEqual(ThisSuccess, OtherSuccess, State, C); + C.addTransition(State); +} + +/// Model calls to AssertionResult constructors that are not inlined. +void GTestChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + /// If the constructor was inlined, there is no need model it. + if (C.wasInlined) + return; + + initIdentifierInfo(C.getASTContext()); + + auto *CtorCall = dyn_cast<CXXConstructorCall>(&Call); + if (!CtorCall) + return; + + const CXXConstructorDecl *CtorDecl = CtorCall->getDecl(); + const CXXRecordDecl *CtorParent = CtorDecl->getParent(); + if (CtorParent->getIdentifier() != AssertionResultII) + return; + + unsigned ParamCount = CtorDecl->getNumParams(); + + // Call the appropriate modeling method based the parameters and their + // types. + + // We have AssertionResult(const &AssertionResult) + if (CtorDecl->isCopyConstructor() && ParamCount == 1) { + modelAssertionResultCopyConstructor(CtorCall, C); + return; + } + + // There are two possible boolean constructors, depending on which + // version of gtest is being used: + // + // v1.7 and earlier: + // AssertionResult(bool success) + // + // v1.8 and greater: + // template <typename T> + // AssertionResult(const T& success, + // typename internal::EnableIf< + // !internal::ImplicitlyConvertible<T, + // AssertionResult>::value>::type*) + // + CanQualType BoolTy = C.getASTContext().BoolTy; + if (ParamCount == 1 && CtorDecl->getParamDecl(0)->getType() == BoolTy) { + // We have AssertionResult(bool) + modelAssertionResultBoolConstructor(CtorCall, /*IsRef=*/false, C); + return; + } + if (ParamCount == 2){ + auto *RefTy = CtorDecl->getParamDecl(0)->getType()->getAs<ReferenceType>(); + if (RefTy && + RefTy->getPointeeType()->getCanonicalTypeUnqualified() == BoolTy) { + // We have AssertionResult(bool &, ...) + modelAssertionResultBoolConstructor(CtorCall, /*IsRef=*/true, C); + return; + } + } +} + +void GTestChecker::initIdentifierInfo(ASTContext &Ctx) const { + if (AssertionResultII) + return; + + AssertionResultII = &Ctx.Idents.get("AssertionResult"); + SuccessII = &Ctx.Idents.get("success_"); +} + +/// Returns the value stored in the 'success_' field of the passed-in +/// AssertionResult instance. +SVal GTestChecker::getAssertionResultSuccessFieldValue( + const CXXRecordDecl *AssertionResultDecl, SVal Instance, + ProgramStateRef State) const { + + DeclContext::lookup_result Result = AssertionResultDecl->lookup(SuccessII); + if (Result.empty()) + return UnknownVal(); + + auto *SuccessField = dyn_cast<FieldDecl>(Result.front()); + if (!SuccessField) + return UnknownVal(); + + Optional<Loc> FieldLoc = + State->getLValue(SuccessField, Instance).getAs<Loc>(); + if (!FieldLoc.hasValue()) + return UnknownVal(); + + return State->getSVal(*FieldLoc); +} + +/// Constrain the passed-in state to assume two values are equal. +ProgramStateRef GTestChecker::assumeValuesEqual(SVal Val1, SVal Val2, + ProgramStateRef State, + CheckerContext &C) { + if (!Val1.getAs<DefinedOrUnknownSVal>() || + !Val2.getAs<DefinedOrUnknownSVal>()) + return State; + + auto ValuesEqual = + C.getSValBuilder().evalEQ(State, Val1.castAs<DefinedOrUnknownSVal>(), + Val2.castAs<DefinedOrUnknownSVal>()); + + if (!ValuesEqual.getAs<DefinedSVal>()) + return State; + + State = C.getConstraintManager().assume( + State, ValuesEqual.castAs<DefinedSVal>(), true); + + return State; +} + +void ento::registerGTestChecker(CheckerManager &Mgr) { + const LangOptions &LangOpts = Mgr.getLangOpts(); + // gtest is a C++ API so there is no sense running the checker + // if not compiling for C++. + if (!LangOpts.CPlusPlus) + return; + + Mgr.registerChecker<GTestChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp b/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp index 7be2f574f0e94..d1dab6d27d45f 100644 --- a/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp @@ -19,6 +19,9 @@ #include "clang/AST/Attr.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclObjC.h" +#include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/StmtVisitor.h" +#include "clang/Lex/Lexer.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" @@ -26,11 +29,7 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h" -#include "clang/Lex/Lexer.h" -#include "clang/AST/RecursiveASTVisitor.h" -#include "clang/AST/StmtVisitor.h" #include "llvm/Support/Unicode.h" -#include "llvm/ADT/StringSet.h" using namespace clang; using namespace ento; @@ -189,6 +188,22 @@ void NonLocalizedStringChecker::initUIMethods(ASTContext &Ctx) const { NEW_RECEIVER(NSButton) ADD_UNARY_METHOD(NSButton, setTitle, 0) ADD_UNARY_METHOD(NSButton, setAlternateTitle, 0) + IdentifierInfo *radioButtonWithTitleNSButton[] = { + &Ctx.Idents.get("radioButtonWithTitle"), &Ctx.Idents.get("target"), + &Ctx.Idents.get("action")}; + ADD_METHOD(NSButton, radioButtonWithTitleNSButton, 3, 0) + IdentifierInfo *buttonWithTitleNSButtonImage[] = { + &Ctx.Idents.get("buttonWithTitle"), &Ctx.Idents.get("image"), + &Ctx.Idents.get("target"), &Ctx.Idents.get("action")}; + ADD_METHOD(NSButton, buttonWithTitleNSButtonImage, 4, 0) + IdentifierInfo *checkboxWithTitleNSButton[] = { + &Ctx.Idents.get("checkboxWithTitle"), &Ctx.Idents.get("target"), + &Ctx.Idents.get("action")}; + ADD_METHOD(NSButton, checkboxWithTitleNSButton, 3, 0) + IdentifierInfo *buttonWithTitleNSButtonTarget[] = { + &Ctx.Idents.get("buttonWithTitle"), &Ctx.Idents.get("target"), + &Ctx.Idents.get("action")}; + ADD_METHOD(NSButton, buttonWithTitleNSButtonTarget, 3, 0) NEW_RECEIVER(NSSavePanel) ADD_UNARY_METHOD(NSSavePanel, setPrompt, 0) @@ -271,6 +286,9 @@ void NonLocalizedStringChecker::initUIMethods(ASTContext &Ctx) const { ADD_UNARY_METHOD(NSButtonCell, setTitle, 0) ADD_UNARY_METHOD(NSButtonCell, setAlternateTitle, 0) + NEW_RECEIVER(NSDatePickerCell) + ADD_UNARY_METHOD(NSDatePickerCell, initTextCell, 0) + NEW_RECEIVER(NSSliderCell) ADD_UNARY_METHOD(NSSliderCell, setTitle, 0) @@ -336,9 +354,6 @@ void NonLocalizedStringChecker::initUIMethods(ASTContext &Ctx) const { ADD_UNARY_METHOD(UIActionSheet, addButtonWithTitle, 0) ADD_UNARY_METHOD(UIActionSheet, setTitle, 0) - NEW_RECEIVER(NSURLSessionTask) - ADD_UNARY_METHOD(NSURLSessionTask, setTaskDescription, 0) - NEW_RECEIVER(UIAccessibilityCustomAction) IdentifierInfo *initWithNameUIAccessibilityCustomAction[] = { &Ctx.Idents.get("initWithName"), &Ctx.Idents.get("target"), @@ -363,6 +378,9 @@ void NonLocalizedStringChecker::initUIMethods(ASTContext &Ctx) const { NEW_RECEIVER(NSTextField) ADD_UNARY_METHOD(NSTextField, setPlaceholderString, 0) + ADD_UNARY_METHOD(NSTextField, textFieldWithString, 0) + ADD_UNARY_METHOD(NSTextField, wrappingLabelWithString, 0) + ADD_UNARY_METHOD(NSTextField, labelWithString, 0) NEW_RECEIVER(NSAttributedString) ADD_UNARY_METHOD(NSAttributedString, initWithString, 0) @@ -523,9 +541,6 @@ void NonLocalizedStringChecker::initUIMethods(ASTContext &Ctx) const { ADD_METHOD(NSUserNotificationAction, actionWithIdentifierNSUserNotificationAction, 2, 1) - NEW_RECEIVER(NSURLSession) - ADD_UNARY_METHOD(NSURLSession, setSessionDescription, 0) - NEW_RECEIVER(UITextField) ADD_UNARY_METHOD(UITextField, setText, 0) ADD_UNARY_METHOD(UITextField, setPlaceholder, 0) @@ -1001,6 +1016,8 @@ void EmptyLocalizationContextChecker::checkASTDecl( void EmptyLocalizationContextChecker::MethodCrawler::VisitObjCMessageExpr( const ObjCMessageExpr *ME) { + // FIXME: We may be able to use PPCallbacks to check for empy context + // comments as part of preprocessing and avoid this re-lexing hack. const ObjCInterfaceDecl *OD = ME->getReceiverInterface(); if (!OD) return; @@ -1035,7 +1052,12 @@ void EmptyLocalizationContextChecker::MethodCrawler::VisitObjCMessageExpr( SE = Mgr.getSourceManager().getSLocEntry(SLInfo.first); } - llvm::MemoryBuffer *BF = SE.getFile().getContentCache()->getRawBuffer(); + bool Invalid = false; + llvm::MemoryBuffer *BF = + Mgr.getSourceManager().getBuffer(SLInfo.first, SL, &Invalid); + if (Invalid) + return; + Lexer TheLexer(SL, LangOptions(), BF->getBufferStart(), BF->getBufferStart() + SLInfo.second, BF->getBufferEnd()); diff --git a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h index 22fbf4c5b303d..8474d2d194e80 100644 --- a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h +++ b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIBugReporter.h @@ -46,9 +46,7 @@ public: const ExplodedNode *const ExplNode, BugReporter &BReporter) const; - /// Report a missing wait for a nonblocking call. A missing wait report - /// is emitted if a nonblocking call is not matched in the scope of a - /// function. + /// Report a missing wait for a nonblocking call. /// /// \param Req request that is not matched by a wait /// \param RequestRegion memory region of the request diff --git a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp index c3d0f8f2a1298..c667b9e67d4bf 100644 --- a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp @@ -43,7 +43,8 @@ void MPIChecker::checkDoubleNonblocking(const CallEvent &PreCallEvent, // double nonblocking detected if (Req && Req->CurrentState == Request::State::Nonblocking) { ExplodedNode *ErrorNode = Ctx.generateNonFatalErrorNode(); - BReporter.reportDoubleNonblocking(PreCallEvent, *Req, MR, ErrorNode, Ctx.getBugReporter()); + BReporter.reportDoubleNonblocking(PreCallEvent, *Req, MR, ErrorNode, + Ctx.getBugReporter()); Ctx.addTransition(ErrorNode->getState(), ErrorNode); } // no error @@ -85,7 +86,8 @@ void MPIChecker::checkUnmatchedWaits(const CallEvent &PreCallEvent, State = ErrorNode->getState(); } // A wait has no matching nonblocking call. - BReporter.reportUnmatchedWait(PreCallEvent, ReqRegion, ErrorNode, Ctx.getBugReporter()); + BReporter.reportUnmatchedWait(PreCallEvent, ReqRegion, ErrorNode, + Ctx.getBugReporter()); } } @@ -118,7 +120,8 @@ void MPIChecker::checkMissingWaits(SymbolReaper &SymReaper, ErrorNode = Ctx.generateNonFatalErrorNode(State, &Tag); State = ErrorNode->getState(); } - BReporter.reportMissingWait(Req.second, Req.first, ErrorNode, Ctx.getBugReporter()); + BReporter.reportMissingWait(Req.second, Req.first, ErrorNode, + Ctx.getBugReporter()); } State = State->remove<RequestMap>(Req.first); } diff --git a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.h b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.h index 20c60ad076a21..6b1c062ef3d5b 100644 --- a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.h +++ b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.h @@ -19,8 +19,8 @@ #define LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_MPICHECKER_MPICHECKER_H #include "MPIBugReporter.h" -#include "MPIFunctionClassifier.h" #include "MPITypes.h" +#include "clang/StaticAnalyzer/Checkers/MPIFunctionClassifier.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" @@ -30,7 +30,7 @@ namespace mpi { class MPIChecker : public Checker<check::PreCall, check::DeadSymbols> { public: - MPIChecker() : BReporter(*this) { } + MPIChecker() : BReporter(*this) {} // path-sensitive callbacks void checkPreCall(const CallEvent &CE, CheckerContext &Ctx) const { @@ -49,7 +49,6 @@ public: return; const_cast<std::unique_ptr<MPIFunctionClassifier> &>(FuncClassifier) .reset(new MPIFunctionClassifier{Ctx.getASTContext()}); - } /// Checks if a request is used by nonblocking calls multiple times @@ -60,10 +59,9 @@ public: void checkDoubleNonblocking(const clang::ento::CallEvent &PreCallEvent, clang::ento::CheckerContext &Ctx) const; - /// Checks if a request is used by a wait multiple times in sequence without - /// intermediate nonblocking call or if the request used by the wait - /// function was not used at all before. The check contains a guard, - /// in order to only inspect wait functions. + /// Checks if the request used by the wait function was not used at all + /// before. The check contains a guard, in order to only inspect wait + /// functions. /// /// \param PreCallEvent MPI call to verify void checkUnmatchedWaits(const clang::ento::CallEvent &PreCallEvent, diff --git a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.cpp b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.cpp index ad937f683d30e..12760abaeeffb 100644 --- a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.cpp +++ b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.cpp @@ -12,7 +12,7 @@ /// //===----------------------------------------------------------------------===// -#include "MPIFunctionClassifier.h" +#include "clang/StaticAnalyzer/Checkers/MPIFunctionClassifier.h" #include "llvm/ADT/STLExtras.h" namespace clang { diff --git a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.h b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.h deleted file mode 100644 index 65e908912c548..0000000000000 --- a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIFunctionClassifier.h +++ /dev/null @@ -1,97 +0,0 @@ -//===-- MPIFunctionClassifier.h - classifies MPI functions ----*- C++ -*-===// -// -// The LLVM Compiler Infrastructure -// -// This file is distributed under the University of Illinois Open Source -// License. See LICENSE.TXT for details. -// -//===----------------------------------------------------------------------===// -/// -/// \file -/// This file defines functionality to identify and classify MPI functions. -/// -//===----------------------------------------------------------------------===// - -#ifndef LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_MPICHECKER_MPIFUNCTIONCLASSIFIER_H -#define LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_MPICHECKER_MPIFUNCTIONCLASSIFIER_H - -#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" - -namespace clang { -namespace ento { -namespace mpi { - -class MPIFunctionClassifier { -public: - MPIFunctionClassifier(ASTContext &ASTCtx) { identifierInit(ASTCtx); } - - // general identifiers - bool isMPIType(const IdentifierInfo *const IdentInfo) const; - bool isNonBlockingType(const IdentifierInfo *const IdentInfo) const; - - // point-to-point identifiers - bool isPointToPointType(const IdentifierInfo *const IdentInfo) const; - - // collective identifiers - bool isCollectiveType(const IdentifierInfo *const IdentInfo) const; - bool isCollToColl(const IdentifierInfo *const IdentInfo) const; - bool isScatterType(const IdentifierInfo *const IdentInfo) const; - bool isGatherType(const IdentifierInfo *const IdentInfo) const; - bool isAllgatherType(const IdentifierInfo *const IdentInfo) const; - bool isAlltoallType(const IdentifierInfo *const IdentInfo) const; - bool isReduceType(const IdentifierInfo *const IdentInfo) const; - bool isBcastType(const IdentifierInfo *const IdentInfo) const; - - // additional identifiers - bool isMPI_Wait(const IdentifierInfo *const IdentInfo) const; - bool isMPI_Waitall(const IdentifierInfo *const IdentInfo) const; - bool isWaitType(const IdentifierInfo *const IdentInfo) const; - -private: - // Initializes function identifiers, to recognize them during analysis. - void identifierInit(ASTContext &ASTCtx); - void initPointToPointIdentifiers(ASTContext &ASTCtx); - void initCollectiveIdentifiers(ASTContext &ASTCtx); - void initAdditionalIdentifiers(ASTContext &ASTCtx); - - // The containers are used, to enable classification of MPI-functions during - // analysis. - llvm::SmallVector<IdentifierInfo *, 12> MPINonBlockingTypes; - - llvm::SmallVector<IdentifierInfo *, 10> MPIPointToPointTypes; - llvm::SmallVector<IdentifierInfo *, 16> MPICollectiveTypes; - - llvm::SmallVector<IdentifierInfo *, 4> MPIPointToCollTypes; - llvm::SmallVector<IdentifierInfo *, 4> MPICollToPointTypes; - llvm::SmallVector<IdentifierInfo *, 6> MPICollToCollTypes; - - llvm::SmallVector<IdentifierInfo *, 32> MPIType; - - // point-to-point functions - IdentifierInfo *IdentInfo_MPI_Send = nullptr, *IdentInfo_MPI_Isend = nullptr, - *IdentInfo_MPI_Ssend = nullptr, *IdentInfo_MPI_Issend = nullptr, - *IdentInfo_MPI_Bsend = nullptr, *IdentInfo_MPI_Ibsend = nullptr, - *IdentInfo_MPI_Rsend = nullptr, *IdentInfo_MPI_Irsend = nullptr, - *IdentInfo_MPI_Recv = nullptr, *IdentInfo_MPI_Irecv = nullptr; - - // collective functions - IdentifierInfo *IdentInfo_MPI_Scatter = nullptr, - *IdentInfo_MPI_Iscatter = nullptr, *IdentInfo_MPI_Gather = nullptr, - *IdentInfo_MPI_Igather = nullptr, *IdentInfo_MPI_Allgather = nullptr, - *IdentInfo_MPI_Iallgather = nullptr, *IdentInfo_MPI_Bcast = nullptr, - *IdentInfo_MPI_Ibcast = nullptr, *IdentInfo_MPI_Reduce = nullptr, - *IdentInfo_MPI_Ireduce = nullptr, *IdentInfo_MPI_Allreduce = nullptr, - *IdentInfo_MPI_Iallreduce = nullptr, *IdentInfo_MPI_Alltoall = nullptr, - *IdentInfo_MPI_Ialltoall = nullptr, *IdentInfo_MPI_Barrier = nullptr; - - // additional functions - IdentifierInfo *IdentInfo_MPI_Comm_rank = nullptr, - *IdentInfo_MPI_Comm_size = nullptr, *IdentInfo_MPI_Wait = nullptr, - *IdentInfo_MPI_Waitall = nullptr; -}; - -} // end of namespace: mpi -} // end of namespace: ento -} // end of namespace: clang - -#endif diff --git a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h index 27ec950d31eb9..2e7140cd771e4 100644 --- a/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h +++ b/lib/StaticAnalyzer/Checkers/MPI-Checker/MPITypes.h @@ -17,7 +17,7 @@ #ifndef LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_MPICHECKER_MPITYPES_H #define LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_MPICHECKER_MPITYPES_H -#include "MPIFunctionClassifier.h" +#include "clang/StaticAnalyzer/Checkers/MPIFunctionClassifier.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" #include "llvm/ADT/SmallSet.h" @@ -53,7 +53,6 @@ typedef llvm::ImmutableMap<const clang::ento::MemRegion *, } // end of namespace: mpi - template <> struct ProgramStateTrait<mpi::RequestMap> : public ProgramStatePartialTrait<mpi::RequestMapImpl> { diff --git a/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp b/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp index 1e56d709e4f91..86c827045e9ac 100644 --- a/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp @@ -524,12 +524,7 @@ MacOSKeychainAPIChecker::generateAllocatedDataNotReleasedReport( // allocated, and only report a single path. PathDiagnosticLocation LocUsedForUniqueing; const ExplodedNode *AllocNode = getAllocationNode(N, AP.first, C); - const Stmt *AllocStmt = nullptr; - ProgramPoint P = AllocNode->getLocation(); - if (Optional<CallExitEnd> Exit = P.getAs<CallExitEnd>()) - AllocStmt = Exit->getCalleeContext()->getCallSite(); - else if (Optional<clang::PostStmt> PS = P.getAs<clang::PostStmt>()) - AllocStmt = PS->getStmt(); + const Stmt *AllocStmt = PathDiagnosticLocation::getStmt(AllocNode); if (AllocStmt) LocUsedForUniqueing = PathDiagnosticLocation::createBegin(AllocStmt, diff --git a/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp b/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp index c038a2649e15d..0e0f52af31653 100644 --- a/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp @@ -33,6 +33,8 @@ namespace { class MacOSXAPIChecker : public Checker< check::PreStmt<CallExpr> > { mutable std::unique_ptr<BugType> BT_dispatchOnce; + static const ObjCIvarRegion *getParentIvarRegion(const MemRegion *R); + public: void checkPreStmt(const CallExpr *CE, CheckerContext &C) const; @@ -49,27 +51,34 @@ public: // dispatch_once and dispatch_once_f //===----------------------------------------------------------------------===// +const ObjCIvarRegion * +MacOSXAPIChecker::getParentIvarRegion(const MemRegion *R) { + const SubRegion *SR = dyn_cast<SubRegion>(R); + while (SR) { + if (const ObjCIvarRegion *IR = dyn_cast<ObjCIvarRegion>(SR)) + return IR; + SR = dyn_cast<SubRegion>(SR->getSuperRegion()); + } + return nullptr; +} + void MacOSXAPIChecker::CheckDispatchOnce(CheckerContext &C, const CallExpr *CE, StringRef FName) const { if (CE->getNumArgs() < 1) return; - // 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(); - if (!R || !isa<StackSpaceRegion>(R->getMemorySpace())) + // Check if the first argument is improperly allocated. If so, issue a + // warning because that's likely to be bad news. + const MemRegion *R = C.getSVal(CE->getArg(0)).getAsRegion(); + if (!R) return; - ExplodedNode *N = C.generateErrorNode(state); - if (!N) + // Global variables are fine. + const MemRegion *RB = R->getBaseRegion(); + const MemSpaceRegion *RS = RB->getMemorySpace(); + if (isa<GlobalsSpaceRegion>(RS)) return; - if (!BT_dispatchOnce) - BT_dispatchOnce.reset(new BugType(this, "Improper use of 'dispatch_once'", - "API Misuse (Apple)")); - // Handle _dispatch_once. In some versions of the OS X SDK we have the case // that dispatch_once is a macro that wraps a call to _dispatch_once. // _dispatch_once is then a function which then calls the real dispatch_once. @@ -82,16 +91,48 @@ void MacOSXAPIChecker::CheckDispatchOnce(CheckerContext &C, const CallExpr *CE, SmallString<256> S; llvm::raw_svector_ostream os(S); + bool SuggestStatic = false; os << "Call to '" << FName << "' uses"; - if (const VarRegion *VR = dyn_cast<VarRegion>(R)) - os << " the local variable '" << VR->getDecl()->getName() << '\''; - else + if (const VarRegion *VR = dyn_cast<VarRegion>(RB)) { + // We filtered out globals earlier, so it must be a local variable + // or a block variable which is under UnknownSpaceRegion. + if (VR != R) + os << " memory within"; + if (VR->getDecl()->hasAttr<BlocksAttr>()) + os << " the block variable '"; + else + os << " the local variable '"; + os << VR->getDecl()->getName() << '\''; + SuggestStatic = true; + } else if (const ObjCIvarRegion *IVR = getParentIvarRegion(R)) { + if (IVR != R) + os << " memory within"; + os << " the instance variable '" << IVR->getDecl()->getName() << '\''; + } else if (isa<HeapSpaceRegion>(RS)) { + os << " heap-allocated memory"; + } else if (isa<UnknownSpaceRegion>(RS)) { + // Presence of an IVar superregion has priority over this branch, because + // ObjC objects are on the heap even if the core doesn't realize this. + // Presence of a block variable base region has priority over this branch, + // because block variables are known to be either on stack or on heap + // (might actually move between the two, hence UnknownSpace). + return; + } else { os << " stack allocated memory"; + } os << " for the predicate value. Using such transient memory for " "the predicate is potentially dangerous."; - if (isa<VarRegion>(R) && isa<StackLocalsSpaceRegion>(R->getMemorySpace())) + if (SuggestStatic) os << " Perhaps you intended to declare the variable as 'static'?"; + ExplodedNode *N = C.generateErrorNode(); + if (!N) + return; + + if (!BT_dispatchOnce) + BT_dispatchOnce.reset(new BugType(this, "Improper use of 'dispatch_once'", + "API Misuse (Apple)")); + auto report = llvm::make_unique<BugReport>(*BT_dispatchOnce, os.str(), N); report->addRange(CE->getArg(0)->getSourceRange()); C.emitReport(std::move(report)); diff --git a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index e06662b169342..f7c4ea10c4386 100644 --- a/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -26,7 +26,6 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h" -#include "llvm/ADT/ImmutableMap.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallString.h" #include "llvm/ADT/StringExtras.h" @@ -288,6 +287,9 @@ private: ProgramStateRef State, AllocationFamily Family = AF_Malloc); + static ProgramStateRef addExtentSize(CheckerContext &C, const CXXNewExpr *NE, + ProgramStateRef State); + // Check if this malloc() for special flags. At present that means M_ZERO or // __GFP_ZERO (in which case, treat it like calloc). llvm::Optional<ProgramStateRef> @@ -776,6 +778,8 @@ void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const { State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State); } } else if (FunI == II_kmalloc) { + if (CE->getNumArgs() < 1) + return; llvm::Optional<ProgramStateRef> MaybeState = performKernelMalloc(CE, C, State); if (MaybeState.hasValue()) @@ -805,6 +809,8 @@ void MallocChecker::checkPostStmt(const CallExpr *CE, CheckerContext &C) const { } else if (FunI == II_strndup) { State = MallocUpdateRefState(C, CE, State); } else if (FunI == II_alloca || FunI == II_win_alloca) { + if (CE->getNumArgs() < 1) + return; State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State, AF_Alloca); State = ProcessZeroAllocation(C, CE, 0, State); @@ -982,10 +988,58 @@ void MallocChecker::checkPostStmt(const CXXNewExpr *NE, // existing binding. State = MallocUpdateRefState(C, NE, State, NE->isArray() ? AF_CXXNewArray : AF_CXXNew); + State = addExtentSize(C, NE, State); State = ProcessZeroAllocation(C, NE, 0, State); C.addTransition(State); } +// 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) { + 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()); + // Store the extent size for the (symbolic)region + // containing the elements. + Region = (State->getSVal(NE, LCtx)) + .getAsRegion() + ->getAs<SubRegion>() + ->getSuperRegion() + ->getAs<SubRegion>(); + } else { + ElementCount = svalBuilder.makeIntVal(1, true); + Region = (State->getSVal(NE, LCtx)).getAsRegion()->getAs<SubRegion>(); + } + assert(Region); + + // Set the region's extent equal to the Size in Bytes. + QualType ElementType = NE->getAllocatedType(); + ASTContext &AstContext = C.getASTContext(); + CharUnits TypeSize = AstContext.getTypeSizeInChars(ElementType); + + if (ElementCount.getAs<NonLoc>()) { + DefinedOrUnknownSVal Extent = Region->getExtent(svalBuilder); + // size in Bytes = ElementCount*TypeSize + SVal SizeInBytes = svalBuilder.evalBinOpNN( + State, BO_Mul, ElementCount.castAs<NonLoc>(), + svalBuilder.makeArrayIndex(TypeSize.getQuantity()), + svalBuilder.getArrayIndexType()); + DefinedOrUnknownSVal extentMatchesSize = svalBuilder.evalEQ( + State, Extent, SizeInBytes.castAs<DefinedOrUnknownSVal>()); + State = State->assume(extentMatchesSize, true); + } + return State; +} + void MallocChecker::checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const { @@ -2095,12 +2149,7 @@ void MallocChecker::reportLeak(SymbolRef Sym, ExplodedNode *N, const MemRegion *Region = nullptr; std::tie(AllocNode, Region) = getAllocationSite(N, Sym, C); - ProgramPoint P = AllocNode->getLocation(); - const Stmt *AllocationStmt = nullptr; - if (Optional<CallExitEnd> Exit = P.getAs<CallExitEnd>()) - AllocationStmt = Exit->getCalleeContext()->getCallSite(); - else if (Optional<StmtPoint> SP = P.getAs<StmtPoint>()) - AllocationStmt = SP->getStmt(); + const Stmt *AllocationStmt = PathDiagnosticLocation::getStmt(AllocNode); if (AllocationStmt) LocUsedForUniqueing = PathDiagnosticLocation::createBegin(AllocationStmt, C.getSourceManager(), @@ -2529,6 +2578,11 @@ bool MallocChecker::mayFreeAnyEscapedMemoryOrIsModeledExplicitly( return true; } + if (FName == "connectImpl" && + FD->getQualifiedNameAsString() == "QObject::connectImpl") { + return true; + } + // Handle cases where we know a buffer's /address/ can escape. // Note that the above checks handle some special cases where we know that // even though the address escapes, it's still our responsibility to free the @@ -2627,22 +2681,7 @@ MallocChecker::MallocBugVisitor::VisitNode(const ExplodedNode *N, if (!RS) return nullptr; - const Stmt *S = nullptr; - const char *Msg = nullptr; - StackHintGeneratorForSymbol *StackHint = nullptr; - - // Retrieve the associated statement. - ProgramPoint ProgLoc = N->getLocation(); - if (Optional<StmtPoint> SP = ProgLoc.getAs<StmtPoint>()) { - S = SP->getStmt(); - } else if (Optional<CallExitEnd> Exit = ProgLoc.getAs<CallExitEnd>()) { - S = Exit->getCalleeContext()->getCallSite(); - } else if (Optional<BlockEdge> Edge = ProgLoc.getAs<BlockEdge>()) { - // If an assumption was made on a branch, it should be caught - // here by looking at the state transition. - S = Edge->getSrc()->getTerminator(); - } - + const Stmt *S = PathDiagnosticLocation::getStmt(N); if (!S) return nullptr; @@ -2650,6 +2689,8 @@ MallocChecker::MallocBugVisitor::VisitNode(const ExplodedNode *N, // (__attribute__((cleanup))). // Find out if this is an interesting point and what is the kind. + const char *Msg = nullptr; + StackHintGeneratorForSymbol *StackHint = nullptr; if (Mode == Normal) { if (isAllocated(RS, RSPrev, S)) { Msg = "Memory is allocated"; diff --git a/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp index d7ec6b10c6f76..d96017a1f5325 100644 --- a/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp @@ -325,10 +325,7 @@ PathDiagnosticPiece *NullabilityChecker::NullabilityBugVisitor::VisitNode( // Retrieve the associated statement. const Stmt *S = TrackedNullab->getNullabilitySource(); if (!S) { - ProgramPoint ProgLoc = N->getLocation(); - if (Optional<StmtPoint> SP = ProgLoc.getAs<StmtPoint>()) { - S = SP->getStmt(); - } + S = PathDiagnosticLocation::getStmt(N); } if (!S) @@ -336,7 +333,7 @@ PathDiagnosticPiece *NullabilityChecker::NullabilityBugVisitor::VisitNode( std::string InfoText = (llvm::Twine("Nullability '") + - getNullabilityString(TrackedNullab->getValue()) + "' is infered") + getNullabilityString(TrackedNullab->getValue()) + "' is inferred") .str(); // Generate the extra diagnostic. @@ -613,9 +610,9 @@ void NullabilityChecker::checkPreStmt(const ReturnStmt *S, SmallString<256> SBuf; llvm::raw_svector_ostream OS(SBuf); - OS << "Null is returned from a " << C.getDeclDescription(D) << + OS << (RetExpr->getType()->isObjCObjectPointerType() ? "nil" : "Null"); + OS << " returned from a " << C.getDeclDescription(D) << " that is expected to return a non-null value"; - reportBugIfInvariantHolds(OS.str(), ErrorKind::NilReturnedToNonnull, N, nullptr, C, RetExpr); @@ -682,9 +679,10 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call, if (Param->isParameterPack()) break; - const Expr *ArgExpr = nullptr; - if (Idx < Call.getNumArgs()) - ArgExpr = Call.getArgExpr(Idx); + if (Idx >= Call.getNumArgs()) + break; + + const Expr *ArgExpr = Call.getArgExpr(Idx); auto ArgSVal = Call.getArgSVal(Idx++).getAs<DefinedOrUnknownSVal>(); if (!ArgSVal) continue; @@ -709,9 +707,11 @@ void NullabilityChecker::checkPreCall(const CallEvent &Call, ExplodedNode *N = C.generateErrorNode(State); if (!N) return; + SmallString<256> SBuf; llvm::raw_svector_ostream OS(SBuf); - OS << "Null passed to a callee that requires a non-null " << ParamIdx + OS << (Param->getType()->isObjCObjectPointerType() ? "nil" : "Null"); + OS << " passed to a callee that requires a non-null " << ParamIdx << llvm::getOrdinalSuffix(ParamIdx) << " parameter"; reportBugIfInvariantHolds(OS.str(), ErrorKind::NilPassedToNonnull, N, nullptr, C, @@ -1130,8 +1130,11 @@ void NullabilityChecker::checkBind(SVal L, SVal V, const Stmt *S, if (ValueExpr) ValueStmt = ValueExpr; - reportBugIfInvariantHolds("Null is assigned to a pointer which is " - "expected to have non-null value", + SmallString<256> SBuf; + llvm::raw_svector_ostream OS(SBuf); + OS << (LocType->isObjCObjectPointerType() ? "nil" : "Null"); + OS << " assigned to a pointer which is expected to have non-null value"; + reportBugIfInvariantHolds(OS.str(), ErrorKind::NilAssignedToNonnull, N, nullptr, C, ValueStmt); return; diff --git a/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp b/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp new file mode 100644 index 0000000000000..40e379cb2efc6 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp @@ -0,0 +1,348 @@ +//===- NumberObjectConversionChecker.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 NumberObjectConversionChecker, which checks for a +// particular common mistake when dealing with numbers represented as objects +// passed around by pointers. Namely, the language allows to reinterpret the +// pointer as a number directly, often without throwing any warnings, +// but in most cases the result of such conversion is clearly unexpected, +// as pointer value, rather than number value represented by the pointee object, +// becomes the result of such operation. +// +// Currently the checker supports the Objective-C NSNumber class, +// and the OSBoolean class found in macOS low-level code; the latter +// can only hold boolean values. +// +// This checker has an option "Pedantic" (boolean), which enables detection of +// more conversion patterns (which are most likely more harmless, and therefore +// are more likely to produce false positives) - disabled by default, +// enabled with `-analyzer-config osx.NumberObjectConversion:Pedantic=true'. +// +//===----------------------------------------------------------------------===// + +#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 "clang/Lex/Lexer.h" +#include "llvm/ADT/APSInt.h" + +using namespace clang; +using namespace ento; +using namespace ast_matchers; + +namespace { + +class NumberObjectConversionChecker : public Checker<check::ASTCodeBody> { +public: + bool Pedantic; + + void checkASTCodeBody(const Decl *D, AnalysisManager &AM, + BugReporter &BR) const; +}; + +class Callback : public MatchFinder::MatchCallback { + const NumberObjectConversionChecker *C; + BugReporter &BR; + AnalysisDeclContext *ADC; + +public: + Callback(const NumberObjectConversionChecker *C, + BugReporter &BR, AnalysisDeclContext *ADC) + : C(C), BR(BR), ADC(ADC) {} + virtual void run(const MatchFinder::MatchResult &Result); +}; +} // end of anonymous namespace + +void Callback::run(const MatchFinder::MatchResult &Result) { + bool IsPedanticMatch = + (Result.Nodes.getNodeAs<Stmt>("pedantic") != nullptr); + if (IsPedanticMatch && !C->Pedantic) + return; + + ASTContext &ACtx = ADC->getASTContext(); + + if (const Expr *CheckIfNull = + Result.Nodes.getNodeAs<Expr>("check_if_null")) { + // Unless the macro indicates that the intended type is clearly not + // a pointer type, we should avoid warning on comparing pointers + // to zero literals in non-pedantic mode. + // FIXME: Introduce an AST matcher to implement the macro-related logic? + bool MacroIndicatesWeShouldSkipTheCheck = false; + SourceLocation Loc = CheckIfNull->getLocStart(); + if (Loc.isMacroID()) { + StringRef MacroName = Lexer::getImmediateMacroName( + Loc, ACtx.getSourceManager(), ACtx.getLangOpts()); + if (MacroName == "NULL" || MacroName == "nil") + return; + if (MacroName == "YES" || MacroName == "NO") + MacroIndicatesWeShouldSkipTheCheck = true; + } + if (!MacroIndicatesWeShouldSkipTheCheck) { + llvm::APSInt Result; + if (CheckIfNull->IgnoreParenCasts()->EvaluateAsInt( + Result, ACtx, Expr::SE_AllowSideEffects)) { + if (Result == 0) { + if (!C->Pedantic) + return; + IsPedanticMatch = true; + } + } + } + } + + const Stmt *Conv = Result.Nodes.getNodeAs<Stmt>("conv"); + assert(Conv); + + const Expr *ConvertedCObject = Result.Nodes.getNodeAs<Expr>("c_object"); + const Expr *ConvertedCppObject = Result.Nodes.getNodeAs<Expr>("cpp_object"); + const Expr *ConvertedObjCObject = Result.Nodes.getNodeAs<Expr>("objc_object"); + bool IsCpp = (ConvertedCppObject != nullptr); + bool IsObjC = (ConvertedObjCObject != nullptr); + const Expr *Obj = IsObjC ? ConvertedObjCObject + : IsCpp ? ConvertedCppObject + : ConvertedCObject; + assert(Obj); + + bool IsComparison = + (Result.Nodes.getNodeAs<Stmt>("comparison") != nullptr); + + bool IsOSNumber = + (Result.Nodes.getNodeAs<Decl>("osnumber") != nullptr); + + bool IsInteger = + (Result.Nodes.getNodeAs<QualType>("int_type") != nullptr); + bool IsObjCBool = + (Result.Nodes.getNodeAs<QualType>("objc_bool_type") != nullptr); + bool IsCppBool = + (Result.Nodes.getNodeAs<QualType>("cpp_bool_type") != nullptr); + + llvm::SmallString<64> Msg; + llvm::raw_svector_ostream OS(Msg); + + // Remove ObjC ARC qualifiers. + QualType ObjT = Obj->getType().getUnqualifiedType(); + + // Remove consts from pointers. + if (IsCpp) { + assert(ObjT.getCanonicalType()->isPointerType()); + ObjT = ACtx.getPointerType( + ObjT->getPointeeType().getCanonicalType().getUnqualifiedType()); + } + + if (IsComparison) + OS << "Comparing "; + else + OS << "Converting "; + + OS << "a pointer value of type '" << ObjT.getAsString() << "' to a "; + + std::string EuphemismForPlain = "primitive"; + std::string SuggestedApi = IsObjC ? (IsInteger ? "" : "-boolValue") + : IsCpp ? (IsOSNumber ? "" : "getValue()") + : "CFNumberGetValue()"; + if (SuggestedApi.empty()) { + // A generic message if we're not sure what API should be called. + // FIXME: Pattern-match the integer type to make a better guess? + SuggestedApi = + "a method on '" + ObjT.getAsString() + "' to get the scalar value"; + // "scalar" is not quite correct or common, but some documentation uses it + // when describing object methods we suggest. For consistency, we use + // "scalar" in the whole sentence when we need to use this word in at least + // one place, otherwise we use "primitive". + EuphemismForPlain = "scalar"; + } + + if (IsInteger) + OS << EuphemismForPlain << " integer value"; + else if (IsObjCBool) + OS << EuphemismForPlain << " BOOL value"; + else if (IsCppBool) + OS << EuphemismForPlain << " bool value"; + else // Branch condition? + OS << EuphemismForPlain << " boolean value"; + + + if (IsPedanticMatch) + OS << "; instead, either compare the pointer to " + << (IsObjC ? "nil" : IsCpp ? "nullptr" : "NULL") << " or "; + else + OS << "; did you mean to "; + + if (IsComparison) + OS << "compare the result of calling " << SuggestedApi; + else + OS << "call " << SuggestedApi; + + if (!IsPedanticMatch) + OS << "?"; + + BR.EmitBasicReport( + ADC->getDecl(), C, "Suspicious number object conversion", "Logic error", + OS.str(), + PathDiagnosticLocation::createBegin(Obj, BR.getSourceManager(), ADC), + Conv->getSourceRange()); +} + +void NumberObjectConversionChecker::checkASTCodeBody(const Decl *D, + AnalysisManager &AM, + BugReporter &BR) const { + // Currently this matches CoreFoundation opaque pointer typedefs. + auto CSuspiciousNumberObjectExprM = + expr(ignoringParenImpCasts( + expr(hasType( + typedefType(hasDeclaration(anyOf( + typedefDecl(hasName("CFNumberRef")), + typedefDecl(hasName("CFBooleanRef"))))))) + .bind("c_object"))); + + // Currently this matches XNU kernel number-object pointers. + auto CppSuspiciousNumberObjectExprM = + expr(ignoringParenImpCasts( + expr(hasType(hasCanonicalType( + pointerType(pointee(hasCanonicalType( + recordType(hasDeclaration( + anyOf( + cxxRecordDecl(hasName("OSBoolean")), + cxxRecordDecl(hasName("OSNumber")) + .bind("osnumber")))))))))) + .bind("cpp_object"))); + + // Currently this matches NeXTSTEP number objects. + auto ObjCSuspiciousNumberObjectExprM = + expr(ignoringParenImpCasts( + expr(hasType(hasCanonicalType( + objcObjectPointerType(pointee( + qualType(hasCanonicalType( + qualType(hasDeclaration( + objcInterfaceDecl(hasName("NSNumber"))))))))))) + .bind("objc_object"))); + + auto SuspiciousNumberObjectExprM = anyOf( + CSuspiciousNumberObjectExprM, + CppSuspiciousNumberObjectExprM, + ObjCSuspiciousNumberObjectExprM); + + // Useful for predicates like "Unless we've seen the same object elsewhere". + auto AnotherSuspiciousNumberObjectExprM = + expr(anyOf( + equalsBoundNode("c_object"), + equalsBoundNode("objc_object"), + equalsBoundNode("cpp_object"))); + + // The .bind here is in order to compose the error message more accurately. + auto ObjCSuspiciousScalarBooleanTypeM = + qualType(typedefType(hasDeclaration( + typedefDecl(hasName("BOOL"))))).bind("objc_bool_type"); + + // The .bind here is in order to compose the error message more accurately. + auto SuspiciousScalarBooleanTypeM = + qualType(anyOf(qualType(booleanType()).bind("cpp_bool_type"), + ObjCSuspiciousScalarBooleanTypeM)); + + // The .bind here is in order to compose the error message more accurately. + // Also avoid intptr_t and uintptr_t because they were specifically created + // for storing pointers. + auto SuspiciousScalarNumberTypeM = + qualType(hasCanonicalType(isInteger()), + unless(typedefType(hasDeclaration( + typedefDecl(matchesName("^::u?intptr_t$")))))) + .bind("int_type"); + + auto SuspiciousScalarTypeM = + qualType(anyOf(SuspiciousScalarBooleanTypeM, + SuspiciousScalarNumberTypeM)); + + auto SuspiciousScalarExprM = + expr(ignoringParenImpCasts(expr(hasType(SuspiciousScalarTypeM)))); + + auto ConversionThroughAssignmentM = + binaryOperator(allOf(hasOperatorName("="), + hasLHS(SuspiciousScalarExprM), + hasRHS(SuspiciousNumberObjectExprM))); + + auto ConversionThroughBranchingM = + ifStmt(hasCondition(SuspiciousNumberObjectExprM)) + .bind("pedantic"); + + auto ConversionThroughCallM = + callExpr(hasAnyArgument(allOf(hasType(SuspiciousScalarTypeM), + ignoringParenImpCasts( + SuspiciousNumberObjectExprM)))); + + // We bind "check_if_null" to modify the warning message + // in case it was intended to compare a pointer to 0 with a relatively-ok + // construct "x == 0" or "x != 0". + auto ConversionThroughEquivalenceM = + binaryOperator(allOf(anyOf(hasOperatorName("=="), hasOperatorName("!=")), + hasEitherOperand(SuspiciousNumberObjectExprM), + hasEitherOperand(SuspiciousScalarExprM + .bind("check_if_null")))) + .bind("comparison"); + + auto ConversionThroughComparisonM = + binaryOperator(allOf(anyOf(hasOperatorName(">="), hasOperatorName(">"), + hasOperatorName("<="), hasOperatorName("<")), + hasEitherOperand(SuspiciousNumberObjectExprM), + hasEitherOperand(SuspiciousScalarExprM))) + .bind("comparison"); + + auto ConversionThroughConditionalOperatorM = + conditionalOperator(allOf( + hasCondition(SuspiciousNumberObjectExprM), + unless(hasTrueExpression( + hasDescendant(AnotherSuspiciousNumberObjectExprM))), + unless(hasFalseExpression( + hasDescendant(AnotherSuspiciousNumberObjectExprM))))) + .bind("pedantic"); + + auto ConversionThroughExclamationMarkM = + unaryOperator(allOf(hasOperatorName("!"), + has(expr(SuspiciousNumberObjectExprM)))) + .bind("pedantic"); + + auto ConversionThroughExplicitBooleanCastM = + explicitCastExpr(allOf(hasType(SuspiciousScalarBooleanTypeM), + has(expr(SuspiciousNumberObjectExprM)))); + + auto ConversionThroughExplicitNumberCastM = + explicitCastExpr(allOf(hasType(SuspiciousScalarNumberTypeM), + has(expr(SuspiciousNumberObjectExprM)))); + + auto ConversionThroughInitializerM = + declStmt(hasSingleDecl( + varDecl(hasType(SuspiciousScalarTypeM), + hasInitializer(SuspiciousNumberObjectExprM)))); + + auto FinalM = stmt(anyOf(ConversionThroughAssignmentM, + ConversionThroughBranchingM, + ConversionThroughCallM, + ConversionThroughComparisonM, + ConversionThroughConditionalOperatorM, + ConversionThroughEquivalenceM, + ConversionThroughExclamationMarkM, + ConversionThroughExplicitBooleanCastM, + ConversionThroughExplicitNumberCastM, + ConversionThroughInitializerM)).bind("conv"); + + MatchFinder F; + Callback CB(this, BR, AM.getAnalysisDeclContext(D)); + + F.addMatcher(stmt(forEachDescendant(FinalM)), &CB); + F.match(*D->getBody(), AM.getASTContext()); +} + +void ento::registerNumberObjectConversionChecker(CheckerManager &Mgr) { + NumberObjectConversionChecker *Chk = + Mgr.registerChecker<NumberObjectConversionChecker>(); + Chk->Pedantic = + Mgr.getAnalyzerOptions().getBooleanOption("Pedantic", false, Chk); +} diff --git a/lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp new file mode 100644 index 0000000000000..b9857e51f3eaa --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/ObjCPropertyChecker.cpp @@ -0,0 +1,82 @@ +//==- ObjCPropertyChecker.cpp - Check ObjC properties ------------*- C++ -*-==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This checker finds issues with Objective-C properties. +// Currently finds only one kind of issue: +// - Find synthesized properties with copy attribute of mutable NS collection +// types. Calling -copy on such collections produces an immutable copy, +// which contradicts the type of the property. +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" +#include "clang/StaticAnalyzer/Core/Checker.h" + +using namespace clang; +using namespace ento; + +namespace { +class ObjCPropertyChecker + : public Checker<check::ASTDecl<ObjCPropertyDecl>> { + void checkCopyMutable(const ObjCPropertyDecl *D, BugReporter &BR) const; + +public: + void checkASTDecl(const ObjCPropertyDecl *D, AnalysisManager &Mgr, + BugReporter &BR) const; +}; +} // end anonymous namespace. + +void ObjCPropertyChecker::checkASTDecl(const ObjCPropertyDecl *D, + AnalysisManager &Mgr, + BugReporter &BR) const { + checkCopyMutable(D, BR); +} + +void ObjCPropertyChecker::checkCopyMutable(const ObjCPropertyDecl *D, + BugReporter &BR) const { + if (D->isReadOnly() || D->getSetterKind() != ObjCPropertyDecl::Copy) + return; + + QualType T = D->getType(); + if (!T->isObjCObjectPointerType()) + return; + + const std::string &PropTypeName(T->getPointeeType().getCanonicalType() + .getUnqualifiedType() + .getAsString()); + if (!StringRef(PropTypeName).startswith("NSMutable")) + return; + + const ObjCImplDecl *ImplD = nullptr; + if (const ObjCInterfaceDecl *IntD = + dyn_cast<ObjCInterfaceDecl>(D->getDeclContext())) { + ImplD = IntD->getImplementation(); + } else { + const ObjCCategoryDecl *CatD = cast<ObjCCategoryDecl>(D->getDeclContext()); + ImplD = CatD->getClassInterface()->getImplementation(); + } + + if (!ImplD || ImplD->HasUserDeclaredSetterMethod(D)) + return; + + SmallString<128> Str; + llvm::raw_svector_ostream OS(Str); + OS << "Property of mutable type '" << PropTypeName + << "' has 'copy' attribute; an immutable object will be stored instead"; + + BR.EmitBasicReport( + D, this, "Objective-C property misuse", "Logic error", OS.str(), + PathDiagnosticLocation::createBegin(D, BR.getSourceManager()), + D->getSourceRange()); +} + +void ento::registerObjCPropertyChecker(CheckerManager &Mgr) { + Mgr.registerChecker<ObjCPropertyChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp index 15980c5c53870..e75d20897710a 100644 --- a/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ObjCSuperDeallocChecker.cpp @@ -191,7 +191,7 @@ void ObjCSuperDeallocChecker::reportUseAfterDealloc(SymbolRef Sym, return; if (Desc.empty()) - Desc = "use of 'self' after it has been deallocated"; + Desc = "Use of 'self' after it has been deallocated"; // Generate the report. std::unique_ptr<BugReport> BR( diff --git a/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp b/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp index 0640d2f49f43a..a51dda6fe858c 100644 --- a/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/PaddingChecker.cpp @@ -82,7 +82,11 @@ public: CharUnits BaselinePad = calculateBaselinePad(RD, ASTContext, RL); if (BaselinePad.isZero()) return; - CharUnits OptimalPad = calculateOptimalPad(RD, ASTContext, RL); + + CharUnits OptimalPad; + SmallVector<const FieldDecl *, 20> OptimalFieldsOrder; + std::tie(OptimalPad, OptimalFieldsOrder) = + calculateOptimalPad(RD, ASTContext, RL); CharUnits DiffPad = PadMultiplier * (BaselinePad - OptimalPad); if (DiffPad.getQuantity() <= AllowedPad) { @@ -90,7 +94,7 @@ public: // There is not enough excess padding to trigger a warning. return; } - reportRecord(RD, BaselinePad, OptimalPad); + reportRecord(RD, BaselinePad, OptimalPad, OptimalFieldsOrder); } /// \brief Look for arrays of overly padded types. If the padding of the @@ -199,22 +203,30 @@ public: /// 7. Add tail padding by rounding the current offset up to the structure /// alignment. Track the amount of padding added. - static CharUnits calculateOptimalPad(const RecordDecl *RD, - const ASTContext &ASTContext, - const ASTRecordLayout &RL) { - struct CharUnitPair { + static std::pair<CharUnits, SmallVector<const FieldDecl *, 20>> + calculateOptimalPad(const RecordDecl *RD, const ASTContext &ASTContext, + const ASTRecordLayout &RL) { + struct FieldInfo { CharUnits Align; CharUnits Size; - bool operator<(const CharUnitPair &RHS) const { + const FieldDecl *Field; + bool operator<(const FieldInfo &RHS) const { // Order from small alignments to large alignments, // then large sizes to small sizes. - return std::make_pair(Align, -Size) < - std::make_pair(RHS.Align, -RHS.Size); + // then large field indices to small field indices + return std::make_tuple(Align, -Size, + Field ? -static_cast<int>(Field->getFieldIndex()) + : 0) < + std::make_tuple( + RHS.Align, -RHS.Size, + RHS.Field ? -static_cast<int>(RHS.Field->getFieldIndex()) + : 0); } }; - SmallVector<CharUnitPair, 20> Fields; + SmallVector<FieldInfo, 20> Fields; auto GatherSizesAndAlignments = [](const FieldDecl *FD) { - CharUnitPair RetVal; + FieldInfo RetVal; + RetVal.Field = FD; auto &Ctx = FD->getASTContext(); std::tie(RetVal.Size, RetVal.Align) = Ctx.getTypeInfoInChars(FD->getType()); @@ -226,14 +238,13 @@ public: std::transform(RD->field_begin(), RD->field_end(), std::back_inserter(Fields), GatherSizesAndAlignments); std::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 // could pack more bytes in to a base class's tail padding. CharUnits NewOffset = ASTContext.toCharUnitsFromBits(RL.getFieldOffset(0)); CharUnits NewPad; - + SmallVector<const FieldDecl *, 20> OptimalFieldsOrder; while (!Fields.empty()) { unsigned TrailingZeros = llvm::countTrailingZeros((unsigned long long)NewOffset.getQuantity()); @@ -242,7 +253,7 @@ public: // our long long (and CharUnits internal type) negative. So shift 62. long long CurAlignmentBits = 1ull << (std::min)(TrailingZeros, 62u); CharUnits CurAlignment = CharUnits::fromQuantity(CurAlignmentBits); - CharUnitPair InsertPoint = {CurAlignment, CharUnits::Zero()}; + FieldInfo InsertPoint = {CurAlignment, CharUnits::Zero(), nullptr}; auto CurBegin = Fields.begin(); auto CurEnd = Fields.end(); @@ -255,6 +266,7 @@ public: // We found a field that we can layout with the current alignment. --Iter; NewOffset += Iter->Size; + OptimalFieldsOrder.push_back(Iter->Field); Fields.erase(Iter); } else { // We are poorly aligned, and we need to pad in order to layout another @@ -268,18 +280,18 @@ public: // Calculate tail padding. CharUnits NewSize = NewOffset.alignTo(RL.getAlignment()); NewPad += NewSize - NewOffset; - return NewPad; + return {NewPad, std::move(OptimalFieldsOrder)}; } - void reportRecord(const RecordDecl *RD, CharUnits BaselinePad, - CharUnits TargetPad) const { + void reportRecord( + const RecordDecl *RD, CharUnits BaselinePad, CharUnits OptimalPad, + const SmallVector<const FieldDecl *, 20> &OptimalFieldsOrder) const { if (!PaddingBug) PaddingBug = llvm::make_unique<BugType>(this, "Excessive Padding", "Performance"); SmallString<100> Buf; llvm::raw_svector_ostream Os(Buf); - Os << "Excessive padding in '"; Os << QualType::getAsString(RD->getTypeForDecl(), Qualifiers()) << "'"; @@ -294,16 +306,18 @@ public: } Os << " (" << BaselinePad.getQuantity() << " padding bytes, where " - << TargetPad.getQuantity() << " is optimal). Consider reordering " - << "the fields or adding explicit padding members."; + << OptimalPad.getQuantity() << " is optimal). \n" + << "Optimal fields order: \n"; + for (const auto *FD : OptimalFieldsOrder) + Os << FD->getName() << ", \n"; + Os << "consider reordering the fields or adding explicit padding " + "members."; PathDiagnosticLocation CELoc = PathDiagnosticLocation::create(RD, BR->getSourceManager()); - auto Report = llvm::make_unique<BugReport>(*PaddingBug, Os.str(), CELoc); Report->setDeclWithIssue(RD); Report->addRange(RD->getSourceRange()); - BR->emitReport(std::move(Report)); } }; diff --git a/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp b/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp index df5118806bff4..8caf6df4d970e 100644 --- a/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp @@ -19,7 +19,6 @@ #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/CheckerManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" -#include "llvm/ADT/SmallVector.h" using namespace clang; using namespace ento; diff --git a/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp b/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp index 28a4a083ea3cd..7ef79c683c49e 100644 --- a/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp @@ -18,7 +18,6 @@ #include "clang/StaticAnalyzer/Core/CheckerManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" -#include "llvm/ADT/ImmutableList.h" using namespace clang; using namespace ento; diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp index b646127cfae7d..204b0a6c468b5 100644 --- a/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp @@ -740,7 +740,7 @@ public: ObjCAllocRetE(gcenabled ? RetEffect::MakeGCNotOwned() : (usesARC ? RetEffect::MakeNotOwned(RetEffect::ObjC) - : RetEffect::MakeOwned(RetEffect::ObjC, true))), + : RetEffect::MakeOwned(RetEffect::ObjC))), ObjCInitRetE(gcenabled ? RetEffect::MakeGCNotOwned() : (usesARC ? RetEffect::MakeNotOwned(RetEffect::ObjC) @@ -953,7 +953,10 @@ void RetainSummaryManager::updateSummaryForCall(const RetainSummary *&S, if (IdentifierInfo *Name = FC->getDecl()->getIdentifier()) { // When the CGBitmapContext is deallocated, the callback here will free // the associated data buffer. - if (Name->isStr("CGBitmapContextCreateWithData")) + // The callback in dispatch_data_create frees the buffer, but not + // the data object. + if (Name->isStr("CGBitmapContextCreateWithData") || + Name->isStr("dispatch_data_create")) RE = S->getRetEffect(); } } @@ -1086,7 +1089,7 @@ RetainSummaryManager::getFunctionSummary(const FunctionDecl *FD) { FName == "IOOpenFirmwarePathMatching") { // Part of <rdar://problem/6961230>. (IOKit) // This should be addressed using a API table. - S = getPersistentSummary(RetEffect::MakeOwned(RetEffect::CF, true), + S = getPersistentSummary(RetEffect::MakeOwned(RetEffect::CF), DoNothing, DoNothing); } else if (FName == "IOServiceGetMatchingService" || FName == "IOServiceGetMatchingServices") { @@ -1116,7 +1119,7 @@ RetainSummaryManager::getFunctionSummary(const FunctionDecl *FD) { // passed to CGBitmapContextCreateWithData is released via // a callback and doing full IPA to make sure this is done correctly. ScratchArgs = AF.add(ScratchArgs, 8, StopTracking); - S = getPersistentSummary(RetEffect::MakeOwned(RetEffect::CF, true), + S = getPersistentSummary(RetEffect::MakeOwned(RetEffect::CF), DoNothing, DoNothing); } else if (FName == "CVPixelBufferCreateWithPlanarBytes") { // FIXES: <rdar://problem/7283567> @@ -1126,6 +1129,14 @@ RetainSummaryManager::getFunctionSummary(const FunctionDecl *FD) { // correctly. ScratchArgs = AF.add(ScratchArgs, 12, StopTracking); S = getPersistentSummary(RetEffect::MakeNoRet(), DoNothing, DoNothing); + } else if (FName == "VTCompressionSessionEncodeFrame") { + // The context argument passed to VTCompressionSessionEncodeFrame() + // is passed to the callback specified when creating the session + // (e.g. with VTCompressionSessionCreate()) which can release it. + // To account for this possibility, conservatively stop tracking + // the context. + ScratchArgs = AF.add(ScratchArgs, 5, StopTracking); + S = getPersistentSummary(RetEffect::MakeNoRet(), DoNothing, DoNothing); } else if (FName == "dispatch_set_context" || FName == "xpc_connection_set_context") { // <rdar://problem/11059275> - The analyzer currently doesn't have @@ -1171,8 +1182,9 @@ RetainSummaryManager::getFunctionSummary(const FunctionDecl *FD) { break; } - // For CoreGraphics ('CG') types. - if (cocoa::isRefType(RetTy, "CG", FName)) { + // For CoreGraphics ('CG') and CoreVideo ('CV') types. + if (cocoa::isRefType(RetTy, "CG", FName) || + cocoa::isRefType(RetTy, "CV", FName)) { if (isRetain(FD, FName)) S = getUnarySummary(FT, cfretain); else @@ -1283,7 +1295,7 @@ const RetainSummary * RetainSummaryManager::getCFSummaryCreateRule(const FunctionDecl *FD) { assert (ScratchArgs.isEmpty()); - return getPersistentSummary(RetEffect::MakeOwned(RetEffect::CF, true)); + return getPersistentSummary(RetEffect::MakeOwned(RetEffect::CF)); } const RetainSummary * @@ -1313,7 +1325,7 @@ RetainSummaryManager::getRetEffectFromAnnotations(QualType RetTy, } if (D->hasAttr<CFReturnsRetainedAttr>()) - return RetEffect::MakeOwned(RetEffect::CF, true); + return RetEffect::MakeOwned(RetEffect::CF); if (D->hasAttr<CFReturnsNotRetainedAttr>()) return RetEffect::MakeNotOwned(RetEffect::CF); @@ -1426,7 +1438,7 @@ RetainSummaryManager::getStandardMethodSummary(const ObjCMethodDecl *MD, case OMF_new: case OMF_copy: case OMF_mutableCopy: - ResultEff = RetEffect::MakeOwned(RetEffect::CF, true); + ResultEff = RetEffect::MakeOwned(RetEffect::CF); break; default: ResultEff = RetEffect::MakeNotOwned(RetEffect::CF); @@ -1448,7 +1460,7 @@ RetainSummaryManager::getStandardMethodSummary(const ObjCMethodDecl *MD, if (cocoa::isCocoaObjectRef(RetTy)) ResultEff = ObjCAllocRetE; else if (coreFoundation::isCFObjectRef(RetTy)) - ResultEff = RetEffect::MakeOwned(RetEffect::CF, true); + ResultEff = RetEffect::MakeOwned(RetEffect::CF); break; case OMF_autorelease: ReceiverEff = Autorelease; @@ -1579,7 +1591,7 @@ void RetainSummaryManager::InitializeMethodSummaries() { // The next methods are allocators. const RetainSummary *AllocSumm = getPersistentSummary(ObjCAllocRetE); const RetainSummary *CFAllocSumm = - getPersistentSummary(RetEffect::MakeOwned(RetEffect::CF, true)); + getPersistentSummary(RetEffect::MakeOwned(RetEffect::CF)); // Create the "retain" selector. RetEffect NoRet = RetEffect::MakeNoRet(); @@ -1978,11 +1990,23 @@ PathDiagnosticPiece *CFRefReportVisitor::VisitNode(const ExplodedNode *N, } if (CurrV.getObjKind() == RetEffect::CF) { - os << " returns a Core Foundation object with a "; + if (Sym->getType().isNull()) { + os << " returns a Core Foundation object with a "; + } else { + os << " returns a Core Foundation object of type " + << Sym->getType().getAsString() << " with a "; + } } else { assert (CurrV.getObjKind() == RetEffect::ObjC); - os << " returns an Objective-C object with a "; + QualType T = Sym->getType(); + if (T.isNull() || !isa<ObjCObjectPointerType>(T)) { + os << " returns an Objective-C object with a "; + } else { + const ObjCObjectPointerType *PT = cast<ObjCObjectPointerType>(T); + os << " returns an instance of " + << PT->getPointeeType().getAsString() << " with a "; + } } if (CurrV.isOwned()) { @@ -2358,10 +2382,15 @@ CFRefLeakReportVisitor::getEndPath(BugReporterContext &BRC, os << "that is annotated as NS_RETURNS_NOT_RETAINED"; else { if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D)) { - os << "whose name ('" << MD->getSelector().getAsString() - << "') does not start with 'copy', 'mutableCopy', 'alloc' or 'new'." - " This violates the naming convention rules" - " given in the Memory Management Guide for Cocoa"; + if (BRC.getASTContext().getLangOpts().ObjCAutoRefCount) { + os << "managed by Automatic Reference Counting"; + } else { + os << "whose name ('" << MD->getSelector().getAsString() + << "') does not start with " + "'copy', 'mutableCopy', 'alloc' or 'new'." + " This violates the naming convention rules" + " given in the Memory Management Guide for Cocoa"; + } } else { const FunctionDecl *FD = cast<FunctionDecl>(D); @@ -2417,12 +2446,7 @@ CFRefLeakReport::CFRefLeakReport(CFRefBug &D, const LangOptions &LOpts, // FIXME: This will crash the analyzer if an allocation comes from an // implicit call (ex: a destructor call). // (Currently there are no such allocations in Cocoa, though.) - const Stmt *AllocStmt = nullptr; - ProgramPoint P = AllocNode->getLocation(); - if (Optional<CallExitEnd> Exit = P.getAs<CallExitEnd>()) - AllocStmt = Exit->getCalleeContext()->getCallSite(); - else - AllocStmt = P.castAs<PostStmt>().getStmt(); + const Stmt *AllocStmt = PathDiagnosticLocation::getStmt(AllocNode); assert(AllocStmt && "Cannot find allocation statement"); PathDiagnosticLocation AllocLocation = @@ -2640,10 +2664,6 @@ public: ArrayRef<const MemRegion *> Regions, const CallEvent *Call) const; - bool wantsRegionChangeUpdate(ProgramStateRef state) const { - return true; - } - void checkPreStmt(const ReturnStmt *S, CheckerContext &C) const; void checkReturnWithRetEffect(const ReturnStmt *S, CheckerContext &C, ExplodedNode *Pred, RetEffect RE, RefVal X, @@ -3071,7 +3091,6 @@ void RetainCountChecker::checkSummary(const RetainSummary &Summ, // No work necessary. break; - case RetEffect::OwnedAllocatedSymbol: case RetEffect::OwnedSymbol: { SymbolRef Sym = CallOrMsg.getReturnValue().getAsSymbol(); if (!Sym) @@ -3372,12 +3391,13 @@ bool RetainCountChecker::evalCall(const CallExpr *CE, CheckerContext &C) const { // Handle: id NSMakeCollectable(CFTypeRef) canEval = II->isStr("NSMakeCollectable"); } else if (ResultTy->isPointerType()) { - // Handle: (CF|CG)Retain + // Handle: (CF|CG|CV)Retain // CFAutorelease // CFMakeCollectable // It's okay to be a little sloppy here (CGMakeCollectable doesn't exist). if (cocoa::isRefType(ResultTy, "CF", FName) || - cocoa::isRefType(ResultTy, "CG", FName)) { + cocoa::isRefType(ResultTy, "CG", FName) || + cocoa::isRefType(ResultTy, "CV", FName)) { canEval = isRetain(FD, FName) || isAutorelease(FD, FName) || isMakeCollectable(FD, FName); } @@ -3866,7 +3886,7 @@ void RetainCountChecker::checkEndFunction(CheckerContext &Ctx) const { // Don't process anything within synthesized bodies. const LocationContext *LCtx = Pred->getLocationContext(); if (LCtx->getAnalysisDeclContext()->isBodyAutosynthesized()) { - assert(LCtx->getParent()); + assert(!LCtx->inTopFrame()); return; } diff --git a/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp new file mode 100644 index 0000000000000..93ad17cffb34d --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp @@ -0,0 +1,1055 @@ +//=== StdLibraryFunctionsChecker.cpp - Model standard functions -*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This checker improves modeling of a few simple library functions. +// It does not generate warnings. +// +// This checker provides a specification format - `FunctionSummaryTy' - and +// contains descriptions of some library functions in this format. Each +// specification contains a list of branches for splitting the program state +// upon call, and range constraints on argument and return-value symbols that +// are satisfied on each branch. This spec can be expanded to include more +// items, like external effects of the function. +// +// The main difference between this approach and the body farms technique is +// in more explicit control over how many branches are produced. For example, +// consider standard C function `ispunct(int x)', which returns a non-zero value +// iff `x' is a punctuation character, that is, when `x' is in range +// ['!', '/'] [':', '@'] U ['[', '\`'] U ['{', '~']. +// `FunctionSummaryTy' provides only two branches for this function. However, +// any attempt to describe this range with if-statements in the body farm +// would result in many more branches. Because each branch needs to be analyzed +// independently, this significantly reduces performance. Additionally, +// once we consider a branch on which `x' is in range, say, ['!', '/'], +// we assume that such branch is an important separate path through the program, +// which may lead to false positives because considering this particular path +// was not consciously intended, and therefore it might have been unreachable. +// +// This checker uses eval::Call for modeling "pure" functions, for which +// their `FunctionSummaryTy' is a precise model. This avoids unnecessary +// invalidation passes. Conflicts with other checkers are unlikely because +// if the function has no other effects, other checkers would probably never +// want to improve upon the modeling done by this checker. +// +// Non-"pure" functions, for which only partial improvement over the default +// behavior is expected, are modeled via check::PostCall, non-intrusively. +// +// The following standard C functions are currently supported: +// +// fgetc getline isdigit isupper +// fread isalnum isgraph isxdigit +// fwrite isalpha islower read +// getc isascii isprint write +// getchar isblank ispunct +// getdelim iscntrl isspace +// +//===----------------------------------------------------------------------===// + +#include "ClangSACheckers.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 clang::ento; + +namespace { +class StdLibraryFunctionsChecker : public Checker<check::PostCall, eval::Call> { + /// Below is a series of typedefs necessary to define function specs. + /// We avoid nesting types here because each additional qualifier + /// would need to be repeated in every function spec. + struct FunctionSummaryTy; + + /// Specify how much the analyzer engine should entrust modeling this function + /// to us. If he doesn't, he performs additional invalidations. + enum InvalidationKindTy { NoEvalCall, EvalCallAsPure }; + + /// A pair of ValueRangeKindTy and IntRangeVectorTy would describe a range + /// imposed on a particular argument or return value symbol. + /// + /// Given a range, should the argument stay inside or outside this range? + /// The special `ComparesToArgument' value indicates that we should + /// impose a constraint that involves other argument or return value symbols. + enum ValueRangeKindTy { OutOfRange, WithinRange, ComparesToArgument }; + + // The universal integral type to use in value range descriptions. + // Unsigned to make sure overflows are well-defined. + typedef uint64_t RangeIntTy; + + /// Normally, describes a single range constraint, eg. {{0, 1}, {3, 4}} is + /// a non-negative integer, which less than 5 and not equal to 2. For + /// `ComparesToArgument', holds information about how exactly to compare to + /// the argument. + typedef std::vector<std::pair<RangeIntTy, RangeIntTy>> IntRangeVectorTy; + + /// A reference to an argument or return value by its number. + /// ArgNo in CallExpr and CallEvent is defined as Unsigned, but + /// obviously uint32_t should be enough for all practical purposes. + typedef uint32_t ArgNoTy; + static const ArgNoTy Ret = std::numeric_limits<ArgNoTy>::max(); + + /// Incapsulates a single range on a single symbol within a branch. + class ValueRange { + ArgNoTy ArgNo; // Argument to which we apply the range. + ValueRangeKindTy Kind; // Kind of range definition. + IntRangeVectorTy Args; // Polymorphic arguments. + + public: + ValueRange(ArgNoTy ArgNo, ValueRangeKindTy Kind, + const IntRangeVectorTy &Args) + : ArgNo(ArgNo), Kind(Kind), Args(Args) {} + + ArgNoTy getArgNo() const { return ArgNo; } + ValueRangeKindTy getKind() const { return Kind; } + + BinaryOperator::Opcode getOpcode() const { + assert(Kind == ComparesToArgument); + assert(Args.size() == 1); + BinaryOperator::Opcode Op = + static_cast<BinaryOperator::Opcode>(Args[0].first); + assert(BinaryOperator::isComparisonOp(Op) && + "Only comparison ops are supported for ComparesToArgument"); + return Op; + } + + ArgNoTy getOtherArgNo() const { + assert(Kind == ComparesToArgument); + assert(Args.size() == 1); + return static_cast<ArgNoTy>(Args[0].second); + } + + const IntRangeVectorTy &getRanges() const { + assert(Kind != ComparesToArgument); + return Args; + } + + // We avoid creating a virtual apply() method because + // it makes initializer lists harder to write. + private: + ProgramStateRef + applyAsOutOfRange(ProgramStateRef State, const CallEvent &Call, + const FunctionSummaryTy &Summary) const; + ProgramStateRef + applyAsWithinRange(ProgramStateRef State, const CallEvent &Call, + const FunctionSummaryTy &Summary) const; + ProgramStateRef + applyAsComparesToArgument(ProgramStateRef State, const CallEvent &Call, + const FunctionSummaryTy &Summary) const; + + public: + ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call, + const FunctionSummaryTy &Summary) const { + switch (Kind) { + case OutOfRange: + return applyAsOutOfRange(State, Call, Summary); + case WithinRange: + return applyAsWithinRange(State, Call, Summary); + case ComparesToArgument: + return applyAsComparesToArgument(State, Call, Summary); + } + llvm_unreachable("Unknown ValueRange kind!"); + } + }; + + /// The complete list of ranges that defines a single branch. + typedef std::vector<ValueRange> ValueRangeSet; + + /// Includes information about function prototype (which is necessary to + /// ensure we're modeling the right function and casting values properly), + /// approach to invalidation, and a list of branches - essentially, a list + /// of list of ranges - essentially, a list of lists of lists of segments. + struct FunctionSummaryTy { + const std::vector<QualType> ArgTypes; + const QualType RetType; + const InvalidationKindTy InvalidationKind; + const std::vector<ValueRangeSet> Ranges; + + private: + static void assertTypeSuitableForSummary(QualType T) { + assert(!T->isVoidType() && + "We should have had no significant void types in the spec"); + assert(T.isCanonical() && + "We should only have canonical types in the spec"); + // FIXME: lift this assert (but not the ones above!) + assert(T->isIntegralOrEnumerationType() && + "We only support integral ranges in the spec"); + } + + public: + QualType getArgType(ArgNoTy ArgNo) const { + QualType T = (ArgNo == Ret) ? RetType : ArgTypes[ArgNo]; + assertTypeSuitableForSummary(T); + return T; + } + + /// Try our best to figure out if the call expression is the call of + /// *the* library function to which this specification applies. + bool matchesCall(const CallExpr *CE) const; + }; + + // The same function (as in, function identifier) may have different + // summaries assigned to it, with different argument and return value types. + // We call these "variants" of the function. This can be useful for handling + // C++ function overloads, and also it can be used when the same function + // may have different definitions on different platforms. + typedef std::vector<FunctionSummaryTy> FunctionVariantsTy; + + // The map of all functions supported by the checker. It is initialized + // lazily, and it doesn't change after initialization. + typedef llvm::StringMap<FunctionVariantsTy> FunctionSummaryMapTy; + mutable FunctionSummaryMapTy FunctionSummaryMap; + + // Auxiliary functions to support ArgNoTy within all structures + // in a unified manner. + static QualType getArgType(const FunctionSummaryTy &Summary, ArgNoTy ArgNo) { + return Summary.getArgType(ArgNo); + } + static QualType getArgType(const CallEvent &Call, ArgNoTy ArgNo) { + return ArgNo == Ret ? Call.getResultType().getCanonicalType() + : Call.getArgExpr(ArgNo)->getType().getCanonicalType(); + } + static QualType getArgType(const CallExpr *CE, ArgNoTy ArgNo) { + return ArgNo == Ret ? CE->getType().getCanonicalType() + : CE->getArg(ArgNo)->getType().getCanonicalType(); + } + static SVal getArgSVal(const CallEvent &Call, ArgNoTy ArgNo) { + return ArgNo == Ret ? Call.getReturnValue() : Call.getArgSVal(ArgNo); + } + +public: + void checkPostCall(const CallEvent &Call, CheckerContext &C) const; + bool evalCall(const CallExpr *CE, CheckerContext &C) const; + +private: + Optional<FunctionSummaryTy> findFunctionSummary(const FunctionDecl *FD, + const CallExpr *CE, + CheckerContext &C) const; + + void initFunctionSummaries(BasicValueFactory &BVF) const; +}; +} // end of anonymous namespace + +ProgramStateRef StdLibraryFunctionsChecker::ValueRange::applyAsOutOfRange( + ProgramStateRef State, const CallEvent &Call, + const FunctionSummaryTy &Summary) const { + + ProgramStateManager &Mgr = State->getStateManager(); + SValBuilder &SVB = Mgr.getSValBuilder(); + BasicValueFactory &BVF = SVB.getBasicValueFactory(); + ConstraintManager &CM = Mgr.getConstraintManager(); + QualType T = getArgType(Summary, getArgNo()); + SVal V = getArgSVal(Call, getArgNo()); + + if (auto N = V.getAs<NonLoc>()) { + const IntRangeVectorTy &R = getRanges(); + size_t E = R.size(); + for (size_t I = 0; I != E; ++I) { + const llvm::APSInt &Min = BVF.getValue(R[I].first, T); + const llvm::APSInt &Max = BVF.getValue(R[I].second, T); + assert(Min <= Max); + State = CM.assumeInclusiveRange(State, *N, Min, Max, false); + if (!State) + break; + } + } + + return State; +} + +ProgramStateRef +StdLibraryFunctionsChecker::ValueRange::applyAsWithinRange( + ProgramStateRef State, const CallEvent &Call, + const FunctionSummaryTy &Summary) const { + + ProgramStateManager &Mgr = State->getStateManager(); + SValBuilder &SVB = Mgr.getSValBuilder(); + BasicValueFactory &BVF = SVB.getBasicValueFactory(); + ConstraintManager &CM = Mgr.getConstraintManager(); + QualType T = getArgType(Summary, getArgNo()); + SVal V = getArgSVal(Call, getArgNo()); + + // "WithinRange R" is treated as "outside [T_MIN, T_MAX] \ R". + // We cut off [T_MIN, min(R) - 1] and [max(R) + 1, T_MAX] if necessary, + // and then cut away all holes in R one by one. + if (auto N = V.getAs<NonLoc>()) { + const IntRangeVectorTy &R = getRanges(); + size_t E = R.size(); + + const llvm::APSInt &MinusInf = BVF.getMinValue(T); + const llvm::APSInt &PlusInf = BVF.getMaxValue(T); + + const llvm::APSInt &Left = BVF.getValue(R[0].first - 1ULL, T); + if (Left != PlusInf) { + assert(MinusInf <= Left); + State = CM.assumeInclusiveRange(State, *N, MinusInf, Left, false); + if (!State) + return nullptr; + } + + const llvm::APSInt &Right = BVF.getValue(R[E - 1].second + 1ULL, T); + if (Right != MinusInf) { + assert(Right <= PlusInf); + State = CM.assumeInclusiveRange(State, *N, Right, PlusInf, false); + if (!State) + return nullptr; + } + + for (size_t I = 1; I != E; ++I) { + const llvm::APSInt &Min = BVF.getValue(R[I - 1].second + 1ULL, T); + const llvm::APSInt &Max = BVF.getValue(R[I].first - 1ULL, T); + assert(Min <= Max); + State = CM.assumeInclusiveRange(State, *N, Min, Max, false); + if (!State) + return nullptr; + } + } + + return State; +} + +ProgramStateRef +StdLibraryFunctionsChecker::ValueRange::applyAsComparesToArgument( + ProgramStateRef State, const CallEvent &Call, + const FunctionSummaryTy &Summary) const { + + ProgramStateManager &Mgr = State->getStateManager(); + SValBuilder &SVB = Mgr.getSValBuilder(); + QualType CondT = SVB.getConditionType(); + QualType T = getArgType(Summary, getArgNo()); + SVal V = getArgSVal(Call, getArgNo()); + + BinaryOperator::Opcode Op = getOpcode(); + ArgNoTy OtherArg = getOtherArgNo(); + SVal OtherV = getArgSVal(Call, OtherArg); + QualType OtherT = getArgType(Call, OtherArg); + // Note: we avoid integral promotion for comparison. + OtherV = SVB.evalCast(OtherV, T, OtherT); + if (auto CompV = SVB.evalBinOp(State, Op, V, OtherV, CondT) + .getAs<DefinedOrUnknownSVal>()) + State = State->assume(*CompV, true); + return State; +} + +void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl()); + if (!FD) + return; + + const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr()); + if (!CE) + return; + + Optional<FunctionSummaryTy> FoundSummary = findFunctionSummary(FD, CE, C); + if (!FoundSummary) + return; + + // Now apply ranges. + const FunctionSummaryTy &Summary = *FoundSummary; + ProgramStateRef State = C.getState(); + + for (const auto &VRS: Summary.Ranges) { + ProgramStateRef NewState = State; + for (const auto &VR: VRS) { + NewState = VR.apply(NewState, Call, Summary); + if (!NewState) + break; + } + + if (NewState && NewState != State) + C.addTransition(NewState); + } +} + +bool StdLibraryFunctionsChecker::evalCall(const CallExpr *CE, + CheckerContext &C) const { + const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(CE->getCalleeDecl()); + if (!FD) + return false; + + Optional<FunctionSummaryTy> FoundSummary = findFunctionSummary(FD, CE, C); + if (!FoundSummary) + return false; + + const FunctionSummaryTy &Summary = *FoundSummary; + switch (Summary.InvalidationKind) { + case EvalCallAsPure: { + ProgramStateRef State = C.getState(); + const LocationContext *LC = C.getLocationContext(); + SVal V = C.getSValBuilder().conjureSymbolVal( + CE, LC, CE->getType().getCanonicalType(), C.blockCount()); + State = State->BindExpr(CE, LC, V); + C.addTransition(State); + return true; + } + case NoEvalCall: + // Summary tells us to avoid performing eval::Call. The function is possibly + // evaluated by another checker, or evaluated conservatively. + return false; + } + llvm_unreachable("Unknown invalidation kind!"); +} + +bool StdLibraryFunctionsChecker::FunctionSummaryTy::matchesCall( + const CallExpr *CE) const { + // Check number of arguments: + if (CE->getNumArgs() != ArgTypes.size()) + return false; + + // Check return type if relevant: + if (!RetType.isNull() && RetType != CE->getType().getCanonicalType()) + return false; + + // Check argument types when relevant: + for (size_t I = 0, E = ArgTypes.size(); I != E; ++I) { + QualType FormalT = ArgTypes[I]; + // Null type marks irrelevant arguments. + if (FormalT.isNull()) + continue; + + assertTypeSuitableForSummary(FormalT); + + QualType ActualT = StdLibraryFunctionsChecker::getArgType(CE, I); + assert(ActualT.isCanonical()); + if (ActualT != FormalT) + return false; + } + + return true; +} + +Optional<StdLibraryFunctionsChecker::FunctionSummaryTy> +StdLibraryFunctionsChecker::findFunctionSummary(const FunctionDecl *FD, + const CallExpr *CE, + CheckerContext &C) const { + // Note: we cannot always obtain FD from CE + // (eg. virtual call, or call by pointer). + assert(CE); + + if (!FD) + return None; + + SValBuilder &SVB = C.getSValBuilder(); + BasicValueFactory &BVF = SVB.getBasicValueFactory(); + initFunctionSummaries(BVF); + + std::string Name = FD->getQualifiedNameAsString(); + if (Name.empty() || !C.isCLibraryFunction(FD, Name)) + return None; + + auto FSMI = FunctionSummaryMap.find(Name); + if (FSMI == FunctionSummaryMap.end()) + return None; + + // Verify that function signature matches the spec in advance. + // Otherwise we might be modeling the wrong function. + // Strict checking is important because we will be conducting + // very integral-type-sensitive operations on arguments and + // return values. + const FunctionVariantsTy &SpecVariants = FSMI->second; + for (const FunctionSummaryTy &Spec : SpecVariants) + if (Spec.matchesCall(CE)) + return Spec; + + return None; +} + +void StdLibraryFunctionsChecker::initFunctionSummaries( + BasicValueFactory &BVF) const { + if (!FunctionSummaryMap.empty()) + return; + + ASTContext &ACtx = BVF.getContext(); + + // These types are useful for writing specifications quickly, + // New specifications should probably introduce more types. + // Some types are hard to obtain from the AST, eg. "ssize_t". + // In such cases it should be possible to provide multiple variants + // of function summary for common cases (eg. ssize_t could be int or long + // or long long, so three summary variants would be enough). + // Of course, function variants are also useful for C++ overloads. + QualType Irrelevant; // A placeholder, whenever we do not care about the type. + QualType IntTy = ACtx.IntTy; + QualType LongTy = ACtx.LongTy; + QualType LongLongTy = ACtx.LongLongTy; + QualType SizeTy = ACtx.getSizeType(); + + RangeIntTy IntMax = BVF.getMaxValue(IntTy).getLimitedValue(); + RangeIntTy LongMax = BVF.getMaxValue(LongTy).getLimitedValue(); + RangeIntTy LongLongMax = BVF.getMaxValue(LongLongTy).getLimitedValue(); + + // We are finally ready to define specifications for all supported functions. + // + // The signature needs to have the correct number of arguments. + // However, we insert `Irrelevant' when the type is insignificant. + // + // Argument ranges should always cover all variants. If return value + // is completely unknown, omit it from the respective range set. + // + // All types in the spec need to be canonical. + // + // Every item in the list of range sets represents a particular + // execution path the analyzer would need to explore once + // the call is modeled - a new program state is constructed + // for every range set, and each range line in the range set + // corresponds to a specific constraint within this state. + // + // Upon comparing to another argument, the other argument is casted + // to the current argument's type. This avoids proper promotion but + // seems useful. For example, read() receives size_t argument, + // and its return value, which is of type ssize_t, cannot be greater + // than this argument. If we made a promotion, and the size argument + // is equal to, say, 10, then we'd impose a range of [0, 10] on the + // return value, however the correct range is [-1, 10]. + // + // Please update the list of functions in the header after editing! + // + // The format is as follows: + // + //{ "function name", + // { spec: + // { argument types list, ... }, + // return type, purity, { range set list: + // { range list: + // { argument index, within or out of, {{from, to}, ...} }, + // { argument index, compares to argument, {{how, which}} }, + // ... + // } + // } + // } + //} + +#define SUMMARY_WITH_VARIANTS(identifier) {#identifier, { +#define END_SUMMARY_WITH_VARIANTS }}, +#define VARIANT(argument_types, return_type, invalidation_approach) \ + { argument_types, return_type, invalidation_approach, { +#define END_VARIANT } }, +#define SUMMARY(identifier, argument_types, return_type, \ + invalidation_approach) \ + { #identifier, { { argument_types, return_type, invalidation_approach, { +#define END_SUMMARY } } } }, +#define ARGUMENT_TYPES(...) { __VA_ARGS__ } +#define RETURN_TYPE(x) x +#define INVALIDATION_APPROACH(x) x +#define CASE { +#define END_CASE }, +#define ARGUMENT_CONDITION(argument_number, condition_kind) \ + { argument_number, condition_kind, { +#define END_ARGUMENT_CONDITION }}, +#define RETURN_VALUE_CONDITION(condition_kind) \ + { Ret, condition_kind, { +#define END_RETURN_VALUE_CONDITION }}, +#define ARG_NO(x) x##U +#define RANGE(x, y) { x, y }, +#define SINGLE_VALUE(x) RANGE(x, x) +#define IS_LESS_THAN(arg) { BO_LE, arg } + + FunctionSummaryMap = { + // The isascii() family of functions. + SUMMARY(isalnum, ARGUMENT_TYPES(IntTy), RETURN_TYPE(IntTy), + INVALIDATION_APPROACH(EvalCallAsPure)) + CASE // Boils down to isupper() or islower() or isdigit() + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE('0', '9') + RANGE('A', 'Z') + RANGE('a', 'z') + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(OutOfRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + CASE // The locale-specific range. + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE(128, 255) + END_ARGUMENT_CONDITION + // No post-condition. We are completely unaware of + // locale-specific return values. + END_CASE + CASE + ARGUMENT_CONDITION(ARG_NO(0), OutOfRange) + RANGE('0', '9') + RANGE('A', 'Z') + RANGE('a', 'z') + RANGE(128, 255) + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + SUMMARY(isalpha, ARGUMENT_TYPES(IntTy), RETURN_TYPE(IntTy), + INVALIDATION_APPROACH(EvalCallAsPure)) + CASE // isupper() or islower(). Note that 'Z' is less than 'a'. + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE('A', 'Z') + RANGE('a', 'z') + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(OutOfRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + CASE // The locale-specific range. + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE(128, 255) + END_ARGUMENT_CONDITION + END_CASE + CASE // Other. + ARGUMENT_CONDITION(ARG_NO(0), OutOfRange) + RANGE('A', 'Z') + RANGE('a', 'z') + RANGE(128, 255) + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + SUMMARY(isascii, ARGUMENT_TYPES(IntTy), RETURN_TYPE(IntTy), + INVALIDATION_APPROACH(EvalCallAsPure)) + CASE // Is ASCII. + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE(0, 127) + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(OutOfRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + CASE + ARGUMENT_CONDITION(ARG_NO(0), OutOfRange) + RANGE(0, 127) + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + SUMMARY(isblank, ARGUMENT_TYPES(IntTy), RETURN_TYPE(IntTy), + INVALIDATION_APPROACH(EvalCallAsPure)) + CASE + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + SINGLE_VALUE('\t') + SINGLE_VALUE(' ') + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(OutOfRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + CASE + ARGUMENT_CONDITION(ARG_NO(0), OutOfRange) + SINGLE_VALUE('\t') + SINGLE_VALUE(' ') + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + SUMMARY(iscntrl, ARGUMENT_TYPES(IntTy), RETURN_TYPE(IntTy), + INVALIDATION_APPROACH(EvalCallAsPure)) + CASE // 0..31 or 127 + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE(0, 32) + SINGLE_VALUE(127) + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(OutOfRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + CASE + ARGUMENT_CONDITION(ARG_NO(0), OutOfRange) + RANGE(0, 32) + SINGLE_VALUE(127) + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + SUMMARY(isdigit, ARGUMENT_TYPES(IntTy), RETURN_TYPE(IntTy), + INVALIDATION_APPROACH(EvalCallAsPure)) + CASE // Is a digit. + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE('0', '9') + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(OutOfRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + CASE + ARGUMENT_CONDITION(ARG_NO(0), OutOfRange) + RANGE('0', '9') + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + SUMMARY(isgraph, ARGUMENT_TYPES(IntTy), RETURN_TYPE(IntTy), + INVALIDATION_APPROACH(EvalCallAsPure)) + CASE + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE(33, 126) + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(OutOfRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + CASE + ARGUMENT_CONDITION(ARG_NO(0), OutOfRange) + RANGE(33, 126) + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + SUMMARY(islower, ARGUMENT_TYPES(IntTy), RETURN_TYPE(IntTy), + INVALIDATION_APPROACH(EvalCallAsPure)) + CASE // Is certainly lowercase. + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE('a', 'z') + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(OutOfRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + CASE // Is ascii but not lowercase. + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE(0, 127) + END_ARGUMENT_CONDITION + ARGUMENT_CONDITION(ARG_NO(0), OutOfRange) + RANGE('a', 'z') + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + CASE // The locale-specific range. + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE(128, 255) + END_ARGUMENT_CONDITION + END_CASE + CASE // Is not an unsigned char. + ARGUMENT_CONDITION(ARG_NO(0), OutOfRange) + RANGE(0, 255) + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + SUMMARY(isprint, ARGUMENT_TYPES(IntTy), RETURN_TYPE(IntTy), + INVALIDATION_APPROACH(EvalCallAsPure)) + CASE + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE(32, 126) + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(OutOfRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + CASE + ARGUMENT_CONDITION(ARG_NO(0), OutOfRange) + RANGE(32, 126) + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + SUMMARY(ispunct, ARGUMENT_TYPES(IntTy), RETURN_TYPE(IntTy), + INVALIDATION_APPROACH(EvalCallAsPure)) + CASE + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE('!', '/') + RANGE(':', '@') + RANGE('[', '`') + RANGE('{', '~') + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(OutOfRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + CASE + ARGUMENT_CONDITION(ARG_NO(0), OutOfRange) + RANGE('!', '/') + RANGE(':', '@') + RANGE('[', '`') + RANGE('{', '~') + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + SUMMARY(isspace, ARGUMENT_TYPES(IntTy), RETURN_TYPE(IntTy), + INVALIDATION_APPROACH(EvalCallAsPure)) + CASE // Space, '\f', '\n', '\r', '\t', '\v'. + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE(9, 13) + SINGLE_VALUE(' ') + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(OutOfRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + CASE // The locale-specific range. + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE(128, 255) + END_ARGUMENT_CONDITION + END_CASE + CASE + ARGUMENT_CONDITION(ARG_NO(0), OutOfRange) + RANGE(9, 13) + SINGLE_VALUE(' ') + RANGE(128, 255) + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + SUMMARY(isupper, ARGUMENT_TYPES(IntTy), RETURN_TYPE (IntTy), + INVALIDATION_APPROACH(EvalCallAsPure)) + CASE // Is certainly uppercase. + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE('A', 'Z') + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(OutOfRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + CASE // The locale-specific range. + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE(128, 255) + END_ARGUMENT_CONDITION + END_CASE + CASE // Other. + ARGUMENT_CONDITION(ARG_NO(0), OutOfRange) + RANGE('A', 'Z') RANGE(128, 255) + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + SUMMARY(isxdigit, ARGUMENT_TYPES(IntTy), RETURN_TYPE(IntTy), + INVALIDATION_APPROACH(EvalCallAsPure)) + CASE + ARGUMENT_CONDITION(ARG_NO(0), WithinRange) + RANGE('0', '9') + RANGE('A', 'F') + RANGE('a', 'f') + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(OutOfRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + CASE + ARGUMENT_CONDITION(ARG_NO(0), OutOfRange) + RANGE('0', '9') + RANGE('A', 'F') + RANGE('a', 'f') + END_ARGUMENT_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(0) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + + // The getc() family of functions that returns either a char or an EOF. + SUMMARY(getc, ARGUMENT_TYPES(Irrelevant), RETURN_TYPE(IntTy), + INVALIDATION_APPROACH(NoEvalCall)) + CASE // FIXME: EOF is assumed to be defined as -1. + RETURN_VALUE_CONDITION(WithinRange) + RANGE(-1, 255) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + SUMMARY(fgetc, ARGUMENT_TYPES(Irrelevant), RETURN_TYPE(IntTy), + INVALIDATION_APPROACH(NoEvalCall)) + CASE // FIXME: EOF is assumed to be defined as -1. + RETURN_VALUE_CONDITION(WithinRange) + RANGE(-1, 255) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + SUMMARY(getchar, ARGUMENT_TYPES(), RETURN_TYPE(IntTy), + INVALIDATION_APPROACH(NoEvalCall)) + CASE // FIXME: EOF is assumed to be defined as -1. + RETURN_VALUE_CONDITION(WithinRange) + RANGE(-1, 255) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + + // read()-like functions that never return more than buffer size. + // We are not sure how ssize_t is defined on every platform, so we provide + // three variants that should cover common cases. + SUMMARY_WITH_VARIANTS(read) + VARIANT(ARGUMENT_TYPES(Irrelevant, Irrelevant, SizeTy), + RETURN_TYPE(IntTy), INVALIDATION_APPROACH(NoEvalCall)) + CASE + RETURN_VALUE_CONDITION(ComparesToArgument) + IS_LESS_THAN(ARG_NO(2)) + END_RETURN_VALUE_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + RANGE(-1, IntMax) + END_RETURN_VALUE_CONDITION + END_CASE + END_VARIANT + VARIANT(ARGUMENT_TYPES(Irrelevant, Irrelevant, SizeTy), + RETURN_TYPE(LongTy), INVALIDATION_APPROACH(NoEvalCall)) + CASE + RETURN_VALUE_CONDITION(ComparesToArgument) + IS_LESS_THAN(ARG_NO(2)) + END_RETURN_VALUE_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + RANGE(-1, LongMax) + END_RETURN_VALUE_CONDITION + END_CASE + END_VARIANT + VARIANT(ARGUMENT_TYPES(Irrelevant, Irrelevant, SizeTy), + RETURN_TYPE(LongLongTy), INVALIDATION_APPROACH(NoEvalCall)) + CASE + RETURN_VALUE_CONDITION(ComparesToArgument) + IS_LESS_THAN(ARG_NO(2)) + END_RETURN_VALUE_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + RANGE(-1, LongLongMax) + END_RETURN_VALUE_CONDITION + END_CASE + END_VARIANT + END_SUMMARY_WITH_VARIANTS + SUMMARY_WITH_VARIANTS(write) + // Again, due to elusive nature of ssize_t, we have duplicate + // our summaries to cover different variants. + VARIANT(ARGUMENT_TYPES(Irrelevant, Irrelevant, SizeTy), + RETURN_TYPE(IntTy), INVALIDATION_APPROACH(NoEvalCall)) + CASE + RETURN_VALUE_CONDITION(ComparesToArgument) + IS_LESS_THAN(ARG_NO(2)) + END_RETURN_VALUE_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + RANGE(-1, IntMax) + END_RETURN_VALUE_CONDITION + END_CASE + END_VARIANT + VARIANT(ARGUMENT_TYPES(Irrelevant, Irrelevant, SizeTy), + RETURN_TYPE(LongTy), INVALIDATION_APPROACH(NoEvalCall)) + CASE + RETURN_VALUE_CONDITION(ComparesToArgument) + IS_LESS_THAN(ARG_NO(2)) + END_RETURN_VALUE_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + RANGE(-1, LongMax) + END_RETURN_VALUE_CONDITION + END_CASE + END_VARIANT + VARIANT(ARGUMENT_TYPES(Irrelevant, Irrelevant, SizeTy), + RETURN_TYPE(LongLongTy), INVALIDATION_APPROACH(NoEvalCall)) + CASE + RETURN_VALUE_CONDITION(ComparesToArgument) + IS_LESS_THAN(ARG_NO(2)) + END_RETURN_VALUE_CONDITION + RETURN_VALUE_CONDITION(WithinRange) + RANGE(-1, LongLongMax) + END_RETURN_VALUE_CONDITION + END_CASE + END_VARIANT + END_SUMMARY_WITH_VARIANTS + SUMMARY(fread, + ARGUMENT_TYPES(Irrelevant, Irrelevant, SizeTy, Irrelevant), + RETURN_TYPE(SizeTy), INVALIDATION_APPROACH(NoEvalCall)) + CASE + RETURN_VALUE_CONDITION(ComparesToArgument) + IS_LESS_THAN(ARG_NO(2)) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + SUMMARY(fwrite, + ARGUMENT_TYPES(Irrelevant, Irrelevant, SizeTy, Irrelevant), + RETURN_TYPE(SizeTy), INVALIDATION_APPROACH(NoEvalCall)) + CASE + RETURN_VALUE_CONDITION(ComparesToArgument) + IS_LESS_THAN(ARG_NO(2)) + END_RETURN_VALUE_CONDITION + END_CASE + END_SUMMARY + + // getline()-like functions either fail or read at least the delimiter. + SUMMARY_WITH_VARIANTS(getline) + VARIANT(ARGUMENT_TYPES(Irrelevant, Irrelevant, Irrelevant), + RETURN_TYPE(IntTy), INVALIDATION_APPROACH(NoEvalCall)) + CASE + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(-1) + RANGE(1, IntMax) + END_RETURN_VALUE_CONDITION + END_CASE + END_VARIANT + VARIANT(ARGUMENT_TYPES(Irrelevant, Irrelevant, Irrelevant), + RETURN_TYPE(LongTy), INVALIDATION_APPROACH(NoEvalCall)) + CASE + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(-1) + RANGE(1, LongMax) + END_RETURN_VALUE_CONDITION + END_CASE + END_VARIANT + VARIANT(ARGUMENT_TYPES(Irrelevant, Irrelevant, Irrelevant), + RETURN_TYPE(LongLongTy), INVALIDATION_APPROACH(NoEvalCall)) + CASE + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(-1) + RANGE(1, LongLongMax) + END_RETURN_VALUE_CONDITION + END_CASE + END_VARIANT + END_SUMMARY_WITH_VARIANTS + SUMMARY_WITH_VARIANTS(getdelim) + VARIANT(ARGUMENT_TYPES(Irrelevant, Irrelevant, Irrelevant, Irrelevant), + RETURN_TYPE(IntTy), INVALIDATION_APPROACH(NoEvalCall)) + CASE + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(-1) + RANGE(1, IntMax) + END_RETURN_VALUE_CONDITION + END_CASE + END_VARIANT + VARIANT(ARGUMENT_TYPES(Irrelevant, Irrelevant, Irrelevant, Irrelevant), + RETURN_TYPE(LongTy), INVALIDATION_APPROACH(NoEvalCall)) + CASE + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(-1) + RANGE(1, LongMax) + END_RETURN_VALUE_CONDITION + END_CASE + END_VARIANT + VARIANT(ARGUMENT_TYPES(Irrelevant, Irrelevant, Irrelevant, Irrelevant), + RETURN_TYPE(LongLongTy), INVALIDATION_APPROACH(NoEvalCall)) + CASE + RETURN_VALUE_CONDITION(WithinRange) + SINGLE_VALUE(-1) + RANGE(1, LongLongMax) + END_RETURN_VALUE_CONDITION + END_CASE + END_VARIANT + END_SUMMARY_WITH_VARIANTS + }; +} + +void ento::registerStdCLibraryFunctionsChecker(CheckerManager &mgr) { + // If this checker grows large enough to support C++, Objective-C, or other + // standard libraries, we could use multiple register...Checker() functions, + // which would register various checkers with the help of the same Checker + // class, turning on different function summaries. + mgr.registerChecker<StdLibraryFunctionsChecker>(); +} diff --git a/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index 82b01fe814da0..915514b42133c 100644 --- a/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -19,7 +19,6 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h" -#include "llvm/ADT/ImmutableMap.h" using namespace clang; using namespace ento; diff --git a/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp b/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp index 4b78c20583419..26bf597bd950c 100644 --- a/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp @@ -21,6 +21,7 @@ #include "llvm/ADT/Optional.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallString.h" +#include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringSwitch.h" #include "llvm/Support/raw_ostream.h" #include <fcntl.h> @@ -28,6 +29,16 @@ using namespace clang; using namespace ento; +enum class OpenVariant { + /// The standard open() call: + /// int open(const char *path, int oflag, ...); + Open, + + /// The variant taking a directory file descriptor and a relative path: + /// int openat(int fd, const char *path, int oflag, ...); + OpenAt +}; + namespace { class UnixAPIChecker : public Checker< check::PreStmt<CallExpr> > { mutable std::unique_ptr<BugType> BT_open, BT_pthreadOnce, BT_mallocZero; @@ -37,17 +48,24 @@ public: void checkPreStmt(const CallExpr *CE, CheckerContext &C) const; void CheckOpen(CheckerContext &C, const CallExpr *CE) const; + void CheckOpenAt(CheckerContext &C, const CallExpr *CE) const; + void CheckPthreadOnce(CheckerContext &C, const CallExpr *CE) const; void CheckCallocZero(CheckerContext &C, const CallExpr *CE) const; void CheckMallocZero(CheckerContext &C, const CallExpr *CE) const; void CheckReallocZero(CheckerContext &C, const CallExpr *CE) const; void CheckReallocfZero(CheckerContext &C, const CallExpr *CE) const; void CheckAllocaZero(CheckerContext &C, const CallExpr *CE) const; + void CheckAllocaWithAlignZero(CheckerContext &C, const CallExpr *CE) const; void CheckVallocZero(CheckerContext &C, const CallExpr *CE) const; typedef void (UnixAPIChecker::*SubChecker)(CheckerContext &, const CallExpr *) const; private: + + void CheckOpenVariant(CheckerContext &C, + const CallExpr *CE, OpenVariant Variant) const; + bool ReportZeroByteAllocation(CheckerContext &C, ProgramStateRef falseState, const Expr *arg, @@ -89,25 +107,71 @@ void UnixAPIChecker::ReportOpenBug(CheckerContext &C, } void UnixAPIChecker::CheckOpen(CheckerContext &C, const CallExpr *CE) const { + CheckOpenVariant(C, CE, OpenVariant::Open); +} + +void UnixAPIChecker::CheckOpenAt(CheckerContext &C, const CallExpr *CE) const { + CheckOpenVariant(C, CE, OpenVariant::OpenAt); +} + +void UnixAPIChecker::CheckOpenVariant(CheckerContext &C, + const CallExpr *CE, + OpenVariant Variant) const { + // The index of the argument taking the flags open flags (O_RDONLY, + // O_WRONLY, O_CREAT, etc.), + unsigned int FlagsArgIndex; + const char *VariantName; + switch (Variant) { + case OpenVariant::Open: + FlagsArgIndex = 1; + VariantName = "open"; + break; + case OpenVariant::OpenAt: + FlagsArgIndex = 2; + VariantName = "openat"; + break; + }; + + // All calls should at least provide arguments up to the 'flags' parameter. + unsigned int MinArgCount = FlagsArgIndex + 1; + + // If the flags has O_CREAT set then open/openat() require an additional + // argument specifying the file mode (permission bits) for the created file. + unsigned int CreateModeArgIndex = FlagsArgIndex + 1; + + // The create mode argument should be the last argument. + unsigned int MaxArgCount = CreateModeArgIndex + 1; + ProgramStateRef state = C.getState(); - if (CE->getNumArgs() < 2) { + if (CE->getNumArgs() < MinArgCount) { // The frontend should issue a warning for this case, so this is a sanity // check. return; - } else if (CE->getNumArgs() == 3) { - const Expr *Arg = CE->getArg(2); + } else if (CE->getNumArgs() == MaxArgCount) { + const Expr *Arg = CE->getArg(CreateModeArgIndex); QualType QT = Arg->getType(); if (!QT->isIntegerType()) { + SmallString<256> SBuf; + llvm::raw_svector_ostream OS(SBuf); + OS << "The " << CreateModeArgIndex + 1 + << llvm::getOrdinalSuffix(CreateModeArgIndex + 1) + << " argument to '" << VariantName << "' is not an integer"; + ReportOpenBug(C, state, - "Third argument to 'open' is not an integer", + SBuf.c_str(), Arg->getSourceRange()); return; } - } else if (CE->getNumArgs() > 3) { + } else if (CE->getNumArgs() > MaxArgCount) { + SmallString<256> SBuf; + llvm::raw_svector_ostream OS(SBuf); + OS << "Call to '" << VariantName << "' with more than " << MaxArgCount + << " arguments"; + ReportOpenBug(C, state, - "Call to 'open' with more than three arguments", - CE->getArg(3)->getSourceRange()); + SBuf.c_str(), + CE->getArg(MaxArgCount)->getSourceRange()); return; } @@ -127,7 +191,7 @@ void UnixAPIChecker::CheckOpen(CheckerContext &C, const CallExpr *CE) const { } // Now check if oflags has O_CREAT set. - const Expr *oflagsEx = CE->getArg(1); + const Expr *oflagsEx = CE->getArg(FlagsArgIndex); const SVal V = state->getSVal(oflagsEx, C.getLocationContext()); if (!V.getAs<NonLoc>()) { // The case where 'V' can be a location can only be due to a bad header, @@ -153,10 +217,15 @@ void UnixAPIChecker::CheckOpen(CheckerContext &C, const CallExpr *CE) const { if (!(trueState && !falseState)) return; - if (CE->getNumArgs() < 3) { + if (CE->getNumArgs() < MaxArgCount) { + SmallString<256> SBuf; + llvm::raw_svector_ostream OS(SBuf); + OS << "Call to '" << VariantName << "' requires a " + << CreateModeArgIndex + 1 + << llvm::getOrdinalSuffix(CreateModeArgIndex + 1) + << " argument when the 'O_CREAT' flag is set"; ReportOpenBug(C, trueState, - "Call to 'open' requires a third argument when " - "the 'O_CREAT' flag is set", + SBuf.c_str(), oflagsEx->getSourceRange()); } } @@ -337,6 +406,11 @@ void UnixAPIChecker::CheckAllocaZero(CheckerContext &C, BasicAllocationCheck(C, CE, 1, 0, "alloca"); } +void UnixAPIChecker::CheckAllocaWithAlignZero(CheckerContext &C, + const CallExpr *CE) const { + BasicAllocationCheck(C, CE, 2, 0, "__builtin_alloca_with_align"); +} + void UnixAPIChecker::CheckVallocZero(CheckerContext &C, const CallExpr *CE) const { BasicAllocationCheck(C, CE, 1, 0, "valloc"); @@ -353,6 +427,12 @@ void UnixAPIChecker::checkPreStmt(const CallExpr *CE, if (!FD || FD->getKind() != Decl::Function) return; + // Don't treat functions in namespaces with the same name a Unix function + // as a call to the Unix function. + const DeclContext *NamespaceCtx = FD->getEnclosingNamespaceContext(); + if (NamespaceCtx && isa<NamespaceDecl>(NamespaceCtx)) + return; + StringRef FName = C.getCalleeName(FD); if (FName.empty()) return; @@ -360,12 +440,15 @@ void UnixAPIChecker::checkPreStmt(const CallExpr *CE, SubChecker SC = llvm::StringSwitch<SubChecker>(FName) .Case("open", &UnixAPIChecker::CheckOpen) + .Case("openat", &UnixAPIChecker::CheckOpenAt) .Case("pthread_once", &UnixAPIChecker::CheckPthreadOnce) .Case("calloc", &UnixAPIChecker::CheckCallocZero) .Case("malloc", &UnixAPIChecker::CheckMallocZero) .Case("realloc", &UnixAPIChecker::CheckReallocZero) .Case("reallocf", &UnixAPIChecker::CheckReallocfZero) .Cases("alloca", "__builtin_alloca", &UnixAPIChecker::CheckAllocaZero) + .Case("__builtin_alloca_with_align", + &UnixAPIChecker::CheckAllocaWithAlignZero) .Case("valloc", &UnixAPIChecker::CheckVallocZero) .Default(nullptr); diff --git a/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp b/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp index 892e713d241f0..ccd8e9a18b00f 100644 --- a/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp @@ -147,6 +147,14 @@ void UnreachableCodeChecker::checkEndAnalysis(ExplodedGraph &G, PathDiagnosticLocation DL; SourceLocation SL; if (const Stmt *S = getUnreachableStmt(CB)) { + // In macros, 'do {...} while (0)' is often used. Don't warn about the + // condition 0 when it is unreachable. + if (S->getLocStart().isMacroID()) + if (const auto *I = dyn_cast<IntegerLiteral>(S)) + if (I->getValue() == 0ULL) + if (const Stmt *Parent = PM->getParent(S)) + if (isa<DoStmt>(Parent)) + continue; SR = S->getSourceRange(); DL = PathDiagnosticLocation::createBegin(S, B.getSourceManager(), LC); SL = DL.asLocation(); @@ -191,8 +199,10 @@ void UnreachableCodeChecker::FindUnreachableEntryPoints(const CFGBlock *CB, // Find the Stmt* in a CFGBlock for reporting a warning const Stmt *UnreachableCodeChecker::getUnreachableStmt(const CFGBlock *CB) { for (CFGBlock::const_iterator I = CB->begin(), E = CB->end(); I != E; ++I) { - if (Optional<CFGStmt> S = I->getAs<CFGStmt>()) - return S->getStmt(); + if (Optional<CFGStmt> S = I->getAs<CFGStmt>()) { + if (!isa<DeclStmt>(S->getStmt())) + return S->getStmt(); + } } if (const Stmt *S = CB->getTerminator()) return S; diff --git a/lib/StaticAnalyzer/Checkers/ValistChecker.cpp b/lib/StaticAnalyzer/Checkers/ValistChecker.cpp new file mode 100644 index 0000000000000..b4bfa0c033415 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/ValistChecker.cpp @@ -0,0 +1,373 @@ +//== ValistChecker.cpp - stdarg.h macro usage checker -----------*- C++ -*--==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This defines checkers which detect usage of uninitialized va_list values +// and va_start calls with no matching va_end. +// +//===----------------------------------------------------------------------===// + +#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; + +REGISTER_SET_WITH_PROGRAMSTATE(InitializedVALists, const MemRegion *) + +namespace { +typedef SmallVector<const MemRegion *, 2> RegionVector; + +class ValistChecker : public Checker<check::PreCall, check::PreStmt<VAArgExpr>, + check::DeadSymbols> { + mutable std::unique_ptr<BugType> BT_leakedvalist, BT_uninitaccess; + + struct VAListAccepter { + CallDescription Func; + int VAListPos; + }; + static const SmallVector<VAListAccepter, 15> VAListAccepters; + static const CallDescription VaStart, VaEnd, VaCopy; + +public: + enum CheckKind { + CK_Uninitialized, + CK_Unterminated, + CK_CopyToSelf, + CK_NumCheckKinds + }; + + DefaultBool ChecksEnabled[CK_NumCheckKinds]; + CheckName CheckNames[CK_NumCheckKinds]; + + void checkPreStmt(const VAArgExpr *VAA, CheckerContext &C) const; + void checkPreCall(const CallEvent &Call, CheckerContext &C) const; + void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const; + +private: + const MemRegion *getVAListAsRegion(SVal SV, CheckerContext &C) const; + StringRef getVariableNameFromRegion(const MemRegion *Reg) const; + const ExplodedNode *getStartCallSite(const ExplodedNode *N, + const MemRegion *Reg, + CheckerContext &C) const; + + void reportUninitializedAccess(const MemRegion *VAList, StringRef Msg, + CheckerContext &C) const; + void reportLeakedVALists(const RegionVector &LeakedVALists, StringRef Msg1, + StringRef Msg2, CheckerContext &C, ExplodedNode *N, + bool ForceReport = 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> { + public: + ValistBugVisitor(const MemRegion *Reg, bool IsLeak = false) + : Reg(Reg), IsLeak(IsLeak) {} + void Profile(llvm::FoldingSetNodeID &ID) const override { + static int X = 0; + ID.AddPointer(&X); + ID.AddPointer(Reg); + } + std::unique_ptr<PathDiagnosticPiece> + getEndPath(BugReporterContext &BRC, const ExplodedNode *EndPathNode, + BugReport &BR) override { + if (!IsLeak) + return nullptr; + + 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); + } + PathDiagnosticPiece *VisitNode(const ExplodedNode *N, + const ExplodedNode *PrevN, + BugReporterContext &BRC, + BugReport &BR) override; + + private: + const MemRegion *Reg; + bool IsLeak; + }; +}; + +const SmallVector<ValistChecker::VAListAccepter, 15> + ValistChecker::VAListAccepters = { + {{"vfprintf", 3}, 2}, + {{"vfscanf", 3}, 2}, + {{"vprintf", 2}, 1}, + {{"vscanf", 2}, 1}, + {{"vsnprintf", 4}, 3}, + {{"vsprintf", 3}, 2}, + {{"vsscanf", 3}, 2}, + {{"vfwprintf", 3}, 2}, + {{"vfwscanf", 3}, 2}, + {{"vwprintf", 2}, 1}, + {{"vwscanf", 2}, 1}, + {{"vswprintf", 4}, 3}, + // vswprintf is the wide version of vsnprintf, + // vsprintf has no wide version + {{"vswscanf", 3}, 2}}; +const CallDescription ValistChecker::VaStart("__builtin_va_start", 2), + ValistChecker::VaCopy("__builtin_va_copy", 2), + ValistChecker::VaEnd("__builtin_va_end", 1); +} // end anonymous namespace + +void ValistChecker::checkPreCall(const CallEvent &Call, + CheckerContext &C) const { + if (!Call.isGlobalCFunction()) + return; + if (Call.isCalled(VaStart)) + checkVAListStartCall(Call, C, false); + else if (Call.isCalled(VaCopy)) + checkVAListStartCall(Call, C, true); + else if (Call.isCalled(VaEnd)) + checkVAListEndCall(Call, C); + else { + for (auto FuncInfo : VAListAccepters) { + if (!Call.isCalled(FuncInfo.Func)) + continue; + const MemRegion *VAList = + getVAListAsRegion(Call.getArgSVal(FuncInfo.VAListPos), C); + if (!VAList) + return; + + if (C.getState()->contains<InitializedVALists>(VAList)) + return; + + SmallString<80> Errmsg("Function '"); + Errmsg += FuncInfo.Func.getFunctionName(); + Errmsg += "' is called with an uninitialized va_list argument"; + reportUninitializedAccess(VAList, Errmsg.c_str(), C); + break; + } + } +} + +void ValistChecker::checkPreStmt(const VAArgExpr *VAA, + CheckerContext &C) const { + ProgramStateRef State = C.getState(); + SVal VAListSVal = State->getSVal(VAA->getSubExpr(), C.getLocationContext()); + const MemRegion *VAList = getVAListAsRegion(VAListSVal, C); + if (!VAList) + return; + if (!State->contains<InitializedVALists>(VAList)) + reportUninitializedAccess( + VAList, "va_arg() is called on an uninitialized va_list", C); +} + +void ValistChecker::checkDeadSymbols(SymbolReaper &SR, + CheckerContext &C) const { + ProgramStateRef State = C.getState(); + InitializedVAListsTy TrackedVALists = State->get<InitializedVALists>(); + RegionVector LeakedVALists; + for (auto Reg : TrackedVALists) { + if (SR.isLiveRegion(Reg)) + continue; + LeakedVALists.push_back(Reg); + State = State->remove<InitializedVALists>(Reg); + } + if (ExplodedNode *N = C.addTransition(State)) + reportLeakedVALists(LeakedVALists, "Initialized va_list", " is leaked", C, + N); +} + +const MemRegion *ValistChecker::getVAListAsRegion(SVal SV, + CheckerContext &C) const { + const MemRegion *Reg = SV.getAsRegion(); + const auto *TReg = dyn_cast_or_null<TypedValueRegion>(Reg); + // Some VarRegion based VLAs reach here as ElementRegions. + const auto *EReg = dyn_cast_or_null<ElementRegion>(TReg); + return EReg ? EReg->getSuperRegion() : TReg; +} + +// This function traverses the exploded graph backwards and finds the node where +// the va_list is initialized. That node is used for uniquing the bug paths. +// It is not likely that there are several different va_lists that belongs to +// different stack frames, so that case is not yet handled. +const ExplodedNode *ValistChecker::getStartCallSite(const ExplodedNode *N, + const MemRegion *Reg, + CheckerContext &C) const { + const LocationContext *LeakContext = N->getLocationContext(); + const ExplodedNode *StartCallNode = N; + + bool FoundInitializedState = false; + + while (N) { + ProgramStateRef State = N->getState(); + if (!State->contains<InitializedVALists>(Reg)) { + if (FoundInitializedState) + break; + } else { + FoundInitializedState = true; + } + const LocationContext *NContext = N->getLocationContext(); + if (NContext == LeakContext || NContext->isParentOf(LeakContext)) + StartCallNode = N; + N = N->pred_empty() ? nullptr : *(N->pred_begin()); + } + + return StartCallNode; +} + +void ValistChecker::reportUninitializedAccess(const MemRegion *VAList, + StringRef Msg, + CheckerContext &C) const { + if (!ChecksEnabled[CK_Uninitialized]) + return; + if (ExplodedNode *N = C.generateErrorNode()) { + if (!BT_uninitaccess) + BT_uninitaccess.reset(new BugType(CheckNames[CK_Uninitialized], + "Uninitialized va_list", + "Memory Error")); + auto R = llvm::make_unique<BugReport>(*BT_uninitaccess, Msg, N); + R->markInteresting(VAList); + R->addVisitor(llvm::make_unique<ValistBugVisitor>(VAList)); + C.emitReport(std::move(R)); + } +} + +void ValistChecker::reportLeakedVALists(const RegionVector &LeakedVALists, + StringRef Msg1, StringRef Msg2, + CheckerContext &C, ExplodedNode *N, + bool ForceReport) const { + if (!(ChecksEnabled[CK_Unterminated] || + (ChecksEnabled[CK_Uninitialized] && ForceReport))) + return; + for (auto Reg : LeakedVALists) { + if (!BT_leakedvalist) { + BT_leakedvalist.reset(new BugType(CheckNames[CK_Unterminated], + "Leaked va_list", "Memory Error")); + BT_leakedvalist->setSuppressOnSink(true); + } + + const ExplodedNode *StartNode = getStartCallSite(N, Reg, C); + PathDiagnosticLocation LocUsedForUniqueing; + + if (const Stmt *StartCallStmt = PathDiagnosticLocation::getStmt(StartNode)) + LocUsedForUniqueing = PathDiagnosticLocation::createBegin( + StartCallStmt, C.getSourceManager(), StartNode->getLocationContext()); + + SmallString<100> Buf; + llvm::raw_svector_ostream OS(Buf); + OS << Msg1; + std::string VariableName = Reg->getDescriptiveName(); + if (!VariableName.empty()) + OS << " " << VariableName; + OS << Msg2; + + auto R = llvm::make_unique<BugReport>( + *BT_leakedvalist, OS.str(), N, LocUsedForUniqueing, + StartNode->getLocationContext()->getDecl()); + R->markInteresting(Reg); + R->addVisitor(llvm::make_unique<ValistBugVisitor>(Reg, true)); + C.emitReport(std::move(R)); + } +} + +void ValistChecker::checkVAListStartCall(const CallEvent &Call, + CheckerContext &C, bool IsCopy) const { + const MemRegion *VAList = getVAListAsRegion(Call.getArgSVal(0), C); + ProgramStateRef State = C.getState(); + if (!VAList) + return; + + if (IsCopy) { + const MemRegion *Arg2 = getVAListAsRegion(Call.getArgSVal(1), C); + if (Arg2) { + if (ChecksEnabled[CK_CopyToSelf] && VAList == Arg2) { + RegionVector LeakedVALists{VAList}; + if (ExplodedNode *N = C.addTransition(State)) + reportLeakedVALists(LeakedVALists, "va_list", + " is copied onto itself", C, N, true); + return; + } else if (!State->contains<InitializedVALists>(Arg2)) { + if (State->contains<InitializedVALists>(VAList)) { + State = State->remove<InitializedVALists>(VAList); + RegionVector LeakedVALists{VAList}; + if (ExplodedNode *N = C.addTransition(State)) + reportLeakedVALists(LeakedVALists, "Initialized va_list", + " is overwritten by an uninitialized one", C, N, + true); + } else { + reportUninitializedAccess(Arg2, "Uninitialized va_list is copied", C); + } + return; + } + } + } + if (State->contains<InitializedVALists>(VAList)) { + RegionVector LeakedVALists{VAList}; + if (ExplodedNode *N = C.addTransition(State)) + reportLeakedVALists(LeakedVALists, "Initialized va_list", + " is initialized again", C, N); + return; + } + + State = State->add<InitializedVALists>(VAList); + C.addTransition(State); +} + +void ValistChecker::checkVAListEndCall(const CallEvent &Call, + CheckerContext &C) const { + const MemRegion *VAList = getVAListAsRegion(Call.getArgSVal(0), C); + if (!VAList) + return; + + if (!C.getState()->contains<InitializedVALists>(VAList)) { + reportUninitializedAccess( + VAList, "va_end() is called on an uninitialized va_list", C); + return; + } + ProgramStateRef State = C.getState(); + State = State->remove<InitializedVALists>(VAList); + C.addTransition(State); +} + +PathDiagnosticPiece *ValistChecker::ValistBugVisitor::VisitNode( + const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC, + BugReport &BR) { + ProgramStateRef State = N->getState(); + ProgramStateRef StatePrev = PrevN->getState(); + + const Stmt *S = PathDiagnosticLocation::getStmt(N); + if (!S) + return nullptr; + + StringRef Msg; + if (State->contains<InitializedVALists>(Reg) && + !StatePrev->contains<InitializedVALists>(Reg)) + Msg = "Initialized va_list"; + else if (!State->contains<InitializedVALists>(Reg) && + StatePrev->contains<InitializedVALists>(Reg)) + Msg = "Ended va_list"; + + if (Msg.empty()) + return nullptr; + + PathDiagnosticLocation Pos(S, BRC.getSourceManager(), + N->getLocationContext()); + return new PathDiagnosticEventPiece(Pos, Msg, true); +} + +#define REGISTER_CHECKER(name) \ + void ento::register##name##Checker(CheckerManager &mgr) { \ + ValistChecker *checker = mgr.registerChecker<ValistChecker>(); \ + checker->ChecksEnabled[ValistChecker::CK_##name] = true; \ + checker->CheckNames[ValistChecker::CK_##name] = mgr.getCurrentCheckName(); \ + } + +REGISTER_CHECKER(Uninitialized) +REGISTER_CHECKER(Unterminated) +REGISTER_CHECKER(CopyToSelf) diff --git a/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp b/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp index 5502503026118..15e8ea31c4c44 100644 --- a/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp @@ -32,6 +32,18 @@ class WalkAST : public StmtVisitor<WalkAST> { BugReporter &BR; AnalysisDeclContext *AC; + /// The root constructor or destructor whose callees are being analyzed. + const CXXMethodDecl *RootMethod = nullptr; + + /// Whether the checker should walk into bodies of called functions. + /// Controlled by the "Interprocedural" analyzer-config option. + bool IsInterprocedural = false; + + /// Whether the checker should only warn for calls to pure virtual functions + /// (which is undefined behavior) or for all virtual functions (which may + /// may result in unexpected behavior). + bool ReportPureOnly = false; + typedef const CallExpr * WorkListUnit; typedef SmallVector<WorkListUnit, 20> DFSWorkList; @@ -59,9 +71,16 @@ class WalkAST : public StmtVisitor<WalkAST> { const CallExpr *visitingCallExpr; public: - WalkAST(const CheckerBase *checker, BugReporter &br, - AnalysisDeclContext *ac) - : Checker(checker), BR(br), AC(ac), visitingCallExpr(nullptr) {} + WalkAST(const CheckerBase *checker, BugReporter &br, AnalysisDeclContext *ac, + const CXXMethodDecl *rootMethod, bool isInterprocedural, + bool reportPureOnly) + : Checker(checker), BR(br), AC(ac), RootMethod(rootMethod), + IsInterprocedural(isInterprocedural), ReportPureOnly(reportPureOnly), + visitingCallExpr(nullptr) { + // Walking should always start from either a constructor or a destructor. + assert(isa<CXXConstructorDecl>(rootMethod) || + isa<CXXDestructorDecl>(rootMethod)); + } bool hasWork() const { return !WList.empty(); } @@ -132,7 +151,8 @@ void WalkAST::VisitChildren(Stmt *S) { void WalkAST::VisitCallExpr(CallExpr *CE) { VisitChildren(CE); - Enqueue(CE); + if (IsInterprocedural) + Enqueue(CE); } void WalkAST::VisitCXXMemberCallExpr(CallExpr *CE) { @@ -164,51 +184,64 @@ void WalkAST::VisitCXXMemberCallExpr(CallExpr *CE) { !MD->getParent()->hasAttr<FinalAttr>()) ReportVirtualCall(CE, MD->isPure()); - Enqueue(CE); + if (IsInterprocedural) + Enqueue(CE); } void WalkAST::ReportVirtualCall(const CallExpr *CE, bool isPure) { + if (ReportPureOnly && !isPure) + return; + SmallString<100> buf; llvm::raw_svector_ostream os(buf); - os << "Call Path : "; - // Name of current visiting CallExpr. - os << *CE->getDirectCallee(); - - // Name of the CallExpr whose body is current walking. - if (visitingCallExpr) - os << " <-- " << *visitingCallExpr->getDirectCallee(); - // Names of FunctionDecls in worklist with state PostVisited. - for (SmallVectorImpl<const CallExpr *>::iterator I = WList.end(), + // FIXME: The interprocedural diagnostic experience here is not good. + // Ultimately this checker should be re-written to be path sensitive. + // For now, only diagnose intraprocedurally, by default. + if (IsInterprocedural) { + os << "Call Path : "; + // Name of current visiting CallExpr. + os << *CE->getDirectCallee(); + + // Name of the CallExpr whose body is current being walked. + if (visitingCallExpr) + os << " <-- " << *visitingCallExpr->getDirectCallee(); + // Names of FunctionDecls in worklist with state PostVisited. + for (SmallVectorImpl<const CallExpr *>::iterator I = WList.end(), E = WList.begin(); I != E; --I) { - const FunctionDecl *FD = (*(I-1))->getDirectCallee(); - assert(FD); - if (VisitedFunctions[FD] == PostVisited) - os << " <-- " << *FD; + const FunctionDecl *FD = (*(I-1))->getDirectCallee(); + assert(FD); + if (VisitedFunctions[FD] == PostVisited) + os << " <-- " << *FD; + } + + os << "\n"; } PathDiagnosticLocation CELoc = PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), AC); SourceRange R = CE->getCallee()->getSourceRange(); - if (isPure) { - os << "\n" << "Call pure virtual functions during construction or " - << "destruction may leads undefined behaviour"; - BR.EmitBasicReport(AC->getDecl(), Checker, - "Call pure virtual function during construction or " - "Destruction", - "Cplusplus", os.str(), CELoc, R); - return; - } - else { - os << "\n" << "Call virtual functions during construction or " - << "destruction will never go to a more derived class"; - BR.EmitBasicReport(AC->getDecl(), Checker, - "Call virtual function during construction or " - "Destruction", - "Cplusplus", os.str(), CELoc, R); - return; - } + os << "Call to "; + if (isPure) + os << "pure "; + + os << "virtual function during "; + + if (isa<CXXConstructorDecl>(RootMethod)) + os << "construction "; + else + os << "destruction "; + + if (isPure) + os << "has undefined behavior"; + else + os << "will not dispatch to derived class"; + + BR.EmitBasicReport(AC->getDecl(), Checker, + "Call to virtual function during construction or " + "destruction", + "C++ Object Lifecycle", os.str(), CELoc, R); } //===----------------------------------------------------------------------===// @@ -218,14 +251,18 @@ void WalkAST::ReportVirtualCall(const CallExpr *CE, bool isPure) { namespace { class VirtualCallChecker : public Checker<check::ASTDecl<CXXRecordDecl> > { public: + DefaultBool isInterprocedural; + DefaultBool isPureOnly; + void checkASTDecl(const CXXRecordDecl *RD, AnalysisManager& mgr, BugReporter &BR) const { - WalkAST walker(this, BR, mgr.getAnalysisDeclContext(RD)); + AnalysisDeclContext *ADC = mgr.getAnalysisDeclContext(RD); // Check the constructors. for (const auto *I : RD->ctors()) { if (!I->isCopyOrMoveConstructor()) if (Stmt *Body = I->getBody()) { + WalkAST walker(this, BR, ADC, I, isInterprocedural, isPureOnly); walker.Visit(Body); walker.Execute(); } @@ -234,6 +271,7 @@ public: // Check the destructor. if (CXXDestructorDecl *DD = RD->getDestructor()) if (Stmt *Body = DD->getBody()) { + WalkAST walker(this, BR, ADC, DD, isInterprocedural, isPureOnly); walker.Visit(Body); walker.Execute(); } @@ -242,5 +280,12 @@ public: } void ento::registerVirtualCallChecker(CheckerManager &mgr) { - mgr.registerChecker<VirtualCallChecker>(); + VirtualCallChecker *checker = mgr.registerChecker<VirtualCallChecker>(); + checker->isInterprocedural = + mgr.getAnalyzerOptions().getBooleanOption("Interprocedural", false, + checker); + + checker->isPureOnly = + mgr.getAnalyzerOptions().getBooleanOption("PureOnly", false, + checker); } |