From 58adf8dd529e1ac8afc22a94bc2ed2ff13ece66d Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Thu, 1 Mar 2018 11:25:10 -0800 Subject: [PATCH] [ownership] implementing borrowing Summary: Fairly simple approach here: - If the RHS of an assignment is a frontend-generated temporary variable, assume it transfers ownership to the LHS variable - If the RHS of an assignment is a program variable, assume that the LHS variable is borrowing from it. - If we try to access a variable that has borrowed from a variable that is now invalid, complain. Reviewed By: jeremydubreil Differential Revision: D7069947 fbshipit-source-id: 99b8ee2 --- infer/src/checkers/Ownership.ml | 73 +++++++++++++++---- .../codetoanalyze/cpp/ownership/issues.exp | 2 + .../cpp/ownership/use_after_destructor.cpp | 19 +++-- 3 files changed, 74 insertions(+), 20 deletions(-) diff --git a/infer/src/checkers/Ownership.ml b/infer/src/checkers/Ownership.ml index 6d0f6bc01..ffa5f4fb6 100644 --- a/infer/src/checkers/Ownership.ml +++ b/infer/src/checkers/Ownership.ml @@ -20,6 +20,11 @@ module CapabilityDomain = struct | Owned (** owned. permits reads, writes, and ownership transfer (e.g. call a destructor, delete, or free) *) + let make_borrowed_vars vars = + assert (not (VarSet.is_empty vars)) ; + BorrowedFrom vars + + (** Owned <= BorrowedFrom <= Invalid *) let ( <= ) ~lhs ~rhs = match (lhs, rhs) with @@ -117,9 +122,40 @@ module Domain = struct ~init:astate + let borrow lhs_var rhs_exp astate = + let rhs_vars = + List.fold (HilExp.get_access_paths rhs_exp) + ~f:(fun acc ((var, _), _) -> VarSet.add var acc) + ~init:VarSet.empty + in + (* borrow all of the vars read on the rhs *) + if VarSet.is_empty rhs_vars then remove lhs_var astate + else add lhs_var (CapabilityDomain.make_borrowed_vars rhs_vars) astate + + (* handle assigning directly to a base var *) let handle_var_assign lhs_var rhs_exp loc summary astate = - exp_add_reads rhs_exp loc summary astate |> remove lhs_var + if Var.is_return lhs_var then exp_add_reads rhs_exp loc summary astate + else + match rhs_exp with + | HilExp.AccessExpression AccessExpression.AddressOf address_taken_exp -> + borrow lhs_var (AccessExpression address_taken_exp) astate + | HilExp.AccessExpression AccessExpression.Base (rhs_var, _) + when not (Var.appears_in_source_code rhs_var) -> ( + try + (* 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_var astate in + add lhs_var base_capability astate + with Not_found -> + (* no existing capability on RHS. don't make any assumptions about LHS capability *) + remove lhs_var astate ) + | HilExp.AccessExpression AccessExpression.Base _ -> + borrow lhs_var rhs_exp astate + | _ -> + (* TODO: support capability transfer between source vars, other assignment modes *) + exp_add_reads rhs_exp loc summary astate |> remove lhs_var end module TransferFunctions (CFG : ProcCfg.S) = struct @@ -128,6 +164,11 @@ module TransferFunctions (CFG : ProcCfg.S) = struct type extras = Specs.summary + let acquires_ownership pname = + (* TODO: support new[], malloc, others? *) + Typ.Procname.equal pname BuiltinDecl.__new + + let transfers_ownership pname = (* TODO: support delete[], free, and (in some cases) std::move *) Typ.Procname.equal pname BuiltinDecl.__delete @@ -148,6 +189,24 @@ module TransferFunctions (CFG : ProcCfg.S) = struct (* assign to field, array, indirectly with &/*, or a combination *) Domain.exp_add_reads rhs_exp loc summary astate |> Domain.access_path_add_read (AccessExpression.to_access_path lhs_access_exp) loc summary + | Call (Some (lhs_var, _), Direct callee_pname, actuals, _, loc) + when acquires_ownership callee_pname -> + Domain.actuals_add_reads actuals loc summary astate + |> Domain.add lhs_var CapabilityDomain.Owned + | Call (None, Direct callee_pname, lhs_exp :: actuals, _, loc) + when Typ.Procname.is_constructor callee_pname + -> ( + (* similar to acquires ownership case *) + let astate' = Domain.actuals_add_reads actuals loc summary astate in + (* frontend translates constructors as assigning-by-ref to first actual *) + match lhs_exp with + | AccessExpression + ( AccessExpression.AddressOf Base (constructed_var, _) + | Base (constructed_var, _) + (* TODO: get rid of second case once AccessExpression conversion is complete *) ) -> + Domain.add constructed_var CapabilityDomain.Owned astate' + | _ -> + astate' ) | Call (_, Direct callee_pname, [(AccessExpression Base (lhs_var, _))], _, loc) when transfers_ownership callee_pname -> Domain.check_var_lifetime lhs_var loc summary astate ; @@ -155,18 +214,6 @@ module TransferFunctions (CFG : ProcCfg.S) = struct | Call (_, Direct callee_pname, [(AccessExpression Base (lhs_var, _)); rhs_exp], _, loc) when String.equal (Typ.Procname.get_method callee_pname) "operator=" -> Domain.handle_var_assign lhs_var rhs_exp loc summary astate - | Call (None, Direct callee_pname, lhs_exp :: actuals, _, loc) - when Typ.Procname.is_constructor callee_pname -> - (* frontend translates constructors as assigning-by-ref to first actual *) - let constructed_var = - match lhs_exp with - | AccessExpression Base (var, _) -> - var - | _ -> - L.die InternalError "Unexpected: constructor called on %a" HilExp.pp lhs_exp - in - Domain.actuals_add_reads actuals loc summary astate - |> Domain.add constructed_var CapabilityDomain.Owned | Call (ret_opt, _, actuals, _, loc) -> ( let astate' = Domain.actuals_add_reads actuals loc summary astate in diff --git a/infer/tests/codetoanalyze/cpp/ownership/issues.exp b/infer/tests/codetoanalyze/cpp/ownership/issues.exp index 82af62845..c5cc9bfe0 100644 --- a/infer/tests/codetoanalyze/cpp/ownership/issues.exp +++ b/infer/tests/codetoanalyze/cpp/ownership/issues.exp @@ -7,3 +7,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, [Use of invalid variable] codetoanalyze/cpp/ownership/use_after_delete.cpp, use_in_loop_bad, 4, USE_AFTER_LIFETIME, [Use of invalid variable] codetoanalyze/cpp/ownership/use_after_destructor.cpp, double_destructor_bad, 5, USE_AFTER_LIFETIME, [Use of invalid variable] +codetoanalyze/cpp/ownership/use_after_destructor.cpp, use_after_destructor_bad, 3, USE_AFTER_LIFETIME, [Use of invalid variable] +codetoanalyze/cpp/ownership/use_after_destructor.cpp, use_after_scope1_bad, 7, USE_AFTER_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 4fbab4eb0..309c17625 100644 --- a/infer/tests/codetoanalyze/cpp/ownership/use_after_destructor.cpp +++ b/infer/tests/codetoanalyze/cpp/ownership/use_after_destructor.cpp @@ -51,21 +51,26 @@ void double_destructor_bad() { // undefined behavior } -// frontend does the right thing here, but we need to propagate permissions from -// tmp to s -int FN_use_after_scope1_bad() { +int use_after_destructor_bad() { + S s{1}; + s.~S(); + int ret = s.f; + s = S{2}; + return ret; +} + +void use_after_scope1_bad() { S s; { S tmp{1}; s = tmp; - } // destructor runs here - return s.f; + } // destructor for tmp runs here + // destructor for s here; second time the value held by s has been desructed } -int FN_use_after_scope2_bad() { +void FN_use_after_scope2_bad() { S s; { s = S{1}; } // destructor runs here, but our frontend currently doesn't insert it - return s.f; }