diff --git a/infer/src/checkers/Ownership.ml b/infer/src/checkers/Ownership.ml index af42b4b23..477cddbd0 100644 --- a/infer/src/checkers/Ownership.ml +++ b/infer/src/checkers/Ownership.ml @@ -155,7 +155,7 @@ module Domain = struct (* handle assigning directly to a base var *) - let handle_var_assign lhs_var rhs_exp loc summary astate = + let handle_var_assign (lhs_var, lhs_typ) rhs_exp loc summary astate = if Var.is_return lhs_var then exp_add_reads rhs_exp loc summary astate else match rhs_exp with @@ -172,7 +172,10 @@ module Domain = struct with Not_found -> (* no existing capability on RHS. don't make any assumptions about LHS capability *) remove lhs_var astate ) - | HilExp.AccessExpression AccessExpression.Base _ -> + | 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_var rhs_exp astate | HilExp.Closure (_, captured_vars) -> (* TODO: can be folded into the case above once we have proper AccessExpressions *) @@ -213,8 +216,8 @@ module TransferFunctions (CFG : ProcCfg.S) = struct let exec_instr (astate: Domain.astate) (proc_data: extras ProcData.t) _ (instr: HilInstr.t) = let summary = proc_data.extras in match instr with - | Assign (Base (lhs_var, _), rhs_exp, loc) -> - Domain.handle_var_assign lhs_var rhs_exp loc summary astate + | Assign (Base lhs_base, rhs_exp, loc) -> + Domain.handle_var_assign lhs_base rhs_exp loc summary astate | Assign (lhs_access_exp, rhs_exp, loc) -> (* assign to field, array, indirectly with &/*, or a combination *) Domain.exp_add_reads rhs_exp loc summary astate @@ -244,13 +247,13 @@ module TransferFunctions (CFG : ProcCfg.S) = struct | Call ( _ , Direct Typ.Procname.ObjC_Cpp callee_pname - , [(AccessExpression Base (lhs_var, _)); rhs_exp] + , [(AccessExpression Base lhs_base); 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 + Domain.handle_var_assign lhs_base rhs_exp loc summary astate | Call ( _ , Direct Typ.Procname.ObjC_Cpp callee_pname diff --git a/infer/tests/codetoanalyze/cpp/ownership/closures.cpp b/infer/tests/codetoanalyze/cpp/ownership/closures.cpp index 28fd5b93e..d429e5e19 100644 --- a/infer/tests/codetoanalyze/cpp/ownership/closures.cpp +++ b/infer/tests/codetoanalyze/cpp/ownership/closures.cpp @@ -32,6 +32,16 @@ int implicit_ref_capture_destroy_invoke_bad() { return f(); } +int reassign_lambda_capture_destroy_invoke_bad() { + std::function f; + { + auto s = S(); + auto tmp = [&] { return s.f; }; + f = tmp; + } + return f(); +} + // frontend doesn't understand difference between capture-by-value and // capture-by-ref, need to fix int value_capture_destroy_invoke_ok() { diff --git a/infer/tests/codetoanalyze/cpp/ownership/issues.exp b/infer/tests/codetoanalyze/cpp/ownership/issues.exp index 484f74d03..dbe6bd404 100644 --- a/infer/tests/codetoanalyze/cpp/ownership/issues.exp +++ b/infer/tests/codetoanalyze/cpp/ownership/issues.exp @@ -1,4 +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, reassign_lambda_capture_destroy_invoke_bad, 7, 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] @@ -10,4 +11,3 @@ codetoanalyze/cpp/ownership/use_after_delete.cpp, use_in_branch_bad, 4, USE_AFTE 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 309c17625..83f5b2403 100644 --- a/infer/tests/codetoanalyze/cpp/ownership/use_after_destructor.cpp +++ b/infer/tests/codetoanalyze/cpp/ownership/use_after_destructor.cpp @@ -12,65 +12,87 @@ #include struct S { - int f; + int* f; + S(int i) { + f = new int; + *f = i; + } + + // missing: operator= to copy the pointer. double delete can happen if + // operator= is called - ~S() {} + ~S() { delete f; } }; // destructor called at end of function, no issues -void normal_scope_destructor_ok() { S s{1}; } +void normal_scope_destructor_ok() { S s(1); } void nested_scope_destructor_ok() { - { S s{1}; } + { S s(1); } } int reinit_after_explicit_destructor_ok() { - S s{1}; + S s(1); s.~S(); - s = S{2}; - return s.f; + s = S(2); + return *s.f; } void placement_new_explicit_destructor_ok() { char buf[sizeof(S)]; { - S* s = new (buf) S; + S* s = new (buf) S(1); s->~S(); } { // this use of [buf] shouldn't be flagged - S* s = new (buf) S; + S* s = new (buf) S(2); s->~S(); } } void double_destructor_bad() { - S s{1}; + S s(1); s.~S(); // destructor will be called again after S goes out of scope, which is // undefined behavior } int use_after_destructor_bad() { - S s{1}; + S s(1); s.~S(); - int ret = s.f; + int ret = *s.f; s = S{2}; return ret; } -void use_after_scope1_bad() { - S s; +// can't get this yet because we assume operator= copies resources correctly +// (but this isn't true for S) +void FN_use_after_scope1_bad() { + S s(1); { - S tmp{1}; - s = tmp; + S tmp(2); + s = tmp; // translated as operator=(s, tmp) } // destructor for tmp runs here - // destructor for s here; second time the value held by s has been desructed + // destructor for s here; second time the value held by s has been destructed } void FN_use_after_scope2_bad() { - S s; + S s(1); { - s = S{1}; + s = S(1); } // destructor runs here, but our frontend currently doesn't insert it } + +struct POD { + int f; +}; + +// this code is ok since double-destructing POD struct is ok +void destruct_twice_ok() { + POD p{1}; + { + POD tmp{2}; + p = tmp; + } // destructor for tmp +} // destructor for p runs here, but it's harmless