[dispatch-once-static-init] New checker to flag when dispatch_once is…

… called inside a static constructor Summary: See T205836062 for the description of the problem. Here we build the checker to be intra-procedural, we'll extend it to be inter-procedural next. Reviewed By: ngorogiannis Differential Revision: D65544362 fbshipit-source-id: dbaa1c8df02539a82e30c46a555fea9dd03909e7
facebook · Nov 8, 2024 · a79ecc2 · a79ecc2
1 parent 3a86ac0
commit a79ecc2
Show file tree

Hide file tree

Showing 14 changed files with 163 additions and 14 deletions.
diff --git a/Makefile b/Makefile
@@ -140,6 +140,7 @@ DIRECT_TESTS += \
   objc_pulse \
   objc_pulse-data-lineage \
   objc_self-in-block \
+  objc_dispatch-once-static-init \
   objcpp_biabduction \
   objcpp_frontend \
   objcpp_liveness \

diff --git a/infer/man/man1/infer-analyze.txt b/infer/man/man1/infer-analyze.txt
@@ -114,10 +114,11 @@ OPTIONS
            and/or reporting. (Conversely: --deduplicate)
 
        --no-default-checkers
-           Deactivates: Default checkers: --fragment-retains-view,
-           --inefficient-keyset-iterator, --liveness,
-           --parameter-not-null-checked, --pulse, --racerd, --siof,
-           --self-in-block, --starvation (Conversely: --default-checkers)
+           Deactivates: Default checkers: --dispatch-once-static-init,
+           --fragment-retains-view, --inefficient-keyset-iterator,
+           --liveness, --parameter-not-null-checked, --pulse, --racerd,
+           --siof, --self-in-block, --starvation (Conversely:
+           --default-checkers)
 
        --detach-analysis-dependency
            Activates: Detach analysis dependencies of checkers during the
@@ -130,6 +131,15 @@ OPTIONS
        --dict-missing-key-var-block-list +string
            Skip analyzing the variables in the dict-missing-key checker.
 
+       --no-dispatch-once-static-init
+           Deactivates: dispatch-once-static-init checker: Detect if
+           dispatch_once is called from a static constructor. (Conversely:
+           --dispatch-once-static-init)
+
+       --dispatch-once-static-init-only
+           Activates: Enable dispatch-once-static-init and disable all other
+           checkers (Conversely: --no-dispatch-once-static-init-only)
+
        --files-to-analyze-index file
            File containing a list of source files where analysis should start
            from. When used, the set of files given to this argument must be a

diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt
@@ -539,10 +539,11 @@ OPTIONS
            infer-reportdiff(1).
 
        --no-default-checkers
-           Deactivates: Default checkers: --fragment-retains-view,
-           --inefficient-keyset-iterator, --liveness,
-           --parameter-not-null-checked, --pulse, --racerd, --siof,
-           --self-in-block, --starvation (Conversely: --default-checkers)
+           Deactivates: Default checkers: --dispatch-once-static-init,
+           --fragment-retains-view, --inefficient-keyset-iterator,
+           --liveness, --parameter-not-null-checked, --pulse, --racerd,
+           --siof, --self-in-block, --starvation (Conversely:
+           --default-checkers)
            See also infer-analyze(1).
 
        --dependencies
@@ -633,6 +634,7 @@ OPTIONS
              DATA_FLOW_TO_SINK (disabled by default),
              DEADLOCK (enabled by default),
              DEAD_STORE (enabled by default),
+             DISPATCH_ONCE_IN_STATIC_INIT (disabled by default),
              DIVIDE_BY_ZERO (disabled by default),
              DO_NOT_REPORT (enabled by default),
              EMPTY_VECTOR_ACCESS (enabled by default),
@@ -767,6 +769,17 @@ OPTIONS
 
            See also infer-report(1).
 
+       --no-dispatch-once-static-init
+           Deactivates: dispatch-once-static-init checker: Detect if
+           dispatch_once is called from a static constructor. (Conversely:
+           --dispatch-once-static-init)
+           See also infer-analyze(1).
+
+       --dispatch-once-static-init-only
+           Activates: Enable dispatch-once-static-init and disable all other
+           checkers (Conversely: --no-dispatch-once-static-init-only)
+           See also infer-analyze(1).
+
        --dump-duplicate-symbols
            Activates: Dump all symbols with the same name that are defined in
            more than one file. (Conversely: --no-dump-duplicate-symbols)

diff --git a/infer/man/man1/infer-report.txt b/infer/man/man1/infer-report.txt
@@ -149,6 +149,7 @@ OPTIONS
              DATA_FLOW_TO_SINK (disabled by default),
              DEADLOCK (enabled by default),
              DEAD_STORE (enabled by default),
