aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--include/clang/StaticAnalyzer/Core/AnalyzerOptions.h14
-rw-r--r--include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h6
-rw-r--r--lib/StaticAnalyzer/Core/AnalyzerOptions.cpp6
-rw-r--r--lib/StaticAnalyzer/Core/BugReporterVisitors.cpp113
-rw-r--r--lib/StaticAnalyzer/Core/PathDiagnostic.cpp7
-rw-r--r--test/Analysis/inlining/false-positive-suppression.c99
6 files changed, 222 insertions, 23 deletions
diff --git a/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h b/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
index ac30836561..fa0754acb1 100644
--- a/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ b/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/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
index a3784739d1..78e35ca82b 100644
--- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitor.h
+++ b/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/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp b/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
index 32073d726d..da88589c86 100644
--- a/lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
+++ b/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/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index dace7f3c8b..328e8a650d 100644
--- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/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/lib/StaticAnalyzer/Core/PathDiagnostic.cpp b/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
index 258ad253bd..0f48d1e1c7 100644
--- a/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ b/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/test/Analysis/inlining/false-positive-suppression.c b/test/Analysis/inlining/false-positive-suppression.c
index be485e0c1c..20cc311487 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 @@ 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}}
generated by cgit v1.2.3-18-g5258 (git 2.32.0) at 2025-06-25 03:49:18 +0000