From f69d0992bc6b37f63480566fb74c3f5ed6e8e702 Mon Sep 17 00:00:00 2001 From: Dulma Churchill Date: Tue, 3 Dec 2019 01:52:49 -0800 Subject: [PATCH] [self-in-block] Add new issue type MULTIPLE_WEAKSELF Summary: Raises an error when weakSelf is used multiple times in a block. The issue is that there could be unexpected behaviour. Even if `weakSelf` is not nil in the first use, it could be nil in the following uses since the object that `weakSelf` points to could be freed anytime. Reviewed By: skcho Differential Revision: D18765493 fbshipit-source-id: 37fc652e3 --- infer/man/man1/infer-full.txt | 1 + infer/man/man1/infer-report.txt | 1 + infer/man/man1/infer.txt | 1 + infer/src/base/IssueType.ml | 2 ++ infer/src/base/IssueType.mli | 2 ++ infer/src/checkers/SelfInBlock.ml | 33 +++++++++++++++++-- .../objc/self-in-block/StrongSelf.m | 13 ++++++++ .../objc/self-in-block/issues.exp | 1 + 8 files changed, 51 insertions(+), 3 deletions(-) diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt index 33e320cc1..2d028401d 100644 --- a/infer/man/man1/infer-full.txt +++ b/infer/man/man1/infer-full.txt @@ -449,6 +449,7 @@ OPTIONS MEMORY_LEAK (enabled by default), MISSING_REQUIRED_PROP (enabled by default), MIXED_SELF_WEAKSELF (enabled by default), + MULTIPLE_WEAKSELF (enabled by default), MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE (enabled by default), Missing_fld (enabled by default), NULLPTR_DEREFERENCE (disabled by default), diff --git a/infer/man/man1/infer-report.txt b/infer/man/man1/infer-report.txt index 94942c967..609027274 100644 --- a/infer/man/man1/infer-report.txt +++ b/infer/man/man1/infer-report.txt @@ -191,6 +191,7 @@ OPTIONS MEMORY_LEAK (enabled by default), MISSING_REQUIRED_PROP (enabled by default), MIXED_SELF_WEAKSELF (enabled by default), + MULTIPLE_WEAKSELF (enabled by default), MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE (enabled by default), Missing_fld (enabled by default), NULLPTR_DEREFERENCE (disabled by default), diff --git a/infer/man/man1/infer.txt b/infer/man/man1/infer.txt index dab6370f9..563bd021d 100644 --- a/infer/man/man1/infer.txt +++ b/infer/man/man1/infer.txt @@ -449,6 +449,7 @@ OPTIONS MEMORY_LEAK (enabled by default), MISSING_REQUIRED_PROP (enabled by default), MIXED_SELF_WEAKSELF (enabled by default), + MULTIPLE_WEAKSELF (enabled by default), MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE (enabled by default), Missing_fld (enabled by default), NULLPTR_DEREFERENCE (disabled by default), diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index 55511925c..9c9dc19cd 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -362,6 +362,8 @@ let missing_required_prop = register_from_string "MISSING_REQUIRED_PROP" let mixed_self_weakself = register_from_string "MIXED_SELF_WEAKSELF" ~hum:"Mixed Self WeakSelf" +let multiple_weakself = register_from_string "MULTIPLE_WEAKSELF" ~hum:"Multiple WeakSelf Use" + let mutable_local_variable_in_component_file = register_from_string "MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE" diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index c54a4dc13..6a047c3ef 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -235,6 +235,8 @@ val missing_required_prop : t val mixed_self_weakself : t +val multiple_weakself : t + val mutable_local_variable_in_component_file : t val null_dereference : t diff --git a/infer/src/checkers/SelfInBlock.ml b/infer/src/checkers/SelfInBlock.ml index 390d01a3a..6b22c8ad2 100644 --- a/infer/src/checkers/SelfInBlock.ml +++ b/infer/src/checkers/SelfInBlock.ml @@ -210,7 +210,7 @@ module TransferFunctions = struct astate end -let make_trace_mixed_self_weakself domain = +let make_trace_use_self_weakself domain = let trace_elems = Vars.fold (fun _ {pvar; loc; kind} trace_elems -> @@ -240,12 +240,38 @@ let report_mix_self_weakself_issues summary domain = unexpected behavior." (Pvar.pp Pp.text) weakSelf Location.pp weakLoc (Pvar.pp Pp.text) self Location.pp selfLoc in - let ltr = make_trace_mixed_self_weakself domain in + let ltr = make_trace_use_self_weakself domain in Reporting.log_error summary ~ltr ~loc:selfLoc IssueType.mixed_self_weakself message | _ -> () +let report_weakself_multiple_issues summary domain = + let weakSelfs = + Vars.filter (fun _ {kind} -> DomainData.is_weak_self kind) domain + |> Vars.bindings |> List.map ~f:snd + in + let sorted_WeakSelfs = + List.sort weakSelfs ~compare:(fun {DomainData.loc= loc1} {DomainData.loc= loc2} -> + Location.compare loc1 loc2 ) + in + match sorted_WeakSelfs with + | {pvar= weakSelf; loc= weakLoc1} :: {loc= weakLoc2} :: _ -> + let message = + F.asprintf + "This block uses the weak pointer `%a` more than once (%a) and (%a). This could lead to \ + unexpected behavior. Even if `%a` is not nil in the first use, it could be nil in the \ + following uses since the object that `%a` points to could be freed anytime; assign it \ + to a strong variable first." + (Pvar.pp Pp.text) weakSelf Location.pp weakLoc1 Location.pp weakLoc2 (Pvar.pp Pp.text) + weakSelf (Pvar.pp Pp.text) weakSelf + in + let ltr = make_trace_use_self_weakself domain in + Reporting.log_error summary ~ltr ~loc:weakLoc1 IssueType.multiple_weakself message + | _ -> + () + + let make_trace_unchecked_strongself (domain : Domain.t) = let trace_elems_strongVars = StrongEqualToWeakCapturedVars.fold @@ -304,7 +330,8 @@ let checker {Callbacks.exe_env; summary} = match Analyzer.compute_post proc_data ~initial with | Some domain -> report_mix_self_weakself_issues summary domain.vars ; - report_unchecked_strongself_issues summary domain + report_unchecked_strongself_issues summary domain ; + report_weakself_multiple_issues summary domain.vars | None -> () ) ; summary diff --git a/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m b/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m index d14c37c0e..2f5e73dd9 100644 --- a/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m +++ b/infer/tests/codetoanalyze/objc/self-in-block/StrongSelf.m @@ -11,6 +11,8 @@ - (void)foo; +- (void)bar; + @end @implementation SelfInBlockTest { @@ -20,6 +22,9 @@ - (void)foo { } +- (void)bar { +} + - (void)mixSelfWeakSelf_bad { __weak __typeof(self) weakSelf = self; int (^my_block)() = ^() { @@ -116,4 +121,12 @@ }; } +- (void)wekSelfMultiple_bad { + __weak __typeof(self) weakSelf = self; + int (^my_block)(BOOL) = ^(BOOL isTapped) { + [weakSelf foo]; + [weakSelf bar]; + 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 index 9bab105c5..0cd1a926b 100644 --- a/infer/tests/codetoanalyze/objc/self-in-block/issues.exp +++ b/infer/tests/codetoanalyze/objc/self-in-block/issues.exp @@ -3,3 +3,4 @@ codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strong codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfCheckOnce_bad_5, 8, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, [&strongSelf assigned here,Using &strongSelf not checked for null,Using &strongSelf not checked for null] codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfNoCheck2_bad_4, 2, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, [&strongSelf assigned here,Using &strongSelf not checked for null] codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfNoCheck_bad_3, 2, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, [&strongSelf assigned here,Using &strongSelf not checked for null] +codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::wekSelfMultiple_bad_9, 1, MULTIPLE_WEAKSELF, no_bucket, ERROR, [Using &weakSelf,Using &weakSelf]