+             DISPATCH_ONCE_IN_STATIC_INIT (disabled by default),
              DIVIDE_BY_ZERO (disabled by default),
              DO_NOT_REPORT (enabled by default),
              EMPTY_VECTOR_ACCESS (enabled by default),

diff --git a/infer/man/man1/infer.txt b/infer/man/man1/infer.txt
@@ -539,10 +539,11 @@ OPTIONS
            infer-reportdiff(1).
 
        --no-default-checkers
-           Deactivates: Default checkers: --fragment-retains-view,
-           --inefficient-keyset-iterator, --liveness,
-           --parameter-not-null-checked, --pulse, --racerd, --siof,
-           --self-in-block, --starvation (Conversely: --default-checkers)
+           Deactivates: Default checkers: --dispatch-once-static-init,
+           --fragment-retains-view, --inefficient-keyset-iterator,
+           --liveness, --parameter-not-null-checked, --pulse, --racerd,
+           --siof, --self-in-block, --starvation (Conversely:
+           --default-checkers)
            See also infer-analyze(1).
 
        --dependencies
@@ -633,6 +634,7 @@ OPTIONS
              DATA_FLOW_TO_SINK (disabled by default),
              DEADLOCK (enabled by default),
              DEAD_STORE (enabled by default),
+             DISPATCH_ONCE_IN_STATIC_INIT (disabled by default),
              DIVIDE_BY_ZERO (disabled by default),
              DO_NOT_REPORT (enabled by default),
              EMPTY_VECTOR_ACCESS (enabled by default),
@@ -767,6 +769,17 @@ OPTIONS
 
            See also infer-report(1).
 
+       --no-dispatch-once-static-init
+           Deactivates: dispatch-once-static-init checker: Detect if
+           dispatch_once is called from a static constructor. (Conversely:
+           --dispatch-once-static-init)
+           See also infer-analyze(1).
+
+       --dispatch-once-static-init-only
+           Activates: Enable dispatch-once-static-init and disable all other
+           checkers (Conversely: --no-dispatch-once-static-init-only)
+           See also infer-analyze(1).
+
        --dump-duplicate-symbols
            Activates: Dump all symbols with the same name that are defined in
            more than one file. (Conversely: --no-dump-duplicate-symbols)

