Merge pull request #18900 from jcogs33/jcogs33/java/unreleased-lock-fp

Java: handle lock state check stored in variable for `java/unreleased-lock`

Author: jcogs33

java/ql/src/Likely Bugs/Concurrency/UnreleasedLock.ql

@@ -118,6 +118,26 @@ predicate heldByCurrentThreadCheck(LockType t, BasicBlock checkblock, BasicBlock
   )
 }
 
+/**
+ * Holds if there is 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 variableLockStateCheck(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 or of `variableLockStateCheck`.
     not heldByCurrentThreadCheck(t, pred, b) and
+    not variableLockStateCheck(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
     (

java/ql/src/change-notes/2025-03-02-unreleased-lock-fp.md

@@ -0,0 +1,4 @@
+---
+category: majorAnalysis
+---
+* Updated the `java/unreleased-lock` query so that it no longer report alerts in cases where a boolean variable is used to track lock state.

java/ql/test/query-tests/UnreleasedLock/UnreleasedLock.expected

@@ -3,3 +3,4 @@
 | UnreleasedLock.java:40:3:40:15 | lock(...) | This lock might not be unlocked or might be locked more times than it is unlocked. |
 | UnreleasedLock.java:50:3:50:15 | lock(...) | This lock might not be unlocked or might be locked more times than it is unlocked. |
 | UnreleasedLock.java:72:8:72:23 | tryLock(...) | This lock might not be unlocked or might be locked more times than it is unlocked. |
+| UnreleasedLock.java:114:13:114:28 | tryLock(...) | This lock might not be unlocked or might be locked more times than it is unlocked. |

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,29 @@ void good8() {
 			mylock.unlock();
 		}
 	}
+
+	void good9() {
+		boolean locked = false;
+		try {
+			locked = mylock.tryLock();
+			if (!locked) { return; }
+		} finally {
+			if (locked) {
+				mylock.unlock();
+			}
+		}
+	}
+
+	void bad10() {
+		boolean locked = false;
+		try {
+			locked = mylock.tryLock();
+			if (!locked) { return; }
+		} finally {
+			if (locked) {
+				g();
+				mylock.unlock();
+			}
+		}
+	}
 }