[analyzer] New option to not suppress null return paths if an argument is null.
Our one basic suppression heuristic is to assume that functions do not
usually return NULL. However, when one of the arguments is NULL it is
suddenly much more likely that NULL is a valid return value. In this case,
we don't suppress the report here, but we do attach /another/ visitor to
go find out if this NULL argument also comes from an inlined function's
error path.
This new behavior, controlled by the 'avoid-suppressing-null-argument-paths'
analyzer-config option, is turned off by default. Turning it on produced
two false positives and no new true positives when running over LLVM/Clang.
This is one of the possible refinements to our suppression heuristics.
<rdar://problem/12350829>
git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@166941 91177308-0d34-0410-b5e6-96231b3b80d8
diff --git a/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h b/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
index ac30836..fa0754a 100644
--- a/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ b/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -187,6 +187,9 @@
/// \sa shouldPruneNullReturnPaths
llvm::Optional<bool> PruneNullReturnPaths;
+ /// \sa shouldAvoidSuppressingNullArgumentPaths
+ llvm::Optional<bool> AvoidSuppressingNullArgumentPaths;
+
/// \sa getGraphTrimInterval
llvm::Optional<unsigned> GraphTrimInterval;
@@ -245,6 +248,17 @@
/// 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/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
index a378473..78e35ca 100644
--- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
+++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
@@ -268,7 +268,11 @@
/// \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/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp b/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
index 32073d7..da88589 100644
--- a/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
+++ b/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
@@ -102,6 +102,12 @@
/* 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/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index dace7f3..328e8a6 100644
--- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -135,10 +135,15 @@
/// 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 @@
}
}
- 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 @@
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 @@
// 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 @@
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 @@
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 @@
} while (N);
if (!N)
- return;
+ return false;
}
ProgramStateRef state = N->getState();
@@ -600,7 +681,7 @@
}
report.addVisitor(new FindLastStoreBRVisitor(V, R));
- return;
+ return true;
}
}
}
@@ -634,6 +715,8 @@
S = E->IgnoreParenCasts();
ReturnVisitor::addVisitorIfNecessary(N, S, report);
}
+
+ return true;
}
BugReporterVisitor *
diff --git a/lib/StaticAnalyzer/Core/PathDiagnostic.cpp b/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
index 258ad25..0f48d1e 100644
--- a/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ b/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -586,8 +586,8 @@
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 @@
CEE->getLocationContext(),
SMng);
}
+ else {
+ llvm_unreachable("Unexpected ProgramPoint");
+ }
return PathDiagnosticLocation(S, SMng, P.getLocationContext());
}
diff --git a/test/Analysis/inlining/false-positive-suppression.c b/test/Analysis/inlining/false-positive-suppression.c
index be485e0..20cc311 100644
--- a/test/Analysis/inlining/false-positive-suppression.c
+++ b/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 @@
}
-// ---------------------------------------
-// FALSE NEGATIVES (over-suppression)
-// ---------------------------------------
+// --------------------------
+// "Suppression suppression"
+// --------------------------
void testDynCastOrNullOfNull() {
- // In this case we have a known value for the argument, and thus the path
- // through the function doesn't ever split.
+ // 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 testDynCastOfNull() {
+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 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 = getNullIfNonNull(input);
+ *casted = 1;
+#ifndef SUPPRESSED
+ // expected-warning@-2 {{Dereference of null pointer}}
+#endif
+}
+
+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}}