From: Ted Kremenek Date: Tue, 22 Jul 2008 00:46:16 +0000 (+0000) Subject: Added path-sensitive checking for null pointer values passed to function arguments... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=584def7364f51e35bfcaf5c3c64673096533adda;p=clang Added path-sensitive checking for null pointer values passed to function arguments marked nonnull. This implements git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@53891 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/AST/Attr.h b/include/clang/AST/Attr.h index 0f80f23882..817d66de1a 100644 --- a/include/clang/AST/Attr.h +++ b/include/clang/AST/Attr.h @@ -197,7 +197,7 @@ public: delete [] ArgNums; } - bool isNonNull(unsigned arg) { + bool isNonNull(unsigned arg) const { return ArgNums ? std::binary_search(ArgNums, ArgNums+Size, arg) : true; } }; diff --git a/include/clang/Analysis/PathSensitive/BugReporter.h b/include/clang/Analysis/PathSensitive/BugReporter.h index 4374c1e40a..803a70397d 100644 --- a/include/clang/Analysis/PathSensitive/BugReporter.h +++ b/include/clang/Analysis/PathSensitive/BugReporter.h @@ -300,12 +300,14 @@ public: }; class SimpleBugType : public BugTypeCacheLocation { - const char* name; + const char* name; + const char* desc; public: - SimpleBugType(const char* n) : name(n) {} + SimpleBugType(const char* n) : name(n), desc(n) {} + SimpleBugType(const char* n, const char* d) : name(n), desc(d) {} virtual const char* getName() const { return name; } - virtual const char* getDescription() const { return name; } + virtual const char* getDescription() const { return desc; } }; } // end clang namespace diff --git a/include/clang/Analysis/PathSensitive/GRAuditor.h b/include/clang/Analysis/PathSensitive/GRAuditor.h index 29fb3bc186..eca591d4af 100644 --- a/include/clang/Analysis/PathSensitive/GRAuditor.h +++ b/include/clang/Analysis/PathSensitive/GRAuditor.h @@ -26,10 +26,11 @@ namespace clang { template class GRAuditor { public: - typedef ExplodedNode NodeTy; + typedef ExplodedNode NodeTy; + typedef typename STATE::ManagerTy ManagerTy; virtual ~GRAuditor() {} - virtual bool Audit(NodeTy* N) = 0; + virtual bool Audit(NodeTy* N, ManagerTy& M) = 0; }; diff --git a/include/clang/Analysis/PathSensitive/GRCoreEngine.h b/include/clang/Analysis/PathSensitive/GRCoreEngine.h index 9c3098f93e..16f79856ba 100644 --- a/include/clang/Analysis/PathSensitive/GRCoreEngine.h +++ b/include/clang/Analysis/PathSensitive/GRCoreEngine.h @@ -166,17 +166,19 @@ public: template class GRStmtNodeBuilder { public: - typedef STATE StateTy; - typedef ExplodedNode NodeTy; + typedef STATE StateTy; + typedef typename StateTy::ManagerTy StateManagerTy; + typedef ExplodedNode NodeTy; private: GRStmtNodeBuilderImpl& NB; + StateManagerTy& Mgr; const StateTy* CleanedState; GRAuditor* Auditor; public: - GRStmtNodeBuilder(GRStmtNodeBuilderImpl& nb) : NB(nb), - Auditor(0), PurgingDeadSymbols(false), + GRStmtNodeBuilder(GRStmtNodeBuilderImpl& nb, StateManagerTy& mgr) : + NB(nb), Mgr(mgr), Auditor(0), PurgingDeadSymbols(false), BuildSinks(false), HasGeneratedNode(false) { CleanedState = getLastNode()->getState(); @@ -244,7 +246,7 @@ public: if (BuildSinks) N->markAsSink(); else { - if (Auditor && Auditor->Audit(N)) + if (Auditor && Auditor->Audit(N, Mgr)) N->markAsSink(); Dst.Add(N); @@ -552,6 +554,7 @@ class GRCoreEngine : public GRCoreEngineImpl { public: typedef SUBENGINE SubEngineTy; typedef typename SubEngineTy::StateTy StateTy; + typedef typename StateTy::ManagerTy StateManagerTy; typedef ExplodedGraph GraphTy; typedef typename GraphTy::NodeTy NodeTy; @@ -568,7 +571,7 @@ protected: } virtual void ProcessStmt(Stmt* S, GRStmtNodeBuilderImpl& BuilderImpl) { - GRStmtNodeBuilder Builder(BuilderImpl); + GRStmtNodeBuilder Builder(BuilderImpl,SubEngine.getStateManager()); SubEngine.ProcessStmt(S, Builder); } diff --git a/include/clang/Analysis/PathSensitive/GRSimpleAPICheck.h b/include/clang/Analysis/PathSensitive/GRSimpleAPICheck.h index 782ddcfffe..fcc9a0c3be 100644 --- a/include/clang/Analysis/PathSensitive/GRSimpleAPICheck.h +++ b/include/clang/Analysis/PathSensitive/GRSimpleAPICheck.h @@ -17,10 +17,10 @@ #define LLVM_CLANG_ANALYSIS_GRAPICHECKS #include "clang/Analysis/PathSensitive/GRAuditor.h" +#include "clang/Analysis/PathSensitive/ValueState.h" namespace clang { -class ValueState; class Diagnostic; class BugReporter; class ASTContext; diff --git a/include/clang/Analysis/PathSensitive/ValueState.h b/include/clang/Analysis/PathSensitive/ValueState.h index 3a56abe81b..554e6f5f86 100644 --- a/include/clang/Analysis/PathSensitive/ValueState.h +++ b/include/clang/Analysis/PathSensitive/ValueState.h @@ -58,6 +58,8 @@ public: typedef llvm::ImmutableMap ConstNotEqTy; typedef llvm::ImmutableMap ConstEqTy; + typedef ValueStateManager ManagerTy; + private: void operator=(const ValueState& R) const; @@ -120,6 +122,8 @@ public: // Queries. bool isNotEqual(SymbolID sym, const llvm::APSInt& V) const; + bool isEqual(SymbolID sym, const llvm::APSInt& V) const; + const llvm::APSInt* getSymVal(SymbolID sym) const; RVal LookupExpr(Expr* E) const { @@ -361,6 +365,10 @@ public: const ValueState* AddNE(const ValueState* St, SymbolID sym, const llvm::APSInt& V); + + bool isEqual(const ValueState* state, Expr* Ex, const llvm::APSInt& V); + bool isEqual(const ValueState* state, Expr* Ex, uint64_t); + // Assumption logic. const ValueState* Assume(const ValueState* St, RVal Cond, bool Assumption, bool& isFeasible) { diff --git a/lib/Analysis/BasicObjCFoundationChecks.cpp b/lib/Analysis/BasicObjCFoundationChecks.cpp index 4475ed2bc5..37cfc280b8 100644 --- a/lib/Analysis/BasicObjCFoundationChecks.cpp +++ b/lib/Analysis/BasicObjCFoundationChecks.cpp @@ -126,7 +126,7 @@ public: delete *I; } - virtual bool Audit(ExplodedNode* N); + virtual bool Audit(ExplodedNode* N, ValueStateManager&); virtual void EmitWarnings(BugReporter& BR); @@ -153,7 +153,8 @@ clang::CreateBasicObjCFoundationChecks(ASTContext& Ctx, -bool BasicObjCFoundationChecks::Audit(ExplodedNode* N) { +bool BasicObjCFoundationChecks::Audit(ExplodedNode* N, + ValueStateManager&) { ObjCMessageExpr* ME = cast(cast(N->getLocation()).getStmt()); @@ -348,7 +349,7 @@ public: virtual ~AuditCFNumberCreate() {} - virtual bool Audit(ExplodedNode* N); + virtual bool Audit(ExplodedNode* N, ValueStateManager&); virtual void EmitWarnings(BugReporter& BR) { Desc.EmitWarnings(BR); @@ -454,7 +455,7 @@ static const char* GetCFNumberTypeStr(uint64_t i) { } #endif -bool AuditCFNumberCreate::Audit(ExplodedNode* N) { +bool AuditCFNumberCreate::Audit(ExplodedNode* N,ValueStateManager&){ CallExpr* CE = cast(cast(N->getLocation()).getStmt()); Expr* Callee = CE->getCallee(); RVal CallV = GetRVal(N->getState(), Callee); diff --git a/lib/Analysis/GRExprEngine.cpp b/lib/Analysis/GRExprEngine.cpp index 8dee018931..f0c0d8dd9e 100644 --- a/lib/Analysis/GRExprEngine.cpp +++ b/lib/Analysis/GRExprEngine.cpp @@ -85,7 +85,7 @@ public: } } - virtual bool Audit(NodeTy* N) { + virtual bool Audit(NodeTy* N, ValueStateManager& VMgr) { Stmt* S = cast(N->getLocation()).getStmt(); void* key = reinterpret_cast((uintptr_t) S->getStmtClass()); MapTy::iterator MI = M.find(key); @@ -96,7 +96,7 @@ public: bool isSink = false; for (Checks::iterator I=MI->second.begin(), E=MI->second.end(); I!=E; ++I) - isSink |= (*I)->Audit(N); + isSink |= (*I)->Audit(N, VMgr); return isSink; } diff --git a/lib/Analysis/GRSimpleVals.cpp b/lib/Analysis/GRSimpleVals.cpp index 36e7233f25..38327cbad5 100644 --- a/lib/Analysis/GRSimpleVals.cpp +++ b/lib/Analysis/GRSimpleVals.cpp @@ -335,6 +335,63 @@ void UndefBranch::EmitWarnings(BugReporter& BR) { } } +//===----------------------------------------------------------------------===// +// __attribute__(nonnull) checking + +class VISIBILITY_HIDDEN CheckAttrNonNull : public GRSimpleAPICheck { + SimpleBugType BT; + std::list Reports; + +public: + CheckAttrNonNull() : + BT("'nonnull' argument passed null", + "Null pointer passed as an argument to a 'nonnull' parameter") {} + + + virtual bool Audit(ExplodedNode* N, ValueStateManager& VMgr) { + CallExpr* CE = cast(cast(N->getLocation()).getStmt()); + const ValueState* state = N->getState(); + + RVal X = VMgr.GetRVal(state, CE->getCallee()); + + if (!isa(X)) + return false; + + FunctionDecl* FD = dyn_cast(cast(X).getDecl()); + const NonNullAttr* Att = FD->getAttr(); + + if (!Att) + return false; + + // Iterate through the arguments of CE and check them for null. + + unsigned idx = 0; + bool hasError = false; + + for (CallExpr::arg_iterator I=CE->arg_begin(), E=CE->arg_end(); I!=E; + ++I, ++idx) { + + if (!VMgr.isEqual(state, *I, 0) || !Att->isNonNull(idx)) + continue; + + RangedBugReport R(BT, N); + R.addRange((*I)->getSourceRange()); + Reports.push_back(R); + hasError = true; + } + + return hasError; + } + + virtual void EmitWarnings(BugReporter& BR) { + for (std::list::iterator I=Reports.begin(), + E=Reports.end(); I!=E; ++I) + BR.EmitWarning(*I); + } +}; + +//===----------------------------------------------------------------------===// +// Check registration. void GRSimpleVals::RegisterChecks(GRExprEngine& Eng) { @@ -360,6 +417,7 @@ void GRSimpleVals::RegisterChecks(GRExprEngine& Eng) { Check = CreateAuditCFNumberCreate(Ctx, VMgr); Eng.AddCheck(Check, Stmt::CallExprClass); + Eng.AddCheck(new CheckAttrNonNull(), Stmt::CallExprClass); } //===----------------------------------------------------------------------===// diff --git a/lib/Analysis/ValueState.cpp b/lib/Analysis/ValueState.cpp index 2c3e6dabc6..1cf695438b 100644 --- a/lib/Analysis/ValueState.cpp +++ b/lib/Analysis/ValueState.cpp @@ -26,6 +26,15 @@ bool ValueState::isNotEqual(SymbolID sym, const llvm::APSInt& V) const { return T ? T->contains(&V) : false; } +bool ValueState::isEqual(SymbolID sym, const llvm::APSInt& V) const { + + // Retrieve the EQ-set associated with the given symbol. + const ConstEqTy::data_type* T = ConstEq.lookup(sym); + + // See if V is present in the EQ-set. + return T ? **T == V : false; +} + const llvm::APSInt* ValueState::getSymVal(SymbolID sym) const { ConstEqTy::data_type* T = ConstEq.lookup(sym); return T ? *T : NULL; @@ -296,6 +305,35 @@ void ValueState::print(std::ostream& Out, CheckerStatePrinter* P, P->PrintCheckerState(Out, CheckerState, nl, sep); } + +//===----------------------------------------------------------------------===// +// Queries. +//===----------------------------------------------------------------------===// + +bool ValueStateManager::isEqual(const ValueState* state, Expr* Ex, + const llvm::APSInt& Y) { + RVal V = GetRVal(state, Ex); + + if (lval::ConcreteInt* X = dyn_cast(&V)) + return X->getValue() == Y; + + if (nonlval::ConcreteInt* X = dyn_cast(&V)) + return X->getValue() == Y; + + if (nonlval::SymbolVal* X = dyn_cast(&V)) + return state->isEqual(X->getSymbol(), Y); + + if (lval::SymbolVal* X = dyn_cast(&V)) + return state->isEqual(X->getSymbol(), Y); + + return false; +} + +bool ValueStateManager::isEqual(const ValueState* state, Expr* Ex, + uint64_t x) { + return isEqual(state, Ex, BasicVals.getValue(x, Ex->getType())); +} + //===----------------------------------------------------------------------===// // "Assume" logic. //===----------------------------------------------------------------------===// diff --git a/test/Analysis/null-deref-ps.c b/test/Analysis/null-deref-ps.c index 86691bd9a3..964b3e4550 100644 --- a/test/Analysis/null-deref-ps.c +++ b/test/Analysis/null-deref-ps.c @@ -56,3 +56,10 @@ int f5() { return s[0]; // no-warning } +int bar(int* p) __attribute__((nonnull)); + +int f6(int *p) { + return !p ? bar(p) : *p; // expected-warning {{Null pointer passed as an argument to a 'nonnull' parameter}} +} + +