From 31bb9b399aeb7e431461107beab58da73831cafc Mon Sep 17 00:00:00 2001 From: Nikos Gorogiannis Date: Wed, 16 Oct 2019 04:05:35 -0700 Subject: [PATCH] [starvation] add tests documenting path sensitivity FPs and FNs Summary: Starvation is currently path insensitive. Two special cases of sensitivity cover a large range of useful cases: - sensitivity on whether the current thread is a UI/background thread; - sensitivity on whether a lock can be acquired (without blocking) or not. We add a few tests capturing some of the false positives and negatives of the current analysis. Reviewed By: mityal Differential Revision: D17907492 fbshipit-source-id: fbce896ac --- .../java/starvation/LockSensitivity.java | 49 ++++++++++ .../java/starvation/OurThreadUtils.java | 6 ++ .../java/starvation/ThreadSensitivity.java | 89 +++++++++++++++++++ .../codetoanalyze/java/starvation/issues.exp | 6 ++ 4 files changed, 150 insertions(+) create mode 100644 infer/tests/codetoanalyze/java/starvation/LockSensitivity.java create mode 100644 infer/tests/codetoanalyze/java/starvation/ThreadSensitivity.java diff --git a/infer/tests/codetoanalyze/java/starvation/LockSensitivity.java b/infer/tests/codetoanalyze/java/starvation/LockSensitivity.java new file mode 100644 index 000000000..6e9d448dc --- /dev/null +++ b/infer/tests/codetoanalyze/java/starvation/LockSensitivity.java @@ -0,0 +1,49 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +// Tests documenting FPs and FNs due to lack of sensitivity in starvation analysis + +import java.util.concurrent.locks.Lock; + +class LockSensitivity { + Lock lockA, lockB; + + // In the following two methods, AB vs BA deadlock pattern + // remains undetected since one of the locks happens via `tryLock` and result check. + + public void FN_tryLockDeadlockAB_Bad() { + boolean locked = lockA.tryLock(); + if (locked) { + lockB.lock(); + lockB.unlock(); + lockA.unlock(); + } else { + } + } + + public void FN_tryLockDeadlockBA_Bad() { + lockB.lock(); + lockA.lock(); // deadlock: `lockA` may be locked via `tryLock()` above + lockA.unlock(); + lockB.unlock(); + } + + // Asserting a lock is held is not the same as taking it wrt deadlocks. + // In the following two methods, AB vs BA pattern is wrongly detected. + + Object monitorA, monitorB; + + public void FP_assertHoldsLockAB_Ok() { + OurThreadUtils.assertHoldsLock(monitorA); + OurThreadUtils.assertHoldsLock(monitorB); + } + + public void FP_assertHoldsLockBA_Ok() { + OurThreadUtils.assertHoldsLock(monitorB); + OurThreadUtils.assertHoldsLock(monitorA); + } +} diff --git a/infer/tests/codetoanalyze/java/starvation/OurThreadUtils.java b/infer/tests/codetoanalyze/java/starvation/OurThreadUtils.java index 1435b1253..507f419ec 100644 --- a/infer/tests/codetoanalyze/java/starvation/OurThreadUtils.java +++ b/infer/tests/codetoanalyze/java/starvation/OurThreadUtils.java @@ -8,7 +8,13 @@ class OurThreadUtils { static native boolean isMainThread(); + static native boolean isUiThread(); + static void assertMainThread() {} + static void assertOnUiThread() {} + + static void assertOnBackgroundThread() {} + static void assertHoldsLock(Object lock) {} } diff --git a/infer/tests/codetoanalyze/java/starvation/ThreadSensitivity.java b/infer/tests/codetoanalyze/java/starvation/ThreadSensitivity.java new file mode 100644 index 000000000..9aed39a81 --- /dev/null +++ b/infer/tests/codetoanalyze/java/starvation/ThreadSensitivity.java @@ -0,0 +1,89 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +// Examples where the code checks what thread it's currently running on. +class ThreadSensitivity { + Object monitorA, monitorB; + + void FN_conditionalAssertMainThread_Bad(boolean b) { + if (b) { + // this branch asserts on Main thread + OurThreadUtils.assertMainThread(); + synchronized (monitorA) { + synchronized (monitorB) { + } + } + } else { + // this branch asserts nothing, so may run in parallel with the other branch + synchronized (monitorB) { + synchronized (monitorA) { + } + } + } + } + + Object monitorC, monitorD; + + // the branches in the following two methods are both on main/UI thread so cannot deadlock + + void FP_conditionalIsMainThread_Ok() { + if (OurThreadUtils.isMainThread()) { + synchronized (monitorC) { + synchronized (monitorD) { + } + } + } + } + + void FP_conditionalIsUiThread_Ok() { + if (OurThreadUtils.isUiThread()) { + synchronized (monitorD) { + synchronized (monitorC) { + } + } + } + } + + Object monitorE, monitorF; + // identical to the first case above but negated + void conditionalNegatedIsMainThread_Bad() { + if (!OurThreadUtils.isMainThread()) { + synchronized (monitorE) { + synchronized (monitorF) { + } + } + } else { + synchronized (monitorF) { + synchronized (monitorE) { + } + } + } + } + + Object monitorG, monitorH; + + public void FN_confusedAssertBad(boolean b, boolean c) { + if (b) { + OurThreadUtils.assertOnBackgroundThread(); + } else { + OurThreadUtils.assertOnUiThread(); + } + + // not sure if we're on UI or background, should report + if (c) { + synchronized (monitorG) { + synchronized (monitorH) { + } + } + } else { + synchronized (monitorH) { + synchronized (monitorG) { + } + } + } + } +} diff --git a/infer/tests/codetoanalyze/java/starvation/issues.exp b/infer/tests/codetoanalyze/java/starvation/issues.exp index 8ced2bc3a..acbc14dac 100644 --- a/infer/tests/codetoanalyze/java/starvation/issues.exp +++ b/infer/tests/codetoanalyze/java/starvation/issues.exp @@ -20,6 +20,8 @@ codetoanalyze/java/starvation/InnerClass.java, InnerClass.outerInnerBad(InnerCla codetoanalyze/java/starvation/Interclass.java, Interclass.interclass1Bad(InterclassA):void, 10, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void Interclass.interclass1Bad(InterclassA)`, locks `this` in `class Interclass`,Method call: `void InterclassA.interclass1Bad()`, locks `this` in `class InterclassA`,[Trace 2] `void InterclassA.interclass2Bad(Interclass)`, locks `this` in `class InterclassA`,Method call: `void Interclass.interclass2Bad()`, locks `this` in `class Interclass`] codetoanalyze/java/starvation/Interclass.java, InterclassA.interclass2Bad(Interclass):void, 37, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void InterclassA.interclass2Bad(Interclass)`, locks `this` in `class InterclassA`,Method call: `void Interclass.interclass2Bad()`, locks `this` in `class Interclass`,[Trace 2] `void Interclass.interclass1Bad(InterclassA)`, locks `this` in `class Interclass`,Method call: `void InterclassA.interclass1Bad()`, locks `this` in `class InterclassA`] codetoanalyze/java/starvation/LegacySync.java, LegacySync.onUiThreadOpBad():java.lang.Object, 25, STARVATION, no_bucket, ERROR, [[Trace 1] `Object LegacySync.onUiThreadOpBad()`, locks `this.table` in `class LegacySync`,[Trace 2] `void LegacySync.notOnUiThreadSyncedBad()`, locks `this.table` in `class LegacySync`,calls `Object Future.get()`,[Trace 1 on UI thread] `Object LegacySync.onUiThreadOpBad()`,`Object LegacySync.onUiThreadOpBad()` is annotated `UiThread`] +codetoanalyze/java/starvation/LockSensitivity.java, LockSensitivity.FP_assertHoldsLockAB_Ok():void, 41, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void LockSensitivity.FP_assertHoldsLockAB_Ok()`, locks `this.monitorA` in `class LockSensitivity`, locks `this.monitorB` in `class LockSensitivity`,[Trace 2] `void LockSensitivity.FP_assertHoldsLockBA_Ok()`, locks `this.monitorB` in `class LockSensitivity`, locks `this.monitorA` in `class LockSensitivity`] +codetoanalyze/java/starvation/LockSensitivity.java, LockSensitivity.FP_assertHoldsLockBA_Ok():void, 46, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void LockSensitivity.FP_assertHoldsLockBA_Ok()`, locks `this.monitorB` in `class LockSensitivity`, locks `this.monitorA` in `class LockSensitivity`,[Trace 2] `void LockSensitivity.FP_assertHoldsLockAB_Ok()`, locks `this.monitorA` in `class LockSensitivity`, locks `this.monitorB` in `class LockSensitivity`] codetoanalyze/java/starvation/LocklessTests.java, LocklessTestsA.locklessMethod():void, 23, LOCKLESS_VIOLATION, no_bucket, ERROR, [`void LocklessTestsA.locklessMethod()`, locks `this` in `class LocklessTestsA`] codetoanalyze/java/starvation/LocklessTests.java, LocklessTestsB.locklessMethod():void, 39, LOCKLESS_VIOLATION, no_bucket, ERROR, [`void LocklessTestsB.locklessMethod()`, locks `this` in `class LocklessTestsB`] codetoanalyze/java/starvation/LocklessTests.java, LocklessTestsC.locklessMethod():void, 52, LOCKLESS_VIOLATION, no_bucket, ERROR, [`void LocklessTestsC.locklessMethod()`,Method call: `void LocklessTestsC.takeLock()`, locks `this` in `class LocklessTestsC`] @@ -65,6 +67,10 @@ codetoanalyze/java/starvation/StrictModeViolation.java, StrictModeViolation.viol codetoanalyze/java/starvation/StrictModeViolation.java, StrictModeViolation.violateStrictModeBad():void, 38, STRICT_MODE_VIOLATION, no_bucket, ERROR, [`void StrictModeViolation.violateStrictModeBad()`,calls `boolean File.setReadOnly()`,[Trace on UI thread] `void StrictModeViolation.violateStrictModeBad()`,`void StrictModeViolation.violateStrictModeBad()` is annotated `UiThread`] codetoanalyze/java/starvation/StrictModeViolation.java, StrictModeViolation.violateStrictModeBad():void, 39, STRICT_MODE_VIOLATION, no_bucket, ERROR, [`void StrictModeViolation.violateStrictModeBad()`,calls `boolean File.setWritable(boolean)`,[Trace on UI thread] `void StrictModeViolation.violateStrictModeBad()`,`void StrictModeViolation.violateStrictModeBad()` is annotated `UiThread`] codetoanalyze/java/starvation/SuppLint.java, SuppLint.onUiThreadBad():void, 25, STARVATION, no_bucket, ERROR, [`void SuppLint.onUiThreadBad()`,calls `Object Future.get()`,[Trace on UI thread] `void SuppLint.onUiThreadBad()`,`void SuppLint.onUiThreadBad()` is annotated `UiThread`] +codetoanalyze/java/starvation/ThreadSensitivity.java, ThreadSensitivity.FP_conditionalIsMainThread_Ok():void, 35, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void ThreadSensitivity.FP_conditionalIsMainThread_Ok()`, locks `this.monitorC` in `class ThreadSensitivity`, locks `this.monitorD` in `class ThreadSensitivity`,[Trace 2] `void ThreadSensitivity.FP_conditionalIsUiThread_Ok()`, locks `this.monitorD` in `class ThreadSensitivity`, locks `this.monitorC` in `class ThreadSensitivity`] +codetoanalyze/java/starvation/ThreadSensitivity.java, ThreadSensitivity.FP_conditionalIsUiThread_Ok():void, 44, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void ThreadSensitivity.FP_conditionalIsUiThread_Ok()`, locks `this.monitorD` in `class ThreadSensitivity`, locks `this.monitorC` in `class ThreadSensitivity`,[Trace 2] `void ThreadSensitivity.FP_conditionalIsMainThread_Ok()`, locks `this.monitorC` in `class ThreadSensitivity`, locks `this.monitorD` in `class ThreadSensitivity`] +codetoanalyze/java/starvation/ThreadSensitivity.java, ThreadSensitivity.conditionalNegatedIsMainThread_Bad():void, 55, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void ThreadSensitivity.conditionalNegatedIsMainThread_Bad()`, locks `this.monitorE` in `class ThreadSensitivity`, locks `this.monitorF` in `class ThreadSensitivity`,[Trace 2] `void ThreadSensitivity.conditionalNegatedIsMainThread_Bad()`, locks `this.monitorF` in `class ThreadSensitivity`, locks `this.monitorE` in `class ThreadSensitivity`] +codetoanalyze/java/starvation/ThreadSensitivity.java, ThreadSensitivity.conditionalNegatedIsMainThread_Bad():void, 60, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void ThreadSensitivity.conditionalNegatedIsMainThread_Bad()`, locks `this.monitorF` in `class ThreadSensitivity`, locks `this.monitorE` in `class ThreadSensitivity`,[Trace 2] `void ThreadSensitivity.conditionalNegatedIsMainThread_Bad()`, locks `this.monitorE` in `class ThreadSensitivity`, locks `this.monitorF` in `class ThreadSensitivity`] codetoanalyze/java/starvation/ThreadSleep.java, ThreadSleep.indirectSleepOnUIThreadBad():void, 24, STARVATION, no_bucket, ERROR, [[Trace 1] `void ThreadSleep.indirectSleepOnUIThreadBad()`, locks `this.lock` in `class ThreadSleep`,[Trace 2] `void ThreadSleep.lockAndSleepOnNonUIThread()`, locks `this.lock` in `class ThreadSleep`,Method call: `void ThreadSleep.sleepOnAnyThreadOk()`,calls `void Thread.sleep(long)`,[Trace 1 on UI thread] `void ThreadSleep.indirectSleepOnUIThreadBad()`,`void ThreadSleep.indirectSleepOnUIThreadBad()` is annotated `UiThread`] codetoanalyze/java/starvation/ThreadSleep.java, ThreadSleep.sleepOnUIThreadBad():void, 17, STARVATION, no_bucket, ERROR, [`void ThreadSleep.sleepOnUIThreadBad()`,calls `void Thread.sleep(long)`,[Trace on UI thread] `void ThreadSleep.sleepOnUIThreadBad()`,`void ThreadSleep.sleepOnUIThreadBad()` is annotated `UiThread`] codetoanalyze/java/starvation/UIDeadlock.java, UIDeadlock.notOnUIThreadBad():void, 34, DEADLOCK, no_bucket, ERROR, [[Trace 1] `void UIDeadlock.notOnUIThreadBad()`, locks `this.lockB` in `class UIDeadlock`, locks `this` in `class UIDeadlock`,[Trace 2] `void UIDeadlock.onUIThreadBad()`, locks `this` in `class UIDeadlock`, locks `this.lockB` in `class UIDeadlock`]