Merge branch 'main' into cwe-925

java/ql/lib/semmle/code/java/dataflow/internal/DataFlowNodes.qll

@@ -36,14 +36,12 @@ module SsaFlow {
     TExplicitParameterNode(result.(Impl::ParameterNode).getParameter()) = n
   }
 
-  predicate localFlowStep(
-    SsaImpl::Impl::DefinitionExt def, Node nodeFrom, Node nodeTo, boolean isUseStep
-  ) {
-    Impl::localFlowStep(def, asNode(nodeFrom), asNode(nodeTo), isUseStep)
+  predicate localFlowStep(SsaSourceVariable v, Node nodeFrom, Node nodeTo, boolean isUseStep) {
+    Impl::localFlowStep(v, asNode(nodeFrom), asNode(nodeTo), isUseStep)
   }
 
-  predicate localMustFlowStep(SsaImpl::Impl::DefinitionExt def, Node nodeFrom, Node nodeTo) {
-    Impl::localMustFlowStep(def, asNode(nodeFrom), asNode(nodeTo))
+  predicate localMustFlowStep(Node nodeFrom, Node nodeTo) {
+    Impl::localMustFlowStep(_, asNode(nodeFrom), asNode(nodeTo))
   }
 }
 

java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll

@@ -168,7 +168,7 @@ predicate localMustFlowStep(Node node1, Node node2) {
       node2.(ImplicitInstanceAccess).getInstanceAccess().(OwnInstanceAccess).getEnclosingCallable()
   )
   or
-  SsaFlow::localMustFlowStep(_, node1, node2)
+  SsaFlow::localMustFlowStep(node1, node2)
   or
   node2.asExpr().(CastingExpr).getExpr() = node1.asExpr()
   or

java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll

@@ -544,15 +544,13 @@ private module Cached {
     import DataFlowIntegrationImpl
 
     cached
-    predicate localFlowStep(Impl::DefinitionExt def, Node nodeFrom, Node nodeTo, boolean isUseStep) {
-      not def instanceof UntrackedDef and
-      DataFlowIntegrationImpl::localFlowStep(def, nodeFrom, nodeTo, isUseStep)
+    predicate localFlowStep(TrackedVar v, Node nodeFrom, Node nodeTo, boolean isUseStep) {
+      DataFlowIntegrationImpl::localFlowStep(v, nodeFrom, nodeTo, isUseStep)
     }
 
     cached
-    predicate localMustFlowStep(Impl::DefinitionExt def, Node nodeFrom, Node nodeTo) {
-      not def instanceof UntrackedDef and
-      DataFlowIntegrationImpl::localMustFlowStep(def, nodeFrom, nodeTo)
+    predicate localMustFlowStep(TrackedVar v, Node nodeFrom, Node nodeTo) {
+      DataFlowIntegrationImpl::localMustFlowStep(v, nodeFrom, nodeTo)
     }
 
     signature predicate guardChecksSig(Guards::Guard g, Expr e, boolean branch);

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 @@ class Test {
 		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 @@ class Test {
 			mylock.unlock();
 		}
 	}
-	
+
 	void bad3() {
 		mylock.lock();
 		f();
@@ -35,7 +35,7 @@ class Test {
 			mylock.unlock();
 		}
 	}
-	
+
 	void bad4() {
 		mylock.lock();
 		try {
@@ -45,7 +45,7 @@ class Test {
 			mylock.unlock();
 		}
 	}
-	
+
 	void bad5(boolean lockmore) {
 		mylock.lock();
 		try {
@@ -58,7 +58,7 @@ class Test {
 			mylock.unlock();
 		}
 	}
-	
+
 	void good6() {
 		if (!mylock.tryLock()) { return; }
 		try {
@@ -67,7 +67,7 @@ class Test {
 			mylock.unlock();
 		}
 	}
-	
+
 	void bad7() {
 		if (!mylock.tryLock()) { return; }
 		f();
@@ -95,4 +95,29 @@ class Test {
 			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();
+			}
+		}
+	}
 }