diff --git a/infer/man/man1/infer-analyze.txt b/infer/man/man1/infer-analyze.txt index 12c09519e..f7ea39bc8 100644 --- a/infer/man/man1/infer-analyze.txt +++ b/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)) 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++: diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt index 24c6d5db2..3555de280 100644 --- a/infer/man/man1/infer-full.txt +++ b/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)) 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). diff --git a/infer/man/man1/infer.txt b/infer/man/man1/infer.txt index 99488474b..bb0ae3615 100644 --- a/infer/man/man1/infer.txt +++ b/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)) 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). diff --git a/infer/src/IR/Typ.ml b/infer/src/IR/Typ.ml index e61e9b04b..bcb9f9426 100644 --- a/infer/src/IR/Typ.ml +++ b/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 _ -> diff --git a/infer/src/IR/Typ.mli b/infer/src/IR/Typ.mli index 2c0296e89..d435e367a 100644 --- a/infer/src/IR/Typ.mli +++ b/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 diff --git a/infer/src/backend/preanal.ml b/infer/src/backend/preanal.ml index 71aa22b50..5bfa1a36b 100644 --- a/infer/src/backend/preanal.ml +++ b/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 diff --git a/infer/src/base/Config.ml b/infer/src/base/Config.ml index df4354bca..382985001 100644 --- a/infer/src/base/Config.ml +++ b/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)) 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 diff --git a/infer/src/base/Config.mli b/infer/src/base/Config.mli index 2aa064542..54e8b3381 100644 --- a/infer/src/base/Config.mli +++ b/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 diff --git a/infer/src/checkers/liveness.ml b/infer/src/checkers/liveness.ml index e0ac1b3a7..0447fbaa1 100644 --- a/infer/src/checkers/liveness.ml +++ b/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 -> diff --git a/infer/src/checkers/liveness.mli b/infer/src/checkers/liveness.mli new file mode 100644 index 000000000..9622bc2f6 --- /dev/null +++ b/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 diff --git a/infer/src/unit/livenessTests.ml b/infer/src/unit/livenessTests.ml index 184e847ef..9283869ea 100644 --- a/infer/src/unit/livenessTests.ml +++ b/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 diff --git a/infer/tests/codetoanalyze/cpp/liveness/.inferconfig b/infer/tests/codetoanalyze/cpp/liveness/.inferconfig index bf04eac65..6a8eb0b6a 100644 --- a/infer/tests/codetoanalyze/cpp/liveness/.inferconfig +++ b/infer/tests/codetoanalyze/cpp/liveness/.inferconfig @@ -1,4 +1,7 @@ { + "liveness-dangerous-classes": [ + "dead_stores::BlacklistedStruct" + ], "cxx-scope-guards": [ "infer::ScopeGuard" ] diff --git a/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp b/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp index fdb8ab2a3..25fccb5c1 100644 --- a/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp +++ b/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 clone() const { + return std::make_unique(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(*something); +} + +void unused_unique_ptr_good(A* something) { + auto x = std::make_unique(*something); +} + } // namespace dead_stores diff --git a/infer/tests/codetoanalyze/cpp/liveness/issues.exp b/infer/tests/codetoanalyze/cpp/liveness/issues.exp index 676977fcd..4d876360d 100644 --- a/infer/tests/codetoanalyze/cpp/liveness/issues.exp +++ b/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]