From 23a0a611dc24788c028530a21ae623c259523460 Mon Sep 17 00:00:00 2001 From: Peter O'Hearn Date: Fri, 16 Dec 2016 10:38:19 -0800 Subject: [PATCH] [threadsafety] Fix situation where @NotThreadSafe is reported on, when super is @ThreadSafe Summary: Don't warn on NotThreadSafe class, particularly when super is ThreadSafe Reviewed By: sblackshear Differential Revision: D4334417 fbshipit-source-id: 0df3b9d --- infer/src/checkers/ThreadSafety.ml | 5 ++++ infer/src/checkers/annotations.ml | 5 +++- infer/src/checkers/annotations.mli | 1 + infer/src/checkers/patternMatch.ml | 14 ++++++++++- infer/src/checkers/patternMatch.mli | 7 +++++- .../java/threadsafety/ThreadSafeExample.java | 24 +++++++++++++++++++ .../java/threadsafety/issues.exp | 1 + 7 files changed, 54 insertions(+), 3 deletions(-) diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml index 7e3f65a62..2f77dca43 100644 --- a/infer/src/checkers/ThreadSafety.ml +++ b/infer/src/checkers/ThreadSafety.ml @@ -364,6 +364,11 @@ let should_report_on_file file_env = not (thread_safe_annotated_classes = []) | _ -> false in + let current_class_marked_not_threadsafe = + fun (_, tenv, pname, _) -> + PatternMatch.check_current_class_attributes Annotations.ia_is_not_thread_safe tenv pname + in + not (IList.exists current_class_marked_not_threadsafe file_env) && IList.exists current_class_or_super_marked_threadsafe file_env (*This is a "cluster checker" *) diff --git a/infer/src/checkers/annotations.ml b/infer/src/checkers/annotations.ml index 93fa6bf7c..424a81388 100644 --- a/infer/src/checkers/annotations.ml +++ b/infer/src/checkers/annotations.ml @@ -115,10 +115,13 @@ let guarded_by = "GuardedBy" let thread_safe = "ThreadSafe" let not_thread_safe = "NotThreadSafe" +let ia_is_not_thread_safe ia = + ia_ends_with ia not_thread_safe + (* we don't want it to just end with ThreadSafe, because this falls foul of the @NotThreadSafe annotation *) let ia_is_thread_safe ia = - ia_ends_with ia thread_safe && not (ia_ends_with ia not_thread_safe) + ia_ends_with ia thread_safe let ia_is_nullable ia = ia_ends_with ia nullable diff --git a/infer/src/checkers/annotations.mli b/infer/src/checkers/annotations.mli index db34194ff..753025f99 100644 --- a/infer/src/checkers/annotations.mli +++ b/infer/src/checkers/annotations.mli @@ -93,6 +93,7 @@ val ia_is_privacy_sink : Annot.Item.t -> bool val ia_is_integrity_source : Annot.Item.t -> bool val ia_is_integrity_sink : Annot.Item.t -> bool val ia_is_guarded_by : Annot.Item.t -> bool +val ia_is_not_thread_safe : Annot.Item.t -> bool val ia_is_thread_safe : Annot.Item.t -> bool val ia_iter : (Annot.t -> unit) -> Annot.Item.t -> unit diff --git a/infer/src/checkers/patternMatch.ml b/infer/src/checkers/patternMatch.ml index 6ebb93bb2..346a5c80b 100644 --- a/infer/src/checkers/patternMatch.ml +++ b/infer/src/checkers/patternMatch.ml @@ -371,7 +371,8 @@ let is_exception tenv typename = let is_throwable tenv typename = is_subtype_of_str tenv typename "java.lang.Throwable" -(** tests whether any class attributes (e.g., @ThreadSafe) pass check of first argument*) +(** tests whether any class attributes (e.g., @ThreadSafe) pass check of first argument, + including for supertypes*) let check_class_attributes check tenv = function | Procname.Java java_pname -> let check_class_annots _ { StructTyp.annots; } = check annots in @@ -380,6 +381,17 @@ let check_class_attributes check tenv = function (Procname.java_get_class_type_name java_pname) | _ -> false +(** tests whether any class attributes (e.g., @ThreadSafe) pass check of first argument, + for the current class only*) +let check_current_class_attributes check tenv = function + | Procname.Java java_pname -> + (match Tenv.lookup tenv (Procname.java_get_class_type_name java_pname) with + | Some (struct_typ) -> check struct_typ.annots + | _ -> false + ) + | _ -> false + + (** find superclasss with attributes (e.g., @ThreadSafe), including current class*) let rec find_superclasses_with_attributes check tenv tname = match Tenv.lookup tenv tname with diff --git a/infer/src/checkers/patternMatch.mli b/infer/src/checkers/patternMatch.mli index d1ad3c458..d6ca7e2f9 100644 --- a/infer/src/checkers/patternMatch.mli +++ b/infer/src/checkers/patternMatch.mli @@ -111,9 +111,14 @@ val is_throwable : Tenv.t -> Typename.t -> bool of type java.lang.RuntimeException *) val is_runtime_exception : Tenv.t -> Typename.t -> bool -(** tests whether any class attributes (e.g., @ThreadSafe) pass check of first argument*) +(** tests whether any class attributes (e.g., @ThreadSafe) pass check of first argument, + including supertypes*) val check_class_attributes : (Annot.Item.t -> bool) -> Tenv.t -> Procname.t -> bool +(** tests whether any class attributes (e.g., @ThreadSafe) pass check of first argument, + for current class only*) +val check_current_class_attributes : (Annot.Item.t -> bool) -> Tenv.t -> Procname.t -> bool + (** find superclasss with attributes (e.g., @ThreadSafe), including current class*) val find_superclasses_with_attributes : (Annot.Item.t -> bool) -> Tenv.t -> Typename.t -> Typename.t list diff --git a/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java b/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java index 0ed7a330e..713686fae 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java +++ b/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java @@ -107,3 +107,27 @@ class ExtendsThreadSafeExample extends ThreadSafeExample{ } } + +@NotThreadSafe +class NotThreadSafeExtendsThreadSafeExample extends ThreadSafeExample{ + + Integer field; + +/* We don't want to warn on this */ + public void newmethodBad() { + field = 22; + } + +} + +@ThreadSafe +class YesThreadSafeExtendsNotThreadSafeExample extends NotThreadSafeExtendsThreadSafeExample{ + + Integer subsubfield; + +/* We do want to warn on this */ + public void subsubmethodBad() { + subsubfield = 22; + } + +} diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp index 5da645ff3..2be758f37 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp +++ b/infer/tests/codetoanalyze/java/threadsafety/issues.exp @@ -17,3 +17,4 @@ codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.o codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.recursiveBad(), 1, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.ThreadSafeExample.f] codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.recursiveBad(), 2, THREAD_SAFETY_ERROR, [call to void ThreadSafeExample.recursiveBad(),access to codetoanalyze.java.checkers.ThreadSafeExample.f] codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.tsBad(), 1, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.ThreadSafeExample.f] +codetoanalyze/java/threadsafety/ThreadSafeExample.java, void YesThreadSafeExtendsNotThreadSafeExample.subsubmethodBad(), 1, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.YesThreadSafeExtendsNotThreadSafeExample.subsubfield]