From 870c636e41d421513bffd003ef09bf4cb2e01097 Mon Sep 17 00:00:00 2001 From: Dulma Churchill Date: Thu, 28 Sep 2017 10:03:50 -0700 Subject: [PATCH] [objc] Adding missing fields to env when executing generated getters and setters. Reviewed By: mbouaziz Differential Revision: D5923752 fbshipit-source-id: 0f3a7db --- Makefile | 2 +- infer/src/IR/ProcAttributes.ml | 4 +-- infer/src/IR/ProcAttributes.mli | 4 +-- infer/src/IR/Procdesc.ml | 8 ++--- infer/src/IR/Tenv.ml | 11 +++++++ infer/src/IR/Tenv.mli | 3 ++ infer/src/IR/Typ.ml | 6 ++-- infer/src/IR/Typ.mli | 2 ++ infer/src/backend/symExec.ml | 29 ++++++++++++------- infer/src/clang/cMethod_trans.ml | 26 ++++++++++++----- .../codetoanalyze/objc_getters_setters/A.h | 16 ++++++++++ .../codetoanalyze/objc_getters_setters/A.m | 13 +++++++++ .../codetoanalyze/objc_getters_setters/B.h | 13 +++++++++ .../codetoanalyze/objc_getters_setters/B.m | 26 +++++++++++++++++ .../objc_getters_setters/Makefile | 24 +++++++++++++++ .../objc_getters_setters/issues.exp | 2 ++ 16 files changed, 161 insertions(+), 28 deletions(-) create mode 100644 infer/tests/build_systems/codetoanalyze/objc_getters_setters/A.h create mode 100644 infer/tests/build_systems/codetoanalyze/objc_getters_setters/A.m create mode 100644 infer/tests/build_systems/codetoanalyze/objc_getters_setters/B.h create mode 100644 infer/tests/build_systems/codetoanalyze/objc_getters_setters/B.m create mode 100644 infer/tests/build_systems/objc_getters_setters/Makefile create mode 100644 infer/tests/build_systems/objc_getters_setters/issues.exp diff --git a/Makefile b/Makefile index 436e1f4a3..893e259cb 100644 --- a/Makefile +++ b/Makefile @@ -61,7 +61,7 @@ ifneq ($(PYTHON_lxml),no) BUILD_SYSTEMS_TESTS += results_xml endif ifneq ($(XCODE_SELECT),no) -BUILD_SYSTEMS_TESTS += xcodebuild_no_xcpretty +BUILD_SYSTEMS_TESTS += xcodebuild_no_xcpretty objc_getters_setters DIRECT_TESTS += \ objc_frontend objc_errors objc_linters objc_ioslints \ objcpp_frontend objcpp_linters objc_linters-for-test-only objcpp_linters-for-test-only \ diff --git a/infer/src/IR/ProcAttributes.ml b/infer/src/IR/ProcAttributes.ml index 40178399c..ee7a5ae08 100644 --- a/infer/src/IR/ProcAttributes.ml +++ b/infer/src/IR/ProcAttributes.ml @@ -31,8 +31,8 @@ let proc_flags_find proc_flags key = Hashtbl.find proc_flags key (** Type for ObjC accessors *) type objc_accessor_type = - | Objc_getter of Typ.Fieldname.t - | Objc_setter of Typ.Fieldname.t + | Objc_getter of Typ.Struct.field + | Objc_setter of Typ.Struct.field [@@deriving compare] type t = diff --git a/infer/src/IR/ProcAttributes.mli b/infer/src/IR/ProcAttributes.mli index 66896e23e..ca8745d90 100644 --- a/infer/src/IR/ProcAttributes.mli +++ b/infer/src/IR/ProcAttributes.mli @@ -29,8 +29,8 @@ val proc_flags_find : proc_flags -> string -> string (** find a value for a key in the proc flags *) type objc_accessor_type = - | Objc_getter of Typ.Fieldname.t - | Objc_setter of Typ.Fieldname.t + | Objc_getter of Typ.Struct.field + | Objc_setter of Typ.Struct.field [@@deriving compare] type t = diff --git a/infer/src/IR/Procdesc.ml b/infer/src/IR/Procdesc.ml index 4d7a94791..453da4ae2 100644 --- a/infer/src/IR/Procdesc.ml +++ b/infer/src/IR/Procdesc.ml @@ -483,10 +483,10 @@ let pp_variable_list fmt etl = let pp_objc_accessor fmt accessor = match accessor with - | Some ProcAttributes.Objc_getter name - -> Format.fprintf fmt "Getter of %a, " Typ.Fieldname.pp name - | Some ProcAttributes.Objc_setter name - -> Format.fprintf fmt "Setter of %a, " Typ.Fieldname.pp name + | Some ProcAttributes.Objc_getter field + -> Format.fprintf fmt "Getter of %a, " (Typ.Struct.pp_field Pp.text) field + | Some ProcAttributes.Objc_setter field + -> Format.fprintf fmt "Setter of %a, " (Typ.Struct.pp_field Pp.text) field | None -> () diff --git a/infer/src/IR/Tenv.ml b/infer/src/IR/Tenv.ml index 992df74e2..9fdd5e494 100644 --- a/infer/src/IR/Tenv.ml +++ b/infer/src/IR/Tenv.ml @@ -61,6 +61,17 @@ let lookup tenv name : Typ.Struct.t option = (** Add a (name,type) pair to the global type environment. *) let add tenv name struct_typ = TypenameHash.replace tenv name struct_typ +(** Add a field to a given struct in the global type environment. *) +let add_field tenv class_tn_name field = + match lookup tenv class_tn_name with + | Some ({fields} as struct_typ) + -> let field_equal (f1, _, _) (f2, _, _) = Typ.Fieldname.equal f1 f2 in + if not (List.mem ~equal:field_equal fields field) then + let new_fields = field :: fields in + ignore (mk_struct tenv ~default:struct_typ ~fields:new_fields ~statics:[] class_tn_name) + | _ + -> () + (** Get method that is being overriden by java_pname (if any) **) let get_overriden_method tenv pname_java = let struct_typ_get_method_by_name (struct_typ: Typ.Struct.t) method_name = diff --git a/infer/src/IR/Tenv.mli b/infer/src/IR/Tenv.mli index 1ca422445..f6ce0e73f 100644 --- a/infer/src/IR/Tenv.mli +++ b/infer/src/IR/Tenv.mli @@ -38,6 +38,9 @@ val mk_struct : -> Typ.Struct.t (** Construct a struct_typ, normalizing field types *) +val add_field : t -> Typ.Name.t -> Typ.Struct.field -> unit +(** Add a field to a given struct in the global type environment. *) + val mem : t -> Typ.Name.t -> bool (** Check if typename is found in t *) diff --git a/infer/src/IR/Typ.ml b/infer/src/IR/Typ.ml index 931153575..b93c4c0ac 100644 --- a/infer/src/IR/Typ.ml +++ b/infer/src/IR/Typ.ml @@ -1235,14 +1235,16 @@ module Struct = struct type lookup = Name.t -> t option + let pp_field pe f (field_name, typ, ann) = + 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} = if Config.debug_mode then (* change false to true to print the details of struct *) F.fprintf f "%a @\n\tfields: {%a@\n\t}@\n\tsupers: {%a@\n\t}@\n\tmethods: {%a@\n\t}@\n\tannots: {%a@\n\t}" Name.pp name - (Pp.seq (fun f (fld, t, a) -> - F.fprintf f "@\n\t\t%a %a %a" (pp_full pe) t Fieldname.pp fld Annot.Item.pp a )) + (Pp.seq (pp_field pe)) fields (Pp.seq (fun f n -> F.fprintf f "@\n\t\t%a" Name.pp n)) supers diff --git a/infer/src/IR/Typ.mli b/infer/src/IR/Typ.mli index a2a18d5ad..94915ae73 100644 --- a/infer/src/IR/Typ.mli +++ b/infer/src/IR/Typ.mli @@ -580,6 +580,8 @@ module Struct : sig type lookup = Name.t -> t option + val pp_field : Pp.env -> F.formatter -> field -> unit + val pp : Pp.env -> Name.t -> F.formatter -> t -> unit (** Pretty print a struct type. *) diff --git a/infer/src/backend/symExec.ml b/infer/src/backend/symExec.ml index a28a2f800..753b1620f 100644 --- a/infer/src/backend/symExec.ml +++ b/infer/src/backend/symExec.ml @@ -1631,26 +1631,35 @@ and check_variadic_sentinel_if_present ({Builtin.prop_; path; proc_name} as buil -> let formals = callee_attributes.ProcAttributes.formals in check_variadic_sentinel (List.length formals) sentinel_arg builtin_args -and sym_exec_objc_getter field_name ret_typ tenv ret_id pdesc pname loc args prop = +and sym_exec_objc_getter field ret_typ tenv ret_id pdesc pname loc args prop = + let field_name, _, _ = field in L.d_strln ( "No custom getter found. Executing the ObjC builtin getter with ivar " ^ Typ.Fieldname.to_string field_name ^ "." ) ; let ret_id = match ret_id with Some (ret_id, _) -> ret_id | None -> assert false in match args with - | [(lexp, ({Typ.desc= Tstruct _} as typ | {desc= Tptr (({desc= Tstruct _} as typ), _)}))] - -> let field_access_exp = Exp.Lfield (lexp, field_name, typ) in + | [ ( lexp + , ( {Typ.desc= Tstruct struct_name} as typ + | {desc= Tptr (({desc= Tstruct struct_name} as typ), _)} ) ) ] + -> Tenv.add_field tenv struct_name field ; + let field_access_exp = Exp.Lfield (lexp, field_name, typ) in execute_load ~report_deref_errors:false pname pdesc tenv ret_id field_access_exp ret_typ loc prop | _ -> raise (Exceptions.Wrong_argument_number __POS__) -and sym_exec_objc_setter field_name _ tenv _ pdesc pname loc args prop = +and sym_exec_objc_setter field _ tenv _ pdesc pname loc args prop = + let field_name, _, _ = field in L.d_strln ( "No custom setter found. Executing the ObjC builtin setter with ivar " ^ Typ.Fieldname.to_string field_name ^ "." ) ; match args with - | (lexp1, ({Typ.desc= Tstruct _} as typ1 | {Typ.desc= Tptr (typ1, _)})) :: (lexp2, typ2) :: _ - -> let field_access_exp = Exp.Lfield (lexp1, field_name, typ1) in + | ( lexp1 + , ( {Typ.desc= Tstruct struct_name} as typ1 + | {Typ.desc= Tptr (({Typ.desc= Tstruct struct_name} as typ1), _)} ) ) + :: (lexp2, typ2) :: _ + -> Tenv.add_field tenv struct_name field ; + let field_access_exp = Exp.Lfield (lexp1, field_name, typ1) in execute_store ~report_deref_errors:false pname pdesc tenv field_access_exp typ2 lexp2 loc prop | _ @@ -1660,10 +1669,10 @@ and sym_exec_objc_accessor property_accesor ret_typ tenv ret_id pdesc _ loc args : Builtin.ret_typ = let f_accessor = match property_accesor with - | ProcAttributes.Objc_getter field_name - -> sym_exec_objc_getter field_name - | ProcAttributes.Objc_setter field_name - -> sym_exec_objc_setter field_name + | ProcAttributes.Objc_getter field + -> sym_exec_objc_getter field + | ProcAttributes.Objc_setter field + -> sym_exec_objc_setter field in (* we want to execute in the context of the current procedure, not in the context of callee_pname, since this is the procname of the setter/getter method *) diff --git a/infer/src/clang/cMethod_trans.ml b/infer/src/clang/cMethod_trans.ml index 8ec3dabd5..ca56f9046 100644 --- a/infer/src/clang/cMethod_trans.ml +++ b/infer/src/clang/cMethod_trans.ml @@ -491,7 +491,7 @@ let get_byval_args_indices ~shift args = let index' = index + shift in Option.some_if (is_value qual_type) index' ) -let get_objc_property_accessor ms = +let get_objc_property_accessor tenv ms = let open Clang_ast_t in match CAst_utils.get_decl_opt (CMethod_signature.ms_get_pointer_to_property_opt ms) with | Some ObjCPropertyDecl (_, _, obj_c_property_decl_info) @@ -499,7 +499,8 @@ let get_objc_property_accessor ms = let ivar_decl_ref = obj_c_property_decl_info.Clang_ast_t.opdi_ivar_decl in match CAst_utils.get_decl_opt_with_decl_ref ivar_decl_ref with | Some ObjCIvarDecl (_, {ni_name}, _, _, _) - -> let class_tname = + -> ( + let class_tname = match CMethod_signature.ms_get_name ms with | Typ.Procname.ObjC_Cpp objc_cpp -> Typ.Procname.objc_cpp_get_class_type_name objc_cpp @@ -507,10 +508,21 @@ let get_objc_property_accessor ms = -> assert false in let field_name = CGeneral_utils.mk_class_field_name class_tname ni_name in - if CMethod_signature.ms_is_getter ms then Some (ProcAttributes.Objc_getter field_name) - else if CMethod_signature.ms_is_setter ms then - Some (ProcAttributes.Objc_setter field_name) - else None + match Tenv.lookup tenv class_tname with + | Some {fields} + -> ( + let field_opt = + List.find ~f:(fun (name, _, _) -> Typ.Fieldname.equal name field_name) fields + in + match field_opt with + | Some field when CMethod_signature.ms_is_getter ms + -> Some (ProcAttributes.Objc_getter field) + | Some field when CMethod_signature.ms_is_setter ms + -> Some (ProcAttributes.Objc_setter field) + | _ + -> None ) + | None + -> None ) | _ -> None ) | _ @@ -566,7 +578,7 @@ let create_local_procdesc ?(set_objc_accessor_attr= false) trans_unit_ctx cfg te let loc_exit = CLocation.get_sil_location_from_range trans_unit_ctx source_range false in let ret_type = get_return_type tenv ms in let objc_property_accessor = - if set_objc_accessor_attr then get_objc_property_accessor ms else None + if set_objc_accessor_attr then get_objc_property_accessor tenv ms else None in let procdesc = let proc_attributes = diff --git a/infer/tests/build_systems/codetoanalyze/objc_getters_setters/A.h b/infer/tests/build_systems/codetoanalyze/objc_getters_setters/A.h new file mode 100644 index 000000000..9dc6c536a --- /dev/null +++ b/infer/tests/build_systems/codetoanalyze/objc_getters_setters/A.h @@ -0,0 +1,16 @@ +/* + * Copyright (c) 2017 - 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 +#import + +@interface A : NSObject + +@property(nullable, nonatomic, copy) NSData* metadata; + +@end diff --git a/infer/tests/build_systems/codetoanalyze/objc_getters_setters/A.m b/infer/tests/build_systems/codetoanalyze/objc_getters_setters/A.m new file mode 100644 index 000000000..86c31fe0c --- /dev/null +++ b/infer/tests/build_systems/codetoanalyze/objc_getters_setters/A.m @@ -0,0 +1,13 @@ +/* + * Copyright (c) 2017 - 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 "A.h" + +@implementation A + +@end diff --git a/infer/tests/build_systems/codetoanalyze/objc_getters_setters/B.h b/infer/tests/build_systems/codetoanalyze/objc_getters_setters/B.h new file mode 100644 index 000000000..eb8049e75 --- /dev/null +++ b/infer/tests/build_systems/codetoanalyze/objc_getters_setters/B.h @@ -0,0 +1,13 @@ +/* + * Copyright (c) 2017 - 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 + +@end diff --git a/infer/tests/build_systems/codetoanalyze/objc_getters_setters/B.m b/infer/tests/build_systems/codetoanalyze/objc_getters_setters/B.m new file mode 100644 index 000000000..b995a2d42 --- /dev/null +++ b/infer/tests/build_systems/codetoanalyze/objc_getters_setters/B.m @@ -0,0 +1,26 @@ +/* + * Copyright (c) 2017 - 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 "A.h" +#import "B.h" + +@implementation B + +- (int)npe_no_bad_footprint_in_getter:(A*)a { + int* p = nil; + NSData* metadata = a.metadata; // Doesn't crash here with Bad_footprint + return *p; // NPE +} + +- (int)npe_no_bad_footprint_in_setter:(A*)a andMetadata:(NSData*)metadata { + int* p = nil; + a.metadata = metadata; // Doesn't crash here with Bad_footprint + return *p; // NPE +} +@end diff --git a/infer/tests/build_systems/objc_getters_setters/Makefile b/infer/tests/build_systems/objc_getters_setters/Makefile new file mode 100644 index 000000000..33e01af78 --- /dev/null +++ b/infer/tests/build_systems/objc_getters_setters/Makefile @@ -0,0 +1,24 @@ +# Copyright (c) 2017 - 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. + +TESTS_DIR = ../.. +ROOT_DIR = $(TESTS_DIR)/../.. +CODETOANALYZE_DIR = ../codetoanalyze/objc_getters_setters + +ANALYZER = infer +SOURCES = $(CODETOANALYZE_DIR)/A.m $(CODETOANALYZE_DIR)/B.m +OBJECTS = $(CODETOANALYZE_DIR)/A.o $(CODETOANALYZE_DIR)/B.o +INFER_OPTIONS = --report-custom-error --developer-mode --project-root $(TESTS_DIR) +INFERPRINT_OPTIONS = --project-root $(TESTS_DIR) --issues-tests +CLEAN_EXTRA = -c + +include $(TESTS_DIR)/infer.make + +infer-out/report.json: $(CLANG_DEPS) $(SOURCES) $(MAKEFILE_LIST) + $(QUIET)$(REMOVE_DIR) buck-out && \ + $(call silent_on_success,Testing analysis with Objective-C getters and setters,\ + $(INFER_BIN) $(INFER_OPTIONS) --results-dir $(CURDIR)/infer-out -- clang $(CLEAN_EXTRA) $(SOURCES)) diff --git a/infer/tests/build_systems/objc_getters_setters/issues.exp b/infer/tests/build_systems/objc_getters_setters/issues.exp new file mode 100644 index 000000000..c13afdd14 --- /dev/null +++ b/infer/tests/build_systems/objc_getters_setters/issues.exp @@ -0,0 +1,2 @@ +build_systems/codetoanalyze/objc_getters_setters/B.m, B_npe_no_bad_footprint_in_getter:, 3, NULL_DEREFERENCE, [start of procedure npe_no_bad_footprint_in_getter:] +build_systems/codetoanalyze/objc_getters_setters/B.m, B_npe_no_bad_footprint_in_setter:andMetadata:, 3, NULL_DEREFERENCE, [start of procedure npe_no_bad_footprint_in_setter:andMetadata:]