From 9346105426c5f7532df9de480f475c346857e407 Mon Sep 17 00:00:00 2001 From: Dulma Churchill Date: Wed, 27 Nov 2024 05:35:42 -0800 Subject: [PATCH] [dispatch-once-static-init] Improve domain with traces Summary: We keep a list of traces as the main element of the domain. We only add an element to the domain when there is a call to `dispatch_once` or when the summary is non-empty. Once we report an issue (only in a static initializer), we then don't keep track of the calls in the summary anymore. Reviewed By: jvillard Differential Revision: D66537767 fbshipit-source-id: 68ac7560cb0507ea05d99763fa2b63169fac666e --- infer/src/backend/Payloads.ml | 3 +- infer/src/checkers/DispatchOnceStaticInit.ml | 72 +++++++++++++------ infer/src/checkers/DispatchOnceStaticInit.mli | 6 +- .../DispatchOnceInStaticInit.m | 13 +++- .../objc/dispatch-once-static-init/issues.exp | 5 +- 5 files changed, 70 insertions(+), 29 deletions(-) diff --git a/infer/src/backend/Payloads.ml b/infer/src/backend/Payloads.ml index fc0d41a0ef..8464126898 100644 --- a/infer/src/backend/Payloads.ml +++ b/infer/src/backend/Payloads.ml @@ -65,7 +65,8 @@ let all_fields = ~config_impact_analysis:(fun f -> mk f ConfigImpactAnalysis ConfigImpactAnalysis.Summary.pp) ~cost:(fun f -> mk f Cost CostDomain.pp_summary) ~disjunctive_demo:(fun f -> mk f DisjunctiveDemo DisjunctiveDemo.pp_domain) - ~dispatch_once_static_init:(fun f -> mk f DisjunctiveDemo DispatchOnceStaticInit.Summary.pp) + ~dispatch_once_static_init:(fun f -> + mk f DispatchOnceStaticInit DispatchOnceStaticInit.Summary.pp ) ~litho_required_props:(fun f -> mk f LithoRequiredProps LithoDomain.pp_summary) ~pulse:(fun f -> mk_full f Pulse PulseSummary.pp) ~purity:(fun f -> mk f Purity PurityDomain.pp_summary) diff --git a/infer/src/checkers/DispatchOnceStaticInit.ml b/infer/src/checkers/DispatchOnceStaticInit.ml index 5e160ffe34..3e5d26f130 100644 --- a/infer/src/checkers/DispatchOnceStaticInit.ml +++ b/infer/src/checkers/DispatchOnceStaticInit.ml @@ -6,17 +6,27 @@ *) open! IStd module F = Format +module L = Logging module Mem = struct - type t = {loc: Location.t} [@@deriving compare] + type kind = [`Call | `Dispatch_once] [@@deriving compare, equal] - let pp fmt {loc} = F.fprintf fmt "calls_dispatch_once at %a" Location.pp loc + type trace_elem = {call_site: CallSite.t; kind: kind} [@@deriving compare, equal] + + type t = trace_elem list [@@deriving compare, equal] + + let pp_trace_elem fmt {call_site} = F.fprintf fmt "call to %a" CallSite.pp call_site + + let pp fmt trace = (Pp.comma_seq pp_trace_elem) fmt trace + + let add_call call trace_elems = call :: trace_elems end module Summary = struct include AbstractDomain.FiniteSet (Mem) - let add_calls_dispatch_once loc astate = add {Mem.loc} astate + let add_call t astate = + if is_empty astate then add (Mem.add_call t []) astate else map (Mem.add_call t) astate end module TransferFunctions = struct @@ -31,33 +41,48 @@ module TransferFunctions = struct (instr : Sil.instr) = match instr with | Call (_, Exp.Const (Const.Cfun procname), _, loc, _) -> + let call_site = CallSite.make procname loc in let astate = match analyze_dependency procname with | Ok summary -> - Summary.join astate summary + L.d_printf "Applying summary of callee `%a`@\n" Procname.pp procname ; + L.d_printf "Summary: %a @\n" Summary.pp summary ; + if not (Summary.is_empty summary) then + let astate' = Summary.add_call {call_site; kind= `Call} summary in + Summary.join astate astate' + else astate | Error _ -> astate in let calls_dispatch_once = String.equal "_dispatch_once" (Procname.get_method procname) in - if calls_dispatch_once then Summary.add_calls_dispatch_once loc astate else astate + if calls_dispatch_once then Summary.add_call {call_site; kind= `Dispatch_once} astate + else astate | _ -> astate end module Analyzer = AbstractInterpreter.MakeWTO (TransferFunctions) -let make_trace loc = - let trace_elem = Errlog.make_trace_element 0 loc "Call to `dispatch_once` here" [] in - [trace_elem] +let make_trace trace_elems {Mem.call_site} = + let loc = CallSite.loc call_site in + let procname = CallSite.pname call_site in + let trace_elem = + let s = F.asprintf "Call to %a" Procname.pp procname in + Errlog.make_trace_element 0 loc s [] + in + trace_elem :: trace_elems -let report_issue proc_desc err_log {Mem.loc} = - let ltr = make_trace loc in +let report_issue proc_desc err_log trace_elems = + let ltr = List.fold ~f:make_trace ~init:[] trace_elems |> List.rev in + let {Mem.call_site; kind} = List.hd_exn trace_elems in + let loc = CallSite.loc call_site in + let kind_to_string kind = match kind with `Call -> "an indirect" | `Dispatch_once -> "a" in let message = F.asprintf - "There is a call to `disptach_once` at line %a from a static constructor. This could cause a \ - deadlock." - Location.pp loc + "There is %s call to `disptach_once` at line %a from a static constructor. This could cause \ + a deadlock." + (kind_to_string kind) Location.pp loc in Reporting.log_issue proc_desc err_log ~ltr ~loc DispatchOnceStaticInit IssueType.dispatch_once_in_static_init message @@ -67,14 +92,15 @@ let checker ({InterproceduralAnalysis.proc_desc; err_log} as analysis_data) = let attributes = Procdesc.get_attributes proc_desc in let initial = Summary.empty in let summary_opt = Analyzer.compute_post analysis_data ~initial proc_desc in - ( if attributes.ProcAttributes.is_static_ctor then - match summary_opt with - | Some domain -> ( - match Summary.choose_opt domain with - | Some mem -> - report_issue proc_desc err_log mem - | None -> - () ) + if attributes.ProcAttributes.is_static_ctor then + match summary_opt with + | Some domain -> ( + match Summary.choose_opt domain with + | Some mem -> + report_issue proc_desc err_log mem ; + None | None -> - () ) ; - summary_opt + summary_opt ) + | None -> + summary_opt + else summary_opt diff --git a/infer/src/checkers/DispatchOnceStaticInit.mli b/infer/src/checkers/DispatchOnceStaticInit.mli index d4965c7cca..49d2788195 100644 --- a/infer/src/checkers/DispatchOnceStaticInit.mli +++ b/infer/src/checkers/DispatchOnceStaticInit.mli @@ -8,7 +8,11 @@ open! IStd module F = Format module Mem : sig - type t = {loc: Location.t} + type kind = [`Call | `Dispatch_once] [@@deriving compare, equal] + + type trace_elem = {call_site: CallSite.t; kind: kind} [@@deriving compare, equal] + + type t = trace_elem list [@@deriving compare, equal] val compare : t -> t -> int diff --git a/infer/tests/codetoanalyze/objc/dispatch-once-static-init/DispatchOnceInStaticInit.m b/infer/tests/codetoanalyze/objc/dispatch-once-static-init/DispatchOnceInStaticInit.m index a66e59f8fd..7c9999c713 100644 --- a/infer/tests/codetoanalyze/objc/dispatch-once-static-init/DispatchOnceInStaticInit.m +++ b/infer/tests/codetoanalyze/objc/dispatch-once-static-init/DispatchOnceInStaticInit.m @@ -23,14 +23,23 @@ + (instancetype)getInstance { @end -__attribute__((constructor)) static void initializer_test_interproc_bad(void) { +__attribute__((constructor)) static void initializer_test_interproc_bad() { [Manager getInstance]; } -__attribute__((constructor)) static void initializer_test_intraproc_bad(void) { +void foo_good() { initializer_test_interproc_bad(); } + +__attribute__((constructor)) static void initializer_test_intraproc_bad() { static Manager* manager; static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ manager = [Manager new]; }); } + +__attribute__((constructor)) static void +initializer_test_interproc_condition_bad(BOOL flag) { + if (flag) { + [Manager getInstance]; + } +} diff --git a/infer/tests/codetoanalyze/objc/dispatch-once-static-init/issues.exp b/infer/tests/codetoanalyze/objc/dispatch-once-static-init/issues.exp index 856ef24b39..4b90b0ee96 100644 --- a/infer/tests/codetoanalyze/objc/dispatch-once-static-init/issues.exp +++ b/infer/tests/codetoanalyze/objc/dispatch-once-static-init/issues.exp @@ -1,2 +1,3 @@ -codetoanalyze/objc/dispatch-once-static-init/DispatchOnceInStaticInit.m, initializer_test_interproc_bad, -8, DISPATCH_ONCE_IN_STATIC_INIT, no_bucket, ERROR, [macro expanded here,Call to `dispatch_once` here] -codetoanalyze/objc/dispatch-once-static-init/DispatchOnceInStaticInit.m, initializer_test_intraproc_bad, 3, DISPATCH_ONCE_IN_STATIC_INIT, no_bucket, ERROR, [macro expanded here,Call to `dispatch_once` here] +codetoanalyze/objc/dispatch-once-static-init/DispatchOnceInStaticInit.m, initializer_test_interproc_bad, 1, DISPATCH_ONCE_IN_STATIC_INIT, no_bucket, ERROR, [Call to Manager.getInstance,macro expanded here,Call to _dispatch_once] +codetoanalyze/objc/dispatch-once-static-init/DispatchOnceInStaticInit.m, initializer_test_intraproc_bad, 3, DISPATCH_ONCE_IN_STATIC_INIT, no_bucket, ERROR, [macro expanded here,Call to _dispatch_once] +codetoanalyze/objc/dispatch-once-static-init/DispatchOnceInStaticInit.m, initializer_test_interproc_condition_bad, 3, DISPATCH_ONCE_IN_STATIC_INIT, no_bucket, ERROR, [Call to Manager.getInstance,macro expanded here,Call to _dispatch_once]