From a0149872ad1670325855dcaceb08b8de79132891 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Wed, 7 Mar 2018 08:20:41 -0800 Subject: [PATCH] [ownership] report invocation of lambdas capturing a var whose lifetime is over Summary: If a `Closure` expression `e` captures variable `x`, consider `e` as borrowing from `x`. When the closure is invoked via `operator()`, check that the borrow is still valid. Reviewed By: jeremydubreil Differential Revision: D7071839 fbshipit-source-id: d923a6a --- infer/src/IR/HilExp.ml | 13 ++- infer/src/IR/Typ.ml | 2 + infer/src/IR/Typ.mli | 3 + infer/src/checkers/Ownership.ml | 70 +++++++++--- .../codetoanalyze/cpp/ownership/closures.cpp | 107 ++++++++++++++++++ .../codetoanalyze/cpp/ownership/issues.exp | 2 + 6 files changed, 183 insertions(+), 14 deletions(-) create mode 100644 infer/tests/codetoanalyze/cpp/ownership/closures.cpp diff --git a/infer/src/IR/HilExp.ml b/infer/src/IR/HilExp.ml index 5c859d133..39c9b0181 100644 --- a/infer/src/IR/HilExp.ml +++ b/infer/src/IR/HilExp.ml @@ -140,7 +140,18 @@ let of_sil ~include_array_indexes ~f_resolve_id exp typ = | Closure closure -> let environment = List.map - ~f:(fun (value, pvar, typ) -> (AccessPath.base_of_pvar pvar typ, of_sil_ value typ)) + ~f:(fun (value, pvar, typ) -> + (* TODO: this is a hack than can disappear once we have proper AccessExpressions (t23176430) *) + let typ' = + match value with + | Exp.Lvar pvar1 when Pvar.equal pvar1 pvar -> + (* captured by reference, change type to pointer *) + {typ with Typ.desc= Typ.Tptr (typ, Typ.Pk_pointer)} + | _ -> + (* capture by value *) + typ + in + (AccessPath.base_of_pvar pvar typ', of_sil_ value typ') ) closure.captured_vars in Closure (closure.name, environment) diff --git a/infer/src/IR/Typ.ml b/infer/src/IR/Typ.ml index ce05e8f22..2fe5f2b85 100644 --- a/infer/src/IR/Typ.ml +++ b/infer/src/IR/Typ.ml @@ -756,6 +756,8 @@ module Procname = struct let is_cpp_lambda {method_name} = String.is_substring ~substring:"operator()" method_name + let is_operator_equal {method_name} = String.is_substring ~substring:"operator=" method_name + let kind_to_verbose_string = function | CPPMethod m | CPPDestructor m -> "(" ^ (match m with None -> "" | Some s -> s) ^ ")" diff --git a/infer/src/IR/Typ.mli b/infer/src/IR/Typ.mli index 82d2d067c..666f609d1 100644 --- a/infer/src/IR/Typ.mli +++ b/infer/src/IR/Typ.mli @@ -382,6 +382,9 @@ module Procname : sig val is_cpp_lambda : t -> bool (** Return whether the procname is a cpp lambda. *) + + val is_operator_equal : t -> bool + (** Return true if the procname is operator= *) end (** Type of c procedure names. *) diff --git a/infer/src/checkers/Ownership.ml b/infer/src/checkers/Ownership.ml index ffa5f4fb6..fb74d00ad 100644 --- a/infer/src/checkers/Ownership.ml +++ b/infer/src/checkers/Ownership.ml @@ -63,6 +63,15 @@ module CapabilityDomain = struct F.fprintf fmt "Owned" end +let rec is_function_typ = function + | {Typ.desc= Tptr (t, _)} -> + is_function_typ t + | {Typ.desc= Tstruct typename} -> + String.is_prefix ~prefix:"std::function" (Typ.Name.name typename) + | _ -> + false + + (** map from program variable to its capability *) module Domain = struct include AbstractDomain.Map (Var) (CapabilityDomain) @@ -103,11 +112,16 @@ module Domain = struct () - let access_path_add_read ((base_var, _), _) loc summary astate = - check_var_lifetime base_var loc summary astate ; + let base_add_read (base_var, typ) loc summary astate = + (* don't warn if it's a read of a std::function type. we model closures as borrowing their + captured vars, but simply reading a closure doesn't actually use these vars. this means that + we'll miss bugs involving invalid uses of std::function's, but that seems ok *) + if not (is_function_typ typ) then check_var_lifetime base_var loc summary astate ; astate + let access_path_add_read (base, _) loc summary astate = base_add_read base loc summary astate + let exp_add_reads exp loc summary astate = List.fold ~f:(fun acc access_path -> access_path_add_read access_path loc summary acc) @@ -122,15 +136,19 @@ module Domain = struct ~init:astate - let borrow lhs_var rhs_exp astate = + let borrow_vars lhs_var rhs_vars astate = + (* 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 + + + let borrow_exp 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 + borrow_vars lhs_var rhs_vars astate (* handle assigning directly to a base var *) @@ -139,7 +157,7 @@ module Domain = struct else match rhs_exp with | HilExp.AccessExpression AccessExpression.AddressOf address_taken_exp -> - borrow lhs_var (AccessExpression address_taken_exp) astate + borrow_exp lhs_var (AccessExpression address_taken_exp) astate | HilExp.AccessExpression AccessExpression.Base (rhs_var, _) when not (Var.appears_in_source_code rhs_var) -> ( try @@ -152,7 +170,16 @@ module Domain = struct (* 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 + borrow_exp lhs_var 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 = + List.fold captured_vars + ~f:(fun acc ((var, typ), _) -> + match typ.Typ.desc with Typ.Tptr _ -> VarSet.add var acc | _ -> acc ) + ~init:VarSet.empty + in + borrow_vars lhs_var vars_captured_by_ref astate | _ -> (* TODO: support capability transfer between source vars, other assignment modes *) exp_add_reads rhs_exp loc summary astate |> remove lhs_var @@ -207,13 +234,30 @@ module TransferFunctions (CFG : ProcCfg.S) = struct Domain.add constructed_var CapabilityDomain.Owned astate' | _ -> astate' ) - | Call (_, Direct callee_pname, [(AccessExpression Base (lhs_var, _))], _, loc) + | Call (_, Direct callee_pname, [(AccessExpression Base ((lhs_var, _) as lhs_base))], _, loc) when transfers_ownership callee_pname -> - Domain.check_var_lifetime lhs_var loc summary astate ; - Domain.add lhs_var CapabilityDomain.Invalid astate - | Call (_, Direct callee_pname, [(AccessExpression Base (lhs_var, _)); rhs_exp], _, loc) - when String.equal (Typ.Procname.get_method callee_pname) "operator=" -> + Domain.base_add_read lhs_base loc summary astate + |> Domain.add lhs_var CapabilityDomain.Invalid + | Call + ( _ + , Direct Typ.Procname.ObjC_Cpp callee_pname + , [(AccessExpression Base (lhs_var, _)); rhs_exp] + , _ + , loc ) + when Typ.Procname.ObjC_Cpp.is_operator_equal callee_pname -> + (* TODO: once we go interprocedural, this case should only apply for operator='s with an + empty summary *) Domain.handle_var_assign lhs_var rhs_exp loc summary astate + | Call + ( _ + , Direct Typ.Procname.ObjC_Cpp callee_pname + , (AccessExpression Base (lhs_var, _)) :: _ + , _ + , loc ) + when Typ.Procname.ObjC_Cpp.is_cpp_lambda callee_pname -> + (* invoking a lambda; check that it's captured vars are valid *) + Domain.check_var_lifetime lhs_var loc summary astate ; + astate | Call (ret_opt, _, actuals, _, loc) -> ( let astate' = Domain.actuals_add_reads actuals loc summary astate in diff --git a/infer/tests/codetoanalyze/cpp/ownership/closures.cpp b/infer/tests/codetoanalyze/cpp/ownership/closures.cpp new file mode 100644 index 000000000..28fd5b93e --- /dev/null +++ b/infer/tests/codetoanalyze/cpp/ownership/closures.cpp @@ -0,0 +1,107 @@ +/* + * Copyright (c) 2018 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ +#include + +struct S { + int f; + + S() { f = 1; } +}; + +int ref_capture_destroy_invoke_bad() { + std::function f; + { + S s; + f = [&s] { return s.f; }; + } // destructor for s called here + return f(); // s used here +} + +int implicit_ref_capture_destroy_invoke_bad() { + std::function f; + { + auto s = S(); + f = [&] { return s.f; }; + } + return f(); +} + +// frontend doesn't understand difference between capture-by-value and +// capture-by-ref, need to fix +int value_capture_destroy_invoke_ok() { + std::function f; + { + S s; + f = [s] { return s.f; }; + } + return f(); +} + +// same thing here +int implicit_value_capture_destroy_invoke_ok() { + std::function f; + { + S s; + f = [=] { return s.f; }; + } + return f(); +} + +int ref_capture_invoke_ok() { + std::function f; + int ret; + { + S s; + f = [&s] { return s.f; }; + ret = f(); + } + return ret; +} + +void invoke_twice_ok() { + std::function f; + int ret; + { + S s; + f = [&s] { return s.f; }; + f(); + f(); + } +} + +std::function ref_capture_read_lambda_ok() { + std::function f; + int ret; + { + S s; + f = [&s] { return s.f; }; + } + auto tmp = + f; // reading (but not invoking) the lambda doesn't use its captured vars +} + +// we'll miss this because we count invoking a lambda object as a use of its +// captured vars, not the lambda object itself. +void FN_delete_lambda_then_call_bad() { + auto lambda = [] { return 1; }; + ~lambda(); + return lambda(); +} + +// need to treat escaping as a use in order to catch this +std::function FN_ref_capture_return_lambda_bad() { + std::function f; + int ret; + { + S s; + f = [&s] { return s.f; }; + } + return f; // if the caller invokes the lambda, it will try to read the invalid + // stack address +} diff --git a/infer/tests/codetoanalyze/cpp/ownership/issues.exp b/infer/tests/codetoanalyze/cpp/ownership/issues.exp index c5cc9bfe0..484f74d03 100644 --- a/infer/tests/codetoanalyze/cpp/ownership/issues.exp +++ b/infer/tests/codetoanalyze/cpp/ownership/issues.exp @@ -1,3 +1,5 @@ +codetoanalyze/cpp/ownership/closures.cpp, implicit_ref_capture_destroy_invoke_bad, 6, USE_AFTER_LIFETIME, [Use of invalid variable] +codetoanalyze/cpp/ownership/closures.cpp, ref_capture_destroy_invoke_bad, 6, USE_AFTER_LIFETIME, [Use of invalid variable] codetoanalyze/cpp/ownership/use_after_delete.cpp, delete_in_branch_bad, 5, USE_AFTER_LIFETIME, [Use of invalid variable] codetoanalyze/cpp/ownership/use_after_delete.cpp, delete_in_loop_bad, 3, USE_AFTER_LIFETIME, [Use of invalid variable] codetoanalyze/cpp/ownership/use_after_delete.cpp, deref_deleted_bad, 3, USE_AFTER_LIFETIME, [Use of invalid variable]