[thread-safety] support conditional writes to containers

Reviewed By: jeremydubreil

Differential Revision: D4544815

fbshipit-source-id: a96d074

infer/src/checkers/ThreadSafety.ml

@@ -220,20 +220,40 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
           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 : Domain.astate) =
+    let add_container_write callee_pname actuals ~f_resolve_id callee_loc =
+      match actuals with
+      | (receiver_exp, receiver_typ) :: _ ->
+          (* create a dummy write that represents mutating the contents of the container *)
+          let open Domain in
           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 unconditional_writes =
-        add_path_to_state
-          dummy_access_exp
-          typ
-          loc
-          astate.unconditional_writes
-          astate.id_map
-          astate.attribute_map
-          tenv in
-      { astate with unconditional_writes; } in
+            Ident.create_fieldname (Mangled.from_string (Procname.get_method callee_pname)) 0 in
+          let dummy_access_exp = Exp.Lfield (receiver_exp, dummy_fieldname, receiver_typ) in
+          let callee_conditional_writes =
+            match AccessPath.of_lhs_exp dummy_access_exp receiver_typ ~f_resolve_id with
+            | Some container_ap ->
+                let writes =
+                  PathDomain.add_sink
+                    (make_access container_ap callee_loc)
+                    PathDomain.empty in
+                ConditionalWritesDomain.add 0 writes ConditionalWritesDomain.empty
+            | None ->
+                ConditionalWritesDomain.empty in
+          Some
+            (false,
+             PathDomain.empty,
+             callee_conditional_writes,
+             PathDomain.empty,
+             AttributeSetDomain.empty)
+      | _ ->
+          failwithf
+            "Call to %a is marked as a container write, but has no receiver"
+            Procname.pp callee_pname in
+    let get_summary caller_pdesc callee_pname actuals ~f_resolve_id callee_loc tenv =
+      if is_container_write callee_pname tenv
+      then
+        add_container_write callee_pname actuals ~f_resolve_id callee_loc
+      else
+        Summary.read_summary caller_pdesc callee_pname in
     let is_unprotected is_locked =
       not is_locked && not (Procdesc.is_java_synchronized pdesc) in
     (* return true if the given procname boxes a primitive type into a reference type *)
@@ -292,17 +312,8 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
           | Unlock ->
               { astate with locks = false; }
           | NoEffect ->
-              if is_unprotected astate.locks && is_container_write callee_pname tenv
-              then
-                match actuals with
-                | (receiver_exp, receiver_typ) :: _ ->
-                    add_container_write callee_pname loc receiver_exp receiver_typ astate
-                | [] ->
-                    failwithf
-                      "Call to %a is marked as a container write, but has no receiver"
-                      Procname.pp callee_pname
-              else
-                match Summary.read_summary pdesc callee_pname with
+              match
+                get_summary pdesc callee_pname actuals ~f_resolve_id loc tenv with
               | Some (callee_locks,
                       callee_reads,
                       callee_conditional_writes,
@@ -310,8 +321,6 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
                       return_attributes) ->
                   let locks' = callee_locks || astate.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 callee_pname loc in

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

@@ -20,6 +20,21 @@ import java.util.concurrent.CopyOnWriteArrayList;
 
 import javax.annotation.concurrent.ThreadSafe;
 
+class ContainerWrapper {
+  private final List<Object> children = new ArrayList<Object>();
+
+  public Object write(Object v) {
+    return _write(v);
+  }
+
+  private Object _write(Object node)
+  {
+    children.add(node);
+    return this;
+  }
+
+}
+
 @ThreadSafe
 class Containers {
 
@@ -126,4 +141,15 @@ class Containers {
     concurrentHashMap.remove(key);
   }
 
+  public void containerWrapperOwnedWriteOk(Object o) {
+    ContainerWrapper wrapper = new ContainerWrapper();
+    wrapper.write(o);
+  }
+
+  ContainerWrapper mContainerWrapper;
+
+  public void containerWrapperUnownedWriteBad(Object o) {
+    mContainerWrapper.write(o);
+  }
+
 }

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

@@ -10,6 +10,7 @@ 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.containerWrapperUnownedWriteBad(Object), 1, THREAD_SAFETY_VIOLATION, [call to Object ContainerWrapper.write(Object),call to Object ContainerWrapper._write(Object),access to codetoanalyze.java.checkers.ContainerWrapper.children.add]
 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]