java: move shared code into Concurrency.qll

java/ql/lib/semmle/code/java/Concurrency.qll

@@ -2,6 +2,47 @@ overlay[local?]
 module;
 
 import java
+import semmle.code.java.frameworks.Mockito
+
+class LockType extends RefType {
+  LockType() {
+    this.getAMethod().hasName("lock") and
+    this.getAMethod().hasName("unlock")
+  }
+
+  Method getLockMethod() {
+    result.getDeclaringType() = this and
+    result.hasName(["lock", "lockInterruptibly", "tryLock"])
+  }
+
+  Method getUnlockMethod() {
+    result.getDeclaringType() = this and
+    result.hasName("unlock")
+  }
+
+  Method getIsHeldByCurrentThreadMethod() {
+    result.getDeclaringType() = this and
+    result.hasName("isHeldByCurrentThread")
+  }
+
+  MethodCall getLockAccess() {
+    result.getMethod() = this.getLockMethod() and
+    // Not part of a Mockito verification call
+    not result instanceof MockitoVerifiedMethodCall
+  }
+
+  MethodCall getUnlockAccess() {
+    result.getMethod() = this.getUnlockMethod() and
+    // Not part of a Mockito verification call
+    not result instanceof MockitoVerifiedMethodCall
+  }
+
+  MethodCall getIsHeldByCurrentThreadAccess() {
+    result.getMethod() = this.getIsHeldByCurrentThreadMethod() and
+    // Not part of a Mockito verification call
+    not result instanceof MockitoVerifiedMethodCall
+  }
+}
 
 /**
  * Holds if `e` is synchronized by a local synchronized statement `sync` on the variable `v`.
@@ -49,3 +90,123 @@ class SynchronizedCallable extends Callable {
     )
   }
 }
+
+/**
+ * This module provides predicates, chiefly `locallyMonitors`, to check if a given expression is synchronized on a specific monitor.
+ */
+module Monitors {
+  /**
+   * A monitor is any object that is used to synchronize access to a shared resource.
+   * This includes locks as well as variables used in synchronized blocks (including `this`).
+   */
+  newtype TMonitor =
+    /** Either a lock or a variable used in a synchronized block. */
+    TVariableMonitor(Variable v) {
+      v.getType() instanceof LockType or locallySynchronizedOn(_, _, v)
+    } or
+    /** An instance reference used as a monitor. */
+    TInstanceMonitor(RefType thisType) { locallySynchronizedOnThis(_, thisType) } or
+    /** A class used as a monitor. */
+    TClassMonitor(RefType classType) { locallySynchronizedOnClass(_, classType) }
+
+  /**
+   * A monitor is any object that is used to synchronize access to a shared resource.
+   * This includes locks as well as variables used in synchronized blocks (including `this`).
+   */
+  class Monitor extends TMonitor {
+    /** Gets the location of this monitor. */
+    abstract Location getLocation();
+
+    /** Gets a textual representation of this element. */
+    abstract string toString();
+  }
+
+  /**
+   * A variable used as a monitor.
+   * The variable is either a lock or is used in a synchronized block.
+   * E.g `synchronized (m) { ... }` or `m.lock();`
+   */
+  class VariableMonitor extends Monitor, TVariableMonitor {
+    override Location getLocation() { result = this.getVariable().getLocation() }
+
+    override string toString() { result = "VariableMonitor(" + this.getVariable().toString() + ")" }
+
+    /** Gets the variable being used as a monitor. */
+    Variable getVariable() { this = TVariableMonitor(result) }
+  }
+
+  /**
+   * An instance reference used as a monitor.
+   * Either via `synchronized (this) { ... }` or by marking a non-static method as `synchronized`.
+   */
+  class InstanceMonitor extends Monitor, TInstanceMonitor {
+    override Location getLocation() { result = this.getThisType().getLocation() }
+
+    override string toString() { result = "InstanceMonitor(" + this.getThisType().toString() + ")" }
+
+    /** Gets the instance reference being used as a monitor. */
+    RefType getThisType() { this = TInstanceMonitor(result) }
+  }
+
+  /**
+   * A class used as a monitor.
+   * This is achieved by marking a static method as `synchronized`.
+   */
+  class ClassMonitor extends Monitor, TClassMonitor {
+    override Location getLocation() { result = this.getClassType().getLocation() }
+
+    override string toString() { result = "ClassMonitor(" + this.getClassType().toString() + ")" }
+
+    /** Gets the class being used as a monitor. */
+    RefType getClassType() { this = TClassMonitor(result) }
+  }
+
+  /** Holds if the expression `e` is synchronized on the monitor `m`. */
+  predicate locallyMonitors(Expr e, Monitor m) {
+    exists(Variable v | v = m.(VariableMonitor).getVariable() |
+      locallyLockedOn(e, v)
+      or
+      locallySynchronizedOn(e, _, v)
+    )
+    or
+    locallySynchronizedOnThis(e, m.(InstanceMonitor).getThisType())
+    or
+    locallySynchronizedOnClass(e, m.(ClassMonitor).getClassType())
+  }
+
+  /** Holds if `localLock` refers to `lock`. */
+  predicate represents(Field lock, Variable localLock) {
+    lock.getType() instanceof LockType and
+    (
+      localLock = lock
+      or
+      localLock.getInitializer() = lock.getAnAccess()
+    )
+  }
+
+  /** Gets the control flow node that must dominate `e` when `e` is synchronized on a lock. */
+  ControlFlowNode getNodeToBeDominated(Expr e) {
+    // If `e` is the LHS of an assignment, use the control flow node for the assignment
+    exists(Assignment asgn | asgn.getDest() = e | result = asgn.getControlFlowNode())
+    or
+    // if `e` is not the LHS of an assignment, use the default control flow node
+    not exists(Assignment asgn | asgn.getDest() = e) and
+    result = e.getControlFlowNode()
+  }
+
+  /** Holds if `e` is synchronized on the `Lock` `lock` by a locking call. */
+  predicate locallyLockedOn(Expr e, Field lock) {
+    lock.getType() instanceof LockType and
+    exists(Variable localLock, MethodCall lockCall, MethodCall unlockCall |
+      represents(lock, localLock) and
+      lockCall.getQualifier() = localLock.getAnAccess() and
+      lockCall.getMethod() = lock.getType().(LockType).getLockMethod() and
+      unlockCall.getQualifier() = localLock.getAnAccess() and
+      unlockCall.getMethod() = lock.getType().(LockType).getUnlockMethod()
+    |
+      dominates(lockCall.getControlFlowNode(), unlockCall.getControlFlowNode()) and
+      dominates(lockCall.getControlFlowNode(), getNodeToBeDominated(e)) and
+      postDominates(unlockCall.getControlFlowNode(), getNodeToBeDominated(e))
+    )
+  }
+}

