Skip to content

Commit 0ba7633

Browse files
yh-semmleyh-semmle
authored
Merge pull request #553 from aschackmull/java/double-checked-locking
Java: Add two double-checked-locking queries.
2 parents b80cf30 + e836fa7 commit 0ba7633

15 files changed

Lines changed: 355 additions & 0 deletions

‎change-notes/1.20/analysis-java.md‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
# Improvements to Java analysis
2+
3+
## General improvements
4+
5+
6+
## New queries
7+
8+
| **Query** | **Tags** | **Purpose** |
9+
|-----------------------------|-----------|--------------------------------------------------------------------|
10+
| Double-checked locking is not thread-safe (`java/unsafe-double-checked-locking`) | reliability, correctness, concurrency, external/cwe/cwe-609 | Identifies wrong implementations of double-checked locking that does not use the `volatile` keyword. |
11+
| Race condition in double-checked locking object initialization (`java/unsafe-double-checked-locking-init-order`) | reliability, correctness, concurrency, external/cwe/cwe-609 | Identifies wrong implementations of double-checked locking that performs additional initialization after exposing the constructed object. |
12+
13+
## Changes to existing queries
14+
15+
| **Query** | **Expected impact** | **Change** |
16+
|----------------------------|------------------------|------------------------------------------------------------------|
17+
18+
## Changes to QL libraries
19+
20+

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<include src="DoubleCheckedLockingShared.qhelp" />
7+
8+
</qhelp>

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* @name Double-checked locking is not thread-safe
3+
* @description A repeated check on a non-volatile field is not thread-safe, and
4+
* could result in unexpected behavior.
5+
* @kind problem
6+
* @problem.severity error
7+
* @precision high
8+
* @id java/unsafe-double-checked-locking
9+
* @tags reliability
10+
* correctness
11+
* concurrency
12+
* external/cwe/cwe-609
13+
*/
14+
15+
import java
16+
import DoubleCheckedLocking
17+
18+
from IfStmt if1, IfStmt if2, SynchronizedStmt sync, Field f
19+
where
20+
doubleCheckedLocking(if1, if2, sync, f) and
21+
not f.isVolatile()
22+
select sync, "Double-checked locking on the non-volatile field $@ is not thread-safe.", f,
23+
f.toString()

