From 5894258f433ebab0f2c4585954f37c70e0603aba Mon Sep 17 00:00:00 2001
From: Jules Villard <jul@fb.com>
Date: Tue, 28 Aug 2018 08:45:36 -0700
Subject: [PATCH] [ownership] do not warn on returning ref to outer local

Summary:
Lambdas can capture references to locals of the enclosing method as long as
they are not propagated outside the method. However to keep things simple
always allow them to capture locals of the enclosing method at the price of
some false negatives.

Reviewed By: da319

Differential Revision: D8974434

fbshipit-source-id: 957ae44bd
---
 infer/src/IR/Var.ml                           |  2 ++
 infer/src/IR/Var.mli                          |  2 ++
 infer/src/checkers/Ownership.ml               | 23 +++++++++++--------
 .../codetoanalyze/cpp/ownership/closures.cpp  | 20 ++++++++++++++++
 4 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/infer/src/IR/Var.ml b/infer/src/IR/Var.ml
index bb079034c..90f427fce 100644
--- a/infer/src/IR/Var.ml
+++ b/infer/src/IR/Var.ml
@@ -30,6 +30,8 @@ let of_formal_index formal_index = of_id (Ident.create_footprint Ident.name_spec
 
 let to_exp = function ProgramVar pvar -> Exp.Lvar pvar | LogicalVar id -> Exp.Var id
 
+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
diff --git a/infer/src/IR/Var.mli b/infer/src/IR/Var.mli
index 6fd30d728..3bddad91f 100644
--- a/infer/src/IR/Var.mli
+++ b/infer/src/IR/Var.mli
@@ -27,6 +27,8 @@ val get_all_vars_in_exp : Exp.t -> t Sequence.t
 
 val to_exp : t -> Exp.t
 
+val get_mangled : t -> Mangled.t option
+
 val is_global : t -> bool
 
 val is_return : t -> bool
diff --git a/infer/src/checkers/Ownership.ml b/infer/src/checkers/Ownership.ml
index 5c81b23de..10da35b88 100644
--- a/infer/src/checkers/Ownership.ml
+++ b/infer/src/checkers/Ownership.ml
@@ -353,7 +353,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
         , _
         , loc )
       when Typ.Procname.ObjC_Cpp.is_cpp_lambda callee_pname ->
-        (* invoking a lambda; check that it's captured vars are valid *)
+        (* invoking a lambda; check that its captured vars are valid *)
         Domain.check_var_lifetime lhs_base loc summary astate ;
         astate
     | Call (ret_id_typ, call_exp, actuals, _, loc) -> (
@@ -376,18 +376,24 @@ end
 
 module Analyzer = LowerHil.MakeAbstractInterpreter (ProcCfg.Exceptional) (TransferFunctions)
 
-let report_invalid_return post end_loc formal_map summary =
+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 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
   (* look for return values that are borrowed from (now-invalid) local variables *)
   let report_invalid_return base (capability : CapabilityDomain.astate) =
     if Var.is_return (fst base) then
       match capability with
       | BorrowedFrom vars ->
           VarSet.iter
-            (fun borrowed_base ->
-              if
-                (not (FormalMap.is_formal borrowed_base formal_map))
-                && not (Var.is_global (fst borrowed_base))
-              then Domain.report_return_stack_var borrowed_base end_loc summary )
+            (fun ((var, _) as borrowed_base) ->
+              if is_local_to_procedure var then
+                Domain.report_return_stack_var borrowed_base end_loc summary )
             vars
       | InvalidatedAt invalidated_loc ->
           Domain.report_use_after_lifetime base ~use_loc:end_loc ~invalidated_loc summary
@@ -403,8 +409,7 @@ let checker {Callbacks.proc_desc; tenv; summary} =
   ( match Analyzer.compute_post proc_data ~initial with
   | Some post ->
       let end_loc = Procdesc.Node.get_loc (Procdesc.get_exit_node proc_desc) in
-      let formal_map = FormalMap.make proc_desc in
-      report_invalid_return post end_loc formal_map summary
+      report_invalid_return post end_loc summary
   | None ->
       () ) ;
   summary
diff --git a/infer/tests/codetoanalyze/cpp/ownership/closures.cpp b/infer/tests/codetoanalyze/cpp/ownership/closures.cpp
index 22fde6185..b64b4b294 100644
--- a/infer/tests/codetoanalyze/cpp/ownership/closures.cpp
+++ b/infer/tests/codetoanalyze/cpp/ownership/closures.cpp
@@ -113,3 +113,23 @@ std::function<int()> FN_ref_capture_return_lambda_bad() {
   return f; // if the caller invokes the lambda, it will try to read the invalid
             // stack address
 }
+
+int ref_capture_return_local_lambda_ok() {
+  S x;
+  auto f = [&x](void) -> S& {
+    // do not report this because there is a good chance that this function will
+    // only be used in the local scope
+    return x;
+  };
+  return f().f;
+}
+
+S& fn_ref_capture_return_local_lambda_bad() {
+  S x;
+  auto f = [&x](void) -> S& {
+    // no way to know if ok here
+    return x;
+  };
+  // woops, this returns a ref to a local!
+  return f();
+}