From 43823266ec958a484230f3d2360ab9b7543a7694 Mon Sep 17 00:00:00 2001 From: Dulma Churchill Date: Wed, 6 Nov 2019 03:43:45 -0800 Subject: [PATCH] [self in block] Add a new checker to detect correct uses of when ObjC blocks capture self. Reviewed By: skcho Differential Revision: D18245267 fbshipit-source-id: 6e3f1a7f7 --- Makefile | 1 + infer/man/man1/infer-analyze.txt | 12 ++- infer/man/man1/infer-full.txt | 13 ++- infer/man/man1/infer-report.txt | 1 + infer/man/man1/infer.txt | 13 ++- infer/src/IR/Typ.ml | 4 + infer/src/IR/Typ.mli | 4 + infer/src/base/Config.ml | 8 ++ infer/src/base/Config.mli | 2 + infer/src/base/IssueType.ml | 2 + infer/src/base/IssueType.mli | 2 + infer/src/checkers/SelfInBlock.ml | 101 ++++++++++++++++++ infer/src/checkers/SelfInBlock.mli | 10 ++ infer/src/checkers/registerCheckers.ml | 5 +- .../codetoanalyze/objc/self-in-block/Makefile | 17 +++ .../objc/self-in-block/StrongSelf.m | 35 ++++++ .../objc/self-in-block/issues.exp | 1 + 17 files changed, 224 insertions(+), 7 deletions(-) create mode 100644 infer/src/checkers/SelfInBlock.ml create mode 100644 infer/src/checkers/SelfInBlock.mli create mode 100644 infer/tests/codetoanalyze/objc/self-in-block/Makefile create mode 100644 infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m create mode 100644 infer/tests/codetoanalyze/objc/self-in-block/issues.exp diff --git a/Makefile b/Makefile index 37952a1ef..49c375a25 100644 --- a/Makefile +++ b/Makefile @@ -111,6 +111,7 @@ DIRECT_TESTS += \ objc_performance \ objc_pulse \ objc_quandary \ + objc_self-in-block \ objc_uninit \ objcpp_errors \ objcpp_frontend \ diff --git a/infer/man/man1/infer-analyze.txt b/infer/man/man1/infer-analyze.txt index 5baee5091..019df0480 100644 --- a/infer/man/man1/infer-analyze.txt +++ b/infer/man/man1/infer-analyze.txt @@ -103,8 +103,8 @@ OPTIONS --no-default-checkers Deactivates: Default checkers: --biabduction, --fragment-retains-view, --inefficient-keyset-iterator, --linters, - --liveness, --racerd, --siof, --starvation, --uninit (Conversely: - --default-checkers) + --liveness, --racerd, --siof, --starvation, --self_in_block, + --uninit (Conversely: --default-checkers) --eradicate Activates: the eradicate @Nullable checker for Java annotations @@ -308,6 +308,14 @@ OPTIONS --results-dir,-o dir Write results and internal files in the specified directory + --no-self_in_block + Deactivates: checker to flag incorrect uses of when Objective-C + blocks capture self (Conversely: --self_in_block) + + --self_in_block-only + Activates: Enable --self_in_block and disable all other checkers + (Conversely: --no-self_in_block-only) + --no-siof Deactivates: the Static Initialization Order Fiasco analysis (C++ only) (Conversely: --siof) diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt index e387d1991..97ae4bed2 100644 --- a/infer/man/man1/infer-full.txt +++ b/infer/man/man1/infer-full.txt @@ -310,8 +310,8 @@ OPTIONS --no-default-checkers Deactivates: Default checkers: --biabduction, --fragment-retains-view, --inefficient-keyset-iterator, --linters, - --liveness, --racerd, --siof, --starvation, --uninit (Conversely: - --default-checkers) See also infer-analyze(1). + --liveness, --racerd, --siof, --starvation, --self_in_block, + --uninit (Conversely: --default-checkers) See also infer-analyze(1). --no-default-linters Deactivates: Use the default linters for the analysis. @@ -440,6 +440,7 @@ OPTIONS Leak_in_footprint (enabled by default), MEMORY_LEAK (enabled by default), MISSING_REQUIRED_PROP (enabled by default), + MIXED_SELF_WEAKSELF (enabled by default), MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE (enabled by default), Missing_fld (enabled by default), NULLSAFE_FIELD_NOT_NULLABLE (enabled by default), @@ -990,6 +991,14 @@ OPTIONS --select N Select bug number N. If omitted, prompt for input. See also infer-explore(1). + --no-self_in_block + Deactivates: checker to flag incorrect uses of when Objective-C + blocks capture self (Conversely: --self_in_block) See also infer-analyze(1). + + --self_in_block-only + Activates: Enable --self_in_block and disable all other checkers + (Conversely: --no-self_in_block-only) See also infer-analyze(1). + --no-siof Deactivates: the Static Initialization Order Fiasco analysis (C++ only) (Conversely: --siof) See also infer-analyze(1). diff --git a/infer/man/man1/infer-report.txt b/infer/man/man1/infer-report.txt index 6c718ec04..7416bc324 100644 --- a/infer/man/man1/infer-report.txt +++ b/infer/man/man1/infer-report.txt @@ -182,6 +182,7 @@ OPTIONS Leak_in_footprint (enabled by default), MEMORY_LEAK (enabled by default), MISSING_REQUIRED_PROP (enabled by default), + MIXED_SELF_WEAKSELF (enabled by default), MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE (enabled by default), Missing_fld (enabled by default), NULLSAFE_FIELD_NOT_NULLABLE (enabled by default), diff --git a/infer/man/man1/infer.txt b/infer/man/man1/infer.txt index 1cece97f4..34a2e6eae 100644 --- a/infer/man/man1/infer.txt +++ b/infer/man/man1/infer.txt @@ -310,8 +310,8 @@ OPTIONS --no-default-checkers Deactivates: Default checkers: --biabduction, --fragment-retains-view, --inefficient-keyset-iterator, --linters, - --liveness, --racerd, --siof, --starvation, --uninit (Conversely: - --default-checkers) See also infer-analyze(1). + --liveness, --racerd, --siof, --starvation, --self_in_block, + --uninit (Conversely: --default-checkers) See also infer-analyze(1). --no-default-linters Deactivates: Use the default linters for the analysis. @@ -440,6 +440,7 @@ OPTIONS Leak_in_footprint (enabled by default), MEMORY_LEAK (enabled by default), MISSING_REQUIRED_PROP (enabled by default), + MIXED_SELF_WEAKSELF (enabled by default), MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE (enabled by default), Missing_fld (enabled by default), NULLSAFE_FIELD_NOT_NULLABLE (enabled by default), @@ -990,6 +991,14 @@ OPTIONS --select N Select bug number N. If omitted, prompt for input. See also infer-explore(1). + --no-self_in_block + Deactivates: checker to flag incorrect uses of when Objective-C + blocks capture self (Conversely: --self_in_block) See also infer-analyze(1). + + --self_in_block-only + Activates: Enable --self_in_block and disable all other checkers + (Conversely: --no-self_in_block-only) See also infer-analyze(1). + --no-siof Deactivates: the Static Initialization Order Fiasco analysis (C++ only) (Conversely: --siof) See also infer-analyze(1). diff --git a/infer/src/IR/Typ.ml b/infer/src/IR/Typ.ml index 2a22ac441..7d7fe8f1a 100644 --- a/infer/src/IR/Typ.ml +++ b/infer/src/IR/Typ.ml @@ -246,6 +246,10 @@ let is_restrict {is_restrict} = is_restrict let is_volatile {is_volatile} = is_volatile +let is_weak_pointer t = match t.desc with Tptr (_, Pk_objc_weak) -> true | _ -> false + +let is_strong_pointer t = match t.desc with Tptr (_, Pk_pointer) -> true | _ -> false + let mk ?default ?quals desc : t = let default_ = {desc; quals= mk_type_quals ()} in let mk_aux ?(default = default_) ?(quals = default.quals) desc = {desc; quals} in diff --git a/infer/src/IR/Typ.mli b/infer/src/IR/Typ.mli index ea943128a..808403376 100644 --- a/infer/src/IR/Typ.mli +++ b/infer/src/IR/Typ.mli @@ -150,6 +150,10 @@ val get_ikind_opt : t -> ikind option val size_t : ikind (** ikind of size_t *) +val is_weak_pointer : t -> bool + +val is_strong_pointer : t -> bool + module Name : sig (** Named types. *) type t = name [@@deriving compare] diff --git a/infer/src/base/Config.ml b/infer/src/base/Config.ml index aa9ff0fd1..44ed2eae6 100644 --- a/infer/src/base/Config.ml +++ b/infer/src/base/Config.ml @@ -35,6 +35,7 @@ type checkers = ; liveness: bool ref ; loop_hoisting: bool ref ; nullsafe: bool ref + ; self_in_block: bool ref ; printf_args: bool ref ; pulse: bool ref ; purity: bool ref @@ -649,6 +650,7 @@ and { annotation_reachability ; liveness ; loop_hoisting ; nullsafe + ; self_in_block ; printf_args ; pulse ; purity @@ -726,6 +728,9 @@ and { annotation_reachability mk_checker ~long:"siof" ~default:true "the Static Initialization Order Fiasco analysis (C++ only)" and starvation = mk_checker ~long:"starvation" ~default:true "starvation analysis" + and self_in_block = + mk_checker ~long:"self_in_block" ~default:true + "checker to flag incorrect uses of when Objective-C blocks capture self" and uninit = mk_checker ~long:"uninit" "checker for use of uninitialized values" ~default:true in let mk_only (var, long, doc, _) = let (_ : bool ref) = @@ -785,6 +790,7 @@ and { annotation_reachability ; quandaryBO ; racerd ; resource_leak + ; self_in_block ; siof ; starvation ; uninit } @@ -3005,6 +3011,8 @@ and only_footprint = !only_footprint and only_show = !only_show +and self_in_block = !self_in_block + and passthroughs = !passthroughs and patterns_modeled_expensive = match patterns_modeled_expensive with k, r -> (k, !r) diff --git a/infer/src/base/Config.mli b/infer/src/base/Config.mli index 3676c8a7a..0428aa6e2 100644 --- a/infer/src/base/Config.mli +++ b/infer/src/base/Config.mli @@ -506,6 +506,8 @@ val only_footprint : bool val only_show : bool +val self_in_block : bool + val perf_profiler_data_file : string option val pmd_xml : bool diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index c2aef7c93..6ec187012 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -411,6 +411,8 @@ let strict_mode_violation = register_from_string "STRICT_MODE_VIOLATION" ~hum:"Strict Mode Violation" +let mixed_self_weakself = register_from_string "MIXED_SELF_WEAKSELF" ~hum:"Mixed Self WeakSelf" + let symexec_memory_error = register_from_string "Symexec_memory_error" ~hum:"Symbolic Execution Memory Error" diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index 55f0bbf2e..cdf30c895 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -286,6 +286,8 @@ val static_initialization_order_fiasco : t val strict_mode_violation : t +val mixed_self_weakself : t + val symexec_memory_error : t val tainted_buffer_access : t diff --git a/infer/src/checkers/SelfInBlock.ml b/infer/src/checkers/SelfInBlock.ml new file mode 100644 index 000000000..e4f340065 --- /dev/null +++ b/infer/src/checkers/SelfInBlock.ml @@ -0,0 +1,101 @@ +(* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + *) +open! IStd +module F = Format + +module DomainData = struct + type self_pointer_kind = SELF | WEAK_SELF [@@deriving compare] + + let is_self kind = match kind with SELF -> true | WEAK_SELF -> false + + let is_weak_self kind = match kind with SELF -> false | WEAK_SELF -> true + + let pp_self_pointer_kind fmt kind = + let s = match kind with SELF -> "SELF" | WEAK_SELF -> "WEAK_SELF" in + F.fprintf fmt "%s" s + + + type t = {pvar: Pvar.t; typ: Typ.t; loc: Location.t; kind: self_pointer_kind} + [@@deriving compare] + + let pp fmt {pvar; typ; loc; kind} = + F.fprintf fmt "%a:%a, at %a (%a)" (Pvar.pp Pp.text) pvar (Typ.pp Pp.text) typ Location.pp loc + pp_self_pointer_kind kind +end + +module TransferFunctions = struct + module Domain = AbstractDomain.FiniteSet (DomainData) + module CFG = ProcCfg.Normal + + type extras = unit + + let pp_session_name _node fmt = F.pp_print_string fmt "SelfCapturedInBlock" + + let is_captured_strong_self attributes pvar = + List.exists + ~f:(fun (captured, typ) -> + Mangled.equal captured (Pvar.get_name pvar) + && Pvar.is_self pvar && Typ.is_strong_pointer typ ) + attributes.ProcAttributes.captured + + + let is_captured_weak_self attributes pvar = + List.exists + ~f:(fun (captured, typ) -> + Mangled.equal captured (Pvar.get_name pvar) + && String.is_substring ~substring:"self" (String.lowercase (Mangled.to_string captured)) + && Typ.is_weak_pointer typ ) + attributes.ProcAttributes.captured + + + let exec_instr (astate : Domain.t) {ProcData.summary} _cfg_node (instr : Sil.instr) = + let attributes = Summary.get_attributes summary in + match instr with + | Load {e= Lvar pvar; loc; typ} -> + if is_captured_strong_self attributes pvar then + Domain.add {pvar; typ; loc; kind= SELF} astate + else if is_captured_weak_self attributes pvar then + Domain.add {pvar; typ; loc; kind= WEAK_SELF} astate + else astate + | _ -> + astate +end + +let report_issues summary domain = + let weakSelf_opt = + TransferFunctions.Domain.find_first_opt (fun {kind} -> DomainData.is_weak_self kind) domain + in + let self_opt = + TransferFunctions.Domain.find_first_opt (fun {kind} -> DomainData.is_self kind) domain + in + match (weakSelf_opt, self_opt) with + | Some {pvar= weakSelf; loc= weakLoc}, Some {pvar= self; loc= selfLoc} -> + let message = + F.asprintf + "This block uses both %a (%a) and %a (%a). This could lead to retain cycles or \ + unexpected behavior." + (Pvar.pp Pp.text) weakSelf Location.pp weakLoc (Pvar.pp Pp.text) self Location.pp selfLoc + in + Reporting.log_error summary ~loc:selfLoc IssueType.mixed_self_weakself message + | _ -> + () + + +module Analyzer = AbstractInterpreter.MakeWTO (TransferFunctions) + +let checker {Callbacks.exe_env; summary} = + let initial = TransferFunctions.Domain.empty in + let procname = Summary.get_proc_name summary in + let tenv = Exe_env.get_tenv exe_env procname in + let proc_data = ProcData.make summary tenv () in + ( if Typ.Procname.is_objc_block procname then + match Analyzer.compute_post proc_data ~initial with + | Some domain -> + report_issues summary domain + | None -> + () ) ; + summary diff --git a/infer/src/checkers/SelfInBlock.mli b/infer/src/checkers/SelfInBlock.mli new file mode 100644 index 000000000..489676eb8 --- /dev/null +++ b/infer/src/checkers/SelfInBlock.mli @@ -0,0 +1,10 @@ +(* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + *) + +open! IStd + +val checker : Callbacks.proc_callback_t diff --git a/infer/src/checkers/registerCheckers.ml b/infer/src/checkers/registerCheckers.ml index 5f4742018..53e2fea48 100644 --- a/infer/src/checkers/registerCheckers.ml +++ b/infer/src/checkers/registerCheckers.ml @@ -143,7 +143,10 @@ let all_checkers = [(Procedure Purity.checker, Language.Java); (Procedure Purity.checker, Language.Clang)] } ; { name= "Class loading analysis" ; active= Config.class_loads - ; callbacks= [(Procedure ClassLoads.analyze_procedure, Language.Java)] } ] + ; callbacks= [(Procedure ClassLoads.analyze_procedure, Language.Java)] } + ; { name= "Self captured in block checker" + ; active= Config.self_in_block + ; callbacks= [(Procedure SelfInBlock.checker, Language.Clang)] } ] let get_active_checkers () = diff --git a/infer/tests/codetoanalyze/objc/self-in-block/Makefile b/infer/tests/codetoanalyze/objc/self-in-block/Makefile new file mode 100644 index 000000000..f85b79b71 --- /dev/null +++ b/infer/tests/codetoanalyze/objc/self-in-block/Makefile @@ -0,0 +1,17 @@ +# Copyright (c) Facebook, Inc. and its affiliates. +# +# This source code is licensed under the MIT license found in the +# LICENSE file in the root directory of this source tree. + +TESTS_DIR = ../../.. + +CLANG_OPTIONS = -c $(OBJC_CLANG_OPTIONS) -fobjc-arc +INFER_OPTIONS = --self_in_block-only --debug-exceptions --project-root $(TESTS_DIR) +INFERPRINT_OPTIONS = --issues-tests + +SOURCES = $(wildcard *.m) + +include $(TESTS_DIR)/clang.make +include $(TESTS_DIR)/objc.make + +infer-out/report.json: $(MAKEFILE_LIST) diff --git a/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m b/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m new file mode 100644 index 000000000..f104efce7 --- /dev/null +++ b/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m @@ -0,0 +1,35 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#include + +@interface SelfInBlockTest : NSObject + +- (void)foo; + +@end + +@implementation SelfInBlockTest { + int x; +} + +- (void)foo { +} + +- (void)mixSelfWeakSelf_bad { + __weak __typeof(self) weakSelf = self; + int (^my_block)() = ^() { + __strong __typeof(weakSelf) strongSelf = weakSelf; + if (strongSelf) { + [strongSelf foo]; + int x = self->x; // bug here + } + return 0; + }; +} + +@end diff --git a/infer/tests/codetoanalyze/objc/self-in-block/issues.exp b/infer/tests/codetoanalyze/objc/self-in-block/issues.exp new file mode 100644 index 000000000..62a2d5dde --- /dev/null +++ b/infer/tests/codetoanalyze/objc/self-in-block/issues.exp @@ -0,0 +1 @@ +codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::mixSelfWeakSelf_bad_1, 4, MIXED_SELF_WEAKSELF, no_bucket, ERROR, []