From cb82dacd54c356aba347e1a6168046b5e94f0055 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Fri, 2 Dec 2016 08:35:30 -0800 Subject: [PATCH] [traces] moving logic for reporting flow-sensitive traces from quandary to trace domain Summary: Trying to stop other users of the trace domain from making the mistake that Quandary made before D4234766. This should also improve the performance of Quandary, since the filtering of FP's is now done before building up the full interprocedural trace (which requires disk reads). Reviewed By: jeremydubreil Differential Revision: D4234770 fbshipit-source-id: e7e9291 --- infer/src/checkers/Trace.ml | 29 +++++++++++++++++++++-------- infer/src/checkers/Trace.mli | 13 ++++++++----- infer/src/quandary/TaintAnalysis.ml | 23 ++++------------------- 3 files changed, 33 insertions(+), 32 deletions(-) diff --git a/infer/src/checkers/Trace.ml b/infer/src/checkers/Trace.ml index c6dbb068b..9f18e73a6 100644 --- a/infer/src/checkers/Trace.ml +++ b/infer/src/checkers/Trace.ml @@ -44,11 +44,14 @@ module type S = sig (** get the passthroughs of the trace *) val passthroughs : t -> Passthroughs.t - (** get the reportable source-sink flows in this trace *) - val get_reports : t -> (Source.t * Sink.t * Passthroughs.t) list + (** get the reportable source-sink flows in this trace. specifying [cur_site] restricts the + reported paths to ones introduced by the call at [cur_site] *) + val get_reports : ?cur_site:CallSite.t -> t -> (Source.t * Sink.t * Passthroughs.t) list - (** get a path for each of the reportable source -> sink flows in this trace *) - val get_reportable_paths : t -> trace_of_pname:(Procname.t -> t) -> path list + (** get a path for each of the reportable source -> sink flows in this trace. specifying + [cur_site] restricts the reported paths to ones introduced by the call at [cur_site] *) + val get_reportable_paths : + ?cur_site:CallSite.t -> t -> trace_of_pname:(Procname.t -> t) -> path list (** create a loc_trace from a path; [source_should_nest s] should be true when we are going one deeper into a call-chain, ie when lt_level should be bumper in the next loc_trace_elem, and @@ -156,15 +159,25 @@ module Make (Spec : Spec) = struct (* sources empty => sinks empty and passthroughs empty *) Sources.is_empty t.sources - let get_reports t = + let get_reports ?cur_site t = if Sinks.is_empty t.sinks || Sources.is_empty t.sources then [] else + let should_report_at_site source sink = match cur_site with + | None -> + true + | Some call_site -> + (* report when: (1) [cur_site] introduces the sink, and (2) [cur_site] does not also + introduce the source. otherwise, we'll report paths that don't respect control + flow. *) + CallSite.equal call_site (Sink.call_site sink) && + not (CallSite.equal call_site (Source.call_site source)) in + (* written to avoid closure allocations in hot code. change with caution. *) let report_source source sinks acc0 = let report_one sink acc = - if Spec.should_report source sink + if Spec.should_report source sink && should_report_at_site source sink then (source, sink, t.passthroughs) :: acc else acc in Sinks.fold report_one sinks acc0 in @@ -202,7 +215,7 @@ module Make (Spec : Spec) = struct pp_passthroughs cur_passthroughs pp_sinks (IList.rev sinks_passthroughs) - let get_reportable_paths t ~trace_of_pname = + let get_reportable_paths ?cur_site t ~trace_of_pname = let expand_path source sink = let sources_of_pname pname = let trace = trace_of_pname pname in @@ -220,7 +233,7 @@ module Make (Spec : Spec) = struct (fun (source, sink, passthroughs) -> let sources_passthroughs, sinks_passthroughs = expand_path source sink in passthroughs, sources_passthroughs, sinks_passthroughs) - (get_reports t) + (get_reports ?cur_site t) let to_loc_trace ?(desc_of_source=fun source -> diff --git a/infer/src/checkers/Trace.mli b/infer/src/checkers/Trace.mli index e55977652..849b8e33c 100644 --- a/infer/src/checkers/Trace.mli +++ b/infer/src/checkers/Trace.mli @@ -44,11 +44,14 @@ module type S = sig (** get the passthroughs of the trace *) val passthroughs : t -> Passthroughs.t - (** get the reportable source-sink flows in this trace *) - val get_reports : t -> (Source.t * Sink.t * Passthroughs.t) list - - (** get a path for each of the reportable source -> sink flows in this trace *) - val get_reportable_paths : t -> trace_of_pname:(Procname.t -> t) -> path list + (** get the reportable source-sink flows in this trace. specifying [cur_site] restricts the + reported paths to ones introduced by the call at [cur_site] *) + val get_reports : ?cur_site:CallSite.t -> t -> (Source.t * Sink.t * Passthroughs.t) list + + (** get a path for each of the reportable source -> sink flows in this trace. specifying + [cur_site] restricts the reported paths to ones introduced by the call at [cur_site] *) + val get_reportable_paths : + ?cur_site:CallSite.t -> t -> trace_of_pname:(Procname.t -> t) -> path list (** create a loc_trace from a path; [source_should_nest s] should be true when we are going one deeper into a call-chain, ie when lt_level should be bumper in the next loc_trace_elem, and diff --git a/infer/src/quandary/TaintAnalysis.ml b/infer/src/quandary/TaintAnalysis.ml index 59d92f886..fd4efff91 100644 --- a/infer/src/quandary/TaintAnalysis.ml +++ b/infer/src/quandary/TaintAnalysis.ml @@ -154,7 +154,7 @@ module Make (TaintSpecification : TaintSpec.S) = struct TaintDomain.add_trace id_ap trace access_tree (** log any new reportable source-sink flows in [trace] *) - let report_trace trace callee_site (proc_data : formal_map ProcData.t) = + let report_trace trace cur_site (proc_data : formal_map ProcData.t) = let trace_of_pname pname = match Summary.read_summary proc_data.pdesc pname with | Some summary -> @@ -179,24 +179,9 @@ module Make (TaintSpecification : TaintSpec.S) = struct let trace_str = F.asprintf "%a" pp_path_short path in let ltr = TraceDomain.to_loc_trace path in let exn = Exceptions.Checkers (msg, Localise.verbatim_desc trace_str) in - Reporting.log_error caller_pname ~loc:(CallSite.loc callee_site) ~ltr exn in - - let reportable_paths = TraceDomain.get_reportable_paths trace ~trace_of_pname in - IList.iter - (fun ((_, sources, sinks) as path) -> - (* this is the first callee in a chain that transitively calls a sink *) - let indirect_sink = fst (IList.hd (IList.rev sinks)) in - let indirect_sink_site = TraceDomain.Sink.call_site indirect_sink in - (* report when: (1) this call site introduces the sink, and (2) this call site does not - also introduce the source. otherwise, we'll report paths that don't respect control - flow. *) - if CallSite.equal indirect_sink_site callee_site && - (* the is the first callee in a chain that transitively returns a source *) - let indirect_source = fst (IList.hd (IList.rev sources)) in - not (CallSite.equal indirect_sink_site (TraceDomain.Source.call_site indirect_source)) - then - report_error path) - reportable_paths + Reporting.log_error caller_pname ~loc:(CallSite.loc cur_site) ~ltr exn in + + IList.iter report_error (TraceDomain.get_reportable_paths ~cur_site trace ~trace_of_pname) 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