[ownership] report invocation of lambdas capturing a var whose lifetime is over

Summary: If a `Closure` expression `e` captures variable `x`, consider `e` as borrowing from `x`. When the closure is invoked via `operator()`, check that the borrow is still valid.

Reviewed By: jeremydubreil

Differential Revision: D7071839

fbshipit-source-id: d923a6a

infer/src/IR/HilExp.ml

@@ -140,7 +140,18 @@ let of_sil ~include_array_indexes ~f_resolve_id exp typ =
     | Closure closure ->
         let environment =
           List.map
-            ~f:(fun (value, pvar, typ) -> (AccessPath.base_of_pvar pvar typ, of_sil_ value typ))
+            ~f:(fun (value, pvar, typ) ->
+              (* TODO: this is a hack than can disappear once we have proper AccessExpressions (t23176430) *)
+              let typ' =
+                match value with
+                | Exp.Lvar pvar1 when Pvar.equal pvar1 pvar ->
+                    (* captured by reference, change type to pointer *)
+                    {typ with Typ.desc= Typ.Tptr (typ, Typ.Pk_pointer)}
+                | _ ->
+                    (* capture by value *)
+                    typ
+              in
+              (AccessPath.base_of_pvar pvar typ', of_sil_ value typ') )
             closure.captured_vars
         in
         Closure (closure.name, environment)

infer/src/IR/Typ.ml

@@ -756,6 +756,8 @@ module Procname = struct
 
     let is_cpp_lambda {method_name} = String.is_substring ~substring:"operator()" method_name
 
+    let is_operator_equal {method_name} = String.is_substring ~substring:"operator=" method_name
+
     let kind_to_verbose_string = function
       | CPPMethod m | CPPDestructor m ->
           "(" ^ (match m with None -> "" | Some s -> s) ^ ")"

infer/src/IR/Typ.mli

@@ -382,6 +382,9 @@ module Procname : sig
 
     val is_cpp_lambda : t -> bool
     (** Return whether the procname is a cpp lambda. *)
+
+    val is_operator_equal : t -> bool
+    (** Return true if the procname is operator= *)
   end
 
   (** Type of c procedure names. *)

infer/src/checkers/Ownership.ml

@@ -63,6 +63,15 @@ module CapabilityDomain = struct
         F.fprintf fmt "Owned"
 end
 
+let rec is_function_typ = function
+  | {Typ.desc= Tptr (t, _)} ->
+      is_function_typ t
+  | {Typ.desc= Tstruct typename} ->
+      String.is_prefix ~prefix:"std::function" (Typ.Name.name typename)
+  | _ ->
+      false
+
+
 (** map from program variable to its capability *)
 module Domain = struct
   include AbstractDomain.Map (Var) (CapabilityDomain)
@@ -103,11 +112,16 @@ module Domain = struct
         ()
 
 
-  let access_path_add_read ((base_var, _), _) loc summary astate =
+  let base_add_read (base_var, typ) loc summary astate =
-    check_var_lifetime base_var loc summary astate ;
+    (* don't warn if it's a read of a std::function type. we model closures as borrowing their
+       captured vars, but simply reading a closure doesn't actually use these vars. this means that
+       we'll miss bugs involving invalid uses of std::function's, but that seems ok *)
+    if not (is_function_typ typ) then check_var_lifetime base_var loc summary astate ;
     astate
 
 
+  let access_path_add_read (base, _) loc summary astate = base_add_read base loc summary astate
+
   let exp_add_reads exp loc summary astate =
     List.fold
       ~f:(fun acc access_path -> access_path_add_read access_path loc summary acc)
@@ -122,15 +136,19 @@ module Domain = struct
       ~init:astate
 
 
-  let borrow lhs_var rhs_exp astate =
+  let borrow_vars lhs_var rhs_vars astate =
+    (* borrow all of the vars read on the rhs *)
+    if VarSet.is_empty rhs_vars then remove lhs_var astate
+    else add lhs_var (CapabilityDomain.make_borrowed_vars rhs_vars) astate
+
+
+  let borrow_exp lhs_var rhs_exp astate =
     let rhs_vars =
       List.fold (HilExp.get_access_paths rhs_exp)
         ~f:(fun acc ((var, _), _) -> VarSet.add var acc)
         ~init:VarSet.empty
     in
-    (* borrow all of the vars read on the rhs *)
+    borrow_vars lhs_var rhs_vars astate
-    if VarSet.is_empty rhs_vars then remove lhs_var astate
-    else add lhs_var (CapabilityDomain.make_borrowed_vars rhs_vars) astate
 
 
   (* handle assigning directly to a base var *)
@@ -139,7 +157,7 @@ module Domain = struct
     else
       match rhs_exp with
       | HilExp.AccessExpression AccessExpression.AddressOf address_taken_exp ->
