diff --git a/infer/src/checkers/ThreadSafety.ml b/infer/src/checkers/ThreadSafety.ml index f33c02871..2571b4237 100644 --- 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 new file mode 100644 index 000000000..c1419bdb7 --- /dev/null +++ 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 new file mode 100644 index 000000000..88152fe57 --- /dev/null +++ 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 index e0a7297a4..599e4c7af 100644 --- 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 class ThreadSafeExample{ 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 class ThreadSafeExample{ 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 index e641a0ad8..2949eaf96 100644 --- 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]