[liveness] blacklist of dangerous classes

Summary:
Add an option to specify some classes that we really want to warn about
with the liveness checker, even when they appear used because of the
implicit destructor call inserted by the compiler.

Reviewed By: mbouaziz

Differential Revision: D13991129

fbshipit-source-id: 7fafdba84

infer/man/man1/infer-analyze.txt

@@ -332,6 +332,13 @@ CLANG OPTIONS
            Specify scope guard classes that can be read only by destructors
            without being reported as dead stores. (default: [])
 
+       --liveness-dangerous-classes json
+           Specify classes where the destructor should be ignored when
+           computing liveness. In other words, assignement to variables of
+           these types (or common wrappers around these types such as
+           $(u,unique_ptr<type>)) will count as dead stores when the
+           variables are not read explicitly by the program. (default: [])
+
        --ml-buckets ,-separated sequence of { all | cf | arc | narc | cpp |
        unknown_origin }
            Specify the memory leak buckets to be checked in C++:       

infer/man/man1/infer-full.txt

@@ -516,6 +516,14 @@ OPTIONS
            Deactivates: the detection of dead stores and unused variables
            (Conversely: --liveness)           See also infer-analyze(1).
 
+       --liveness-dangerous-classes json
+           Specify classes where the destructor should be ignored when
+           computing liveness. In other words, assignement to variables of
+           these types (or common wrappers around these types such as
+           $(u,unique_ptr<type>)) will count as dead stores when the
+           variables are not read explicitly by the program. (default: [])       
+    See also infer-analyze(1).
+
        --liveness-only
            Activates: Enable --liveness and disable all other checkers
            (Conversely: --no-liveness-only)           See also infer-analyze(1).

infer/man/man1/infer.txt

@@ -516,6 +516,14 @@ OPTIONS
            Deactivates: the detection of dead stores and unused variables
            (Conversely: --liveness)           See also infer-analyze(1).
 
+       --liveness-dangerous-classes json
+           Specify classes where the destructor should be ignored when
+           computing liveness. In other words, assignement to variables of
+           these types (or common wrappers around these types such as
+           $(u,unique_ptr<type>)) will count as dead stores when the
+           variables are not read explicitly by the program. (default: [])       
+    See also infer-analyze(1).
+
        --liveness-only
            Activates: Enable --liveness and disable all other checkers
            (Conversely: --no-liveness-only)           See also infer-analyze(1).

infer/src/IR/Typ.ml

@@ -368,6 +368,8 @@ module Name = struct
         QualifiedCppName.empty
 
 
+  let get_template_spec_info = function CppClass (_, templ_args) -> Some templ_args | _ -> None
+
   let name n =
     match n with
     | CStruct _ | CUnion _ | CppClass _ | ObjcClass _ | ObjcProtocol _ ->

infer/src/IR/Typ.mli

@@ -127,6 +127,8 @@ and template_spec_info =
       ; args: template_arg list }
 [@@deriving compare]
 
+val pp_template_spec_info : Pp.env -> F.formatter -> template_spec_info -> unit [@@warning "-32"]
+
 val mk : ?default:t -> ?quals:type_quals -> desc -> t
 (** Create Typ.t from given desc. if [default] is passed then use its value to set other fields such as quals *)
 
@@ -177,6 +179,8 @@ module Name : sig
 
   val unqualified_name : t -> QualifiedCppName.t
 
+  val get_template_spec_info : t -> template_spec_info option
+
   module C : sig
     val from_string : string -> t
 

infer/src/backend/preanal.ml

@@ -35,7 +35,8 @@ let add_abstraction_instructions pdesc =
 
 
 module BackwardCfg = ProcCfg.Backward (ProcCfg.Exceptional)
-module LivenessAnalysis = AbstractInterpreter.MakeRPO (Liveness.TransferFunctions (BackwardCfg))
+module LivenessAnalysis =
+  AbstractInterpreter.MakeRPO (Liveness.PreAnalysisTransferFunctions (BackwardCfg))
 module VarDomain = Liveness.Domain
 
 (** computes the non-nullified reaching definitions at the end of each node by building on the

infer/src/base/Config.ml

@@ -1489,6 +1489,15 @@ and join_cond =
 |}
 
 