‎java/ql/src/Likely Bugs/Concurrency/DoubleCheckedLocking.qll‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import java
2+
import semmle.code.java.dataflow.SSA
3+
4+
/**
5+
* Gets a read of `f`. Gets both direct reads and indirect reads through
6+
* assignment to a local variable.
7+
*/
8+
private Expr getAFieldRead(Field f) {
9+
result = f.getAnAccess()
10+
or
11+
exists(SsaExplicitUpdate v | v.getSourceVariable().getVariable() instanceof LocalScopeVariable |
12+
result = v.getAUse() and
13+
v.getDefiningExpr().(VariableAssign).getSource() = getAFieldRead(f)
14+
)
15+
or
16+
result.(ParExpr).getExpr() = getAFieldRead(f)
17+
or
18+
result.(AssignExpr).getSource() = getAFieldRead(f)
19+
}
20+
21+
/**
22+
* Gets an expression that corresponds to `f == null`, either directly testing
23+
* `f` or indirectly through a local variable `(x = f) == null`.
24+
*/
25+
private Expr getANullCheck(Field f) {
26+
exists(EqualityTest eq | eq.polarity() = true |
27+
eq.hasOperands(any(NullLiteral nl), getAFieldRead(f)) and
28+
result = eq
29+
)
30+
}
31+
32+
/**
33+
* Holds if the sequence `if1`-`sync`-`if2` corresponds to a double-checked
34+
* locking pattern for the field `f`. Fields with immutable types are excluded,
35+
* as they are always safe to initialize with double-checked locking.
36+
*/
37+
predicate doubleCheckedLocking(IfStmt if1, IfStmt if2, SynchronizedStmt sync, Field f) {
38+
if1.getThen() = sync.getParent*() and
39+
sync.getBlock() = if2.getParent*() and
40+
if1.getCondition() = getANullCheck(f) and
41+
if2.getCondition() = getANullCheck(f) and
42+
not f.getType() instanceof ImmutableType
43+
}

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
private Object lock = new Object();
2+
private MyObject f = null;
3+
4+
public MyObject getMyObject() {
5+
if (f == null) {
6+
synchronized(lock) {
7+
if (f == null) {
8+
f = new MyObject(); // BAD
9+
}
10+
}
11+
}
12+
return f;
13+
}

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
private Object lock = new Object();
2+
private volatile MyObject f = null;
3+
4+
public MyObject getMyObject() {
5+
if (f == null) {
6+
synchronized(lock) {
7+
if (f == null) {
8+
f = new MyObject();
9+
f.init(); // BAD
10+
}
11+
}
12+
}
13+
return f;
14+
}

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
private Object lock = new Object();
2+
private volatile MyObject f = null;
3+
4+
public MyObject getMyObject() {
5+
MyObject result = f;
6+
if (result == null) {
7+
synchronized(lock) {
8+
result = f;
9+
if (result == null) {
10+
result = new MyObject();
11+
result.init();
12+
f = result; // GOOD
13+
}
14+
}
15+
}
16+
return result;
17+
}

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

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
Double-checked locking is a common pattern for lazy initialization of a field
9+
accessed by multiple threads.
10+
Depending on the memory model of the underlying runtime, it can, however, be
11+
quite difficult to implement correctly, since reorderings performed by
12+
compiler, runtime, or CPU might expose un-initialized or half-way initialized
13+
objects to other threads.
14+
Java has since version 5 improved its memory model to support double-checked
15+
locking if the underlying field is marked <code>volatile</code> and if all
16+
initialization happens before the volatile write.
17+
</p>
18+
</overview>
19+
20+
<recommendation>
21+
22+
<p>
23+
First, it should be considered whether the getter that performs the lazy
24+
initialization is performance critical.
25+
If not, a much simpler solution is to completely avoid double-checked locking
26+
and simply mark the entire getter as <code>synchronized</code>.
27+
This is much easier to get right and guards against hard-to-find concurrency bugs.
28+
</p>
29+
<p>
30+
If double-checked locking is used, it is important that the underlying field is
31+
<code>volatile</code> and that the update to the field is the last thing that
32+
happens in the synchronized region, that is, all initialization must be done
33+
before the field is assigned.
34+
Furthermore, the Java version must be 5 or newer.
35+
Reading a <code>volatile</code> field has a slight overhead, so it is also
36+
useful to use a local variable to minimize the number of volatile reads.
37+
</p>
38+
39+
</recommendation>
40+
41+
<example>
42+
<p>
43+
The following code lazily initializes <code>f</code> to <code>new MyObject()</code>.
44+
</p>
45+
<sample src="DoubleCheckedLockingBad1.java"/>
46+
<p>
47+
This code is not thread-safe as another thread might see the assignment to
48+
<code>f</code> before the constructor finishes evaluating, for example if the
49+
compiler inlines the memory allocation and the constructor and reorders the
50+
assignment to <code>f</code> to occur just after the memory allocation.
51+
</p>
52+
53+
<p>
54+
Another example that also is not thread-safe, even when <code>volatile</code>
55+
is used, is if additional initialization happens after the assignment to
56+
<code>f</code>, since then other threads may access the constructed object
57+
before it is fully initialized, even without any reorderings by the compiler or
58+
runtime.
59+
</p>
60+
<sample src="DoubleCheckedLockingBad2.java"/>
61+
62+
<p>
63+
The code above should be rewritten to both use <code>volatile</code> and finish
64+
all initialization before <code>f</code> is updated. Additionally, a local
65+
variable can be used to avoid reading the field more times than neccessary.
66+
</p>
67+
<sample src="DoubleCheckedLockingGood.java"/>
68+
69+
</example>
70+
71+
<references>
72+
73+
<li>
74+
<a href="http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html">The "Double-Checked Locking is Broken" Declaration</a>.
75+
</li>
76+
<li>
77+
Java Language Specification:
78+
<a href="https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.4">17.4. Memory Model</a>.
79+
</li>
80+
<li>
81+
Wikipedia: <a href="https://en.wikipedia.org/wiki/Double-checked_locking">Double-checked locking</a>.
82+
</li>
83+
84+
</references>
85+
86+
</qhelp>

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<include src="DoubleCheckedLockingShared.qhelp" />
7+
8+
</qhelp>

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

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
/**
2+
* @name Race condition in double-checked locking object initialization
3+
* @description Performing additional initialization on an object after
4+
* assignment to a shared variable guarded by double-checked
5+
* locking is not thread-safe, and could result in unexpected
6+
* behavior.
7+
* @kind problem
8+
* @problem.severity warning
9+
* @precision high
10+
* @id java/unsafe-double-checked-locking-init-order
11+
* @tags reliability
12+
* correctness
13+
* concurrency
14+
* external/cwe/cwe-609
15+
*/
16+
17+
import java
18+
import DoubleCheckedLocking
19+
20+
predicate whitelistedMethod(Method m) {
21+
m.getDeclaringType().hasQualifiedName("java.io", _) and
22+
m.hasName("println")
23+
}
24+
25+
class SideEffect extends Expr {
26+
SideEffect() {
27+
this instanceof MethodAccess and
28+
not whitelistedMethod(this.(MethodAccess).getMethod())
29+
or
30+
this.(Assignment).getDest() instanceof FieldAccess
31+
}
32+
}
33+
34+
from IfStmt if1, IfStmt if2, SynchronizedStmt sync, Field f, AssignExpr a, SideEffect se
35+
where
36+
doubleCheckedLocking(if1, if2, sync, f) and
37+
a.getEnclosingStmt().getParent*() = if2.getThen() and
38+
se.getEnclosingStmt().getParent*() = sync.getBlock() and
39+
a.(ControlFlowNode).getASuccessor+() = se and
40+
a.getDest().(FieldAccess).getField() = f
41+
select a,
42+
"Potential race condition. This assignment to $@ is visible to other threads before the subsequent statements are executed.",
43+
f, f.toString()

0 commit comments

Comments
 (0)