diff --git a/java/ql/src/Likely Bugs/Concurrency/SafePublication.java b/java/ql/src/Likely Bugs/Concurrency/SafePublication.java new file mode 100644 index 00000000000..64341017890 --- /dev/null +++ b/java/ql/src/Likely Bugs/Concurrency/SafePublication.java @@ -0,0 +1,11 @@ +public class SafePublication { + private Object value; + + public synchronized void produce() { + value = new Object(); // Safely published using synchronization + } + + public synchronized Object getValue() { + return value; + } +} \ No newline at end of file diff --git a/java/ql/src/Likely Bugs/Concurrency/SafePublication.qhelp b/java/ql/src/Likely Bugs/Concurrency/SafePublication.qhelp new file mode 100644 index 00000000000..a24977e6730 --- /dev/null +++ b/java/ql/src/Likely Bugs/Concurrency/SafePublication.qhelp @@ -0,0 +1,62 @@ + + + + + +

+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. +

+

+In particular, values of primitive types should not be initialised to anything but their default values (which for Object is null) unless this happens in a static context. +

+

+Techniques for safe publication include: +

+ + + + + +

+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 final 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 java.util.concurrent. +

+ + + + +

In the following example, the value of value is not safely published. The produce method + creates a new object and assigns it to the field value. However, the field is not + declared as volatile, and there are no synchronization mechanisms in place to ensure + that the value is fully constructed before it is published.

+ + + +

To fix this example, declare the field value as volatile, or use + synchronized blocks or methods to ensure that the value is fully constructed before it is + published. We illustrate the latter with the following example:

+ + + + + + + +
  • + Java Language Specification, chapter 17: + Threads and Locks. +
    • +
  • + Java concurrency package: + java.util.concurrent. +
    • + + +     +     diff --git a/java/ql/src/Likely Bugs/Concurrency/SafePublication.ql b/java/ql/src/Likely Bugs/Concurrency/SafePublication.ql new file mode 100644 index 00000000000..08cd3d5a577 --- /dev/null +++ b/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 // TODO: Consider non-primitive types + 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() diff --git a/java/ql/src/Likely Bugs/Concurrency/UnsafePublication.java b/java/ql/src/Likely Bugs/Concurrency/UnsafePublication.java new file mode 100644 index 00000000000..ddf8c8b400f --- /dev/null +++ b/java/ql/src/Likely Bugs/Concurrency/UnsafePublication.java @@ -0,0 +1,12 @@ +@ThreadSafe +public class UnsafePublication { + private Object value; + + public void produce() { + value = new Object(); // Not safely published, other threads may see the default value null + } + + public Object getValue() { + return value; + } +} \ No newline at end of file diff --git a/java/ql/src/change-notes/2025-06-22-query-safe-publication.md b/java/ql/src/change-notes/2025-06-22-query-safe-publication.md new file mode 100644 index 00000000000..23b64c970b3 --- /dev/null +++ b/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`. \ No newline at end of file diff --git a/java/ql/test/query-tests/SafePublication/SafePublication.expected b/java/ql/test/query-tests/SafePublication/SafePublication.expected new file mode 100644 index 00000000000..fbb54ff7b8c --- /dev/null +++ b/java/ql/test/query-tests/SafePublication/SafePublication.expected @@ -0,0 +1,7 @@ +| SafePublication.java:5:9:5:9 | z | The class $@ is marked as thread-safe, but this field is not safely published. | SafePublication.java:2:14:2:28 | SafePublication | SafePublication | +| SafePublication.java:6:9:6:9 | w | The class $@ is marked as thread-safe, but this field is not safely published. | SafePublication.java:2:14:2:28 | SafePublication | SafePublication | +| SafePublication.java:7:9:7:9 | u | The class $@ is marked as thread-safe, but this field is not safely published. | SafePublication.java:2:14:2:28 | SafePublication | SafePublication | +| SafePublication.java:11:10:11:10 | d | The class $@ is marked as thread-safe, but this field is not safely published. | SafePublication.java:2:14:2:28 | SafePublication | SafePublication | +| SafePublication.java:12:10:12:10 | e | The class $@ is marked as thread-safe, but this field is not safely published. | SafePublication.java:2:14:2:28 | SafePublication | SafePublication | +| SafePublication.java:14:11:14:13 | arr | The class $@ is marked as thread-safe, but this field is not safely published. | SafePublication.java:2:14:2:28 | SafePublication | SafePublication | +| SafePublication.java:17:10:17:11 | cc | The class $@ is marked as thread-safe, but this field is not safely published. | SafePublication.java:2:14:2:28 | SafePublication | SafePublication | diff --git a/java/ql/test/query-tests/SafePublication/SafePublication.java b/java/ql/test/query-tests/SafePublication/SafePublication.java new file mode 100644 index 00000000000..9c1d031987b --- /dev/null +++ b/java/ql/test/query-tests/SafePublication/SafePublication.java @@ -0,0 +1,29 @@ +@ThreadSafe +public class SafePublication { + int x; + int y = 0; + int z = 3; //$ Alert + int w; //$ Alert + int u; //$ Alert + long a; + long b = 0; + long c = 0L; + long d = 3; //$ Alert + long e = 3L; //$ Alert + + int[] arr = new int[3]; //$ Alert + float f = 0.0f; + double dd = 00.0d; + char cc = 'a'; //$ Alert + char ok = '\u0000'; + + public SafePublication(int a) { + x = 0; + w = 3; // not ok + u = a; // not ok + } + + public void methodLocal() { + int i; + } +} \ No newline at end of file diff --git a/java/ql/test/query-tests/SafePublication/SafePublication.qlref b/java/ql/test/query-tests/SafePublication/SafePublication.qlref new file mode 100644 index 00000000000..51bf2ced966 --- /dev/null +++ b/java/ql/test/query-tests/SafePublication/SafePublication.qlref @@ -0,0 +1,2 @@ +query: Likely Bugs/Concurrency/SafePublication.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql \ No newline at end of file diff --git a/java/ql/test/query-tests/SafePublication/ThreadSafe.java b/java/ql/test/query-tests/SafePublication/ThreadSafe.java new file mode 100644 index 00000000000..1a4534cc78f --- /dev/null +++ b/java/ql/test/query-tests/SafePublication/ThreadSafe.java @@ -0,0 +1,2 @@ +public @interface ThreadSafe { +} \ No newline at end of file