[PATCH] D41253: [analyzer] WIP: trackNullOrUndefValue: track last store to symbolic pointers.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs, eraman.

`bugreporter::trackNullOrUndefValue()` checker API function extends a bug report with a recursive family of bug report visitors (`ReturnVisitor`, `FindLastStoreBRVisitor`, etc.) that collectively try to figure out where the given value came from. In particular, a null or undefined value, which is useful for null dereferences, uninitialized value checks, or divisions by zero. This not only improves the bug report with additional helpful diagnostic pieces, but also helps us suppress bugs that come from inlined defensive checks (the more solid potential solution for at least some of these false positives would be to introduce a state merge at call exit, but state merge is a new operation over the program states, so it would require checker side support, which is heavy).

In this patch i attempt to add more logic into the tracking, namely to be able to track a value stored in an arbitrary memory region `R` back to the expression that caused this value to be stored there. Previously it only worked when `R` is coming from a `ExplodedGraph::isInterestingLValueExpression()` (the exact meaning of which is "the respective node would not be reclaimed during garbage collection" (!), of course it would indeed be a shame if the node we're looking for disappears). This leaves with plain variables, member variables, and ObjC instance variables.

`FindLastStoreBRVisitor` is already capable of doing this in the general case of an arbitrary memory region - not only it finds the node where `getSVal(R)` changed, but also it finds `PostStore` nodes to this region even if the newly written value is same as old value.

Note that visitors are deduplicated, so we're not afraid of adding too many identical visitors.

TODO: For now i just stuffed the visitor in there, and it seems to work. But i'd like to see how much the existing code for variables can (or even must) be reused before committing, hence WIP.


Repository:
  rC Clang

https://reviews.llvm.org/D41253

Files:
  lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  test/Analysis/inlining/inline-defensive-checks.c
  test/Analysis/nullptr.cpp


Index: test/Analysis/nullptr.cpp
===================================================================
--- test/Analysis/nullptr.cpp
+++ test/Analysis/nullptr.cpp
@@ -142,8 +142,9 @@
                       // expected-note at -1{{Passing null pointer value via 1st parameter 'x'}}
   if (getSymbol()) {  // expected-note  {{Assuming the condition is true}}
                       // expected-note at -1{{Taking true branch}}
-    X *x = Type().x; // expected-note{{'x' initialized to a null pointer value}}
-    x->f(); // expected-warning{{Called C++ object pointer is null}}
+    X *xx = Type().x; // expected-note   {{Null pointer value stored to field 'x'}}
+                      // expected-note at -1{{'xx' initialized to a null pointer value}}
+    xx->f(); // expected-warning{{Called C++ object pointer is null}}
             // expected-note at -1{{Called C++ object pointer is null}}
   }
 }
Index: test/Analysis/inlining/inline-defensive-checks.c
===================================================================
--- test/Analysis/inlining/inline-defensive-checks.c
+++ test/Analysis/inlining/inline-defensive-checks.c
@@ -190,3 +190,21 @@
   idc(s);
   *(&(s->a[0])) = 7; // no-warning
 }
+
+void idcTrackConstraintThroughSymbolicRegion(int **x) {
+  idc(*x);
+  // FIXME: Should not warn.
+  **x = 7; // expected-warning{{Dereference of null pointer}}
+}
+
+int *idcPlainNull(int coin) {
+  if (coin)
+    return 0;
+  static int X;
+  return &X;
+}
+
+void idcTrackZeroValueThroughSymbolicRegion(int coin, int **x) {
+  *x = idcPlainNull(coin);
+  **x = 7; // no-warning
+}
Index: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1142,9 +1142,12 @@
     else
       RVal = state->getSVal(L->getRegion());
 
-    const MemRegion *RegionRVal = RVal.getAsRegion();
     report.addVisitor(llvm::make_unique<UndefOrNullArgVisitor>(L->getRegion()));
+    if (Optional<KnownSVal> KV = RVal.getAs<KnownSVal>())
+      report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
+          *KV, L->getRegion(), EnableNullFPSuppression));
 
+    const MemRegion *RegionRVal = RVal.getAsRegion();
     if (RegionRVal && isa<SymbolicRegion>(RegionRVal)) {
       report.markInteresting(RegionRVal);
       report.addVisitor(llvm::make_unique<TrackConstraintBRVisitor>(


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D41253.127002.patch
Type: text/x-patch
Size: 2437 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171214/3bbd4099/attachment-0001.bin>