From cfe79cf1ca41665051f20e98d7fc43ba785aabe0 Mon Sep 17 00:00:00 2001 From: Kyriakos Nikolaos Gkorogiannis Date: Fri, 9 Jun 2017 04:20:34 -0700 Subject: [PATCH] [thread-safety] Add SimplePool as container, mild refactoring of container handling. Reviewed By: sblackshear Differential Revision: D5209926 fbshipit-source-id: e886267 --- infer/src/checkers/ThreadSafety.ml | 85 +++++++++++-------- .../java/threadsafety/Containers.java | 12 +++ .../java/threadsafety/issues.exp | 1 + 3 files changed, 61 insertions(+), 37 deletions(-) diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml index 52769c717..4810c8048 100644 --- a/infer/src/checkers/ThreadSafety.ml +++ b/infer/src/checkers/ThreadSafety.ml @@ -329,7 +329,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct | ("android.app.Activity" | "android.view.View"), "findViewById" -> (* assume findViewById creates fresh View's (note: not always true) *) true - | "android.support.v4.util.Pools$SynchronizedPool", "acquire" -> + | "android.support.v4.util.Pools$SimplePool", "acquire" -> (* a pool should own all of its objects *) true | _ -> @@ -352,51 +352,62 @@ module TransferFunctions (CFG : ProcCfg.S) = struct | "android.support.v4.util.SimpleArrayMap", ("clear" | "ensureCapacity" | "put" | "putAll" | "remove" | "removeAt" | "setValueAt") -> true + | "android.support.v4.util.Pools$SimplePool", + ("acquire" | "release") -> true | "java.util.List", ("add" | "addAll" | "clear" | "remove" | "set") -> true | "java.util.Map", ("clear" | "put" | "putAll" | "remove") -> true | _ -> false in - let is_threadsafe_collection typename _ = match Typ.Name.name typename with - | "java.util.concurrent.ConcurrentMap" | "java.util.concurrent.CopyOnWriteArrayList" -> - true - | _ -> - false in - PatternMatch.supertype_exists tenv is_container_write_ typename && - not (PatternMatch.supertype_exists tenv is_threadsafe_collection typename) + PatternMatch.supertype_exists tenv is_container_write_ typename | _ -> false - let is_synchronized_container ((_, (base_typ : Typ.t)), accesses) tenv = - let is_annotated_synchronized base_typename container_field tenv = - match Tenv.lookup tenv base_typename with - | Some base_typ -> - Annotations.field_has_annot - container_field - base_typ Annotations.ia_is_synchronized_collection - | None -> - false in - match List.rev accesses with - | AccessPath.FieldAccess base_field :: - AccessPath.FieldAccess container_field :: _-> - let base_typename = - Typ.Name.Java.from_string (Fieldname.java_get_class base_field) in - is_annotated_synchronized base_typename container_field tenv - | [AccessPath.FieldAccess container_field] -> - begin - match base_typ.desc with - | Typ.Tstruct base_typename | Tptr ({Typ.desc=Tstruct base_typename}, _) -> - is_annotated_synchronized base_typename container_field tenv - | _ -> - false - end - | _ -> - false + let is_threadsafe_collection pn tenv = match pn with + | Typ.Procname.Java java_pname -> + let typename = Typ.Name.Java.from_string (Typ.Procname.java_get_class_name java_pname) in + let aux tn _ = + match Typ.Name.name tn with + | "java.util.concurrent.ConcurrentMap" + | "java.util.concurrent.CopyOnWriteArrayList" + | "android.support.v4.util.Pools$SynchronizedPool" -> true + | _ -> false in + PatternMatch.supertype_exists tenv aux typename + | _ -> false + + let is_synchronized_container callee_pname ((_, (base_typ : Typ.t)), accesses) tenv = + if is_threadsafe_collection callee_pname tenv + then + true + else + let is_annotated_synchronized base_typename container_field tenv = + match Tenv.lookup tenv base_typename with + | Some base_typ -> + Annotations.field_has_annot + container_field + base_typ Annotations.ia_is_synchronized_collection + | None -> + false in + match List.rev accesses with + | AccessPath.FieldAccess base_field :: + AccessPath.FieldAccess container_field :: _-> + let base_typename = + Typ.Name.Java.from_string (Fieldname.java_get_class base_field) in + is_annotated_synchronized base_typename container_field tenv + | [AccessPath.FieldAccess container_field] -> + begin + match base_typ.desc with + | Typ.Tstruct base_typename | Tptr ({Typ.desc=Tstruct base_typename}, _) -> + is_annotated_synchronized base_typename container_field tenv + | _ -> + false + end + | _ -> + false - let add_container_write callee_pname receiver_ap callee_loc tenv = - (* return true if field pointing to container is marked @SyncrhonizedContainer *) + let make_container_write callee_pname receiver_ap callee_loc tenv = (* create a dummy write that represents mutating the contents of the container *) let open Domain in let callee_accesses = - if is_synchronized_container receiver_ap tenv + if is_synchronized_container callee_pname receiver_ap tenv then AccessDomain.empty else @@ -420,7 +431,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct failwithf "Call to %a is marked as a container write, but has no receiver" Typ.Procname.pp callee_pname in - add_container_write callee_pname receiver_ap callee_loc tenv + make_container_write callee_pname receiver_ap callee_loc tenv else Summary.read_summary caller_pdesc callee_pname diff --git a/infer/tests/codetoanalyze/java/threadsafety/Containers.java b/infer/tests/codetoanalyze/java/threadsafety/Containers.java index d188c206e..3222ce37c 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/Containers.java +++ b/infer/tests/codetoanalyze/java/threadsafety/Containers.java @@ -24,6 +24,7 @@ import android.support.v4.util.Pools.SynchronizedPool; import android.support.v4.util.SparseArrayCompat; import android.util.SparseArray; import android.support.v4.util.SimpleArrayMap; +import android.support.v4.util.Pools.SimplePool; class ContainerWrapper { private final List children = new ArrayList(); @@ -252,5 +253,16 @@ class Containers { return si_map.get(1); } + SimplePool simplePool = new SimplePool(10); + synchronized public Integer getFromPoolOK() { + return simplePool.acquire(); + } + public void poolBad() { + Integer a; + synchronized(this) { + a = simplePool.acquire(); + } + simplePool.release(a); + } } diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp index ec8572dc0..617b2b2b8 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp +++ b/infer/tests/codetoanalyze/java/threadsafety/issues.exp @@ -38,6 +38,7 @@ codetoanalyze/java/threadsafety/Containers.java, void Containers.mapPutAllBad(Ma codetoanalyze/java/threadsafety/Containers.java, void Containers.mapPutBad(String,String), 1, THREAD_SAFETY_VIOLATION, [container `codetoanalyze.java.checkers.Containers.mMap` via call to `put`] codetoanalyze/java/threadsafety/Containers.java, void Containers.mapRemoveBad(String), 1, THREAD_SAFETY_VIOLATION, [container `codetoanalyze.java.checkers.Containers.mMap` via call to `remove`] codetoanalyze/java/threadsafety/Containers.java, void Containers.mapSubclassWriteBad(HashMap,String), 1, THREAD_SAFETY_VIOLATION, [container `&m` via call to `remove`] +codetoanalyze/java/threadsafety/Containers.java, void Containers.poolBad(), 5, THREAD_SAFETY_VIOLATION, [container `codetoanalyze.java.checkers.Containers.simplePool` via call to `release`] 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_read`,,access to `codetoanalyze.java.checkers.DeDup.colocated_read`] 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(), 2, THREAD_SAFETY_VIOLATION, [access to `codetoanalyze.java.checkers.DeDup.field`]