java/ql/lib/semmle/code/java/ConflictingAccess.qll

@@ -7,133 +7,6 @@ module;
 import java
 import Concurrency
 
-/**
- * Holds if `t` is the type of a lock.
- * Currently a crude test of the type name.
- */
-bindingset[t]
-overlay[caller?]
-pragma[inline_late]
-predicate isLockType(Type t) { t.getName().matches("%Lock%") }
-
-/**
- * This module provides predicates, chiefly `locallyMonitors`, to check if a given expression is synchronized on a specific monitor.
- */
-module Monitors {
-  /**
-   * A monitor is any object that is used to synchronize access to a shared resource.
-   * This includes locks as well as variables used in synchronized blocks (including `this`).
-   */
-  newtype TMonitor =
-    /** Either a lock or a variable used in a synchronized block. */
-    TVariableMonitor(Variable v) { isLockType(v.getType()) or locallySynchronizedOn(_, _, v) } or
-    /** An instance reference used as a monitor. */
-    TInstanceMonitor(RefType thisType) { locallySynchronizedOnThis(_, thisType) } or
-    /** A class used as a monitor. */
-    TClassMonitor(RefType classType) { locallySynchronizedOnClass(_, classType) }
-
-  /**
-   * A monitor is any object that is used to synchronize access to a shared resource.
-   * This includes locks as well as variables used in synchronized blocks (including `this`).
-   */
-  class Monitor extends TMonitor {
-    /** Gets the location of this monitor. */
-    abstract Location getLocation();
-
-    /** Gets a textual representation of this element. */
-    abstract string toString();
-  }
-
-  /**
-   * A variable used as a monitor.
-   * The variable is either a lock or is used in a synchronized block.
-   * E.g `synchronized (m) { ... }` or `m.lock();`
-   */
-  class VariableMonitor extends Monitor, TVariableMonitor {
-    override Location getLocation() { result = this.getVariable().getLocation() }
-
-    override string toString() { result = "VariableMonitor(" + this.getVariable().toString() + ")" }
-
-    /** Gets the variable being used as a monitor. */
-    Variable getVariable() { this = TVariableMonitor(result) }
-  }
-
-  /**
-   * An instance reference used as a monitor.
-   * Either via `synchronized (this) { ... }` or by marking a non-static method as `synchronized`.
-   */
-  class InstanceMonitor extends Monitor, TInstanceMonitor {
-    override Location getLocation() { result = this.getThisType().getLocation() }
-
-    override string toString() { result = "InstanceMonitor(" + this.getThisType().toString() + ")" }
-
-    /** Gets the instance reference being used as a monitor. */
-    RefType getThisType() { this = TInstanceMonitor(result) }
-  }
-
-  /**
-   * A class used as a monitor.
-   * This is achieved by marking a static method as `synchronized`.
-   */
-  class ClassMonitor extends Monitor, TClassMonitor {
-    override Location getLocation() { result = this.getClassType().getLocation() }
-
-    override string toString() { result = "ClassMonitor(" + this.getClassType().toString() + ")" }
-
-    /** Gets the class being used as a monitor. */
-    RefType getClassType() { this = TClassMonitor(result) }
-  }
-
-  /** Holds if the expression `e` is synchronized on the monitor `m`. */
-  predicate locallyMonitors(Expr e, Monitor m) {
-    exists(Variable v | v = m.(VariableMonitor).getVariable() |
-      locallyLockedOn(e, v)
-      or
-      locallySynchronizedOn(e, _, v)
-    )
-    or
-    locallySynchronizedOnThis(e, m.(InstanceMonitor).getThisType())
-    or
-    locallySynchronizedOnClass(e, m.(ClassMonitor).getClassType())
-  }
-
-  /** Holds if `localLock` refers to `lock`. */
-  predicate represents(Field lock, Variable localLock) {
-    isLockType(lock.getType()) and
-    (
-      localLock = lock
-      or
-      localLock.getInitializer() = lock.getAnAccess()
-    )
-  }
-
-  /** Gets the control flow node that must dominate `e` when `e` is synchronized on a lock. */
-  ControlFlowNode getNodeToBeDominated(Expr e) {
-    // If `e` is the LHS of an assignment, use the control flow node for the assignment
-    exists(Assignment asgn | asgn.getDest() = e | result = asgn.getControlFlowNode())
-    or
-    // if `e` is not the LHS of an assignment, use the default control flow node
-    not exists(Assignment asgn | asgn.getDest() = e) and
-    result = e.getControlFlowNode()
-  }
-
-  /** Holds if `e` is synchronized on the `Lock` `lock` by a locking call. */
-  predicate locallyLockedOn(Expr e, Field lock) {
-    isLockType(lock.getType()) and
-    exists(Variable localLock, MethodCall lockCall, MethodCall unlockCall |
-      represents(lock, localLock) and
-      lockCall.getQualifier() = localLock.getAnAccess() and
-      lockCall.getMethod().getName() in ["lock", "lockInterruptibly", "tryLock"] and
-      unlockCall.getQualifier() = localLock.getAnAccess() and
-      unlockCall.getMethod().getName() = "unlock"
-    |
-      dominates(lockCall.getControlFlowNode(), unlockCall.getControlFlowNode()) and
-      dominates(lockCall.getControlFlowNode(), getNodeToBeDominated(e)) and
-      postDominates(unlockCall.getControlFlowNode(), getNodeToBeDominated(e))
-    )
-  }
-}
-
 /** Provides predicates, chiefly `isModifying`, to check if a given expression modifies a shared resource. */
 module Modification {
   import semmle.code.java.dataflow.FlowSummary
@@ -183,7 +56,7 @@ class ExposedField extends Field {
     this.getDeclaringType() instanceof ClassAnnotatedAsThreadSafe and
     not this.isVolatile() and
     // field is not a lock
-    not isLockType(this.getType()) and
+    not this.getType() instanceof LockType and
     // field is not thread-safe
     not isThreadSafeType(this.getType()) and
     not isThreadSafeType(this.getInitializer().getType()) and

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

@@ -16,47 +16,7 @@
 import java
 import semmle.code.java.controlflow.Guards
 import semmle.code.java.dataflow.SSA
-import semmle.code.java.frameworks.Mockito
+import semmle.code.java.Concurrency
-
-class LockType extends RefType {
-  LockType() {
-    this.getAMethod().hasName("lock") and
-    this.getAMethod().hasName("unlock")
-  }
-
-  Method getLockMethod() {
-    result.getDeclaringType() = this and
-    (result.hasName("lock") or result.hasName("tryLock"))
-  }
-
-  Method getUnlockMethod() {
-    result.getDeclaringType() = this and
-    result.hasName("unlock")
-  }
-
-  Method getIsHeldByCurrentThreadMethod() {
-    result.getDeclaringType() = this and
-    result.hasName("isHeldByCurrentThread")
-  }
-
-  MethodCall getLockAccess() {
-    result.getMethod() = this.getLockMethod() and
-    // Not part of a Mockito verification call
-    not result instanceof MockitoVerifiedMethodCall
-  }
-
-  MethodCall getUnlockAccess() {
-    result.getMethod() = this.getUnlockMethod() and
-    // Not part of a Mockito verification call
-    not result instanceof MockitoVerifiedMethodCall
-  }
-
-  MethodCall getIsHeldByCurrentThreadAccess() {
-    result.getMethod() = this.getIsHeldByCurrentThreadMethod() and
-    // Not part of a Mockito verification call
-    not result instanceof MockitoVerifiedMethodCall
-  }
-}
 
 predicate lockBlock(LockType t, BasicBlock b, int locks) {
   locks = strictcount(int i | b.getNode(i).asExpr() = t.getLockAccess())