diff --git a/infer/lib/python/inferlib/issues.py b/infer/lib/python/inferlib/issues.py index ef90d1544..04371b852 100644 --- a/infer/lib/python/inferlib/issues.py +++ b/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 = [ diff --git a/infer/src/clang/cField_decl.ml b/infer/src/clang/cField_decl.ml index f01a1a313..138197ba2 100644 --- a/infer/src/clang/cField_decl.ml +++ b/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) diff --git a/infer/src/clang/cField_decl.mli b/infer/src/clang/cField_decl.mli index 7c84db8cb..dd0ba7993 100644 --- a/infer/src/clang/cField_decl.mli +++ b/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 diff --git a/infer/src/clang/cFrontend_checkers.ml b/infer/src/clang/cFrontend_checkers.ml index 647408ffc..104fb6543 100644 --- a/infer/src/clang/cFrontend_checkers.ml +++ b/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 *) diff --git a/infer/src/clang/cFrontend_checkers.mli b/infer/src/clang/cFrontend_checkers.mli index a88f5a16b..df99803d9 100644 --- a/infer/src/clang/cFrontend_checkers.mli +++ b/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) diff --git a/infer/src/clang/cFrontend_config.ml b/infer/src/clang/cFrontend_config.ml index eaee22d3d..04830a0a9 100644 --- a/infer/src/clang/cFrontend_config.ml +++ b/infer/src/clang/cFrontend_config.ml @@ -166,3 +166,5 @@ let nsarray_cl = "NSArray" let infer = "infer" let block = "block" + +let atomic_att = "<\"Atomic\">" diff --git a/infer/src/clang/cFrontend_config.mli b/infer/src/clang/cFrontend_config.mli index 9ae2bae49..2a9e1bdaa 100644 --- a/infer/src/clang/cFrontend_config.mli +++ b/infer/src/clang/cFrontend_config.mli @@ -161,3 +161,5 @@ val nsarray_cl : string val infer : string val block : string + +val atomic_att : string diff --git a/infer/src/clang/cFrontend_errors.ml b/infer/src/clang/cFrontend_errors.ml index 080453d39..d5fb087ed 100644 --- a/infer/src/clang/cFrontend_errors.ml +++ b/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 diff --git a/infer/src/clang/cFrontend_errors.mli b/infer/src/clang/cFrontend_errors.mli index d2bcc3302..2604ba115 100644 --- a/infer/src/clang/cFrontend_errors.mli +++ b/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 diff --git a/infer/src/clang/cTrans.ml b/infer/src/clang/cTrans.ml index e4d3e8df5..5c5a8e03d 100644 --- a/infer/src/clang/cTrans.ml +++ b/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 = diff --git a/infer/src/clang/objcProperty_decl.ml b/infer/src/clang/objcProperty_decl.ml index b60e0a21f..11e439d52 100644 --- a/infer/src/clang/objcProperty_decl.ml +++ b/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 diff --git a/infer/src/clang/objcProperty_decl.mli b/infer/src/clang/objcProperty_decl.mli index 8565020e1..3da5279e8 100644 --- a/infer/src/clang/objcProperty_decl.mli +++ b/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 diff --git a/infer/tests/codetoanalyze/objc/warnings/atomic_prop.m b/infer/tests/codetoanalyze/objc/warnings/atomic_prop.m new file mode 100644 index 000000000..88ac2a83f --- /dev/null +++ b/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 + +@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 diff --git a/infer/tests/endtoend/objc/AtomicPropertyTest.java b/infer/tests/endtoend/objc/AtomicPropertyTest.java new file mode 100644 index 000000000..c4baba538 --- /dev/null +++ b/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 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" + } + ) + ); + } + +} diff --git a/infer/tests/utils/InferResults.java b/infer/tests/utils/InferResults.java index 7397bc3b1..ed02caff3 100644 --- a/infer/tests/utils/InferResults.java +++ b/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") ||