From: Artem Dergachev <artem.dergachev@gmail.com>
Date: Tue, 25 Jul 2017 09:25:10 +0000 (+0000)
Subject: [analyzer] Further improve suppress-on-sink behavior in incomplete analyses.
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=6705082a00ade5348271bdd1c7ba178befeb9e24;p=clang

[analyzer] Further improve suppress-on-sink behavior in incomplete analyses.

If a certain memory leak (or other similar bug) found by the analyzer is known
to be happening only before abnormal termination of the program ("sink", eg.
assertion failure in the code under analysis, or another bug that introduces
undefined behavior), such leak warning is discarded. However, if the analysis
has never reaches completion (due to complexity of the code), it may be
failing to notice the sink.

This commit further extends the partial solution introduced in r290341 to cover
cases when a complicated control flow occurs before encountering a no-return
statement (which anyway inevitably leads to such statement(s)) by traversing
the respective section of the CFG in a depth-first manner. A complete solution
still seems elusive.

rdar://problem/28157554

Differential Revision: https://reviews.llvm.org/D35673


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@308957 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/lib/StaticAnalyzer/Core/BugReporter.cpp b/lib/StaticAnalyzer/Core/BugReporter.cpp
index d8fca00681..a306462605 100644
--- a/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ b/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -3310,6 +3310,45 @@ static const CFGBlock *findBlockForNode(const ExplodedNode *N) {
   return nullptr;
 }
 
+static bool isDominatedByNoReturnBlocks(const ExplodedNode *N) {
+  const CFG &Cfg = N->getCFG();
+
+  const CFGBlock *StartBlk = findBlockForNode(N);
+  if (!StartBlk)
+    return false;
+  if (StartBlk->hasNoReturnElement())
+    return true;
+
+  llvm::SmallVector<const CFGBlock *, 32> DFSWorkList;
+  llvm::SmallPtrSet<const CFGBlock *, 32> Visited;
+
+  DFSWorkList.push_back(StartBlk);
+  while (!DFSWorkList.empty()) {
+    const CFGBlock *Blk = DFSWorkList.back();
+    DFSWorkList.pop_back();
+    Visited.insert(Blk);
+
+    for (const auto &Succ : Blk->succs()) {
+      if (const CFGBlock *SuccBlk = Succ.getReachableBlock()) {
+        if (SuccBlk == &Cfg.getExit()) {
+          // We seem to be leaving the current CFG.
+          // We're no longer sure what happens next.
+          return false;
+        }
+
+        if (!SuccBlk->hasNoReturnElement() && !Visited.count(SuccBlk)) {
+          // If the block has reachable child blocks that aren't no-return,
+          // add them to the worklist.
+          DFSWorkList.push_back(SuccBlk);
+        }
+      }
+    }
+  }
+
+  // Nothing reached the exit. It can only mean one thing: there's no return.
+  return true;
+}
+
 static BugReport *
 FindReportInEquivalenceClass(BugReportEquivClass& EQ,
                              SmallVectorImpl<BugReport*> &bugReports) {
@@ -3366,9 +3405,8 @@ FindReportInEquivalenceClass(BugReportEquivClass& EQ,
     // We may be post-dominated in subsequent blocks, or even
     // inter-procedurally. However, it is not clear if more complicated
     // cases are generally worth suppressing.
-    if (const CFGBlock *B = findBlockForNode(errorNode))
-      if (B->hasNoReturnElement())
-        continue;
+    if (isDominatedByNoReturnBlocks(errorNode))
+      continue;
 
     // At this point we know that 'N' is not a sink and it has at least one
     // successor.  Use a DFS worklist to find a non-sink end-of-path node.
diff --git a/test/Analysis/max-nodes-suppress-on-sink.c b/test/Analysis/max-nodes-suppress-on-sink.c
index 8d955b91c1..da1e7bf022 100644
--- a/test/Analysis/max-nodes-suppress-on-sink.c
+++ b/test/Analysis/max-nodes-suppress-on-sink.c
@@ -15,6 +15,8 @@ extern void exit(int) __attribute__ ((__noreturn__));
 
 void clang_analyzer_warnIfReached(void);
 
+int coin();
+
 void test_single_cfg_block_sink() {
   void *p = malloc(1); // no-warning (wherever the leak warning may occur here)
 
@@ -29,3 +31,53 @@ void test_single_cfg_block_sink() {
   // the leak report.
   exit(0);
 }
+
+// A similar test with more complicated control flow before the no-return thing,
+// so that the no-return thing wasn't in the same CFG block.
+void test_more_complex_control_flow_before_sink() {
+  void *p = malloc(1); // no-warning
+
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  clang_analyzer_warnIfReached(); // no-warning
+
+  if (coin())
+    exit(0);
+  else
+    exit(1);
+}
+
+// A loop before the no-return function, to make sure that
+// the dominated-by-sink analysis doesn't hang.
+void test_loop_before_sink(int n) {
+  void *p = malloc(1); // no-warning
+
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  clang_analyzer_warnIfReached(); // no-warning
+
+  for (int i = 0; i < n; ++i) {
+  }
+  exit(1);
+}
+
+// We're not sure if this is no-return.
+void test_loop_with_sink(int n) {
+  void *p = malloc(1); // expected-warning@+2{{Potential leak of memory}}
+
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  clang_analyzer_warnIfReached(); // no-warning
+
+  for (int i = 0; i < n; ++i)
+    if (i == 0)
+      exit(1);
+}
+
+// Handle unreachable blocks correctly.
+void test_unreachable_successor_blocks() {
+  void *p = malloc(1); // no-warning
+
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  clang_analyzer_warnIfReached(); // no-warning
+
+  if (1) // no-crash
+    exit(1);
+}