From 68c0705f26a6d8ee8c56d3a4eec054df054a0f12 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Thu, 5 Jan 2017 10:45:45 -0800 Subject: [PATCH] [thread-safety] don't warn on methods annotated with UiThread Summary: These methods should only be called from other methods that also run on the UI thread, and they should not be starting new threads. Reviewed By: peterogithub Differential Revision: D4383133 fbshipit-source-id: 6cb2e40 --- infer/src/checkers/ThreadSafety.ml | 120 +++++++++++------- infer/src/checkers/annotations.ml | 4 + infer/src/checkers/annotations.mli | 1 + .../java/threadsafety/Annotations.java | 38 ++++++ .../java/threadsafety/issues.exp | 2 +- 5 files changed, 117 insertions(+), 48 deletions(-) create mode 100644 infer/tests/codetoanalyze/java/threadsafety/Annotations.java diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml index 22761e660..d94ca940d 100644 --- a/infer/src/checkers/ThreadSafety.ml +++ b/infer/src/checkers/ThreadSafety.ml @@ -23,38 +23,6 @@ module Summary = Summary.Make (struct payload.Specs.threadsafety end) -(* we want to consider Builder classes and other safe immutablility-ensuring patterns as - thread-safe. we are overly friendly about this for now; any class whose name ends with `Builder` - is assumed to be thread-safe. in the future, we can ask for builder classes to be annotated with - @Builder and verify that annotated classes satisfy the expected invariants. *) -let is_builder_class class_name = - String.is_suffix ~suffix:"Builder" class_name - -(* similarly, we assume that immutable classes safely encapsulate their state *) -let is_immutable_collection_class class_name tenv = - let immutable_collections = [ - "com.google.common.collect.ImmutableCollection"; - "com.google.common.collect.ImmutableMap"; - "com.google.common.collect.ImmutableTable"; - ] in - PatternMatch.supertype_exists - tenv (fun typename _ -> IList.mem (=) (Typename.name typename) immutable_collections) class_name - -let is_call_to_builder_class_method = function - | Procname.Java java_pname -> is_builder_class (Procname.java_get_class_name java_pname) - | _ -> false - -let is_call_to_immutable_collection_method tenv = function - | Procname.Java java_pname -> - is_immutable_collection_class (Procname.java_get_class_type_name java_pname) tenv - | _ -> - false - -let is_initializer tenv proc_name = - Procname.is_constructor proc_name || - Procname.is_class_initializer proc_name || - FbThreadSafety.is_custom_init tenv proc_name - module TransferFunctions (CFG : ProcCfg.S) = struct module CFG = CFG module Domain = ThreadSafetyDomain @@ -123,7 +91,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct let exec_instr ({ ThreadSafetyDomain.locks; reads; writes; id_map; owned; } as astate) - { ProcData.pdesc; tenv; } _ = + { ProcData.pdesc; } _ = let is_allocation pn = Procname.equal pn BuiltinDecl.__new || @@ -157,11 +125,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct let astate' = (* TODO (14842325): report on constructors that aren't threadsafe (e.g., constructors that access static fields) *) - if is_unprotected locks' && - not (is_initializer tenv pn) && - not (FbThreadSafety.is_logging_method pn) && - not (is_call_to_builder_class_method pn) && - not (is_call_to_immutable_collection_method tenv pn) + if is_unprotected locks' then let call_site = CallSite.make pn loc in let reads' = @@ -249,13 +213,69 @@ module ResultsTableType = Caml.Map.Make (struct let compare (_, _, pn1, _) (_,_,pn2,_) = Procname.compare pn1 pn2 end) -let should_report_on_proc (_,tenv,proc_name,proc_desc) = +(* we want to consider Builder classes and other safe immutablility-ensuring patterns as + thread-safe. we are overly friendly about this for now; any class whose name ends with `Builder` + is assumed to be thread-safe. in the future, we can ask for builder classes to be annotated with + @Builder and verify that annotated classes satisfy the expected invariants. *) +let is_builder_class class_name = + String.is_suffix ~suffix:"Builder" class_name + +(* similarly, we assume that immutable classes safely encapsulate their state *) +let is_immutable_collection_class class_name tenv = + let immutable_collections = [ + "com.google.common.collect.ImmutableCollection"; + "com.google.common.collect.ImmutableMap"; + "com.google.common.collect.ImmutableTable"; + ] in + PatternMatch.supertype_exists + tenv (fun typename _ -> IList.mem (=) (Typename.name typename) immutable_collections) class_name + +let is_call_to_builder_class_method = function + | Procname.Java java_pname -> is_builder_class (Procname.java_get_class_name java_pname) + | _ -> false + +let is_call_to_immutable_collection_method tenv = function + | Procname.Java java_pname -> + is_immutable_collection_class (Procname.java_get_class_type_name java_pname) tenv + | _ -> + false + +let is_initializer tenv proc_name = + Procname.is_constructor proc_name || + Procname.is_class_initializer proc_name || + FbThreadSafety.is_custom_init tenv proc_name + +(* we don't want to warn on methods that run on the UI thread because they should always be + single-threaded *) +let runs_on_ui_thread proc_desc = + (* assume that methods named onClick always run on the UI thread *) + let is_on_click pdesc = match Procdesc.get_proc_name pdesc with + | Procname.Java java_pname -> Procname.java_get_method java_pname = "onClick" + | _ -> false in + let is_annotated pdesc = + let annotated_signature = Annotations.get_annotated_signature (Procdesc.get_attributes pdesc) in + let ret_annotation, _ = annotated_signature.Annotations.ret in + Annotations.ia_is_ui_thread ret_annotation in + is_on_click proc_desc || is_annotated proc_desc + +(* return true if we should compute a summary for the procedure. if this returns false, we won't + analyze the procedure or report any warnings on it *) +(* note: in the future, we will want to analyze the procedures in all of these cases in order to + find more bugs. this is just a temporary measure to avoid obvious false positives *) +let should_analyze_proc pdesc tenv = + let pn = Procdesc.get_proc_name pdesc in + not (is_initializer tenv pn) && + not (FbThreadSafety.is_logging_method pn) && + not (is_call_to_builder_class_method pn) && + not (is_call_to_immutable_collection_method tenv pn) && + not (runs_on_ui_thread pdesc) + +(* return true if we should report on unprotected accesses during the procedure *) +let should_report_on_proc (_, tenv, proc_name, proc_desc) = not (is_initializer tenv proc_name) && not (Procname.java_is_autogen_method proc_name) && Procdesc.get_access proc_desc <> PredSymb.Private -let dummy_cg = Cg.create None - (* creates a map from proc_envs to postconditions *) let make_results_table get_proc_desc file_env = (* make a Map sending each element e of list l to (f e) *) @@ -265,12 +285,19 @@ let make_results_table get_proc_desc file_env = in let compute_post_for_procedure = (* takes proc_env as arg *) fun (idenv, tenv, proc_name, proc_desc) -> + let empty = false, ThreadSafetyDomain.PathDomain.empty, ThreadSafetyDomain.PathDomain.empty in (* convert the abstract state to a summary by dropping the id map *) let compute_post ({ ProcData.pdesc; tenv; } as proc_data) = - if not (Procdesc.did_preanalysis pdesc) then Preanal.do_liveness pdesc tenv; - match Analyzer.compute_post proc_data ~initial:ThreadSafetyDomain.empty with - | Some { locks; reads; writes; } -> Some (locks, reads, writes) - | None -> None in + if should_analyze_proc pdesc tenv + then + begin + if not (Procdesc.did_preanalysis pdesc) then Preanal.do_liveness pdesc tenv; + match Analyzer.compute_post proc_data ~initial:ThreadSafetyDomain.empty with + | Some { locks; reads; writes; } -> Some (locks, reads, writes) + | None -> None + end + else + Some empty in let callback_arg = {Callbacks.get_proc_desc; get_procs_in_file = (fun _ -> []); idenv; tenv; proc_name; proc_desc} in @@ -280,8 +307,7 @@ let make_results_table get_proc_desc file_env = ~make_extras:ProcData.make_empty_extras callback_arg with | Some post -> post - | None -> - false, ThreadSafetyDomain.PathDomain.empty, ThreadSafetyDomain.PathDomain.empty + | None -> empty in map_post_computation_over_procs compute_post_for_procedure file_env diff --git a/infer/src/checkers/annotations.ml b/infer/src/checkers/annotations.ml index ba6a3b45d..7eef7eaad 100644 --- a/infer/src/checkers/annotations.ml +++ b/infer/src/checkers/annotations.ml @@ -115,6 +115,7 @@ let integrity_sink = "IntegritySink" let guarded_by = "GuardedBy" let thread_safe = "ThreadSafe" let not_thread_safe = "NotThreadSafe" +let ui_thread = "UiThread" let ia_is_not_thread_safe ia = ia_ends_with ia not_thread_safe @@ -214,6 +215,9 @@ let ia_is_integrity_sink ia = let ia_is_guarded_by ia = ia_ends_with ia guarded_by +let ia_is_ui_thread ia = + ia_ends_with ia ui_thread + type annotation = | Nullable | Present diff --git a/infer/src/checkers/annotations.mli b/infer/src/checkers/annotations.mli index 864c793b0..fe43be7d2 100644 --- a/infer/src/checkers/annotations.mli +++ b/infer/src/checkers/annotations.mli @@ -96,6 +96,7 @@ 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_is_ui_thread : Annot.Item.t -> bool val ia_iter : (Annot.t -> unit) -> Annot.Item.t -> unit diff --git a/infer/tests/codetoanalyze/java/threadsafety/Annotations.java b/infer/tests/codetoanalyze/java/threadsafety/Annotations.java new file mode 100644 index 000000000..4beccf7b7 --- /dev/null +++ b/infer/tests/codetoanalyze/java/threadsafety/Annotations.java @@ -0,0 +1,38 @@ +/* + * Copyright (c) 2017 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +package codetoanalyze.java.checkers; + +import android.support.annotation.UiThread; + +/** tests for classes and method annotations that are meaningful w.r.t thread-safety */ + +@ThreadSafe +class Annotations { + Object f; + + @UiThread + public void setF(Object newF) { + this.f = newF; // shouldn't report here + } + + public void callSetFOnMethodOk(Annotations obj) { + obj.setF(new Object()); // or here + } + + public void mutateOffUiThreadBad() { + this.f = new Object(); + } + + // we model onClick as running on the UI thread, should not warn + public void onClick() { + this.f = new Object(); + } + +} diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp index 1e74a5d27..9bdfb468a 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp +++ b/infer/tests/codetoanalyze/java/threadsafety/issues.exp @@ -1,6 +1,6 @@ +codetoanalyze/java/threadsafety/Annotations.java, void Annotations.mutateOffUiThreadBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Annotations.f] 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/Builders.java, void TopLevelBuilder.setG(String), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.TopLevelBuilder.g] codetoanalyze/java/threadsafety/Locks.java, void Locks.FP_unlockOneLock(), 4, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Locks.f] codetoanalyze/java/threadsafety/Locks.java, void Locks.afterReentrantLockUnlockBad(), 3, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Locks.f] codetoanalyze/java/threadsafety/Locks.java, void Locks.afterUnlockBad(), 3, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Locks.f]