Merge pull request #19539 from yoff/java/conflicting-access

java/ql/src/Likely Bugs/Concurrency/Escaping.qhelp

@@ -0,0 +1,34 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>
+In a thread-safe class, non-final fields should generally be private (or possibly volatile) to ensure that they cannot be accessed by other threads in an unsafe manner.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+If the field does not change, mark it as <code>final</code>. If the field is mutable, mark it as <code>private</code> and provide properly synchronized accessors.</p>
+
+</recommendation>
+
+<references>
+
+
+<li>
+  Java Language Specification, chapter 17:
+  <a href="https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.4">Threads and Locks</a>.
+</li>
+<li>
+  Java concurrency package:
+  <a href="https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/package-summary.html">java.util.concurrent</a>.
+</li>
+
+
+</references>
+</qhelp>

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

@@ -0,0 +1,26 @@
+/**
+ * @name Escaping
+ * @description In a thread-safe class, care should be taken to avoid exposing mutable state.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id java/escaping
+ * @tags quality
+ *       reliability
+ *       concurrency
+ */
+
+import java
+import semmle.code.java.ConflictingAccess
+
+from Field f, ClassAnnotatedAsThreadSafe c
+where
+  f = c.getAField() and
+  not f.isFinal() and // final fields do not change
+  not f.isPrivate() and
+  // We believe that protected fields are also dangerous
+  // Volatile fields cannot cause data races, but it is dubious to allow changes.
+  // For now, we ignore volatile fields, but there are likely bugs to be caught here.
+  not f.isVolatile()
+select f, "The class $@ is marked as thread-safe, but this field is potentially escaping.", c,
+  c.getName()

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

@@ -0,0 +1,17 @@
+public class SafePublication {
+    private volatile Object value;
+    private final int server_id;
+
+    public SafePublication() {
+        value = new Object(); // Safely published as volatile
+        server_id = 1; // Safely published as final
+    }
+
+    public synchronized Object getValue() {
+        return value;
+    }
+
+    public int getServerId() {
+        return server_id;
+    }
+}

java/ql/src/Likely Bugs/Concurrency/SafePublication.qhelp

@@ -0,0 +1,57 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>
+In a thread-safe class, values must be published safely to avoid inconsistent or unexpected behavior caused by visibility issues between threads. If a value is not safely published, one thread may see a stale or partially constructed value written by another thread, leading to subtle concurrency bugs.
+</p>
+<p>
+In particular, values of primitive types should not be initialised to anything but their default values (which for <code>Object</code> is <code>null</code>) unless this happens in a static context.
+</p>
+<p>
+Techniques for safe publication include:
+</p>
+<ul>
+<li>Using synchronized blocks or methods to ensure that a value is fully constructed before it is published.</li>
+<li>Using volatile fields to ensure visibility of changes across threads.</li>
+<li>Using thread-safe collections or classes that provide built-in synchronization, such as are found in <code>java.util.concurrent</code>.</li>
+<li>Using the <code>final</code> keyword to ensure that a reference to an object is safely published when the object is constructed.</li>
+</ul>
+
+</overview>
+<recommendation>
+
+<p>
+Choose a safe publication technique that fits your use case. If the value only needs to be written once, say for a singleton, consider using the <code>final</code> keyword. If the value is mutable and needs to be shared across threads, consider using synchronized blocks or methods, or using a thread-safe collection from <code>java.util.concurrent</code>.
+</p>
+
+</recommendation>
+<example>
+
+<p>In the following example, the values of <code>value</code> and <code>server_id</code> are not safely published. The constructor creates a new object and assigns it to the field <code>value</code>. However, the field is not declared as <code>volatile</code> or <code>final</code>, and there are no synchronization mechanisms in place to ensure that the value is fully constructed before it is published. A different thread may see the default value <code>null</code>. Similarly, the field <code>server_id</code> may be observed to be <code>0</code>.</p>
+
+<sample src="UnsafePublication.java" />
+
+<p>To fix this example, we declare the field <code>value</code> as volatile. This will ensure that all changes to the field are visible to all threads. The field <code>server_id</code> is only meant to be written once, so we only need the write inside the constructor to be visible to other threads; declaring it <code>final</code> guarantees this:</p>
+
+<sample src="SafePublication.java" />
+
+</example>
+<references>
+
+
+<li>
+  Java Language Specification, chapter 17:
+  <a href="https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.4">Threads and Locks</a>.
+</li>
+<li>
+  Java concurrency package:
+  <a href="https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/package-summary.html">java.util.concurrent</a>.
+</li>
+
+
+</references>
+</qhelp>

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

