From 4c4bb84e2c5cfff75590fb420863f5cf146ba66d Mon Sep 17 00:00:00 2001 From: Jules Villard Date: Tue, 12 Feb 2019 06:29:20 -0800 Subject: [PATCH] [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 | 7 ++ infer/man/man1/infer-full.txt | 8 ++ infer/man/man1/infer.txt | 8 ++ infer/src/IR/Typ.ml | 2 + infer/src/IR/Typ.mli | 4 + infer/src/backend/preanal.ml | 3 +- infer/src/base/Config.ml | 12 +++ infer/src/base/Config.mli | 2 + infer/src/checkers/liveness.ml | 99 ++++++++++++++++--- infer/src/checkers/liveness.mli | 19 ++++ infer/src/unit/livenessTests.ml | 2 +- .../codetoanalyze/cpp/liveness/.inferconfig | 3 + .../cpp/liveness/dead_stores.cpp | 25 +++++ .../codetoanalyze/cpp/liveness/issues.exp | 3 + 14 files changed, 181 insertions(+), 16 deletions(-) create mode 100644 infer/src/checkers/liveness.mli 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]