From c0c813c65751977145c4510803320b1842b87435 Mon Sep 17 00:00:00 2001 From: Jeremy Dubreil Date: Wed, 26 Apr 2017 20:23:49 -0700 Subject: [PATCH] [infer][java] only report one annotation reachability issue per end of call stack Summary: The purpose of the annotation reachability analysis is to report when a method annotated with `X` never calls, directly or indirectly, another method annotated with `Y`. However, there can be different call stacks following different execution paths from `X` to `Y`. Reporting more than one call stack ending with the same annotated procedure does not bring more signal to the end user. So the purpose of this diff is to avoid those duplicated reports and report at most one annotation reachability issue per end of call stack. Reviewed By: sblackshear Differential Revision: D4942765 fbshipit-source-id: 46325a7 --- infer/src/IR/CallSite.ml | 2 - infer/src/IR/CallSite.mli | 2 - infer/src/checkers/AnnotReachabilityDomain.ml | 6 +- infer/src/checkers/annotationReachability.ml | 81 +++++++++++-------- ...notationReachabilityDuplicatesExample.java | 54 +++++++++++++ .../codetoanalyze/java/checkers/issues.exp | 2 + 6 files changed, 110 insertions(+), 37 deletions(-) create mode 100644 infer/tests/codetoanalyze/java/checkers/AnnotationReachabilityDuplicatesExample.java diff --git a/infer/src/IR/CallSite.ml b/infer/src/IR/CallSite.ml index 56f17b15c..9598b85a4 100644 --- a/infer/src/IR/CallSite.ml +++ b/infer/src/IR/CallSite.ml @@ -40,5 +40,3 @@ module Set = PrettyPrintable.MakePPSet(struct let compare = compare let pp = pp end) - -module SetDomain = AbstractDomain.FiniteSet (Set) diff --git a/infer/src/IR/CallSite.mli b/infer/src/IR/CallSite.mli index 2b2513627..0bdb69eab 100644 --- a/infer/src/IR/CallSite.mli +++ b/infer/src/IR/CallSite.mli @@ -26,5 +26,3 @@ val dummy : t val pp : F.formatter -> t -> unit module Set : PrettyPrintable.PPSet with type elt = t - -module SetDomain : module type of AbstractDomain.FiniteSet (Set) diff --git a/infer/src/checkers/AnnotReachabilityDomain.ml b/infer/src/checkers/AnnotReachabilityDomain.ml index 3261a1069..1520ee4f3 100644 --- a/infer/src/checkers/AnnotReachabilityDomain.ml +++ b/infer/src/checkers/AnnotReachabilityDomain.ml @@ -7,4 +7,8 @@ * of patent rights can be found in the PATENTS file in the same directory. *) -include AbstractDomain.Map (Annot.Map) (CallSite.SetDomain) +module CallSites = AbstractDomain.FiniteSet (CallSite.Set) + +module SinkMap = AbstractDomain.Map (Typ.Procname.Map) (CallSites) + +include AbstractDomain.Map (Annot.Map) (SinkMap) diff --git a/infer/src/checkers/annotationReachability.ml b/infer/src/checkers/annotationReachability.ml index 3e8b23f31..0d01a4143 100644 --- a/infer/src/checkers/annotationReachability.ml +++ b/infer/src/checkers/annotationReachability.ml @@ -46,17 +46,22 @@ module Domain = struct module TrackingDomain = AbstractDomain.BottomLifted (TrackingVar) include AbstractDomain.Pair (AnnotReachabilityDomain) (TrackingDomain) - let add_call_site annot call_site ((annot_map, previous_vstate) as astate) = + let add_call_site annot sink call_site ((annot_map, previous_vstate) as astate) = match previous_vstate with | TrackingDomain.Bottom -> astate | TrackingDomain.NonBottom _ -> - let call_set = + let sink_map = try AnnotReachabilityDomain.find annot annot_map - with Not_found -> CallSite.SetDomain.empty in - let call_set' = CallSite.SetDomain.add call_site call_set in - if phys_equal call_set' call_set + with Not_found -> AnnotReachabilityDomain.SinkMap.empty in + let sink_map' = + if AnnotReachabilityDomain.SinkMap.mem sink sink_map + then sink_map + else + let singleton = AnnotReachabilityDomain.CallSites.singleton call_site in + AnnotReachabilityDomain.SinkMap.singleton sink singleton in + if phys_equal sink_map' sink_map then astate - else (AnnotReachabilityDomain.add annot call_set' annot_map, previous_vstate) + else (AnnotReachabilityDomain.add annot sink_map' annot_map, previous_vstate) let stop_tracking (annot_map, _ : astate) = (annot_map, TrackingDomain.Bottom) @@ -149,16 +154,17 @@ let method_has_annot annot tenv pname = let method_overrides_annot annot tenv pname = method_overrides (method_has_annot annot) tenv pname -let lookup_annotation_calls caller_pdesc annot pname : CallSite.SetDomain.t = +let lookup_annotation_calls caller_pdesc annot pname = match Ondemand.analyze_proc_name ~propagate_exceptions:false caller_pdesc pname with | Some { Specs.payload = { Specs.annot_map = Some annot_map; }; } -> begin try Annot.Map.find annot annot_map with Not_found -> - CallSite.SetDomain.empty + AnnotReachabilityDomain.SinkMap.empty end - | _ -> CallSite.SetDomain.empty + | _ -> + AnnotReachabilityDomain.SinkMap.empty let update_trace loc trace = if Location.equal loc Location.dummy then trace @@ -207,7 +213,7 @@ let report_annotation_stack src_annot snk_annot src_summary loc trace stack_str Exceptions.Checkers (msg, Localise.verbatim_desc description) in Reporting.log_error_from_summary src_summary ~loc ~ltr:final_trace exn -let report_call_stack summary end_of_stack lookup_next_calls report call_site calls = +let report_call_stack summary end_of_stack lookup_next_calls report call_site sink_map = (* TODO: stop using this; we can use the call site instead *) let lookup_location pname = match Specs.get_summary pname with @@ -222,23 +228,29 @@ let report_call_stack summary end_of_stack lookup_next_calls report call_site ca let callee_pname_str = string_of_pname callee_pname in let new_stack_str = stack_str ^ callee_pname_str ^ " -> " in let new_trace = update_trace call_loc trace |> update_trace callee_def_loc in - let unseen_callees, visited_callees = - CallSite.SetDomain.fold - (fun call_site ((unseen, visited) as accu) -> - let p = CallSite.pname call_site in - let loc = CallSite.loc call_site in - if Typ.Procname.Set.mem p visited then accu - else ((p, loc) :: unseen, Typ.Procname.Set.add p visited)) + let unseen_callees, updated_callees = + AnnotReachabilityDomain.SinkMap.fold + (fun _ call_sites ((unseen, visited) as accu) -> + try + let call_site = AnnotReachabilityDomain.CallSites.choose call_sites in + let p = CallSite.pname call_site in + let loc = CallSite.loc call_site in + if Typ.Procname.Set.mem p visited then accu + else ((p, loc) :: unseen, Typ.Procname.Set.add p visited) + with Not_found -> accu) next_calls ([], visited_pnames) in - List.iter ~f:(loop fst_call_loc visited_callees (new_trace, new_stack_str)) unseen_callees in - CallSite.SetDomain.iter - (fun fst_call_site -> - let fst_callee_pname = CallSite.pname fst_call_site in - let fst_call_loc = CallSite.loc fst_call_site in - let start_trace = update_trace (CallSite.loc call_site) [] in - loop fst_call_loc Typ.Procname.Set.empty (start_trace, "") (fst_callee_pname, fst_call_loc)) - calls + List.iter ~f:(loop fst_call_loc updated_callees (new_trace, new_stack_str)) unseen_callees in + AnnotReachabilityDomain.SinkMap.iter + (fun _ call_sites -> + try + let fst_call_site = AnnotReachabilityDomain.CallSites.choose call_sites in + let fst_callee_pname = CallSite.pname fst_call_site in + let fst_call_loc = CallSite.loc fst_call_site in + let start_trace = update_trace (CallSite.loc call_site) [] in + loop fst_call_loc Typ.Procname.Set.empty (start_trace, "") (fst_callee_pname, fst_call_loc) + with Not_found -> ()) + sink_map module TransferFunctions (CFG : ProcCfg.S) = struct module CFG = CFG @@ -285,7 +297,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct ~f:(fun astate (_, annot) -> if method_has_annot annot tenv callee_pname && not (method_is_sanitizer annot tenv caller_pname) - then Domain.add_call_site annot call_site astate + then Domain.add_call_site annot callee_pname call_site astate else astate) src_snk_pairs @@ -293,11 +305,16 @@ module TransferFunctions (CFG : ProcCfg.S) = struct match Summary.read_summary pdesc callee_pname with | None -> astate | Some callee_call_map -> + let add_call_site annot sink calls astate = + if AnnotReachabilityDomain.CallSites.is_empty calls + then astate + else Domain.add_call_site annot sink call_site astate in Annot.Map.fold - (fun annot calls astate -> - if CallSite.SetDomain.is_empty calls - then astate - else Domain.add_call_site annot call_site astate) + (fun annot sink_map astate -> + AnnotReachabilityDomain.SinkMap.fold + (add_call_site annot) + sink_map + astate) callee_call_map astate @@ -359,7 +376,7 @@ module Interprocedural = struct PatternMatch.override_iter check_expensive_subtyping_rules tenv proc_name; let report_src_snk_paths annot_map (src_annot_list, (snk_annot: Annot.t)) = - let report_src_snk_path (calls : CallSite.SetDomain.t) (src_annot: Annot.t) = + let report_src_snk_path sink_map (src_annot: Annot.t) = if method_overrides_annot src_annot tenv proc_name then let f_report = @@ -370,7 +387,7 @@ module Interprocedural = struct (lookup_annotation_calls proc_desc snk_annot) f_report (CallSite.make proc_name loc) - calls in + sink_map in try let sink_map = Annot.Map.find snk_annot annot_map in List.iter ~f:(report_src_snk_path sink_map) src_annot_list diff --git a/infer/tests/codetoanalyze/java/checkers/AnnotationReachabilityDuplicatesExample.java b/infer/tests/codetoanalyze/java/checkers/AnnotationReachabilityDuplicatesExample.java new file mode 100644 index 000000000..1af39617d --- /dev/null +++ b/infer/tests/codetoanalyze/java/checkers/AnnotationReachabilityDuplicatesExample.java @@ -0,0 +1,54 @@ +/* + * Copyright (c) 2013 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +package codetoanalyze.java.checkers; + +import com.facebook.infer.annotation.Expensive; +import com.facebook.infer.annotation.PerformanceCritical; + +public class AnnotationReachabilityDuplicatesExample { + + + @Expensive + native void expensive(); + + void callsExpensive1() { + expensive(); + } + + void callsExpensive2() { + expensive(); + } + + void callsExpensiveTwice() { + callsExpensive1(); + callsExpensive2(); + } + + @PerformanceCritical + void perfCriticalBad2() { + callsExpensiveTwice(); // should report here only once + } + + native boolean star(); + + void callsEitherExpensive() { + if (star()) { + callsExpensive1(); + } else { + callsExpensive2(); + } + } + + @PerformanceCritical + void perfCriticalBad1() { + callsEitherExpensive(); // should report here only once + } + +} diff --git a/infer/tests/codetoanalyze/java/checkers/issues.exp b/infer/tests/codetoanalyze/java/checkers/issues.exp index d6121a19f..bcb3eff29 100644 --- a/infer/tests/codetoanalyze/java/checkers/issues.exp +++ b/infer/tests/codetoanalyze/java/checkers/issues.exp @@ -1,3 +1,5 @@ +codetoanalyze/java/checkers/AnnotationReachabilityDuplicatesExample.java, void AnnotationReachabilityDuplicatesExample.perfCriticalBad1(), 1, CHECKERS_CALLS_EXPENSIVE_METHOD, [] +codetoanalyze/java/checkers/AnnotationReachabilityDuplicatesExample.java, void AnnotationReachabilityDuplicatesExample.perfCriticalBad2(), 1, CHECKERS_CALLS_EXPENSIVE_METHOD, [] codetoanalyze/java/checkers/CustomAnnotations.java, void CustomAnnotations.source1Bad(), 1, CHECKERS_ANNOTATION_REACHABILITY_ERROR, [] codetoanalyze/java/checkers/CustomAnnotations.java, void CustomAnnotations.source2Bad(), 1, CHECKERS_ANNOTATION_REACHABILITY_ERROR, [] codetoanalyze/java/checkers/ExpensiveCallExample.java, View ExpensiveCallExample.callsFindViewByIdFromActivity(FragmentActivity,int), 1, CHECKERS_CALLS_EXPENSIVE_METHOD, []