Introducing the DIRECT_ATOMIC_PROPERTY_ACCESS

Reviewed By: dulmarod

Differential Revision: D2699663

fb-gh-sync-id: c77957e

infer/lib/python/inferlib/issues.py

@@ -46,6 +46,7 @@ ISSUE_TYPES = [
     'NULL_DEREFERENCE',
     'PARAMETER_NOT_NULL_CHECKED',
     'PREMATURE_NIL_TERMINATION_ARGUMENT',
+    'DIRECT_ATOMIC_PROPERTY_ACCESS',
 ]
 
 NULL_STYLE_ISSUE_TYPES = [

infer/src/clang/cField_decl.ml

@@ -113,3 +113,17 @@ let add_missing_fields tenv class_name fields =
       Printing.log_out " Updating info for class '%s' in tenv\n" class_name;
       Sil.tenv_add tenv class_tn_name class_type_info
   | _ -> assert false
+
+(* checks if ivar is defined among a set of fields and if it is atomic *)
+let is_ivar_atomic ivar fields =
+  let do_one_annot a =
+    (a.Sil.class_name = Config.property_attributes) &&
+    IList.exists (fun p -> p = CFrontend_config.atomic_att) a.Sil.parameters in
+  let has_atomic_annot ans =
+    IList.exists (fun (a, _) -> do_one_annot a) ans in
+  try
+    let _, _, annot = IList.find (fun (fn, _, _) -> Ident.fieldname_equal ivar fn) fields in
+    has_atomic_annot annot
+  with Not_found -> (
+      Printing.log_out "NOT Found field ivar = '%s' " (Ident.fieldname_to_string ivar);
+      false)

infer/src/clang/cField_decl.mli

@@ -25,3 +25,5 @@ val build_sil_field_property : Ast_utils.type_ptr_to_sil_type -> CContext.curr_c
   -> field_type
 
 val add_missing_fields : Sil.tenv -> string -> field_type list -> unit
+
+val is_ivar_atomic : Ident.fieldname -> Sil.struct_fields -> bool

infer/src/clang/cFrontend_checkers.ml

@@ -28,3 +28,36 @@ let checker_strong_delegate_warning pname ptype =
                      loc = string_of_int (ObjcProperty_decl.property_line ptype);
                     } in
   (condition, warning_desc)
+
+(* Direct Atomic Property access:
+   a property declared atomic should not be accessed directly via its ivar *)
+let direct_atomic_property_access context stmt_info ivar_name =
+  let tenv = CContext.get_tenv context in
+  let curr_class = CContext.get_curr_class context in
+  let mname = Cfg.Procdesc.get_proc_name (CContext.get_procdesc context) in
+  let ivar, cname = match ivar_name with
+    | Some n ->
+        General_utils.mk_class_field_name n,
+        Ast_utils.get_class_name_from_member n
+    | _ -> Ident.create_fieldname (Mangled.from_string "") 0, "" in
+  let tname = Sil.TN_csu (Sil.Class, Mangled.from_string cname) in
+  let line = match (fst stmt_info.Clang_ast_t.si_source_range).Clang_ast_t.sl_line with
+    | Some l -> string_of_int l
+    | _ -> "-1" in
+  match Sil.tenv_lookup tenv tname with
+  | Some Sil.Tstruct (flds1, flds2, _, _, _, _, _) ->
+      (* We give the warning when:
+         (1) the property has the atomic attribute and
+         (2) the access of the ivar is not in a getter or setter method. This condition
+             avoids false positives *)
+      let condition = (CField_decl.is_ivar_atomic ivar (flds1 @ flds2))
+                      && not (ObjcProperty_decl.is_getter_setter curr_class mname ivar_name) in
+      let warning_desc = {
+        name = "DIRECT_ATOMIC_PROPERTY_ACCESS";
+        description = "Direct access to ivar " ^ (Ident.fieldname_to_string ivar) ^
+                      " of an atomic property";
+        suggestion = "Accessing an ivar of an atomic property makes the property nonatomic";
+        loc = line;
+      } in
+      (condition, Some warning_desc)
+  | _ -> (false, None) (* No warning *)

infer/src/clang/cFrontend_checkers.mli

