diff options
6 files changed, 222 insertions, 23 deletions
diff --git a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h index ac308365617..fa0754acb15 100644 --- a/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h +++ b/clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h @@ -187,6 +187,9 @@ private: /// \sa shouldPruneNullReturnPaths llvm::Optional<bool> PruneNullReturnPaths; + /// \sa shouldAvoidSuppressingNullArgumentPaths + llvm::Optional<bool> AvoidSuppressingNullArgumentPaths; + /// \sa getGraphTrimInterval llvm::Optional<unsigned> GraphTrimInterval; @@ -245,6 +248,17 @@ public: /// which accepts the values "true" and "false". bool shouldPruneNullReturnPaths(); + /// Returns whether a bug report should \em not be suppressed if its path + /// includes a call with a null argument, even if that call has a null return. + /// + /// This option has no effect when #shouldPruneNullReturnPaths() is false. + /// + /// This is a counter-heuristic to avoid false negatives. + /// + /// This is controlled by the 'avoid-suppressing-null-argument-paths' config + /// option, which accepts the values "true" and "false". + bool shouldAvoidSuppressingNullArgumentPaths(); + // Returns the size of the functions (in basic blocks), which should be // considered to be small enough to always inline. // diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h index a3784739d16..78e35ca82b8 100644 --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h @@ -268,7 +268,11 @@ 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. -void trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S, BugReport &R, +/// +/// \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); const Stmt *GetDerefExpr(const ExplodedNode *N); diff --git a/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp b/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp index 32073d726d9..da88589c869 100644 --- a/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp +++ b/clang/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp @@ -102,6 +102,12 @@ bool AnalyzerOptions::shouldPruneNullReturnPaths() { /* Default = */ true); } +bool AnalyzerOptions::shouldAvoidSuppressingNullArgumentPaths() { + return getBooleanOption(AvoidSuppressingNullArgumentPaths, + "avoid-suppressing-null-argument-paths", + /* Default = */ false); +} + int AnalyzerOptions::getOptionAsInteger(StringRef Name, int DefaultVal) { llvm::SmallString<10> StrBuf; llvm::raw_svector_ostream OS(StrBuf); diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index dace7f3c8b6..328e8a650df 100644 --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -135,10 +135,15 @@ namespace { /// interesting value comes from an inlined function call. class ReturnVisitor : public BugReporterVisitorImpl<ReturnVisitor> { const StackFrameContext *StackFrame; - bool Satisfied; + enum { + Initial, + MaybeSuppress, + Satisfied + } Mode; + public: ReturnVisitor(const StackFrameContext *Frame) - : StackFrame(Frame), Satisfied(false) {} + : StackFrame(Frame), Mode(Initial) {} static void *getTag() { static int Tag = 0; @@ -190,13 +195,16 @@ public: } } - PathDiagnosticPiece *VisitNode(const ExplodedNode *N, - const ExplodedNode *PrevN, - BugReporterContext &BRC, - BugReport &BR) { - if (Satisfied) - return 0; + /// Returns true if any counter-suppression heuristics are enabled for + /// ReturnVisitor. + static bool hasCounterSuppression(AnalyzerOptions &Options) { + return Options.shouldAvoidSuppressingNullArgumentPaths(); + } + PathDiagnosticPiece *visitNodeInitial(const ExplodedNode *N, + const ExplodedNode *PrevN, + BugReporterContext &BRC, + BugReport &BR) { // Only print a message at the interesting return statement. if (N->getLocationContext() != StackFrame) return 0; @@ -217,7 +225,7 @@ public: return 0; // Don't print any more notes after this one. - Satisfied = true; + Mode = Satisfied; const Expr *RetE = Ret->getRetValue(); assert(RetE && "Tracking a return value for a void function"); @@ -243,8 +251,13 @@ public: // report invalid. We still want to emit a path note, however, in case // the report is resurrected as valid later on. ExprEngine &Eng = BRC.getBugReporter().getEngine(); - if (Eng.getAnalysisManager().options.shouldPruneNullReturnPaths()) - BR.markInvalid(ReturnVisitor::getTag(), StackFrame); + AnalyzerOptions &Options = Eng.getAnalysisManager().options; + if (Options.shouldPruneNullReturnPaths()) { + if (hasCounterSuppression(Options)) + Mode = MaybeSuppress; + else + BR.markInvalid(ReturnVisitor::getTag(), StackFrame); + } if (RetE->getType()->isObjCObjectPointerType()) Out << "Returning nil"; @@ -262,6 +275,74 @@ public: PathDiagnosticLocation L(Ret, BRC.getSourceManager(), StackFrame); return new PathDiagnosticEventPiece(L, Out.str()); } + + PathDiagnosticPiece *visitNodeMaybeSuppress(const ExplodedNode *N, + const ExplodedNode *PrevN, + BugReporterContext &BRC, + BugReport &BR) { + // Are we at the entry node for this call? + const CallEnter *CE = N->getLocationAs<CallEnter>(); + if (!CE) + return 0; + + if (CE->getCalleeContext() != StackFrame) + return 0; + + Mode = Satisfied; + + ExprEngine &Eng = BRC.getBugReporter().getEngine(); + AnalyzerOptions &Options = Eng.getAnalysisManager().options; + if (Options.shouldAvoidSuppressingNullArgumentPaths()) { + // Don't automatically suppress a report if one of the arguments is + // known to be a null pointer. Instead, start tracking /that/ null + // value back to its origin. + ProgramStateManager &StateMgr = BRC.getStateManager(); + CallEventManager &CallMgr = StateMgr.getCallEventManager(); + + ProgramStateRef State = N->getState(); + CallEventRef<> Call = CallMgr.getCaller(StackFrame, State); + for (unsigned I = 0, E = Call->getNumArgs(); I != E; ++I) { + SVal ArgV = Call->getArgSVal(I); + if (!isa<Loc>(ArgV)) + continue; + + const Expr *ArgE = Call->getArgExpr(I); + if (!ArgE) + continue; + + // Is it possible for this argument to be non-null? + if (State->assume(cast<Loc>(ArgV), true)) + continue; + + if (bugreporter::trackNullOrUndefValue(N, ArgE, BR, /*IsArg=*/true)) + return 0; + + // If we /can't/ track the null pointer, we should err on the side of + // false negatives, and continue towards marking this report invalid. + // (We will still look at the other arguments, though.) + } + } + + // There is no reason not to suppress this report; go ahead and do it. + BR.markInvalid(ReturnVisitor::getTag(), StackFrame); + return 0; + } + + PathDiagnosticPiece *VisitNode(const ExplodedNode *N, + const ExplodedNode *PrevN, + BugReporterContext &BRC, + BugReport &BR) { + switch (Mode) { + case Initial: + return visitNodeInitial(N, PrevN, BRC, BR); + case MaybeSuppress: + return visitNodeMaybeSuppress(N, PrevN, BRC, BR); + case Satisfied: + return 0; + } + + llvm_unreachable("Invalid visit mode!"); + } }; } // end anonymous namespace @@ -525,10 +606,10 @@ TrackConstraintBRVisitor::VisitNode(const ExplodedNode *N, return NULL; } -void bugreporter::trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S, +bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S, BugReport &report, bool IsArg) { if (!S || !N) - return; + return false; if (const OpaqueValueExpr *OVE = dyn_cast<OpaqueValueExpr>(S)) S = OVE->getSourceExpr(); @@ -550,7 +631,7 @@ void bugreporter::trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S, } while (N); if (!N) - return; + return false; } ProgramStateRef state = N->getState(); @@ -600,7 +681,7 @@ void bugreporter::trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S, } report.addVisitor(new FindLastStoreBRVisitor(V, R)); - return; + return true; } } } @@ -634,6 +715,8 @@ void bugreporter::trackNullOrUndefValue(const ExplodedNode *N, const Stmt *S, S = E->IgnoreParenCasts(); ReturnVisitor::addVisitorIfNecessary(N, S, report); } + + return true; } BugReporterVisitor * diff --git a/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp b/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp index 258ad253bdc..0f48d1e1c79 100644 --- a/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp +++ b/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp @@ -586,8 +586,8 @@ PathDiagnosticLocation const CFGBlock *BSrc = BE->getSrc(); S = BSrc->getTerminatorCondition(); } - else if (const PostStmt *PS = dyn_cast<PostStmt>(&P)) { - S = PS->getStmt(); + else if (const StmtPoint *SP = dyn_cast<StmtPoint>(&P)) { + S = SP->getStmt(); } else if (const PostImplicitCall *PIE = dyn_cast<PostImplicitCall>(&P)) { return PathDiagnosticLocation(PIE->getLocation(), SMng); @@ -602,6 +602,9 @@ PathDiagnosticLocation CEE->getLocationContext(), SMng); } + else { + llvm_unreachable("Unexpected ProgramPoint"); + } return PathDiagnosticLocation(S, SMng, P.getLocationContext()); } diff --git a/clang/test/Analysis/inlining/false-positive-suppression.c b/clang/test/Analysis/inlining/false-positive-suppression.c index be485e0c1c6..20cc3114875 100644 --- a/clang/test/Analysis/inlining/false-positive-suppression.c +++ b/clang/test/Analysis/inlining/false-positive-suppression.c @@ -1,9 +1,14 @@ // RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-config suppress-null-return-paths=false -verify %s -// RUN: %clang_cc1 -analyze -analyzer-checker=core -verify -DSUPPRESSED %s +// RUN: %clang_cc1 -analyze -analyzer-checker=core -verify -DSUPPRESSED=1 %s +// RUN: %clang_cc1 -analyze -analyzer-checker=core -analyzer-config avoid-suppressing-null-argument-paths=true -DSUPPRESSED=1 -DNULL_ARGS=1 -verify %s int opaquePropertyCheck(void *object); int coin(); +int *getNull() { + return 0; +} + int *dynCastToInt(void *ptr) { if (opaquePropertyCheck(ptr)) return (int *)ptr; @@ -69,24 +74,108 @@ void testBranchReversed(void *p) { } +// -------------------------- +// "Suppression suppression" +// -------------------------- + +void testDynCastOrNullOfNull() { + // Don't suppress when one of the arguments is NULL. + int *casted = dynCastOrNull(0); + *casted = 1; +#if !SUPPRESSED || NULL_ARGS + // expected-warning@-2 {{Dereference of null pointer}} +#endif +} + +void testDynCastOfNull() { + // Don't suppress when one of the arguments is NULL. + int *casted = dynCastToInt(0); + *casted = 1; +#if !SUPPRESSED || NULL_ARGS + // expected-warning@-2 {{Dereference of null pointer}} +#endif +} + +int *lookUpInt(int unused) { + if (coin()) + return 0; + static int x; + return &x; +} + +void testZeroIsNotNull() { + // /Do/ suppress when the argument is 0 (an integer). + int *casted = lookUpInt(0); + *casted = 1; +#ifndef SUPPRESSED + // expected-warning@-2 {{Dereference of null pointer}} +#endif +} + +void testTrackNull() { + // /Do/ suppress if the null argument came from another call returning null. + int *casted = dynCastOrNull(getNull()); + *casted = 1; +#ifndef SUPPRESSED + // expected-warning@-2 {{Dereference of null pointer}} +#endif +} + +void testTrackNullVariable() { + // /Do/ suppress if the null argument came from another call returning null. + int *ptr; + ptr = getNull(); + int *casted = dynCastOrNull(ptr); + *casted = 1; +#ifndef SUPPRESSED + // expected-warning@-2 {{Dereference of null pointer}} +#endif +} + + // --------------------------------------- // FALSE NEGATIVES (over-suppression) // --------------------------------------- -void testDynCastOrNullOfNull() { +void testNoArguments() { + // In this case the function has no branches, and MUST return null. + int *casted = getNull(); + *casted = 1; +#ifndef SUPPRESSED + // expected-warning@-2 {{Dereference of null pointer}} +#endif +} + +int *getNullIfNonNull(void *input) { + if (input) + return 0; + static int x; + return &x; +} + +void testKnownPath(void *input) { + if (!input) + return; + // In this case we have a known value for the argument, and thus the path // through the function doesn't ever split. - int *casted = dynCastOrNull(0); + int *casted = getNullIfNonNull(input); *casted = 1; #ifndef SUPPRESSED // expected-warning@-2 {{Dereference of null pointer}} #endif } -void testDynCastOfNull() { +int *alwaysReturnNull(void *input) { + if (opaquePropertyCheck(input)) + return 0; + return 0; +} + +void testAlwaysReturnNull(void *input) { // In this case all paths out of the function return 0, but they are all // dominated by a branch whose condition we don't know! - int *casted = dynCastToInt(0); + int *casted = alwaysReturnNull(input); *casted = 1; #ifndef SUPPRESSED // expected-warning@-2 {{Dereference of null pointer}} |
