[thread-safety] warn on unsafe accesses to lists

Summary: Simple model for List methods that write to the collection.

Reviewed By: peterogithub

Differential Revision: D4453381

fbshipit-source-id: 19edc51

infer/src/checkers/ThreadSafety.ml

@@ -128,11 +128,14 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
           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.List", ("add" | "addAll" | "clear" | "remove" | "set") -> true
             | "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
+            | "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)
       | _ -> false in

infer/tests/codetoanalyze/java/threadsafety/Containers.java

@@ -9,18 +9,72 @@
 
 package codetoanalyze.java.checkers;
 
-import javax.annotation.concurrent.ThreadSafe;
-
+import java.util.ArrayList;
+import java.util.Collection;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CopyOnWriteArrayList;
+
+import javax.annotation.concurrent.ThreadSafe;
 
 @ThreadSafe
 class Containers {
 
+  List<String> mList;
   Map<String,String> mMap;
 
+  // lists
+  void listAddBad1(String s) {
+    mList.add(s);
+  }
+
+  void listAddBad2(int index, String s) {
+    mList.add(index, s);
+  }
+
+  void listAddAllBad(Collection<String> c) {
+    mList.addAll(c);
+  }
+
+  void listClearBad() {
+    mList.clear();
+  }
+
+  void listRemoveBad1(int index) {
+    mList.remove(index);
+  }
+
+  void listRemoveBad2(String s) {
+    mList.remove(s);
+  }
+
+  void listRemoveAllBad(Collection<String> c) {
+    mList.removeAll(c);
+  }
+
+  void listSetBad(int index, String s) {
+    mList.set(index, s);
+  }
+
+  void listSubclassWriteBad(ArrayList<String> list, int index) {
+    list.remove(index);
+  }
+
+  void listReadOk(int index, String s) {
+    mList.contains(s);
+    mList.get(index);
+    mList.isEmpty();
+    mList.size();
+  }
+
+  void accessSafeListOk(CopyOnWriteArrayList list, int index) {
+    list.remove(index);
+  }
+
+  // maps
   void mapPutBad(String key, String value) {
     mMap.put(key, value);
   }
@@ -53,7 +107,6 @@ class Containers {
     m.remove(key);
   }
 
-
   synchronized void synchronizedWriteOk1(String key) {
     mMap.remove(key);
   }

infer/tests/codetoanalyze/java/threadsafety/issues.exp

@@ -5,6 +5,14 @@ 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.<init>(Object),access to Constructors.staticField]
 codetoanalyze/java/threadsafety/Constructors.java, Constructors.<init>(), 1, THREAD_SAFETY_VIOLATION, [access to Constructors.staticField]
 codetoanalyze/java/threadsafety/Constructors.java, Constructors.<init>(Constructors), 1, THREAD_SAFETY_VIOLATION, [access to Constructors.field]
+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]