@@ -0,0 +1,93 @@
+/**
+ * @name Safe publication
+ * @description A field of a thread-safe class is not safely published.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id java/safe-publication
+ * @tags quality
+ *       reliability
+ *       concurrency
+ */
+
+import java
+import semmle.code.java.ConflictingAccess
+
+/**
+ * Holds if `v` should be the default value for the field `f`.
+ * That is, `v` is an initial (or constructor) assignment of `f`.
+ */
+predicate shouldBeDefaultValueFor(Field f, Expr v) {
+  v = f.getAnAssignedValue() and
+  (
+    v = f.getInitializer()
+    or
+    v.getEnclosingCallable() = f.getDeclaringType().getAConstructor()
+  )
+}
+
+/**
+ * Gets the default value for the field `f`.
+ * See https://docs.oracle.com/javase/tutorial/java/nutsandbolts/datatypes.html
+ * for the default values of the primitive types.
+ * The default value for non-primitive types is null.
+ */
+bindingset[result]
+Expr getDefaultValue(Field f) {
+  f.getType().hasName("byte") and result.(IntegerLiteral).getIntValue() = 0
+  or
+  f.getType().hasName("short") and result.(IntegerLiteral).getIntValue() = 0
+  or
+  f.getType().hasName("int") and result.(IntegerLiteral).getIntValue() = 0
+  or
+  f.getType().hasName("long") and
+  (
+    result.(LongLiteral).getValue() = "0" or
+    result.(IntegerLiteral).getValue() = "0"
+  )
+  or
+  f.getType().hasName("float") and result.(FloatLiteral).getValue() = "0.0"
+  or
+  f.getType().hasName("double") and result.(DoubleLiteral).getValue() = "0.0"
+  or
+  f.getType().hasName("char") and result.(CharacterLiteral).getCodePointValue() = 0
+  or
+  f.getType().hasName("boolean") and result.(BooleanLiteral).getBooleanValue() = false
+  or
+  not f.getType().getName() in [
+      "byte", "short", "int", "long", "float", "double", "char", "boolean"
+    ] and
+  result instanceof NullLiteral
+}
+
+/**
+ * Holds if all constructor or initial assignments (if any) are to the default value.
+ * That is, assignments by the declaration:
+ *   int x = 0; OK
+ *   int x = 3; not OK
+ * or inside a constructor:
+ *   public c(a) {
+ *     x = 0; OK
+ *     x = 3; not OK
+ *     x = a; not OK
+ *   }
+ */
+predicate isAssignedDefaultValue(Field f) {
+  forall(Expr v | shouldBeDefaultValueFor(f, v) | v = getDefaultValue(f))
+}
+
+predicate isSafelyPublished(Field f) {
+  f.isFinal() or // NOTE: For non-primitive types, 'final' alone does not guarantee safe publication unless the object is immutable or safely constructed. Consider reviewing the handling of non-primitive fields for safe publication.
+  f.isStatic() or
+  f.isVolatile() or
+  isThreadSafeType(f.getType()) or
+  isThreadSafeType(f.getInitializer().getType()) or
+  isAssignedDefaultValue(f)
+}
+
+from Field f, ClassAnnotatedAsThreadSafe c
+where
+  f = c.getAField() and
+  not isSafelyPublished(f)
+select f, "The class $@ is marked as thread-safe, but this field is not safely published.", c,
+  c.getName()

