Improved rule on block capturing CXX Reference

Reviewed By: jvillard

Differential Revision: D15737387

fbshipit-source-id: efe677b3d

infer/lib/linter_rules/linters.al

@@ -176,11 +176,35 @@ DEFINE-CHECKER GLOBAL_VARIABLE_INITIALIZED_WITH_FUNCTION_OR_METHOD_CALL = {
 
 
 DEFINE-CHECKER CXX_REFERENCE_CAPTURED_IN_OBJC_BLOCK = {
-	  SET report_when =
+
-		     WHEN
+		// The current node is block definition capturing a C++ reference
-				   ((is_node("BlockDecl") AND captures_cxx_references())
+		LET capture_reference =
-					 HOLDS-NEXT)
+ 			WHEN
-         HOLDS-IN-NODE BlockExpr;
+				((is_node("BlockDecl") AND captures_cxx_references()) HOLDS-NEXT)
+ 			HOLDS-IN-NODE BlockExpr;
+
+ 		// At some point we encounter a block definition capturing a C++ reference
+ 		LET block_definition_capture_reference =
+			capture_reference HOLDS-EVENTUALLY;
+
+		// A variable definition initialized with a block capturing a C++ reference
+ 		LET variable_initialized_with_block =
+		IN-NODE VarDecl WITH-TRANSITION InitExpr
+			(block_definition_capture_reference)
+ 		HOLDS-EVENTUALLY;
+
+ 		// Reference to a variable initialized with a capturing block
+ 		LET variable_block_definition =
+			IN-NODE DeclRefExpr WITH-TRANSITION PointerToDecl
+				(variable_initialized_with_block)
+			HOLDS-EVENTUALLY;
+
+		// Report when a function that does not have NoEscapeAttribute call a block or a variable definied with
+		// a block capturing a C++ ref
+		SET report_when =
+			WHEN
+				(NOT has_no_escape_attribute) AND (block_definition_capture_reference OR variable_block_definition)
+			HOLDS-IN-NODE CallExpr;
 
 	  SET message =
 	        "C++ Reference variable(s) %cxx_ref_captured_in_block% captured by Objective-C block";

infer/src/clang/cAst_utils.ml

@@ -130,6 +130,7 @@ let get_stmt_opt stmt_ptr_opt source_range =
 let get_decl_opt_with_decl_ref decl_ref_opt =
   match decl_ref_opt with
   | Some decl_ref ->
+      L.debug Capture Verbose "#####POINTER LOOK UP: '%i'@\n" decl_ref.Clang_ast_t.dr_decl_pointer ;
       get_decl decl_ref.Clang_ast_t.dr_decl_pointer
   | None ->
       None

infer/src/clang/cPredicates.ml

@@ -1402,6 +1402,30 @@ let rec get_decl_attributes_for_callexpr an =
       []
 
 
+let rec get_decl_attributes_for_callexpr_param an =
+  let open Clang_ast_t in
+  let open Ctl_parser_types in
+  let get_attr_param p = match p with ParmVarDecl (di, _, _, _) -> di.di_attributes | _ -> [] in
+  match an with
+  | Stmt (CallExpr (_, func :: _, _)) ->
+      get_decl_attributes_for_callexpr_param (Stmt func)
+  | Stmt (ImplicitCastExpr (_, [stmt], _, _)) ->
+      get_decl_attributes_for_callexpr_param (Stmt stmt)
+  | Stmt (DeclRefExpr (si, _, _, drti)) -> (
+      L.debug Linters Verbose "#####POINTER LOOP UP: '%i'@\n" si.si_pointer ;
+      match CAst_utils.get_decl_opt_with_decl_ref drti.drti_decl_ref with
+      | Some (FunctionDecl (_, _, _, fdi)) ->
+          List.fold fdi.fdi_parameters
+            ~f:(fun acc p -> List.append (get_attr_param p) acc)
+            ~init:[]
+      | Some (ParmVarDecl _ as d) ->
+          get_attr_param d
+      | _ ->
+          [] )
+  | _ ->
+      []
+
+
 let visibility_matches vis_str (visibility : Clang_ast_t.visibility_attr) =
   match (visibility, vis_str) with
   | DefaultVisibility, "Default" ->
@@ -1426,6 +1450,11 @@ let has_visibility_attribute an visibility =
   match visibility with ALVar.Const vis -> has_visibility_attr attributes vis | _ -> false
 
 
+let has_no_escape_attribute an =
+  let attributes = get_decl_attributes_for_callexpr_param an in
+  List.exists ~f:(fun attr -> match attr with `NoEscapeAttr _ -> true | _ -> false) attributes
+
+
 let has_used_attribute an =
   let attributes = get_decl_attributes_for_callexpr an in
   List.exists ~f:(fun attr -> match attr with `UsedAttr _ -> true | _ -> false) attributes

infer/src/clang/cPredicates.mli

@@ -488,6 +488,8 @@ val cxx_construct_expr_has_name : Ctl_parser_types.ast_node -> ALVar.alexp -> bo
 
 val has_used_attribute : Ctl_parser_types.ast_node -> bool
 
+val has_no_escape_attribute : Ctl_parser_types.ast_node -> bool
+
 val iphoneos_target_sdk_version_by_path : CLintersContext.context -> string option
 
 val iphoneos_target_sdk_version_greater_or_equal : CLintersContext.context -> string -> bool

infer/src/clang/cTL.ml

@@ -1191,6 +1191,8 @@ let rec eval_Atomic pred_name_ args an lcxt =
       CPredicates.has_visibility_attribute an vis
   | "has_used_attribute", [], an ->
       CPredicates.has_used_attribute an
+  | "has_no_escape_attribute", [], an ->
+      CPredicates.has_no_escape_attribute an
   | "has_unavailable_attribute", [], an ->
       CPredicates.has_unavailable_attribute an
   | "within_available_class_block", [], an ->

infer/tests/codetoanalyze/objcpp/linters/cxx_reference_in_block/block2.mm

@@ -0,0 +1,41 @@
+/*
+ * 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 <Foundation/Foundation.h>
+
+void runBlock(__attribute__((noescape)) dispatch_block_t block) { block(); }
+
+void safe() {
+
+  int x = 5;
+  int& xref = x;
+  runBlock(^{
+    xref = 6;
+  }); // safe
+}
+
+void unsafe() {
+  int x = 5;
+  int& xref = x;
+  dispatch_queue_t queue = dispatch_queue_create("queue", NULL);
+  dispatch_async(queue, ^{
+    // Infer flags the following:
+    xref = 6; // CRASH! local has gone away
+  });
+}
+
+void both_safe_and_unsafe() {
+
+  int x = 5;
+  int& xref = x;
+  dispatch_block_t my_block = ^{
+    xref = 6;
+  };
+  dispatch_queue_t queue = dispatch_queue_create("queue", NULL);
+
+  runBlock(my_block); // safe
+  dispatch_async(queue, my_block); // unsafe
+}

infer/tests/codetoanalyze/objcpp/linters/issues.exp

@@ -28,6 +28,8 @@ codetoanalyze/objcpp/linters/componentkit/MutableLocalVariablesTest.mm, SomeClas
 codetoanalyze/objcpp/linters/componentkit/UnconventionalSuperclassTest.h, Linters_dummy_method, 15, COMPONENT_WITH_UNCONVENTIONAL_SUPERCLASS, no_bucket, ADVICE, []
 codetoanalyze/objcpp/linters/cxx_reference_in_block/block.mm, A::foo, 18, CXX_REFERENCE_CAPTURED_IN_OBJC_BLOCK, no_bucket, ERROR, []
 codetoanalyze/objcpp/linters/cxx_reference_in_block/block.mm, A::foo3, 35, CXX_REFERENCE_CAPTURED_IN_OBJC_BLOCK, no_bucket, ERROR, []
+codetoanalyze/objcpp/linters/cxx_reference_in_block/block2.mm, both_safe_and_unsafe, 40, CXX_REFERENCE_CAPTURED_IN_OBJC_BLOCK, no_bucket, ERROR, []
+codetoanalyze/objcpp/linters/cxx_reference_in_block/block2.mm, unsafe, 24, CXX_REFERENCE_CAPTURED_IN_OBJC_BLOCK, no_bucket, ERROR, []
 codetoanalyze/objcpp/linters/global-var/B.mm, Linters_dummy_method, 28, GLOBAL_VARIABLE_INITIALIZED_WITH_FUNCTION_OR_METHOD_CALL, no_bucket, WARNING, []
 codetoanalyze/objcpp/linters/global-var/B.mm, Linters_dummy_method, 30, GLOBAL_VARIABLE_INITIALIZED_WITH_FUNCTION_OR_METHOD_CALL, no_bucket, WARNING, []
 codetoanalyze/objcpp/linters/global-var/B.mm, Linters_dummy_method, 32, GLOBAL_VARIABLE_INITIALIZED_WITH_FUNCTION_OR_METHOD_CALL, no_bucket, WARNING, []