diff --git a/Makefile b/Makefile index 9aa66c642..32d1ddc4c 100644 --- a/Makefile +++ b/Makefile @@ -82,7 +82,8 @@ BUILD_SYSTEMS_TESTS += objc_getters_setters objc_missing_fld objc_retain_cycles DIRECT_TESTS += \ objc_frontend objc_errors objc_linters objc_ioslints objcpp_errors objcpp_nullable objcpp_retain-cycles \ objc_linters-def-folder objc_nullable objc_liveness objcpp_liveness objc_uninit \ - objcpp_frontend objcpp_linters cpp_linters objc_linters-for-test-only objcpp_linters-for-test-only + objcpp_frontend objcpp_linters cpp_linters objc_linters-for-test-only objcpp_linters-for-test-only \ + objcpp_racerd ifneq ($(XCODE_SELECT),no) BUILD_SYSTEMS_TESTS += xcodebuild_no_xcpretty endif diff --git a/infer/src/IR/Mangled.ml b/infer/src/IR/Mangled.ml index 27363b24d..b860bcca9 100644 --- a/infer/src/IR/Mangled.ml +++ b/infer/src/IR/Mangled.ml @@ -38,6 +38,10 @@ let this = from_string "this" let is_this = function {plain= "this"} -> true | _ -> false +let self = from_string "self" + +let is_self = function {plain= "self"} -> true | _ -> false + module Set = Caml.Set.Make (struct type nonrec t = t diff --git a/infer/src/IR/Mangled.mli b/infer/src/IR/Mangled.mli index 1d7289134..86578c40f 100644 --- a/infer/src/IR/Mangled.mli +++ b/infer/src/IR/Mangled.mli @@ -35,6 +35,10 @@ val this : t val is_this : t -> bool +val self : t [@@warning "-32"] + +val is_self : t -> bool + (** Set of Mangled. *) module Set : Caml.Set.S with type elt = t diff --git a/infer/src/IR/Pvar.ml b/infer/src/IR/Pvar.ml index 1e0f29411..43faefb07 100644 --- a/infer/src/IR/Pvar.ml +++ b/infer/src/IR/Pvar.ml @@ -47,7 +47,7 @@ let compare_modulo_this x y = else let cmp = Mangled.compare x.pv_name y.pv_name in if not (Int.equal 0 cmp) then cmp - else if Mangled.is_this x.pv_name then 0 + else if Mangled.is_this x.pv_name || Mangled.is_self x.pv_name then 0 else compare_pvar_kind x.pv_kind y.pv_kind @@ -134,7 +134,7 @@ let is_static_local pv = match pv.pv_kind with Global_var (_, _, _, true, _) -> let is_this pvar = Mangled.is_this (get_name pvar) (** Check if a pvar is the special "self" var *) -let is_self pvar = Mangled.equal (get_name pvar) (Mangled.from_string "self") +let is_self pvar = Mangled.is_self (get_name pvar) (** Check if the pvar is a return var *) let is_return pv = Mangled.equal (get_name pv) Ident.name_return diff --git a/infer/src/IR/Pvar.mli b/infer/src/IR/Pvar.mli index 6e08c2ed1..d25c61718 100644 --- a/infer/src/IR/Pvar.mli +++ b/infer/src/IR/Pvar.mli @@ -22,7 +22,7 @@ type translation_unit = SourceFile.t option [@@deriving compare] type t [@@deriving compare] val compare_modulo_this : t -> t -> int -(** Comparison considering all pvars named 'this' to be equal *) +(** Comparison considering all pvars named 'this'/'self' to be equal *) val equal : t -> t -> bool (** Equality for pvar's *) diff --git a/infer/src/biabduction/Prover.ml b/infer/src/biabduction/Prover.ml index 0b4507df0..44888a8e6 100644 --- a/infer/src/biabduction/Prover.ml +++ b/infer/src/biabduction/Prover.ml @@ -999,8 +999,7 @@ let check_inconsistency_base tenv prop = let language = Typ.Procname.get_language (Procdesc.get_proc_name pdesc) in let is_java_this pvar = Language.equal language Java && Pvar.is_this pvar in let is_objc_instance_self pvar = - Language.equal language Clang - && Mangled.equal (Pvar.get_name pvar) (Mangled.from_string "self") + Language.equal language Clang && Pvar.is_self pvar && ClangMethodKind.equal procedure_attr.ProcAttributes.clang_method_kind ClangMethodKind.OBJC_INSTANCE in diff --git a/infer/src/biabduction/SymExec.ml b/infer/src/biabduction/SymExec.ml index ac67ee8ed..3bf3ce2a0 100644 --- a/infer/src/biabduction/SymExec.ml +++ b/infer/src/biabduction/SymExec.ml @@ -781,8 +781,7 @@ let receiver_self receiver prop = ~f:(fun hpred -> match hpred with | Sil.Hpointsto (Exp.Lvar pv, Sil.Eexp (e, _), _) -> - Exp.equal e receiver && Pvar.is_seed pv - && Mangled.equal (Pvar.get_name pv) (Mangled.from_string "self") + Exp.equal e receiver && Pvar.is_seed pv && Pvar.is_self pv | _ -> false ) prop.Prop.sigma diff --git a/infer/src/clang/cPredicates.ml b/infer/src/clang/cPredicates.ml index 1c069c9c5..618c7b396 100644 --- a/infer/src/clang/cPredicates.ml +++ b/infer/src/clang/cPredicates.ml @@ -983,7 +983,7 @@ let is_receiver_self an = | Ctl_parser_types.Stmt (Clang_ast_t.ObjCMessageExpr (_, fst_param :: _, _, _)) -> CAst_utils.exists_eventually_st (decl_ref_name ~kind:`ImplicitParam) - (ALVar.Const "self") fst_param + (ALVar.Const CFrontend_config.self) fst_param | _ -> false diff --git a/infer/src/concurrency/RacerD.ml b/infer/src/concurrency/RacerD.ml index b0e4199e5..5f5986985 100644 --- a/infer/src/concurrency/RacerD.ml +++ b/infer/src/concurrency/RacerD.ml @@ -1254,31 +1254,20 @@ let report_unsafe_accesses (aggregated_access_map : ReportMap.t) = { reported_acc with reported_writes= Typ.Procname.Set.empty; reported_reads= Typ.Procname.Set.empty } in - let class_has_mutex_member objc_cpp tenv = - let class_name = Typ.Procname.ObjC_Cpp.get_class_type_name objc_cpp in - let matcher = ConcurrencyModels.cpp_lock_types_matcher in - 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 -> - QualifiedCppName.Match.match_qualifiers matcher (Typ.Name.qual_name name) ) ) ) - in let should_report {tenv; procdesc} = + List.exists grouped_accesses ~f:(fun ({threads} : reported_access) -> + ThreadsDomain.is_any threads ) + && match Procdesc.get_proc_name procdesc with | Java _ -> - List.exists grouped_accesses ~f:(fun ({threads} : reported_access) -> - ThreadsDomain.is_any threads ) - && should_report_on_proc procdesc tenv + should_report_in_java procdesc tenv | ObjC_Cpp objc_cpp -> - (* do not report if a procedure is private *) - Procdesc.get_access procdesc <> PredSymb.Private - && (* report if the class has a mutex member *) - class_has_mutex_member objc_cpp tenv + should_report_in_objcpp procdesc objc_cpp tenv | _ -> false in let reportable_accesses = List.filter ~f:should_report grouped_accesses in - List.fold ~f:(report_unsafe_access reportable_accesses) reportable_accesses ~init:reported + List.fold reportable_accesses ~init:reported ~f:(report_unsafe_access reportable_accesses) in ReportMap.fold report_accesses_on_location aggregated_access_map empty_reported |> ignore @@ -1291,43 +1280,37 @@ let make_results_table file_env = let open RacerDDomain in let aggregate_post tenv procdesc acc {threads; accesses; wobbly_paths} = AccessDomain.fold - (fun snapshot acc -> - let reported_access : reported_access = - {threads; snapshot; tenv; procdesc; wobbly_paths} - in - ReportMap.add reported_access acc ) + (fun snapshot acc -> ReportMap.add {threads; snapshot; tenv; procdesc; wobbly_paths} acc) accesses acc in let aggregate_posts acc (tenv, proc_desc) = Payload.read proc_desc (Procdesc.get_proc_name proc_desc) |> Option.fold ~init:acc ~f:(aggregate_post tenv proc_desc) in - List.fold ~f:aggregate_posts file_env ~init:ReportMap.empty + List.fold file_env ~init:ReportMap.empty ~f:aggregate_posts (* aggregate all of the procedures in the file env by their declaring class. this lets us analyze each class individually *) let aggregate_by_class file_env = - List.fold file_env - ~f:(fun acc ((_, pdesc) as proc) -> - let pname = Procdesc.get_proc_name pdesc in + List.fold file_env ~init:String.Map.empty ~f:(fun acc ((_, pdesc) as proc) -> let classname = - match pname with + match Procdesc.get_proc_name pdesc with | Typ.Procname.Java java_pname -> - Typ.Procname.Java.get_class_name java_pname + Some (Typ.Procname.Java.get_class_name java_pname) + | ObjC_Cpp objc_cpp_pname -> + Some (Typ.Procname.ObjC_Cpp.get_class_name objc_cpp_pname) | _ -> - "unknown" + None in - String.Map.update acc classname ~f:(function None -> [proc] | Some bucket -> proc :: bucket) - ) - ~init:String.Map.empty + Option.fold classname ~init:acc ~f:(fun acc classname -> + String.Map.add_multi acc ~key:classname ~data:proc ) ) (* Gathers results by analyzing all the methods in a file, then post-processes the results to check an (approximation of) thread safety *) let file_analysis {Callbacks.procedures; source_file} = - String.Map.iter - ~f:(fun class_env -> report_unsafe_accesses (make_results_table class_env)) - (aggregate_by_class procedures) ; + String.Map.iter (aggregate_by_class procedures) ~f:(fun class_env -> + report_unsafe_accesses (make_results_table class_env) ) ; IssueLog.store Config.racerd_issues_dir_name source_file diff --git a/infer/src/concurrency/RacerDModels.ml b/infer/src/concurrency/RacerDModels.ml index 6511b9189..c31b880b6 100644 --- a/infer/src/concurrency/RacerDModels.ml +++ b/infer/src/concurrency/RacerDModels.ml @@ -371,8 +371,8 @@ let is_marked_thread_safe pdesc tenv = (* return true if procedure is at an abstraction boundary or reporting has been explicitly - requested via @ThreadSafe *) -let should_report_on_proc proc_desc tenv = + requested via @ThreadSafe in java *) +let should_report_in_java proc_desc tenv = let proc_name = Procdesc.get_proc_name proc_desc in is_thread_safe_method proc_name tenv || (not @@ -383,3 +383,15 @@ let should_report_on_proc proc_desc tenv = false )) && Procdesc.get_access proc_desc <> PredSymb.Private && not (Annotations.pdesc_return_annot_ends_with proc_desc Annotations.visibleForTesting) + + +let should_report_in_objcpp proc_desc objc_cpp tenv = + Procdesc.get_access proc_desc <> PredSymb.Private + && + let class_name = Typ.Procname.ObjC_Cpp.get_class_type_name objc_cpp in + let matcher = ConcurrencyModels.cpp_lock_types_matcher in + 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 -> + QualifiedCppName.Match.match_qualifiers matcher (Typ.Name.qual_name name) ) ) ) diff --git a/infer/src/concurrency/RacerDModels.mli b/infer/src/concurrency/RacerDModels.mli index 579f1d233..b7fc5a1c9 100644 --- a/infer/src/concurrency/RacerDModels.mli +++ b/infer/src/concurrency/RacerDModels.mli @@ -51,6 +51,9 @@ val is_thread_safe_method : Typ.Procname.t -> Tenv.t -> bool val is_marked_thread_safe : Procdesc.t -> Tenv.t -> bool -val should_report_on_proc : Procdesc.t -> Tenv.t -> bool +val should_report_in_java : Procdesc.t -> Tenv.t -> bool (** return true if procedure is at an abstraction boundary or reporting has been explicitly requested via @ThreadSafe *) + +val should_report_in_objcpp : Procdesc.t -> Typ.Procname.ObjC_Cpp.t -> Tenv.t -> bool +(** check if the class contains a lock member *) diff --git a/infer/tests/codetoanalyze/objcpp/racerd/Basic.mm b/infer/tests/codetoanalyze/objcpp/racerd/Basic.mm new file mode 100644 index 000000000..42a382822 --- /dev/null +++ b/infer/tests/codetoanalyze/objcpp/racerd/Basic.mm @@ -0,0 +1,29 @@ +/* + * 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 + +@interface Basic : NSObject +- (int)read; +- (void)write:(int)data; +@end + +@implementation Basic { + std::mutex mutex_; + int data_; +} + +- (int)read { + return data_; +} + +- (void)write:(int)data { + mutex_.lock(); + data_ = data; + mutex_.unlock(); +} +@end diff --git a/infer/tests/codetoanalyze/objcpp/racerd/Makefile b/infer/tests/codetoanalyze/objcpp/racerd/Makefile new file mode 100644 index 000000000..51b3bf045 --- /dev/null +++ b/infer/tests/codetoanalyze/objcpp/racerd/Makefile @@ -0,0 +1,19 @@ +# Copyright (c) 2016-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. + +TESTS_DIR = ../../.. + +CLANG_OPTIONS = -c $(OBJCPP_CLANG_OPTIONS) + +INFER_OPTIONS = --racerd-only --debug-exceptions --project-root $(TESTS_DIR) + +INFERPRINT_OPTIONS = --issues-tests + +SOURCES = $(wildcard *.mm) + +include $(TESTS_DIR)/clang.make +include $(TESTS_DIR)/objc.make + +infer-out/report.json: $(MAKEFILE_LIST) diff --git a/infer/tests/codetoanalyze/objcpp/racerd/issues.exp b/infer/tests/codetoanalyze/objcpp/racerd/issues.exp new file mode 100644 index 000000000..8bb20fc28 --- /dev/null +++ b/infer/tests/codetoanalyze/objcpp/racerd/issues.exp @@ -0,0 +1 @@ +codetoanalyze/objcpp/racerd/Basic.mm, Basic_read, 21, LOCK_CONSISTENCY_VIOLATION, no_bucket, ERROR, [,access to `self.data_`,,access to `self.data_`]