-          borrow lhs_var (AccessExpression address_taken_exp) astate
+          borrow_exp lhs_var (AccessExpression address_taken_exp) astate
       | HilExp.AccessExpression AccessExpression.Base (rhs_var, _)
         when not (Var.appears_in_source_code rhs_var) -> (
         try
@@ -152,7 +170,16 @@ module Domain = struct
           (* no existing capability on RHS. don't make any assumptions about LHS capability *)
           remove lhs_var astate )
       | HilExp.AccessExpression AccessExpression.Base _ ->
-          borrow lhs_var rhs_exp astate
+          borrow_exp lhs_var rhs_exp astate
+      | HilExp.Closure (_, captured_vars) ->
+          (* TODO: can be folded into the case above once we have proper AccessExpressions *)
+          let vars_captured_by_ref =
+            List.fold captured_vars
+              ~f:(fun acc ((var, typ), _) ->
+                match typ.Typ.desc with Typ.Tptr _ -> VarSet.add var acc | _ -> acc )
+              ~init:VarSet.empty
+          in
+          borrow_vars lhs_var vars_captured_by_ref astate
       | _ ->
           (* TODO: support capability transfer between source vars, other assignment modes *)
           exp_add_reads rhs_exp loc summary astate |> remove lhs_var
@@ -207,13 +234,30 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
             Domain.add constructed_var CapabilityDomain.Owned astate'
         | _ ->
             astate' )
-    | Call (_, Direct callee_pname, [(AccessExpression Base (lhs_var, _))], _, loc)
+    | Call (_, Direct callee_pname, [(AccessExpression Base ((lhs_var, _) as lhs_base))], _, loc)
       when transfers_ownership callee_pname ->
-        Domain.check_var_lifetime lhs_var loc summary astate ;
+        Domain.base_add_read lhs_base loc summary astate
-        Domain.add lhs_var CapabilityDomain.Invalid astate
+        |> Domain.add lhs_var CapabilityDomain.Invalid
-    | Call (_, Direct callee_pname, [(AccessExpression Base (lhs_var, _)); rhs_exp], _, loc)
+    | Call
-      when String.equal (Typ.Procname.get_method callee_pname) "operator=" ->
+        ( _
+        , Direct Typ.Procname.ObjC_Cpp callee_pname
+        , [(AccessExpression Base (lhs_var, _)); rhs_exp]
+        , _
+        , loc )
+      when Typ.Procname.ObjC_Cpp.is_operator_equal callee_pname ->
+        (* TODO: once we go interprocedural, this case should only apply for operator='s with an
+           empty summary *)
         Domain.handle_var_assign lhs_var rhs_exp loc summary astate
+    | Call
+        ( _
+        , Direct Typ.Procname.ObjC_Cpp callee_pname
+        , (AccessExpression Base (lhs_var, _)) :: _
+        , _
+        , loc )
+      when Typ.Procname.ObjC_Cpp.is_cpp_lambda callee_pname ->
+        (* invoking a lambda; check that it's captured vars are valid *)
+        Domain.check_var_lifetime lhs_var loc summary astate ;
+        astate
     | Call (ret_opt, _, actuals, _, loc)
       -> (
         let astate' = Domain.actuals_add_reads actuals loc summary astate in

infer/tests/codetoanalyze/cpp/ownership/closures.cpp

@@ -0,0 +1,107 @@
+/*
+ * Copyright (c) 2018 - present Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+#include <functional>
+
+struct S {
+  int f;
+
+  S() { f = 1; }
+};
+
+int ref_capture_destroy_invoke_bad() {
+  std::function<int()> f;
+  {
+    S s;
+    f = [&s] { return s.f; };
+  } // destructor for s called here
+  return f(); // s used here
+}
+
+int implicit_ref_capture_destroy_invoke_bad() {
+  std::function<int()> f;
+  {
+    auto s = S();
+    f = [&] { return s.f; };
+  }
+  return f();
+}
+
+// frontend doesn't understand difference between capture-by-value and
+// capture-by-ref, need to fix
+int value_capture_destroy_invoke_ok() {
+  std::function<int()> f;
+  {
+    S s;
+    f = [s] { return s.f; };
+  }
+  return f();
+}
+
+// same thing here
+int implicit_value_capture_destroy_invoke_ok() {
+  std::function<int()> f;
+  {
+    S s;
+    f = [=] { return s.f; };
+  }
+  return f();
+}
+
+int ref_capture_invoke_ok() {
+  std::function<int()> f;
+  int ret;
+  {
+    S s;
+    f = [&s] { return s.f; };
+    ret = f();
+  }
+  return ret;
+}
+
+void invoke_twice_ok() {
+  std::function<int()> f;
+  int ret;
+  {
+    S s;
+    f = [&s] { return s.f; };
+    f();
+    f();
+  }
+}
+
+std::function<int()> ref_capture_read_lambda_ok() {
+  std::function<int()> f;
+  int ret;
+  {
+    S s;
+    f = [&s] { return s.f; };
+  }
+  auto tmp =
+      f; // reading (but not invoking) the lambda doesn't use its captured vars
+}
+
+// we'll miss this because we count invoking a lambda object as a use of its
+// captured vars, not the lambda object itself.
+void FN_delete_lambda_then_call_bad() {
+  auto lambda = [] { return 1; };
+  ~lambda();
+  return lambda();
+}
+
+// need to treat escaping as a use in order to catch this
+std::function<int()> FN_ref_capture_return_lambda_bad() {
+  std::function<int()> f;
+  int ret;
+  {
+    S s;
+    f = [&s] { return s.f; };
+  }
+  return f; // if the caller invokes the lambda, it will try to read the invalid
+            // stack address
+}

infer/tests/codetoanalyze/cpp/ownership/issues.exp

@@ -1,3 +1,5 @@
+codetoanalyze/cpp/ownership/closures.cpp, implicit_ref_capture_destroy_invoke_bad, 6, USE_AFTER_LIFETIME, [Use of invalid variable]
+codetoanalyze/cpp/ownership/closures.cpp, ref_capture_destroy_invoke_bad, 6, USE_AFTER_LIFETIME, [Use of invalid variable]
 codetoanalyze/cpp/ownership/use_after_delete.cpp, delete_in_branch_bad, 5, USE_AFTER_LIFETIME, [Use of invalid variable]
 codetoanalyze/cpp/ownership/use_after_delete.cpp, delete_in_loop_bad, 3, USE_AFTER_LIFETIME, [Use of invalid variable]
 codetoanalyze/cpp/ownership/use_after_delete.cpp, deref_deleted_bad, 3, USE_AFTER_LIFETIME, [Use of invalid variable]