diff --git a/java/ql/src/Likely Bugs/Concurrency/UnreleasedLock.ql b/java/ql/src/Likely Bugs/Concurrency/UnreleasedLock.ql index d46acc6aee06..0f02421c7b91 100644 --- a/java/ql/src/Likely Bugs/Concurrency/UnreleasedLock.ql +++ b/java/ql/src/Likely Bugs/Concurrency/UnreleasedLock.ql @@ -118,6 +118,26 @@ predicate heldByCurrentThreadCheck(LockType t, BasicBlock checkblock, BasicBlock ) } +/** + * A variable access in `checkblock` that has `falsesucc` as the false successor. + * + * The variable access must have an assigned value that is a lock access on `t`, and + * the true successor of `checkblock` must contain an unlock access. + */ +predicate variableLockCheck(LockType t, BasicBlock checkblock, BasicBlock falsesucc) { + exists(ConditionBlock conditionBlock, VarAccess v | + v.getType() instanceof BooleanType and + // Ensure that a lock access is assigned to the variable + v.getVariable().getAnAssignedValue() = t.getLockAccess() and + // Ensure that the `true` successor of the condition block contains an unlock access + conditionBlock.getTestSuccessor(true) = t.getUnlockAccess().getBasicBlock() and + conditionBlock.getCondition() = v + | + conditionBlock.getBasicBlock() = checkblock and + conditionBlock.getTestSuccessor(false) = falsesucc + ) +} + /** * A control flow path from a locking call in `src` to `b` such that the number of * locks minus the number of unlocks along the way is positive and equal to `locks`. @@ -131,8 +151,9 @@ predicate blockIsLocked(LockType t, BasicBlock src, BasicBlock b, int locks) { // The number of net locks from the `src` block to the predecessor block `pred` is `predlocks`. blockIsLocked(t, src, pred, predlocks) and // The recursive call ensures that at least one lock is held, so do not consider the false - // successor of the `isHeldByCurrentThread()` check. + // successor of the `isHeldByCurrentThread()` check and of `variableLockCheck`. not heldByCurrentThreadCheck(t, pred, b) and + not variableLockCheck(t, pred, b) and // Count a failed lock as an unlock so the net is zero. (if failedLock(t, pred, b) then failedlock = 1 else failedlock = 0) and ( diff --git a/java/ql/test/query-tests/UnreleasedLock/UnreleasedLock.java b/java/ql/test/query-tests/UnreleasedLock/UnreleasedLock.java index 732ba7cd325d..5b715f57ae3e 100644 --- a/java/ql/test/query-tests/UnreleasedLock/UnreleasedLock.java +++ b/java/ql/test/query-tests/UnreleasedLock/UnreleasedLock.java @@ -5,18 +5,18 @@ void lock() throws RuntimeException { } void unlock() { } boolean isHeldByCurrentThread() { return true; } } - + void f() throws RuntimeException { } void g() throws RuntimeException { } - + MyLock mylock = new MyLock(); - + void bad1() { mylock.lock(); f(); mylock.unlock(); } - + void good2() { mylock.lock(); try { @@ -25,7 +25,7 @@ void good2() { mylock.unlock(); } } - + void bad3() { mylock.lock(); f(); @@ -35,7 +35,7 @@ void bad3() { mylock.unlock(); } } - + void bad4() { mylock.lock(); try { @@ -45,7 +45,7 @@ void bad4() { mylock.unlock(); } } - + void bad5(boolean lockmore) { mylock.lock(); try { @@ -58,7 +58,7 @@ void bad5(boolean lockmore) { mylock.unlock(); } } - + void good6() { if (!mylock.tryLock()) { return; } try { @@ -67,7 +67,7 @@ void good6() { mylock.unlock(); } } - + void bad7() { if (!mylock.tryLock()) { return; } f(); @@ -95,4 +95,28 @@ void good8() { mylock.unlock(); } } + + void good9() { + boolean locked = mylock.tryLock(); + if (!locked) { return; } + try { + f(); + } finally { + if (locked) { + mylock.unlock(); + } + } + } + + void good10() { + boolean locked = false; + try { + locked = mylock.tryLock(); + if (!locked) { return; } + } finally { + if (locked) { + mylock.unlock(); + } + } + } }