From 73f219560d77974abf5cce8b84f21642876cbb4f Mon Sep 17 00:00:00 2001 From: Sam Blackshear Date: Tue, 10 Jan 2017 12:57:00 -0800 Subject: [PATCH] [thread-safety] warn on unsafe accesses to maps Summary: Adding models that allow us to warn on unguarded accesses to subclasses of `Map`, but not on accesses of threadsafe containers like `ConcurrentMap`. Lots more containers to model later, but stopping at `Map`s for now to make sure the approach looks ok. Reviewed By: jvillard Differential Revision: D4385306 fbshipit-source-id: d791eee --- infer/src/checkers/ThreadSafety.ml | 81 +++++++++++++------ .../java/threadsafety/Containers.java | 74 +++++++++++++++++ .../java/threadsafety/issues.exp | 5 ++ 3 files changed, 135 insertions(+), 25 deletions(-) create mode 100644 infer/tests/codetoanalyze/java/threadsafety/Containers.java diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml index bbab6a01c..09b58828a 100644 --- a/infer/src/checkers/ThreadSafety.ml +++ b/infer/src/checkers/ThreadSafety.ml @@ -91,14 +91,35 @@ module TransferFunctions (CFG : ProcCfg.S) = struct let exec_instr ({ ThreadSafetyDomain.locks; reads; writes; id_map; owned; } as astate) - { ProcData.pdesc; } _ = + { ProcData.pdesc; tenv; } _ = let is_allocation pn = Procname.equal pn BuiltinDecl.__new || Procname.equal pn BuiltinDecl.__new_array in + let is_container_write pn tenv = match pn with + | Procname.Java java_pname -> + let typename = Typename.Java.from_string (Procname.java_get_class_name java_pname) in + let is_container_write_ typename _ = + match Typename.name typename, Procname.java_get_method java_pname with + | "java.util.Map", ("clear" | "put" | "putAll" | "remove") -> true + | _ -> false in + let is_threadsafe_collection typename _ = match Typename.name typename with + | "java.util.concurrent.ConcurrentMap" -> true + | _ -> false in + PatternMatch.supertype_exists tenv is_container_write_ typename && + not (PatternMatch.supertype_exists tenv is_threadsafe_collection typename) + | _ -> false in + let add_container_write pn loc exp typ (astate : ThreadSafetyDomain.astate) = + let dummy_fieldname = + Ident.create_fieldname (Mangled.from_string (Procname.get_method pn)) 0 in + let dummy_access_exp = Exp.Lfield (exp, dummy_fieldname, typ) in + let writes = + add_path_to_state dummy_access_exp typ loc astate.writes astate.id_map astate.owned in + { astate with writes; } in let is_unprotected is_locked = not is_locked && not (Procdesc.is_java_synchronized pdesc) in let f_resolve_id = resolve_id id_map in + function | Sil.Call (Some (lhs_id, lhs_typ), Const (Cfun pn), _, _, _) when is_allocation pn -> begin @@ -109,7 +130,7 @@ module TransferFunctions (CFG : ProcCfg.S) = struct | None -> astate end - | Sil.Call (_, Const (Cfun pn), _, loc, _) -> + | Sil.Call (_, Const (Cfun pn), actuals, loc, _) -> begin (* assuming that modeled procedures do not have useful summaries *) match get_lock_model pn with @@ -118,29 +139,39 @@ module TransferFunctions (CFG : ProcCfg.S) = struct | Unlock -> { astate with locks = false; } | NoEffect -> - begin - match Summary.read_summary pdesc pn with - | Some (callee_locks, callee_reads, callee_writes) -> - let locks' = callee_locks || locks in - let astate' = - (* TODO (14842325): report on constructors that aren't threadsafe - (e.g., constructors that access static fields) *) - if is_unprotected locks' - then - let call_site = CallSite.make pn loc in - let reads' = - ThreadSafetyDomain.PathDomain.with_callsite callee_reads call_site - |> ThreadSafetyDomain.PathDomain.join reads in - let writes' = - ThreadSafetyDomain.PathDomain.with_callsite callee_writes call_site - |> ThreadSafetyDomain.PathDomain.join writes in - { astate with reads = reads'; writes = writes'; } - else - astate in - { astate' with locks = locks'; } - | None -> - astate - end + if is_unprotected locks && is_container_write pn tenv + then + match actuals with + | (receiver_exp, receiver_typ) :: _ -> + add_container_write pn loc receiver_exp receiver_typ astate + | [] -> + failwithf + "Call to %a is marked as a container write, but has no receiver" + Procname.pp pn + else + begin + match Summary.read_summary pdesc pn with + | Some (callee_locks, callee_reads, callee_writes) -> + let locks' = callee_locks || locks in + let astate' = + (* TODO (14842325): report on constructors that aren't threadsafe + (e.g., constructors that access static fields) *) + if is_unprotected locks' + then + let call_site = CallSite.make pn loc in + let reads' = + ThreadSafetyDomain.PathDomain.with_callsite callee_reads call_site + |> ThreadSafetyDomain.PathDomain.join reads in + let writes' = + ThreadSafetyDomain.PathDomain.with_callsite callee_writes call_site + |> ThreadSafetyDomain.PathDomain.join writes in + { astate with reads = reads'; writes = writes'; } + else + astate in + { astate' with locks = locks'; } + | None -> + astate + end end | Sil.Store (Exp.Lvar lhs_pvar, lhs_typ, rhs_exp, _) when Pvar.is_frontend_tmp lhs_pvar -> diff --git a/infer/tests/codetoanalyze/java/threadsafety/Containers.java b/infer/tests/codetoanalyze/java/threadsafety/Containers.java new file mode 100644 index 000000000..9a28b9480 --- /dev/null +++ b/infer/tests/codetoanalyze/java/threadsafety/Containers.java @@ -0,0 +1,74 @@ +/* + * Copyright (c) 2017 - 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 java.util.HashMap; +import java.util.Map; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.ConcurrentHashMap; + +@ThreadSafe +class Containers { + + Map mMap; + + void mapPutBad(String key, String value) { + mMap.put(key, value); + } + + void mapRemoveBad(String key) { + mMap.remove(key); + } + + void mapClearBad() { + mMap.clear(); + } + + void mapPutAllBad(Map otherMap) { + mMap.putAll(otherMap); + } + + void mapReadsOk(String s) { + mMap.containsKey(s); + mMap.containsValue(s); + mMap.entrySet(); + mMap.hashCode(); + mMap.isEmpty(); + mMap.keySet(); + mMap.size(); + mMap.values(); + } + + // make sure we still warn on subtypes of Map + void mapSubclassWriteBad(HashMap m, String key) { + m.remove(key); + } + + + synchronized void synchronizedWriteOk1(String key) { + mMap.remove(key); + } + + void synchronizedWriteOk2(String key, String lock) { + synchronized (lock) { + mMap.remove(key); + } + } + + void accessToSychronizedMapsOk( + String key, + ConcurrentMap concurrentMap, + ConcurrentHashMap concurrentHashMap) { + + concurrentMap.remove(key); + concurrentHashMap.remove(key); + } + +} diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp index 9bdfb468a..509dd2a00 100644 --- a/infer/tests/codetoanalyze/java/threadsafety/issues.exp +++ b/infer/tests/codetoanalyze/java/threadsafety/issues.exp @@ -1,6 +1,11 @@ codetoanalyze/java/threadsafety/Annotations.java, void Annotations.mutateOffUiThreadBad(), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Annotations.f] codetoanalyze/java/threadsafety/Builders.java, Builders$Obj Builders.buildThenMutateBad(Builders$Obj), 3, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Builders$Obj.g] codetoanalyze/java/threadsafety/Builders.java, Builders$Obj Builders.mutateBad(Builders$Obj), 1, THREAD_SAFETY_VIOLATION, [access to codetoanalyze.java.checkers.Builders$Obj.g] +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/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]