@@ -14,3 +14,9 @@
 val checker_strong_delegate_warning : Clang_ast_t.named_decl_info ->
   ObjcProperty_decl.property_type ->
   (bool * CFrontend_utils.General_utils.warning_desc)
+
+(* Direct Atomic Property access:
+   a property declared atomic should not be accesses directly via its iva *)
+val direct_atomic_property_access :
+  CContext.t -> Clang_ast_t.stmt_info -> Clang_ast_t.named_decl_info option ->
+  (bool * CFrontend_utils.General_utils.warning_desc option)

infer/src/clang/cFrontend_config.ml

@@ -166,3 +166,5 @@ let nsarray_cl = "NSArray"
 let infer = "infer"
 
 let block = "block"
+
+let atomic_att = "<\"Atomic\">"

infer/src/clang/cFrontend_config.mli

@@ -161,3 +161,5 @@ val nsarray_cl : string
 val infer : string
 
 val block : string
+
+val atomic_att : string

infer/src/clang/cFrontend_errors.ml

@@ -18,10 +18,12 @@ open Utils
 open CFrontend_utils
 open General_utils
 
-(* === Warnings on properties === *)
+(* List of checkers on properties *)
-
 let property_checkers_list = [CFrontend_checkers.checker_strong_delegate_warning]
 
