[thread-safety] model ReadLock/WriteLock

Summary: `ReentrantReadWriteLock.ReadLock` and `ReentrantReadWriteLock.WriteLock` are commonly used lock types that were not previously modeled. Reviewed By: peterogithub Differential Revision: D4262032 fbshipit-source-id: 4ff81a7
ryzn0518 · Dec 2, 2016 · c1205c1 · c1205c1
1 parent 826accc
commit c1205c1
Show file tree

Hide file tree

Showing 5 changed files with 180 additions and 118 deletions.
diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml
@@ -44,10 +44,15 @@ module TransferFunctions (CFG : ProcCfg.S) = struct
           match Procname.java_get_class_name java_pname, Procname.java_get_method java_pname with
           | "java.util.concurrent.locks.Lock", "lock" ->
               Lock
-          | "java.util.concurrent.locks.ReentrantLock",
+          | ("java.util.concurrent.locks.ReentrantLock"
+            | "java.util.concurrent.locks.ReentrantReadWriteLock$ReadLock"
+            | "java.util.concurrent.locks.ReentrantReadWriteLock$WriteLock"),
             ("lock" | "tryLock" | "lockInterruptibly") ->
               Lock
-          | ("java.util.concurrent.locks.Lock" | "java.util.concurrent.locks.ReentrantLock"),
+          | ("java.util.concurrent.locks.Lock"
+            |"java.util.concurrent.locks.ReentrantLock"
+            | "java.util.concurrent.locks.ReentrantReadWriteLock$ReadLock"
+            | "java.util.concurrent.locks.ReentrantReadWriteLock$WriteLock"),
             "unlock" ->
               Unlock
           | _ ->

