[PATCH] D28023: [analyzer] Fix leak false positives before no-return functions caused by incomplete analyses.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: zaks.anna, dcoughlin, xazax.hun, a.sidorin.
NoQ added a subscriber: cfe-commits.

Consider an example:

  void foo(int y) {
    void *x = malloc(1);
    assert(y); // macro that expands to "if (!y) exit(1);"
    free(x);
  }

In CFG block corresponding to `exit(1)`, variable `x` is //dead//, because both CFG and `LivenessAnalysis` are aware of no-return functions and realize that we don't ever reach the reference to `x` in `free(x)` from this block.

Because `x` is dead, its binding - symbol returned by malloc - is also dead, and `MallocChecker` reports a memory leak warning.

However, because such warning would not be particularly useful (nobody ever frees memory before assertion failures), `MallocChecker`'s `BugType` has `setSuppressOnSink(true)`, and this warning would be discarded later by `BugReporter` and never presented to the user.

Warnings with suppress-on-sink are discarded during `FlushReports` when `BugReporter` notices that all paths in `ExplodedGraph` that pass through the warning eventually run into a sink node.

Apart from suppressing false positives similar to the example above, this mechanism has a second purpose - to filter out non-fatal bugs when the path runs into a fatal bug. For that second purpose, the mechanism works perfectly.

However, suppress-on-sink fails to filter out false positives when the analysis terminates too early - by running into analyzer limits, such as block count limits or graph size limits - and //the interruption hits the narrow window between throwing the leak report and reaching the no-return function call//. In such case the report is there, however suppression-on-sink doesn't work, because the sink node was never constructed in the incomplete `ExplodedGraph`.

In a particular report i've been investigating, the false positive disappeared when i set `-analyzer-config max-nodes` to less than 149995 or more than 150105 (+/- 0.08% of the default value 150000).

Note that suppress-on-sink is an "all paths" problem - we're trying to detect an event that occurs on all execution paths after a certain point. Such problems should not be solved by exploring `ExplodedGraph` for this very reason - the graph is not guaranteed to even contain all paths. In some cases it is acceptable to behave conservatively when the graph is known to be incomplete, however in this case it would result in disproportional amounts of leak false-negatives.

This patch implements a very partial solution: also suppress reports thrown against a statement-node that corresponds to a statement that belongs to a no-return block of the CFG.

This solution is partial because no-return functions that we failed to reach may also be found in subsequent blocks or in a different function. However, for the simple implementation of the `assert()` macro (that expands to an `if` followed by a no-return function), this patch fixes the problem.


https://reviews.llvm.org/D28023

Files:
  lib/StaticAnalyzer/Core/BugReporter.cpp
  test/Analysis/max-nodes-suppress-on-sink.c


Index: test/Analysis/max-nodes-suppress-on-sink.c
===================================================================
--- /dev/null
+++ test/Analysis/max-nodes-suppress-on-sink.c
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-config max-nodes=12 -verify %s
+
+// Here we test how "suppress on sink" feature of certain bugtypes interacts
+// with reaching analysis limits.
+
+// If we throw a warning of a bug-type with "suppress on sink" attribute set
+// (such as MallocChecker's memory leak warning), then failing to reach the
+// reason for the sink (eg. no-return function such as "exit()") due to analysis
+// limits (eg. max-nodes option), we may produce a false positive.
+
+#include "Inputs/system-header-simulator.h"
+
+typedef __typeof(sizeof(int)) size_t;
+void *malloc(size_t);
+
+extern void exit(int) __attribute__ ((__noreturn__));
+
+void clang_analyzer_warnIfReached(void);
+
+void test_single_cfg_block_sink() {
+  void *p = malloc(1); // no-warning (wherever the leak warning may occur here)
+
+  // Due to max-nodes option in the run line, we should reach the first call
+  // but bail out before the second call.
+  // If the test on these two lines starts failing, see if modifying
+  // the max-nodes run-line helps.
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  clang_analyzer_warnIfReached(); // no-warning
+
+  // Even though we do not reach this line, we should still suppress
+  // the leak report.
+  exit(0);
+}
Index: lib/StaticAnalyzer/Core/BugReporter.cpp
===================================================================
--- lib/StaticAnalyzer/Core/BugReporter.cpp
+++ lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -3285,6 +3285,29 @@
 };
 }
 
+static const CFGBlock *findBlockForNode(const ExplodedNode *N) {
+  ProgramPoint P = N->getLocation();
+  if (auto BEP = P.getAs<BlockEntrance>())
+    return BEP->getBlock();
+
+  // Find the node's current statement in the CFG.
+  // FIXME: CFG lookup should be made easier.
+  const CFG &Cfg = N->getCFG();
+  if (const Stmt *S = PathDiagnosticLocation::getStmt(N)) {
+    for (auto BI = Cfg.begin(), BE = Cfg.end(); BI != BE; ++BI) {
+      const CFGBlock *B = *BI;
+      for (auto EI = B->begin(), EE = B->end(); EI != EE; ++EI) {
+        const CFGElement &E = *EI;
+        if (auto SE = E.getAs<CFGStmt>())
+          if (SE->getStmt() == S)
+            return B;
+      }
+    }
+  }
+
+  return nullptr;
+}
+
 static BugReport *
 FindReportInEquivalenceClass(BugReportEquivClass& EQ,
                              SmallVectorImpl<BugReport*> &bugReports) {
@@ -3333,6 +3356,17 @@
       continue;
     }
 
+    // See if we are in a no-return CFG block. If so, treat this similarly
+    // to being post-dominated by a sink. This works better when the analysis
+    // is incomplete and we have never reached a no-return function
+    // we're post-dominated by.
+    // FIXME: This is far from enough to handle the incomplete analysis case.
+    // We may be post-dominated in subsequent blocks, or even
+    // inter-procedurally.
+    if (const CFGBlock *B = findBlockForNode(errorNode))
+      if (B->hasNoReturnElement())
+        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.
     typedef FRIEC_WLItem WLItem;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D28023.82229.patch
Type: text/x-patch
Size: 3415 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161221/617c9a6f/attachment.bin>