From 5130952ee8ef2dbfa617f97df6ae858e8d9f7e93 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Mon, 27 Mar 2017 10:58:49 -0700 Subject: [PATCH] [thread-safety] move reporting of unprotected writes into aggregation function Summary: A step toward simplifying aggregation/reporting. Reviewed By: peterogithub Differential Revision: D4772207 fbshipit-source-id: 3dab2ea --- infer/src/checkers/ThreadSafety.ml | 45 ++++++++++--------- .../java/threadsafety/issues.exp | 2 + 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml index cfedeba96..7cc0cf66a 100644 --- a/infer/src/checkers/ThreadSafety.ml +++ b/infer/src/checkers/ThreadSafety.ml @@ -817,7 +817,7 @@ let is_thread_safe_method pdesc tenv = (Procdesc.get_proc_name pdesc) (* return true if we should report on unprotected accesses during the procedure *) -let should_report_on_proc (tenv, proc_desc) = +let should_report_on_proc proc_desc tenv = let proc_name = Procdesc.get_proc_name proc_desc in is_thread_safe_method proc_desc tenv || (not (Typ.Procname.java_is_autogen_method proc_name) && @@ -1023,8 +1023,8 @@ let collect_conflicts sink (*kind*) threaded tab = (*kind implicitly Read for no ) (ResultsTableType.bindings tab) in List.filter - ~f:(fun (proc_env, _, writes) -> - (should_report_on_proc proc_env) + ~f:(fun ((tenv, pdesc), _, writes) -> + (should_report_on_proc pdesc tenv) && not (ThreadSafetyDomain.PathDomain.Sinks.is_empty (ThreadSafetyDomain.PathDomain.sinks writes)) ) @@ -1160,19 +1160,32 @@ let make_read_write_race_description (calculate_addendum_message tenv pname) (* report accesses that may race with each other *) -let report_unsafe_accesses tab aggregated_access_map = +let report_unsafe_accesses tab ~should_report aggregated_access_map = let report_unsafe_access (access, pre, threaded, tenv, pdesc) accesses reported_sites = let open ThreadSafetyDomain in let call_site = TraceElem.call_site access in - if CallSite.Set.mem call_site reported_sites + if CallSite.Set.mem call_site reported_sites || not (should_report pdesc tenv) then (* already reported a warning originating at this call site; don't double-report *) reported_sites else match snd (TraceElem.kind access), pre with | Access.Write, AccessPrecondition.Unprotected _ -> - (* unprotected write. TODO: warn. *) - reported_sites + if threaded + then reported_sites + else + begin + (* unprotected write. warn. *) + report_thread_safety_violations + tenv + pdesc + ~get_unsafe_accesses:get_possibly_unsafe_writes + make_unprotected_write_description + (PathDomain.of_sink access) + threaded + tab; + CallSite.Set.add call_site reported_sites + end | Access.Write, AccessPrecondition.Protected -> (* protected write, do nothing *) reported_sites @@ -1309,16 +1322,16 @@ let should_report_on_file file_env = *) let process_results_table file_env tab = let should_report_on_all_procs = should_report_on_file file_env in - let should_report ((tenv, pdesc) as proc_env) = - (should_report_on_all_procs && should_report_on_proc proc_env) || + let should_report pdesc tenv = + (should_report_on_all_procs && should_report_on_proc pdesc tenv) || is_thread_safe_method pdesc tenv in aggregate_accesses tab - |> report_unsafe_accesses tab; + |> report_unsafe_accesses tab ~should_report; ResultsTableType.iter (* report errors for each method *) (fun ((tenv, pdesc) as proc_env) (threaded, _, accesses, _) -> - if should_report proc_env + if should_report pdesc tenv then let open ThreadSafetyDomain in let reads, writes = @@ -1336,16 +1349,6 @@ let process_results_table file_env tab = let stripped_unsafe_reads = strip_reads_that_have_co_located_write unsafe_reads unsafe_writes in - if not threaded - then (*don't report writes for threaded; TODO to extend this*) - report_thread_safety_violations - tenv - pdesc - ~get_unsafe_accesses:get_possibly_unsafe_writes - make_unprotected_write_description - unsafe_writes - threaded - tab; report_reads proc_env stripped_unsafe_reads threaded tab end ) diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp index 295b5ecd2..2884f09d2 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp +++ b/infer/tests/codetoanalyze/java/threadsafety/issues.exp @@ -33,9 +33,11 @@ codetoanalyze/java/threadsafety/Containers.java, void Containers.mapSubclassWrit codetoanalyze/java/threadsafety/DeDup.java, void DeDup.colocated_read_write(), 1, THREAD_SAFETY_VIOLATION, [call to void DeDup.read_and_write(),access to codetoanalyze.java.checkers.DeDup.colocated_write] codetoanalyze/java/threadsafety/DeDup.java, void DeDup.separate_write_to_colocated_read(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.DeDup.colocated_read] codetoanalyze/java/threadsafety/DeDup.java, void DeDup.twoWritesOneInCaller(), 1, THREAD_SAFETY_VIOLATION, [call to void DeDup.writeToField(),access to codetoanalyze.java.checkers.DeDup.field] +codetoanalyze/java/threadsafety/DeDup.java, void DeDup.twoWritesOneInCaller(), 2, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.DeDup.field] codetoanalyze/java/threadsafety/DeDup.java, void DeDup.two_fields(), 1, THREAD_SAFETY_VIOLATION, [call to void DeDup.foo(),access to codetoanalyze.java.checkers.DeDup.fielda] codetoanalyze/java/threadsafety/DeDup.java, void DeDup.two_reads(), 2, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.DeDup.field] codetoanalyze/java/threadsafety/DeDup.java, void DeDup.two_writes(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.DeDup.field] +codetoanalyze/java/threadsafety/DeDup.java, void DeDup.two_writes(), 2, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.DeDup.field] codetoanalyze/java/threadsafety/DeDup.java, void DeDup.write_read(), 2, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.DeDup.field] codetoanalyze/java/threadsafety/DeDup.java, void DeDup.write_read(), 3, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.DeDup.field] codetoanalyze/java/threadsafety/Locks.java, void Locks.FP_unlockOneLock(), 4, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Locks.f]