From 8a9fcdc43f23b80fc5265f2f1d202bcd67abb602 Mon Sep 17 00:00:00 2001 From: Dulma Churchill Date: Wed, 31 Jan 2018 06:38:27 -0800 Subject: [PATCH] [retain cycles] Find cycles about blocks Reviewed By: mbouaziz Differential Revision: D6835738 fbshipit-source-id: 5d46a5c --- infer/src/IR/Sil.ml | 3 - infer/src/IR/Sil.mli | 3 - infer/src/backend/RetainCycles.ml | 105 ++++++++++++------ infer/src/backend/RetainCyclesType.ml | 31 ++++-- infer/src/backend/RetainCyclesType.mli | 4 +- infer/src/backend/dotty.ml | 27 ++++- .../tests/codetoanalyze/objc/errors/Makefile | 1 + .../objc/errors/blocks_in_heap/BlockInHeap.m | 8 +- .../codetoanalyze/objc/errors/issues.exp | 4 +- .../RetainCycleBlocks.m | 45 ++++++++ 10 files changed, 168 insertions(+), 63 deletions(-) create mode 100644 infer/tests/codetoanalyze/objc/errors/memory_leaks_benchmark/RetainCycleBlocks.m diff --git a/infer/src/IR/Sil.ml b/infer/src/IR/Sil.ml index 4d88051e0..0aba3fb3a 100644 --- a/infer/src/IR/Sil.ml +++ b/infer/src/IR/Sil.ml @@ -431,9 +431,6 @@ let add_with_block_parameters_flag instr = (** Check if a pvar is a local pointing to a block in objc *) let is_block_pvar pvar = Typ.has_block_prefix (Mangled.to_string (Pvar.get_name pvar)) -(** A block pvar used to explain retain cycles *) -let block_pvar = Pvar.mk (Mangled.from_string "block") (Typ.Procname.from_string_c_fun "") - (** Dump an instruction. *) let d_instr (i: instr) = L.add_print_action (L.PTinstr, Obj.repr i) diff --git a/infer/src/IR/Sil.mli b/infer/src/IR/Sil.mli index 5dfdd2c89..4933ee0fe 100644 --- a/infer/src/IR/Sil.mli +++ b/infer/src/IR/Sil.mli @@ -255,9 +255,6 @@ val zero_value_of_numerical_type : Typ.t -> Exp.t val is_static_local_name : string -> Pvar.t -> bool (** Check if a pvar is a local static in objc *) -val block_pvar : Pvar.t -(** A block pvar used to explain retain cycles *) - val is_block_pvar : Pvar.t -> bool (** Check if a pvar is a local pointing to a block in objc *) diff --git a/infer/src/backend/RetainCycles.ml b/infer/src/backend/RetainCycles.ml index 8e329090e..705f3e9ed 100644 --- a/infer/src/backend/RetainCycles.ml +++ b/infer/src/backend/RetainCycles.ml @@ -17,27 +17,34 @@ let desc_retain_cycle tenv (cycle: RetainCyclesType.t) = let do_edge index_ edge = let index = index_ + 1 in let node = State.get_node () in - let from_exp_str = - match edge.rc_from.rc_node_exp with - | Exp.Lvar pvar when Pvar.equal pvar Sil.block_pvar -> - "a block" - | _ -> - match Errdesc.find_outermost_dereference tenv node edge.rc_from.rc_node_exp with - | Some de -> - DecompiledExp.to_string de - | None -> - Format.sprintf "(object of type %s)" (Typ.to_string edge.rc_from.rc_node_typ) + let from_exp_str edge_obj = + match Errdesc.find_outermost_dereference tenv node edge_obj.rc_from.rc_node_exp with + | Some de -> + DecompiledExp.to_string de + | None -> + Format.sprintf "(object of type %s)" (Typ.to_string edge_obj.rc_from.rc_node_typ) in - let inst_str inst = - let update_str_list = Localise.access_desc (ref []) (Errdesc.access_opt inst) in - if List.is_empty update_str_list then " " else ", " ^ String.concat ~sep:"," update_str_list + let location_str = + match edge with + | Object obj -> + let update_str_list = + Localise.access_desc (ref []) (Errdesc.access_opt obj.rc_field.rc_field_inst) + in + if List.is_empty update_str_list then " " + else ", " ^ String.concat ~sep:"," update_str_list + | Block -> + "" in let cycle_item_str = - Format.sprintf "%s->%s" from_exp_str (Typ.Fieldname.to_string edge.rc_field.rc_field_name) + match edge with + | Object obj -> + MF.monospaced_to_string + (Format.sprintf "%s->%s" (from_exp_str obj) + (Typ.Fieldname.to_string obj.rc_field.rc_field_name)) + | Block -> + Format.sprintf "a block" in - Format.sprintf "(%d) %s%s" index - (MF.monospaced_to_string cycle_item_str) - (inst_str edge.rc_field.rc_field_inst) + Format.sprintf "(%d) %s%s" index cycle_item_str location_str in let cycle_str = List.mapi ~f:do_edge cycle.rc_elements in List.fold_left cycle_str ~f:(fun acc s -> Format.sprintf "%s\n %s" acc s) ~init:"" @@ -55,31 +62,55 @@ let get_cycle root prop = Returns a pair (path, bool) where path is a list of edges describing the path to e_root and bool is true if e_root is reached. *) let rec dfs root_node from_node path fields visited = + let get_cycle_blocks exp = + match exp with + | Exp.Closure {captured_vars} + (* will turn on in prod when false positives have been addressed *) + when Config.debug_exceptions -> + List.find ~f:(fun (e, _, _) -> Exp.equal e root_node.rc_node_exp) captured_vars + |> Option.map ~f:snd3 + | _ -> + None + in match fields with | [] -> (path, false) | (field, Sil.Eexp (f_exp, f_inst)) :: el' -> if Exp.equal f_exp root_node.rc_node_exp then let rc_field = {rc_field_name= field; rc_field_inst= f_inst} in - let edge = {rc_from= from_node; rc_field} in + let edge = Object {rc_from= from_node; rc_field} in (edge :: path, true) - else if List.mem ~equal:Exp.equal visited f_exp then (path, false) else - let visited' = from_node.rc_node_exp :: visited in - let res = - match get_points_to f_exp with - | None -> - (path, false) - | Some Sil.Hpointsto (_, Sil.Estruct (new_fields, _), Exp.Sizeof {typ= te}) -> + let cycle_block_opt = get_cycle_blocks f_exp in + if Option.is_some cycle_block_opt then + match cycle_block_opt with + | Some var -> let rc_field = {rc_field_name= field; rc_field_inst= f_inst} in - let edge = {rc_from= from_node; rc_field} in - let rc_to = {rc_node_exp= f_exp; rc_node_typ= te} in - dfs root_node rc_to (edge :: path) new_fields visited' - | _ -> - (path, false) - (* check for lists *) - in - if snd res then res else dfs root_node from_node path el' visited' + (* From the captured variables we get the actual name of the variable + that is more useful for the error message *) + let updated_from_node = {from_node with rc_node_exp= Exp.Lvar var} in + let edge1 = Object {rc_from= updated_from_node; rc_field} in + let edge2 = Block in + (edge1 :: edge2 :: path, true) + | None -> + assert false + else if List.mem ~equal:Exp.equal visited f_exp then (path, false) + else + let visited' = from_node.rc_node_exp :: visited in + let res = + match get_points_to f_exp with + | None -> + (path, false) + | Some Sil.Hpointsto (_, Sil.Estruct (new_fields, _), Exp.Sizeof {typ= te}) -> + let rc_field = {rc_field_name= field; rc_field_inst= f_inst} in + let edge = Object {rc_from= from_node; rc_field} in + let rc_to = {rc_node_exp= f_exp; rc_node_typ= te} in + dfs root_node rc_to (edge :: path) new_fields visited' + | _ -> + (path, false) + (* check for lists *) + in + if snd res then res else dfs root_node from_node path el' visited' | _ -> (path, false) in @@ -149,8 +180,12 @@ let cycle_has_weak_or_unretained_or_assign_field tenv cycle = | [] -> false | edge :: c' -> - let ia = get_item_annotation edge.rc_from.rc_node_typ edge.rc_field.rc_field_name in - if List.exists ~f:do_annotation ia then true else do_cycle c' + match edge with + | Object obj -> + let ia = get_item_annotation obj.rc_from.rc_node_typ obj.rc_field.rc_field_name in + if List.exists ~f:do_annotation ia then true else do_cycle c' + | Block -> + false in do_cycle cycle.rc_elements diff --git a/infer/src/backend/RetainCyclesType.ml b/infer/src/backend/RetainCyclesType.ml index a7eb5985c..167e7d06b 100644 --- a/infer/src/backend/RetainCyclesType.ml +++ b/infer/src/backend/RetainCyclesType.ml @@ -14,22 +14,29 @@ type retain_cycle_field_objc = {rc_field_name: Typ.Fieldname.t; rc_field_inst: Sil.inst} [@@deriving compare] -type retain_cycle_edge = +type retain_cycle_edge_objc = {rc_from: retain_cycle_node; rc_field: retain_cycle_field_objc} [@@deriving compare] +type retain_cycle_edge = Object of retain_cycle_edge_objc | Block [@@deriving compare] + type t = {rc_elements: retain_cycle_edge list; rc_head: retain_cycle_edge} [@@deriving compare] +let is_inst_rearrange node = + match node with + | Object obj -> ( + match obj.rc_field.rc_field_inst with Sil.Irearrange _ -> true | _ -> false ) + | Block -> + false + + let create_cycle cycle = let sorted_cycle = List.sort ~cmp:compare_retain_cycle_edge cycle in match sorted_cycle with - | [hd] -> ( - match (* cycles of length 1 created at rearrange are not real *) - hd.rc_field.rc_field_inst with - | Sil.Irearrange _ -> + | [hd] -> + if is_inst_rearrange hd then (* cycles of length 1 created at rearrange are not real *) None - | _ -> - Some {rc_elements= sorted_cycle; rc_head= hd} ) + else Some {rc_elements= sorted_cycle; rc_head= hd} | hd :: _ -> Some {rc_elements= sorted_cycle; rc_head= hd} | [] -> @@ -47,9 +54,13 @@ let retain_cycle_field_to_string (field: retain_cycle_field_objc) = let retain_cycle_edge_to_string (edge: retain_cycle_edge) = - Format.sprintf "%s ->{%s}" - (retain_cycle_node_to_string edge.rc_from) - (retain_cycle_field_to_string edge.rc_field) + match edge with + | Object obj -> + Format.sprintf "%s ->{%s}" + (retain_cycle_node_to_string obj.rc_from) + (retain_cycle_field_to_string obj.rc_field) + | Block -> + Format.sprintf "a block" let retain_cycle_to_string cycle = diff --git a/infer/src/backend/RetainCyclesType.mli b/infer/src/backend/RetainCyclesType.mli index e83a7adfc..cda6d68bf 100644 --- a/infer/src/backend/RetainCyclesType.mli +++ b/infer/src/backend/RetainCyclesType.mli @@ -13,7 +13,9 @@ type retain_cycle_node = {rc_node_exp: Exp.t; rc_node_typ: Typ.t} type retain_cycle_field_objc = {rc_field_name: Typ.Fieldname.t; rc_field_inst: Sil.inst} -type retain_cycle_edge = {rc_from: retain_cycle_node; rc_field: retain_cycle_field_objc} +type retain_cycle_edge_objc = {rc_from: retain_cycle_node; rc_field: retain_cycle_field_objc} + +type retain_cycle_edge = Object of retain_cycle_edge_objc | Block (** A retain cycle is a non-empty list of paths. It also contains a pointer to the head of the list to model the cycle structure. The next element from the end of the list is the head. *) diff --git a/infer/src/backend/dotty.ml b/infer/src/backend/dotty.ml index 504737816..3311daac0 100644 --- a/infer/src/backend/dotty.ml +++ b/infer/src/backend/dotty.ml @@ -1080,18 +1080,33 @@ let pp_dotty_prop fmt (prop, cycle) = let dotty_retain_cycle_to_str prop (cycle: RetainCyclesType.t) = + let open RetainCyclesType in + let get_exp_from_edge edge = + match edge with + | Object obj -> + obj.rc_from.rc_node_exp + | Block -> + Exp.Lvar (Pvar.mk (Mangled.from_string "block") (Typ.Procname.from_string_c_fun "")) + in + let get_field_from_edge edge = + match edge with + | Object obj -> + obj.rc_field.rc_field_name + | Block -> + Typ.Fieldname.Java.from_string "" + in let open RetainCyclesType in let rec cycle_to_list elements = match elements with | edge1 :: edge2 :: rest -> - ( edge1.rc_from.rc_node_exp - , edge1.rc_field.rc_field_name - , Sil.Eexp (edge2.rc_from.rc_node_exp, Sil.Inone) ) + ( get_exp_from_edge edge1 + , get_field_from_edge edge1 + , Sil.Eexp (get_exp_from_edge edge2, Sil.Inone) ) :: cycle_to_list (edge2 :: rest) | [edge] -> - [ ( edge.rc_from.rc_node_exp - , edge.rc_field.rc_field_name - , Sil.Eexp (cycle.rc_head.rc_from.rc_node_exp, Sil.Inone) ) ] + [ ( get_exp_from_edge edge + , get_field_from_edge edge + , Sil.Eexp (get_exp_from_edge edge, Sil.Inone) ) ] | [] -> [] in diff --git a/infer/tests/codetoanalyze/objc/errors/Makefile b/infer/tests/codetoanalyze/objc/errors/Makefile index 020ec78f5..10ce17c2e 100644 --- a/infer/tests/codetoanalyze/objc/errors/Makefile +++ b/infer/tests/codetoanalyze/objc/errors/Makefile @@ -92,6 +92,7 @@ SOURCES_ARC = \ memory_leaks_benchmark/RetainReleaseExampleBucketingArc.m \ memory_leaks_benchmark/retain_cycle.m \ memory_leaks_benchmark/retain_cycle2.m \ + memory_leaks_benchmark/RetainCycleBlocks.m \ npe/BoxedNumberExample.m \ npe/ObjCMethodCallInCondition.m \ npe/UpdateDict.m \ diff --git a/infer/tests/codetoanalyze/objc/errors/blocks_in_heap/BlockInHeap.m b/infer/tests/codetoanalyze/objc/errors/blocks_in_heap/BlockInHeap.m index 7e6372f67..93881c5bf 100644 --- a/infer/tests/codetoanalyze/objc/errors/blocks_in_heap/BlockInHeap.m +++ b/infer/tests/codetoanalyze/objc/errors/blocks_in_heap/BlockInHeap.m @@ -12,7 +12,7 @@ typedef void (^BlockInHeapHandler)(BlockInHeap* name); -@property(nonatomic, strong) BlockInHeapHandler handler; +@property(nonatomic, weak) BlockInHeapHandler handler; @property(nonatomic, strong) BlockInHeap* child; @@ -27,8 +27,8 @@ typedef void (^BlockInHeapHandler)(BlockInHeap* name); } @end - -int block_in_heap_executed_after_bi_abduction_ok() { +// no retain cycle because handler is a weak pointer. +int block_in_heap_executed_after_bi_abduction_ok_no_retain_cycle() { BlockInHeap* c = [[BlockInHeap alloc] init]; [c assign_block_to_ivar]; BlockInHeap* b = [[BlockInHeap alloc] init]; @@ -37,7 +37,7 @@ int block_in_heap_executed_after_bi_abduction_ok() { } int block_in_heap_executed_after_bi_abduction_ok_test() { - if (block_in_heap_executed_after_bi_abduction_ok() == 5) { + if (block_in_heap_executed_after_bi_abduction_ok_no_retain_cycle() == 5) { int* p = 0; return *p; } else { diff --git a/infer/tests/codetoanalyze/objc/errors/issues.exp b/infer/tests/codetoanalyze/objc/errors/issues.exp index a8cc7ee70..471f4fd82 100644 --- a/infer/tests/codetoanalyze/objc/errors/issues.exp +++ b/infer/tests/codetoanalyze/objc/errors/issues.exp @@ -3,6 +3,7 @@ codetoanalyze/objc/errors/global_const/global_const.m, SimpleRoot_doSomethingBad codetoanalyze/objc/errors/global_const/global_const.m, SimpleRoot_doSomethingOkWithDict:andString:, 3, NULL_DEREFERENCE, [start of procedure doSomethingOkWithDict:andString:,Message stringByAppendingString: with receiver nil returns nil.] codetoanalyze/objc/errors/initialization/compound_literal.c, init_with_compound_literal, 2, DIVIDE_BY_ZERO, [start of procedure init_with_compound_literal()] codetoanalyze/objc/errors/memory_leaks_benchmark/CADisplayLinkRetainCycle.m, testCycle, 3, RETAIN_CYCLE, [start of procedure testCycle(),start of procedure init,return from a call to CADisplay_init] +codetoanalyze/objc/errors/memory_leaks_benchmark/RetainCycleStaticVar.m, RetainCSVycleStaticVar, 2, RETAIN_CYCLE, [start of procedure RetainCSVycleStaticVar(),start of procedure init,return from a call to RetainCSV_init,start of procedure foo,return from a call to RetainCSV_foo] codetoanalyze/objc/errors/npe/null_returned_by_method.m, NullReturnedByMethodA_test1, 1, NULL_DEREFERENCE, [start of procedure test1,start of procedure test,return from a call to NullReturnedByMethodA_test] codetoanalyze/objc/errors/procdescs/main.c, ProcdescMain, 3, MEMORY_LEAK, [start of procedure ProcdescMain(),Skipping plusX:andY:: function or method not found] codetoanalyze/objc/errors/procdescs/main.c, call_nslog, 3, MEMORY_LEAK, [start of procedure call_nslog(),Skipping NSLog(): function or method not found] @@ -22,13 +23,14 @@ codetoanalyze/objc/shared/block/block_no_args.m, My_manager_m, 10, NULL_DEREFERE codetoanalyze/objc/shared/block/block_release.m, My_manager_blockReleaseTODO, 9, MEMORY_LEAK, [start of procedure blockReleaseTODO] codetoanalyze/objc/shared/category_procdesc/main.c, CategoryProcdescMain, 3, MEMORY_LEAK, [start of procedure CategoryProcdescMain(),Skipping performDaysWork: function or method not found] codetoanalyze/objc/shared/field_superclass/SuperExample.m, ASuper_init, 2, NULL_DEREFERENCE, [start of procedure init,start of procedure init,return from a call to BSuper_init] -codetoanalyze/objc/errors/blocks_in_heap/BlockInHeap.m, block_in_heap_executed_after_bi_abduction_ok_test, 3, NULL_DEREFERENCE, [start of procedure block_in_heap_executed_after_bi_abduction_ok_test(),start of procedure block_in_heap_executed_after_bi_abduction_ok(),start of procedure assign_block_to_ivar,return from a call to BlockInHeap_assign_block_to_ivar,start of procedure block,return from a call to objc_blockBlockInHeap_assign_block_to_ivar_1,return from a call to block_in_heap_executed_after_bi_abduction_ok,Condition is true] +codetoanalyze/objc/errors/blocks_in_heap/BlockInHeap.m, block_in_heap_executed_after_bi_abduction_ok_test, 3, NULL_DEREFERENCE, [start of procedure block_in_heap_executed_after_bi_abduction_ok_test(),start of procedure block_in_heap_executed_after_bi_abduction_ok_no_retain_cycle(),start of procedure assign_block_to_ivar,return from a call to BlockInHeap_assign_block_to_ivar,start of procedure block,return from a call to objc_blockBlockInHeap_assign_block_to_ivar_1,return from a call to block_in_heap_executed_after_bi_abduction_ok_no_retain_cycle,Condition is true] codetoanalyze/objc/errors/field_superclass/SubtypingExample.m, Employee_initWithName:andAge:andEducation:, 3, NULL_TEST_AFTER_DEREFERENCE, [start of procedure initWithName:andAge:andEducation:,start of procedure initWithName:andAge:,return from a call to Person_initWithName:andAge:,Condition is false] codetoanalyze/objc/errors/field_superclass/SubtypingExample.m, Employee_initWithName:andAge:andEducation:, 6, DIVIDE_BY_ZERO, [start of procedure initWithName:andAge:andEducation:,start of procedure initWithName:andAge:,return from a call to Person_initWithName:andAge:,Condition is true] codetoanalyze/objc/errors/field_superclass/SubtypingExample.m, subtyping_test, 0, DIVIDE_BY_ZERO, [start of procedure subtyping_test(),start of procedure testFields(),start of procedure setEmployeeEducation:,return from a call to Employee_setEmployeeEducation:,start of procedure setAge:,return from a call to Person_setAge:,start of procedure setEmployeeEducation:,return from a call to Employee_setEmployeeEducation:,start of procedure getAge,return from a call to Person_getAge,return from a call to testFields] codetoanalyze/objc/errors/initialization/struct_initlistexpr.c, field_set_correctly, 2, DIVIDE_BY_ZERO, [start of procedure field_set_correctly()] codetoanalyze/objc/errors/initialization/struct_initlistexpr.c, implicit_expr_set_correctly, 3, DIVIDE_BY_ZERO, [start of procedure implicit_expr_set_correctly()] codetoanalyze/objc/errors/initialization/struct_initlistexpr.c, point_coords_set_correctly, 2, DIVIDE_BY_ZERO, [start of procedure point_coords_set_correctly()] +codetoanalyze/objc/errors/memory_leaks_benchmark/RetainCycleBlocks.m, main, 2, RETAIN_CYCLE, [start of procedure main(),start of procedure retain_self_in_block,return from a call to RCBlock_retain_self_in_block] codetoanalyze/objc/errors/memory_leaks_benchmark/retain_cycle.m, strongcycle, 6, RETAIN_CYCLE, [start of procedure strongcycle()] codetoanalyze/objc/errors/memory_leaks_benchmark/retain_cycle2.m, strongcycle2, 4, RETAIN_CYCLE, [start of procedure strongcycle2(),start of procedure init,return from a call to Parent_init,start of procedure init,return from a call to Child_init,start of procedure setChild:,return from a call to Parent_setChild:,start of procedure setParent:,return from a call to Child_setParent:] codetoanalyze/objc/errors/npe/UpdateDict.m, add_nil_in_dict, 10, NULL_DEREFERENCE, [start of procedure add_nil_in_dict(),Skipping dictionaryWithObjectsAndKeys:: function or method not found] diff --git a/infer/tests/codetoanalyze/objc/errors/memory_leaks_benchmark/RetainCycleBlocks.m b/infer/tests/codetoanalyze/objc/errors/memory_leaks_benchmark/RetainCycleBlocks.m new file mode 100644 index 000000000..e64000e19 --- /dev/null +++ b/infer/tests/codetoanalyze/objc/errors/memory_leaks_benchmark/RetainCycleBlocks.m @@ -0,0 +1,45 @@ +/* + * 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. + */ +#import + +@class RCBlock; + +typedef void (^MyHandler)(RCBlock* name); + +@interface RCBlock : NSObject { + RCBlock* child; +} + +@property(nonatomic, strong) MyHandler handler; + +@property(nonatomic, strong) RCBlock* child; + +@end + +@implementation RCBlock + +// This code can be executed and one can check that there's a cycle because +// "Dealloc" is not being printed. +- (void)dealloc { + NSLog(@"Dealloc"); +} + +- (void)retain_self_in_block { + self.handler = ^(RCBlock* b) { + self->_child = b; + }; +} + +@end + +int main() { + RCBlock* c = [[RCBlock alloc] init]; + [c retain_self_in_block]; + return 0; +}