[ownership] assume operator= borrows for function types, copies for other types

Reviewed By: jeremydubreil

Differential Revision: D7178249

fbshipit-source-id: 17c1469

infer/src/checkers/Ownership.ml

@@ -155,7 +155,7 @@ module Domain = struct
 
 
   (* handle assigning directly to a base var *)
-  let handle_var_assign lhs_var rhs_exp loc summary astate =
+  let handle_var_assign (lhs_var, lhs_typ) rhs_exp loc summary astate =
     if Var.is_return lhs_var then exp_add_reads rhs_exp loc summary astate
     else
       match rhs_exp with
@@ -172,7 +172,10 @@ module Domain = struct
         with Not_found ->
           (* no existing capability on RHS. don't make any assumptions about LHS capability *)
           remove lhs_var astate )
-      | HilExp.AccessExpression AccessExpression.Base _ ->
+      | HilExp.AccessExpression AccessExpression.Base (_, rhs_typ)
+        when is_function_typ lhs_typ || is_function_typ rhs_typ ->
+          (* an assignment borrows if the LHS/RHS is a function type, but for now assume that it
+             copies resources correctly for any other type. eventually, we'll check this assumption *)
           borrow_exp lhs_var rhs_exp astate
       | HilExp.Closure (_, captured_vars) ->
           (* TODO: can be folded into the case above once we have proper AccessExpressions *)
@@ -213,8 +216,8 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
   let exec_instr (astate: Domain.astate) (proc_data: extras ProcData.t) _ (instr: HilInstr.t) =
     let summary = proc_data.extras in
     match instr with
-    | Assign (Base (lhs_var, _), rhs_exp, loc) ->
+    | Assign (Base lhs_base, rhs_exp, loc) ->
-        Domain.handle_var_assign lhs_var rhs_exp loc summary astate
+        Domain.handle_var_assign lhs_base rhs_exp loc summary astate
     | Assign (lhs_access_exp, rhs_exp, loc) ->
         (* assign to field, array, indirectly with &/*, or a combination *)
         Domain.exp_add_reads rhs_exp loc summary astate
@@ -244,13 +247,13 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
     | Call
         ( _
         , Direct Typ.Procname.ObjC_Cpp callee_pname
-        , [(AccessExpression Base (lhs_var, _)); rhs_exp]
+        , [(AccessExpression Base lhs_base); 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
+        Domain.handle_var_assign lhs_base rhs_exp loc summary astate
     | Call
         ( _
         , Direct Typ.Procname.ObjC_Cpp callee_pname

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

@@ -32,6 +32,16 @@ int implicit_ref_capture_destroy_invoke_bad() {
   return f();
 }
 
+int reassign_lambda_capture_destroy_invoke_bad() {
+  std::function<int()> f;
+  {
+    auto s = S();
+    auto tmp = [&] { return s.f; };
+    f = tmp;
+  }
+  return f();
+}
+
 // frontend doesn't understand difference between capture-by-value and
 // capture-by-ref, need to fix
 int value_capture_destroy_invoke_ok() {

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

@@ -1,4 +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, reassign_lambda_capture_destroy_invoke_bad, 7, 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]
@@ -10,4 +11,3 @@ codetoanalyze/cpp/ownership/use_after_delete.cpp, use_in_branch_bad, 4, USE_AFTE
 codetoanalyze/cpp/ownership/use_after_delete.cpp, use_in_loop_bad, 4, USE_AFTER_LIFETIME, [Use of invalid variable]
 codetoanalyze/cpp/ownership/use_after_destructor.cpp, double_destructor_bad, 5, USE_AFTER_LIFETIME, [Use of invalid variable]
 codetoanalyze/cpp/ownership/use_after_destructor.cpp, use_after_destructor_bad, 3, USE_AFTER_LIFETIME, [Use of invalid variable]
-codetoanalyze/cpp/ownership/use_after_destructor.cpp, use_after_scope1_bad, 7, USE_AFTER_LIFETIME, [Use of invalid variable]

infer/tests/codetoanalyze/cpp/ownership/use_after_destructor.cpp

@@ -12,65 +12,87 @@
 #include <string>
 
 struct S {
-  int f;
+  int* f;
+  S(int i) {
+    f = new int;
+    *f = i;
+  }
+
+  // missing: operator= to copy the pointer. double delete can happen if
+  // operator= is called
 
-  ~S() {}
+  ~S() { delete f; }
 };
 
 // destructor called at end of function, no issues
-void normal_scope_destructor_ok() { S s{1}; }
+void normal_scope_destructor_ok() { S s(1); }
 
 void nested_scope_destructor_ok() {
-  { S s{1}; }
+  { S s(1); }
 }
 
 int reinit_after_explicit_destructor_ok() {
-  S s{1};
+  S s(1);
   s.~S();
-  s = S{2};
+  s = S(2);
-  return s.f;
+  return *s.f;
 }
 
 void placement_new_explicit_destructor_ok() {
   char buf[sizeof(S)];
   {
-    S* s = new (buf) S;
+    S* s = new (buf) S(1);
     s->~S();
   }
   {
     // this use of [buf] shouldn't be flagged
-    S* s = new (buf) S;
+    S* s = new (buf) S(2);
     s->~S();
   }
 }
 
 void double_destructor_bad() {
-  S s{1};
+  S s(1);
   s.~S();
   // destructor will be called again after S goes out of scope, which is
   // undefined behavior
 }
 
 int use_after_destructor_bad() {
-  S s{1};
+  S s(1);
   s.~S();
-  int ret = s.f;
+  int ret = *s.f;
   s = S{2};
   return ret;
 }
 
-void use_after_scope1_bad() {
+// can't get this yet because we assume operator= copies resources correctly
-  S s;
+// (but this isn't true for S)
+void FN_use_after_scope1_bad() {
+  S s(1);
   {
-    S tmp{1};
+    S tmp(2);
-    s = tmp;
+    s = tmp; // translated as operator=(s, tmp)
   } // destructor for tmp runs here
-  // destructor for s here; second time the value held by s has been desructed
+  // destructor for s here; second time the value held by s has been destructed
 }
 
 void FN_use_after_scope2_bad() {
-  S s;
+  S s(1);
   {
-    s = S{1};
+    s = S(1);
   } // destructor runs here, but our frontend currently doesn't insert it
 }
+
+struct POD {
+  int f;
+};
+
+// this code is ok since double-destructing POD struct is ok
+void destruct_twice_ok() {
+  POD p{1};
+  {
+    POD tmp{2};
+    p = tmp;
+  } // destructor for tmp
+} // destructor for p runs here, but it's harmless