[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

@@ -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
+              if is_unprotected locks && is_container_write pn tenv
-                match Summary.read_summary pdesc pn with
+              then
-                | Some (callee_locks, callee_reads, callee_writes) ->
+                match actuals with
-                    let locks' = callee_locks || locks in
+                | (receiver_exp, receiver_typ) :: _ ->
-                    let astate' =
+                    add_container_write pn loc receiver_exp receiver_typ astate
-                      (* TODO (14842325): report on constructors that aren't threadsafe
+                | [] ->
-                         (e.g., constructors that access static fields) *)
+                    failwithf
-                      if is_unprotected locks'
+                      "Call to %a is marked as a container write, but has no receiver"
-                      then
+                      Procname.pp pn
-                        let call_site = CallSite.make pn loc in
+              else
-                        let reads' =
+                begin
-                          ThreadSafetyDomain.PathDomain.with_callsite callee_reads call_site
+                  match Summary.read_summary pdesc pn with
-                          |> ThreadSafetyDomain.PathDomain.join reads in
+                  | Some (callee_locks, callee_reads, callee_writes) ->
-                        let writes' =
+                      let locks' = callee_locks || locks in
-                          ThreadSafetyDomain.PathDomain.with_callsite callee_writes call_site
+                      let astate' =
-                          |> ThreadSafetyDomain.PathDomain.join writes in
+                        (* TODO (14842325): report on constructors that aren't threadsafe
-                        { astate with reads = reads'; writes = writes'; }
+                           (e.g., constructors that access static fields) *)
-                      else
+                        if is_unprotected locks'
-                        astate in
+                        then
-                    { astate' with locks = locks'; }
+                          let call_site = CallSite.make pn loc in
-                | None ->
+                          let reads' =
-                    astate
+                            ThreadSafetyDomain.PathDomain.with_callsite callee_reads call_site
-              end
+                            |> 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 ->

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<String,String> 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<String,String> 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<String,String> 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<String,String> concurrentMap,
+    ConcurrentHashMap<String,String> concurrentHashMap) {
+
+    concurrentMap.remove(key);
+    concurrentHashMap.remove(key);
+  }
+
+}

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]