From 4b98543d353c5f25ec4c6e6f5a7b6109dbe4c391 Mon Sep 17 00:00:00 2001
From: Sam Blackshear <shb@fb.com>
Date: Sun, 4 Dec 2016 18:24:01 -0800
Subject: [PATCH] [traces] don't hang when unrolling a mutually recursive trace

Reviewed By: jberdine

Differential Revision: D4272213

fbshipit-source-id: b8a0939
---
 infer/src/checkers/Trace.ml                      | 16 ++++++++--------
 .../java/threadsafety/ThreadSafeExample.java     | 14 ++++++++++++++
 .../codetoanalyze/java/threadsafety/issues.exp   |  3 +++
 3 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/infer/src/checkers/Trace.ml b/infer/src/checkers/Trace.ml
index 9f18e73a6..9fa523740 100644
--- a/infer/src/checkers/Trace.ml
+++ b/infer/src/checkers/Trace.ml
@@ -91,28 +91,28 @@ end
 module Expander (TraceElem : TraceElem.S) = struct
 
   let expand elem0 ~elems_passthroughs_of_pname =
-    let rec expand_ elem elems_passthroughs_acc =
+    let rec expand_ elem (elems_passthroughs_acc, seen_acc) =
       let elem_site = TraceElem.call_site elem in
       let elem_kind = TraceElem.kind elem in
+      let seen_acc' = CallSite.Set.add elem_site seen_acc in
       let elems, passthroughs = elems_passthroughs_of_pname (CallSite.pname elem_site) in
-      let is_recursive elem_site callee_elem =
-        Procname.equal
-          (CallSite.pname elem_site) (CallSite.pname (TraceElem.call_site callee_elem)) in
+      let is_recursive callee_elem seen =
+        CallSite.Set.mem (TraceElem.call_site callee_elem) seen in
       (* find sinks that are the same kind as the caller, but have a different procname *)
       let matching_elems =
         IList.filter
           (fun callee_elem ->
              TraceElem.Kind.compare (TraceElem.kind callee_elem) elem_kind = 0 &&
-             not (is_recursive elem_site callee_elem))
+             not (is_recursive callee_elem seen_acc'))
           elems in
       match matching_elems with
       | callee_elem :: _ ->
           (* TODO: pick the shortest path to a sink here instead (t14242809) *)
           (* arbitrarily pick one elem and explore it further *)
-          expand_ callee_elem ((elem, passthroughs) :: elems_passthroughs_acc)
+          expand_ callee_elem ((elem, passthroughs) :: elems_passthroughs_acc, seen_acc')
       | _ ->
-          (elem, Passthrough.Set.empty) :: elems_passthroughs_acc in
-    expand_ elem0 []
+          (elem, Passthrough.Set.empty) :: elems_passthroughs_acc, seen_acc' in
+    fst (expand_ elem0 ([], CallSite.Set.empty))
 end
 
 module Make (Spec : Spec) = struct
diff --git a/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java b/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java
index 599e4c7af..0ed7a330e 100644
--- a/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java
+++ b/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java
@@ -31,6 +31,20 @@ public class ThreadSafeExample{
     f = 24;
   }
 
+  public void recursiveBad() {
+    f = 44;
+    recursiveBad();
+  }
+
+  private void evenOk() {
+    f = 44;
+    oddBad();
+  }
+
+  public void oddBad() {
+    evenOk(); // should report here
+  }
+
   // shouldn't report here because it's a private method
   private void assignInPrivateMethodOk() {
     f = 24;
diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp
index 2949eaf96..b74e50a74 100644
--- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp
+++ b/infer/tests/codetoanalyze/java/threadsafety/issues.exp
@@ -7,4 +7,7 @@ codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ExtendsThreadSafeEx
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ExtendsThreadSafeExample.tsOK(), 1, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.ExtendsThreadSafeExample.field]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.callPublicMethodBad(), 1, THREAD_SAFETY_ERROR, [call to void ThreadSafeExample.assignInPrivateMethodOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.deeperTraceBad(), 1, THREAD_SAFETY_ERROR, [call to void ThreadSafeExample.callAssignInPrivateMethod(),call to void ThreadSafeExample.assignInPrivateMethodOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f]
+codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.oddBad(), 1, THREAD_SAFETY_ERROR, [call to void ThreadSafeExample.evenOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f]
+codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.recursiveBad(), 1, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.ThreadSafeExample.f]
+codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.recursiveBad(), 2, THREAD_SAFETY_ERROR, [call to void ThreadSafeExample.recursiveBad(),access to codetoanalyze.java.checkers.ThreadSafeExample.f]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.tsBad(), 1, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.ThreadSafeExample.f]