diff --git a/infer/annotations/com/facebook/infer/annotation/ThreadConfined.java b/infer/annotations/com/facebook/infer/annotation/ThreadConfined.java index fe1ae2103..60424da1e 100644 --- a/infer/annotations/com/facebook/infer/annotation/ThreadConfined.java +++ b/infer/annotations/com/facebook/infer/annotation/ThreadConfined.java @@ -15,7 +15,7 @@ import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; -@Target(ElementType.TYPE) +@Target({ ElementType.TYPE, ElementType.FIELD }) @Retention(RetentionPolicy.CLASS) public @interface ThreadConfined { } diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml index 09b58828a..1ddd14746 100644 --- a/infer/src/checkers/ThreadSafety.ml +++ b/infer/src/checkers/ThreadSafety.ml @@ -64,18 +64,29 @@ module TransferFunctions (CFG : ProcCfg.S) = struct try Some (IdAccessPathMapDomain.find id id_map) with Not_found -> None - let add_path_to_state exp typ loc path_state id_map owned = + let add_path_to_state exp typ loc path_state id_map owned tenv = (* remove the last field of the access path, if it has any *) let truncate = function | base, [] | base, _ :: [] -> base, [] | base, accesses -> base, IList.rev (IList.tl (IList.rev accesses)) in + let is_thread_confined (_, accesses) = match IList.rev accesses with + | AccessPath.FieldAccess (fieldname, Typ.Tstruct typename) :: _ -> + begin + match Tenv.lookup tenv typename with + | Some struct_typ -> + Annotations.field_has_annot fieldname struct_typ Annotations.ia_is_thread_confined + | None -> + false + end + | _ -> + false in let f_resolve_id = resolve_id id_map in IList.fold_left (fun acc rawpath -> - let base_path = truncate rawpath in - if not (ThreadSafetyDomain.OwnershipDomain.mem base_path owned) + if not (ThreadSafetyDomain.OwnershipDomain.mem (truncate rawpath) owned) && + not (is_thread_confined rawpath) then ThreadSafetyDomain.PathDomain.add_sink (ThreadSafetyDomain.make_access rawpath loc) acc else @@ -114,7 +125,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct Ident.create_fieldname (Mangled.from_string (Procname.get_method pn)) 0 in let dummy_access_exp = Exp.Lfield (exp, dummy_fieldname, typ) in let writes = - add_path_to_state dummy_access_exp typ loc astate.writes astate.id_map astate.owned in + add_path_to_state dummy_access_exp typ loc astate.writes astate.id_map astate.owned tenv in { astate with writes; } in let is_unprotected is_locked = not is_locked && not (Procdesc.is_java_synchronized pdesc) in @@ -182,7 +193,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct let writes' = match lhs_exp with | Lfield ( _, _, typ) when is_unprotected locks -> (* abstracts no lock being held *) - add_path_to_state lhs_exp typ loc writes id_map owned + add_path_to_state lhs_exp typ loc writes id_map owned tenv | _ -> writes in (* if rhs is owned, propagate ownership to lhs. otherwise, remove lhs from ownerhsip set (since it may have previously held an owned memory loc and is now being reassigned *) @@ -203,7 +214,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct let reads' = match rhs_exp with | Lfield ( _, _, typ) when is_unprotected locks -> - add_path_to_state rhs_exp typ loc reads id_map owned + add_path_to_state rhs_exp typ loc reads id_map owned tenv | _ -> reads in (* if rhs is owned, propagate ownership to lhs *) diff --git a/infer/src/checkers/annotations.ml b/infer/src/checkers/annotations.ml index 33459d387..8fe684ce1 100644 --- a/infer/src/checkers/annotations.ml +++ b/infer/src/checkers/annotations.ml @@ -81,6 +81,12 @@ let ma_contains ma ann_names = let pdesc_has_annot pdesc annot = ma_contains (Procdesc.get_attributes pdesc).ProcAttributes.method_annotation [annot] +let field_has_annot fieldname (struct_typ : StructTyp.t) f = + let fld_has_taint_annot (fname, _, annot) = + Ident.equal_fieldname fieldname fname && f annot in + IList.exists fld_has_taint_annot struct_typ.fields || + IList.exists fld_has_taint_annot struct_typ.statics + let initializer_ = "Initializer" let inject = "Inject" let inject_view = "InjectView" diff --git a/infer/src/checkers/annotations.mli b/infer/src/checkers/annotations.mli index d72bf243a..a17d47a05 100644 --- a/infer/src/checkers/annotations.mli +++ b/infer/src/checkers/annotations.mli @@ -109,6 +109,8 @@ val ma_has_annotation_with : Annot.Method.t -> (Annot.t -> bool) -> bool val pdesc_has_annot : Procdesc.t -> string -> bool +val field_has_annot : Ident.fieldname -> StructTyp.t -> (Annot.Item.t -> bool) -> bool + (** Mark the return of the method_annotation with the given annotation. *) val method_annotation_mark_return : annotation -> Annot.Method.t -> Annot.Method.t diff --git a/infer/tests/codetoanalyze/java/threadsafety/Annotations.java b/infer/tests/codetoanalyze/java/threadsafety/Annotations.java index afd7662c0..434c1bb6c 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/Annotations.java +++ b/infer/tests/codetoanalyze/java/threadsafety/Annotations.java @@ -52,4 +52,22 @@ class Annotations { } } + static class Obj { + Object fld; + } + + @ThreadConfined Obj encapsulatedField; + + public void mutateConfinedFieldDirectlyOk() { + this.encapsulatedField = new Obj(); + } + + public static void mutateConfinedFieldIndirectlyOk(Annotations a) { + a.encapsulatedField = new Obj(); + } + + public void mutateSubfieldOfConfinedBad() { + this.encapsulatedField.fld = new Object(); + } + } diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp index 509dd2a00..69fc0bff6 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp +++ b/infer/tests/codetoanalyze/java/threadsafety/issues.exp @@ -1,4 +1,5 @@ codetoanalyze/java/threadsafety/Annotations.java, void Annotations.mutateOffUiThreadBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Annotations.f] +codetoanalyze/java/threadsafety/Annotations.java, void Annotations.mutateSubfieldOfConfinedBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Annotations.encapsulatedField.codetoanalyze.java.checkers.Annotations$Obj.fld] codetoanalyze/java/threadsafety/Builders.java, Builders$Obj Builders.buildThenMutateBad(Builders$Obj), 3, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Builders$Obj.g] codetoanalyze/java/threadsafety/Builders.java, Builders$Obj Builders.mutateBad(Builders$Obj), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Builders$Obj.g] codetoanalyze/java/threadsafety/Containers.java, void Containers.mapClearBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Containers.mMap.clear]