[quandary] don't double-report when applying summaries

Reviewed By: cristianoc

Differential Revision: D4103729

fbshipit-source-id: 4d51e41

infer/src/quandary/TaintAnalysis.ml

@@ -146,7 +146,24 @@ module Make (TaintSpec : TaintSpec.S) = struct
       let id_ap = AccessPath.Exact (AccessPath.of_id ret_id ret_typ) in
       TaintDomain.add_trace id_ap trace access_tree
 
-    let add_sinks sinks actuals ({ Domain.access_tree; id_map; } as astate) proc_data loc =
+    (* log any reportable source-sink flows in [trace] and remove logged sinks from the trace *)
+    let report_and_filter_trace trace callee_site (proc_data : formal_map ProcData.t) =
+      match TraceDomain.get_reportable_exns trace with
+      | [] ->
+          trace
+      | reportable_exns ->
+          let caller_pname = Cfg.Procdesc.get_proc_name proc_data.pdesc in
+          let reported_sinks =
+            IList.map
+              (fun (_, sink, exn) ->
+                 Reporting.log_error caller_pname ~loc:(CallSite.loc callee_site) exn;
+                 sink)
+              reportable_exns in
+          (* got new source -> sink flow. report it, but don't add the sink to the trace. if we do,
+             we will double-report later on. *)
+          TraceDomain.filter_sinks trace reported_sinks
+
+    let add_sinks sinks actuals ({ Domain.access_tree; id_map; } as astate) proc_data callee_site =
       let f_resolve_id = resolve_id id_map in
       (* add [sink] to the trace associated with the [formal_num]th actual *)
       let add_sink_to_actual access_tree_acc (sink_param : TraceDomain.Sink.t Sink.parameter) =
@@ -166,21 +183,16 @@ module Make (TaintSpec : TaintSpec.S) = struct
               then AccessPath.Abstracted actual_ap_raw
               else AccessPath.Exact actual_ap_raw in
             begin
-              match access_path_get_node actual_ap access_tree_acc proc_data loc with
+              match access_path_get_node
+                      actual_ap access_tree_acc proc_data (CallSite.loc callee_site) with
               | Some (actual_trace, _) ->
                   (* add callee_pname to actual trace as a sink *)
-                  let actual_trace' = TraceDomain.add_sink sink_param.sink actual_trace in
-                  begin
-                    match TraceDomain.get_reportable_exns actual_trace' with
-                    | [] ->
-                        TaintDomain.add_trace actual_ap actual_trace' access_tree_acc
-                    | reportable_exns ->
-                        (* got new source -> sink flow. report it, but don't update the trace. if we
-                           update the trace, we will double-report later on. *)
-                        let pname = Cfg.Procdesc.get_proc_name proc_data.ProcData.pdesc in
-                        IList.iter (Reporting.log_error pname ~loc) reportable_exns;
-                        access_tree_acc
-                  end
+                  let actual_trace' =
+                    report_and_filter_trace
+                      (TraceDomain.add_sink sink_param.sink actual_trace)
+                      callee_site
+                      proc_data in
+                  TaintDomain.add_trace actual_ap actual_trace' access_tree_acc
               | None ->
                   access_tree_acc
             end
@@ -250,10 +262,8 @@ module Make (TaintSpec : TaintSpec.S) = struct
             let output_trace = TaintSpec.of_summary_trace in_out_summary.output_trace in
             let appended_trace = TraceDomain.append in_trace output_trace callee_site in
             let joined_trace = TraceDomain.join caller_trace appended_trace in
-            IList.iter
-              (Reporting.log_error (CallSite.pname callee_site) ~loc:callee_loc)
-              (TraceDomain.get_reportable_exns joined_trace);
-            TaintDomain.add_trace caller_ap joined_trace access_tree
+            let filtered_trace = report_and_filter_trace joined_trace callee_site proc_data in
+            TaintDomain.add_trace caller_ap filtered_trace access_tree
         | None ->
             access_tree in
 
