From 1b79f13a1880506064f5c39c4d27a00315bc4910 Mon Sep 17 00:00:00 2001 From: Jules Villard Date: Tue, 18 Dec 2018 03:25:28 -0800 Subject: [PATCH] [ownership] make heuristic for reporting on lambdas more shareable Summary: I want to use that function in Pulse too so let's share it. Reviewed By: da319 Differential Revision: D13466897 fbshipit-source-id: 1bf3afee4 --- infer/src/IR/Procdesc.ml | 6 +++++- infer/src/IR/Procdesc.mli | 5 ++++- infer/src/IR/Var.ml | 13 +++++++++++-- infer/src/IR/Var.mli | 4 ++-- infer/src/checkers/Ownership.ml | 13 +++++++------ infer/src/checkers/liveness.ml | 2 +- infer/src/checkers/uninit.ml | 2 +- .../tests/codetoanalyze/cpp/ownership/closures.cpp | 13 +++++++++++-- infer/tests/codetoanalyze/cpp/ownership/issues.exp | 1 + 9 files changed, 43 insertions(+), 16 deletions(-) diff --git a/infer/src/IR/Procdesc.ml b/infer/src/IR/Procdesc.ml index 78efc825e..690eba396 100644 --- a/infer/src/IR/Procdesc.ml +++ b/infer/src/IR/Procdesc.ml @@ -710,7 +710,7 @@ let is_specialized pdesc = (* true if pvar is a captured variable of a cpp lambda or objc block *) -let is_captured_var procdesc pvar = +let is_captured_pvar procdesc pvar = let procname = get_proc_name procdesc in let pvar_name = Pvar.get_name pvar in let pvar_local_matches (var_data : ProcAttributes.var_data) = @@ -735,6 +735,10 @@ let is_captured_var procdesc pvar = is_captured_var_cpp_lambda || is_captured_var_objc_block +let is_captured_var procdesc var = + Var.get_pvar var |> Option.exists ~f:(fun pvar -> is_captured_pvar procdesc pvar) + + let has_modify_in_block_attr procdesc pvar = let pvar_name = Pvar.get_name pvar in let pvar_local_matches (var_data : ProcAttributes.var_data) = diff --git a/infer/src/IR/Procdesc.mli b/infer/src/IR/Procdesc.mli index 3a8228acf..e41060b04 100644 --- a/infer/src/IR/Procdesc.mli +++ b/infer/src/IR/Procdesc.mli @@ -288,9 +288,12 @@ val pp_local : Format.formatter -> ProcAttributes.var_data -> unit val is_specialized : t -> bool -val is_captured_var : t -> Pvar.t -> bool +val is_captured_pvar : t -> Pvar.t -> bool (** true if pvar is a captured variable of a cpp lambda or obcj block *) +val is_captured_var : t -> Var.t -> bool +(** true if var is a captured variable of a cpp lambda or obcj block *) + val has_modify_in_block_attr : t -> Pvar.t -> bool val is_connected : t -> (unit, [`Join | `Other]) Result.t diff --git a/infer/src/IR/Var.ml b/infer/src/IR/Var.ml index d615f45ac..2235f3e36 100644 --- a/infer/src/IR/Var.ml +++ b/infer/src/IR/Var.ml @@ -36,8 +36,6 @@ let get_ident = function ProgramVar _ -> None | LogicalVar id -> Some id let get_pvar = function ProgramVar pvar -> Some pvar | LogicalVar _ -> None -let get_mangled = function ProgramVar pvar -> Some (Pvar.get_name pvar) | LogicalVar _ -> None - let is_global = function ProgramVar pvar -> Pvar.is_global pvar | LogicalVar _ -> false let is_return = function ProgramVar pvar -> Pvar.is_return pvar | LogicalVar _ -> false @@ -48,6 +46,17 @@ let is_footprint = function ProgramVar _ -> false | LogicalVar id -> Ident.is_fo let is_none = function LogicalVar id -> Ident.is_none id | _ -> false +let get_declaring_function = function + | LogicalVar _ -> + None + | ProgramVar pvar -> + Pvar.get_declaring_function pvar + + +let is_local_to_procedure proc_name var = + get_declaring_function var |> Option.exists ~f:(Typ.Procname.equal proc_name) + + let get_all_vars_in_exp e = let acc = Exp.free_vars e |> Sequence.map ~f:of_id in Exp.program_vars e |> Sequence.map ~f:of_pvar |> Sequence.append acc diff --git a/infer/src/IR/Var.mli b/infer/src/IR/Var.mli index 72e8aac08..e67203378 100644 --- a/infer/src/IR/Var.mli +++ b/infer/src/IR/Var.mli @@ -31,10 +31,10 @@ val get_ident : t -> Ident.t option val get_pvar : t -> Pvar.t option -val get_mangled : t -> Mangled.t option - val is_global : t -> bool +val is_local_to_procedure : Typ.Procname.t -> t -> bool + val is_return : t -> bool val is_this : t -> bool diff --git a/infer/src/checkers/Ownership.ml b/infer/src/checkers/Ownership.ml index bb135bb2b..f910de9e4 100644 --- a/infer/src/checkers/Ownership.ml +++ b/infer/src/checkers/Ownership.ml @@ -382,13 +382,14 @@ end module Analyzer = LowerHil.MakeAbstractInterpreter (TransferFunctions (ProcCfg.Exceptional)) let report_invalid_return post end_loc summary = - let locals = - Procdesc.get_locals summary.Summary.proc_desc |> List.map ~f:(fun {ProcAttributes.name} -> name) - in + let proc_name = Summary.get_proc_name summary in let is_local_to_procedure var = - Var.get_mangled var - |> Option.value_map ~default:false ~f:(fun mangled -> - List.mem ~equal:Mangled.equal locals mangled ) + (* In case of lambdas, only report on variables local to the lambda that are not captured from + the enclosing procedure to avoid false positives in case the lambda is called within the + same procedure (which is frequent, at the cost of false negatives in case this lambda is + returned to the caller). *) + Var.is_local_to_procedure proc_name var + && not (Procdesc.is_captured_var summary.Summary.proc_desc var) in (* look for return values that are borrowed from (now-invalid) local variables *) let report_invalid_return base (capability : CapabilityDomain.t) = diff --git a/infer/src/checkers/liveness.ml b/infer/src/checkers/liveness.ml index 9bfdc9471..c527b0993 100644 --- a/infer/src/checkers/liveness.ml +++ b/infer/src/checkers/liveness.ml @@ -179,7 +179,7 @@ let checker {Callbacks.tenv; summary; proc_desc} : Summary.t = || is_unused_tmp_var 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 + || Procdesc.is_captured_pvar proc_desc pvar || is_scope_guard typ || Procdesc.has_modify_in_block_attr proc_desc pvar ) in diff --git a/infer/src/checkers/uninit.ml b/infer/src/checkers/uninit.ml index c81c1e1d9..ee90fc482 100644 --- a/infer/src/checkers/uninit.ml +++ b/infer/src/checkers/uninit.ml @@ -69,7 +69,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct match (HilExp.AccessExpression.get_typ access_expr tenv, base) with | Some typ, (Var.ProgramVar pv, _) -> (not (Pvar.is_frontend_tmp pv)) - && (not (Procdesc.is_captured_var pdesc pv)) + && (not (Procdesc.is_captured_pvar pdesc pv)) && MaybeUninitVars.mem access_expr maybe_uninit_vars && should_report_on_type typ | _, _ -> diff --git a/infer/tests/codetoanalyze/cpp/ownership/closures.cpp b/infer/tests/codetoanalyze/cpp/ownership/closures.cpp index b64b4b294..d33df48eb 100644 --- a/infer/tests/codetoanalyze/cpp/ownership/closures.cpp +++ b/infer/tests/codetoanalyze/cpp/ownership/closures.cpp @@ -114,7 +114,16 @@ std::function FN_ref_capture_return_lambda_bad() { // stack address } -int ref_capture_return_local_lambda_ok() { +S& lambda_return_local_bad() { + S x; + auto f = [&x](void) -> S& { + S y; + return y; + }; + return f(); +} + +int ref_capture_return_enclosing_local_lambda_ok() { S x; auto f = [&x](void) -> S& { // do not report this because there is a good chance that this function will @@ -124,7 +133,7 @@ int ref_capture_return_local_lambda_ok() { return f().f; } -S& fn_ref_capture_return_local_lambda_bad() { +S& FN_ref_capture_return_enclosing_local_lambda_bad() { S x; auto f = [&x](void) -> S& { // no way to know if ok here diff --git a/infer/tests/codetoanalyze/cpp/ownership/issues.exp b/infer/tests/codetoanalyze/cpp/ownership/issues.exp index d717f2944..1521709d1 100644 --- a/infer/tests/codetoanalyze/cpp/ownership/issues.exp +++ b/infer/tests/codetoanalyze/cpp/ownership/issues.exp @@ -3,6 +3,7 @@ codetoanalyze/cpp/ownership/basics.cpp, multiple_invalidations_loop_bad, 3, USE_ codetoanalyze/cpp/ownership/basics.cpp, multiple_invalidations_loop_bad, 5, USE_AFTER_LIFETIME, no_bucket, ERROR, [End of variable lifetime,Use of invalid variable] codetoanalyze/cpp/ownership/basics.cpp, multiple_invalidations_loop_bad, 8, USE_AFTER_LIFETIME, no_bucket, ERROR, [End of variable lifetime,Use of invalid variable] codetoanalyze/cpp/ownership/closures.cpp, implicit_ref_capture_destroy_invoke_bad, 6, USE_AFTER_LIFETIME, no_bucket, ERROR, [End of variable lifetime,Use of invalid variable] +codetoanalyze/cpp/ownership/closures.cpp, lambda_return_local_bad::lambda_closures.cpp:119:12_operator(), 3, USE_AFTER_LIFETIME, no_bucket, ERROR, [Return of stack variable,End of procedure] codetoanalyze/cpp/ownership/closures.cpp, ref_capture_destroy_invoke_bad, 6, USE_AFTER_LIFETIME, no_bucket, ERROR, [End of variable lifetime,Use of invalid variable] codetoanalyze/cpp/ownership/returns.cpp, returns::return_deleted_bad, 4, USE_AFTER_LIFETIME, no_bucket, ERROR, [End of variable lifetime,Use of invalid variable] codetoanalyze/cpp/ownership/returns.cpp, returns::return_literal_stack_reference_bad, 0, USE_AFTER_LIFETIME, no_bucket, ERROR, [Return of stack variable,End of procedure]