From 63428e7b69f3b29aa67c20ecbf7efc5a1e1c9ad6 Mon Sep 17 00:00:00 2001 From: Dulma Churchill Date: Mon, 3 Feb 2020 02:54:01 -0800 Subject: [PATCH] [SelfInBlock] Check for when weakSelf is used in a no escaping block Summary: This adds a check for when developers use weakSelf (and maybe strongSelf) in a block, when there is no need for it, because it won't cause a retain cycle. In general there is an annotation in Objective-C for methods that take blocks, NS_NOESCAPE, that means that the passed block won't leave the scope, i.e. is "no escaping". So we report when weakSelf is used and the block has the annotation. Reviewed By: ngorogiannis Differential Revision: D19662468 fbshipit-source-id: f5ac695aa --- 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 | 23 ++++++- .../objc/self-in-block/NoescapeBlock.m | 65 +++++++++++++++++++ .../objc/self-in-block/issues.exp | 1 + 8 files changed, 95 insertions(+), 1 deletion(-) create mode 100644 infer/tests/codetoanalyze/objc/self-in-block/NoescapeBlock.m diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt index 463a7d349..c0f943d7c 100644 --- a/infer/man/man1/infer-full.txt +++ b/infer/man/man1/infer-full.txt @@ -515,6 +515,7 @@ OPTIONS USE_AFTER_LIFETIME (enabled by default), Unknown_proc (enabled by default), VECTOR_INVALIDATION (enabled by default), + WEAK_SELF_IN_NO_ESCAPE_BLOCK (enabled by default), Wrong_argument_number (enabled by default), ZERO_ALLOCATION (disabled by default), ZERO_EXECUTION_TIME (disabled by default). diff --git a/infer/man/man1/infer-report.txt b/infer/man/man1/infer-report.txt index cd983af7c..bb2573bb7 100644 --- a/infer/man/man1/infer-report.txt +++ b/infer/man/man1/infer-report.txt @@ -249,6 +249,7 @@ OPTIONS USE_AFTER_LIFETIME (enabled by default), Unknown_proc (enabled by default), VECTOR_INVALIDATION (enabled by default), + WEAK_SELF_IN_NO_ESCAPE_BLOCK (enabled by default), Wrong_argument_number (enabled by default), ZERO_ALLOCATION (disabled by default), ZERO_EXECUTION_TIME (disabled by default). diff --git a/infer/man/man1/infer.txt b/infer/man/man1/infer.txt index b44f021a1..de44cc748 100644 --- a/infer/man/man1/infer.txt +++ b/infer/man/man1/infer.txt @@ -515,6 +515,7 @@ OPTIONS USE_AFTER_LIFETIME (enabled by default), Unknown_proc (enabled by default), VECTOR_INVALIDATION (enabled by default), + WEAK_SELF_IN_NO_ESCAPE_BLOCK (enabled by default), Wrong_argument_number (enabled by default), ZERO_ALLOCATION (disabled by default), ZERO_EXECUTION_TIME (disabled by default). diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index 64d918ed1..809bdac00 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -498,6 +498,8 @@ let untrusted_variable_length_array = register_from_string "UNTRUSTED_VARIABLE_L let vector_invalidation = register_from_string "VECTOR_INVALIDATION" +let weak_self_in_noescape_block = register_from_string "WEAK_SELF_IN_NO_ESCAPE_BLOCK" + let wrong_argument_number = register_from_string "Wrong_argument_number" ~hum:"Wrong Argument Number" diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index b7a21035a..b7e06008a 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -347,6 +347,8 @@ val user_controlled_sql_risk : t val vector_invalidation : t +val weak_self_in_noescape_block : t + val wrong_argument_number : t val zero_cost_call : kind:CostKind.t -> t diff --git a/infer/src/checkers/SelfInBlock.ml b/infer/src/checkers/SelfInBlock.ml index 00e96d3c5..9433d83ad 100644 --- a/infer/src/checkers/SelfInBlock.ml +++ b/infer/src/checkers/SelfInBlock.ml @@ -398,6 +398,25 @@ let report_mix_self_weakself_issues summary domain = () +let report_weakself_in_no_escape_block_issues summary domain attributes = + if attributes.ProcAttributes.is_no_escape_block then + let weakSelf_opt = + Vars.filter (fun _ {kind} -> DomainData.is_weak_self kind) domain |> Vars.choose_opt + in + match weakSelf_opt with + | Some (_, {pvar= weakSelf; loc= weakLoc}) -> + let message = + F.asprintf + "This block uses `%a` at %a. This is probably not needed since the block is passed to \ + a method in a position annotated with NS_NOESCAPE. Use `self` instead." + (Pvar.pp Pp.text) weakSelf Location.pp weakLoc + in + let ltr = make_trace_use_self_weakself domain in + Reporting.log_error summary ~ltr ~loc:weakLoc IssueType.weak_self_in_noescape_block message + | _ -> + () + + let report_weakself_multiple_issues summary domain = let weakSelfs = Vars.filter (fun _ {kind} -> DomainData.is_weak_self kind) domain @@ -467,12 +486,14 @@ let checker {Callbacks.exe_env; summary} = 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 + let attributes = Summary.get_attributes summary in ( if Procname.is_objc_block procname then match Analyzer.compute_post proc_data ~initial with | Some domain -> report_mix_self_weakself_issues summary domain.vars ; report_weakself_multiple_issues summary domain.vars ; - report_captured_strongself_issues summary domain.vars + report_captured_strongself_issues summary domain.vars ; + report_weakself_in_no_escape_block_issues summary domain.vars attributes | None -> () ) ; summary diff --git a/infer/tests/codetoanalyze/objc/self-in-block/NoescapeBlock.m b/infer/tests/codetoanalyze/objc/self-in-block/NoescapeBlock.m new file mode 100644 index 000000000..2d1fe153f --- /dev/null +++ b/infer/tests/codetoanalyze/objc/self-in-block/NoescapeBlock.m @@ -0,0 +1,65 @@ +/* + * 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. + */ + +#import + +@interface B : NSObject +@end + +@interface ArrayUtils : NSObject + ++ (void)enumerate:(void (^)(id obj, NSUInteger idx, BOOL* stop))block; +@end + +@interface A : NSObject +- (NSMutableArray*)allResultsList:(NSArray*)allResults; + +- (B*)process:(B*)obj; +@end + +@implementation A + +- (B*)process:(B*)obj { + return obj; +} + +- (NSMutableArray*)weak_in_noescape_block_bad:(NSArray*)allResults { + NSMutableArray* resultsList = [[NSMutableArray alloc] init]; + __weak __typeof(self) weakSelf = self; + [allResults enumerateObjectsUsingBlock:^(id obj, NSUInteger idx, BOOL* stop) { + B* result = [weakSelf process:obj]; // bug + if (result != nil) { + [resultsList addObject:result]; + } + }]; + return resultsList; +} + +- (NSMutableArray*)weak_in_noescape_block_good:(NSArray*)allResults { + NSMutableArray* resultsList = [[NSMutableArray alloc] init]; + [allResults enumerateObjectsUsingBlock:^(id obj, NSUInteger idx, BOOL* stop) { + B* result = [self process:obj]; // no bug + if (result != nil) { + [resultsList addObject:result]; + } + }]; + return resultsList; +} + +- (NSMutableArray*)weak_in_noescape_block1_good:(NSArray*)allResults { + NSMutableArray* resultsList = [[NSMutableArray alloc] init]; + __weak __typeof(self) weakSelf = self; + [ArrayUtils enumerate:^(id obj, NSUInteger idx, BOOL* stop) { + B* result = [weakSelf process:obj]; // no bug + if (result != nil) { + [resultsList addObject:result]; + } + }]; + return resultsList; +} + +@end diff --git a/infer/tests/codetoanalyze/objc/self-in-block/issues.exp b/infer/tests/codetoanalyze/objc/self-in-block/issues.exp index 9a77243d5..3209cad21 100644 --- a/infer/tests/codetoanalyze/objc/self-in-block/issues.exp +++ b/infer/tests/codetoanalyze/objc/self-in-block/issues.exp @@ -1,3 +1,4 @@ +codetoanalyze/objc/self-in-block/NoescapeBlock.m, objc_blockA::weak_in_noescape_block_bad:_1, 1, WEAK_SELF_IN_NO_ESCAPE_BLOCK, no_bucket, ERROR, [Using &weakSelf] codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::mixSelfWeakSelf_bad_1, 5, MIXED_SELF_WEAKSELF, no_bucket, ERROR, [Using &weakSelf,Using &self,Using &self] codetoanalyze/objc/self-in-block/StrongSelf.m, objc_blockSelfInBlockTest::strongSelfCheck3_bad_7, 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::strongSelfCheckOnce_bad_5, 6, STRONG_SELF_NOT_CHECKED, no_bucket, ERROR, [&strongSelf assigned here,Using &strongSelf not checked for null,Using &strongSelf not checked for null]