From 1580e5b3bb62f345290af25b9ceb57a2cda01c2a Mon Sep 17 00:00:00 2001 From: Peter O'Hearn Date: Fri, 24 Feb 2017 09:35:34 -0800 Subject: [PATCH] [infer][thread-safety] Better error message for container writes Summary: distinguish writes via method calls (e.g., add) from writes via assignment in the error messages Reviewed By: sblackshear Differential Revision: D4611748 fbshipit-source-id: 7594d3b --- infer/src/checkers/ThreadSafety.ml | 42 +++++++++++++++---- .../java/threadsafety/issues.exp | 28 ++++++------- 2 files changed, 47 insertions(+), 23 deletions(-) diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml index 309d263ab..3d771f5e8 100644 --- a/infer/src/checkers/ThreadSafety.ml +++ b/infer/src/checkers/ThreadSafety.ml @@ -27,6 +27,20 @@ let is_owned access_path attribute_map = ThreadSafetyDomain.AttributeMapDomain.has_attribute access_path ThreadSafetyDomain.Attribute.unconditionally_owned attribute_map +let container_write_string = "__CONTAINERWRITE__" + +let is_container_write_str str = + String.is_substring ~substring:container_write_string str + +let strip_container_write str = + String.substr_replace_first str ~pattern:container_write_string ~with_:"" + +let is_container_write_sink (sink:ThreadSafetyDomain.TraceElem.t) = + let access_list = snd (ThreadSafetyDomain.TraceElem.kind sink) in + match List.rev access_list with + | FieldAccess (fn) :: _ -> is_container_write_str (Ident.fieldname_to_string fn) + | _ -> false + module TransferFunctions (CFG : ProcCfg.S) = struct module CFG = CFG module Domain = ThreadSafetyDomain @@ -271,7 +285,9 @@ module TransferFunctions (CFG : ProcCfg.S) = struct (* create a dummy write that represents mutating the contents of the container *) let open Domain in let dummy_fieldname = - Ident.create_fieldname (Mangled.from_string (Procname.get_method callee_pname)) 0 in + Ident.create_fieldname + (Mangled.from_string + (container_write_string ^ (Procname.get_method callee_pname))) 0 in let dummy_access_exp = Exp.Lfield (receiver_exp, dummy_fieldname, receiver_typ) in let callee_conditional_writes = match AccessPath.of_lhs_exp dummy_access_exp receiver_typ ~f_resolve_id with @@ -839,9 +855,15 @@ let de_dup trace = 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 +let pp_accesses_sink fmt ~is_write_access sink = + let access_path = ThreadSafetyDomain.PathDomain.Sink.kind sink in + let container_write = is_write_access && is_container_write_sink sink in + F.fprintf fmt + (if container_write then "container %a" else "%a") + AccessPath.pp_access_list (if container_write then + snd (AccessPath.Raw.truncate access_path) + else snd access_path + ) (* trace is really a set of accesses*) @@ -859,9 +881,10 @@ let report_thread_safety_violations ( _, tenv, pname, pdesc) make_description tr let initial_sink_site = PathDomain.Sink.call_site initial_sink in let final_sink_site = PathDomain.Sink.call_site final_sink in let desc_of_sink sink = - if CallSite.equal (PathDomain.Sink.call_site sink) final_sink_site + if + CallSite.equal (PathDomain.Sink.call_site sink) final_sink_site then - Format.asprintf "access to %a" pp_accesses_sink sink + Format.asprintf "access to %a" (pp_accesses_sink ~is_write_access:true) sink else Format.asprintf "call to %a" Procname.pp (CallSite.pname (PathDomain.Sink.call_site sink)) in @@ -881,10 +904,11 @@ let report_thread_safety_violations ( _, tenv, pname, pdesc) make_description tr let make_unprotected_write_description tenv pname final_sink_site initial_sink_site final_sink _ = Format.asprintf - "Unprotected write. Public method %a%s writes to field %a outside of synchronization.%s" + "Unprotected write. Public method %a%s %s %a outside of synchronization.%s" Procname.pp pname (if CallSite.equal final_sink_site initial_sink_site then "" else " indirectly") - pp_accesses_sink final_sink + (if is_container_write_sink final_sink then "mutates" else "writes to field") + (pp_accesses_sink ~is_write_access:true) final_sink (calculate_addendum_message tenv pname) let make_read_write_race_description tenv pname final_sink_site initial_sink_site final_sink tab = @@ -904,7 +928,7 @@ let make_read_write_race_description tenv pname final_sink_site initial_sink_sit Format.asprintf "Read/Write race. Public method %a%s reads from field %a. %s %s" Procname.pp pname (if CallSite.equal final_sink_site initial_sink_site then "" else " indirectly") - pp_accesses_sink final_sink + (pp_accesses_sink ~is_write_access:false) final_sink conflicts_description (calculate_addendum_message tenv pname) diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp index 4103e20f9..b3cf58f4b 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp +++ b/infer/tests/codetoanalyze/java/threadsafety/issues.exp @@ -12,20 +12,20 @@ codetoanalyze/java/threadsafety/Builders.java, Builders$Obj Builders.mutateBad(B codetoanalyze/java/threadsafety/Constructors.java, Constructors Constructors.singletonBad(), 2, THREAD_SAFETY_VIOLATION, [call to Constructors.(Object),access to Constructors.staticField] codetoanalyze/java/threadsafety/Constructors.java, Constructors.(), 1, THREAD_SAFETY_VIOLATION, [access to Constructors.staticField] codetoanalyze/java/threadsafety/Constructors.java, Constructors.(Constructors), 1, THREAD_SAFETY_VIOLATION, [access to Constructors.field] -codetoanalyze/java/threadsafety/Containers.java, void Containers.containerWrapperUnownedWriteBad(Object), 1, THREAD_SAFETY_VIOLATION, [call to Object ContainerWrapper.write(Object),call to Object ContainerWrapper._write(Object),access to codetoanalyze.java.checkers.ContainerWrapper.children.add] -codetoanalyze/java/threadsafety/Containers.java, void Containers.listAddAllBad(Collection), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Containers.mList.addAll] -codetoanalyze/java/threadsafety/Containers.java, void Containers.listAddBad1(String), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Containers.mList.add] -codetoanalyze/java/threadsafety/Containers.java, void Containers.listAddBad2(int,String), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Containers.mList.add] -codetoanalyze/java/threadsafety/Containers.java, void Containers.listClearBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Containers.mList.clear] -codetoanalyze/java/threadsafety/Containers.java, void Containers.listRemoveBad1(int), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Containers.mList.remove] -codetoanalyze/java/threadsafety/Containers.java, void Containers.listRemoveBad2(String), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Containers.mList.remove] -codetoanalyze/java/threadsafety/Containers.java, void Containers.listSetBad(int,String), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Containers.mList.set] -codetoanalyze/java/threadsafety/Containers.java, void Containers.listSubclassWriteBad(ArrayList,int), 1, THREAD_SAFETY_VIOLATION, [access to remove] -codetoanalyze/java/threadsafety/Containers.java, void Containers.mapClearBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Containers.mMap.clear] -codetoanalyze/java/threadsafety/Containers.java, void Containers.mapPutAllBad(Map), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Containers.mMap.putAll] -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/Containers.java, void Containers.containerWrapperUnownedWriteBad(Object), 1, THREAD_SAFETY_VIOLATION, [call to Object ContainerWrapper.write(Object),call to Object ContainerWrapper._write(Object),access to container codetoanalyze.java.checkers.ContainerWrapper.children] +codetoanalyze/java/threadsafety/Containers.java, void Containers.listAddAllBad(Collection), 1, THREAD_SAFETY_VIOLATION, [access to container codetoanalyze.java.checkers.Containers.mList] +codetoanalyze/java/threadsafety/Containers.java, void Containers.listAddBad1(String), 1, THREAD_SAFETY_VIOLATION, [access to container codetoanalyze.java.checkers.Containers.mList] +codetoanalyze/java/threadsafety/Containers.java, void Containers.listAddBad2(int,String), 1, THREAD_SAFETY_VIOLATION, [access to container codetoanalyze.java.checkers.Containers.mList] +codetoanalyze/java/threadsafety/Containers.java, void Containers.listClearBad(), 1, THREAD_SAFETY_VIOLATION, [access to container codetoanalyze.java.checkers.Containers.mList] +codetoanalyze/java/threadsafety/Containers.java, void Containers.listRemoveBad1(int), 1, THREAD_SAFETY_VIOLATION, [access to container codetoanalyze.java.checkers.Containers.mList] +codetoanalyze/java/threadsafety/Containers.java, void Containers.listRemoveBad2(String), 1, THREAD_SAFETY_VIOLATION, [access to container codetoanalyze.java.checkers.Containers.mList] +codetoanalyze/java/threadsafety/Containers.java, void Containers.listSetBad(int,String), 1, THREAD_SAFETY_VIOLATION, [access to container codetoanalyze.java.checkers.Containers.mList] +codetoanalyze/java/threadsafety/Containers.java, void Containers.listSubclassWriteBad(ArrayList,int), 1, THREAD_SAFETY_VIOLATION, [access to container ] +codetoanalyze/java/threadsafety/Containers.java, void Containers.mapClearBad(), 1, THREAD_SAFETY_VIOLATION, [access to container codetoanalyze.java.checkers.Containers.mMap] +codetoanalyze/java/threadsafety/Containers.java, void Containers.mapPutAllBad(Map), 1, THREAD_SAFETY_VIOLATION, [access to container codetoanalyze.java.checkers.Containers.mMap] +codetoanalyze/java/threadsafety/Containers.java, void Containers.mapPutBad(String,String), 1, THREAD_SAFETY_VIOLATION, [access to container codetoanalyze.java.checkers.Containers.mMap] +codetoanalyze/java/threadsafety/Containers.java, void Containers.mapRemoveBad(String), 1, THREAD_SAFETY_VIOLATION, [access to container codetoanalyze.java.checkers.Containers.mMap] +codetoanalyze/java/threadsafety/Containers.java, void Containers.mapSubclassWriteBad(HashMap,String), 1, THREAD_SAFETY_VIOLATION, [access to container ] 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]