[pulse] detect stack variable address escape

Summary:
When assign to the special `return` variable, check that the result is
not the address of a local variable, otherwise report.

Reviewed By: ngorogiannis

Differential Revision: D13466896

fbshipit-source-id: 465da7f13

infer/src/checkers/Pulse.ml

@@ -15,8 +15,8 @@ let report summary diagnostic =
 
 
 let check_error summary = function
-  | Ok astate ->
+  | Ok ok ->
-      astate
+      ok
   | Error diagnostic ->
       report summary diagnostic ;
       (* We can also continue the analysis by returning {!PulseDomain.initial} here but there might
@@ -41,22 +41,31 @@ module PulseTransferFunctions (CFG : ProcCfg.S) = struct
         false
 
 
-  let rec exec_assign lhs_access (rhs_exp : HilExp.t) loc astate =
+  let rec exec_assign summary (lhs_access : HilExp.AccessExpression.t) (rhs_exp : HilExp.t) loc
+      astate =
     (* try to evaluate [rhs_exp] down to an abstract address or generate a fresh one *)
     match rhs_exp with
-    | AccessExpression rhs_access ->
+    | AccessExpression rhs_access -> (
         PulseDomain.read loc rhs_access astate
-        >>= fun (astate, rhs_value) -> PulseDomain.write loc lhs_access rhs_value astate
+        >>= fun (astate, rhs_value) ->
+        PulseDomain.write loc lhs_access rhs_value astate
+        >>= fun astate ->
+        match lhs_access with
+        | Base (var, _) when Var.is_return var ->
+            PulseDomain.check_address_of_local_variable summary.Summary.proc_desc rhs_value astate
+        | _ ->
+            Ok astate )
     | Closure (pname, captured) ->
         PulseDomain.Closures.record loc lhs_access pname captured astate
     | Cast (_, e) ->
-        exec_assign lhs_access e loc astate
+        exec_assign summary lhs_access e loc astate
     | _ ->
         PulseDomain.read_all loc (HilExp.get_access_exprs rhs_exp) astate
         >>= PulseDomain.havoc loc lhs_access
 
 
-  let exec_call _ret (call : HilInstr.call) (actuals : HilExp.t list) _flags call_loc astate =
+  let exec_call summary _ret (call : HilInstr.call) (actuals : HilExp.t list) _flags call_loc
+      astate =
     let read_all args astate =
       PulseDomain.read_all call_loc (List.concat_map args ~f:HilExp.get_access_exprs) astate
     in
@@ -87,7 +96,7 @@ module PulseTransferFunctions (CFG : ProcCfg.S) = struct
       | [AccessExpression lhs; (HilExp.AccessExpression (AddressOf (Base rhs_base)) as rhs_exp)]
         when Var.is_cpp_temporary (fst rhs_base) ->
           let lhs_deref = HilExp.AccessExpression.dereference lhs in
-          exec_assign lhs_deref rhs_exp call_loc astate
+          exec_assign summary lhs_deref rhs_exp call_loc astate
       (* copy assignment *)
       | [AccessExpression lhs; HilExp.AccessExpression rhs] ->
           let lhs_deref = HilExp.AccessExpression.dereference lhs in
@@ -100,7 +109,8 @@ module PulseTransferFunctions (CFG : ProcCfg.S) = struct
         read_all actuals astate
 
 
-  let dispatch_call ret (call : HilInstr.call) (actuals : HilExp.t list) flags call_loc astate =
+  let dispatch_call summary ret (call : HilInstr.call) (actuals : HilExp.t list) flags call_loc
+      astate =
     let model =
       match call with
       | Indirect _ ->
@@ -111,7 +121,8 @@ module PulseTransferFunctions (CFG : ProcCfg.S) = struct
     in
     match model with
     | None ->
-        exec_call ret call actuals flags call_loc astate >>| PulseDomain.havoc_var (fst ret)
+        exec_call summary ret call actuals flags call_loc astate
+        >>| PulseDomain.havoc_var (fst ret)
     | Some model ->
         model call_loc ~ret ~actuals astate
 
@@ -120,11 +131,11 @@ module PulseTransferFunctions (CFG : ProcCfg.S) = struct
       =
     match instr with
     | Assign (lhs_access, rhs_exp, loc) ->
-        exec_assign lhs_access rhs_exp loc astate |> check_error summary
+        exec_assign summary lhs_access rhs_exp loc astate |> check_error summary
     | Assume (condition, _, _, loc) ->
         PulseDomain.read_all loc (HilExp.get_access_exprs condition) astate |> check_error summary
     | Call (ret, call, actuals, flags, loc) ->
-        dispatch_call ret call actuals flags loc astate |> check_error summary
+        dispatch_call summary ret call actuals flags loc astate |> check_error summary
     | ExitScope vars ->
         PulseDomain.remove_vars vars astate
 

infer/src/checkers/PulseDomain.ml

@@ -489,42 +489,61 @@ module Diagnostic = struct
         ; invalidated_by: Invalidation.t
         ; accessed_by: actor
         ; address: AbstractAddress.t }
