From 0e7d51e97642e26cc852cca312acd3e1593a9989 Mon Sep 17 00:00:00 2001 From: Devin Coughlin Date: Tue, 15 Sep 2015 01:13:53 +0000 Subject: [PATCH] [analyzer] Skip Pre/Post handlers for ObjC calls when receiver is nil. In Objective-C, method calls with nil receivers are essentially no-ops. They do not fault (although the returned value may be garbage depending on the declared return type and architecture). Programmers are aware of this behavior and will complain about a false alarm when the analyzer diagnoses API violations for method calls when the receiver is known to be nil. Rather than require each individual checker to be aware of this behavior and suppress a warning when the receiver is nil, this commit changes ExprEngineObjC so that VisitObjCMessage skips calling checker pre/post handlers when the receiver is definitely nil. Instead, it adds a new event, ObjCMessageNil, that is only called in that case. The CallAndMessageChecker explicitly cares about this case, so I've changed it to add a callback for ObjCMessageNil and moved the logic in PreObjCMessage that handles nil receivers to the new callback. rdar://problem/18092611 Differential Revision: http://reviews.llvm.org/D12123 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@247653 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/StaticAnalyzer/Core/Checker.h | 15 +++ .../StaticAnalyzer/Core/CheckerManager.h | 30 +++++- .../Checkers/CallAndMessageChecker.cpp | 25 +++-- .../Checkers/CheckerDocumentation.cpp | 10 ++ lib/StaticAnalyzer/Core/CheckerManager.cpp | 47 ++++++++-- lib/StaticAnalyzer/Core/ExprEngineObjC.cpp | 94 ++++++++++++++----- test/Analysis/NSContainers.m | 19 ++++ test/Analysis/objc-message.m | 40 ++++++++ 8 files changed, 230 insertions(+), 50 deletions(-) create mode 100644 test/Analysis/objc-message.m diff --git a/include/clang/StaticAnalyzer/Core/Checker.h b/include/clang/StaticAnalyzer/Core/Checker.h index 6468553d98..1410af1568 100644 --- a/include/clang/StaticAnalyzer/Core/Checker.h +++ b/include/clang/StaticAnalyzer/Core/Checker.h @@ -131,6 +131,21 @@ public: } }; +class ObjCMessageNil { + template + static void _checkObjCMessage(void *checker, const ObjCMethodCall &msg, + CheckerContext &C) { + ((const CHECKER *)checker)->checkObjCMessageNil(msg, C); + } + +public: + template + static void _register(CHECKER *checker, CheckerManager &mgr) { + mgr._registerForObjCMessageNil( + CheckerManager::CheckObjCMessageFunc(checker, _checkObjCMessage)); + } +}; + class PostObjCMessage { template static void _checkObjCMessage(void *checker, const ObjCMethodCall &msg, diff --git a/include/clang/StaticAnalyzer/Core/CheckerManager.h b/include/clang/StaticAnalyzer/Core/CheckerManager.h index 3e07052aae..bc9af496b0 100644 --- a/include/clang/StaticAnalyzer/Core/CheckerManager.h +++ b/include/clang/StaticAnalyzer/Core/CheckerManager.h @@ -93,6 +93,12 @@ public: StringRef getName() const { return Name; } }; +enum class ObjCMessageVisitKind { + Pre, + Post, + MessageNil +}; + class CheckerManager { const LangOptions LangOpts; AnalyzerOptionsRef AOptions; @@ -211,7 +217,7 @@ public: const ExplodedNodeSet &Src, const ObjCMethodCall &msg, ExprEngine &Eng) { - runCheckersForObjCMessage(/*isPreVisit=*/true, Dst, Src, msg, Eng); + runCheckersForObjCMessage(ObjCMessageVisitKind::Pre, Dst, Src, msg, Eng); } /// \brief Run checkers for post-visiting obj-c messages. @@ -220,12 +226,22 @@ public: const ObjCMethodCall &msg, ExprEngine &Eng, bool wasInlined = false) { - runCheckersForObjCMessage(/*isPreVisit=*/false, Dst, Src, msg, Eng, + runCheckersForObjCMessage(ObjCMessageVisitKind::Post, Dst, Src, msg, Eng, wasInlined); } + /// \brief Run checkers for visiting an obj-c message to nil. + void runCheckersForObjCMessageNil(ExplodedNodeSet &Dst, + const ExplodedNodeSet &Src, + const ObjCMethodCall &msg, + ExprEngine &Eng) { + runCheckersForObjCMessage(ObjCMessageVisitKind::MessageNil, Dst, Src, msg, + Eng); + } + + /// \brief Run checkers for visiting obj-c messages. - void runCheckersForObjCMessage(bool isPreVisit, + void runCheckersForObjCMessage(ObjCMessageVisitKind visitKind, ExplodedNodeSet &Dst, const ExplodedNodeSet &Src, const ObjCMethodCall &msg, ExprEngine &Eng, @@ -457,6 +473,8 @@ public: void _registerForPreObjCMessage(CheckObjCMessageFunc checkfn); void _registerForPostObjCMessage(CheckObjCMessageFunc checkfn); + void _registerForObjCMessageNil(CheckObjCMessageFunc checkfn); + void _registerForPreCall(CheckCallFunc checkfn); void _registerForPostCall(CheckCallFunc checkfn); @@ -557,8 +575,14 @@ private: const CachedStmtCheckers &getCachedStmtCheckersFor(const Stmt *S, bool isPreVisit); + /// Returns the checkers that have registered for callbacks of the + /// given \p Kind. + const std::vector & + getObjCMessageCheckers(ObjCMessageVisitKind Kind); + std::vector PreObjCMessageCheckers; std::vector PostObjCMessageCheckers; + std::vector ObjCMessageNilCheckers; std::vector PreCallCheckers; std::vector PostCallCheckers; diff --git a/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp b/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp index 26423b7368..33a24d816c 100644 --- a/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp @@ -40,6 +40,7 @@ class CallAndMessageChecker : public Checker< check::PreStmt, check::PreStmt, check::PreObjCMessage, + check::ObjCMessageNil, check::PreCall > { mutable std::unique_ptr BT_call_null; mutable std::unique_ptr 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(); - - 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/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp b/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp index 86955c4fcb..37b84480f8 100644 --- a/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp +++ b/lib/StaticAnalyzer/Checkers/CheckerDocumentation.cpp @@ -38,6 +38,7 @@ class CheckerDocumentation : public Checker< check::PreStmt, check::PostStmt, 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/lib/StaticAnalyzer/Core/CheckerManager.cpp b/lib/StaticAnalyzer/Core/CheckerManager.cpp index d26a76abe8..c464d301b6 100644 --- a/lib/StaticAnalyzer/Core/CheckerManager.cpp +++ b/lib/StaticAnalyzer/Core/CheckerManager.cpp @@ -177,7 +177,9 @@ void CheckerManager::runCheckersForStmt(bool isPreVisit, namespace { struct CheckObjCMessageContext { typedef std::vector 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::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/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp index da66a46e18..1d77714a00 100644 --- a/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp +++ b/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp @@ -139,6 +139,74 @@ void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME, CallEventRef 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 (). + // 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(); + 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(); - - 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 (). - // 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 diff --git a/test/Analysis/NSContainers.m b/test/Analysis/NSContainers.m index 402ce2c90a..d04b331bf7 100644 --- a/test/Analysis/NSContainers.m +++ b/test/Analysis/NSContainers.m @@ -24,6 +24,8 @@ typedef struct _NSZone NSZone; @interface NSObject {} - (id)init; + (id)alloc; + +- (id)mutableCopy; @end typedef struct { @@ -292,3 +294,20 @@ void testArrayCategory(NSMutableArray *arr) { [arr addObject:0 safe:1]; // no-warning } +@interface MyView : NSObject +-(NSArray *)subviews; +@end + +void testNoReportWhenReceiverNil(NSMutableArray *array, int b) { + // Don't warn about adding nil to a container when the receiver is also + // definitely nil. + if (array == 0) { + [array addObject:0]; // no-warning + } + + MyView *view = b ? [[MyView alloc] init] : 0; + NSMutableArray *subviews = [[view subviews] mutableCopy]; + // When view is nil, subviews is also nil so there should be no warning + // here either. + [subviews addObject:view]; // no-warning +} diff --git a/test/Analysis/objc-message.m b/test/Analysis/objc-message.m new file mode 100644 index 0000000000..dc0fd1f4c5 --- /dev/null +++ b/test/Analysis/objc-message.m @@ -0,0 +1,40 @@ +// RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-store=region -verify -Wno-objc-root-class %s + +extern void clang_analyzer_warnIfReached(); +void clang_analyzer_eval(int); + +@interface SomeClass +-(id)someMethodWithReturn; +-(void)someMethod; +@end + +void consistencyOfReturnWithNilReceiver(SomeClass *o) { + id result = [o someMethodWithReturn]; + if (result) { + if (!o) { + // It is impossible for both o to be nil and result to be non-nil, + // so this should not be reached. + clang_analyzer_warnIfReached(); // no-warning + } + } +} + +void maybeNilReceiverIsNotNilAfterMessage(SomeClass *o) { + [o someMethod]; + + // We intentionally drop the nil flow (losing coverage) after a method + // call when the receiver may be nil in order to avoid inconsistencies of + // the kind tested for in consistencyOfReturnWithNilReceiver(). + clang_analyzer_eval(o != 0); // expected-warning{{TRUE}} +} + +void nilReceiverIsStillNilAfterMessage(SomeClass *o) { + if (o == 0) { + id result = [o someMethodWithReturn]; + + // Both the receiver and the result should be nil after a message + // sent to a nil receiver returning a value of type id. + clang_analyzer_eval(o == 0); // expected-warning{{TRUE}} + clang_analyzer_eval(result == 0); // expected-warning{{TRUE}} + } +} -- 2.40.0