@@ -325,7 +335,7 @@ module Make (TaintSpec : TaintSpec.S) = struct
             let sinks = TraceDomain.Sink.get call_site actuals in
             let astate_with_sink = match sinks with
               | [] -> astate
-              | sinks -> add_sinks sinks actuals astate proc_data callee_loc in
+              | sinks -> add_sinks sinks actuals astate proc_data call_site in
 
             let source = TraceDomain.Source.get call_site in
             let astate_with_source =

infer/src/quandary/Trace.ml

@@ -46,7 +46,7 @@ module type S = sig
   val get_reports : t -> (Source.t * Sink.t * Passthroughs.t) list
 
   (** get logging-ready exceptions for the reportable source-sink flows in this trace *)
-  val get_reportable_exns : t -> exn list
+  val get_reportable_exns : t -> (Source.t * Sink.t * exn) list
 
   (** create a trace from a source *)
   val of_source : Source.t -> t
@@ -57,6 +57,9 @@ module type S = sig
   (** add a sink to the current trace. *)
   val add_sink : Sink.t -> t -> t
 
+  (** remove the given sinks from the current trace *)
+  val filter_sinks : t -> Sink.t list -> t
+
   (** append the trace for given call site to the current caller trace *)
   val append : t -> t -> CallSite.t -> t
 
@@ -126,7 +129,8 @@ module Make (Spec : Spec) = struct
 
   let get_reportable_exns t =
     IList.map
-      (fun (source, sink, passthroughs) -> Spec.get_reportable_exn source sink passthroughs)
+      (fun (source, sink, passthroughs) ->
+         source, sink, Spec.get_reportable_exn source sink passthroughs)
       (get_reports t)
 
   let of_source source =
@@ -143,6 +147,10 @@ module Make (Spec : Spec) = struct
     let sinks = Sinks.add sink t.sinks in
     { t with sinks; }
 
+  let filter_sinks t sinks_to_filter =
+    let sinks = Sinks.diff t.sinks (Sinks.of_list sinks_to_filter) in
+    { t with sinks; }
+
   (** compute caller_trace + callee_trace *)
   let append caller_trace callee_trace callee_site =
     if is_empty callee_trace

infer/src/quandary/Trace.mli

@@ -46,7 +46,7 @@ module type S = sig
   val get_reports : t -> (Source.t * Sink.t * Passthroughs.t) list
 
   (** get logging-ready exceptions for the reportable source-sink flows in this trace *)
-  val get_reportable_exns : t -> exn list
+  val get_reportable_exns : t -> (Source.t * Sink.t * exn) list
 
   (** create a trace from a source *)
   val of_source : Source.t -> t
@@ -57,6 +57,9 @@ module type S = sig
   (** add a sink to the current trace. *)
   val add_sink : Sink.t -> t -> t
 
+  (** remove the given sinks from the current trace *)
+  val filter_sinks : t -> Sink.t list -> t
+
   (** append the trace for given call site to the current caller trace *)
   val append : t -> t -> CallSite.t -> t
 

infer/tests/codetoanalyze/java/quandary/Interprocedural.java

@@ -195,6 +195,13 @@ class Interprocedural {
     getGlobalThenCallSink();
   }
 
+  // this should report two alarms, not three
+  public void callSinkNoTripleReportBad() {
+    Object source = InferTaint.inferSecretSource();
+    callSinkParam1(source, null);
+    callSinkParam2(null, source);
+  }
+
   /** passthrough tests */
 
   public static void singlePassthroughBad() {

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

@@ -35,8 +35,6 @@ DynamicDispatch.java:140: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferT
 DynamicDispatch.java:146: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 144]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 146]) via { Object DynamicDispatch$BadSubtype.propagate(Object) at [line 145] }
 DynamicDispatch.java:154: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 119]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 154]) via { Object DynamicDispatch$BadSubtype.returnSource() at [line 153] }
 DynamicDispatch.java:157: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 156]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 124]) via { void DynamicDispatch$BadSubtype.callSink(Object) at [line 157] }