+    | StackVariableAddressEscape of {variable: Var.t; location: Location.t}
 
-  let get_location (AccessToInvalidAddress {accessed_by= {location}}) = location
+  let get_location = function
-
+    | AccessToInvalidAddress {accessed_by= {location}} | StackVariableAddressEscape {location} ->
-  let get_message (AccessToInvalidAddress {allocated_by; accessed_by; invalidated_by; address}) =
+        location
-    let pp_debug_address f =
-      if Config.debug_mode then F.fprintf f " (debug: %a)" AbstractAddress.pp address
-    in
-    F.asprintf "`%a` accesses address %a%a past its lifetime%t" HilExp.AccessExpression.pp
-      accessed_by.access_expr pp_allocator allocated_by Invalidation.pp invalidated_by
-      pp_debug_address
 
 
-  let get_trace (AccessToInvalidAddress {allocated_by; accessed_by; invalidated_by}) =
+  let get_message = function
-    let allocated_by_trace =
+    | AccessToInvalidAddress {allocated_by; accessed_by; invalidated_by; address} ->
-      match location_of_allocator allocated_by with
+        let pp_debug_address f =
-      | None ->
+          if Config.debug_mode then F.fprintf f " (debug: %a)" AbstractAddress.pp address
-          []
+        in
-      | Some location ->
+        F.asprintf "`%a` accesses address %a%a past its lifetime%t" HilExp.AccessExpression.pp
-          [Errlog.make_trace_element 0 location (F.asprintf "%ahere" pp_allocator allocated_by) []]
+          accessed_by.access_expr pp_allocator allocated_by Invalidation.pp invalidated_by
-    in
+          pp_debug_address
-    let invalidated_by_trace =
+    | StackVariableAddressEscape {variable} ->
-      Invalidation.get_location invalidated_by
+        let pp_var f var =
-      |> Option.map ~f:(fun location ->
+          if Var.is_cpp_temporary var then F.pp_print_string f "C++ temporary"
-             Errlog.make_trace_element 0 location
+          else F.fprintf f "stack variable `%a`" Var.pp var
-               (F.asprintf "%a here" Invalidation.pp invalidated_by)
+        in
-               [] )
+        F.asprintf "address of %a is returned by the function" pp_var variable
-      |> Option.to_list
-    in
-    allocated_by_trace @ invalidated_by_trace
-    @ [ Errlog.make_trace_element 0 accessed_by.location
-          (F.asprintf "accessed `%a` here" HilExp.AccessExpression.pp accessed_by.access_expr)
-          [] ]
 
 
-  let get_issue_type (AccessToInvalidAddress {invalidated_by}) =
+  let get_trace = function
-    Invalidation.issue_type_of_cause invalidated_by
+    | AccessToInvalidAddress {allocated_by; accessed_by; invalidated_by} ->
+        let allocated_by_trace =
+          match location_of_allocator allocated_by with
+          | None ->
+              []
+          | Some location ->
+              [ Errlog.make_trace_element 0 location
+                  (F.asprintf "%ahere" pp_allocator allocated_by)
+                  [] ]
+        in
+        let invalidated_by_trace =
+          Invalidation.get_location invalidated_by
+          |> Option.map ~f:(fun location ->
+                 Errlog.make_trace_element 0 location
+                   (F.asprintf "%a here" Invalidation.pp invalidated_by)
+                   [] )
+          |> Option.to_list
+        in
+        allocated_by_trace @ invalidated_by_trace
+        @ [ Errlog.make_trace_element 0 accessed_by.location
+              (F.asprintf "accessed `%a` here" HilExp.AccessExpression.pp accessed_by.access_expr)
+              [] ]
+    | StackVariableAddressEscape _ ->
+        []
+
+
+  let get_issue_type = function
+    | AccessToInvalidAddress {invalidated_by} ->
+        Invalidation.issue_type_of_cause invalidated_by
+    | StackVariableAddressEscape _ ->
+        IssueType.stack_variable_address_escape
 end
 
 type 'a access_result = ('a, Diagnostic.t) result
@@ -741,6 +760,23 @@ module Operations = struct
           edges astate
 
 
+  let check_address_of_local_variable proc_desc address astate =
+    let proc_location = Procdesc.get_loc proc_desc in
+    let proc_name = Procdesc.get_proc_name proc_desc in
+    let check_address_of_stack_variable () =
+      Container.fold_result ~fold:(IContainer.fold_of_pervasives_map_fold ~fold:Stack.fold)
+        astate.stack ~init:() ~f:(fun () (variable, var_address) ->
+          if
+            AbstractAddress.equal var_address address
+            && ( Var.is_cpp_temporary variable
+               || Var.is_local_to_procedure proc_name variable
+                  && not (Procdesc.is_captured_var proc_desc variable) )
+          then Error (Diagnostic.StackVariableAddressEscape {variable; location= proc_location})
+          else Ok () )
+    in
+    check_address_of_stack_variable () >>| fun () -> astate
+
+
   let remove_vars vars astate =
     let stack = List.fold ~f:(fun var stack -> Stack.remove stack var) ~init:astate.stack vars in
     if phys_equal stack astate.stack then astate else {astate with stack}

