diff --git a/infer/src/checkers/liveness.ml b/infer/src/checkers/liveness.ml index af5a8f9e1..1b1f45541 100644 --- a/infer/src/checkers/liveness.ml +++ b/infer/src/checkers/liveness.ml @@ -39,20 +39,20 @@ let string_list_of_json ~option_name ~init = function module type LivenessConfig = sig - val is_blacklisted_destructor : Procname.t -> bool + val is_dangerous_destructor : 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 + let is_dangerous_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 = +module CheckerMode = struct + let dangerous_destructor_matcher = QualifiedCppName.Match.of_fuzzy_qual_names (string_list_of_json ~option_name:"liveness-dangerous-classes" ~init:[] Config.liveness_dangerous_classes) @@ -63,29 +63,41 @@ module CheckerMode : LivenessConfig = struct QualifiedCppName.Match.of_fuzzy_qual_names ["std::unique_ptr"; "std::shared_ptr"] - let is_blacklisted_class_name class_name = + let is_dangerous_class_name class_name = Typ.Name.unqualified_name class_name - |> QualifiedCppName.Match.match_qualifiers blacklisted_destructor_matcher + |> QualifiedCppName.Match.match_qualifiers dangerous_destructor_matcher - let is_wrapper_of_blacklisted_class_name class_name = + let is_wrapper_of_dangerous_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 + is_dangerous_class_name name | _ -> false - let is_blacklisted_destructor (callee_pname : Procname.t) = - match callee_pname with - | ObjC_Cpp cpp_pname when 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 + let is_dangerous_proc_name (proc_name : Procname.t) = + match proc_name with + | ObjC_Cpp cpp_pname -> + is_dangerous_class_name cpp_pname.class_name + || is_wrapper_of_dangerous_class_name cpp_pname.class_name | _ -> false + + + let is_destructor (proc_name : Procname.t) = + match proc_name with + | ObjC_Cpp cpp_pname -> + Procname.ObjC_Cpp.is_destructor cpp_pname + | _ -> + false + + + let is_dangerous_destructor (proc_name : Procname.t) = + is_destructor proc_name && is_dangerous_proc_name proc_name end (** compilers 101-style backward transfer functions for liveness analysis. gen a variable when it is @@ -129,8 +141,8 @@ module TransferFunctions (LConfig : LivenessConfig) (CFG : ProcCfg.S) = struct | 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" Procname.pp + when LConfig.is_dangerous_destructor callee_pname -> + Logging.d_printfln_escaped "Dangerous destructor %a, ignoring reads@\n" Procname.pp callee_pname ; Domain.remove (Var.of_id ret_id) astate | Sil.Call ((ret_id, _), call_exp, actuals, _, {CallFlags.cf_assign_last_arg}) -> @@ -166,37 +178,62 @@ let matcher_scope_guard = |> QualifiedCppName.Match.of_fuzzy_qual_names -module CapturedByRefTransferFunctions (CFG : ProcCfg.S) = struct +module PassedByRefTransferFunctions (CFG : ProcCfg.S) = struct module CFG = CFG module Domain = VarSet type analysis_data = unit - let exec_instr astate () _ instr = - List.fold (Sil.exps_of_instr instr) - ~f:(fun acc exp -> - Exp.fold_captured exp - ~f:(fun acc exp -> - match Exp.ignore_cast exp with - | Exp.Lvar pvar -> - (* captured by reference, add *) - Domain.add (Var.of_pvar pvar) acc - | _ -> - (* captured by value or init-capture, skip *) - acc ) - acc ) - ~init:astate + let add_if_lvar expr astate = + match Exp.ignore_cast expr with + | Exp.Lvar pvar -> + (* passed or captured by reference, add *) + Domain.add (Var.of_pvar pvar) astate + | _ -> + (* passed or captured by value or init-capture, skip *) + astate + + + let proc_name_of_expr expr = + match (expr : Exp.t) with Const (Cfun proc_name) -> Some proc_name | _ -> None + + + let is_dangerous expr = + (* ignore all captures from "dangerous" classes *) + proc_name_of_expr expr |> Option.exists ~f:CheckerMode.is_dangerous_proc_name + + + let exec_instr astate () _ (instr : Sil.instr) = + let astate = + match instr with + | Call (_ret, f, actuals, _loc, _flags) when not (is_dangerous f) -> + let actuals = + if Option.exists (proc_name_of_expr f) ~f:Procname.is_constructor then + (* Skip "this" in constructors, assuming constructors are less likely to have global + side effects that store "this" in the global state. We could also skip "this" in + all (non-static) methods but this becomes less clear: constructing an object then + calling methods on it can have side-effects even if the object is used for nothing + else. *) + List.tl actuals |> Option.value ~default:[] + else actuals + in + List.fold actuals ~init:astate ~f:(fun astate (actual, _typ) -> add_if_lvar actual astate) + | _ -> + astate + in + List.fold (Sil.exps_of_instr instr) ~init:astate ~f:(fun astate exp -> + Exp.fold_captured exp ~f:(fun astate exp -> add_if_lvar exp astate) astate ) - let pp_session_name _node fmt = F.pp_print_string fmt "captured by ref" + let pp_session_name _node fmt = F.pp_print_string fmt "passed by reference" end -module CapturedByRefAnalyzer = - AbstractInterpreter.MakeRPO (CapturedByRefTransferFunctions (ProcCfg.Exceptional)) +module PassedByRefAnalyzer = + AbstractInterpreter.MakeRPO (PassedByRefTransferFunctions (ProcCfg.Exceptional)) -let get_captured_by_ref_invariant_map proc_desc = +let get_passed_by_ref_invariant_map proc_desc = let cfg = ProcCfg.Exceptional.from_pdesc proc_desc in - CapturedByRefAnalyzer.exec_cfg cfg () ~initial:VarSet.empty + PassedByRefAnalyzer.exec_cfg cfg () ~initial:VarSet.empty module IntLitSet = Caml.Set.Make (IntLit) @@ -215,7 +252,7 @@ let ignored_constants = let checker {IntraproceduralAnalysis.proc_desc; err_log} = - let captured_by_ref_invariant_map = get_captured_by_ref_invariant_map proc_desc in + let passed_by_ref_invariant_map = get_passed_by_ref_invariant_map proc_desc in let cfg = CFG.from_pdesc proc_desc in let invariant_map = CheckerAnalyzer.exec_cfg cfg proc_desc ~initial:Domain.empty in (* we don't want to report in harmless cases like int i = 0; if (...) { i = ... } else { i = ... } @@ -250,11 +287,11 @@ let checker {IntraproceduralAnalysis.proc_desc; err_log} = |> Option.exists ~f:(fun local -> local.ProcAttributes.is_constexpr || local.ProcAttributes.is_declared_unused ) in - let should_report pvar typ live_vars captured_by_ref_vars = + let should_report pvar typ live_vars passed_by_ref_vars = not ( Pvar.is_frontend_tmp pvar || Pvar.is_return pvar || Pvar.is_global pvar || is_constexpr_or_unused pvar - || VarSet.mem (Var.of_pvar pvar) captured_by_ref_vars + || VarSet.mem (Var.of_pvar pvar) passed_by_ref_vars || Domain.mem (Var.of_pvar pvar) live_vars || Procdesc.is_captured_pvar proc_desc pvar || is_scope_guard typ @@ -268,15 +305,14 @@ let checker {IntraproceduralAnalysis.proc_desc; err_log} = let ltr = [Errlog.make_trace_element 0 loc "Write of unused value" []] in Reporting.log_issue proc_desc err_log ~loc ~ltr Liveness IssueType.dead_store message in - let report_dead_store live_vars captured_by_ref_vars = function + let report_dead_store live_vars passed_by_ref_vars = function | Sil.Store {e1= Lvar pvar; typ; e2= rhs_exp; loc} - when should_report pvar typ live_vars captured_by_ref_vars && not (is_sentinel_exp rhs_exp) -> + when should_report pvar typ live_vars passed_by_ref_vars && not (is_sentinel_exp rhs_exp) -> log_report pvar typ loc | Sil.Call (_, e_fun, (arg, typ) :: _, loc, _) -> ( match (Exp.ignore_cast e_fun, Exp.ignore_cast arg) with | Exp.Const (Cfun (Procname.ObjC_Cpp _ as pname)), Exp.Lvar pvar - when Procname.is_constructor pname && should_report pvar typ live_vars captured_by_ref_vars - -> + when Procname.is_constructor pname && should_report pvar typ live_vars passed_by_ref_vars -> log_report pvar typ loc | _, _ -> () ) @@ -284,11 +320,11 @@ let checker {IntraproceduralAnalysis.proc_desc; err_log} = () in let report_on_node node = - let captured_by_ref_vars = + let passed_by_ref_vars = match - CapturedByRefAnalyzer.extract_post + PassedByRefAnalyzer.extract_post (ProcCfg.Exceptional.Node.id (CFG.Node.underlying_node node)) - captured_by_ref_invariant_map + passed_by_ref_invariant_map with | Some post -> post @@ -299,7 +335,7 @@ let checker {IntraproceduralAnalysis.proc_desc; err_log} = Instrs.iter (CFG.instrs node) ~f:(fun instr -> match CheckerAnalyzer.extract_pre node_id invariant_map with | Some live_vars -> - report_dead_store live_vars captured_by_ref_vars instr + report_dead_store live_vars passed_by_ref_vars instr | None -> () ) in diff --git a/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp b/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp index 09b7ecc4d..e394bc1d1 100644 --- a/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp +++ b/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp @@ -64,13 +64,13 @@ void FN_capture_no_read_bad() { [x]() { return; }(); } -void init_capture_reassign_bad() { +int init_capture_reassign_bad() { int i = 1; // this is a dead store return [i = 1]() { return i; }(); } -void FN_init_capture_no_call_bad() { - [i = 1]() { return i; }; +auto FN_init_capture_no_call_bad() { + return [i = 1]() { return i; }; } void FN_init_capture_call_bad2() { @@ -150,7 +150,7 @@ int plus_plus_loop_ok(int n) { return i; } -void lambda_bad() { +auto lambda_bad() { int x = []() { int y = 1; y = 2; @@ -486,7 +486,7 @@ class Exceptions { return 0; } - void read_in_goto_ok(bool b) { + int read_in_goto_ok(bool b) { int i = 1; try { if (b) { @@ -506,6 +506,7 @@ class Exceptions { catch (...) { return i; } + return 0; } int return_in_try1_ok() { bool b; @@ -628,4 +629,31 @@ void ignored_constants_ok() { int z = 44; } +void store_total_in_global_state(int& total); +void reads_global_state_with_total_in_it(); + +void passed_by_ref_ok() { + int total; + store_total_in_global_state(total); + total = 42; + reads_global_state_with_total_in_it(); +} + +void FN_passed_by_ref_not_used_bad() { + int total; + store_total_in_global_state(total); + total = 42; + // any use of [total] would have to occur before function exit as accesses + // after function exit are invalid since total goes out of scope +} + +void passed_by_ref_in_loop_ok(int n) { + int total; + for (int i = 0; i < n; i++) { + store_total_in_global_state(total); + } + total = 42; + reads_global_state_with_total_in_it(); +} + } // namespace dead_stores