java/ql/src/Likely Bugs/Concurrency/ThreadSafe.qhelp

@@ -0,0 +1,33 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>
+In a thread-safe class, all field accesses that can be caused by calls to public methods must be properly synchronized.</p>
+
+</overview>
+<recommendation>
+
+<p>
+Protect the field access with a lock. Alternatively mark the field as <code>volatile</code> if the write operation is atomic. You can also choose to use a data type that guarantees atomic access. If the field is immutable, mark it as <code>final</code>.</p>
+
+</recommendation>
+
+<references>
+
+
+<li>
+  Java Language Specification, chapter 17:
+  <a href="https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.4">Threads and Locks</a>.
+</li>
+<li>
+  Java concurrency package:
+  <a href="https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/package-summary.html">java.util.concurrent</a>.
+</li>
+
+
+</references>
+</qhelp>

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

@@ -0,0 +1,51 @@
+/**
+ * @name Not thread-safe
+ * @description This class is not thread-safe. It is annotated as `@ThreadSafe`, but it has a
+ *              conflicting access to a field that is not synchronized with the same monitor.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id java/not-threadsafe
+ * @tags quality
+ *       reliability
+ *       concurrency
+ */
+
+import java
+import semmle.code.java.ConflictingAccess
+
+predicate unmonitoredAccess(ExposedFieldAccess a, string msg, Expr entry, string entry_desc) {
+  exists(ClassAnnotatedAsThreadSafe cls, ExposedField f |
+    cls.unlockedPublicAccess(f, entry, _, a, true)
+    or
+    cls.unlockedPublicAccess(f, entry, _, a, false) and
+    cls.hasPublicWriteAccess(f)
+  ) and
+  msg =
+    "This field access (publicly accessible via $@) is not protected by any monitor, but the class is annotated as @ThreadSafe." and
+  entry_desc = "this expression"
+}
+
+predicate notFullyMonitoredField(
+  ExposedField f, string msg, ClassAnnotatedAsThreadSafe cls, string cls_name
+) {
+  (
+    // Technically there has to be a write access for a conflict to exist.
+    // But if you are locking your reads with different locks, you likely made a typo,
+    // so in this case we alert without requiring `cls.has_public_write_access(f)`
+    cls.singleMonitorMismatch(f)
+    or
+    cls.notFullyMonitored(f) and
+    cls.hasPublicWriteAccess(f)
+  ) and
+  msg =
+    "This field is not properly synchronized in that no single monitor covers all accesses, but the class $@ is annotated as @ThreadSafe." and
+  cls_name = cls.getName()
+}
+
+from Top alert_element, Top alert_context, string alert_msg, string context_desc
+where
+  unmonitoredAccess(alert_element, alert_msg, alert_context, context_desc)
+  or
+  notFullyMonitoredField(alert_element, alert_msg, alert_context, context_desc)
+select alert_element, alert_msg, alert_context, context_desc

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
-
-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
-  }
-}
+import semmle.code.java.Concurrency
 
 predicate lockBlock(LockType t, BasicBlock b, int locks) {
   locks = strictcount(int i | b.getNode(i).asExpr() = t.getLockAccess())

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

@@ -0,0 +1,17 @@
+public class UnsafePublication {
+    private Object value;
+    private int server_id;
+
+    public UnsafePublication() {
+        value = new Object(); // Not safely published, other threads may see the default value null
+        server_id = 1; // Not safely published, other threads may see the default value 0
+    }
+
+    public Object getValue() {
+        return value;
+    }
+
+    public int getServerId() {
+        return server_id;
+    }
+}

java/ql/src/change-notes/2025-06-22-query-escaping.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `java/escaping`, to detect values escaping from classes marked as `@ThreadSafe`.

java/ql/src/change-notes/2025-06-22-query-not-thread-safe.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `java/not-threadsafe`, to detect data races in classes marked as `@ThreadSafe`.

java/ql/src/change-notes/2025-06-22-query-safe-publication.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `java/safe-publication`, to detect unsafe publication in classes marked as `@ThreadSafe`.