From fee96e043108b6e24e7d4c5464bf89ac970a7f81 Mon Sep 17 00:00:00 2001 From: Ted Kremenek Date: Tue, 24 Nov 2009 21:41:28 +0000 Subject: [PATCH] Cleanups and fixes to the nil-receiver checker, some of it fallout the initial transition of the nil-receiver checker to the Checker interface as done in r89745. Some important changes include: 1) We consolidate the BugType object used for nil receiver bug reports, and don't include the type of the returned value in the BugType (which would be wrong if a nil receiver bug was reported more than once) 2) Added a new (temporary) flag to CheckerContext: DoneEvauating. This is used by GRExprEngine when evaluating message expressions to not continue evaluating the message expression if this flag is set. This flag is currently set by the nil receiver checker. This is an intermediate solution to allow the nil-receiver checker to properly work as a plug-in outside of GRExprEngine. Basically, this flag indicates that the entire message expression has been evaluated, not just a precondition (which is what the nil-receiver checker does). This flag *should not* be repurposed for general use, but just to pull more things out of GRExprEngine that already in there as we devise a better interface in the Checker class. 3) Cleaned up the logic in the nil-receiver checker, making the control-flow a lot easier to read. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@89804 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../clang/Analysis/PathSensitive/Checker.h | 18 +- .../Analysis/PathSensitive/ExplodedGraph.h | 14 +- .../Analysis/PathSensitive/GRExprEngine.h | 2 +- lib/Analysis/CallAndMessageChecker.cpp | 216 +++++++++--------- lib/Analysis/GRExprEngine.cpp | 27 ++- ...ceiver-undefined-larger-than-voidptr-ret.m | 8 +- ...600344-nil-receiver-undefined-struct-ret.m | 4 +- 7 files changed, 162 insertions(+), 127 deletions(-) diff --git a/include/clang/Analysis/PathSensitive/Checker.h b/include/clang/Analysis/PathSensitive/Checker.h index f6a5074fd8..91a4b6d1b1 100644 --- a/include/clang/Analysis/PathSensitive/Checker.h +++ b/include/clang/Analysis/PathSensitive/Checker.h @@ -42,6 +42,7 @@ class CheckerContext { const GRState *state; const Stmt *statement; const unsigned size; + bool DoneEvaluating; // FIXME: This is not a permanent API change. public: CheckerContext(ExplodedNodeSet &dst, GRStmtNodeBuilder &builder, GRExprEngine &eng, ExplodedNode *pred, @@ -52,10 +53,22 @@ public: OldTag(B.Tag, tag), OldPointKind(B.PointKind, K), OldHasGen(B.HasGeneratedNode), - state(st), statement(stmt), size(Dst.size()) {} + state(st), statement(stmt), size(Dst.size()), + DoneEvaluating(false) {} ~CheckerContext(); + // FIXME: This were added to support CallAndMessageChecker to indicating + // to GRExprEngine to "stop evaluating" a message expression under certain + // cases. This is *not* meant to be a permanent API change, and was added + // to aid in the transition of removing logic for checks from GRExprEngine. + void setDoneEvaluating() { + DoneEvaluating = true; + } + bool isDoneEvaluating() const { + return DoneEvaluating; + } + ConstraintManager &getConstraintManager() { return Eng.getConstraintManager(); } @@ -152,7 +165,7 @@ private: friend class GRExprEngine; // FIXME: Remove the 'tag' option. - void GR_Visit(ExplodedNodeSet &Dst, + bool GR_Visit(ExplodedNodeSet &Dst, GRStmtNodeBuilder &Builder, GRExprEngine &Eng, const Stmt *S, @@ -164,6 +177,7 @@ private: _PreVisit(C, S); else _PostVisit(C, S); + return C.isDoneEvaluating(); } // FIXME: Remove the 'tag' option. diff --git a/include/clang/Analysis/PathSensitive/ExplodedGraph.h b/include/clang/Analysis/PathSensitive/ExplodedGraph.h index a7bbdf939f..76cab1ddc1 100644 --- a/include/clang/Analysis/PathSensitive/ExplodedGraph.h +++ b/include/clang/Analysis/PathSensitive/ExplodedGraph.h @@ -352,10 +352,16 @@ public: typedef ImplTy::iterator iterator; typedef ImplTy::const_iterator const_iterator; - inline unsigned size() const { return Impl.size(); } - inline bool empty() const { return Impl.empty(); } - - inline void clear() { Impl.clear(); } + unsigned size() const { return Impl.size(); } + bool empty() const { return Impl.empty(); } + + void clear() { Impl.clear(); } + void insert(const ExplodedNodeSet &S) { + if (empty()) + Impl = S.Impl; + else + Impl.insert(S.begin(), S.end()); + } inline iterator begin() { return Impl.begin(); } inline iterator end() { return Impl.end(); } diff --git a/include/clang/Analysis/PathSensitive/GRExprEngine.h b/include/clang/Analysis/PathSensitive/GRExprEngine.h index 1512b9099f..99ff57443b 100644 --- a/include/clang/Analysis/PathSensitive/GRExprEngine.h +++ b/include/clang/Analysis/PathSensitive/GRExprEngine.h @@ -222,7 +222,7 @@ public: protected: /// CheckerVisit - Dispatcher for performing checker-specific logic /// at specific statements. - void CheckerVisit(Stmt *S, ExplodedNodeSet &Dst, ExplodedNodeSet &Src, + bool CheckerVisit(Stmt *S, ExplodedNodeSet &Dst, ExplodedNodeSet &Src, bool isPrevisit); void CheckerVisitBind(const Stmt *AssignE, const Stmt *StoreE, diff --git a/lib/Analysis/CallAndMessageChecker.cpp b/lib/Analysis/CallAndMessageChecker.cpp index ebc3a4fd56..920f21a22e 100644 --- a/lib/Analysis/CallAndMessageChecker.cpp +++ b/lib/Analysis/CallAndMessageChecker.cpp @@ -27,21 +27,27 @@ class VISIBILITY_HIDDEN CallAndMessageChecker BugType *BT_call_arg; BugType *BT_msg_undef; BugType *BT_msg_arg; - BugType *BT_struct_ret; - BugType *BT_void_ptr; + BugType *BT_msg_ret; public: CallAndMessageChecker() : BT_call_null(0), BT_call_undef(0), BT_call_arg(0), - BT_msg_undef(0), BT_msg_arg(0), BT_struct_ret(0), BT_void_ptr(0) {} + BT_msg_undef(0), BT_msg_arg(0), BT_msg_ret(0) {} static void *getTag() { static int x = 0; return &x; } + void PreVisitCallExpr(CheckerContext &C, const CallExpr *CE); void PreVisitObjCMessageExpr(CheckerContext &C, const ObjCMessageExpr *ME); + private: void EmitBadCall(BugType *BT, CheckerContext &C, const CallExpr *CE); + void EmitNilReceiverBug(CheckerContext &C, const ObjCMessageExpr *ME, + ExplodedNode *N); + + void HandleNilReceiver(CheckerContext &C, const GRState *state, + const ObjCMessageExpr *ME); }; } // end anonymous namespace @@ -142,111 +148,107 @@ void CallAndMessageChecker::PreVisitObjCMessageExpr(CheckerContext &C, } } - // Check if the receiver was nil and then return value a struct. + // Check if the receiver was nil and then returns a value that may + // be garbage. if (const Expr *Receiver = ME->getReceiver()) { - SVal L_untested = state->getSVal(Receiver); - // Assume that the receiver is not NULL. - DefinedOrUnknownSVal L = cast(L_untested); - const GRState *StNotNull = state->Assume(L, true); - - // Assume that the receiver is NULL. - const GRState *StNull = state->Assume(L, false); - - if (StNull) { - QualType RetTy = ME->getType(); - if (RetTy->isRecordType()) { - if (C.getPredecessor()->getParentMap().isConsumedExpr(ME)) { - // The [0 ...] expressions will return garbage. Flag either an - // explicit or implicit error. Because of the structure of this - // function we currently do not bifurfacte the state graph at - // this point. - // FIXME: We should bifurcate and fill the returned struct with - // garbage. - if (ExplodedNode* N = C.GenerateSink(StNull)) { - if (!StNotNull) { - if (!BT_struct_ret) { - std::string sbuf; - llvm::raw_string_ostream os(sbuf); - os << "The receiver in the message expression is 'nil' and " - "results in the returned value (of type '" - << ME->getType().getAsString() - << "') to be garbage or otherwise undefined"; - BT_struct_ret = new BuiltinBug(os.str().c_str()); - } - - EnhancedBugReport *R = new EnhancedBugReport(*BT_struct_ret, - BT_struct_ret->getName(), N); - R->addRange(Receiver->getSourceRange()); - R->addVisitorCreator(bugreporter::registerTrackNullOrUndefValue, - Receiver); - C.EmitReport(R); - return; - } - else - // Do not report implicit bug. - return; - } - } - } else { - ASTContext &Ctx = C.getASTContext(); - if (RetTy != Ctx.VoidTy) { - if (C.getPredecessor()->getParentMap().isConsumedExpr(ME)) { - // sizeof(void *) - const uint64_t voidPtrSize = Ctx.getTypeSize(Ctx.VoidPtrTy); - // sizeof(return type) - const uint64_t returnTypeSize = Ctx.getTypeSize(ME->getType()); - - if (voidPtrSize < returnTypeSize) { - if (ExplodedNode* N = C.GenerateSink(StNull)) { - if (!StNotNull) { - if (!BT_struct_ret) { - std::string sbuf; - llvm::raw_string_ostream os(sbuf); - os << "The receiver in the message expression is 'nil' and " - "results in the returned value (of type '" - << ME->getType().getAsString() - << "' and of size " - << returnTypeSize / 8 - << " bytes) to be garbage or otherwise undefined"; - BT_void_ptr = new BuiltinBug(os.str().c_str()); - } - - EnhancedBugReport *R = new EnhancedBugReport(*BT_void_ptr, - BT_void_ptr->getName(), N); - R->addRange(Receiver->getSourceRange()); - R->addVisitorCreator( - bugreporter::registerTrackNullOrUndefValue, Receiver); - C.EmitReport(R); - return; - } else - // Do not report implicit bug. - return; - } - } - else if (!StNotNull) { - // Handle the safe cases where the return value is 0 if the - // receiver is nil. - // - // FIXME: For now take the conservative approach that we only - // return null values if we *know* that the receiver is nil. - // This is because we can have surprises like: - // - // ... = [[NSScreens screens] objectAtIndex:0]; - // - // What can happen is that [... screens] could return nil, but - // it most likely isn't nil. We should assume the semantics - // of this case unless we have *a lot* more knowledge. - // - SVal V = C.getValueManager().makeZeroVal(ME->getType()); - C.GenerateNode(StNull->BindExpr(ME, V)); - return; - } - } - } - } + DefinedOrUnknownSVal receiverVal = + cast(state->getSVal(Receiver)); + + const GRState *notNullState, *nullState; + llvm::tie(notNullState, nullState) = state->Assume(receiverVal); + + if (nullState && !notNullState) { + HandleNilReceiver(C, nullState, ME); + C.setDoneEvaluating(); // FIXME: eventually remove. + return; } - // Do not propagate null state. - if (StNotNull) - C.GenerateNode(StNotNull); + + assert(notNullState); + state = notNullState; } + + // Add a state transition if the state has changed. + C.addTransition(state); +} + +void CallAndMessageChecker::EmitNilReceiverBug(CheckerContext &C, + const ObjCMessageExpr *ME, + ExplodedNode *N) { + + if (!BT_msg_ret) + BT_msg_ret = + new BuiltinBug("Receiver in message expression is " + "'nil' and returns a garbage value"); + + llvm::SmallString<200> buf; + llvm::raw_svector_ostream os(buf); + os << "The receiver of message '" << ME->getSelector().getAsString() + << "' is nil and returns a value of type '" + << ME->getType().getAsString() << "' that will be garbage"; + + EnhancedBugReport *report = new EnhancedBugReport(*BT_msg_ret, os.str(), N); + const Expr *receiver = ME->getReceiver(); + report->addRange(receiver->getSourceRange()); + report->addVisitorCreator(bugreporter::registerTrackNullOrUndefValue, + receiver); + C.EmitReport(report); +} + +void CallAndMessageChecker::HandleNilReceiver(CheckerContext &C, + const GRState *state, + const ObjCMessageExpr *ME) { + + // Check the return type of the message expression. A message to nil will + // return different values depending on the return type and the architecture. + QualType RetTy = ME->getType(); + + if (RetTy->isStructureType()) { + // FIXME: At some point we shouldn't rely on isConsumedExpr(), but instead + // have the "use of undefined value" be smarter about where the + // undefined value came from. + if (C.getPredecessor()->getParentMap().isConsumedExpr(ME)) { + if (ExplodedNode* N = C.GenerateSink(state)) + EmitNilReceiverBug(C, ME, N); + return; + } + + // The result is not consumed by a surrounding expression. Just propagate + // the current state. + C.addTransition(state); + return; + } + + // Other cases: check if the return type is smaller than void*. + ASTContext &Ctx = C.getASTContext(); + if (RetTy != Ctx.VoidTy && + C.getPredecessor()->getParentMap().isConsumedExpr(ME)) { + // Compute: sizeof(void *) and sizeof(return type) + const uint64_t voidPtrSize = Ctx.getTypeSize(Ctx.VoidPtrTy); + const uint64_t returnTypeSize = Ctx.getTypeSize(ME->getType()); + + if (voidPtrSize < returnTypeSize) { + if (ExplodedNode* N = C.GenerateSink(state)) + EmitNilReceiverBug(C, ME, N); + return; + } + + // Handle the safe cases where the return value is 0 if the + // receiver is nil. + // + // FIXME: For now take the conservative approach that we only + // return null values if we *know* that the receiver is nil. + // This is because we can have surprises like: + // + // ... = [[NSScreens screens] objectAtIndex:0]; + // + // What can happen is that [... screens] could return nil, but + // it most likely isn't nil. We should assume the semantics + // of this case unless we have *a lot* more knowledge. + // + SVal V = C.getValueManager().makeZeroVal(ME->getType()); + C.GenerateNode(state->BindExpr(ME, V)); + return; + } + + C.addTransition(state); } diff --git a/lib/Analysis/GRExprEngine.cpp b/lib/Analysis/GRExprEngine.cpp index a301f2549d..255a6639ac 100644 --- a/lib/Analysis/GRExprEngine.cpp +++ b/lib/Analysis/GRExprEngine.cpp @@ -108,12 +108,12 @@ public: // Checker worklist routines. //===----------------------------------------------------------------------===// -void GRExprEngine::CheckerVisit(Stmt *S, ExplodedNodeSet &Dst, +bool GRExprEngine::CheckerVisit(Stmt *S, ExplodedNodeSet &Dst, ExplodedNodeSet &Src, bool isPrevisit) { if (Checkers.empty()) { - Dst = Src; - return; + Dst.insert(Src); + return false; } ExplodedNodeSet Tmp; @@ -129,8 +129,16 @@ void GRExprEngine::CheckerVisit(Stmt *S, ExplodedNodeSet &Dst, Checker *checker = I->second; for (ExplodedNodeSet::iterator NI = PrevSet->begin(), NE = PrevSet->end(); - NI != NE; ++NI) - checker->GR_Visit(*CurrSet, *Builder, *this, S, *NI, tag, isPrevisit); + NI != NE; ++NI) { + // FIXME: Halting evaluation of the checkers is something we may + // not support later. The design is still evolving. + if (checker->GR_Visit(*CurrSet, *Builder, *this, S, *NI, + tag, isPrevisit)) { + if (CurrSet != &Dst) + Dst.insert(*CurrSet); + return true; + } + } // Update which NodeSet is the current one. PrevSet = CurrSet; @@ -138,6 +146,7 @@ void GRExprEngine::CheckerVisit(Stmt *S, ExplodedNodeSet &Dst, // Don't autotransition. The CheckerContext objects should do this // automatically. + return false; } // FIXME: This is largely copy-paste from CheckerVisit(). Need to @@ -1854,8 +1863,12 @@ void GRExprEngine::VisitObjCMessageExprDispatchHelper(ObjCMessageExpr* ME, // Handle previsits checks. ExplodedNodeSet Src, DstTmp; - Src.Add(Pred); - CheckerVisit(ME, DstTmp, Src, true); + Src.Add(Pred); + + if (CheckerVisit(ME, DstTmp, Src, true)) { + Dst.insert(DstTmp); + return; + } unsigned size = Dst.size(); diff --git a/test/Analysis/nil-receiver-undefined-larger-than-voidptr-ret.m b/test/Analysis/nil-receiver-undefined-larger-than-voidptr-ret.m index 536c4a1b67..46164f8da4 100644 --- a/test/Analysis/nil-receiver-undefined-larger-than-voidptr-ret.m +++ b/test/Analysis/nil-receiver-undefined-larger-than-voidptr-ret.m @@ -28,20 +28,20 @@ void createFoo() { void createFoo2() { MyClass *obj = 0; - long double ld = [obj longDoubleM]; // expected-warning{{The receiver in the message expression is 'nil' and results in the returned value}} + long double ld = [obj longDoubleM]; // expected-warning{{The receiver of message 'longDoubleM' is nil and returns a value of type 'long double' that will be garbage}} } void createFoo3() { MyClass *obj; obj = 0; - long long ll = [obj longlongM]; // expected-warning{{The receiver in the message expression is 'nil' and results in the returned value}} + long long ll = [obj longlongM]; // expected-warning{{The receiver of message 'longlongM' is nil and returns a value of type 'long long' that will be garbage}} } void createFoo4() { MyClass *obj = 0; - double d = [obj doubleM]; // expected-warning{{The receiver in the message expression is 'nil' and results in the returned value}} + double d = [obj doubleM]; // expected-warning{{The receiver of message 'doubleM' is nil and returns a value of type 'double' that will be garbage}} } void createFoo5() { @@ -59,7 +59,7 @@ void handleNilPruneLoop(MyClass *obj) { long long j = [obj longlongM]; // no-warning } - long long j = [obj longlongM]; // expected-warning{{The receiver in the message expression is 'nil' and results in the returned value}} + long long j = [obj longlongM]; // expected-warning{{The receiver of message 'longlongM' is nil and returns a value of type 'long long' that will be garbage}} } int handleVoidInComma() { diff --git a/test/Analysis/rdar-6600344-nil-receiver-undefined-struct-ret.m b/test/Analysis/rdar-6600344-nil-receiver-undefined-struct-ret.m index 7230f21c08..9d6fe5b27d 100644 --- a/test/Analysis/rdar-6600344-nil-receiver-undefined-struct-ret.m +++ b/test/Analysis/rdar-6600344-nil-receiver-undefined-struct-ret.m @@ -15,12 +15,12 @@ typedef struct Foo { int x; } Bar; void createFoo() { MyClass *obj = 0; - Bar f = [obj foo]; // expected-warning{{The receiver in the message expression is 'nil' and results in the returned value (of type 'Bar') to be garbage or otherwise undefined.}} + Bar f = [obj foo]; // expected-warning{{The receiver of message 'foo' is nil and returns a value of type 'Bar' that will be garbage}} } void createFoo2() { MyClass *obj = 0; [obj foo]; // no-warning - Bar f = [obj foo]; // expected-warning{{The receiver in the message expression is 'nil' and results in the returned value (of type 'Bar') to be garbage or otherwise undefined.}} + Bar f = [obj foo]; // expected-warning{{The receiver of message 'foo' is nil and returns a value of type 'Bar' that will be garbage}} } -- 2.40.0