From 7b8eef52f84b5c797e55e4a5d69af0590c61d877 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Thu, 8 Jun 2017 10:58:34 -0700 Subject: [PATCH] [thread-safety] fix bad read/write race traces Summary: Read/write race errors should always show one trace for a read and one trace for a write. We forget to pass the conflicting writes to the reporting function in one case, which prevented us from showing a well-formed trace. Fixed it by making the `conflicts` parameter non-optional Reviewed By: jberdine Differential Revision: D5209332 fbshipit-source-id: 05da01a --- infer/src/checkers/ThreadSafety.ml | 4 +++- infer/tests/codetoanalyze/cpp/threadsafety/issues.exp | 2 +- infer/tests/codetoanalyze/java/threadsafety/issues.exp | 6 +++--- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml index a2fb55f9c..77cd3b7db 100644 --- a/infer/src/checkers/ThreadSafety.ml +++ b/infer/src/checkers/ThreadSafety.ml @@ -1027,7 +1027,7 @@ let make_trace_with_conflicts conflicts original_path pdesc = | [] -> original_trace -let report_thread_safety_violation tenv pdesc ~make_description ?(conflicts=[]) access = +let report_thread_safety_violation tenv pdesc ~make_description ~conflicts access = let open ThreadSafetyDomain in let pname = Procdesc.get_proc_name pdesc in let report_one_path ((_, sinks) as path) = @@ -1157,6 +1157,7 @@ let report_unsafe_accesses ~is_file_threadsafe aggregated_access_map = tenv pdesc ~make_description:make_unprotected_write_description + ~conflicts:[] access; update_reported access pname reported_acc end @@ -1220,6 +1221,7 @@ let report_unsafe_accesses ~is_file_threadsafe aggregated_access_map = tenv pdesc ~make_description:(make_read_write_race_description conflicting_writes) + ~conflicts:(List.map ~f:(fun (access, _, _, _, _) -> access) conflicting_writes) access; update_reported access pname reported_acc end in diff --git a/infer/tests/codetoanalyze/cpp/threadsafety/issues.exp b/infer/tests/codetoanalyze/cpp/threadsafety/issues.exp index 3fb48c686..38ecc4e8a 100644 --- a/infer/tests/codetoanalyze/cpp/threadsafety/issues.exp +++ b/infer/tests/codetoanalyze/cpp/threadsafety/issues.exp @@ -1,4 +1,4 @@ -codetoanalyze/cpp/threadsafety/basics.cpp, basics::Basic_get2, 3, THREAD_SAFETY_VIOLATION, [access to `suspiciously_written`] +codetoanalyze/cpp/threadsafety/basics.cpp, basics::Basic_get2, 3, THREAD_SAFETY_VIOLATION, [,access to `suspiciously_written`,,access to `suspiciously_written`] codetoanalyze/cpp/threadsafety/basics.cpp, basics::Basic_get3, 0, THREAD_SAFETY_VIOLATION, [,access to `not_guarded`,,access to `not_guarded`] codetoanalyze/cpp/threadsafety/basics.cpp, basics::Basic_get4, 0, THREAD_SAFETY_VIOLATION, [,access to `suspiciously_read`,,access to `suspiciously_read`] codetoanalyze/cpp/threadsafety/basics_with_mutex.cpp, basics::BasicsWithHeader_get1, 0, THREAD_SAFETY_VIOLATION, [,access to `field_1`,,access to `field_1`] diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp index 13d64ca2e..ec8572dc0 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp +++ b/infer/tests/codetoanalyze/java/threadsafety/issues.exp @@ -72,7 +72,7 @@ codetoanalyze/java/threadsafety/RaceWithMainThread.java, void RaceWithMainThread codetoanalyze/java/threadsafety/RaceWithMainThread.java, void RaceWithMainThread.conditional_isMainThread_ElseBranch_Bad(), 7, THREAD_SAFETY_VIOLATION, [access to `codetoanalyze.java.checkers.RaceWithMainThread.ff`] codetoanalyze/java/threadsafety/RaceWithMainThread.java, void RaceWithMainThread.conditional_isMainThread_Negation_Bad(), 3, THREAD_SAFETY_VIOLATION, [access to `codetoanalyze.java.checkers.RaceWithMainThread.ff`] codetoanalyze/java/threadsafety/RaceWithMainThread.java, void RaceWithMainThread.conditional_isUiThread_ElseBranch_Bad(), 7, THREAD_SAFETY_VIOLATION, [access to `codetoanalyze.java.checkers.RaceWithMainThread.ff`] -codetoanalyze/java/threadsafety/RaceWithMainThread.java, void RaceWithMainThread.readProtectedUnthreadedBad(), 3, THREAD_SAFETY_VIOLATION, [access to `codetoanalyze.java.checkers.RaceWithMainThread.f`] +codetoanalyze/java/threadsafety/RaceWithMainThread.java, void RaceWithMainThread.readProtectedUnthreadedBad(), 3, THREAD_SAFETY_VIOLATION, [,access to `codetoanalyze.java.checkers.RaceWithMainThread.f`,,access to `codetoanalyze.java.checkers.RaceWithMainThread.f`] codetoanalyze/java/threadsafety/RaceWithMainThread.java, void RaceWithMainThread.read_unprotected_unthreaded1_Bad(), 2, THREAD_SAFETY_VIOLATION, [,access to `codetoanalyze.java.checkers.RaceWithMainThread.f1`,,access to `codetoanalyze.java.checkers.RaceWithMainThread.f1`] codetoanalyze/java/threadsafety/RaceWithMainThread.java, void RaceWithMainThread.read_unprotected_unthreaded_Bad(), 2, THREAD_SAFETY_VIOLATION, [,access to `codetoanalyze.java.checkers.RaceWithMainThread.f`,,access to `codetoanalyze.java.checkers.RaceWithMainThread.f`] codetoanalyze/java/threadsafety/ReadWriteRaces.java, Object ReadWriteRaces.callUnprotecteReadInCallee(), 1, THREAD_SAFETY_VIOLATION, [,call to Object ReadWriteRaces.unprotectedReadInCallee(),access to `codetoanalyze.java.checkers.ReadWriteRaces.field1`,,access to `codetoanalyze.java.checkers.ReadWriteRaces.field1`] @@ -97,8 +97,8 @@ codetoanalyze/java/threadsafety/ThreadSafeExample.java, void YesThreadSafeExtend codetoanalyze/java/threadsafety/ThreadSafeMethods.java, Object ThreadSafeMethods.readSameFieldAsThreadSafeMethod1Bad(), 1, THREAD_SAFETY_VIOLATION, [,access to `codetoanalyze.java.checkers.ThreadSafeMethods.field1`,,access to `codetoanalyze.java.checkers.ThreadSafeMethods.field1`] codetoanalyze/java/threadsafety/ThreadSafeMethods.java, Object ThreadSafeMethods.readSameFieldAsThreadSafeMethod2Bad(), 1, THREAD_SAFETY_VIOLATION, [,access to `codetoanalyze.java.checkers.ThreadSafeMethods.field4`,,access to `codetoanalyze.java.checkers.ThreadSafeMethods.field4`] codetoanalyze/java/threadsafety/ThreadSafeMethods.java, Object ThreadSafeMethods.readSameFieldAsThreadSafeMethod3Bad(), 1, THREAD_SAFETY_VIOLATION, [,access to `codetoanalyze.java.checkers.ThreadSafeMethods.field5`,,access to `codetoanalyze.java.checkers.ThreadSafeMethods.field5`] -codetoanalyze/java/threadsafety/ThreadSafeMethods.java, Object ThreadSafeMethods.readSameFieldAsThreadSafeMethodWhileSynchronized1Bad(), 1, THREAD_SAFETY_VIOLATION, [access to `codetoanalyze.java.checkers.ThreadSafeMethods.field1`] -codetoanalyze/java/threadsafety/ThreadSafeMethods.java, Object ThreadSafeMethods.synchronizedReadBad(), 1, THREAD_SAFETY_VIOLATION, [access to `codetoanalyze.java.checkers.ThreadSafeMethods.field5`] +codetoanalyze/java/threadsafety/ThreadSafeMethods.java, Object ThreadSafeMethods.readSameFieldAsThreadSafeMethodWhileSynchronized1Bad(), 1, THREAD_SAFETY_VIOLATION, [,access to `codetoanalyze.java.checkers.ThreadSafeMethods.field1`,,access to `codetoanalyze.java.checkers.ThreadSafeMethods.field1`] +codetoanalyze/java/threadsafety/ThreadSafeMethods.java, Object ThreadSafeMethods.synchronizedReadBad(), 1, THREAD_SAFETY_VIOLATION, [,access to `codetoanalyze.java.checkers.ThreadSafeMethods.field5`,,access to `codetoanalyze.java.checkers.ThreadSafeMethods.field5`] codetoanalyze/java/threadsafety/ThreadSafeMethods.java, Object ThreadSafeMethods.threadSafeMethodReadBad(), 1, THREAD_SAFETY_VIOLATION, [,access to `codetoanalyze.java.checkers.ThreadSafeMethods.field2`,,access to `codetoanalyze.java.checkers.ThreadSafeMethods.field2`] codetoanalyze/java/threadsafety/ThreadSafeMethods.java, Object ThreadSafeMethodsSubclass.readThreadSafeFieldOfOverrideBad(), 1, THREAD_SAFETY_VIOLATION, [,access to `codetoanalyze.java.checkers.ThreadSafeMethodsSubclass.subclassField`,,access to `codetoanalyze.java.checkers.ThreadSafeMethodsSubclass.subclassField`] codetoanalyze/java/threadsafety/ThreadSafeMethods.java, void ThreadSafeMethods.threadSafeMethodWriteBad(), 1, THREAD_SAFETY_VIOLATION, [access to `codetoanalyze.java.checkers.ThreadSafeMethods.field1`]