From 7bbb7fc8690d18738560ca500ac0b46726d9f360 Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Thu, 10 Jan 2019 01:43:35 -0800 Subject: [PATCH] [clang][objcpp] register exported methods and treat them as private in RacerD Summary: In ObjC there are no access modifiers. The strongest alternative is to put methods in the implementation but omit them from the interface declaration. Put exported ObjC methods in their own field in the class structure and use that in RacerD to decide whether to report on the method. Reviewed By: mbouaziz Differential Revision: D13597504 fbshipit-source-id: c4a3d2705 --- infer/src/IR/Tenv.ml | 5 +-- infer/src/IR/Tenv.mli | 1 + infer/src/IR/Typ.ml | 28 +++++++++++---- infer/src/IR/Typ.mli | 2 ++ infer/src/clang/objcInterface_decl.ml | 2 +- infer/src/concurrency/RacerD.ml | 11 +++--- .../codetoanalyze/objcpp/racerd/Basic.mm | 27 ++++++-------- .../codetoanalyze/objcpp/racerd/Private.h | 13 +++++++ .../codetoanalyze/objcpp/racerd/Private.mm | 36 +++++++++++++++++++ .../codetoanalyze/objcpp/racerd/issues.exp | 3 +- 10 files changed, 96 insertions(+), 32 deletions(-) create mode 100644 infer/tests/codetoanalyze/objcpp/racerd/Private.h create mode 100644 infer/tests/codetoanalyze/objcpp/racerd/Private.mm diff --git a/infer/src/IR/Tenv.ml b/infer/src/IR/Tenv.ml index 7bddb3008..6a25032df 100644 --- a/infer/src/IR/Tenv.ml +++ b/infer/src/IR/Tenv.ml @@ -34,9 +34,10 @@ let pp fmt (tenv : t) = let create () = TypenameHash.create 1000 (** Construct a struct type in a type environment *) -let mk_struct tenv ?default ?fields ?statics ?methods ?supers ?annots name = +let mk_struct tenv ?default ?fields ?statics ?methods ?exported_objc_methods ?supers ?annots name = let struct_typ = - Typ.Struct.internal_mk_struct ?default ?fields ?statics ?methods ?supers ?annots () + Typ.Struct.internal_mk_struct ?default ?fields ?statics ?methods ?exported_objc_methods ?supers + ?annots () in TypenameHash.replace tenv name struct_typ ; struct_typ diff --git a/infer/src/IR/Tenv.mli b/infer/src/IR/Tenv.mli index cf5dc025c..c8689a8f0 100644 --- a/infer/src/IR/Tenv.mli +++ b/infer/src/IR/Tenv.mli @@ -35,6 +35,7 @@ val mk_struct : -> ?fields:Typ.Struct.fields -> ?statics:Typ.Struct.fields -> ?methods:Typ.Procname.t list + -> ?exported_objc_methods:Typ.Procname.t list -> ?supers:Typ.Name.t list -> ?annots:Annot.Item.t -> Typ.Name.t diff --git a/infer/src/IR/Typ.ml b/infer/src/IR/Typ.ml index 5a2689450..662f40d6c 100644 --- a/infer/src/IR/Typ.ml +++ b/infer/src/IR/Typ.ml @@ -1459,6 +1459,7 @@ module Struct = struct ; statics: fields (** static fields *) ; supers: Name.t list (** superclasses *) ; methods: Procname.t list (** methods defined *) + ; exported_objc_methods: Procname.t list (** methods in ObjC interface, subset of [methods] *) ; annots: Annot.Item.t (** annotations *) } type lookup = Name.t -> t option @@ -1467,7 +1468,7 @@ module Struct = struct F.fprintf f "@\n\t\t%a %a %a" (pp_full pe) typ Fieldname.pp field_name Annot.Item.pp ann - let pp pe name f {fields; supers; methods; annots} = + let pp pe name f {fields; supers; methods; exported_objc_methods; annots} = if Config.debug_mode then (* change false to true to print the details of struct *) F.fprintf f @@ -1478,6 +1479,8 @@ module Struct = struct \t}@\n\ \tmethods: {%a@\n\ \t}@\n\ + \texported_obj_methods: {%a@\n\ + \t}@\n\ \tannots: {%a@\n\ \t}" Name.pp name @@ -1486,17 +1489,28 @@ module Struct = struct (Pp.seq (fun f n -> F.fprintf f "@\n\t\t%a" Name.pp n)) supers (Pp.seq (fun f m -> F.fprintf f "@\n\t\t%a" Procname.pp m)) - methods Annot.Item.pp annots + methods + (Pp.seq (fun f m -> F.fprintf f "@\n\t\t%a" Procname.pp m)) + exported_objc_methods Annot.Item.pp annots else Name.pp f name - let internal_mk_struct ?default ?fields ?statics ?methods ?supers ?annots () = - let default_ = {fields= []; statics= []; methods= []; supers= []; annots= Annot.Item.empty} in + let internal_mk_struct ?default ?fields ?statics ?methods ?exported_objc_methods ?supers ?annots + () = + let default_ = + { fields= [] + ; statics= [] + ; methods= [] + ; exported_objc_methods= [] + ; supers= [] + ; annots= Annot.Item.empty } + in let mk_struct_ ?(default = default_) ?(fields = default.fields) ?(statics = default.statics) - ?(methods = default.methods) ?(supers = default.supers) ?(annots = default.annots) () = - {fields; statics; methods; supers; annots} + ?(methods = default.methods) ?(exported_objc_methods = default.exported_objc_methods) + ?(supers = default.supers) ?(annots = default.annots) () = + {fields; statics; methods; exported_objc_methods; supers; annots} in - mk_struct_ ?default ?fields ?statics ?methods ?supers ?annots () + mk_struct_ ?default ?fields ?statics ?methods ?exported_objc_methods ?supers ?annots () (** the element typ of the final extensible array in the given typ, if any *) diff --git a/infer/src/IR/Typ.mli b/infer/src/IR/Typ.mli index 8e6157c21..1ca6a43dd 100644 --- a/infer/src/IR/Typ.mli +++ b/infer/src/IR/Typ.mli @@ -665,6 +665,7 @@ module Struct : sig ; statics: fields (** static fields *) ; supers: Name.t list (** supers *) ; methods: Procname.t list (** methods defined *) + ; exported_objc_methods: Procname.t list (** methods in ObjC interface, subset of [methods] *) ; annots: Annot.Item.t (** annotations *) } type lookup = Name.t -> t option @@ -679,6 +680,7 @@ module Struct : sig -> ?fields:fields -> ?statics:fields -> ?methods:Procname.t list + -> ?exported_objc_methods:Procname.t list -> ?supers:Name.t list -> ?annots:Annot.Item.t -> unit diff --git a/infer/src/clang/objcInterface_decl.ml b/infer/src/clang/objcInterface_decl.ml index 31d07002f..1d5e56718 100644 --- a/infer/src/clang/objcInterface_decl.ml +++ b/infer/src/clang/objcInterface_decl.ml @@ -121,7 +121,7 @@ let add_class_to_tenv qual_type_to_sil_type procname_from_decl tenv decl_info na in ignore (Tenv.mk_struct tenv ~fields:all_fields ~supers ~methods ~annots:Annot.Class.objc - interface_name) ; + ~exported_objc_methods:methods interface_name) ; interface_desc diff --git a/infer/src/concurrency/RacerD.ml b/infer/src/concurrency/RacerD.ml index 0729d824c..b96f67f99 100644 --- a/infer/src/concurrency/RacerD.ml +++ b/infer/src/concurrency/RacerD.ml @@ -968,16 +968,17 @@ let should_report_on_proc procdesc tenv = || Procdesc.get_access procdesc <> PredSymb.Private && (not (Typ.Procname.Java.is_autogen_method java_pname)) && not (Annotations.pdesc_return_annot_ends_with procdesc Annotations.visibleForTesting) - | ObjC_Cpp objc_cpp_pname -> - ( match objc_cpp_pname.Typ.Procname.ObjC_Cpp.kind with + | ObjC_Cpp {kind; class_name} -> + ( match kind with | CPPMethod _ | CPPConstructor _ | CPPDestructor _ -> Procdesc.get_access procdesc <> PredSymb.Private | ObjCClassMethod | ObjCInstanceMethod | ObjCInternalMethod -> - (* ObjC has no private methods, just a convention that they start with an underscore *) - not (String.is_prefix objc_cpp_pname.Typ.Procname.ObjC_Cpp.method_name ~prefix:"_") ) + Tenv.lookup tenv class_name + |> Option.exists ~f:(fun {Typ.Struct.exported_objc_methods} -> + List.mem ~equal:Typ.Procname.equal exported_objc_methods proc_name ) ) && let matcher = ConcurrencyModels.cpp_lock_types_matcher in - Option.exists (Tenv.lookup tenv objc_cpp_pname.class_name) ~f:(fun class_str -> + Option.exists (Tenv.lookup tenv class_name) ~f:(fun class_str -> (* check if the class contains a lock member *) List.exists class_str.Typ.Struct.fields ~f:(fun (_, ft, _) -> Option.exists (Typ.name ft) ~f:(fun name -> diff --git a/infer/tests/codetoanalyze/objcpp/racerd/Basic.mm b/infer/tests/codetoanalyze/objcpp/racerd/Basic.mm index 28b1e6333..b6a8bb05a 100644 --- a/infer/tests/codetoanalyze/objcpp/racerd/Basic.mm +++ b/infer/tests/codetoanalyze/objcpp/racerd/Basic.mm @@ -4,31 +4,26 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. */ -#import #import +#import @interface Basic : NSObject -- (int)read; -- (void)_private_write_no_top_level_report:(int)data; -- (void)write:(int)data; +- (int)read_bad; +- (void)write_bad:(int)data; @end @implementation Basic { - std::mutex mutex_; - int data_; -} - -- (int)read { - return data_; + std::mutex _mutex; + int _data; } -- (void)_private_write_no_top_level_report:(int)data { - data_ = data; +- (int)read_bad { + return _data; } -- (void)write:(int)data { - mutex_.lock(); - [self _private_write_no_top_level_report:data]; - mutex_.unlock(); +- (void)write_bad:(int)data { + _mutex.lock(); + _data = data; + _mutex.unlock(); } @end diff --git a/infer/tests/codetoanalyze/objcpp/racerd/Private.h b/infer/tests/codetoanalyze/objcpp/racerd/Private.h new file mode 100644 index 000000000..b1b2dcc5d --- /dev/null +++ b/infer/tests/codetoanalyze/objcpp/racerd/Private.h @@ -0,0 +1,13 @@ +/* + * Copyright (c) 2018-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ +#import + +@interface Private : NSObject +- (void)write_ok:(int)data; +- (int)read_other_bad; +- (void)write_other_bad:(int)other_data; +@end diff --git a/infer/tests/codetoanalyze/objcpp/racerd/Private.mm b/infer/tests/codetoanalyze/objcpp/racerd/Private.mm new file mode 100644 index 000000000..932f1ee6b --- /dev/null +++ b/infer/tests/codetoanalyze/objcpp/racerd/Private.mm @@ -0,0 +1,36 @@ +/* + * Copyright (c) 2018-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ +#import +#import "Private.h" + +@implementation Private { + std::mutex _mutex; + int _data; + int _other_data; +} + +// no report on _data +- (int)_private_read_ok { + return _data; +} + +- (void)write_ok:(int)data { + _mutex.lock(); + _data = data; + _mutex.unlock(); +} + +- (int)read_other_bad { + return _other_data; +} + +- (void)write_other_bad:(int)other_data { + _mutex.lock(); + _other_data = other_data; + _mutex.unlock(); +} +@end diff --git a/infer/tests/codetoanalyze/objcpp/racerd/issues.exp b/infer/tests/codetoanalyze/objcpp/racerd/issues.exp index 2140bba1e..5595dc415 100644 --- a/infer/tests/codetoanalyze/objcpp/racerd/issues.exp +++ b/infer/tests/codetoanalyze/objcpp/racerd/issues.exp @@ -1 +1,2 @@ -codetoanalyze/objcpp/racerd/Basic.mm, Basic_read, 22, LOCK_CONSISTENCY_VIOLATION, no_bucket, ERROR, [,access to `self.data_`,,call to Basic__private_write_no_top_level_report:,access to `self.data_`] +codetoanalyze/objcpp/racerd/Basic.mm, Basic_read_bad, 21, LOCK_CONSISTENCY_VIOLATION, no_bucket, ERROR, [,access to `self._data`,,access to `self._data`] +codetoanalyze/objcpp/racerd/Private.mm, Private_read_other_bad, 28, LOCK_CONSISTENCY_VIOLATION, no_bucket, ERROR, [,access to `self._other_data`,,access to `self._other_data`]