mirror of
https://github.com/github/codeql.git
synced 2025-12-16 16:53:25 +01:00
java: add SafePublication query (P2)
This commit is contained in:
11
java/ql/src/Likely Bugs/Concurrency/SafePublication.java
Normal file
11
java/ql/src/Likely Bugs/Concurrency/SafePublication.java
Normal file
@@ -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;
|
||||||
|
}
|
||||||
|
}
|
||||||
62
java/ql/src/Likely Bugs/Concurrency/SafePublication.qhelp
Normal file
62
java/ql/src/Likely Bugs/Concurrency/SafePublication.qhelp
Normal file
@@ -0,0 +1,62 @@
|
|||||||
|
<!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 value of <code>value</code> is not safely published. The <code>produce</code> method
|
||||||
|
creates a new object and assigns it to the field <code>value</code>. However, the field is not
|
||||||
|
declared as <code>volatile</code>, and there are no synchronization mechanisms in place to ensure
|
||||||
|
that the value is fully constructed before it is published.</p>
|
||||||
|
|
||||||
|
<sample src="UnsafePublication.java" />
|
||||||
|
|
||||||
|
<p>To fix this example, declare the field <code>value</code> as <code>volatile</code>, 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:</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>
|
||||||
93
java/ql/src/Likely Bugs/Concurrency/SafePublication.ql
Normal file
93
java/ql/src/Likely Bugs/Concurrency/SafePublication.ql
Normal file
@@ -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()
|
||||||
12
java/ql/src/Likely Bugs/Concurrency/UnsafePublication.java
Normal file
12
java/ql/src/Likely Bugs/Concurrency/UnsafePublication.java
Normal file
@@ -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;
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -0,0 +1,4 @@
|
|||||||
|
---
|
||||||
|
category: newQuery
|
||||||
|
---
|
||||||
|
* Added a new query, `java/safe-publication`, to detect unsafe publication in classes marked as `@ThreadSafe`.
|
||||||
@@ -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 |
|
||||||
@@ -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;
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -0,0 +1,2 @@
|
|||||||
|
query: Likely Bugs/Concurrency/SafePublication.ql
|
||||||
|
postprocess: utils/test/InlineExpectationsTestQuery.ql
|
||||||
2
java/ql/test/query-tests/SafePublication/ThreadSafe.java
Normal file
2
java/ql/test/query-tests/SafePublication/ThreadSafe.java
Normal file
@@ -0,0 +1,2 @@
|
|||||||
|
public @interface ThreadSafe {
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user