+and liveness_dangerous_classes =
+  CLOpt.mk_json ~long:"liveness-dangerous-classes"
+    ~in_help:InferCommand.[(Analyze, manual_clang)]
+    "Specify classes where the destructor should be ignored when computing liveness. In other \
+     words, assignement to variables of these types (or common wrappers around these types such \
+     as $(u,unique_ptr<type>)) will count as dead stores when the variables are not read \
+     explicitly by the program."
+
+
 and log_events =
   CLOpt.mk_bool ~long:"log-events"
     ~in_help:InferCommand.[(Run, manual_generic)]
@@ -2691,6 +2700,7 @@ and iphoneos_target_sdk_version = !iphoneos_target_sdk_version
 and iphoneos_target_sdk_version_path_regex =
   process_iphoneos_target_sdk_version_path_regex !iphoneos_target_sdk_version_path_regex
 
+
 and issues_fields = !issues_fields
 
 and issues_tests = !issues_tests
@@ -2736,6 +2746,8 @@ and litho = !litho
 
 and liveness = !liveness
 
+and liveness_dangerous_classes = !liveness_dangerous_classes
+
 and load_average =
   match !load_average with None when !buck -> Some (float_of_int ncpu) | _ -> !load_average
 

infer/src/base/Config.mli

@@ -459,6 +459,8 @@ val litho : bool
 
 val liveness : bool
 
+val liveness_dangerous_classes : Yojson.Basic.json
+
 val log_events : bool
 
 val log_file : string

infer/src/checkers/liveness.ml

@@ -7,18 +7,88 @@
 
 open! IStd
 module F = Format
+module L = Logging
 
 (** backward analysis for computing set of maybe-live variables at each program point *)
 
 module VarSet = AbstractDomain.FiniteSet (Var)
 module Domain = VarSet
 
