From 1320e79201cc7a1a1d91a1f53d876a121836852c Mon Sep 17 00:00:00 2001 From: Jules Villard Date: Wed, 24 Feb 2021 04:54:53 -0800 Subject: [PATCH] [liveness] ignore most dead stores when the variable has been passed by reference Summary: See added tests. Passing a variable by reference to a function `foo` can cause the variable to be added to the global state so any stores after that might be live as long as there is another function call after the store (since the global state shouldn't outlive the scope of the function). Currently we don't check that the latter is true; to report these we would need to extend the abstract domain to remember which stores have been made without a subsequent call. Also change Blacklisted -> Dangerous everywhere since the corresponding option is called "liveness_dangerous_classes". Reviewed By: skcho Differential Revision: D26606151 fbshipit-source-id: e869e5df1 --- infer/src/checkers/liveness.ml | 128 +++++++++++------- .../cpp/liveness/dead_stores.cpp | 38 +++++- 2 files changed, 115 insertions(+), 51 deletions(-) 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