From c52054d3d1e2e39292939f252e3be32265e2f67f Mon Sep 17 00:00:00 2001 From: Peter O'Hearn Date: Wed, 22 Mar 2017 09:56:38 -0700 Subject: [PATCH] [threadsafety] More races with main thread Summary: If I read off the main thread and write on the main we could have a race. (Writes off main are already reported.) Reviewed By: sblackshear Differential Revision: D4746138 fbshipit-source-id: 8b6e9c5 --- infer/src/checkers/ThreadSafety.ml | 36 +++++++++++-------- .../java/threadsafety/Annotations.java | 12 +++++++ .../java/threadsafety/issues.exp | 2 ++ 3 files changed, 36 insertions(+), 14 deletions(-) diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml index e3e580e22..fdf5b60c1 100644 --- a/infer/src/checkers/ThreadSafety.ml +++ b/infer/src/checkers/ThreadSafety.ml @@ -770,8 +770,6 @@ let should_analyze_proc pdesc tenv = 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) && - not (is_thread_confined_method tenv pdesc) && not (pdesc_is_assumed_thread_safe pdesc tenv) let is_thread_safe_method pdesc tenv = @@ -796,9 +794,9 @@ let analyze_procedure callback = Typ.Procname.is_constructor proc_name || FbThreadSafety.is_custom_init tenv proc_name in let open ThreadSafetyDomain in let has_lock = false in - let known_on_ui_thread = false in + let initial_known_on_ui_thread = false in let return_attrs = AttributeSetDomain.empty in - let empty = known_on_ui_thread, has_lock, AccessDomain.empty, return_attrs in + let empty = initial_known_on_ui_thread, has_lock, AccessDomain.empty, return_attrs in (* convert the abstract state to a summary by dropping the id map *) let compute_post ({ ProcData.pdesc; tenv; extras; } as proc_data) = if should_analyze_proc pdesc tenv @@ -806,6 +804,7 @@ let analyze_procedure callback = begin if not (Procdesc.did_preanalysis pdesc) then Preanal.do_liveness pdesc tenv; let initial = + let threads = runs_on_ui_thread pdesc || is_thread_confined_method tenv pdesc in if is_initializer tenv (Procdesc.get_proc_name pdesc) then let add_owned_formal acc formal_index = @@ -826,9 +825,9 @@ let analyze_procedure callback = ~f:add_owned_formal owned_formals ~init:ThreadSafetyDomain.empty.attribute_map in - { ThreadSafetyDomain.empty with attribute_map; } + { ThreadSafetyDomain.empty with attribute_map; threads; } else - ThreadSafetyDomain.empty in + { ThreadSafetyDomain.empty with threads; } in match Analyzer.compute_post proc_data ~initial with | Some { threads; locks; accesses; attribute_map; } -> @@ -971,23 +970,23 @@ let filter_conflicting_sinks sink trace = *) let collect_conflicts sink (*kind*) threaded tab = (*kind implicitly Read for now*) - let procs_and_accesses = + let procs_and_threaded_and_accesses = List.map ~f:(fun (key, (other_access_threaded, _, accesses, _)) -> let conflicting_writes = if threaded && other_access_threaded then ThreadSafetyDomain.PathDomain.empty else filter_conflicting_sinks sink (get_all_accesses Write accesses) in - key, conflicting_writes + key, other_access_threaded, conflicting_writes ) (ResultsTableType.bindings tab) in List.filter - ~f:(fun (proc_env,writes) -> + ~f:(fun (proc_env,_,writes) -> (should_report_on_proc proc_env) && not (ThreadSafetyDomain.PathDomain.Sinks.is_empty (ThreadSafetyDomain.PathDomain.sinks writes)) ) - procs_and_accesses + procs_and_threaded_and_accesses @@ -1089,9 +1088,13 @@ let make_unprotected_write_description let make_read_write_race_description tenv pname final_sink_site initial_sink_site final_sink threaded tab = + let conflicts = collect_conflicts final_sink threaded tab in + let race_with_main_thread = List.exists + ~f:(fun (_,threaded,_) -> threaded) + conflicts in let conflicting_proc_envs = List.map - ~f:fst - (collect_conflicts final_sink threaded tab) in + ~f:(fun (proc_env,_,_) -> proc_env) + conflicts in let conflicting_proc_names = List.map ~f:(fun (_,_,proc_name,_) -> proc_name) conflicting_proc_envs in @@ -1099,9 +1102,14 @@ let make_read_write_race_description let pp_sep _ _ = F.fprintf fmt " , " in F.pp_print_list ~pp_sep Typ.Procname.pp fmt proc_names in let conflicts_description = - Format.asprintf "Potentially races with writes in method%s %a." + Format.asprintf "Potentially races with writes in method%s %a. %s" (if List.length conflicting_proc_names > 1 then "s" else "") - (MF.wrap_monospaced pp_proc_name_list) conflicting_proc_names in + (MF.wrap_monospaced pp_proc_name_list) conflicting_proc_names + (if race_with_main_thread then + "\n Note: some of these write conflicts are confined to the UI or another thread, \ + but the current method is not specified to be. Consider adding synchronization \ + or a @ThreadConfined annotation to the current method." + else "") in Format.asprintf "Read/Write race. Public method %a%s reads from field %a. %s %s" (MF.wrap_monospaced Typ.Procname.pp) pname (if CallSite.equal final_sink_site initial_sink_site then "" else " indirectly") diff --git a/infer/tests/codetoanalyze/java/threadsafety/Annotations.java b/infer/tests/codetoanalyze/java/threadsafety/Annotations.java index 9a4d26553..5078fdb88 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/Annotations.java +++ b/infer/tests/codetoanalyze/java/threadsafety/Annotations.java @@ -146,9 +146,17 @@ class Annotations implements Interface { this.encapsulatedField.f = new Object(); } + Integer zz; + @ThreadConfined("some_custom_string") public void threadConfinedMethodOk() { this.f = new Object(); + zz = 22; + } + + public void read_from_non_confined_method_Bad(){ + Integer i; + i = zz; } @OnBind @@ -156,6 +164,10 @@ class Annotations implements Interface { this.f = new Object(); } + public void read_off_UI_thread_Bad(){ + Object o = f; + } + @OnMount public void onMountMethodOk() { this.f = new Object(); diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp index e34982124..9b765bcb5 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp +++ b/infer/tests/codetoanalyze/java/threadsafety/issues.exp @@ -7,6 +7,8 @@ codetoanalyze/java/threadsafety/Annotations.java, long Annotations.functionaLong codetoanalyze/java/threadsafety/Annotations.java, void Annotations.functionalAndNonfunctionalBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Annotations.mInt] 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.Obj.f] +codetoanalyze/java/threadsafety/Annotations.java, void Annotations.read_from_non_confined_method_Bad(), 2, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Annotations.zz] +codetoanalyze/java/threadsafety/Annotations.java, void Annotations.read_off_UI_thread_Bad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Annotations.f] codetoanalyze/java/threadsafety/Annotations.java, void ThreadSafeAlias.threadSafeAliasBad1(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeAlias.field] codetoanalyze/java/threadsafety/Annotations.java, void ThreadSafeAlias.threadSafeAliasBad2(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeAlias.field] codetoanalyze/java/threadsafety/Builders.java, Builders$Obj Builders.buildThenMutateBad(Builders$Obj), 3, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Builders$Obj.g]