Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DRAFT] Java: handle lock state check stored in variable for java/unreleased-lock #18900

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion java/ql/src/Likely Bugs/Concurrency/UnreleasedLock.ql
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,26 @@
)
}

/**
* 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.
*/
Comment on lines +121 to +126

Check warning

Code scanning / CodeQL

Predicate QLDoc style. Warning

The QLDoc for a predicate without a result should start with 'Holds'.
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`.
Expand All @@ -131,8 +151,9 @@
// 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
(
Expand Down
42 changes: 33 additions & 9 deletions java/ql/test/query-tests/UnreleasedLock/UnreleasedLock.java
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -25,7 +25,7 @@ void good2() {
mylock.unlock();
}
}

void bad3() {
mylock.lock();
f();
Expand All @@ -35,7 +35,7 @@ void bad3() {
mylock.unlock();
}
}

void bad4() {
mylock.lock();
try {
Expand All @@ -45,7 +45,7 @@ void bad4() {
mylock.unlock();
}
}

void bad5(boolean lockmore) {
mylock.lock();
try {
Expand All @@ -58,7 +58,7 @@ void bad5(boolean lockmore) {
mylock.unlock();
}
}

void good6() {
if (!mylock.tryLock()) { return; }
try {
Expand All @@ -67,7 +67,7 @@ void good6() {
mylock.unlock();
}
}

void bad7() {
if (!mylock.tryLock()) { return; }
f();
Expand Down Expand Up @@ -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();
}
}
}
}