diff --git a/infer/src/backend/registerCheckers.ml b/infer/src/backend/registerCheckers.ml
@@ -91,6 +91,8 @@ let all_checkers =
   [ {checker= SelfInBlock; callbacks= [(intraprocedural SelfInBlock.checker, Clang)]}
   ; { checker= ParameterNotNullChecked
     ; callbacks= [(intraprocedural ParameterNotNullChecked.checker, Clang)] }
+  ; { checker= DispatchOnceStaticInit
+    ; callbacks= [(intraprocedural DispatchOnceStaticInit.checker, Clang)] }
   ; { checker= BufferOverrunAnalysis
     ; callbacks=
         (let bo_analysis =

diff --git a/infer/src/base/Checker.ml b/infer/src/base/Checker.ml
@@ -17,6 +17,7 @@ type t =
   | ConfigImpactAnalysis
   | Cost
   | DisjunctiveDemo
+  | DispatchOnceStaticInit
   | FragmentRetainsView
   | Impurity
   | InefficientKeysetIterator
@@ -174,6 +175,14 @@ let config_unsafe checker =
       ; cli_flags= Some {deprecated= []; show_in_help= false}
       ; enabled_by_default= false
       ; activates= [] }
+  | DispatchOnceStaticInit ->
+      { id= "dispatch-once-static-init"
+      ; kind= UserFacing {title= "dispatch-once in static init"; markdown_body= ""}
+      ; support= mk_support_func ~clang:Support ()
+      ; short_documentation= "Detect if dispatch_once is called from a static constructor."
+      ; cli_flags= Some {deprecated= []; show_in_help= true}
+      ; enabled_by_default= true
+      ; activates= [] }
   | FragmentRetainsView ->
       { id= "fragment-retains-view"
       ; kind= UserFacing {title= "Fragment Retains View"; markdown_body= ""}

diff --git a/infer/src/base/Checker.mli b/infer/src/base/Checker.mli
@@ -16,6 +16,7 @@ type t =
   | ConfigImpactAnalysis
   | Cost
   | DisjunctiveDemo
+  | DispatchOnceStaticInit
   | FragmentRetainsView
   | Impurity
   | InefficientKeysetIterator

diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml
@@ -566,6 +566,14 @@ let deadlock =
     ~user_documentation:[%blob "./documentation/issues/DEADLOCK.md"]
 
 
+let dispatch_once_in_static_init =
+  register ~category:Concurrency ~enabled:false ~id:"DISPATCH_ONCE_IN_STATIC_INIT"
+    ~hum:"dispatch_once in static init" Error DispatchOnceStaticInit
+    ~user_documentation:
+      "Calling dispatch_once during the static initialization of objects is risky, for example it \
+       could cause deadlocks, because other objects might not have been initialized yet."
+
+
 let divide_by_zero =
   register ~category:NoCategory ~enabled:false ~id:"DIVIDE_BY_ZERO" Error Biabduction (* TODO *)
     ~user_documentation:""

diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli
@@ -183,6 +183,8 @@ val dead_store : t
 
 val deadlock : t
 
+val dispatch_once_in_static_init : t
+
 val divide_by_zero : t
 
 val do_not_report : t

diff --git a/infer/src/checkers/DispatchOnceStaticInit.ml b/infer/src/checkers/DispatchOnceStaticInit.ml
@@ -0,0 +1,70 @@
+(*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *)
+open! IStd
+module F = Format
+
+module Mem = struct
+  type t = {loc: Location.t} [@@deriving compare]
+
+  let pp fmt {loc} = F.fprintf fmt "calls_dispatch_once at %a" Location.pp loc
+end
+
+module Domain = struct
+  include AbstractDomain.FiniteSet (Mem)
+
+  let add_calls_dispatch_once loc astate = add {Mem.loc} astate
+end
+
+module TransferFunctions = struct
+  module Domain = Domain
+  module CFG = ProcCfg.Normal
+
+  type analysis_data = IntraproceduralAnalysis.t
+
+  let pp_session_name _node fmt = F.pp_print_string fmt "DispatchOnceStaticInit"
+
+  let exec_instr (astate : Domain.t) _ _cfg_node _ (instr : Sil.instr) =
+    match instr with
+    | Call (_, Exp.Const (Const.Cfun procname), _, loc, _) ->
+        let calls_dispatch_once = String.equal "_dispatch_once" (Procname.get_method procname) in
+        if calls_dispatch_once then Domain.add_calls_dispatch_once loc 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 report_issue proc_desc err_log {Mem.loc} =
+  let ltr = make_trace loc 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
+  in
+  Reporting.log_issue proc_desc err_log ~ltr ~loc DispatchOnceStaticInit
+    IssueType.dispatch_once_in_static_init message
+
+
+let checker ({IntraproceduralAnalysis.proc_desc; err_log} as analysis_data) =
+  let attributes = Procdesc.get_attributes proc_desc in
+  if attributes.ProcAttributes.is_static_ctor then
+    let initial = Domain.empty in
+    match Analyzer.compute_post analysis_data ~initial proc_desc with
+    | Some domain -> (
+      match Domain.choose_opt domain with
+      | Some mem ->
+          report_issue proc_desc err_log mem
+      | None ->
+          () )
+    | None ->
+        ()
diff --git a/infer/tests/codetoanalyze/objc/dispatch-once-static-init/DispatchOnceInStaticInit.m b/infer/tests/codetoanalyze/objc/dispatch-once-static-init/DispatchOnceInStaticInit.m
@@ -28,8 +28,7 @@ + (instancetype)getInstance {
   [Manager getInstance];
 }
 
-__attribute__((constructor)) static void initializer_test_intraproc_bad_FN(
-    void) {
+__attribute__((constructor)) static void initializer_test_intraproc_bad(void) {
   static Manager* manager;
   static dispatch_once_t onceToken;
   dispatch_once(&onceToken, ^{

diff --git a/infer/tests/codetoanalyze/objc/dispatch-once-static-init/Makefile b/infer/tests/codetoanalyze/objc/dispatch-once-static-init/Makefile
@@ -0,0 +1,19 @@
+# Copyright (c) Facebook, Inc. and its affiliates.
+#
+# This source code is licensed under the MIT license found in the
+# LICENSE file in the root directory of this source tree.
+
+TESTS_DIR = ../../..
+
+CLANG_OPTIONS = -c $(OBJC_CLANG_OPTIONS) -fobjc-arc
+INFER_OPTIONS = --dispatch-once-static-init-only --debug-exceptions --project-root $(TESTS_DIR)
+INFERPRINT_OPTIONS = \
+  --issues-tests-fields file,procedure,line_offset,bug_type,bucket,severity,bug_trace,taint_extra,transitive_callees_extra,autofix \
+  --issues-tests
+
+SOURCES = $(wildcard *.m)
+
+include $(TESTS_DIR)/clang.make
+include $(TESTS_DIR)/objc.make
+
+infer-out/report.json: $(MAKEFILE_LIST)
diff --git a/infer/tests/codetoanalyze/objc/dispatch-once-static-init/issues.exp b/infer/tests/codetoanalyze/objc/dispatch-once-static-init/issues.exp
@@ -0,0 +1 @@
+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]