From: Jordan Rose Date: Fri, 3 Aug 2012 23:09:01 +0000 (+0000) Subject: [analyzer] When a symbol is null, we should track its constraints. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=685379965c1b105ce89cf4f6c60810932b7f4d0d;p=clang [analyzer] When a symbol is null, we should track its constraints. Because of this, we would previously emit NO path notes when a parameter is constrained to null (because there are no stores). Now we show where we made the assumption, which is much more useful. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@161280 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h index 3e62a920b7..f53c15f117 100644 --- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h +++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h @@ -226,9 +226,8 @@ public: namespace bugreporter { -BugReporterVisitor *getTrackNullOrUndefValueVisitor(const ExplodedNode *N, - const Stmt *S, - BugReport *R); +void addTrackNullOrUndefValueVisitor(const ExplodedNode *N, const Stmt *S, + BugReport *R); const Stmt *GetDerefExpr(const ExplodedNode *N); const Stmt *GetDenomExpr(const ExplodedNode *N); diff --git a/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp b/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp index e337b3ebdf..c582cfc4a8 100644 --- a/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/AttrNonNullChecker.cpp @@ -105,8 +105,7 @@ void AttrNonNullChecker::checkPreCall(const CallEvent &Call, // Highlight the range of the argument that was null. R->addRange(Call.getArgSourceRange(idx)); if (const Expr *ArgE = Call.getArgExpr(idx)) - R->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(errorNode, - ArgE, R)); + bugreporter::addTrackNullOrUndefValueVisitor(errorNode, ArgE, R); // Emit the bug report. C.EmitReport(R); } diff --git a/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp b/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp index 27bc6fb661..955e79ae46 100644 --- a/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp +++ b/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp @@ -432,8 +432,7 @@ void CFRetainReleaseChecker::checkPreStmt(const CallExpr *CE, BugReport *report = new BugReport(*BT, description, N); report->addRange(Arg->getSourceRange()); - report->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, Arg, - report)); + bugreporter::addTrackNullOrUndefValueVisitor(N, Arg, report); C.EmitReport(report); return; } diff --git a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 69373749a2..483082a37f 100644 --- a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -252,8 +252,7 @@ ProgramStateRef CStringChecker::checkNonNull(CheckerContext &C, BugReport *report = new BugReport(*BT, os.str(), N); report->addRange(S->getSourceRange()); - report->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, S, - report)); + bugreporter::addTrackNullOrUndefValueVisitor(N, S, report); C.EmitReport(report); return NULL; } diff --git a/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp b/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp index 70b6241dea..e09d6885a9 100644 --- a/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp @@ -75,7 +75,7 @@ void CallAndMessageChecker::emitBadCall(BugType *BT, CheckerContext &C, BugReport *R = new BugReport(*BT, BT->getName(), N); if (BadE) { R->addRange(BadE->getSourceRange()); - R->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, BadE, R)); + bugreporter::addTrackNullOrUndefValueVisitor(N, BadE, R); } C.EmitReport(R); } @@ -122,8 +122,7 @@ bool CallAndMessageChecker::PreVisitProcessArg(CheckerContext &C, BugReport *R = new BugReport(*BT, Desc, N); R->addRange(argRange); if (argEx) - R->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, argEx, - R)); + bugreporter::addTrackNullOrUndefValueVisitor(N, argEx, R); C.EmitReport(R); } return true; @@ -320,9 +319,7 @@ void CallAndMessageChecker::checkPreObjCMessage(const ObjCMethodCall &msg, // FIXME: getTrackNullOrUndefValueVisitor can't handle "super" yet. if (const Expr *ReceiverE = ME->getInstanceReceiver()) - R->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, - ReceiverE, - R)); + bugreporter::addTrackNullOrUndefValueVisitor(N, ReceiverE, R); C.EmitReport(R); } return; @@ -364,9 +361,7 @@ void CallAndMessageChecker::emitNilReceiverBug(CheckerContext &C, report->addRange(ME->getReceiverRange()); // FIXME: This won't track "self" in messages to super. if (const Expr *receiver = ME->getInstanceReceiver()) { - report->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, - receiver, - report)); + bugreporter::addTrackNullOrUndefValueVisitor(N, receiver, report); } C.EmitReport(report); } diff --git a/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp b/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp index a0022541f9..a94d7a773e 100644 --- a/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp @@ -166,10 +166,8 @@ void DereferenceChecker::reportBug(ProgramStateRef State, const Stmt *S, buf.empty() ? BT_null->getDescription() : buf.str(), N); - report->addVisitor( - bugreporter::getTrackNullOrUndefValueVisitor(N, - bugreporter::GetDerefExpr(N), - report)); + bugreporter::addTrackNullOrUndefValueVisitor(N, bugreporter::GetDerefExpr(N), + report); for (SmallVectorImpl::iterator I = Ranges.begin(), E = Ranges.end(); I!=E; ++I) @@ -193,8 +191,9 @@ void DereferenceChecker::checkLocation(SVal l, bool isLoad, const Stmt* S, BugReport *report = new BugReport(*BT_undef, BT_undef->getDescription(), N); - report->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, - bugreporter::GetDerefExpr(N), report)); + bugreporter::addTrackNullOrUndefValueVisitor(N, + bugreporter::GetDerefExpr(N), + report); report->disablePathPruning(); C.EmitReport(report); } diff --git a/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp b/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp index 5094a03362..dcf6a8603e 100644 --- a/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp @@ -42,8 +42,9 @@ void DivZeroChecker::reportBug(const char *Msg, BugReport *R = new BugReport(*BT, Msg, N); - R->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, - bugreporter::GetDenomExpr(N), R)); + bugreporter::addTrackNullOrUndefValueVisitor(N, + bugreporter::GetDenomExpr(N), + R); C.EmitReport(R); } } diff --git a/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp index 777e9ea219..4cc92ce9e9 100644 --- a/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp @@ -50,8 +50,7 @@ void ObjCAtSyncChecker::checkPreStmt(const ObjCAtSynchronizedStmt *S, "for @synchronized")); BugReport *report = new BugReport(*BT_undef, BT_undef->getDescription(), N); - report->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, Ex, - report)); + bugreporter::addTrackNullOrUndefValueVisitor(N, Ex, report); C.EmitReport(report); } return; @@ -74,8 +73,7 @@ void ObjCAtSyncChecker::checkPreStmt(const ObjCAtSynchronizedStmt *S, "(no synchronization will occur)")); BugReport *report = new BugReport(*BT_null, BT_null->getDescription(), N); - report->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, Ex, - report)); + bugreporter::addTrackNullOrUndefValueVisitor(N, Ex, report); C.EmitReport(report); return; diff --git a/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp b/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp index 0851836f1f..ca2a55d1e7 100644 --- a/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp @@ -55,8 +55,7 @@ void ReturnUndefChecker::checkPreStmt(const ReturnStmt *RS, report->disablePathPruning(); report->addRange(RetE->getSourceRange()); - report->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, RetE, - report)); + bugreporter::addTrackNullOrUndefValueVisitor(N, RetE, report); C.EmitReport(report); } diff --git a/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp index 48b194107e..70a33c76db 100644 --- a/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp @@ -99,7 +99,7 @@ void UndefBranchChecker::checkBranchCondition(const Stmt *Condition, // Emit the bug report. BugReport *R = new BugReport(*BT, BT->getDescription(), N); - R->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, Ex, R)); + bugreporter::addTrackNullOrUndefValueVisitor(N, Ex, R); R->addRange(Ex->getSourceRange()); R->disablePathPruning(); diff --git a/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp index a27fa1dcbe..e220499d73 100644 --- a/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp @@ -76,12 +76,10 @@ void UndefResultChecker::checkPostStmt(const BinaryOperator *B, BugReport *report = new BugReport(*BT, OS.str(), N); if (Ex) { report->addRange(Ex->getSourceRange()); - report->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, Ex, - report)); + bugreporter::addTrackNullOrUndefValueVisitor(N, Ex, report); } else - report->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, B, - report)); + bugreporter::addTrackNullOrUndefValueVisitor(N, B, report); report->disablePathPruning(); C.EmitReport(report); diff --git a/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp index 0297c4eb14..6ae3c1875f 100644 --- a/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp @@ -42,9 +42,7 @@ UndefinedArraySubscriptChecker::checkPreStmt(const ArraySubscriptExpr *A, // Generate a report for this bug. BugReport *R = new BugReport(*BT, BT->getName(), N); R->addRange(A->getIdx()->getSourceRange()); - R->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, - A->getIdx(), - R)); + bugreporter::addTrackNullOrUndefValueVisitor(N, A->getIdx(), R); C.EmitReport(R); } } diff --git a/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp index 7b1081f6bb..14a884e01b 100644 --- a/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp @@ -78,7 +78,7 @@ void UndefinedAssignmentChecker::checkBind(SVal location, SVal val, BugReport *R = new BugReport(*BT, str, N); if (ex) { R->addRange(ex->getSourceRange()); - R->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, ex, R)); + bugreporter::addTrackNullOrUndefValueVisitor(N, ex, R); } R->disablePathPruning(); C.EmitReport(R); diff --git a/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp b/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp index f173cde17d..d35455c219 100644 --- a/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp @@ -224,8 +224,7 @@ bool UnixAPIChecker::ReportZeroByteAllocation(CheckerContext &C, BugReport *report = new BugReport(*BT_mallocZero, os.str(), N); report->addRange(arg->getSourceRange()); - report->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, arg, - report)); + bugreporter::addTrackNullOrUndefValueVisitor(N, arg, report); C.EmitReport(report); return true; diff --git a/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp b/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp index 38c9cc1f33..fab4adf3e0 100644 --- a/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp @@ -69,8 +69,7 @@ void VLASizeChecker::reportBug(VLASize_Kind Kind, BugReport *report = new BugReport(*BT, os.str(), N); report->addRange(SizeE->getSourceRange()); - report->addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, SizeE, - report)); + bugreporter::addTrackNullOrUndefValueVisitor(N, SizeE, report); C.EmitReport(report); return; } diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index 3dedcb3edf..46aa9e2b91 100644 --- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -197,6 +197,9 @@ PathDiagnosticPiece *FindLastStoreBRVisitor::VisitNode(const ExplodedNode *N, os << "declared without an initial value"; } } + else { + os << "initialized here"; + } } } @@ -223,7 +226,7 @@ PathDiagnosticPiece *FindLastStoreBRVisitor::VisitNode(const ExplodedNode *N, << " is assigned to "; } else - return NULL; + os << "Value assigned to "; if (const VarRegion *VR = dyn_cast(R)) { os << '\'' << *VR->getDecl() << '\''; @@ -293,12 +296,11 @@ TrackConstraintBRVisitor::VisitNode(const ExplodedNode *N, return NULL; } -BugReporterVisitor * -bugreporter::getTrackNullOrUndefValueVisitor(const ExplodedNode *N, - const Stmt *S, - BugReport *report) { +void bugreporter::addTrackNullOrUndefValueVisitor(const ExplodedNode *N, + const Stmt *S, + BugReport *report) { if (!S || !N) - return 0; + return; ProgramStateManager &StateMgr = N->getState()->getStateManager(); @@ -314,7 +316,7 @@ bugreporter::getTrackNullOrUndefValueVisitor(const ExplodedNode *N, } if (!N) - return 0; + return; ProgramStateRef state = N->getState(); @@ -331,7 +333,15 @@ bugreporter::getTrackNullOrUndefValueVisitor(const ExplodedNode *N, SVal V = state->getRawSVal(loc::MemRegionVal(R)); report->markInteresting(R); report->markInteresting(V); - return new FindLastStoreBRVisitor(V, R); + + if (V.getAsLocSymbol()) { + BugReporterVisitor *ConstraintTracker + = new TrackConstraintBRVisitor(cast(V), false); + report->addVisitor(ConstraintTracker); + } + + report->addVisitor(new FindLastStoreBRVisitor(V, R)); + return; } } } @@ -351,11 +361,10 @@ bugreporter::getTrackNullOrUndefValueVisitor(const ExplodedNode *N, if (R) { report->markInteresting(R); - return new TrackConstraintBRVisitor(loc::MemRegionVal(R), false); + report->addVisitor(new TrackConstraintBRVisitor(loc::MemRegionVal(R), + false)); } } - - return 0; } BugReporterVisitor * @@ -397,7 +406,7 @@ 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. - BR.addVisitor(bugreporter::getTrackNullOrUndefValueVisitor(N, Receiver, &BR)); + bugreporter::addTrackNullOrUndefValueVisitor(N, Receiver, &BR); // Issue a message saying that the method was skipped. PathDiagnosticLocation L(Receiver, BRC.getSourceManager(), N->getLocationContext()); diff --git a/test/Analysis/inlining/path-notes.c b/test/Analysis/inlining/path-notes.c index 1db3c5aab8..53bc4249b7 100644 --- a/test/Analysis/inlining/path-notes.c +++ b/test/Analysis/inlining/path-notes.c @@ -11,4 +11,46 @@ void testZero(int *a) { // expected-note@-2 {{Returning from 'zero'}} *a = 1; // expected-warning{{Dereference of null pointer}} // expected-note@-1 {{Dereference of null pointer (loaded from variable 'a')}} -} \ No newline at end of file +} + + +void check(int *p) { + if (p) { + // expected-note@-1 + {{Assuming 'p' is null}} + // expected-note@-2 + {{Assuming pointer value is null}} + // expected-note@-3 + {{Taking false branch}} + return; + } + return; +} + +void testCheck(int *a) { + check(a); + // expected-note@-1 {{Calling 'check'}} + // expected-note@-2 {{Returning from 'check'}} + *a = 1; // expected-warning{{Dereference of null pointer}} + // expected-note@-1 {{Dereference of null pointer (loaded from variable 'a')}} +} + + +int *getPointer(); + +void testInitCheck() { + int *a = getPointer(); + // expected-note@-1 {{Variable 'a' initialized here}} + check(a); + // expected-note@-1 {{Calling 'check'}} + // expected-note@-2 {{Returning from 'check'}} + *a = 1; // expected-warning{{Dereference of null pointer}} + // expected-note@-1 {{Dereference of null pointer (loaded from variable 'a')}} +} + +void testStoreCheck(int *a) { + a = getPointer(); + // expected-note@-1 {{Value assigned to 'a'}} + check(a); + // expected-note@-1 {{Calling 'check'}} + // expected-note@-2 {{Returning from 'check'}} + *a = 1; // expected-warning{{Dereference of null pointer}} + // expected-note@-1 {{Dereference of null pointer (loaded from variable 'a')}} +} diff --git a/test/Analysis/method-call-path-notes.cpp b/test/Analysis/method-call-path-notes.cpp index 1e17b838cc..6298ca043f 100644 --- a/test/Analysis/method-call-path-notes.cpp +++ b/test/Analysis/method-call-path-notes.cpp @@ -24,7 +24,7 @@ void test_ic_set_to_null() { } void test_ic_null(TestInstanceCall *p) { - if (!p) // expected-note {{Taking true branch}} + if (!p) // expected-note {{Assuming pointer value is null}} expected-note {{Taking true branch}} p->foo(); // expected-warning {{Called C++ object pointer is null}} expected-note{{Called C++ object pointer is null}} }