Skip to content

Commit 2b19835

Browse files
yoffyoff
committed
java: add SafePublication query (P2)
1 parent 77acc7a commit 2b19835

8 files changed

Lines changed: 213 additions & 0 deletions

File tree

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
public class SafePublication {
2+
private Object value;
3+
4+
public synchronized void produce() {
5+
value = new Object(); // Safely published using synchronization
6+
}
7+
8+
public synchronized Object getValue() {
9+
return value;
10+
}
11+
}

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

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
7+
<overview>
8+
<p>
9+
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.
10+
</p>
11+
<p>
12+
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.
13+
</p>
14+
<p>
15+
Techniques for safe publication include:
16+
</p>
17+
<ul>
18+
<li>Using synchronized blocks or methods to ensure that a value is fully constructed before it is published.</li>
19+
<li>Using volatile fields to ensure visibility of changes across threads.</li>
20+
<li>Using thread-safe collections or classes that provide built-in synchronization, such as are found in <code>java.util.concurrent</code>.</li>
21+
<li>Using the <code>final</code> keyword to ensure that a reference to an object is safely published when the object is constructed.</li>
22+
</ul>
23+
24+
</overview>
25+
<recommendation>
26+
27+
<p>
28+
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>.
29+
</p>
30+
31+
</recommendation>
32+
<example>
33+
34+
<p>In the following example, the value of <code>value</code> is not safely published. The <code>produce</code> method
35+
creates a new object and assigns it to the field <code>value</code>. However, the field is not
36+
declared as <code>volatile</code>, and there are no synchronization mechanisms in place to ensure
37+
that the value is fully constructed before it is published.</p>
38+
39+
<sample src="UnsafePublication.java" />
40+
41+
<p>To fix this example, declare the field <code>value</code> as <code>volatile</code>, or use
42+
synchronized blocks or methods to ensure that the value is fully constructed before it is
43+
published. We illustrate the latter with the following example:</p>
44+
45+
<sample src="SafePublication.java" />
46+
47+
</example>
48+
<references>
49+
50+
51+
<li>
52+
Java Language Specification, chapter 17:
53+
<a href="https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.4">Threads and Locks</a>.
54+
</li>
55+
<li>
56+
Java concurrency package:
57+
<a href="https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/package-summary.html">java.util.concurrent</a>.
58+
</li>
59+
60+
61+
</references>
62+
</qhelp>

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

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
/**
2+
* @name Safe publication
3+
* @kind problem
4+
* @problem.severity error
5+
* @id java/safe-publication
6+
*/
7+
8+
import java
9+
import semmle.code.java.ConflictingAccess
10+
11+
/**
12+
* Holds if `v` should be the default value for the field `f`.
13+
* That is, `v` is an initial (or constructor) assignment of `f`.
14+
*/
15+
predicate shouldBeDefaultValueFor(Field f, Expr v) {
16+
v = f.getAnAssignedValue() and
17+
(
18+
v = f.getInitializer()
19+
or
20+
v.getEnclosingCallable() = f.getDeclaringType().getAConstructor()
21+
)
22+
}
23+
24+
/**
25+
* Gets the default value for the field `f`.
26+
* See https://docs.oracle.com/javase/tutorial/java/nutsandbolts/datatypes.html
27+
* for the default values of the primitive types.
28+
* The default value for non-primitive types is null.
29+
*/
30+
bindingset[result]
31+
Expr getDefaultValue(Field f) {
32+
f.getType().hasName("byte") and result.(IntegerLiteral).getIntValue() = 0
33+
or
34+
f.getType().hasName("short") and result.(IntegerLiteral).getIntValue() = 0
35+
or
36+
f.getType().hasName("int") and result.(IntegerLiteral).getIntValue() = 0
37+
or
38+
f.getType().hasName("long") and
39+
(
40+
result.(LongLiteral).getValue() = "0" or
41+
result.(IntegerLiteral).getValue() = "0"
42+
)
43+
or
44+
f.getType().hasName("float") and result.(FloatLiteral).getValue() = "0.0"
45+
or
46+
f.getType().hasName("double") and result.(DoubleLiteral).getValue() = "0.0"
47+
or
48+
f.getType().hasName("char") and result.(CharacterLiteral).getCodePointValue() = 0
49+
or
50+
f.getType().hasName("boolean") and result.(BooleanLiteral).getBooleanValue() = false
51+
or
52+
not f.getType().getName() in [
53+
"byte", "short", "int", "long", "float", "double", "char", "boolean"
54+
] and
55+
result instanceof NullLiteral
56+
}
57+
58+
/**
59+
* Holds if all constructor or initial assignments (if any) are to the default value.
60+
* That is, assignments by the declaration:
61+
* int x = 0; OK
62+
* int x = 3; not OK
63+
* or inside a constructor:
64+
* public c(a) {
65+
* x = 0; OK
66+
* x = 3; not OK
67+
* x = a; not OK
68+
* }
69+
*/
70+
predicate isAssignedDefaultValue(Field f) {
71+
forall(Expr v | shouldBeDefaultValueFor(f, v) | v = getDefaultValue(f))
72+
}
73+
74+
predicate isSafelyPublished(Field f) {
75+
f.isFinal() or // TODO: Consider non-primitive types
76+
f.isStatic() or
77+
f.isVolatile() or
78+
isThreadSafeType(f.getType()) or
79+
isThreadSafeType(f.getInitializer().getType()) or
80+
isAssignedDefaultValue(f)
81+
}
82+
83+
from Field f, ClassAnnotatedAsThreadSafe c
84+
where
85+
f = c.getAField() and
86+
not isSafelyPublished(f)
87+
select f, "The class $@ is marked as thread-safe, but this field is not safely published.", c,
88+
c.getName()

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
@ThreadSafe
2+
public class UnsafePublication {
3+
private Object value;
4+
5+
public void produce() {
6+
value = new Object(); // Not safely published, other threads may see the default value null
7+
}
8+
9+
public Object getValue() {
10+
return value;
11+
}
12+
}

‎java/ql/test/query-tests/SafePublication/SafePublication.expected‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
| 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 |
2+
| 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 |
3+
| 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 |
4+
| 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 |
5+
| 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 |
6+
| 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 |
7+
| 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 |

‎java/ql/test/query-tests/SafePublication/SafePublication.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
@ThreadSafe
2+
public class SafePublication {
3+
int x;
4+
int y = 0;
5+
int z = 3; //$ Alert
6+
int w; //$ Alert
7+
int u; //$ Alert
8+
long a;
9+
long b = 0;
10+
long c = 0L;
11+
long d = 3; //$ Alert
12+
long e = 3L; //$ Alert
13+
14+
int[] arr = new int[3]; //$ Alert
15+
float f = 0.0f;
16+
double dd = 00.0d;
17+
char cc = 'a'; //$ Alert
18+
char ok = '\u0000';
19+
20+
public SafePublication(int a) {
21+
x = 0;
22+
w = 3; // not ok
23+
u = a; // not ok
24+
}
25+
26+
public void methodLocal() {
27+
int i;
28+
}
29+
}

‎java/ql/test/query-tests/SafePublication/SafePublication.qlref‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
query: Likely Bugs/Concurrency/SafePublication.ql
2+
postprocess: utils/test/InlineExpectationsTestQuery.ql

‎java/ql/test/query-tests/SafePublication/ThreadSafe.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
public @interface ThreadSafe {
2+
}

0 commit comments

Comments
 (0)