diff --git a/infer/src/IR/Exp.ml b/infer/src/IR/Exp.ml index 3f2fcb407..a467c5c45 100644 --- a/infer/src/IR/Exp.ml +++ b/infer/src/IR/Exp.ml @@ -197,6 +197,23 @@ let get_vars exp = get_vars_ exp ([], []) +let fold_captured ~f exp acc = + let rec fold_captured_ exp captured_acc = + match exp with + | Cast (_, e) | UnOp (_, e, _) | Lfield (e, _, _) | Exn e | Sizeof {dynamic_length= Some e} -> + fold_captured_ e captured_acc + | BinOp (_, e1, e2) | Lindex (e1, e2) -> + fold_captured_ e1 captured_acc |> fold_captured_ e2 + | Closure {captured_vars} -> + List.fold captured_vars + ~f:(fun acc (_, captured_pvar, _) -> f acc captured_pvar) + ~init:captured_acc + | Const _ | Lvar _ | Var _ | Sizeof _ -> + captured_acc + in + fold_captured_ exp acc + + (** Pretty print an expression. *) let rec pp_ pe pp_t f e = let pp_exp = pp_ pe pp_t in diff --git a/infer/src/IR/Exp.mli b/infer/src/IR/Exp.mli index f83ed0873..069c36d03 100644 --- a/infer/src/IR/Exp.mli +++ b/infer/src/IR/Exp.mli @@ -119,6 +119,9 @@ val lt : t -> t -> t val get_vars : t -> Ident.t list * Pvar.t list (** Extract the ids and pvars from an expression *) +val fold_captured : f:('a -> Pvar.t -> 'a) -> t -> 'a -> 'a +(** Extract the program variables captured by this expression *) + val pp_printenv : Pp.env -> (Pp.env -> F.formatter -> Typ.t -> unit) -> F.formatter -> t -> unit val pp : F.formatter -> t -> unit diff --git a/infer/src/checkers/liveness.ml b/infer/src/checkers/liveness.ml index 573b36e14..d870f9889 100644 --- a/infer/src/checkers/liveness.ml +++ b/infer/src/checkers/liveness.ml @@ -13,7 +13,8 @@ module L = Logging (** backward analysis for computing set of maybe-live variables at each program point *) -module Domain = AbstractDomain.FiniteSet (Var) +module VarSet = AbstractDomain.FiniteSet (Var) +module Domain = VarSet (* compilers 101-style backward transfer functions for liveness analysis. gen a variable when it is read, kill the variable when it is assigned *) @@ -95,11 +96,32 @@ let matcher_scope_guard = |> QualifiedCppName.Match.of_fuzzy_qual_names +module CapturedByRefTransferFunctions (CFG : ProcCfg.S) = struct + module CFG = CFG + module Domain = VarSet + + type extras = ProcData.no_extras + + let exec_instr astate _ _ instr = + List.fold (Sil.instr_get_exps instr) + ~f:(fun acc exp -> + Exp.fold_captured ~f:(fun acc pvar -> Domain.add (Var.of_pvar pvar) acc) exp acc ) + ~init:astate +end + +module CapturedByRefAnalyzer = + AbstractInterpreter.Make (ProcCfg.Exceptional) (CapturedByRefTransferFunctions) + +let get_captured_by_ref_invariant_map proc_desc proc_data = + let cfg = ProcCfg.Exceptional.from_pdesc proc_desc in + CapturedByRefAnalyzer.exec_cfg cfg proc_data ~initial:VarSet.empty ~debug:false + + let checker {Callbacks.tenv; summary; proc_desc} : Specs.summary = + 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 (ProcData.make_default proc_desc tenv) ~initial:Domain.empty ~debug:true - in + let invariant_map = Analyzer.exec_cfg cfg proc_data ~initial:Domain.empty ~debug:false 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 *) @@ -119,9 +141,10 @@ let checker {Callbacks.tenv; summary; proc_desc} : Specs.summary = | _ -> false in - let should_report pvar typ live_vars = + let should_report pvar typ live_vars captured_by_ref_vars = not ( Pvar.is_frontend_tmp pvar || Pvar.is_return pvar || Pvar.is_global pvar + || VarSet.mem (Var.of_pvar pvar) captured_by_ref_vars || Domain.mem (Var.of_pvar pvar) live_vars || Procdesc.is_captured_var proc_desc pvar || is_scope_guard typ || Procdesc.has_modify_in_block_attr proc_desc pvar ) in @@ -134,24 +157,36 @@ let checker {Callbacks.tenv; summary; proc_desc} : Specs.summary = let exn = Exceptions.Checkers (IssueType.dead_store, Localise.verbatim_desc message) in Reporting.log_error summary ~loc ~ltr exn in - let report_dead_store live_vars = function + let report_dead_store live_vars captured_by_ref_vars = function | Sil.Store (Lvar pvar, typ, rhs_exp, loc) - when should_report pvar typ live_vars && not (is_sentinel_exp rhs_exp) -> + when should_report pvar typ live_vars captured_by_ref_vars && not (is_sentinel_exp rhs_exp) -> log_report pvar typ loc | Sil.Call (None, Exp.Const Cfun (Typ.Procname.ObjC_Cpp _ as pname), (Exp.Lvar pvar, typ) :: _, loc, _) - when Typ.Procname.is_constructor pname && should_report pvar typ live_vars -> + when Typ.Procname.is_constructor pname + && should_report pvar typ live_vars captured_by_ref_vars -> log_report pvar typ loc | _ -> () in let report_on_node node = + let captured_by_ref_vars = + match + CapturedByRefAnalyzer.extract_post + (ProcCfg.Exceptional.id (CFG.underlying_node node)) + captured_by_ref_invariant_map + with + | Some post -> + post + | None -> + VarSet.empty + in List.iter (CFG.instr_ids node) ~f:(fun (instr, node_id_opt) -> match node_id_opt with | Some node_id -> ( match Analyzer.extract_pre node_id invariant_map with | Some live_vars -> - report_dead_store live_vars instr + report_dead_store live_vars captured_by_ref_vars instr | None -> () ) | None -> diff --git a/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp b/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp index a5b142379..004c691c8 100644 --- a/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp +++ b/infer/tests/codetoanalyze/cpp/liveness/dead_stores.cpp @@ -130,12 +130,6 @@ void by_ref1_ok(int& ref) { ref = 7; } void by_ref2_ok(int& ref) { ref++; } -int capture_by_ref3_ok() { - int x = 1; - [&](auto y) { x += y; }(3); - return x; -} - int plus_plus_ok() { int x = 1; return ++x; @@ -183,6 +177,37 @@ int capture_by_ref2_ok() { return x + y; } +int capture_by_ref3_ok() { + int x = 1; + [&](auto y) { x += y; }(3); + return x; +} + +int capture_by_ref4_ok() { + int x = 1; + auto lambda = [&] { return x; }; + x = 2; // not a dead store; updates captured x + return lambda(); +} + +int dead_store_before_capture_by_ref_bad() { + int x = 1; // this is dead. should report it even though x is captured by ref + // later on + x = 2; + auto lambda = [&] { return x; }; + x = 2; + return lambda(); +} + +// frontend can't tell the difference between capture by ref and capture by +// value yet. +int FN_capture_by_value_bad() { + int x = 1; + auto lambda = [=] { return x; }; + x = 2; // this is dead + return lambda(); +} + int FN_capture_by_ref_reuseBad() { int x = 1; [&x]() { diff --git a/infer/tests/codetoanalyze/cpp/liveness/issues.exp b/infer/tests/codetoanalyze/cpp/liveness/issues.exp index 427cbed44..950b6ccad 100644 --- a/infer/tests/codetoanalyze/cpp/liveness/issues.exp +++ b/infer/tests/codetoanalyze/cpp/liveness/issues.exp @@ -1,12 +1,13 @@ codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::FP_assign_array_tricky_ok, 3, DEAD_STORE, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::dead_pointer_bad, 2, DEAD_STORE, [Write of unused value] +codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::dead_store_before_capture_by_ref_bad, 1, DEAD_STORE, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::dead_struct_no_destructor_bad, 0, DEAD_STORE, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::dead_struct_rvalue_ref_bad, 0, DEAD_STORE, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::dead_then_live_bad, 1, DEAD_STORE, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::easy_bad, 0, DEAD_STORE, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::init_capture_no_call_bad, 1, DEAD_STORE, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::init_capture_reassign_bad, 1, DEAD_STORE, [Write of unused value] -codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::lambda_bad::lambda_dead_stores.cpp:153:11_operator(), 1, DEAD_STORE, [Write of unused value] +codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::lambda_bad::lambda_dead_stores.cpp:147:11_operator(), 1, DEAD_STORE, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::plus_plus1_bad, 2, DEAD_STORE, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::plus_plus2_bad, 2, DEAD_STORE, [Write of unused value] codetoanalyze/cpp/liveness/dead_stores.cpp, dead_stores::plus_plus3_bad, 2, DEAD_STORE, [Write of unused value]