diff --git a/infer/src/checkers/Pulse.ml b/infer/src/checkers/Pulse.ml index 3df62c286..149c6bdf4 100644 --- a/infer/src/checkers/Pulse.ml +++ b/infer/src/checkers/Pulse.ml @@ -15,8 +15,8 @@ let report summary diagnostic = let check_error summary = function - | Ok astate -> - astate + | Ok ok -> + 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 diff --git a/infer/src/checkers/PulseDomain.ml b/infer/src/checkers/PulseDomain.ml index 9179173c0..fa02e98b2 100644 --- a/infer/src/checkers/PulseDomain.ml +++ b/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_message (AccessToInvalidAddress {allocated_by; accessed_by; invalidated_by; address}) = - 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_location = function + | AccessToInvalidAddress {accessed_by= {location}} | StackVariableAddressEscape {location} -> + location - let get_trace (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) - [] ] + let get_message = function + | AccessToInvalidAddress {allocated_by; accessed_by; invalidated_by; address} -> + 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 + | StackVariableAddressEscape {variable} -> + let pp_var f var = + if Var.is_cpp_temporary var then F.pp_print_string f "C++ temporary" + else F.fprintf f "stack variable `%a`" Var.pp var + in + F.asprintf "address of %a is returned by the function" pp_var variable - let get_issue_type (AccessToInvalidAddress {invalidated_by}) = - Invalidation.issue_type_of_cause invalidated_by + let get_trace = function + | 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} diff --git a/infer/src/checkers/PulseDomain.mli b/infer/src/checkers/PulseDomain.mli index 306515dbc..acca1965e 100644 --- a/infer/src/checkers/PulseDomain.mli +++ b/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 diff --git a/infer/src/istd/IContainer.ml b/infer/src/istd/IContainer.ml index 103e8c9ed..14f37901e 100644 --- a/infer/src/istd/IContainer.ml +++ b/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 = diff --git a/infer/src/istd/IContainer.mli b/infer/src/istd/IContainer.mli index efdd13236..817b82971 100644 --- a/infer/src/istd/IContainer.mli +++ b/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 diff --git a/infer/tests/codetoanalyze/cpp/pulse/issues.exp b/infer/tests/codetoanalyze/cpp/pulse/issues.exp index 3dd35038b..e92d93130 100644 --- a/infer/tests/codetoanalyze/cpp/pulse/issues.exp +++ b/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] diff --git a/infer/tests/codetoanalyze/cpp/pulse/returns.cpp b/infer/tests/codetoanalyze/cpp/pulse/returns.cpp index 6df0fdbe9..ff763b6d7 100644 --- a/infer/tests/codetoanalyze/cpp/pulse/returns.cpp +++ b/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; }