-DynamicDispatch.java:159: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 156]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 124]) via { void DynamicDispatch$BadSubtype.callSink(Object) at [line 157], Object DynamicDispatch$BadSubtype.propagate(Object) at [line 159] }
-DynamicDispatch.java:160: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 156]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 124]) via { void DynamicDispatch$BadSubtype.callSink(Object) at [line 157], Object DynamicDispatch$BadSubtype.propagate(Object) at [line 159] }
 DynamicDispatch.java:160: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 156]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 160]) via { void DynamicDispatch$BadSubtype.callSink(Object) at [line 157], Object DynamicDispatch$BadSubtype.propagate(Object) at [line 159] }
 Exceptions.java:23: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 19]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 23]) via {  }
 Exceptions.java:33: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 30]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 33]) via {  }
@@ -97,12 +95,14 @@ Interprocedural.java:157: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferT
 Interprocedural.java:166: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 165]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 161]) via { void Interprocedural.callSinkOnGlobal() at [line 166] }
 Interprocedural.java:181: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 180]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 161]) via { void Interprocedural.callSinkOnGlobal() at [line 181], void Interprocedural.setGlobal(Object) at [line 180] }
 Interprocedural.java:195: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 194]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 190]) via { void Interprocedural.getGlobalThenCallSink() at [line 195] }
-Interprocedural.java:203: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 201]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 203]) via { Object Interprocedural.id(Object) at [line 202] }
-Interprocedural.java:210: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 207]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 210]) via { Object Interprocedural.id(Object) at [line 208], Object Interprocedural.id(Object) at [line 209] }
-Interprocedural.java:221: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 216]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 221]) via { Object Interprocedural.returnSourceConditional(boolean) at [line 221] }
-Interprocedural.java:232: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 230]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 232]) via {  }
-Interprocedural.java:244: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 244]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 240]) via { void Interprocedural.callSinkVariadic(java.lang.Object[]) at [line 244] }
-Interprocedural.java:255: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 253]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 255]) via {  }
+Interprocedural.java:201: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 200]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 104]) via { void Interprocedural.callSinkParam1(Object,Object) at [line 201] }
+Interprocedural.java:202: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 200]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 116]) via { void Interprocedural.callSinkParam1(Object,Object) at [line 201], void Interprocedural.callSinkParam2(Object,Object) at [line 202] }
+Interprocedural.java:210: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 208]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 210]) via { Object Interprocedural.id(Object) at [line 209] }
+Interprocedural.java:217: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 214]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 217]) via { Object Interprocedural.id(Object) at [line 215], Object Interprocedural.id(Object) at [line 216] }
+Interprocedural.java:228: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 223]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 228]) via { Object Interprocedural.returnSourceConditional(boolean) at [line 228] }
+Interprocedural.java:239: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 237]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 239]) via {  }
+Interprocedural.java:251: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 251]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 247]) via { void Interprocedural.callSinkVariadic(java.lang.Object[]) at [line 251] }
+Interprocedural.java:262: ERROR: QUANDARY_TAINT_ERROR Error: Other(Object InferTaint.inferSecretSource() at [line 260]) -> Other(void InferTaint.inferSensitiveSink(Object) at [line 262]) via {  }
 LoggingPrivateData.java:20: ERROR: QUANDARY_TAINT_ERROR Error: PrivateData(String SharedPreferences.getString(String,String) at [line 20]) -> Logging(int Log.e(String,String) at [line 20]) via {  }
 LoggingPrivateData.java:24: ERROR: QUANDARY_TAINT_ERROR Error: PrivateData(String SharedPreferences.getString(String,String) at [line 24]) -> Logging(int Log.e(String,String) at [line 24]) via {  }
 LoggingPrivateData.java:39: ERROR: QUANDARY_TAINT_ERROR Error: PrivateData(String SharedPreferences.getString(String,String) at [line 38]) -> Logging(int Log.w(String,Throwable) at [line 39]) via {  }