From e0d7950e0777aa04f204584ee644c7e35bae1e79 Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Mon, 7 Oct 2019 09:13:18 -0700 Subject: [PATCH] [concurrency] fix over-eager modeling of thread status Summary: At some point it was thought that we can assume that any annotation starting with "On" means the method is on the UI thread. That's too imprecise and has led to false positives and negatives. Restrict to a well-known safe set. Reviewed By: ezgicicek Differential Revision: D17769376 fbshipit-source-id: 0f8fee059 --- infer/src/checkers/annotations.ml | 19 +++++++++++++++---- infer/src/checkers/annotations.mli | 5 ++--- infer/src/concurrency/ConcurrencyModels.ml | 14 +------------- .../java/racerd/Annotations.java | 4 ++-- .../codetoanalyze/java/racerd/issues.exp | 1 + 5 files changed, 21 insertions(+), 22 deletions(-) diff --git a/infer/src/checkers/annotations.ml b/infer/src/checkers/annotations.ml index f71a93dbd..35ad91ad0 100644 --- a/infer/src/checkers/annotations.ml +++ b/infer/src/checkers/annotations.ml @@ -67,6 +67,16 @@ let notnull = "NotNull" let not_thread_safe = "NotThreadSafe" +let on_bind = "OnBind" + +let on_event = "OnEvent" + +let on_mount = "OnMount" + +let on_unbind = "OnUnbind" + +let on_unmount = "OnUnmount" + let performance_critical = "PerformanceCritical" let prop = "Prop" @@ -111,10 +121,6 @@ let ma_has_annotation_with ({return; params} : Annot.Method.t) (predicate : Anno has_annot return || List.exists ~f:has_annot params -let get_annot_ending ({class_name} : Annot.t) = - String.rsplit2 class_name ~on:'.' |> Option.value_map ~default:class_name ~f:snd - - (** [annot_ends_with annot ann_name] returns true if the class name of [annot], without the package, is equal to [ann_name] *) let annot_ends_with ({class_name} : Annot.t) ann_name = @@ -232,3 +238,8 @@ let ia_is_suppress_lint ia = ia_ends_with ia suppress_lint let ia_is_thread_confined ia = ia_ends_with ia thread_confined let ia_is_worker_thread ia = ia_ends_with ia worker_thread + +(* methods annotated with the annotations below always run on the UI thread. *) +let ia_is_uithread_equivalent = + let annotations = [mainthread; ui_thread; on_bind; on_event; on_mount; on_unbind; on_unmount] in + fun ia -> List.exists annotations ~f:(ia_ends_with ia) diff --git a/infer/src/checkers/annotations.mli b/infer/src/checkers/annotations.mli index 6489069b2..e61c85d2b 100644 --- a/infer/src/checkers/annotations.mli +++ b/infer/src/checkers/annotations.mli @@ -51,9 +51,6 @@ val visibleForTesting : string val generated_graphql : string -val get_annot_ending : Annot.t -> string -(** get the '.'-last component of an annotation *) - val annot_ends_with : Annot.t -> string -> bool (** [annot_ends_with annot ann_name] returns true if the class name of [annot], without the package, is equal to [ann_name] *) @@ -115,6 +112,8 @@ val ia_is_volatile : Annot.Item.t -> bool val ia_is_worker_thread : Annot.Item.t -> bool +val ia_is_uithread_equivalent : Annot.Item.t -> bool + val pdesc_get_return_annot : Procdesc.t -> Annot.Item.t (** get the list of annotations on the return value of [pdesc] *) diff --git a/infer/src/concurrency/ConcurrencyModels.ml b/infer/src/concurrency/ConcurrencyModels.ml index 0f83b7c0f..5e95f1e22 100644 --- a/infer/src/concurrency/ConcurrencyModels.ml +++ b/infer/src/concurrency/ConcurrencyModels.ml @@ -376,22 +376,10 @@ let if_pred_evalopt ~pred ~f x = IOption.if_none_evalopt x ~f:(fun () -> if pred () then Some (f ()) else None) -(* assume that methods annotated with @MainThread, @UiThread, - and any string starting with "On" always run on the UI thread. - We do the latter because there are too many to precisely list. *) -let is_uithread annots = - let f (annot, _) = - let ending = Annotations.get_annot_ending annot in - String.equal ending Annotations.mainthread - || String.equal ending Annotations.ui_thread - || String.is_prefix ~prefix:"On" ending - in - List.exists annots ~f - - let mono_pname = MF.wrap_monospaced Typ.Procname.pp let runs_on_ui_thread ~attrs_of_pname tenv proc_desc = + let is_uithread = Annotations.ia_is_uithread_equivalent in let pname = Procdesc.get_proc_name proc_desc in if is_method_or_override_annotated ~attrs_of_pname Annotations.ia_is_worker_thread pname tenv diff --git a/infer/tests/codetoanalyze/java/racerd/Annotations.java b/infer/tests/codetoanalyze/java/racerd/Annotations.java index 6eda90250..783f14180 100644 --- a/infer/tests/codetoanalyze/java/racerd/Annotations.java +++ b/infer/tests/codetoanalyze/java/racerd/Annotations.java @@ -440,7 +440,7 @@ class ExtendsClassOnUiThread extends AllMethodsOnUiThread { } } -// All annotations that start with "On" are assumed to be on the main thread +// NOT All annotations that start with "On" are on the main thread @Target(ElementType.METHOD) @Retention(RetentionPolicy.CLASS) @interface OnXYZ {} @@ -450,7 +450,7 @@ class WeirdAnnotation { int f; @OnXYZ - void fooOk() { + void fooBad() { f = 0; } } diff --git a/infer/tests/codetoanalyze/java/racerd/issues.exp b/infer/tests/codetoanalyze/java/racerd/issues.exp index b96f2990d..15eea1d8e 100644 --- a/infer/tests/codetoanalyze/java/racerd/issues.exp +++ b/infer/tests/codetoanalyze/java/racerd/issues.exp @@ -12,6 +12,7 @@ codetoanalyze/java/racerd/Annotations.java, codetoanalyze.java.checkers.Annotati codetoanalyze/java/racerd/Annotations.java, codetoanalyze.java.checkers.Annotations.read_off_UI_thread_Bad():void, 190, THREAD_SAFETY_VIOLATION, no_bucket, WARNING, [,access to `this.f`,,access to `this.f`] codetoanalyze/java/racerd/Annotations.java, codetoanalyze.java.checkers.ThreadSafeAlias.threadSafeAliasBad1():void, 82, THREAD_SAFETY_VIOLATION, no_bucket, WARNING, [access to `this.field`] codetoanalyze/java/racerd/Annotations.java, codetoanalyze.java.checkers.ThreadSafeAlias.threadSafeAliasBad2():void, 87, THREAD_SAFETY_VIOLATION, no_bucket, WARNING, [access to `this.field`] +codetoanalyze/java/racerd/Annotations.java, codetoanalyze.java.checkers.WeirdAnnotation.fooBad():void, 454, THREAD_SAFETY_VIOLATION, no_bucket, WARNING, [access to `this.f`] codetoanalyze/java/racerd/Arrays.java, Arrays.arrayParameterWriteBad(int[]):void, 26, THREAD_SAFETY_VIOLATION, no_bucket, WARNING, [access to `name1.[_]`] codetoanalyze/java/racerd/Arrays.java, Arrays.readWriteRaceBad(java.lang.String):java.lang.String, 47, THREAD_SAFETY_VIOLATION, no_bucket, WARNING, [,access to `this.strArr1.[_]`,,access to `this.strArr1.[_]`] codetoanalyze/java/racerd/Arrays.java, Arrays.writeWriteRaceBad(java.lang.String):void, 39, THREAD_SAFETY_VIOLATION, no_bucket, WARNING, [access to `this.strArr1.[_]`]