+(* List of checkers on ivar access *)
+let ivar_access_checker_list =  [CFrontend_checkers.direct_atomic_property_access]
+
 (* Add a frontend warning with a description desc at location loc to the errlog of a proc desc *)
 let log_frontend_warning pdesc warn_desc =
   let loc = {
@@ -58,3 +60,12 @@ let check_for_property_errors cfg c =
   let properties = ObjcProperty_decl.find_properties_class c in
   Printing.log_out "Retrieved all properties of the class...\n";
   IList.iter do_one_property properties
+
+(* Call checkers on a specific access of an ivar *)
+let check_for_ivar_errors context stmt_info obj_c_ivar_ref_expr_info =
+  let dr_name = obj_c_ivar_ref_expr_info.Clang_ast_t.ovrei_decl_ref.Clang_ast_t.dr_name in
+  let pdesc = CContext.get_procdesc context in
+  IList.iter (fun checker ->
+      match checker context stmt_info dr_name with
+      | true, Some warning_desc -> log_frontend_warning pdesc warning_desc
+      | _, _ -> ()) ivar_access_checker_list

infer/src/clang/cFrontend_errors.mli

@@ -13,3 +13,7 @@
 
 (* Checks for warnings on properties of class c *)
 val check_for_property_errors : Cfg.cfg -> CContext.curr_class -> unit
+
+(* Call checkers on a specific access of an ivar *)
+val check_for_ivar_errors :
+  CContext.t -> Clang_ast_t.stmt_info -> Clang_ast_t.obj_c_ivar_ref_expr_info -> unit

infer/src/clang/cTrans.ml

@@ -1725,6 +1725,7 @@ struct
 
   and objCIvarRefExpr_trans trans_state stmt_info expr_info stmt_list obj_c_ivar_ref_expr_info =
     let decl_ref = obj_c_ivar_ref_expr_info.Clang_ast_t.ovrei_decl_ref in
+    CFrontend_errors.check_for_ivar_errors trans_state.context stmt_info obj_c_ivar_ref_expr_info;
     do_memb_ivar_ref_exp trans_state expr_info stmt_info stmt_list decl_ref
 
   and memberExpr_trans trans_state stmt_info expr_info stmt_list member_expr_info =

infer/src/clang/objcProperty_decl.ml

@@ -377,3 +377,15 @@ let get_methods curr_class decl_list =
         meth_name:: list_methods
     | _ -> list_methods in
   IList.fold_right get_method decl_list []
+
+(* checks whether mname is a getter or setter of the ivar given in dr_name *)
+let is_getter_setter curr_class mname dr_name =
+  match dr_name with
+  | Some ndi ->
+      (match Property.find_property_name_from_ivar curr_class ndi with
+       | Some pname ->
+           let _, _, _, (gn, _), (sn, _), _ = Property.find_property curr_class pname in
+           let m = Procname.to_simplified_string mname in
+           (m = gn) || (m = sn)
+       | None -> false)
+  | _ -> false

infer/src/clang/objcProperty_decl.mli

@@ -82,3 +82,7 @@ val is_strong_property : property_type -> bool
 
 (* Given a property type returns whether the property line *)
 val property_line : property_type -> int
+
+(* Checks whether mname is a getter or setter of the ivar given in dr_name *)
+val is_getter_setter :
+  CContext.curr_class -> Procname.t -> Clang_ast_t.named_decl_info option -> bool

infer/tests/codetoanalyze/objc/warnings/atomic_prop.m

@@ -0,0 +1,84 @@
+/*
+* Copyright (c) 2015 - 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 <Foundation/NSObject.h>
+
+@interface B  : NSObject
+
+- (void) foo;
+
+@end
+
+@implementation B
+
+-(void) foo {
+};
+
+@end
+
+
+@interface A  : NSObject
+
+@property (nonatomic) int *p;
+@property int *q; // atomic by default
+@property (atomic, assign) float f;
+
+@property B *b;
+
+- (void) write_p: (int)i;
+- (int) read_p;
+- (void) write_q: (int)i;
+- (int) read_q;
+- (void) write_f: (int)i;
+- (int) read_f;
+@end
+
+
+@implementation A
+
+@synthesize b;
+
+- (void) writeP: (int)i
+{
+    _p = i; // Good access
+}
+
+- (int) readP
+{
+    return _p; // Good access
+}
+
+- (void) writeQ: (int)i
+{
+    _q = i; // Bad access
+}
+
+- (int) readQ
+{
+    return _q; // Bad access
+}
+
+- (void) writeF: (float) j
+{
+    self.f = j; // Good access
+}
+
+- (int) readF
+{
+    return self.f; // Good access
+}
+
+- (void) bla
+{
+    if (b) { // bad access
+        [b foo]; //bad access
+    }
+}
+
+@end

infer/tests/endtoend/objc/AtomicPropertyTest.java

@@ -0,0 +1,67 @@
+/*
+* Copyright (c) 2015 - 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.
+*/
+
+package endtoend.objc;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static utils.matchers.ResultContainsExactly.containsExactly;
+
+import com.google.common.collect.ImmutableList;
+
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+
+import java.io.IOException;
+
+import utils.DebuggableTemporaryFolder;
+import utils.InferException;
+import utils.InferResults;
+import utils.InferRunner;
+
+public class AtomicPropertyTest {
+
+  public static final String FILE =
+      "infer/tests/codetoanalyze/objc/warnings/atomic_prop.m";
+
+  private static ImmutableList<String> inferCmd;
+
+  public static final String DIRECT_ATOMIC_PROPERTY_ACCESS = "DIRECT_ATOMIC_PROPERTY_ACCESS";
+
+  @ClassRule
+  public static DebuggableTemporaryFolder folder =
+      new DebuggableTemporaryFolder();
+
+  @BeforeClass
+  public static void runInfer() throws InterruptedException, IOException {
+    inferCmd = InferRunner.createObjCInferCommand(
+        folder,
+        FILE);
+  }
+
+  @Test
+  public void whenInferRunsOnAtomicProperty()
+      throws InterruptedException, IOException, InferException {
+    InferResults inferResults = InferRunner.runInferObjC(inferCmd);
+    assertThat(
+        "Results should contain a direct atomic property access",
+        inferResults,
+        containsExactly(
+            DIRECT_ATOMIC_PROPERTY_ACCESS,
+            FILE,
+            new String[]{
+                "writeQ:",
+                "readQ",
+                "bla"
+            }
+        )
+    );
+  }
+
+}

infer/tests/utils/InferResults.java

@@ -59,6 +59,7 @@ public class InferResults {
       if (errorKind.equals("ERROR") ||
           errorType.equals("RETURN_VALUE_IGNORED") ||
           errorType.equals("STRONG_DELEGATE_WARNING") ||
+          errorType.equals("DIRECT_ATOMIC_PROPERTY_ACCESS") ||
           errorType.equals("IMMUTABLE_CAST") ||
           errorType.equals("PARAMETER_NOT_NULL_CHECKED") ||
           errorType.equals("DANGLING_POINTER_DEREFERENCE") ||