diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml index f8327ce7e..a84a88986 100644 --- a/infer/src/checkers/ThreadSafety.ml +++ b/infer/src/checkers/ThreadSafety.ml @@ -772,12 +772,20 @@ let combine_conditional_unconditional_writes conditional_writes unconditional_wr conditional_writes unconditional_writes -let conflicting_accesses (sink1 : ThreadSafetyDomain.TraceElem.t) + +let equal_accesses (sink1 : ThreadSafetyDomain.TraceElem.t) (sink2 : ThreadSafetyDomain.TraceElem.t) = AccessPath.equal_access_list (snd (ThreadSafetyDomain.TraceElem.kind sink1)) (snd (ThreadSafetyDomain.TraceElem.kind sink2)) +(* For now equal-access and conflicting-access are equivalent. + But that will change when we (soon) consider conficting accesses + that are not via assignment, such as add and get for containers*) +let conflicting_accesses (sink1 : ThreadSafetyDomain.TraceElem.t) + (sink2 : ThreadSafetyDomain.TraceElem.t) = + equal_accesses sink1 sink2 + (* trace is really reads or writes set. Fix terminology later *) let filter_conflicting_sinks sink trace = let conflicts = @@ -792,14 +800,14 @@ let filter_conflicting_sinks sink trace = write accesses*) let collect_conflicting_writes sink tab = let procs_and_writes = - IList.map - (fun (key,(_, _, conditional_writes, unconditional_writes, _)) -> - let conflicting_writes = - combine_conditional_unconditional_writes - conditional_writes unconditional_writes - |> filter_conflicting_sinks sink in - key, conflicting_writes - ) + List.map + ~f:(fun (key,(_, _, conditional_writes, unconditional_writes, _)) -> + let conflicting_writes = + combine_conditional_unconditional_writes + conditional_writes unconditional_writes + |> filter_conflicting_sinks sink in + key, conflicting_writes + ) (ResultsTableType.bindings tab) in List.filter ~f:(fun (proc_env,writes) -> @@ -809,7 +817,28 @@ let collect_conflicting_writes sink tab = ) procs_and_writes -(*A helper function used int he error reporting*) +(* keep only the first copy of an access per procedure *) +let de_dup trace = + let original_sinks = ThreadSafetyDomain.PathDomain.sinks trace in + let list_of_original_sinks = ThreadSafetyDomain.PathDomain.Sinks.elements original_sinks in + let de_duped_sinks = + ThreadSafetyDomain.PathDomain.Sinks.filter + (fun sink -> + (* for each sink we will keep one in the equivalence class of those + with same access path. We select that by using find_exn to get + the first element equivalent ot sink in a list of sinks. This + first element is the dedup representative, and it happens to + typically be the first such access in a method. *) + let first_sink = + List.find_exn + ~f:(fun sink2 -> equal_accesses sink sink2) + list_of_original_sinks in + Int.equal (ThreadSafetyDomain.TraceElem.compare sink first_sink) 0 + ) + original_sinks in + ThreadSafetyDomain.PathDomain.update_sinks trace de_duped_sinks + +(*A helper function used in the error reporting*) let pp_accesses_sink fmt sink = let _, accesses = ThreadSafetyDomain.PathDomain.Sink.kind sink in AccessPath.pp_access_list fmt accesses @@ -846,7 +875,7 @@ let report_thread_safety_violations ( _, tenv, pname, pdesc) make_description tr IList.iter report_one_path - (PathDomain.get_reportable_sink_paths trace ~trace_of_pname) + (PathDomain.get_reportable_sink_paths (de_dup trace) ~trace_of_pname) let make_unprotected_write_description @@ -879,7 +908,6 @@ let make_read_write_race_description tenv pname final_sink_site initial_sink_sit conflicts_description (calculate_addendum_message tenv pname) - (* find those elements of reads which have conflicts somewhere else, and report them *) let report_reads proc_env reads tab = diff --git a/infer/tests/codetoanalyze/java/threadsafety/DeDup.java b/infer/tests/codetoanalyze/java/threadsafety/DeDup.java new file mode 100644 index 000000000..ce0a97a33 --- /dev/null +++ b/infer/tests/codetoanalyze/java/threadsafety/DeDup.java @@ -0,0 +1,50 @@ +/* + * Copyright (c) 2016 - 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 javax.annotation.concurrent.ThreadSafe; + +@ThreadSafe +class DeDup{ + +Integer field; + + /*Only the first write should be reported*/ + void two_writes(){ + field = 22; + field = 84; + } + + /*Only the first read should be reported*/ + void two_reads(){ //parallel reads are OK + Integer local; + local = field; + local = field+1; + } + + /*Both accesses should be reported*/ + void write_read(){ //parallel reads are OK + Integer local; + field = 87; + local = field; + } + + /*Should only report the first write, which happens to be interprocedural*/ + void twoWritesOneInCaller() { + writeToField(); + field = 22; + } + + /*Should not report directly as provate method*/ + private void writeToField() { + field = 33; + } + +} diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp index 76b60f883..4103e20f9 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp +++ b/infer/tests/codetoanalyze/java/threadsafety/issues.exp @@ -26,6 +26,11 @@ codetoanalyze/java/threadsafety/Containers.java, void Containers.mapPutAllBad(Ma codetoanalyze/java/threadsafety/Containers.java, void Containers.mapPutBad(String,String), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Containers.mMap.put] codetoanalyze/java/threadsafety/Containers.java, void Containers.mapRemoveBad(String), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Containers.mMap.remove] codetoanalyze/java/threadsafety/Containers.java, void Containers.mapSubclassWriteBad(HashMap,String), 1, THREAD_SAFETY_VIOLATION, [access to remove] +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.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.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] 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] @@ -53,7 +58,6 @@ codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.c codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.callVisibleForTestingBad(), 1, THREAD_SAFETY_VIOLATION, [call to void ThreadSafeExample.visibleForTestingNotPublicOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f] codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.deeperTraceBad(), 1, THREAD_SAFETY_VIOLATION, [call to void ThreadSafeExample.callAssignInPrivateMethod(),call to void ThreadSafeExample.assignInPrivateMethodOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f] codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.oddBad(), 1, THREAD_SAFETY_VIOLATION, [call to void ThreadSafeExample.evenOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f] -codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.recursiveBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeExample.f] codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.recursiveBad(), 2, THREAD_SAFETY_VIOLATION, [call to void ThreadSafeExample.recursiveBad(),access to codetoanalyze.java.checkers.ThreadSafeExample.f] codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.tsBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.ThreadSafeExample.f] codetoanalyze/java/threadsafety/ThreadSafeExample.java, void YesThreadSafeExtendsNotThreadSafeExample.subsubmethodBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.YesThreadSafeExtendsNotThreadSafeExample.subsubfield]