From: Anna Zaks Date: Thu, 28 Mar 2013 23:15:22 +0000 (+0000) Subject: [analyzer] Apply the suppression rules to the nil receiver only if the value particip... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=aabb4c5eacca6d78ef778f33ec5cd4c755d71a39;p=clang [analyzer] Apply the suppression rules to the nil receiver only if the value participates in the computation of the nil we warn about. We should only suppress a bug report if the IDCed or null returned nil value is directly related to the value we are warning about. This was not the case for nil receivers - we would suppress a bug report that had an IDCed nil receiver on the path regardless of how it’s related to the warning. 1) Thread EnableNullFPSuppression parameter through the visitors to differentiate between tracking the value which is directly responsible for the bug and other values that visitors are tracking (ex: general tracking of nil receivers). 2) in trackNullOrUndef specifically address the case when a value of the message send is nil due to the receiver being nil. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@178309 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h index fab70e935b..6eb5f259b3 100644 --- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h +++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h @@ -101,21 +101,22 @@ class FindLastStoreBRVisitor SVal V; bool Satisfied; -public: - /// \brief Convenience method to create a visitor given only the MemRegion. - /// Returns NULL if the visitor cannot be created. For example, when the - /// corresponding value is unknown. - static BugReporterVisitor *createVisitorObject(const ExplodedNode *N, - const MemRegion *R); + /// If the visitor is tracking the value directly responsible for the + /// bug, we are going to employ false positive suppression. + bool EnableNullFPSuppression; +public: /// Creates a visitor for every VarDecl inside a Stmt and registers it with /// the BugReport. - static void registerStatementVarDecls(BugReport &BR, const Stmt *S); + static void registerStatementVarDecls(BugReport &BR, const Stmt *S, + bool EnableNullFPSuppression); - FindLastStoreBRVisitor(KnownSVal V, const MemRegion *R) + FindLastStoreBRVisitor(KnownSVal V, const MemRegion *R, + bool InEnableNullFPSuppression) : R(R), V(V), - Satisfied(false) {} + Satisfied(false), + EnableNullFPSuppression(InEnableNullFPSuppression) {} void Profile(llvm::FoldingSetNodeID &ID) const; @@ -159,16 +160,33 @@ private: /// \brief Prints path notes when a message is sent to a nil receiver. class NilReceiverBRVisitor : public BugReporterVisitorImpl { + + /// \brief The reciever to track. If null, all receivers should be tracked. + const Expr *TrackedReceiver; + + /// If the visitor is tracking the value directly responsible for the + /// bug, we are going to employ false positive suppression. + bool EnableNullFPSuppression; + public: + NilReceiverBRVisitor(const Expr *InTrackedReceiver = 0, + bool InEnableNullFPSuppression = false): + TrackedReceiver(InTrackedReceiver), + EnableNullFPSuppression(InEnableNullFPSuppression) {} + void Profile(llvm::FoldingSetNodeID &ID) const { static int x = 0; ID.AddPointer(&x); + ID.AddPointer(TrackedReceiver); + ID.AddBoolean(EnableNullFPSuppression); } PathDiagnosticPiece *VisitNode(const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC, BugReport &BR); + + static const Expr *getReceiver(const Stmt *S); }; /// Visitor that tries to report interesting diagnostics from conditions. @@ -332,12 +350,15 @@ namespace bugreporter { /// \param IsArg Whether the statement is an argument to an inlined function. /// If this is the case, \p N \em must be the CallEnter node for /// the function. +/// \param EnableNullFPSuppression Whether we should employ false positive +/// suppression (inlined defensive checks, returned null). /// /// \return Whether or not the function was able to add visitors for this /// statement. Note that returning \c true does not actually imply /// that any visitors were added. bool trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S, BugReport &R, - bool IsArg = false); + bool IsArg = false, + bool EnableNullFPSuppression = true); const Expr *getDerefExpr(const Stmt *S); const Stmt *GetDenomExpr(const ExplodedNode *N); diff --git a/lib/StaticAnalyzer/Checkers/IdempotentOperationChecker.cpp b/lib/StaticAnalyzer/Checkers/IdempotentOperationChecker.cpp index 579ba9cf80..271ba4702c 100644 --- a/lib/StaticAnalyzer/Checkers/IdempotentOperationChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/IdempotentOperationChecker.cpp @@ -423,12 +423,12 @@ void IdempotentOperationChecker::checkEndAnalysis(ExplodedGraph &G, if (LHSRelevant) { const Expr *LHS = i->first->getLHS(); report->addRange(LHS->getSourceRange()); - FindLastStoreBRVisitor::registerStatementVarDecls(*report, LHS); + FindLastStoreBRVisitor::registerStatementVarDecls(*report, LHS, false); } if (RHSRelevant) { const Expr *RHS = i->first->getRHS(); report->addRange(i->first->getRHS()->getSourceRange()); - FindLastStoreBRVisitor::registerStatementVarDecls(*report, RHS); + FindLastStoreBRVisitor::registerStatementVarDecls(*report, RHS, false); } BR.emitReport(report); diff --git a/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp index f0ca8a8312..93812f7148 100644 --- a/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp @@ -91,7 +91,8 @@ UndefCapturedBlockVarChecker::checkPostStmt(const BlockExpr *BE, BugReport *R = new BugReport(*BT, os.str(), N); if (const Expr *Ex = FindBlockDeclRefExpr(BE->getBody(), VD)) R->addRange(Ex->getSourceRange()); - R->addVisitor(new FindLastStoreBRVisitor(*V, VR)); + R->addVisitor(new FindLastStoreBRVisitor(*V, VR, + /*EnableNullFPSuppression*/false)); R->disablePathPruning(); // need location of block C.emitReport(R); diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 3146e6c60e..31ba5729ea 100644 --- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -137,11 +137,12 @@ class ReturnVisitor : public BugReporterVisitorImpl { MaybeUnsuppress, Satisfied } Mode; - bool InitiallySuppressed; + + bool EnableNullFPSuppression; public: ReturnVisitor(const StackFrameContext *Frame, bool Suppressed) - : StackFrame(Frame), Mode(Initial), InitiallySuppressed(Suppressed) {} + : StackFrame(Frame), Mode(Initial), EnableNullFPSuppression(Suppressed) {} static void *getTag() { static int Tag = 0; @@ -151,7 +152,7 @@ public: virtual void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddPointer(ReturnVisitor::getTag()); ID.AddPointer(StackFrame); - ID.AddBoolean(InitiallySuppressed); + ID.AddBoolean(EnableNullFPSuppression); } /// Adds a ReturnVisitor if the given statement represents a call that was @@ -162,7 +163,8 @@ public: /// the statement is a call that was inlined, we add the visitor to the /// bug report, so it can print a note later. static void addVisitorIfNecessary(const ExplodedNode *Node, const Stmt *S, - BugReport &BR) { + BugReport &BR, + bool InEnableNullFPSuppression) { if (!CallEvent::isCallStmt(S)) return; @@ -207,13 +209,13 @@ public: assert(Eng && "Cannot file a bug report without an owning engine"); AnalyzerOptions &Options = Eng->getAnalysisManager().options; - bool InitiallySuppressed = false; - if (Options.shouldSuppressNullReturnPaths()) + bool EnableNullFPSuppression = false; + if (InEnableNullFPSuppression && Options.shouldSuppressNullReturnPaths()) if (Optional RetLoc = RetVal.getAs()) - InitiallySuppressed = State->isNull(*RetLoc).isConstrainedTrue(); + EnableNullFPSuppression = State->isNull(*RetLoc).isConstrainedTrue(); BR.markInteresting(CalleeContext); - BR.addVisitor(new ReturnVisitor(CalleeContext, InitiallySuppressed)); + BR.addVisitor(new ReturnVisitor(CalleeContext, EnableNullFPSuppression)); } /// Returns true if any counter-suppression heuristics are enabled for @@ -272,12 +274,14 @@ public: // make sure to track it into any further inner functions. if (!State->isNull(V).isConstrainedTrue()) { BR.markInteresting(V); - ReturnVisitor::addVisitorIfNecessary(N, RetE, BR); + ReturnVisitor::addVisitorIfNecessary(N, RetE, BR, + EnableNullFPSuppression); return 0; } // If we're returning 0, we should track where that 0 came from. - bugreporter::trackNullOrUndefValue(N, RetE, BR); + bugreporter::trackNullOrUndefValue(N, RetE, BR, /*IsArg*/ false, + EnableNullFPSuppression); // Build an appropriate message based on the return value. SmallString<64> Msg; @@ -289,7 +293,7 @@ public: // the report is resurrected as valid later on. ExprEngine &Eng = BRC.getBugReporter().getEngine(); AnalyzerOptions &Options = Eng.getAnalysisManager().options; - if (InitiallySuppressed && hasCounterSuppression(Options)) + if (EnableNullFPSuppression && hasCounterSuppression(Options)) Mode = MaybeUnsuppress; if (RetE->getType()->isObjCObjectPointerType()) @@ -360,7 +364,8 @@ public: if (!State->isNull(*ArgV).isConstrainedTrue()) continue; - if (bugreporter::trackNullOrUndefValue(N, ArgE, BR, /*IsArg=*/true)) + if (bugreporter::trackNullOrUndefValue(N, ArgE, BR, /*IsArg=*/true, + EnableNullFPSuppression)) BR.removeInvalidation(ReturnVisitor::getTag(), StackFrame); // If we /can't/ track the null pointer, we should err on the side of @@ -390,7 +395,7 @@ public: PathDiagnosticPiece *getEndPath(BugReporterContext &BRC, const ExplodedNode *N, BugReport &BR) { - if (InitiallySuppressed) + if (EnableNullFPSuppression) BR.markInvalid(ReturnVisitor::getTag(), StackFrame); return 0; } @@ -403,6 +408,7 @@ void FindLastStoreBRVisitor ::Profile(llvm::FoldingSetNodeID &ID) const { ID.AddPointer(&tag); ID.AddPointer(R); ID.Add(V); + ID.AddBoolean(EnableNullFPSuppression); } PathDiagnosticPiece *FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ, @@ -487,10 +493,11 @@ PathDiagnosticPiece *FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ, if (V.isUndef() || V.getAs()) { if (!IsParam) InitE = InitE->IgnoreParenCasts(); - bugreporter::trackNullOrUndefValue(StoreSite, InitE, BR, IsParam); + bugreporter::trackNullOrUndefValue(StoreSite, InitE, BR, IsParam, + EnableNullFPSuppression); } else { ReturnVisitor::addVisitorIfNecessary(StoreSite, InitE->IgnoreParenCasts(), - BR); + BR, EnableNullFPSuppression); } } @@ -520,7 +527,8 @@ PathDiagnosticPiece *FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ, if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) { if (Optional KV = State->getSVal(OriginalR).getAs()) - BR.addVisitor(new FindLastStoreBRVisitor(*KV, OriginalR)); + BR.addVisitor(new FindLastStoreBRVisitor(*KV, OriginalR, + EnableNullFPSuppression)); } } } @@ -807,7 +815,8 @@ static const Expr *peelOffOuterExpr(const Stmt *S, bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S, - BugReport &report, bool IsArg) { + BugReport &report, bool IsArg, + bool EnableNullFPSuppression) { if (!S || !N) return false; @@ -815,6 +824,12 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N, S = Ex; } + // The message send could be null if the receiver is null. + if (const Expr *Receiver = NilReceiverBRVisitor::getReceiver(S)) { + report.addVisitor(new NilReceiverBRVisitor(Receiver, + EnableNullFPSuppression)); + } + const Expr *Inner = 0; if (const Expr *Ex = dyn_cast(S)) { Ex = Ex->IgnoreParenCasts(); @@ -881,7 +896,8 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N, // got initialized. if (const MemRegion *RR = getLocationRegionIfReference(Inner, N)) { if (Optional KV = LVal.getAs()) - report.addVisitor(new FindLastStoreBRVisitor(*KV, RR)); + report.addVisitor(new FindLastStoreBRVisitor(*KV, RR, + EnableNullFPSuppression)); } } @@ -921,7 +937,8 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N, report.addVisitor(ConstraintTracker); // Add visitor, which will suppress inline defensive checks. - if (N->getState()->isNull(V).isConstrainedTrue()) { + if (N->getState()->isNull(V).isConstrainedTrue() && + EnableNullFPSuppression) { BugReporterVisitor *IDCSuppressor = new SuppressInlineDefensiveChecksVisitor(V.castAs(), N); @@ -930,7 +947,8 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N, } if (Optional KV = V.getAs()) - report.addVisitor(new FindLastStoreBRVisitor(*KV, R)); + report.addVisitor(new FindLastStoreBRVisitor(*KV, R, + EnableNullFPSuppression)); return true; } } @@ -943,7 +961,8 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N, // sure that function isn't pruned in our output. if (const Expr *E = dyn_cast(S)) S = E->IgnoreParenCasts(); - ReturnVisitor::addVisitorIfNecessary(N, S, report); + + ReturnVisitor::addVisitorIfNecessary(N, S, report, EnableNullFPSuppression); // Uncomment this to find cases where we aren't properly getting the // base value that was dereferenced. @@ -966,15 +985,11 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N, return true; } -BugReporterVisitor * -FindLastStoreBRVisitor::createVisitorObject(const ExplodedNode *N, - const MemRegion *R) { - assert(R && "The memory region is null."); - - ProgramStateRef state = N->getState(); - if (Optional KV = state->getSVal(R).getAs()) - return new FindLastStoreBRVisitor(*KV, R); - return 0; +const Expr *NilReceiverBRVisitor::getReceiver(const Stmt *S) { + const ObjCMessageExpr *ME = dyn_cast(S); + if (!ME) + return 0; + return ME->getInstanceReceiver(); } PathDiagnosticPiece *NilReceiverBRVisitor::VisitNode(const ExplodedNode *N, @@ -984,13 +999,15 @@ PathDiagnosticPiece *NilReceiverBRVisitor::VisitNode(const ExplodedNode *N, Optional P = N->getLocationAs(); if (!P) return 0; - const ObjCMessageExpr *ME = P->getStmtAs(); - if (!ME) - return 0; - const Expr *Receiver = ME->getInstanceReceiver(); + + const Expr *Receiver = getReceiver(P->getStmt()); if (!Receiver) return 0; + // Are we tracking a different reciever? + if (TrackedReceiver && TrackedReceiver != Receiver) + return 0; + ProgramStateRef state = N->getState(); SVal V = state->getSVal(Receiver, N->getLocationContext()); if (!state->isNull(V).isConstrainedTrue()) @@ -999,7 +1016,8 @@ PathDiagnosticPiece *NilReceiverBRVisitor::VisitNode(const ExplodedNode *N, // The receiver was nil, and hence the method was skipped. // Register a BugReporterVisitor to issue a message telling us how // the receiver was null. - bugreporter::trackNullOrUndefValue(N, Receiver, BR); + bugreporter::trackNullOrUndefValue(N, Receiver, BR, /*IsArg*/ false, + EnableNullFPSuppression); // Issue a message saying that the method was skipped. PathDiagnosticLocation L(Receiver, BRC.getSourceManager(), N->getLocationContext()); @@ -1009,7 +1027,8 @@ PathDiagnosticPiece *NilReceiverBRVisitor::VisitNode(const ExplodedNode *N, // Registers every VarDecl inside a Stmt with a last store visitor. void FindLastStoreBRVisitor::registerStatementVarDecls(BugReport &BR, - const Stmt *S) { + const Stmt *S, + bool EnableNullFPSuppression) { const ExplodedNode *N = BR.getErrorNode(); std::deque WorkList; WorkList.push_back(S); @@ -1031,7 +1050,8 @@ void FindLastStoreBRVisitor::registerStatementVarDecls(BugReport &BR, if (V.getAs() || V.getAs()) { // Register a new visitor with the BugReport. - BR.addVisitor(new FindLastStoreBRVisitor(V.castAs(), R)); + BR.addVisitor(new FindLastStoreBRVisitor(V.castAs(), R, + EnableNullFPSuppression)); } } } diff --git a/test/Analysis/NSContainers.m b/test/Analysis/NSContainers.m index 828a9acfdd..d6fded5fd0 100644 --- a/test/Analysis/NSContainers.m +++ b/test/Analysis/NSContainers.m @@ -153,15 +153,24 @@ void testIDC(NSMutableDictionary *d, NSString *key) { d[key] = @"abc"; // no-warning } -@interface Foo +@interface Foo { +@public + int x; +} - (int *)getPtr; - (int)getInt; +- (NSMutableDictionary *)getDictPtr; +@property (retain, readonly, nonatomic) Foo* data; +- (NSString*) stringForKeyFE: (id)key; @end void idc2(id x) { if (!x) return; } +Foo *retNil() { + return 0; +} void testIDC2(Foo *obj) { idc2(obj); @@ -173,3 +182,19 @@ int testIDC3(Foo *obj) { return 1/[obj getInt]; } +void testNilReceiverIDC(Foo *obj, NSString *key) { + NSMutableDictionary *D = [obj getDictPtr]; + idc(D); + D[key] = @"abc"; // no-warning +} + +void testNilReceiverRetNil2(NSMutableDictionary *D, Foo *FooPtrIn, id value) { + NSString* const kKeyIdentifier = @"key"; + Foo *FooPtr = retNil(); + NSString *key = [[FooPtr data] stringForKeyFE: kKeyIdentifier]; + // key is nil because FooPtr is nil. However, FooPtr is set to nil inside an + // inlined function, so this error report should be suppressed. + [D setObject: value forKey: key]; // no-warning +} + + diff --git a/test/Analysis/inlining/inline-defensive-checks.m b/test/Analysis/inlining/inline-defensive-checks.m index bafc812486..a757af3d2f 100644 --- a/test/Analysis/inlining/inline-defensive-checks.m +++ b/test/Analysis/inlining/inline-defensive-checks.m @@ -16,7 +16,6 @@ typedef struct objc_object { -(id)retain; @end -// expected-no-diagnostics // Check that inline defensive checks is triggered for null expressions // within CompoundLiteralExpr. typedef union { @@ -44,3 +43,71 @@ dispatch_resume(dispatch_object_t object); dispatch_resume(p); // no warning } @end + +// Test nil receiver suppression. +// We only suppress on nil receiver if the nil value is directly causing the bug. +@interface Foo { +@public + int x; +} +- (Foo *)getFooPtr; +@end + +Foo *retNil() { + return 0; +} + +Foo *retInputOrNil(Foo *p) { + if (p) + return p; + return 0; +} + +void idc(Foo *p) { + if (p) + ; +} + +int testNilReceiver(Foo* fPtr) { + if (fPtr) + ; + // On a path where fPtr is nil, mem should be nil. + Foo *mem = [fPtr getFooPtr]; + return mem->x; // expected-warning {{Access to instance variable 'x' results in a dereference of a null pointer}} +} + +int dontSuppressNilReceiverRetNullCond(Foo* fPtr) { + unsigned zero = 0; + fPtr = retInputOrNil(fPtr); + // On a path where fPtr is nil, mem should be nil. + // The warning is not suppressed because the receiver being nil is not + // directly related to the value that triggers the warning. + Foo *mem = [fPtr getFooPtr]; + if (!mem) + return 5/zero; // expected-warning {{Division by zero}} + return 0; +} + +int dontSuppressNilReceiverRetNull(Foo* fPtr) { + unsigned zero = 0; + fPtr = retNil(); + // On a path where fPtr is nil, mem should be nil. + // The warning is not suppressed because the receiver being nil is not + // directly related to the value that triggers the warning. + Foo *mem = [fPtr getFooPtr]; + if (!mem) + return 5/zero; // expected-warning {{Division by zero}} + return 0; +} + +int dontSuppressNilReceiverIDC(Foo* fPtr) { + unsigned zero = 0; + idc(fPtr); + // On a path where fPtr is nil, mem should be nil. + // The warning is not suppressed because the receiver being nil is not + // directly related to the value that triggers the warning. + Foo *mem = [fPtr getFooPtr]; + if (!mem) + return 5/zero; // expected-warning {{Division by zero}} + return 0; +}