infer/src/checkers/PulseDomain.mli

@@ -73,3 +73,5 @@ val invalidate_array_elements :
   PulseInvalidation.t -> Location.t -> HilExp.AccessExpression.t -> t -> t access_result
 
 val remove_vars : Var.t list -> t -> t
+
+val check_address_of_local_variable : Procdesc.t -> AbstractAddress.t -> t -> t access_result

infer/src/istd/IContainer.ml

@@ -74,7 +74,11 @@ let filter ~fold ~filter t ~init ~f =
 let map ~f:g fold t ~init ~f = fold t ~init ~f:(fun acc item -> f acc (g item))
 
 let fold_of_pervasives_fold ~fold collection ~init ~f =
-  fold (fun accum item -> f item accum) collection init
+  fold (fun item accum -> f accum item) collection init
+
+
+let fold_of_pervasives_map_fold ~fold collection ~init ~f =
+  fold (fun item value accum -> f accum (item, value)) collection init
 
 
 let iter_result ~fold collection ~f =

infer/src/istd/IContainer.mli

@@ -51,5 +51,9 @@ val map : f:('a -> 'b) -> ('t, 'a, 'accum) Container.fold -> ('t, 'b, 'accum) Co
 val fold_of_pervasives_fold :
   fold:(('a -> 'accum -> 'accum) -> 't -> 'accum -> 'accum) -> ('t, 'a, 'accum) Container.fold
 
+val fold_of_pervasives_map_fold :
+     fold:(('key -> 'value -> 'accum -> 'accum) -> 't -> 'accum -> 'accum)
+  -> ('t, 'key * 'value, 'accum) Container.fold
+
 val iter_result :
   fold:('t, 'a, unit) Container.fold -> 't -> f:('a -> (unit, 'err) result) -> (unit, 'err) result

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

@@ -4,6 +4,8 @@ codetoanalyze/cpp/pulse/closures.cpp, implicit_ref_capture_destroy_invoke_bad, 6
 codetoanalyze/cpp/pulse/closures.cpp, ref_capture_destroy_invoke_bad, 6, USE_AFTER_DESTRUCTOR, no_bucket, ERROR, [`s` captured by `&(f)` as `s` here,invalidated by destructor call `S_~S(s)` at line 20, column 3 here,accessed `&(f)` here]
 codetoanalyze/cpp/pulse/join.cpp, invalidate_node_alias_bad, 12, USE_AFTER_DELETE, no_bucket, ERROR, [invalidated by call to `delete result` at line 25, column 5 here,accessed `*(result)` here]
 codetoanalyze/cpp/pulse/returns.cpp, returns::return_deleted_bad, 4, USE_AFTER_DELETE, no_bucket, ERROR, [invalidated by call to `delete x` at line 112, column 3 here,accessed `x` here]
+codetoanalyze/cpp/pulse/returns.cpp, returns::return_literal_stack_reference_bad, 0, STACK_VARIABLE_ADDRESS_ESCAPE, no_bucket, ERROR, []
+codetoanalyze/cpp/pulse/returns.cpp, returns::return_stack_pointer_bad, 0, STACK_VARIABLE_ADDRESS_ESCAPE, no_bucket, ERROR, []
 codetoanalyze/cpp/pulse/use_after_delete.cpp, delete_in_branch_bad, 5, USE_AFTER_DELETE, no_bucket, ERROR, [invalidated by call to `delete s` at line 57, column 5 here,accessed `s` here]
 codetoanalyze/cpp/pulse/use_after_delete.cpp, delete_in_loop_bad, 3, USE_AFTER_DELETE, no_bucket, ERROR, [invalidated by call to `delete s` at line 82, column 5 here,accessed `s` here]
 codetoanalyze/cpp/pulse/use_after_delete.cpp, deref_deleted_bad, 3, USE_AFTER_DELETE, no_bucket, ERROR, [invalidated by call to `delete s` at line 18, column 3 here,accessed `s` here]

infer/tests/codetoanalyze/cpp/pulse/returns.cpp

@@ -17,7 +17,7 @@ struct S {
   ~S() {}
 };
 
-const int& FN_return_literal_stack_reference_bad() { return 1; }
+const int& return_literal_stack_reference_bad() { return 1; }
 
 const int& FN_return_variable_stack_reference1_bad() {
   const int& x = 2;
@@ -82,7 +82,7 @@ const char* return_field_addr_ternary_ok() {
   return t ? *t : "";
 }
 
-int* FN_return_stack_pointer_bad() {
+int* return_stack_pointer_bad() {
   int x = 3;
   return &x;
 }