From 4255d918ad3860b7403db88fb7335f4fe33520f3 Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Wed, 11 Oct 2017 09:45:30 -0700 Subject: [PATCH] [thread-safety][cleanup] clean up error messages/comments Summary: First step in a bigger initiative to improve error messages Reviewed By: da319 Differential Revision: D6025869 fbshipit-source-id: d921166 --- infer/src/checkers/ThreadSafety.ml | 30 ++++++++----------- .../java/threadsafety/issues.exp | 4 +-- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml index 40d285699..a7f947561 100644 --- a/infer/src/checkers/ThreadSafety.ml +++ b/infer/src/checkers/ThreadSafety.ml @@ -398,7 +398,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct with | "android.content.res.Resources", method_name -> (* all methods of Resources are considered @Functional except for the ones in this - blacklist *) + blacklist *) let non_functional_resource_methods = [ "getAssets" ; "getConfiguration" @@ -509,7 +509,6 @@ module TransferFunctions (CFG : ProcCfg.S) = struct AccessDomain.add_access (Unprotected (IntSet.singleton 0)) container_access AccessDomain.empty in - (* we need a more intelligent escape analysis, that branches on whether we own the container *) Some { locks= false ; threads= ThreadsDomain.empty @@ -734,14 +733,14 @@ module TransferFunctions (CFG : ProcCfg.S) = struct OwnershipDomain.get_owned actual_access_path astate.ownership with | OwnershipAbstractValue.OwnedIf formal_indexes - -> (* access path conditionally owned if [formal_indexex] are + -> (* access path conditionally owned if [formal_indexes] are owned *) AccessPrecondition.Unprotected formal_indexes | OwnershipAbstractValue.Owned -> assert false | OwnershipAbstractValue.Unowned -> (* access path not rooted in a formal and not conditionally - owned *) + owned *) AccessPrecondition.TotallyUnprotected in update_caller_accesses pre ownership_accesses accesses_acc @@ -1055,7 +1054,6 @@ let analyze_procedure {Callbacks.proc_desc; get_proc_desc; tenv; summary} = Typ.Procname.is_constructor proc_name || FbThreadSafety.is_custom_init tenv proc_name in let open ThreadSafetyDomain in - (* convert the abstract state to a summary by dropping the id map *) if should_analyze_proc proc_desc tenv then ( if not (Procdesc.did_preanalysis proc_desc) then Preanal.do_liveness proc_desc tenv ; let formal_map = FormalMap.make proc_desc in @@ -1076,9 +1074,9 @@ let analyze_procedure {Callbacks.proc_desc; get_proc_desc; tenv; summary} = -> acc in let owned_formals = - (* if a constructer is called via DI, all of its formals will be freshly allocated - and therefore owned. we assume that constructors annotated with @Inject will only - be called via DI or using fresh parameters. *) + (* if a constructer is called via DI, all of its formals will be freshly allocated and + therefore owned. we assume that constructors annotated with @Inject will only be + called via DI or using fresh parameters. *) if Annotations.pdesc_has_return_annot proc_desc Annotations.ia_is_inject then List.mapi ~f:(fun i _ -> i) (Procdesc.get_formals proc_desc) else [0] @@ -1190,7 +1188,7 @@ let desc_of_sink sink = else F.asprintf "call to %a" Typ.Procname.pp sink_pname | InterfaceCall _ as access -> if Typ.Procname.equal sink_pname Typ.Procname.empty_block then - F.asprintf "%a1" ThreadSafetyDomain.Access.pp access + F.asprintf "%a" ThreadSafetyDomain.Access.pp access else F.asprintf "call to %a" Typ.Procname.pp sink_pname let trace_of_pname orig_sink orig_pdesc callee_pname = @@ -1296,7 +1294,7 @@ let make_read_write_race_description ~read_is_sync conflicts tenv pname final_si in let conflicts_description = Format.asprintf "Potentially races with%s writes in method%s %a." - (if read_is_sync then " unsynchronized" else " synchronized") + (if read_is_sync then " unsynchronized" else "") (if Typ.Procname.Set.cardinal conflicting_proc_names > 1 then "s" else "") (MF.wrap_monospaced pp_conflicts) conflicting_proc_names in @@ -1414,7 +1412,7 @@ let report_unsafe_accesses match Procdesc.get_proc_name pdesc with | Java _ -> (** TODO: use a better warning message when the thread is anything but Any (perhaps - show the write that this may race with) *) + show the write that this may race with) *) (* unprotected write. warn. *) report_thread_safety_violation tenv pdesc IssueType.thread_safety_violation ~make_description:make_unprotected_write_description ~conflicts:[] access thread ; @@ -1428,8 +1426,8 @@ let report_unsafe_accesses reported_acc | ( (Access.Read _ | ContainerRead _) , (AccessPrecondition.Unprotected _ | AccessPrecondition.TotallyUnprotected) ) - -> (* unprotected read. report all writes as conflicts for java *) - (* for c++ filter out unprotected writes *) + -> (* unprotected read. report all writes as conflicts for java. for c++ filter out + unprotected writes *) let is_cpp_protected_write pre = match pre with | AccessPrecondition.Unprotected _ | TotallyUnprotected @@ -1454,10 +1452,8 @@ let report_unsafe_accesses access thread ; update_reported access pname reported_acc ) | (Access.Read _ | ContainerRead _), AccessPrecondition.Protected excl - -> (* protected read. - report unprotected writes and opposite protected writes as conflicts - Thread and Lock are opposites of one another, and - Both has no opposite*) + -> (* protected read. report unprotected writes and opposite protected writes as conflicts + Thread and Lock are opposites of one another, and Both has no opposite *) let is_opposite = function | Excluder.Lock, Excluder.Thread -> true diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp index a46a7b333..e88a90c97 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp +++ b/infer/tests/codetoanalyze/java/threadsafety/issues.exp @@ -44,8 +44,8 @@ codetoanalyze/java/threadsafety/Containers.java, void Containers.mapPutBad(Strin codetoanalyze/java/threadsafety/Containers.java, void Containers.mapRemoveBad(String), 1, THREAD_SAFETY_VIOLATION, [Write to container `&this.codetoanalyze.java.checkers.Containers.mMap` via call to `remove`] codetoanalyze/java/threadsafety/Containers.java, void Containers.mapSubclassWriteBad(HashMap,String), 1, THREAD_SAFETY_VIOLATION, [Write to container `&m` via call to `remove`] codetoanalyze/java/threadsafety/Containers.java, void Containers.poolBad(), 5, THREAD_SAFETY_VIOLATION, [Write to container `&this.codetoanalyze.java.checkers.Containers.simplePool` via call to `release`] -codetoanalyze/java/threadsafety/Dispatch.java, void Dispatch.callUnannotatedInterfaceBad(UnannotatedInterface), 1, INTERFACE_NOT_THREAD_SAFE, [Call to un-annotated interface method void UnannotatedInterface.foo()1] -codetoanalyze/java/threadsafety/Dispatch.java, void Dispatch.callUnannotatedInterfaceIndirectBad(NotThreadSafe,UnannotatedInterface), 1, INTERFACE_NOT_THREAD_SAFE, [call to void NotThreadSafe.notThreadSafeOk(UnannotatedInterface),Call to un-annotated interface method void UnannotatedInterface.foo()1] +codetoanalyze/java/threadsafety/Dispatch.java, void Dispatch.callUnannotatedInterfaceBad(UnannotatedInterface), 1, INTERFACE_NOT_THREAD_SAFE, [Call to un-annotated interface method void UnannotatedInterface.foo()] +codetoanalyze/java/threadsafety/Dispatch.java, void Dispatch.callUnannotatedInterfaceIndirectBad(NotThreadSafe,UnannotatedInterface), 1, INTERFACE_NOT_THREAD_SAFE, [call to void NotThreadSafe.notThreadSafeOk(UnannotatedInterface),Call to un-annotated interface method void UnannotatedInterface.foo()] codetoanalyze/java/threadsafety/Locks.java, void Locks.FP_unlockOneLock(), 4, THREAD_SAFETY_VIOLATION, [access to `&this.codetoanalyze.java.checkers.Locks.f`] codetoanalyze/java/threadsafety/Locks.java, void Locks.afterReentrantLockUnlockBad(), 3, THREAD_SAFETY_VIOLATION, [access to `&this.codetoanalyze.java.checkers.Locks.f`] codetoanalyze/java/threadsafety/Locks.java, void Locks.afterUnlockBad(), 3, THREAD_SAFETY_VIOLATION, [access to `&this.codetoanalyze.java.checkers.Locks.f`]