From 9180ff56c1a4472e07cabe1d1c3354a098800b48 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Wed, 4 Apr 2018 09:31:19 -0700 Subject: [PATCH] [ownership] handle new kinds of access expressions Reviewed By: jeremydubreil Differential Revision: D7482554 fbshipit-source-id: c511bda --- Makefile | 4 +- infer/src/checkers/Ownership.ml | 52 +++++++------------ .../codetoanalyze/cpp/ownership/closures.cpp | 2 +- .../codetoanalyze/cpp/ownership/issues.exp | 2 +- .../cpp/ownership/use_after_destructor.cpp | 2 +- 5 files changed, 24 insertions(+), 38 deletions(-) diff --git a/Makefile b/Makefile index 35307e3ad..d88e105e0 100644 --- a/Makefile +++ b/Makefile @@ -50,8 +50,8 @@ BUILD_SYSTEMS_TESTS += \ DIRECT_TESTS += \ c_biabduction c_bufferoverrun c_errors c_frontend c_performance \ - cpp_bufferoverrun cpp_errors cpp_frontend cpp_liveness cpp_quandary \ - cpp_racerd cpp_siof cpp_uninit cpp_nullable cpp_conflicts \ + cpp_bufferoverrun cpp_errors cpp_frontend cpp_liveness cpp_ownership cpp_quandary \ + cpp_racerd cpp_siof cpp_uninit cpp_nullable cpp_conflicts \ ifneq ($(BUCK),no) BUILD_SYSTEMS_TESTS += buck-clang-db buck_flavors buck_flavors_run buck_flavors_deterministic diff --git a/infer/src/checkers/Ownership.ml b/infer/src/checkers/Ownership.ml index bfd84e2af..61474db62 100644 --- a/infer/src/checkers/Ownership.ml +++ b/infer/src/checkers/Ownership.ml @@ -164,26 +164,13 @@ module Domain = struct else add lhs_base (CapabilityDomain.make_borrowed_vars rhs_vars) astate - let borrow_exp lhs_var rhs_exp astate = - let rhs_vars = - List.fold (HilExp.get_access_exprs rhs_exp) - ~f:(fun acc access_expr -> - let var, _ = AccessExpression.to_access_path access_expr in - VarSet.add var acc ) - ~init:VarSet.empty - in - borrow_vars lhs_var rhs_vars astate - - (* handle assigning directly to a base var *) - let handle_var_assign ((lhs_var, lhs_typ) as lhs_base) rhs_exp loc summary astate = - if Var.is_return lhs_var then exp_add_reads rhs_exp loc summary astate + let handle_var_assign 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.AccessExpression AccessExpression.AddressOf address_taken_exp -> - borrow_exp lhs_base (AccessExpression address_taken_exp) astate - | HilExp.AccessExpression AccessExpression.Base ((rhs_var, _) as rhs_base) - when not (Var.appears_in_source_code rhs_var) -> ( + | HilExp.AccessExpression (Base rhs_base | AddressOf Base rhs_base) + when not (Var.appears_in_source_code (fst rhs_base)) -> ( try (* assume assignments with non-source vars on the RHS transfer capabilities to the LHS var *) @@ -193,11 +180,6 @@ module Domain = struct with Not_found -> (* no existing capability on RHS. don't make any assumptions about LHS capability *) remove lhs_base astate ) - | 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_base rhs_exp astate | HilExp.Closure (_, captured_vars) -> (* TODO: can be folded into the case above once we have proper AccessExpressions *) let vars_captured_by_ref = @@ -210,6 +192,10 @@ module Domain = struct | _ -> (* TODO: support capability transfer between source vars, other assignment modes *) exp_add_reads rhs_exp loc summary astate |> remove lhs_base + + + let release_ownership base loc summary astate = + base_add_read base loc summary astate |> add base (CapabilityDomain.InvalidatedAt loc) end module TransferFunctions (CFG : ProcCfg.S) = struct @@ -265,7 +251,8 @@ module TransferFunctions (CFG : ProcCfg.S) = struct Typ.Procname.pp pname Location.pp loc else if Typ.Procname.is_constructor pname then match actuals with - | (HilExp.AccessExpression access_expression) :: other_actuals -> ( + | (HilExp.AccessExpression AccessExpression.AddressOf access_expression) :: other_actuals + -> ( match get_assigned_base access_expression with | Some constructed_base -> let astate' = Domain.actuals_add_reads other_actuals loc summary astate in @@ -279,11 +266,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct None - let transfers_ownership pname = - (* TODO: support delete[], free, and (in some cases) std::move *) - Typ.Procname.equal pname BuiltinDecl.__delete - || - match pname with + let is_destructor = function | Typ.Procname.ObjC_Cpp clang_pname -> Typ.Procname.ObjC_Cpp.is_destructor clang_pname && not @@ -318,13 +301,16 @@ module TransferFunctions (CFG : ProcCfg.S) = struct (AccessExpression.to_access_path lhs_access_exp) loc summary ) | Call (_, Direct callee_pname, [(AccessExpression Base lhs_base)], _, loc) - when transfers_ownership callee_pname -> - Domain.base_add_read lhs_base loc summary astate - |> Domain.add lhs_base (CapabilityDomain.InvalidatedAt loc) + when Typ.Procname.equal callee_pname BuiltinDecl.__delete -> + (* TODO: support delete[], free, and (in some cases) std::move *) + Domain.release_ownership lhs_base loc summary astate + | Call (_, Direct callee_pname, [(AccessExpression AddressOf Base lhs_base)], _, loc) + when is_destructor callee_pname -> + Domain.release_ownership lhs_base loc summary astate | Call ( _ , Direct Typ.Procname.ObjC_Cpp callee_pname - , [(AccessExpression Base lhs_base); rhs_exp] + , [(AccessExpression AddressOf Base lhs_base); rhs_exp] , _ , loc ) when Typ.Procname.ObjC_Cpp.is_operator_equal callee_pname -> @@ -334,7 +320,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct | Call ( _ , Direct Typ.Procname.ObjC_Cpp callee_pname - , (AccessExpression Base lhs_base) :: _ + , (AccessExpression AddressOf Base lhs_base) :: _ , _ , loc ) when Typ.Procname.ObjC_Cpp.is_cpp_lambda callee_pname -> diff --git a/infer/tests/codetoanalyze/cpp/ownership/closures.cpp b/infer/tests/codetoanalyze/cpp/ownership/closures.cpp index d429e5e19..396c6b7ea 100644 --- a/infer/tests/codetoanalyze/cpp/ownership/closures.cpp +++ b/infer/tests/codetoanalyze/cpp/ownership/closures.cpp @@ -32,7 +32,7 @@ int implicit_ref_capture_destroy_invoke_bad() { return f(); } -int reassign_lambda_capture_destroy_invoke_bad() { +int FN_reassign_lambda_capture_destroy_invoke_bad() { std::function f; { auto s = S(); diff --git a/infer/tests/codetoanalyze/cpp/ownership/issues.exp b/infer/tests/codetoanalyze/cpp/ownership/issues.exp index 1f6478736..401290066 100644 --- a/infer/tests/codetoanalyze/cpp/ownership/issues.exp +++ b/infer/tests/codetoanalyze/cpp/ownership/issues.exp @@ -3,7 +3,6 @@ codetoanalyze/cpp/ownership/basics.cpp, multiple_invalidations_loop_bad, 3, USE_ codetoanalyze/cpp/ownership/basics.cpp, multiple_invalidations_loop_bad, 5, USE_AFTER_LIFETIME, ERROR, [End of variable lifetime,Use of invalid variable] 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, reassign_lambda_capture_destroy_invoke_bad, 7, 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/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] @@ -14,4 +13,5 @@ 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] diff --git a/infer/tests/codetoanalyze/cpp/ownership/use_after_destructor.cpp b/infer/tests/codetoanalyze/cpp/ownership/use_after_destructor.cpp index 6e3141148..546b9cb5f 100644 --- a/infer/tests/codetoanalyze/cpp/ownership/use_after_destructor.cpp +++ b/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* FN_placement_new_aliasing2_bad() { +S* placement_new_aliasing2_bad() { S* s = new S(1); s->~S(); auto alias = new (s) S(2);