[annot] Do not iterate through every pair of sinks between caller and…

… callee

Summary:
Previously in annotation reachability, when we iterated on the summary for a given sink annotation for a source, we ignored the sink procedure name when iterating on callee's summary, causing us to call the reporting function multiple times for the same path. This did not always cause duplicates, because I assume reporting has some mechanism (e.g. we got the same bug hash). But this diff fixes it.

Example:
```
1 Sink void sink1() { }
2 Sink void sink2() { }
3 void intermediate() {
4   sink1();
5   sink2();
6 }
7 Source void source() {
8   intermediate();
9 }
```

The summary of `intermediate`:
```
{Sink -> {
  sink1 -> {line 4},
  sink2 -> {line 5},
}}
```

The summary of `source`:
```
{Sink -> {
  sink1 -> {line 8 via intermediate},
  sink2 -> {line 8 via intermediate},
}}
```

Previously, when we were checking `sink1` for `source`, we've seen that it can be reached via `intermediate`. But when applying the summary of `intermediate` we iterated over both `sink1` and `sink2`. And the same goes for checking `sink2` for `source`.

Reviewed By: rgrig

Differential Revision: D62756696

fbshipit-source-id: 357e80b5cfaa3cc029b0bca3adc54de26ad75460

infer/src/checkers/annotationReachability.ml

@@ -260,40 +260,42 @@ let add_to_trace (call_site_info : Domain.call_site_info) end_of_stack snk_annot
 
 let find_paths_to_snk ({InterproceduralAnalysis.proc_desc; tenv} as analysis_data) src
     (spec : AnnotationSpec.t) sink_map =
-  let snk = spec.sink_annotation in
-  let rec loop fst_call_loc visited_pnames trace (call_site_info : Domain.call_site_info) =
+  let snk_annot = spec.sink_annotation in
+  let rec loop fst_call_loc visited_pnames trace snk_pname (call_site_info : Domain.call_site_info)
+      =
     let callee_pname = CallSite.pname call_site_info.call_site in
-    let end_of_stack = method_overrides_annot snk spec.models tenv callee_pname in
-    let new_trace = add_to_trace call_site_info end_of_stack snk trace in
+    let end_of_stack = method_overrides_annot snk_annot spec.models tenv callee_pname in
+    let new_trace = add_to_trace call_site_info end_of_stack snk_annot trace in
     if end_of_stack then
+      (* Reached sink, report *)
       report_src_to_snk_path analysis_data src spec fst_call_loc (List.rev new_trace) callee_pname
     else if
       Config.annotation_reachability_minimize_sources
       && method_overrides_annot src spec.models tenv callee_pname
     then (* Found a source in the middle, this path is not minimal *)
       ()
     else
