diff options
Diffstat (limited to 'clang/lib')
4 files changed, 129 insertions, 47 deletions
diff --git a/clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp index 26423b7368e..33a24d816c5 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp @@ -40,6 +40,7 @@ class CallAndMessageChecker : public Checker< check::PreStmt<CallExpr>, check::PreStmt<CXXDeleteExpr>, check::PreObjCMessage, + check::ObjCMessageNil, check::PreCall > { mutable std::unique_ptr<BugType> BT_call_null; mutable std::unique_ptr<BugType> BT_call_undef; @@ -60,6 +61,12 @@ public: void checkPreStmt(const CallExpr *CE, CheckerContext &C) const; void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const; void checkPreObjCMessage(const ObjCMethodCall &msg, CheckerContext &C) const; + + /// Fill in the return value that results from messaging nil based on the + /// return type and architecture and diagnose if the return value will be + /// garbage. + void checkObjCMessageNil(const ObjCMethodCall &msg, CheckerContext &C) const; + void checkPreCall(const CallEvent &Call, CheckerContext &C) const; private: @@ -471,22 +478,14 @@ void CallAndMessageChecker::checkPreObjCMessage(const ObjCMethodCall &msg, C.emitReport(std::move(R)); } return; - } else { - // Bifurcate the state into nil and non-nil ones. - DefinedOrUnknownSVal receiverVal = recVal.castAs<DefinedOrUnknownSVal>(); - - ProgramStateRef state = C.getState(); - ProgramStateRef notNilState, nilState; - std::tie(notNilState, nilState) = state->assume(receiverVal); - - // Handle receiver must be nil. - if (nilState && !notNilState) { - HandleNilReceiver(C, state, msg); - return; - } } } +void CallAndMessageChecker::checkObjCMessageNil(const ObjCMethodCall &msg, + CheckerContext &C) const { + HandleNilReceiver(C, C.getState(), msg); +} + void CallAndMessageChecker::emitNilReceiverBug(CheckerContext &C, const ObjCMethodCall &msg, ExplodedNode *N) const { diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp index 86955c4fcb0..37b84480f89 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp @@ -38,6 +38,7 @@ class CheckerDocumentation : public Checker< check::PreStmt<ReturnStmt>, check::PostStmt<DeclStmt>, check::PreObjCMessage, check::PostObjCMessage, + check::ObjCMessageNil, check::PreCall, check::PostCall, check::BranchCondition, @@ -95,6 +96,15 @@ public: /// check::PostObjCMessage void checkPostObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const {} + /// \brief Visit an Objective-C message whose receiver is nil. + /// + /// This will be called when the analyzer core processes a method call whose + /// receiver is definitely nil. In this case, check{Pre/Post}ObjCMessage and + /// check{Pre/Post}Call will not be called. + /// + /// check::ObjCMessageNil + void checkObjCMessageNil(const ObjCMethodCall &M, CheckerContext &C) const {} + /// \brief Pre-visit an abstract "call" event. /// /// This is used for checkers that want to check arguments or attributed diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp index d26a76abe8e..c464d301b6e 100644 --- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp +++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp @@ -177,7 +177,9 @@ void CheckerManager::runCheckersForStmt(bool isPreVisit, namespace { struct CheckObjCMessageContext { typedef std::vector<CheckerManager::CheckObjCMessageFunc> CheckersTy; - bool IsPreVisit, WasInlined; + + ObjCMessageVisitKind Kind; + bool WasInlined; const CheckersTy &Checkers; const ObjCMethodCall &Msg; ExprEngine &Eng; @@ -185,14 +187,28 @@ namespace { CheckersTy::const_iterator checkers_begin() { return Checkers.begin(); } CheckersTy::const_iterator checkers_end() { return Checkers.end(); } - CheckObjCMessageContext(bool isPreVisit, const CheckersTy &checkers, + CheckObjCMessageContext(ObjCMessageVisitKind visitKind, + const CheckersTy &checkers, const ObjCMethodCall &msg, ExprEngine &eng, bool wasInlined) - : IsPreVisit(isPreVisit), WasInlined(wasInlined), Checkers(checkers), + : Kind(visitKind), WasInlined(wasInlined), Checkers(checkers), Msg(msg), Eng(eng) { } void runChecker(CheckerManager::CheckObjCMessageFunc checkFn, NodeBuilder &Bldr, ExplodedNode *Pred) { + + bool IsPreVisit; + + switch (Kind) { + case ObjCMessageVisitKind::Pre: + IsPreVisit = true; + break; + case ObjCMessageVisitKind::MessageNil: + case ObjCMessageVisitKind::Post: + IsPreVisit = false; + break; + } + const ProgramPoint &L = Msg.getProgramPoint(IsPreVisit,checkFn.Checker); CheckerContext C(Bldr, Eng, Pred, L, WasInlined); @@ -202,19 +218,29 @@ namespace { } /// \brief Run checkers for visiting obj-c messages. -void CheckerManager::runCheckersForObjCMessage(bool isPreVisit, +void CheckerManager::runCheckersForObjCMessage(ObjCMessageVisitKind visitKind, ExplodedNodeSet &Dst, const ExplodedNodeSet &Src, const ObjCMethodCall &msg, ExprEngine &Eng, bool WasInlined) { - CheckObjCMessageContext C(isPreVisit, - isPreVisit ? PreObjCMessageCheckers - : PostObjCMessageCheckers, - msg, Eng, WasInlined); + auto &checkers = getObjCMessageCheckers(visitKind); + CheckObjCMessageContext C(visitKind, checkers, msg, Eng, WasInlined); expandGraphWithCheckers(C, Dst, Src); } +const std::vector<CheckerManager::CheckObjCMessageFunc> & +CheckerManager::getObjCMessageCheckers(ObjCMessageVisitKind Kind) { + switch (Kind) { + case ObjCMessageVisitKind::Pre: + return PreObjCMessageCheckers; + break; + case ObjCMessageVisitKind::Post: + return PostObjCMessageCheckers; + case ObjCMessageVisitKind::MessageNil: + return ObjCMessageNilCheckers; + } +} namespace { // FIXME: This has all the same signatures as CheckObjCMessageContext. // Is there a way we can merge the two? @@ -616,6 +642,11 @@ void CheckerManager::_registerForPostStmt(CheckStmtFunc checkfn, void CheckerManager::_registerForPreObjCMessage(CheckObjCMessageFunc checkfn) { PreObjCMessageCheckers.push_back(checkfn); } + +void CheckerManager::_registerForObjCMessageNil(CheckObjCMessageFunc checkfn) { + ObjCMessageNilCheckers.push_back(checkfn); +} + void CheckerManager::_registerForPostObjCMessage(CheckObjCMessageFunc checkfn) { PostObjCMessageCheckers.push_back(checkfn); } diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp index da66a46e18e..1d77714a002 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp @@ -139,6 +139,74 @@ void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME, CallEventRef<ObjCMethodCall> Msg = CEMgr.getObjCMethodCall(ME, Pred->getState(), Pred->getLocationContext()); + // There are three cases for the receiver: + // (1) it is definitely nil, + // (2) it is definitely non-nil, and + // (3) we don't know. + // + // If the receiver is definitely nil, we skip the pre/post callbacks and + // instead call the ObjCMessageNil callbacks and return. + // + // If the receiver is definitely non-nil, we call the pre- callbacks, + // evaluate the call, and call the post- callbacks. + // + // If we don't know, we drop the potential nil flow and instead + // continue from the assumed non-nil state as in (2). This approach + // intentionally drops coverage in order to prevent false alarms + // in the following scenario: + // + // id result = [o someMethod] + // if (result) { + // if (!o) { + // // <-- This program point should be unreachable because if o is nil + // // it must the case that result is nil as well. + // } + // } + // + // We could avoid dropping coverage by performing an explicit case split + // on each method call -- but this would get very expensive. An alternative + // would be to introduce lazy constraints. + // FIXME: This ignores many potential bugs (<rdar://problem/11733396>). + // Revisit once we have lazier constraints. + if (Msg->isInstanceMessage()) { + SVal recVal = Msg->getReceiverSVal(); + if (!recVal.isUndef()) { + // Bifurcate the state into nil and non-nil ones. + DefinedOrUnknownSVal receiverVal = + recVal.castAs<DefinedOrUnknownSVal>(); + ProgramStateRef State = Pred->getState(); + + ProgramStateRef notNilState, nilState; + std::tie(notNilState, nilState) = State->assume(receiverVal); + + // Receiver is definitely nil, so run ObjCMessageNil callbacks and return. + if (nilState && !notNilState) { + StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx); + bool HasTag = Pred->getLocation().getTag(); + Pred = Bldr.generateNode(ME, Pred, nilState, nullptr, + ProgramPoint::PreStmtKind); + assert((Pred || HasTag) && "Should have cached out already!"); + if (!Pred) + return; + getCheckerManager().runCheckersForObjCMessageNil(Dst, Pred, + *Msg, *this); + return; + } + + ExplodedNodeSet dstNonNil; + StmtNodeBuilder Bldr(Pred, dstNonNil, *currBldrCtx); + // Generate a transition to the non-nil state, dropping any potential + // nil flow. + if (notNilState != State) { + bool HasTag = Pred->getLocation().getTag(); + Pred = Bldr.generateNode(ME, Pred, notNilState); + assert((Pred || HasTag) && "Should have cached out already!"); + if (!Pred) + return; + } + } + } + // Handle the previsits checks. ExplodedNodeSet dstPrevisit; getCheckerManager().runCheckersForPreObjCMessage(dstPrevisit, Pred, @@ -160,38 +228,12 @@ void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME, if (UpdatedMsg->isInstanceMessage()) { SVal recVal = UpdatedMsg->getReceiverSVal(); if (!recVal.isUndef()) { - // Bifurcate the state into nil and non-nil ones. - DefinedOrUnknownSVal receiverVal = - recVal.castAs<DefinedOrUnknownSVal>(); - - ProgramStateRef notNilState, nilState; - std::tie(notNilState, nilState) = State->assume(receiverVal); - - // There are three cases: can be nil or non-nil, must be nil, must be - // non-nil. We ignore must be nil, and merge the rest two into non-nil. - // FIXME: This ignores many potential bugs (<rdar://problem/11733396>). - // Revisit once we have lazier constraints. - if (nilState && !notNilState) { - continue; - } - - // Check if the "raise" message was sent. - assert(notNilState); if (ObjCNoRet.isImplicitNoReturn(ME)) { // If we raise an exception, for now treat it as a sink. // Eventually we will want to handle exceptions properly. Bldr.generateSink(ME, Pred, State); continue; } - - // Generate a transition to non-Nil state. - if (notNilState != State) { - bool HasTag = Pred->getLocation().getTag(); - Pred = Bldr.generateNode(ME, Pred, notNilState); - assert((Pred || HasTag) && "Should have cached out already!"); - if (!Pred) - continue; - } } } else { // Check for special class methods that are known to not return |