diff --git a/infer/tests/codetoanalyze/java/threadsafety/Locks.java b/infer/tests/codetoanalyze/java/threadsafety/Locks.java
@@ -0,0 +1,146 @@
+/*
+ * Copyright (c) 2016 - 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.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+@ThreadSafe
+public class Locks {
+
+  Integer f;
+
+  Lock mLock;
+  ReadWriteLock mReadWriteLock;
+  ReentrantLock mReentrantLock;
+  ReentrantReadWriteLock mReentrantReadWriteLock;
+
+  public void lockInOneBranchBad(boolean b) {
+    if (b) {
+      mLock.lock();
+    }
+    f = 24;
+    if (b) {
+      mLock.unlock();
+    }
+  }
+
+  public void afterUnlockBad() {
+    mLock.lock();
+    mLock.unlock();
+    f = 42;
+  }
+
+  public void afterReentrantLockUnlockBad() {
+    mReentrantLock.lock();
+    mReentrantLock.unlock();
+    f = 42;
+  }
+
+  public void afterWriteLockUnlockBad() {
+    mReentrantReadWriteLock.writeLock().lock();
+    mReentrantReadWriteLock.writeLock().unlock();
+    f = 42;
+  }
+
+  public void lockOk() {
+    mLock.lock();
+    f = 42;
+    mLock.unlock();
+  }
+
+  public void lockBothBranchesOk(boolean b) {
+    if (b) {
+      mLock.lock();
+    } else {
+      mLock.lock();
+    }
+    f = 42;
+    mLock.unlock();
+  }
+
+  public void reentrantLockOk() {
+    mReentrantLock.lock();
+    f = 42;
+    mReentrantLock.unlock();
+  }
+
+  public void rReentrantLockTryLockOk() {
+    if (mReentrantLock.tryLock()) {
+      f = 42;
+      mReentrantLock.unlock();
+    }
+  }
+
+  public void reentrantLockInterruptiblyOk() throws InterruptedException {
+    mReentrantLock.lockInterruptibly();
+    f = 42;
+    mReentrantLock.unlock();
+  }
+
+  private void acquireLock() {
+    mLock.lock();
+  }
+
+  public void acquireLockInCalleeOk() {
+    acquireLock();
+    f = 42;
+    mLock.unlock();
+  }
+
+  public void writeLockOk() {
+    mReadWriteLock.writeLock().lock();
+    f = 42;
+    mReadWriteLock.writeLock().unlock();
+  }
+
+  public void reentrantWriteLockOk() {
+    mReentrantReadWriteLock.writeLock().lock();
+    f = 42;
+    mReentrantReadWriteLock.writeLock().unlock();
+  }
+
+  private void releaseLock() {
+    mLock.unlock();
+  }
+
+  // our "squish all locks into one" abstraction is not ideal here...
+  public void FP_unlockOneLock() {
+    mLock.lock();
+    mReentrantLock.lock();
+    mReentrantLock.unlock();
+    f = 42;
+    mLock.unlock();
+  }
+
+  // ... or here
+  public void FN_releaseLockInCalleeBad() {
+    mLock.lock();
+    releaseLock();
+    f = 42;
+  }
+
+  // we don't model the case where `tryLock` fails
+  public void FN_withReentrantLockTryLockBad() {
+    if (!mReentrantLock.tryLock()) {
+      f = 42;
+    }
+  }
+
+  // we shouldn't be able to write when holding a readLock
+  public void FN_readLockOk() {
+    mReentrantReadWriteLock.readLock().lock();
+    f = 42;
+    mReentrantReadWriteLock.readLock().unlock();
+  }
+
+}
diff --git a/infer/tests/codetoanalyze/java/threadsafety/ThreadSafe.java b/infer/tests/codetoanalyze/java/threadsafety/ThreadSafe.java
@@ -0,0 +1,22 @@
+/*
+ * Copyright (c) 2016 - 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.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Documented
+@Target(ElementType.TYPE)
+@Retention(RetentionPolicy.CLASS)
+public @interface ThreadSafe {
+}
diff --git a/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java b/infer/tests/codetoanalyze/java/threadsafety/ThreadSafeExample.java
@@ -9,21 +9,6 @@
 
 package codetoanalyze.java.checkers;
 
-import java.lang.annotation.Documented;
-import java.lang.annotation.ElementType;
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
-import java.lang.annotation.Target;
-
-import java.util.concurrent.locks.Lock;
-import java.util.concurrent.locks.ReentrantLock;
-
-@Documented
-@Target(ElementType.TYPE)
-@Retention(RetentionPolicy.CLASS)
-@interface ThreadSafe {
-}
-
 @ThreadSafe
 public class ThreadSafeExample{
 
@@ -46,37 +31,6 @@ public void tsBad() {
     f = 24;
   }
 
-  Lock mLock;
-  ReentrantLock mReentrantLock;
-
-  public void lockInOneBranchBad(boolean b) {
-    if (b) {
-      mLock.lock();
-    }
-    f = 24;
-    if (b) {
-      mLock.unlock();
-    }
-  }
-
-  public void afterUnlockBad() {
-    mLock.lock();
-    mLock.unlock();
-    f = 42;
-  }
-
-  public void afterReentrantLockUnlockBad() {
-    mReentrantLock.lock();
-    mReentrantLock.unlock();
-    f = 42;
-  }
-
-  public void withLockOk() {
-    mLock.lock();
-    f = 42;
-    mLock.unlock();
-  }
-
   // shouldn't report here because it's a private method
   private void assignInPrivateMethodOk() {
     f = 24;
@@ -121,72 +75,6 @@ public void transitivelyCallConstructorOk() {
     returnConstructorOk();
   }
 
-  public void withLockBothBranchesOk(boolean b) {
-    if (b) {
-      mLock.lock();
-    } else {
-      mLock.lock();
-    }
-    f = 42;
-    mLock.unlock();
-  }
-
-  public void withReentrantLockOk() {
-    mReentrantLock.lock();
-    f = 42;
-    mReentrantLock.unlock();
-  }
-
-  public void withReentrantLockTryLockOk() {
-    if (mReentrantLock.tryLock()) {
-      f = 42;
-      mReentrantLock.unlock();
-    }
-  }
-
-  public void withReentrantLockInterruptiblyOk() throws InterruptedException {
-    mReentrantLock.lockInterruptibly();
-    f = 42;
-    mReentrantLock.unlock();
-  }
-
-  private void acquireLock() {
-    mLock.lock();
-  }
-
-  public void acquireLockInCalleeOk() {
-    acquireLock();
-    f = 42;
-    mLock.unlock();
-  }
-
-  private void releaseLock() {
-    mLock.unlock();
-  }
-
-  // our "squish all locks into one" abstraction is not ideal here...
-  public void FP_unlockOneLock() {
-    mLock.lock();
-    mReentrantLock.lock();
-    mReentrantLock.unlock();
-    f = 42;
-    mLock.unlock();
-  }
-
-  // ... or here
-  public void FN_releaseLockInCalleeBad() {
-    mLock.lock();
-    releaseLock();
-    f = 42;
-  }
-
-  // we don't model the case where `tryLock` fails
-  public void FN_withReentrantLockTryLockBad() {
-    if (!mReentrantLock.tryLock()) {
-      f = 42;
-    }
-  }
-
 }
 
 class ExtendsThreadSafeExample extends ThreadSafeExample{

diff --git a/infer/tests/codetoanalyze/java/threadsafety/issues.exp b/infer/tests/codetoanalyze/java/threadsafety/issues.exp
@@ -1,9 +1,10 @@
+codetoanalyze/java/threadsafety/Locks.java, void Locks.FP_unlockOneLock(), 4, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.Locks.f]
+codetoanalyze/java/threadsafety/Locks.java, void Locks.afterReentrantLockUnlockBad(), 3, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.Locks.f]
+codetoanalyze/java/threadsafety/Locks.java, void Locks.afterUnlockBad(), 3, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.Locks.f]
+codetoanalyze/java/threadsafety/Locks.java, void Locks.afterWriteLockUnlockBad(), 3, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.Locks.f]
+codetoanalyze/java/threadsafety/Locks.java, void Locks.lockInOneBranchBad(boolean), 4, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.Locks.f]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ExtendsThreadSafeExample.newmethodBad(), 1, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.ExtendsThreadSafeExample.field]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ExtendsThreadSafeExample.tsOK(), 1, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.ExtendsThreadSafeExample.field]
-codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.FP_unlockOneLock(), 4, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.ThreadSafeExample.f]
-codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.afterReentrantLockUnlockBad(), 3, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.ThreadSafeExample.f]
-codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.afterUnlockBad(), 3, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.ThreadSafeExample.f]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.callPublicMethodBad(), 1, THREAD_SAFETY_ERROR, [call to void ThreadSafeExample.assignInPrivateMethodOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.deeperTraceBad(), 1, THREAD_SAFETY_ERROR, [call to void ThreadSafeExample.callAssignInPrivateMethod(),call to void ThreadSafeExample.assignInPrivateMethodOk(),access to codetoanalyze.java.checkers.ThreadSafeExample.f]
-codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.lockInOneBranchBad(boolean), 4, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.ThreadSafeExample.f]
 codetoanalyze/java/threadsafety/ThreadSafeExample.java, void ThreadSafeExample.tsBad(), 1, THREAD_SAFETY_ERROR, [access to codetoanalyze.java.checkers.ThreadSafeExample.f]