-      let next_calls = lookup_annotation_calls analysis_data snk callee_pname in
-      let unseen_callees, updated_callees =
-        Domain.SinkMap.fold
-          (fun _ call_sites ((unseen, visited) as accu) ->
-            try
-              let call_site_info = Domain.CallSites.min_elt call_sites in
-              let p = CallSite.pname call_site_info.call_site in
-              if Procname.Set.mem p visited then accu
-              else (call_site_info :: unseen, Procname.Set.add p visited)
-            with Caml.Not_found -> accu )
-          next_calls ([], visited_pnames)
+      (* Sink not yet reached, thus we have an intermediate step: let's get its summary and recurse *)
+      let callee_sink_map = lookup_annotation_calls analysis_data snk_annot callee_pname in
+      let next_call_sites =
+        Domain.SinkMap.find_opt snk_pname callee_sink_map
+        |> Option.value ~default:Domain.CallSites.empty
       in
-      List.iter ~f:(loop fst_call_loc updated_callees new_trace) unseen_callees
+      try
+        let call_site_info = Domain.CallSites.min_elt next_call_sites in
+        let p = CallSite.pname call_site_info.call_site in
+        if Procname.Set.mem p visited_pnames then ()
+        else
+          loop fst_call_loc (Procname.Set.add p visited_pnames) new_trace snk_pname call_site_info
+      with Caml.Not_found -> ()
   in
   let trace = start_trace proc_desc src in
   Domain.SinkMap.iter
-    (fun _ call_sites ->
+    (fun snk_pname call_sites ->
       try
         let fst_call_site = Domain.CallSites.min_elt call_sites in
         let fst_call_loc = CallSite.loc fst_call_site.call_site in
-        loop fst_call_loc Procname.Set.empty trace fst_call_site
+        loop fst_call_loc Procname.Set.empty trace snk_pname fst_call_site
       with Caml.Not_found -> () )
     sink_map
 

infer/tests/codetoanalyze/java/annotreach/CustomAnnotations.java

@@ -54,6 +54,22 @@ void source22Bad() {
     sink2();
   }
 
+  @UserDefinedSource1
+  void sourceCallsTwoSinksBad() {
+    sink1();
+    sink2();
+  }
+
+  void callsTwoSinks() {
+    sink1();
+    sink2();
+  }
+
+  @UserDefinedSource1
+  void sourceCallsTwoSinksIndirectlyBad() {
+    callsTwoSinks();
+  }
+
   // This is reported even though it is a superset of an other trace
   @UserDefinedSource2
   void sourceCallsSourceBad() {

infer/tests/codetoanalyze/java/annotreach/issues.exp

@@ -4,6 +4,10 @@ codetoanalyze/java/annotreach/CustomAnnotations.java, codetoanalyze.java.checker
 codetoanalyze/java/annotreach/CustomAnnotations.java, codetoanalyze.java.checkers.CustomAnnotations.source12Bad():void, 1, CHECKERS_ANNOTATION_REACHABILITY_ERROR, no_bucket, ERROR, [Method source12Bad(), marked as source @UserDefinedSource1,calls sink2(),sink2() defined here, marked as sink @UserDefinedSink2]
 codetoanalyze/java/annotreach/CustomAnnotations.java, codetoanalyze.java.checkers.CustomAnnotations.source21Bad():void, 1, CHECKERS_ANNOTATION_REACHABILITY_ERROR, no_bucket, ERROR, [Method source21Bad(), marked as source @UserDefinedSource2,calls sink1(),sink1() defined here, marked as sink @UserDefinedSink1]
 codetoanalyze/java/annotreach/CustomAnnotations.java, codetoanalyze.java.checkers.CustomAnnotations.source22Bad():void, 1, CHECKERS_ANNOTATION_REACHABILITY_ERROR, no_bucket, ERROR, [Method source22Bad(), marked as source @UserDefinedSource2,calls sink2(),sink2() defined here, marked as sink @UserDefinedSink2]
+codetoanalyze/java/annotreach/CustomAnnotations.java, codetoanalyze.java.checkers.CustomAnnotations.sourceCallsTwoSinksBad():void, 1, CHECKERS_ANNOTATION_REACHABILITY_ERROR, no_bucket, ERROR, [Method sourceCallsTwoSinksBad(), marked as source @UserDefinedSource1,calls sink1(),sink1() defined here, marked as sink @UserDefinedSink1]
+codetoanalyze/java/annotreach/CustomAnnotations.java, codetoanalyze.java.checkers.CustomAnnotations.sourceCallsTwoSinksBad():void, 2, CHECKERS_ANNOTATION_REACHABILITY_ERROR, no_bucket, ERROR, [Method sourceCallsTwoSinksBad(), marked as source @UserDefinedSource1,calls sink2(),sink2() defined here, marked as sink @UserDefinedSink2]
+codetoanalyze/java/annotreach/CustomAnnotations.java, codetoanalyze.java.checkers.CustomAnnotations.sourceCallsTwoSinksIndirectlyBad():void, 1, CHECKERS_ANNOTATION_REACHABILITY_ERROR, no_bucket, ERROR, [Method sourceCallsTwoSinksIndirectlyBad(), marked as source @UserDefinedSource1,calls callsTwoSinks(),callsTwoSinks() defined here,calls sink1(),sink1() defined here, marked as sink @UserDefinedSink1]
+codetoanalyze/java/annotreach/CustomAnnotations.java, codetoanalyze.java.checkers.CustomAnnotations.sourceCallsTwoSinksIndirectlyBad():void, 1, CHECKERS_ANNOTATION_REACHABILITY_ERROR, no_bucket, ERROR, [Method sourceCallsTwoSinksIndirectlyBad(), marked as source @UserDefinedSource1,calls callsTwoSinks(),callsTwoSinks() defined here,calls sink2(),sink2() defined here, marked as sink @UserDefinedSink2]
 codetoanalyze/java/annotreach/CustomAnnotations.java, codetoanalyze.java.checkers.CustomAnnotations.sourceCallsSourceBad():void, 1, CHECKERS_ANNOTATION_REACHABILITY_ERROR, no_bucket, ERROR, [Method sourceCallsSourceBad(), marked as source @UserDefinedSource2,calls source22Bad(),source22Bad() defined here,calls sink2(),sink2() defined here, marked as sink @UserDefinedSink2]
 codetoanalyze/java/annotreach/CustomAnnotations.java, codetoanalyze.java.checkers.CustomAnnotations.sourceCallsSinkThatCallsSinkBad():void, 1, CHECKERS_ANNOTATION_REACHABILITY_ERROR, no_bucket, ERROR, [Method sourceCallsSinkThatCallsSinkBad(), marked as source @UserDefinedSource1,calls sinkCallsSink(),sinkCallsSink() defined here, marked as sink @UserDefinedSink1]
 codetoanalyze/java/annotreach/CustomAnnotations.java, codetoanalyze.java.checkers.CustomAnnotations.sourceTransitiveCallBad():void, 1, CHECKERS_ANNOTATION_REACHABILITY_ERROR, no_bucket, ERROR, [Method sourceTransitiveCallBad(), marked as source @UserDefinedSource1,calls callsSink(),callsSink() defined here,calls sink1(),sink1() defined here, marked as sink @UserDefinedSink1]