[ownership] warn when returning references to borrowed values

Reviewed By: jeremydubreil

Differential Revision: D7312430

fbshipit-source-id: 636e31b

infer/src/IR/Pvar.ml

@@ -148,6 +148,9 @@ let is_return pv = Mangled.equal (get_name pv) Ident.name_return
 (** something that can't be part of a legal identifier in any conceivable language *)
 let tmp_prefix = "0$?%__sil_tmp"
 
+(** name given to variables representing C++ temporary objects *)
+let materialized_cpp_temporary = "SIL_materialize_temp__"
+
 (** return true if [pvar] is a temporary variable generated by the frontend *)
 let is_frontend_tmp pvar =
   (* Check whether the program variable is a temporary one generated by Sawja, javac, or some other
@@ -176,6 +179,11 @@ let is_ssa_frontend_tmp pvar =
   not (String.contains name '_' && String.contains name '$')
 
 
+let is_cpp_temporary pvar =
+  let name = to_string pvar in
+  String.is_substring ~substring:materialized_cpp_temporary name
+
+
 (** Turn an ordinary program variable into a callee program variable *)
 let to_callee pname pvar =
   match pvar.pv_kind with

infer/src/IR/Pvar.mli

@@ -78,6 +78,9 @@ val is_ssa_frontend_tmp : t -> bool
 (** return true if [pvar] is a temporary variable generated by the frontend and is only assigned
     once on a non-looping control-flow path *)
 
+val is_cpp_temporary : t -> bool
+(** return true if this pvar represents a C++ temporary object (see http://en.cppreference.com/w/cpp/language/lifetime) *)
+
 val mk : Mangled.t -> Typ.Procname.t -> t
 (** [mk name proc_name suffix] creates a program var with the given function name and suffix *)
 
@@ -133,3 +136,5 @@ val get_initializer_pname : t -> Typ.Procname.t option
 val get_name_of_local_with_procname : t -> Mangled.t
 (** [get_name_of_local_with_procname var] Return a name that is composed of the name of
 var and the name of the procname in case of locals *)
+
+val materialized_cpp_temporary : string

infer/src/IR/Typ.ml

@@ -524,6 +524,8 @@ let is_cpp_class = is_class_of_kind Name.Cpp.is_class
 
 let is_pointer typ = match typ.desc with Tptr _ -> true | _ -> false
 
+let is_reference typ = match typ.desc with Tptr (_, Pk_reference) -> true | _ -> false
+
 let is_pointer_to_cpp_class typ = match typ.desc with Tptr (t, _) -> is_cpp_class t | _ -> false
 
 let has_block_prefix s =

infer/src/IR/Typ.mli

@@ -265,6 +265,8 @@ val is_pointer_to_cpp_class : t -> bool
 
 val is_pointer : t -> bool
 
+val is_reference : t -> bool
+
 val has_block_prefix : string -> bool
 
 val unsome : string -> t option -> t

infer/src/IR/Var.ml

@@ -45,6 +45,13 @@ let appears_in_source_code = function
       not (Pvar.is_frontend_tmp pvar)
 
 
+let is_cpp_temporary = function
+  | LogicalVar _ ->
+      false
+  | ProgramVar pvar ->
+      Pvar.is_cpp_temporary pvar
+
+
 let pp fmt = function
   | ProgramVar pv ->
       F.pp_print_string fmt (Pvar.get_simplified_name pv)

infer/src/IR/Var.mli

@@ -36,6 +36,8 @@ val appears_in_source_code : t -> bool
 (** return true if this variable appears in source code (i.e., is not a LogicalVar or a
     frontend-generated ProgramVar) *)
 
+val is_cpp_temporary : t -> bool
+
 val get_footprint_index : t -> int option
 
 val pp : Format.formatter -> t -> unit

infer/src/checkers/Ownership.ml

@@ -94,6 +94,22 @@ let rec is_function_typ = function
 module Domain = struct
   include AbstractDomain.Map (Base) (CapabilityDomain)
 
+  let report message loc ltr summary =
+    let exn = Exceptions.Checkers (IssueType.use_after_lifetime, Localise.verbatim_desc message) in
+    Reporting.log_error summary ~loc ~ltr exn
+
+
+  let report_return_stack_var (var, _) loc summary =
+    let message =
+      F.asprintf "Reference to stack variable %a is returned at %a" Var.pp var Location.pp loc
+    in
+    let ltr =
+      [ Errlog.make_trace_element 0 loc "Return of stack variable" []
+      ; Errlog.make_trace_element 0 loc "End of procedure" [] ]
+    in
+    report message loc ltr summary
+
+
   let report_use_after_lifetime (var, _) ~use_loc ~invalidated_loc summary =
     let message =
       F.asprintf "Variable %a is used at line %a after its lifetime ended at %a" Var.pp var
@@ -103,8 +119,7 @@ module Domain = struct
       [ Errlog.make_trace_element 0 invalidated_loc "End of variable lifetime" []
       ; Errlog.make_trace_element 0 use_loc "Use of invalid variable" [] ]
     in
-    let exn = Exceptions.Checkers (IssueType.use_after_lifetime, Localise.verbatim_desc message) in
+    report message use_loc ltr summary
-    Reporting.log_error summary ~loc:use_loc ~ltr exn
 
 
   (* complain if we do not have the right capability to access [var] *)
@@ -164,22 +179,25 @@ module Domain = struct
     else add lhs_base (CapabilityDomain.make_borrowed_vars rhs_vars) astate
 
 
+  (* copy capability if it exists, consider borrowing if it doesn't *)
+  let copy_or_borrow_var lhs_base rhs_base astate =
+    try
+      let rhs_capability = find rhs_base astate in
+      add lhs_base rhs_capability astate
+    with Caml.Not_found ->
+      if Var.is_cpp_temporary (fst rhs_base) then
+        borrow_vars lhs_base (VarSet.singleton rhs_base) astate
+      else add lhs_base CapabilityDomain.Owned astate
+
+
   (* handle assigning directly to a base var *)
-  let handle_var_assign lhs_base rhs_exp loc summary astate =
+  let handle_var_assign ?(is_operator_equal= false) lhs_base rhs_exp loc summary astate =
-    if Var.is_return (fst lhs_base) then exp_add_reads rhs_exp loc summary astate
-    else
     match rhs_exp with
+    | HilExp.Constant _ when not (Var.is_cpp_temporary (fst lhs_base)) ->
+        add lhs_base CapabilityDomain.Owned astate
     | HilExp.AccessExpression (Base rhs_base | AddressOf (Base rhs_base))
-        when not (Var.appears_in_source_code (fst rhs_base)) -> (
+      when not (Var.appears_in_source_code (fst rhs_base)) ->
-        try
+        copy_or_borrow_var lhs_base rhs_base astate
-          (* assume assignments with non-source vars on the RHS transfer capabilities to the LHS
-             var *)
-          (* copy capability from RHS to LHS *)
-          let base_capability = find rhs_base astate in
-          add lhs_base base_capability astate
-        with Caml.Not_found ->
-          (* no existing capability on RHS. don't make any assumptions about LHS capability *)
-          remove lhs_base astate )
     | HilExp.Closure (_, captured_vars) ->
         (* TODO: can be folded into the case above once we have proper AccessExpressions *)
         let vars_captured_by_ref =
@@ -189,6 +207,11 @@ module Domain = struct
             ~init:VarSet.empty
         in
         borrow_vars lhs_base vars_captured_by_ref astate
+    | HilExp.AccessExpression (Base rhs_base)
+      when not is_operator_equal && Typ.is_reference (snd rhs_base) ->
+        copy_or_borrow_var lhs_base rhs_base astate
+    | HilExp.AccessExpression (AddressOf (Base rhs_base)) when not is_operator_equal ->
+        borrow_vars lhs_base (VarSet.singleton rhs_base) astate
     | _ ->
         (* TODO: support capability transfer between source vars, other assignment modes *)
         exp_add_reads rhs_exp loc summary astate |> remove lhs_base
@@ -234,11 +257,12 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
         else if Typ.Procname.equal pname BuiltinDecl.__placement_new then
           match List.rev actuals with
           | HilExp.AccessExpression (Base placement_base) :: other_actuals ->
-              (* placement new creates an alias between return var and placement var. model as
+              (* TODO: placement new creates an alias between return var and placement var, should
-                 return borrowing from placement *)
+                 eventually model as return borrowing from placement (see
+                 FN_placement_new_aliasing2_bad test) *)
               Domain.actuals_add_reads other_actuals loc summary astate
               |> Domain.add placement_base CapabilityDomain.Owned
-              |> Domain.borrow_vars return_base (VarSet.singleton placement_base) |> Option.some
+              |> Domain.add return_base CapabilityDomain.Owned |> Option.some
           | _ :: other_actuals ->
               Domain.actuals_add_reads other_actuals loc summary astate
               |> Domain.add return_base CapabilityDomain.Owned |> Option.some
@@ -312,7 +336,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
       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_base rhs_exp loc summary astate
+        Domain.handle_var_assign ~is_operator_equal:true lhs_base rhs_exp loc summary astate
     | Call
         ( _
         , Direct (Typ.Procname.ObjC_Cpp callee_pname)
@@ -343,8 +367,35 @@ end
 
 module Analyzer = LowerHil.MakeAbstractInterpreter (ProcCfg.Exceptional) (TransferFunctions)
 
+let report_invalid_return post end_loc formal_map summary =
+  (* 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 )
+            vars
+      | InvalidatedAt invalidated_loc ->
+          Domain.report_use_after_lifetime base ~use_loc:end_loc ~invalidated_loc summary
+      | Owned ->
+          ()
+  in
+  Domain.iter report_invalid_return post
+
+
 let checker {Callbacks.proc_desc; tenv; summary} =
   let proc_data = ProcData.make proc_desc tenv summary in
   let initial = Domain.empty in
-  ignore (Analyzer.compute_post proc_data ~initial) ;
+  ( 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
+  | None ->
+      () ) ;
   summary

infer/src/clang/cTrans.ml

@@ -1433,11 +1433,16 @@ module CTrans_funct (F : CModule_type.CFrontend) : CModule_type.CTranslation = s
             ~f:(function
                 | Clang_ast_t.VarDecl (_, _, qual_type, _) as decl ->
                     let pvar = CVar_decl.sil_var_of_decl context decl procname in
+                    if Pvar.is_static_local pvar then
+                      (* don't call destructors on static vars *)
+                      None
+                    else
                       let exp = Exp.Lvar pvar in
                       let typ = CType_decl.qual_type_to_sil_type context.CContext.tenv qual_type in
                       let this_res_trans_destruct = mk_trans_result (exp, typ) empty_control in
-                    cxx_destructor_call_trans trans_state_pri stmt_info_loc this_res_trans_destruct
+                      cxx_destructor_call_trans trans_state_pri stmt_info_loc
-                      qual_type.Clang_ast_t.qt_type_ptr ~is_inner_destructor:false
+                        this_res_trans_destruct qual_type.Clang_ast_t.qt_type_ptr
+                        ~is_inner_destructor:false
                 | _ ->
                     assert false)
             vars_to_destroy
@@ -2867,7 +2872,8 @@ module CTrans_funct (F : CModule_type.CFrontend) : CModule_type.CTranslation = s
     (* typ_tmp is 'best guess' type of variable - translation may decide to use different type
        later *)
     let pvar, typ_tmp =
-      mk_temp_sil_var_for_expr context.CContext.tenv procdesc "SIL_materialize_temp__" expr_info
+      mk_temp_sil_var_for_expr context.CContext.tenv procdesc Pvar.materialized_cpp_temporary
+        expr_info
     in
     let temp_exp = match stmt_list with [p] -> p | _ -> assert false in
     let var_exp_typ = (Exp.Lvar pvar, typ_tmp) in

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

@@ -4,6 +4,11 @@ codetoanalyze/cpp/ownership/basics.cpp, multiple_invalidations_loop_bad, 5, USE_
 codetoanalyze/cpp/ownership/basics.cpp, multiple_invalidations_loop_bad, 8, USE_AFTER_LIFETIME, ERROR, [End of variable lifetime,Use of invalid variable]
 codetoanalyze/cpp/ownership/closures.cpp, implicit_ref_capture_destroy_invoke_bad, 6, USE_AFTER_LIFETIME, ERROR, [End of variable lifetime,Use of invalid variable]
 codetoanalyze/cpp/ownership/closures.cpp, ref_capture_destroy_invoke_bad, 6, USE_AFTER_LIFETIME, ERROR, [End of variable lifetime,Use of invalid variable]
+codetoanalyze/cpp/ownership/returns.cpp, returns::return_deleted_bad, 4, USE_AFTER_LIFETIME, ERROR, [End of variable lifetime,Use of invalid variable]
+codetoanalyze/cpp/ownership/returns.cpp, returns::return_literal_stack_reference_bad, 0, USE_AFTER_LIFETIME, ERROR, [Return of stack variable,End of procedure]
+codetoanalyze/cpp/ownership/returns.cpp, returns::return_stack_pointer_bad, 3, USE_AFTER_LIFETIME, ERROR, [Return of stack variable,End of procedure]
+codetoanalyze/cpp/ownership/returns.cpp, returns::return_variable_stack_reference1_bad, 3, USE_AFTER_LIFETIME, ERROR, [Return of stack variable,End of procedure]
+codetoanalyze/cpp/ownership/returns.cpp, returns::return_variable_stack_reference2_bad, 4, USE_AFTER_LIFETIME, ERROR, [Return of stack variable,End of procedure]
 codetoanalyze/cpp/ownership/use_after_delete.cpp, delete_in_branch_bad, 5, USE_AFTER_LIFETIME, ERROR, [End of variable lifetime,Use of invalid variable]
 codetoanalyze/cpp/ownership/use_after_delete.cpp, delete_in_loop_bad, 3, USE_AFTER_LIFETIME, ERROR, [End of variable lifetime,Use of invalid variable]
 codetoanalyze/cpp/ownership/use_after_delete.cpp, deref_deleted_bad, 3, USE_AFTER_LIFETIME, ERROR, [End of variable lifetime,Use of invalid variable]
@@ -13,5 +18,4 @@ codetoanalyze/cpp/ownership/use_after_delete.cpp, return_deleted_bad, 3, USE_AFT
 codetoanalyze/cpp/ownership/use_after_delete.cpp, use_in_branch_bad, 4, USE_AFTER_LIFETIME, ERROR, [End of variable lifetime,Use of invalid variable]
 codetoanalyze/cpp/ownership/use_after_delete.cpp, use_in_loop_bad, 4, USE_AFTER_LIFETIME, ERROR, [End of variable lifetime,Use of invalid variable]
 codetoanalyze/cpp/ownership/use_after_destructor.cpp, double_destructor_bad, 5, USE_AFTER_LIFETIME, ERROR, [End of variable lifetime,Use of invalid variable]
-codetoanalyze/cpp/ownership/use_after_destructor.cpp, placement_new_aliasing2_bad, 5, USE_AFTER_LIFETIME, ERROR, [End of variable lifetime,Use of invalid variable]
 codetoanalyze/cpp/ownership/use_after_destructor.cpp, use_after_destructor_bad, 3, USE_AFTER_LIFETIME, ERROR, [End of variable lifetime,Use of invalid variable]

infer/tests/codetoanalyze/cpp/ownership/returns.cpp

@@ -0,0 +1,133 @@
+/*
+ * 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 <string>
+
+namespace returns {
+
+struct S {
+  int f_;
+
+  S(int f) : f_(f) {}
+
+  ~S() {}
+};
+
+const int& return_literal_stack_reference_bad() { return 1; }
+
+const int& return_variable_stack_reference1_bad() {
+  const int& x = 2;
+  return x;
+}
+
+const int& return_variable_stack_reference2_bad() {
+  const int& x = 2;
+  const int& y = x;
+  return y;
+}
+
+const int return_read_of_stack_reference_ok() {
+  const int& x = 2;
+  return x;
+}
+
+const int& return_formal_reference_ok(int& formal) { return formal; }
+
+const int& return_reference_to_formal_pointer_ok(const int* formal) {
+  const int& local = *formal;
+  return local;
+}
+
+extern const int& callee();
+
+const int& return_reference_from_callee_ok() {
+  const int& local = callee();
+  return local;
+}
+
+const int return_int_ok() { return 1; }
+
+const bool return_comparison_temp_ok() { return 1 != 2; }
+
+const bool compare_local_refs_ok() {
+  const int& local1 = 1;
+  const int& local2 = 1;
+  return local1 != local2;
+}
+
+extern int& global;
+
+const int& return_global_reference_ok() { return global; }
+
+struct MemberReference {
+  int& member1;
+
+  int& return_member_reference_ok() { return member1; }
+
+  int* member2;
+  int* return_member_reference_indirect_ok() {
+    int* local = member2;
+    return local;
+  }
+};
+
+extern const char* const kOptions;
+
+const char* return_field_addr_ternary_ok() {
+  const char* const* const t = &kOptions;
+  return t ? *t : "";
+}
+
+int* return_stack_pointer_bad() {
+  int x = 3;
+  return &x;
+}
+
+S* return_static_local_ok() {
+  S* local;
+  static S s{1};
+  local = &s;
+  return local;
+}
+
+S* return_static_local_inner_scope_ok(bool b) {
+  S* local = nullptr;
+  if (b) {
+    static S s{1};
+    local = &s;
+    // destructor for s gets called here, but it shouldn't be
+  }
+  return local;
+}
+
+int* return_formal_pointer_ok(int* formal) { return formal; }
+
+int* return_deleted_bad() {
+  int* x = new int;
+  *x = 2;
+  delete x;
+  return x;
+}
+
+// this *could* be ok depending on what the destructor does, but there's
+// probably no good reason to do it
+S* FN_return_destructed_pointer_bad() {
+  S* s = new S(1);
+  s->~S();
+  return s;
+}
+
+const char* return_nullptr1_ok() { return nullptr; }
+
+const char* return_nullptr2_ok() {
+  const char* local = nullptr;
+  return local;
+}
+
+} // namespace returns

infer/tests/codetoanalyze/cpp/ownership/use_after_delete.cpp

@@ -9,6 +9,7 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 struct S {
   int f;
@@ -91,6 +92,14 @@ void delete_in_loop_ok() {
   }
 }
 
+void delete_ref_in_loop_ok(int j, std::vector<std::string> v) {
+  int i = 0;
+  for (int i = 0; i < 10; i++) {
+    auto s = &v[i];
+    delete s;
+  }
+}
+
 void use_in_loop_bad() {
   auto s = new S{1};
   delete s;

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

@@ -136,7 +136,7 @@ S* FN_placement_new_aliasing1_bad() {
   return s; // bad, returning freed memory
 }
 
-S* placement_new_aliasing2_bad() {
+S* FN_placement_new_aliasing2_bad() {
   S* s = new S(1);
   s->~S();
   auto alias = new (s) S(2);
@@ -154,6 +154,11 @@ void placement_new_non_var_ok() {
   delete m.s;
 }
 
+void return_placement_new_ok() {
+  auto mem = new S(1);
+  return new (mem) S(2);
+}
+
 void destructor_in_loop_ok() {
   for (int i = 0; i < 10; i++) {
     S s(1);