-(* only kill pvars that are local; don't kill those that can escape *)
+(** only kill pvars that are local; don't kill those that can escape *)
 let is_always_in_scope pvar = Pvar.is_return pvar || Pvar.is_global pvar
 
-(* compilers 101-style backward transfer functions for liveness analysis. gen a variable when it is
-   read, kill the variable when it is assigned *)
-module TransferFunctions (CFG : ProcCfg.S) = struct
+let json_error ~option_name ~expected ~actual =
+  L.die UserError "When parsing option %s: expected %s but got '%s'" option_name expected
+    (Yojson.Basic.Util.to_string actual)
+
+
+let string_list_of_json ~option_name ~init = function
+  | `List json ->
+      List.fold json
+        ~f:(fun acc json ->
+          match json with
+          | `String s ->
+              s :: acc
+          | _ ->
+              json_error ~option_name ~expected:"string" ~actual:json )
+        ~init
+  | json ->
+      json_error ~option_name ~expected:"list of strings" ~actual:json
+
+
+module type LivenessConfig = sig
+  val is_blacklisted_destructor : Typ.Procname.t -> bool
+end
+
+(** Use this config to get a reliable liveness pre-analysis that tells you which variables are live
+   at which program point *)
+module PreAnalysisMode : LivenessConfig = struct
+  (** do not do any funky stuff *)
+  let is_blacklisted_destructor _proc_name = false
+end
+
+(** Use this config to get a dead store checker that can take some liberties wrt a strict liveness
+   analysis *)
+module CheckerMode : LivenessConfig = struct
+  let blacklisted_destructor_matcher =
+    QualifiedCppName.Match.of_fuzzy_qual_names
+      (string_list_of_json ~option_name:"liveness-dangerous-classes" ~init:[]
+         Config.liveness_dangerous_classes)
+
+
+  (** hardcoded list of wrappers, mostly because they are impossible to specify as config options *)
+  let standard_wrappers_matcher =
+    QualifiedCppName.Match.of_fuzzy_qual_names ["std::unique_ptr"; "std::shared_ptr"]
+
+
+  let is_blacklisted_class_name class_name =
+    Typ.Name.unqualified_name class_name
+    |> QualifiedCppName.Match.match_qualifiers blacklisted_destructor_matcher
+
+
+  let is_wrapper_of_blacklisted_class_name class_name =
+    Typ.Name.unqualified_name class_name
+    |> QualifiedCppName.Match.match_qualifiers standard_wrappers_matcher
+    &&
+    match Typ.Name.get_template_spec_info class_name with
+    | Some (Template {args= TType {desc= Tstruct name} :: _; _}) ->
+        is_blacklisted_class_name name
+    | _ ->
+        false
+
+
+  let is_blacklisted_destructor (callee_pname : Typ.Procname.t) =
+    match callee_pname with
+    | ObjC_Cpp cpp_pname when Typ.Procname.ObjC_Cpp.is_destructor cpp_pname ->
+        is_blacklisted_class_name cpp_pname.class_name
+        || is_wrapper_of_blacklisted_class_name cpp_pname.class_name
+    | _ ->
+        false
+end
+
+(** compilers 101-style backward transfer functions for liveness analysis. gen a variable when it is
+    read, kill the variable when it is assigned *)
+module TransferFunctions (LConfig : LivenessConfig) (CFG : ProcCfg.S) = struct
   module CFG = CFG
   module Domain = Domain
 
@@ -68,6 +138,11 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
         exp_add_live lhs_exp astate |> exp_add_live rhs_exp
     | Sil.Prune (exp, _, _, _) ->
         exp_add_live exp astate
+    | Sil.Call ((ret_id, _), Const (Cfun callee_pname), _, _, _)
+      when LConfig.is_blacklisted_destructor callee_pname ->
+        Logging.d_printfln_escaped "Blacklisted destructor %a, ignoring reads@\n" Typ.Procname.pp
+          callee_pname ;
+        Domain.remove (Var.of_id ret_id) astate
     | Sil.Call ((ret_id, _), call_exp, actuals, _, {CallFlags.cf_assign_last_arg}) ->
         let actuals_to_read, astate =
           if cf_assign_last_arg then
@@ -89,20 +164,16 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
 end
 
 module CFG = ProcCfg.OneInstrPerNode (ProcCfg.Backward (ProcCfg.Exceptional))
-module Analyzer = AbstractInterpreter.MakeRPO (TransferFunctions (CFG))
+module CheckerAnalyzer = AbstractInterpreter.MakeRPO (TransferFunctions (CheckerMode) (CFG))
+module PreAnalysisTransferFunctions = TransferFunctions (PreAnalysisMode)
 
 (* It's fine to have a dead store on a type that uses the "scope guard" pattern. These types
    are only read in their destructors, and this is expected/ok.
    (e.g., https://github.com/facebook/folly/blob/master/folly/ScopeGuard.h). *)
 let matcher_scope_guard =
-  let of_json init = function
-    | `List scope_guards ->
-        List.fold scope_guards ~f:(fun acc json -> Yojson.Basic.Util.to_string json :: acc) ~init
-    | _ ->
-        init
-  in
   let default_scope_guards = ["CKComponentKey"; "CKComponentScope"] in
-  of_json default_scope_guards Config.cxx_scope_guards
+  string_list_of_json ~option_name:"cxx-scope_guards" ~init:default_scope_guards
+    Config.cxx_scope_guards
   |> QualifiedCppName.Match.of_fuzzy_qual_names
 
 
@@ -143,7 +214,7 @@ let checker {Callbacks.tenv; summary; proc_desc} : Summary.t =
   let proc_data = ProcData.make_default proc_desc tenv in
   let captured_by_ref_invariant_map = get_captured_by_ref_invariant_map proc_desc proc_data in
   let cfg = CFG.from_pdesc proc_desc in
-  let invariant_map = Analyzer.exec_cfg cfg proc_data ~initial:Domain.empty in
+  let invariant_map = CheckerAnalyzer.exec_cfg cfg proc_data ~initial:Domain.empty in
   (* we don't want to report in harmless cases like int i = 0; if (...) { i = ... } else { i = ... }
      that create an intentional dead store as an attempt to imitate default value semantics.
      use dead stores to a "sentinel" value as a heuristic for ignoring this case *)
@@ -218,7 +289,7 @@ let checker {Callbacks.tenv; summary; proc_desc} : Summary.t =
     in
     let node_id = CFG.Node.id node in
     Instrs.iter (CFG.instrs node) ~f:(fun instr ->
-        match Analyzer.extract_pre node_id invariant_map with
+        match CheckerAnalyzer.extract_pre node_id invariant_map with
         | Some live_vars ->
             report_dead_store live_vars captured_by_ref_vars instr
         | None ->

infer/src/checkers/liveness.mli

@@ -0,0 +1,19 @@
+(*
+ * Copyright (c) 2019-present, Facebook, Inc.
+ *
+ * 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 VarSet : module type of AbstractDomain.FiniteSet (Var)
+
+module Domain = VarSet
+
+module PreAnalysisTransferFunctions (CFG : ProcCfg.S) :
+  TransferFunctions.SIL
+  with module CFG = CFG
+   and module Domain = Domain
+   and type extras = ProcData.no_extras
+
+val checker : Callbacks.proc_callback_args -> Summary.t

infer/src/unit/livenessTests.ml

@@ -7,7 +7,7 @@
 
 open! IStd
 module TestInterpreter =
-  AnalyzerTester.Make (Liveness.TransferFunctions (ProcCfg.Backward (ProcCfg.Normal)))
+  AnalyzerTester.Make (Liveness.PreAnalysisTransferFunctions (ProcCfg.Backward (ProcCfg.Normal)))
 
 let tests =
   let open OUnit2 in

infer/tests/codetoanalyze/cpp/liveness/.inferconfig

@@ -1,4 +1,7 @@
 {
+  "liveness-dangerous-classes": [
+    "dead_stores::BlacklistedStruct"
+  ],
   "cxx-scope-guards": [
     "infer::ScopeGuard"
   ]

infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp

@@ -515,4 +515,29 @@ int decltype_read_ok_FP(int x) {
   return x + i;
 }
 
+// destructor blacklisted for liveness in .inferconfig
+struct BlacklistedStruct {
+  ~BlacklistedStruct(){};
+
+  BlacklistedStruct cloneAsValue() const { return BlacklistedStruct(); }
+
+  std::unique_ptr<BlacklistedStruct> clone() const {
+    return std::make_unique<BlacklistedStruct>(cloneAsValue());
+  }
+};
+
+void unused_blacklisted_constructed_bad() { auto x = BlacklistedStruct(); }
+
+void unused_blacklisted_clone_bad(BlacklistedStruct* something) {
+  auto x = something->clone();
+}
+
+void unused_blacklisted_unique_ptr_bad(BlacklistedStruct* something) {
+  auto x = std::make_unique<BlacklistedStruct>(*something);
+}
+
+void unused_unique_ptr_good(A* something) {
+  auto x = std::make_unique<A>(*something);
+}
+
 } // namespace dead_stores

infer/tests/codetoanalyze/cpp/liveness/issues.exp

@@ -18,6 +18,9 @@ codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::plus_plus1_bad, 2, DEAD
 codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::plus_plus2_bad, 2, DEAD_STORE, no_bucket, ERROR, [Write of unused value]
 codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::plus_plus3_bad, 2, DEAD_STORE, no_bucket, ERROR, [Write of unused value]
 codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::reassign_param_bad, 0, DEAD_STORE, no_bucket, ERROR, [Write of unused value]
+codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::unused_blacklisted_clone_bad, 1, DEAD_STORE, no_bucket, ERROR, [Write of unused value]
+codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::unused_blacklisted_constructed_bad, 0, DEAD_STORE, no_bucket, ERROR, [Write of unused value]
+codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::unused_blacklisted_unique_ptr_bad, 1, DEAD_STORE, no_bucket, ERROR, [Write of unused value]
 codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::unused_tmp_bad, 0, DEAD_STORE, no_bucket, ERROR, [Write of unused value]
 codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::use_then_dead_bad, 3, DEAD_STORE, no_bucket, ERROR, [Write of unused value]
 codetoanalyze/cpp/liveness/dead_stores_constexpr.cpp, capture_const_bad, 1, DEAD_STORE, no_bucket, ERROR, [Write of unused value]