From 9ca945aa2f87197e2dcfd22cce3320b0561bbb2e Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Fri, 18 May 2018 09:59:24 -0700 Subject: [PATCH] [ownership] warn when returning references to borrowed values Reviewed By: jeremydubreil Differential Revision: D7312430 fbshipit-source-id: 636e31b --- infer/src/IR/Pvar.ml | 8 ++ infer/src/IR/Pvar.mli | 5 + infer/src/IR/Typ.ml | 2 + infer/src/IR/Typ.mli | 2 + infer/src/IR/Var.ml | 7 + infer/src/IR/Var.mli | 2 + infer/src/checkers/Ownership.ml | 119 +++++++++++----- infer/src/clang/cTrans.ml | 18 ++- .../codetoanalyze/cpp/ownership/issues.exp | 6 +- .../codetoanalyze/cpp/ownership/returns.cpp | 133 ++++++++++++++++++ .../cpp/ownership/use_after_delete.cpp | 9 ++ .../cpp/ownership/use_after_destructor.cpp | 7 +- 12 files changed, 276 insertions(+), 42 deletions(-) create mode 100644 infer/tests/codetoanalyze/cpp/ownership/returns.cpp diff --git a/infer/src/IR/Pvar.ml b/infer/src/IR/Pvar.ml index c3bd869cf..e22b62f4e 100644 --- a/infer/src/IR/Pvar.ml +++ b/infer/src/IR/Pvar.ml @@ -148,6 +148,9 @@ let is_return pv = Mangled.equal (get_name pv) Ident.name_return (** something that can't be part of a legal identifier in any conceivable language *) let tmp_prefix = "0$?%__sil_tmp" +(** name given to variables representing C++ temporary objects *) +let materialized_cpp_temporary = "SIL_materialize_temp__" + (** return true if [pvar] is a temporary variable generated by the frontend *) let is_frontend_tmp pvar = (* Check whether the program variable is a temporary one generated by Sawja, javac, or some other @@ -176,6 +179,11 @@ let is_ssa_frontend_tmp pvar = not (String.contains name '_' && String.contains name '$') +let is_cpp_temporary pvar = + let name = to_string pvar in + String.is_substring ~substring:materialized_cpp_temporary name + + (** Turn an ordinary program variable into a callee program variable *) let to_callee pname pvar = match pvar.pv_kind with diff --git a/infer/src/IR/Pvar.mli b/infer/src/IR/Pvar.mli index 50f99eeef..658723418 100644 --- a/infer/src/IR/Pvar.mli +++ b/infer/src/IR/Pvar.mli @@ -78,6 +78,9 @@ val is_ssa_frontend_tmp : t -> bool (** return true if [pvar] is a temporary variable generated by the frontend and is only assigned once on a non-looping control-flow path *) +val is_cpp_temporary : t -> bool +(** return true if this pvar represents a C++ temporary object (see http://en.cppreference.com/w/cpp/language/lifetime) *) + val mk : Mangled.t -> Typ.Procname.t -> t (** [mk name proc_name suffix] creates a program var with the given function name and suffix *) @@ -133,3 +136,5 @@ val get_initializer_pname : t -> Typ.Procname.t option val get_name_of_local_with_procname : t -> Mangled.t (** [get_name_of_local_with_procname var] Return a name that is composed of the name of var and the name of the procname in case of locals *) + +val materialized_cpp_temporary : string diff --git a/infer/src/IR/Typ.ml b/infer/src/IR/Typ.ml index a76d3a017..da63be53b 100644 --- a/infer/src/IR/Typ.ml +++ b/infer/src/IR/Typ.ml @@ -524,6 +524,8 @@ let is_cpp_class = is_class_of_kind Name.Cpp.is_class let is_pointer typ = match typ.desc with Tptr _ -> true | _ -> false +let is_reference typ = match typ.desc with Tptr (_, Pk_reference) -> true | _ -> false + let is_pointer_to_cpp_class typ = match typ.desc with Tptr (t, _) -> is_cpp_class t | _ -> false let has_block_prefix s = diff --git a/infer/src/IR/Typ.mli b/infer/src/IR/Typ.mli index 81f6f72c7..354dd13da 100644 --- a/infer/src/IR/Typ.mli +++ b/infer/src/IR/Typ.mli @@ -265,6 +265,8 @@ val is_pointer_to_cpp_class : t -> bool val is_pointer : t -> bool +val is_reference : t -> bool + val has_block_prefix : string -> bool val unsome : string -> t option -> t diff --git a/infer/src/IR/Var.ml b/infer/src/IR/Var.ml index 7b8b2c6f3..9294e84bc 100644 --- a/infer/src/IR/Var.ml +++ b/infer/src/IR/Var.ml @@ -45,6 +45,13 @@ let appears_in_source_code = function not (Pvar.is_frontend_tmp pvar) +let is_cpp_temporary = function + | LogicalVar _ -> + false + | ProgramVar pvar -> + Pvar.is_cpp_temporary pvar + + let pp fmt = function | ProgramVar pv -> F.pp_print_string fmt (Pvar.get_simplified_name pv) diff --git a/infer/src/IR/Var.mli b/infer/src/IR/Var.mli index c74905c6b..81a20dcc1 100644 --- a/infer/src/IR/Var.mli +++ b/infer/src/IR/Var.mli @@ -36,6 +36,8 @@ val appears_in_source_code : t -> bool (** return true if this variable appears in source code (i.e., is not a LogicalVar or a frontend-generated ProgramVar) *) +val is_cpp_temporary : t -> bool + val get_footprint_index : t -> int option val pp : Format.formatter -> t -> unit diff --git a/infer/src/checkers/Ownership.ml b/infer/src/checkers/Ownership.ml index da9adb07e..39b885eb4 100644 --- a/infer/src/checkers/Ownership.ml +++ b/infer/src/checkers/Ownership.ml @@ -94,6 +94,22 @@ let rec is_function_typ = function module Domain = struct include AbstractDomain.Map (Base) (CapabilityDomain) + let report message loc ltr summary = + let exn = Exceptions.Checkers (IssueType.use_after_lifetime, Localise.verbatim_desc message) in + Reporting.log_error summary ~loc ~ltr exn + + + let report_return_stack_var (var, _) loc summary = + let message = + F.asprintf "Reference to stack variable %a is returned at %a" Var.pp var Location.pp loc + in + let ltr = + [ Errlog.make_trace_element 0 loc "Return of stack variable" [] + ; Errlog.make_trace_element 0 loc "End of procedure" [] ] + in + report message loc ltr summary + + let report_use_after_lifetime (var, _) ~use_loc ~invalidated_loc summary = let message = F.asprintf "Variable %a is used at line %a after its lifetime ended at %a" Var.pp var @@ -103,8 +119,7 @@ module Domain = struct [ Errlog.make_trace_element 0 invalidated_loc "End of variable lifetime" [] ; Errlog.make_trace_element 0 use_loc "Use of invalid variable" [] ] in - let exn = Exceptions.Checkers (IssueType.use_after_lifetime, Localise.verbatim_desc message) in - Reporting.log_error summary ~loc:use_loc ~ltr exn + report message use_loc ltr summary (* complain if we do not have the right capability to access [var] *) @@ -164,34 +179,42 @@ module Domain = struct else add lhs_base (CapabilityDomain.make_borrowed_vars rhs_vars) astate + (* copy capability if it exists, consider borrowing if it doesn't *) + let copy_or_borrow_var lhs_base rhs_base astate = + try + let rhs_capability = find rhs_base astate in + add lhs_base rhs_capability astate + with Caml.Not_found -> + if Var.is_cpp_temporary (fst rhs_base) then + borrow_vars lhs_base (VarSet.singleton rhs_base) astate + else add lhs_base CapabilityDomain.Owned astate + + (* handle assigning directly to a base var *) - 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 (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 *) - (* copy capability from RHS to LHS *) - let base_capability = find rhs_base astate in - add lhs_base base_capability astate - with Caml.Not_found -> - (* no existing capability on RHS. don't make any assumptions about LHS capability *) - remove lhs_base 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 (((_, typ) as base), _) -> - match typ.Typ.desc with Typ.Tptr _ -> VarSet.add base acc | _ -> acc ) - ~init:VarSet.empty - in - borrow_vars lhs_base 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_base + let handle_var_assign ?(is_operator_equal= false) lhs_base rhs_exp loc summary astate = + match rhs_exp with + | HilExp.Constant _ when not (Var.is_cpp_temporary (fst lhs_base)) -> + add lhs_base CapabilityDomain.Owned astate + | HilExp.AccessExpression (Base rhs_base | AddressOf (Base rhs_base)) + when not (Var.appears_in_source_code (fst rhs_base)) -> + copy_or_borrow_var lhs_base rhs_base 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 (((_, typ) as base), _) -> + match typ.Typ.desc with Typ.Tptr _ -> VarSet.add base acc | _ -> acc ) + ~init:VarSet.empty + in + borrow_vars lhs_base vars_captured_by_ref astate + | HilExp.AccessExpression (Base rhs_base) + when not is_operator_equal && Typ.is_reference (snd rhs_base) -> + copy_or_borrow_var lhs_base rhs_base astate + | HilExp.AccessExpression (AddressOf (Base rhs_base)) when not is_operator_equal -> + borrow_vars lhs_base (VarSet.singleton rhs_base) astate + | _ -> + (* 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 = @@ -234,11 +257,12 @@ module TransferFunctions (CFG : ProcCfg.S) = struct else if Typ.Procname.equal pname BuiltinDecl.__placement_new then match List.rev actuals with | HilExp.AccessExpression (Base placement_base) :: other_actuals -> - (* placement new creates an alias between return var and placement var. model as - return borrowing from placement *) + (* TODO: placement new creates an alias between return var and placement var, should + eventually model as return borrowing from placement (see + FN_placement_new_aliasing2_bad test) *) Domain.actuals_add_reads other_actuals loc summary astate |> Domain.add placement_base CapabilityDomain.Owned - |> Domain.borrow_vars return_base (VarSet.singleton placement_base) |> Option.some + |> Domain.add return_base CapabilityDomain.Owned |> Option.some | _ :: other_actuals -> Domain.actuals_add_reads other_actuals loc summary astate |> Domain.add return_base CapabilityDomain.Owned |> Option.some @@ -312,7 +336,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct 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_base rhs_exp loc summary astate + Domain.handle_var_assign ~is_operator_equal:true lhs_base rhs_exp loc summary astate | Call ( _ , Direct (Typ.Procname.ObjC_Cpp callee_pname) @@ -343,8 +367,35 @@ end module Analyzer = LowerHil.MakeAbstractInterpreter (ProcCfg.Exceptional) (TransferFunctions) +let report_invalid_return post end_loc formal_map summary = + (* look for return values that are borrowed from (now-invalid) local variables *) + let report_invalid_return base (capability: CapabilityDomain.astate) = + if Var.is_return (fst base) then + match capability with + | BorrowedFrom vars -> + VarSet.iter + (fun borrowed_base -> + if + not (FormalMap.is_formal borrowed_base formal_map) + && not (Var.is_global (fst borrowed_base)) + then Domain.report_return_stack_var borrowed_base end_loc summary ) + vars + | InvalidatedAt invalidated_loc -> + Domain.report_use_after_lifetime base ~use_loc:end_loc ~invalidated_loc summary + | Owned -> + () + in + Domain.iter report_invalid_return post + + let checker {Callbacks.proc_desc; tenv; summary} = let proc_data = ProcData.make proc_desc tenv summary in let initial = Domain.empty in - ignore (Analyzer.compute_post proc_data ~initial) ; + ( match Analyzer.compute_post proc_data ~initial with + | Some post -> + let end_loc = Procdesc.Node.get_loc (Procdesc.get_exit_node proc_desc) in + let formal_map = FormalMap.make proc_desc in + report_invalid_return post end_loc formal_map summary + | None -> + () ) ; summary diff --git a/infer/src/clang/cTrans.ml b/infer/src/clang/cTrans.ml index 1c1892eeb..29c32b338 100644 --- a/infer/src/clang/cTrans.ml +++ b/infer/src/clang/cTrans.ml @@ -1433,11 +1433,16 @@ module CTrans_funct (F : CModule_type.CFrontend) : CModule_type.CTranslation = s ~f:(function | Clang_ast_t.VarDecl (_, _, qual_type, _) as decl -> let pvar = CVar_decl.sil_var_of_decl context decl procname in - let exp = Exp.Lvar pvar in - let typ = CType_decl.qual_type_to_sil_type context.CContext.tenv qual_type in - let this_res_trans_destruct = mk_trans_result (exp, typ) empty_control in - cxx_destructor_call_trans trans_state_pri stmt_info_loc this_res_trans_destruct - qual_type.Clang_ast_t.qt_type_ptr ~is_inner_destructor:false + if Pvar.is_static_local pvar then + (* don't call destructors on static vars *) + None + else + let exp = Exp.Lvar pvar in + let typ = CType_decl.qual_type_to_sil_type context.CContext.tenv qual_type in + let this_res_trans_destruct = mk_trans_result (exp, typ) empty_control in + cxx_destructor_call_trans trans_state_pri stmt_info_loc + this_res_trans_destruct qual_type.Clang_ast_t.qt_type_ptr + ~is_inner_destructor:false | _ -> assert false) vars_to_destroy @@ -2867,7 +2872,8 @@ module CTrans_funct (F : CModule_type.CFrontend) : CModule_type.CTranslation = s (* typ_tmp is 'best guess' type of variable - translation may decide to use different type later *) let pvar, typ_tmp = - mk_temp_sil_var_for_expr context.CContext.tenv procdesc "SIL_materialize_temp__" expr_info + mk_temp_sil_var_for_expr context.CContext.tenv procdesc Pvar.materialized_cpp_temporary + expr_info in let temp_exp = match stmt_list with [p] -> p | _ -> assert false in let var_exp_typ = (Exp.Lvar pvar, typ_tmp) in diff --git a/infer/tests/codetoanalyze/cpp/ownership/issues.exp b/infer/tests/codetoanalyze/cpp/ownership/issues.exp index 401290066..8beaf0c11 100644 --- a/infer/tests/codetoanalyze/cpp/ownership/issues.exp +++ b/infer/tests/codetoanalyze/cpp/ownership/issues.exp @@ -4,6 +4,11 @@ codetoanalyze/cpp/ownership/basics.cpp, multiple_invalidations_loop_bad, 5, USE_ 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, ref_capture_destroy_invoke_bad, 6, USE_AFTER_LIFETIME, ERROR, [End of variable lifetime,Use of invalid variable] +codetoanalyze/cpp/ownership/returns.cpp, returns::return_deleted_bad, 4, USE_AFTER_LIFETIME, ERROR, [End of variable lifetime,Use of invalid variable] +codetoanalyze/cpp/ownership/returns.cpp, returns::return_literal_stack_reference_bad, 0, USE_AFTER_LIFETIME, ERROR, [Return of stack variable,End of procedure] +codetoanalyze/cpp/ownership/returns.cpp, returns::return_stack_pointer_bad, 3, USE_AFTER_LIFETIME, ERROR, [Return of stack variable,End of procedure] +codetoanalyze/cpp/ownership/returns.cpp, returns::return_variable_stack_reference1_bad, 3, USE_AFTER_LIFETIME, ERROR, [Return of stack variable,End of procedure] +codetoanalyze/cpp/ownership/returns.cpp, returns::return_variable_stack_reference2_bad, 4, USE_AFTER_LIFETIME, ERROR, [Return of stack variable,End of procedure] 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] codetoanalyze/cpp/ownership/use_after_delete.cpp, deref_deleted_bad, 3, USE_AFTER_LIFETIME, ERROR, [End of variable lifetime,Use of invalid variable] @@ -13,5 +18,4 @@ 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/returns.cpp b/infer/tests/codetoanalyze/cpp/ownership/returns.cpp new file mode 100644 index 000000000..55fb33e3e --- /dev/null +++ b/infer/tests/codetoanalyze/cpp/ownership/returns.cpp @@ -0,0 +1,133 @@ +/* + * 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 + +namespace returns { + +struct S { + int f_; + + S(int f) : f_(f) {} + + ~S() {} +}; + +const int& return_literal_stack_reference_bad() { return 1; } + +const int& return_variable_stack_reference1_bad() { + const int& x = 2; + return x; +} + +const int& return_variable_stack_reference2_bad() { + const int& x = 2; + const int& y = x; + return y; +} + +const int return_read_of_stack_reference_ok() { + const int& x = 2; + return x; +} + +const int& return_formal_reference_ok(int& formal) { return formal; } + +const int& return_reference_to_formal_pointer_ok(const int* formal) { + const int& local = *formal; + return local; +} + +extern const int& callee(); + +const int& return_reference_from_callee_ok() { + const int& local = callee(); + return local; +} + +const int return_int_ok() { return 1; } + +const bool return_comparison_temp_ok() { return 1 != 2; } + +const bool compare_local_refs_ok() { + const int& local1 = 1; + const int& local2 = 1; + return local1 != local2; +} + +extern int& global; + +const int& return_global_reference_ok() { return global; } + +struct MemberReference { + int& member1; + + int& return_member_reference_ok() { return member1; } + + int* member2; + int* return_member_reference_indirect_ok() { + int* local = member2; + return local; + } +}; + +extern const char* const kOptions; + +const char* return_field_addr_ternary_ok() { + const char* const* const t = &kOptions; + return t ? *t : ""; +} + +int* return_stack_pointer_bad() { + int x = 3; + return &x; +} + +S* return_static_local_ok() { + S* local; + static S s{1}; + local = &s; + return local; +} + +S* return_static_local_inner_scope_ok(bool b) { + S* local = nullptr; + if (b) { + static S s{1}; + local = &s; + // destructor for s gets called here, but it shouldn't be + } + return local; +} + +int* return_formal_pointer_ok(int* formal) { return formal; } + +int* return_deleted_bad() { + int* x = new int; + *x = 2; + delete x; + return x; +} + +// this *could* be ok depending on what the destructor does, but there's +// probably no good reason to do it +S* FN_return_destructed_pointer_bad() { + S* s = new S(1); + s->~S(); + return s; +} + +const char* return_nullptr1_ok() { return nullptr; } + +const char* return_nullptr2_ok() { + const char* local = nullptr; + return local; +} + +} // namespace returns diff --git a/infer/tests/codetoanalyze/cpp/ownership/use_after_delete.cpp b/infer/tests/codetoanalyze/cpp/ownership/use_after_delete.cpp index 750b260fc..b98705d0e 100644 --- a/infer/tests/codetoanalyze/cpp/ownership/use_after_delete.cpp +++ b/infer/tests/codetoanalyze/cpp/ownership/use_after_delete.cpp @@ -9,6 +9,7 @@ #include #include +#include struct S { int f; @@ -91,6 +92,14 @@ void delete_in_loop_ok() { } } +void delete_ref_in_loop_ok(int j, std::vector v) { + int i = 0; + for (int i = 0; i < 10; i++) { + auto s = &v[i]; + delete s; + } +} + void use_in_loop_bad() { auto s = new S{1}; delete s; diff --git a/infer/tests/codetoanalyze/cpp/ownership/use_after_destructor.cpp b/infer/tests/codetoanalyze/cpp/ownership/use_after_destructor.cpp index 546b9cb5f..1acdd1fd6 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* placement_new_aliasing2_bad() { +S* FN_placement_new_aliasing2_bad() { S* s = new S(1); s->~S(); auto alias = new (s) S(2); @@ -154,6 +154,11 @@ void placement_new_non_var_ok() { delete m.s; } +void return_placement_new_ok() { + auto mem = new S(1); + return new (mem) S(2); +} + void destructor_in_loop_ok() { for (int i = 0; i < 10; i++) { S s(1);