diff --git a/infer/src/checkers/annotations.ml b/infer/src/checkers/annotations.ml index 3caa41fbb..4b97bb2be 100644 --- a/infer/src/checkers/annotations.ml +++ b/infer/src/checkers/annotations.ml @@ -251,3 +251,5 @@ let ia_is_mainthread ia = ia_ends_with ia mainthread let ia_is_thread_confined ia = ia_ends_with ia thread_confined let ia_is_worker_thread ia = ia_ends_with ia worker_thread + +let ia_is_guardedby ia = ia_ends_with ia guarded_by diff --git a/infer/src/checkers/annotations.mli b/infer/src/checkers/annotations.mli index c1d5d6e64..32265cab6 100644 --- a/infer/src/checkers/annotations.mli +++ b/infer/src/checkers/annotations.mli @@ -84,6 +84,8 @@ val ia_is_expensive : Annot.Item.t -> bool val ia_is_functional : Annot.Item.t -> bool +val ia_is_guardedby : Annot.Item.t -> bool + val ia_is_propagates_nullable : Annot.Item.t -> bool val ia_is_ignore_allocations : Annot.Item.t -> bool diff --git a/infer/src/concurrency/RacerD.ml b/infer/src/concurrency/RacerD.ml index 4e0a979b7..b2e6e671f 100644 --- a/infer/src/concurrency/RacerD.ml +++ b/infer/src/concurrency/RacerD.ml @@ -936,14 +936,25 @@ let should_report_on_proc tenv procdesc = false -let should_report_guardedby_violation ({snapshot; tenv} : reported_access) = +let should_report_guardedby_violation classname_str ({snapshot; tenv; procdesc} : reported_access) + = (not snapshot.lock) - && RacerDDomain.Access.get_access_path snapshot.access.elem - |> Option.exists ~f:(fun path -> - AccessPath.get_field_and_annotation path tenv - |> Option.exists ~f:(fun (_, annotlist) -> - List.exists annotlist ~f:(fun (annot, _) -> - Annotations.annot_ends_with annot Annotations.guarded_by ) ) ) + && Procdesc.get_proc_name procdesc |> Typ.Procname.is_java + && + (* restrict check to access paths of length one *) + match RacerDDomain.Access.get_access_path snapshot.access.elem with + | Some ((_, base_type), [AccessPath.FieldAccess field_name]) -> ( + Typ.Struct.get_field_type_and_annotation ~lookup:(Tenv.lookup tenv) field_name base_type + |> Option.exists ~f:(fun (_, annotlist) -> Annotations.ia_is_guardedby annotlist) + && + (* is the base class a subclass of the one containing the GuardedBy annotation? *) + match base_type.Typ.desc with + | Tstruct base_name | Tptr ({Typ.desc= Tstruct base_name}, _) -> + PatternMatch.is_subtype tenv base_name (Typ.Name.Java.from_string classname_str) + | _ -> + false ) + | _ -> + false (** Report accesses that may race with each other. @@ -974,7 +985,7 @@ let should_report_guardedby_violation ({snapshot; tenv} : reported_access) = currently not distinguishing different locks, and are treating "known to be confined to a thread" as if "known to be confined to UI thread". *) -let report_unsafe_accesses (aggregated_access_map : ReportMap.t) = +let report_unsafe_accesses classname (aggregated_access_map : ReportMap.t) = let open RacerDDomain in let open RacerDModels in let is_duplicate_report ({snapshot; procdesc} : reported_access) @@ -1095,13 +1106,14 @@ let report_unsafe_accesses (aggregated_access_map : ReportMap.t) = (* Don't report on location if all accesses are on non-concurrent contexts *) if List.for_all reportable_accesses ~f:(fun ({threads} : reported_access) -> + (* FIXME this should be any thread or no thread *) ThreadsDomain.is_any threads |> not ) then init else List.fold reportable_accesses ~init ~f:(report_unsafe_access reportable_accesses) in let report_guardedby_violations_on_location grouped_accesses init = List.fold grouped_accesses ~init ~f:(fun acc r -> - if should_report_guardedby_violation r && not (is_duplicate_report r acc) then ( + if should_report_guardedby_violation classname r && not (is_duplicate_report r acc) then ( report_thread_safety_violation ~make_description:make_guardedby_violation_description ~report_kind:GuardedByViolation r ; update_reported r acc ) @@ -1151,5 +1163,6 @@ let aggregate_by_class file_env = safety *) let file_analysis {Callbacks.procedures; source_file} = aggregate_by_class procedures - |> String.Map.iter ~f:(fun class_env -> report_unsafe_accesses (make_results_table class_env)) ; + |> String.Map.iteri ~f:(fun ~key:classname ~data:class_env -> + report_unsafe_accesses classname (make_results_table class_env) ) ; IssueLog.store Config.racerd_issues_dir_name source_file diff --git a/infer/tests/codetoanalyze/java/racerd/GuardedByTests.java b/infer/tests/codetoanalyze/java/racerd/GuardedByTests.java index e67e5dc6a..f95db9e43 100644 --- a/infer/tests/codetoanalyze/java/racerd/GuardedByTests.java +++ b/infer/tests/codetoanalyze/java/racerd/GuardedByTests.java @@ -89,4 +89,23 @@ public class GuardedByTests { synchronized int syncReadOk() { return g; } + + GuardedByOther o; + + void accessThroughMemberObjectOk() { + o.accessBad(); + } + + void accessIndirectOk(GuardedByOther o) { + o.accessBad(); + } +} + +class GuardedByOther { + @GuardedBy("bla") + int x; + + void accessBad() { + x = 0; + } } diff --git a/infer/tests/codetoanalyze/java/racerd/issues.exp b/infer/tests/codetoanalyze/java/racerd/issues.exp index 472abf5ef..cef8324c8 100644 --- a/infer/tests/codetoanalyze/java/racerd/issues.exp +++ b/infer/tests/codetoanalyze/java/racerd/issues.exp @@ -51,6 +51,7 @@ codetoanalyze/java/racerd/DeepOwnership.java, DeepOwnership.reassignBaseToGlobal codetoanalyze/java/racerd/Dispatch.java, codetoanalyze.java.checkers.Dispatch.callUnannotatedInterfaceBad(codetoanalyze.java.checkers.UnannotatedInterface):void, 49, INTERFACE_NOT_THREAD_SAFE, no_bucket, ERROR, [Call to un-annotated interface method void UnannotatedInterface.foo()] codetoanalyze/java/racerd/Dispatch.java, codetoanalyze.java.checkers.Dispatch.callUnannotatedInterfaceIndirectBad(codetoanalyze.java.checkers.NotThreadSafe,codetoanalyze.java.checkers.UnannotatedInterface):void, 53, INTERFACE_NOT_THREAD_SAFE, no_bucket, ERROR, [call to void NotThreadSafe.notThreadSafeOk(UnannotatedInterface),Call to un-annotated interface method void UnannotatedInterface.foo()] codetoanalyze/java/racerd/Dispatch.java, codetoanalyze.java.checkers.ThreadConfinedField.interfaceCallOnNormalFieldBad():void, 102, INTERFACE_NOT_THREAD_SAFE, no_bucket, ERROR, [Call to un-annotated interface method void UnannotatedInterface.foo()] +codetoanalyze/java/racerd/GuardedByTests.java, codetoanalyze.java.infer.GuardedByOther.accessBad():void, 109, GUARDEDBY_VIOLATION, no_bucket, ERROR, [access to `this.x`] codetoanalyze/java/racerd/GuardedByTests.java, codetoanalyze.java.infer.GuardedByTests.interprocUnlockedReadBad():int, 58, GUARDEDBY_VIOLATION, no_bucket, ERROR, [call to int GuardedByTests.privateUnlockedReadOk(),access to `this.f`] codetoanalyze/java/racerd/GuardedByTests.java, codetoanalyze.java.infer.GuardedByTests.interprocUnlockedReadBad():int, 58, THREAD_SAFETY_VIOLATION, no_bucket, ERROR, [,call to int GuardedByTests.privateUnlockedReadOk(),access to `this.f`,,access to `this.f`] codetoanalyze/java/racerd/GuardedByTests.java, codetoanalyze.java.infer.GuardedByTests.interprocUnlockedWriteBad():void, 54, GUARDEDBY_VIOLATION, no_bucket, ERROR, [call to void GuardedByTests.privateUnlockedWriteOk(),access to `this.f`]