From d57ed5086e8e9bdafc53dc3aba91d6fee9a6e88f Mon Sep 17 00:00:00 2001 From: Jules Villard Date: Thu, 11 Apr 2019 03:55:02 -0700 Subject: [PATCH] [pulse] better treatment of variables going out of scope Summary: Detect when a variable goes out of scope. When that's the case, mark its address *and* its contents as invalid. Give subsequent uses a USE_AFTER_LIFETIME error type instead of USE_AFTER_DESTRUCTOR. Reviewed By: jberdine Differential Revision: D14387147 fbshipit-source-id: a2c530fda --- infer/src/base/IssueType.ml | 2 - infer/src/base/IssueType.mli | 2 - infer/src/pulse/Pulse.ml | 48 ++++++++----------- infer/src/pulse/PulseInvalidation.ml | 11 ++--- infer/src/pulse/PulseInvalidation.mli | 2 +- .../codetoanalyze/cpp/pulse/closures.cpp | 2 +- .../tests/codetoanalyze/cpp/pulse/issues.exp | 10 ++-- .../cpp/pulse/reference_wrapper.cpp | 2 +- 8 files changed, 35 insertions(+), 44 deletions(-) diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index a28ed8e01..a4632222f 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -417,8 +417,6 @@ let unsafe_guarded_by_access = from_string "UNSAFE_GUARDED_BY_ACCESS" let use_after_delete = from_string "USE_AFTER_DELETE" -let use_after_destructor = from_string "USE_AFTER_DESTRUCTOR" - let use_after_free = from_string "USE_AFTER_FREE" let use_after_lifetime = from_string "USE_AFTER_LIFETIME" diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index dfec9040e..3225f0190 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -311,8 +311,6 @@ val unsafe_guarded_by_access : t val use_after_delete : t -val use_after_destructor : t - val use_after_free : t val use_after_lifetime : t diff --git a/infer/src/pulse/Pulse.ml b/infer/src/pulse/Pulse.ml index bd8c2e614..dff7e96ca 100644 --- a/infer/src/pulse/Pulse.ml +++ b/infer/src/pulse/Pulse.ml @@ -51,17 +51,6 @@ module PulseTransferFunctions = struct type extras = Summary.t - let is_destructor = function - | Typ.Procname.ObjC_Cpp clang_pname -> - Typ.Procname.ObjC_Cpp.is_destructor clang_pname - && not - (* Our frontend generates synthetic inner destructors to model invocation of base class - destructors correctly; see D5834555/D7189239 *) - (Typ.Procname.ObjC_Cpp.is_inner_destructor clang_pname) - | _ -> - false - - 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 *) @@ -150,27 +139,31 @@ module PulseTransferFunctions = struct (** has an object just gone out of scope? *) - let get_destructed_object (call : HilInstr.call) (actuals : HilExp.t list) (flags : CallFlags.t) - = + let get_out_of_scope_object (call : HilInstr.call) (actuals : HilExp.t list) + (flags : CallFlags.t) = (* injected destructors are precisely inserted where an object goes out of scope *) if flags.cf_injected_destructor then match (call, actuals) with - | Direct callee_pname, [AccessExpression destroyed_access] when is_destructor callee_pname -> - Some (callee_pname, destroyed_access) + | ( Direct (Typ.Procname.ObjC_Cpp pname) + , [AccessExpression (*AddressOf (Base _) as *) destroyed_access] ) + when not (Typ.Procname.ObjC_Cpp.is_inner_destructor pname) -> + (* ignore inner destructors, only trigger out of scope on the final destructor call *) + Some destroyed_access | _ -> None else None - (** [destroyed_access_expr] has just gone out of scope and in now invalid *) - let destruct_object callee_pname call_loc destroyed_access_expr astate = - let destroyed_object = HilExp.AccessExpression.dereference destroyed_access_expr in - (* TODO: need an [OutOfScope] invalidation reason instead of [CppDestructor]? *) - PulseOperations.invalidate - (PulseTrace.Immediate - { imm= PulseInvalidation.CppDestructor (callee_pname, destroyed_object, call_loc) - ; location= call_loc }) - call_loc destroyed_object astate + (** [out_of_scope_access_expr] has just gone out of scope and in now invalid *) + let exec_object_out_of_scope call_loc out_of_scope_access_expr astate = + (* invalidate both [&x] and [x]: reading either is now forbidden *) + let invalidate access_expr = + PulseOperations.invalidate + (PulseTrace.Immediate {imm= GoneOutOfScope (access_expr, call_loc); location= call_loc}) + call_loc access_expr + in + invalidate (HilExp.AccessExpression.dereference out_of_scope_access_expr) astate + >>= invalidate out_of_scope_access_expr let dispatch_call summary ret (call : HilInstr.call) (actuals : HilExp.t list) flags call_loc @@ -191,13 +184,12 @@ module PulseTransferFunctions = struct PerfEvent.(log (fun logger -> log_begin_event logger ~name:"pulse interproc call" ())) ; let posts = interprocedural_call summary ret call actuals flags call_loc astate in PerfEvent.(log (fun logger -> log_end_event logger ())) ; - match get_destructed_object call actuals flags with - | Some (destructor_pname, access_expr) -> + match get_out_of_scope_object call actuals flags with + | Some access_expr -> L.d_printfln "%a is going out of scope" HilExp.AccessExpression.pp access_expr ; posts >>= fun posts -> - List.map posts ~f:(fun astate -> - destruct_object destructor_pname call_loc access_expr astate ) + List.map posts ~f:(fun astate -> exec_object_out_of_scope call_loc access_expr astate) |> Result.all | None -> posts ) diff --git a/infer/src/pulse/PulseInvalidation.ml b/infer/src/pulse/PulseInvalidation.ml index 5df74cfad..f921a39cf 100644 --- a/infer/src/pulse/PulseInvalidation.ml +++ b/infer/src/pulse/PulseInvalidation.ml @@ -40,7 +40,7 @@ let pp_std_vector_function f = function type t = | CFree of HilExp.AccessExpression.t * Location.t | CppDelete of HilExp.AccessExpression.t * Location.t - | CppDestructor of Typ.Procname.t * HilExp.AccessExpression.t * Location.t + | GoneOutOfScope of HilExp.AccessExpression.t * Location.t | Nullptr | StdVector of std_vector_function * HilExp.AccessExpression.t * Location.t [@@deriving compare] @@ -50,8 +50,8 @@ let issue_type_of_cause = function IssueType.use_after_free | CppDelete _ -> IssueType.use_after_delete - | CppDestructor _ -> - IssueType.use_after_destructor + | GoneOutOfScope _ -> + IssueType.use_after_lifetime | Nullptr -> IssueType.null_dereference | StdVector _ -> @@ -63,9 +63,8 @@ let pp f = function F.fprintf f "by call to `free()` on `%a`" HilExp.AccessExpression.pp access_expr | CppDelete (access_expr, _) -> F.fprintf f "by `delete` on `%a`" HilExp.AccessExpression.pp access_expr - | CppDestructor (proc_name, access_expr, _) -> - F.fprintf f "by destructor call `%a()` on `%a`" Typ.Procname.pp proc_name - HilExp.AccessExpression.pp access_expr + | GoneOutOfScope (access_expr, _) -> + F.fprintf f "`%a` gone out of scope" HilExp.AccessExpression.pp access_expr | Nullptr -> F.fprintf f "null pointer" | StdVector (std_vector_f, access_expr, _) -> diff --git a/infer/src/pulse/PulseInvalidation.mli b/infer/src/pulse/PulseInvalidation.mli index ce6508900..951257ea2 100644 --- a/infer/src/pulse/PulseInvalidation.mli +++ b/infer/src/pulse/PulseInvalidation.mli @@ -22,7 +22,7 @@ val pp_std_vector_function : Format.formatter -> std_vector_function -> unit type t = | CFree of HilExp.AccessExpression.t * Location.t | CppDelete of HilExp.AccessExpression.t * Location.t - | CppDestructor of Typ.Procname.t * HilExp.AccessExpression.t * Location.t + | GoneOutOfScope of HilExp.AccessExpression.t * Location.t | Nullptr | StdVector of std_vector_function * HilExp.AccessExpression.t * Location.t [@@deriving compare] diff --git a/infer/tests/codetoanalyze/cpp/pulse/closures.cpp b/infer/tests/codetoanalyze/cpp/pulse/closures.cpp index 30f98729e..abd651c6c 100644 --- a/infer/tests/codetoanalyze/cpp/pulse/closures.cpp +++ b/infer/tests/codetoanalyze/cpp/pulse/closures.cpp @@ -128,7 +128,7 @@ int ref_capture_return_local_lambda_ok() { return f().f; } -S& ref_capture_return_local_lambda_bad() { +S& FN_ref_capture_return_local_lambda_bad() { S x; auto f = [&x](void) -> S& { // no way to know if ok here diff --git a/infer/tests/codetoanalyze/cpp/pulse/issues.exp b/infer/tests/codetoanalyze/cpp/pulse/issues.exp index 1583bf25c..8fc1fe396 100644 --- a/infer/tests/codetoanalyze/cpp/pulse/issues.exp +++ b/infer/tests/codetoanalyze/cpp/pulse/issues.exp @@ -1,12 +1,16 @@ +codetoanalyze/cpp/pulse/basics.cpp, aggregate_reassign2_ok, 5, USE_AFTER_LIFETIME, no_bucket, ERROR, [variable declared,invalidated `&(a)` gone out of scope here,accessed `a.str` here] +codetoanalyze/cpp/pulse/basics.cpp, aggregate_reassign3_ok, 5, USE_AFTER_LIFETIME, no_bucket, ERROR, [variable declared,invalidated `&(a)` gone out of scope here,accessed `a.a.str` here] +codetoanalyze/cpp/pulse/basics.cpp, aggregate_reassign_ok, 4, USE_AFTER_LIFETIME, no_bucket, ERROR, [variable declared,invalidated `&(s)` gone out of scope here,accessed `s.i` here] codetoanalyze/cpp/pulse/basics.cpp, multiple_invalidations_branch_bad, 6, USE_AFTER_DELETE, no_bucket, ERROR, [invalidated by `delete` on `ptr` here,accessed `*(ptr)` here] codetoanalyze/cpp/pulse/basics.cpp, multiple_invalidations_loop_bad, 3, USE_AFTER_DELETE, no_bucket, ERROR, [invalidated by `delete` on `ptr` here,accessed `ptr` here] -codetoanalyze/cpp/pulse/closures.cpp, implicit_ref_capture_destroy_invoke_bad, 6, USE_AFTER_DESTRUCTOR, no_bucket, ERROR, [variable declared,`&(s)` captured as `s`,invalidated by destructor call `S::~S()` on `s` here,accessed `&(f)` here] -codetoanalyze/cpp/pulse/closures.cpp, ref_capture_destroy_invoke_bad, 6, USE_AFTER_DESTRUCTOR, no_bucket, ERROR, [variable declared,`&(s)` captured as `s`,invalidated by destructor call `S::~S()` on `s` here,accessed `&(f)` here] +codetoanalyze/cpp/pulse/closures.cpp, implicit_ref_capture_destroy_invoke_bad, 6, USE_AFTER_LIFETIME, no_bucket, ERROR, [variable declared,`&(s)` captured as `s`,invalidated `s` gone out of scope here,accessed `&(f)` here] +codetoanalyze/cpp/pulse/closures.cpp, ref_capture_destroy_invoke_bad, 6, USE_AFTER_LIFETIME, no_bucket, ERROR, [variable declared,`&(s)` captured as `s`,invalidated `s` gone out of scope here,accessed `&(f)` here] codetoanalyze/cpp/pulse/interprocedural.cpp, delete_inner_then_write_bad, 2, USE_AFTER_DELETE, no_bucket, ERROR, [invalidated during call to `wraps_delete_inner` here,invalidated by `delete` on `x` here,accessed during call to `wraps_read` here,accessed during call to `wraps_read_inner` here,accessed `x->f` here] codetoanalyze/cpp/pulse/interprocedural.cpp, delete_then_read_bad, 2, USE_AFTER_DELETE, no_bucket, ERROR, [invalidated by `delete` on `x` here,accessed during call to `wraps_read` here,accessed during call to `wraps_read_inner` here,accessed `x->f` here] codetoanalyze/cpp/pulse/interprocedural.cpp, delete_then_write_bad, 2, USE_AFTER_DELETE, no_bucket, ERROR, [invalidated during call to `wraps_delete` here,invalidated during call to `wraps_delete_inner` here,invalidated by `delete` on `x` here,accessed during call to `wraps_read` here,accessed during call to `wraps_read_inner` here,accessed `x->f` here] codetoanalyze/cpp/pulse/interprocedural.cpp, feed_invalid_into_access_bad, 2, USE_AFTER_DELETE, no_bucket, ERROR, [assigned to `y`,assigned to `return`,returned from call to `may_return_invalid_ptr_ok()`,assigned to `y`,invalidated during call to `may_return_invalid_ptr_ok` here,invalidated by `delete` on `y` here,accessed during call to `call_store` here,accessed during call to `store` here,accessed `y->p` here] codetoanalyze/cpp/pulse/join.cpp, invalidate_node_alias_bad, 12, USE_AFTER_DELETE, no_bucket, ERROR, [assigned to `result`,invalidated by `delete` on `result` here,accessed `*(result)` here] +codetoanalyze/cpp/pulse/reference_wrapper.cpp, reference_wrapper_stack_bad, 2, USE_AFTER_LIFETIME, no_bucket, ERROR, [assigned to `this->b`,returned from call to `ReferenceWrapperStack::ReferenceWrapperStack()`,invalidated during call to `getwrapperStack` here,invalidated `&(b)` gone out of scope here,accessed `rw.b->f` 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, 1, STACK_VARIABLE_ADDRESS_ESCAPE, no_bucket, ERROR, [] codetoanalyze/cpp/pulse/returns.cpp, returns::return_variable_stack_reference1_bad, 1, STACK_VARIABLE_ADDRESS_ESCAPE, no_bucket, ERROR, [] @@ -26,7 +30,7 @@ codetoanalyze/cpp/pulse/use_after_destructor.cpp, use_after_destructor::placemen codetoanalyze/cpp/pulse/use_after_destructor.cpp, use_after_destructor::reinit_after_explicit_destructor2_bad, 5, USE_AFTER_DELETE, no_bucket, ERROR, [variable declared,invalidated during call to `use_after_destructor::S::~S` here,invalidated during call to `use_after_destructor::S::__infer_inner_destructor_~S` here,invalidated by `delete` on `this->f` here,accessed during call to `use_after_destructor::S::~S` here,accessed during call to `use_after_destructor::S::__infer_inner_destructor_~S` here,accessed `this->f` here] codetoanalyze/cpp/pulse/use_after_destructor.cpp, use_after_destructor::use_after_destructor_bad, 3, USE_AFTER_DELETE, no_bucket, ERROR, [assigned to `this->f`,returned from call to `use_after_destructor::S::S()`,invalidated during call to `use_after_destructor::S::~S` here,invalidated during call to `use_after_destructor::S::__infer_inner_destructor_~S` here,invalidated by `delete` on `this->f` here,accessed `*(s.f)` here] codetoanalyze/cpp/pulse/use_after_destructor.cpp, use_after_destructor::use_after_scope1_bad, 7, USE_AFTER_DELETE, no_bucket, ERROR, [variable declared,invalidated during call to `use_after_destructor::S::~S` here,invalidated during call to `use_after_destructor::S::__infer_inner_destructor_~S` here,invalidated by `delete` on `this->f` here,accessed during call to `use_after_destructor::S::~S` here,accessed during call to `use_after_destructor::S::__infer_inner_destructor_~S` here,accessed `this->f` here] -codetoanalyze/cpp/pulse/use_after_destructor.cpp, use_after_destructor::use_after_scope4_bad, 6, USE_AFTER_DESTRUCTOR, no_bucket, ERROR, [variable declared,invalidated by destructor call `use_after_destructor::C::~C()` on `c` here,accessed `pc->f` here] +codetoanalyze/cpp/pulse/use_after_destructor.cpp, use_after_destructor::use_after_scope4_bad, 6, USE_AFTER_LIFETIME, no_bucket, ERROR, [variable declared,assigned to `pc`,invalidated `&(c)` gone out of scope here,accessed `pc->f` here] codetoanalyze/cpp/pulse/use_after_free.cpp, double_free_simple_bad, 2, USE_AFTER_FREE, no_bucket, ERROR, [invalidated by call to `free()` on `x` here,accessed `x` here] codetoanalyze/cpp/pulse/use_after_free.cpp, use_after_free_simple_bad, 2, USE_AFTER_FREE, no_bucket, ERROR, [invalidated by call to `free()` on `x` here,accessed `*(x)` here] codetoanalyze/cpp/pulse/vector.cpp, FP_init_fill_then_push_back_loop_ok, 6, VECTOR_INVALIDATION, no_bucket, ERROR, [returned from call to `std::vector::at(&(vec),(unsigned long) 1)`,assigned to `elt`,invalidated potentially invalidated by call to `std::vector::push_back()` on `&(vec)` here,accessed `*(elt)` here] diff --git a/infer/tests/codetoanalyze/cpp/pulse/reference_wrapper.cpp b/infer/tests/codetoanalyze/cpp/pulse/reference_wrapper.cpp index 9b1ff26f3..87f4f298e 100644 --- a/infer/tests/codetoanalyze/cpp/pulse/reference_wrapper.cpp +++ b/infer/tests/codetoanalyze/cpp/pulse/reference_wrapper.cpp @@ -42,7 +42,7 @@ ReferenceWrapperStack getwrapperStack() { return b; } -int FN_reference_wrapper_stack_bad() { +int reference_wrapper_stack_bad() { ReferenceWrapperStack rw = getwrapperStack(); return rw.b->f; // we want to report use after lifetime bug here }