diff options
6 files changed, 186 insertions, 52 deletions
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<NilReceiverBRVisitor> { + + /// \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<ReturnVisitor> { 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<Loc> RetLoc = RetVal.getAs<Loc>()) - 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<loc::ConcreteInt>()) { 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<KnownSVal> KV = State->getSVal(OriginalR).getAs<KnownSVal>()) - 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<Expr>(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<KnownSVal> KV = LVal.getAs<KnownSVal>()) - 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<DefinedSVal>(), N); @@ -930,7 +947,8 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N, } if (Optional<KnownSVal> KV = V.getAs<KnownSVal>()) - 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<Expr>(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<KnownSVal> KV = state->getSVal(R).getAs<KnownSVal>()) - return new FindLastStoreBRVisitor(*KV, R); - return 0; +const Expr *NilReceiverBRVisitor::getReceiver(const Stmt *S) { + const ObjCMessageExpr *ME = dyn_cast<ObjCMessageExpr>(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<PreStmt> P = N->getLocationAs<PreStmt>(); if (!P) return 0; - const ObjCMessageExpr *ME = P->getStmtAs<ObjCMessageExpr>(); - 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<const Stmt *> WorkList; WorkList.push_back(S); @@ -1031,7 +1050,8 @@ void FindLastStoreBRVisitor::registerStatementVarDecls(BugReport &BR, if (V.getAs<loc::ConcreteInt>() || V.getAs<nonloc::ConcreteInt>()) { // Register a new visitor with the BugReport. - BR.addVisitor(new FindLastStoreBRVisitor(V.castAs<KnownSVal>(), R)); + BR.addVisitor(new FindLastStoreBRVisitor(V.castAs<KnownSVal>(), 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<NSCopying>)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; +} |