False Negative: DoubleCheckedLocking.ql misses non-volatile lazy initialization once the null checks are split across locals, helpers, or early returns.
Version
codeql 2.24.3
Checker
- Checker id:
Likely Bugs/Concurrency/DoubleCheckedLocking.ql
- Checker description: Detects double-checked locking patterns on non-volatile fields that are not thread-safe due to improper synchronization and field visibility issues.
Description of the false negative
All five samples preserve the same unsafe publication pattern: a non-volatile field is read outside synchronization and lazily initialized under synchronization. The code only changes how the null checks and the return are spelled.
That should still be enough for Likely Bugs/Concurrency/DoubleCheckedLocking.ql to report the pattern.
Affected test cases
PosCase1_Var2.java
The boolean temporaries only cache the same two null checks. This is still ordinary double-checked locking on a non-volatile field.
// Double-checked locking on non-volatile, non-immutable field should be flagged as thread-unsafe.
package scensct.var.pos;
public class PosCase1_Var2 {
private Object instance;
public Object getInstance() {
// outer check with boolean flag
boolean needSync = instance == null;
if (needSync) {
synchronized (this) {
// inner check with explicit condition variable
boolean stillNull = instance == null;
if (stillNull) {
instance = new Object();
}
}
}
return instance;
}
}PosCase1_Var3.java
The early return does not make the pattern safe. The field is still observed outside synchronization and initialized lazily inside the synchronized block.
// Double-checked locking on non-volatile, non-immutable field should be flagged as thread-unsafe.
package scensct.var.pos;
public class PosCase1_Var3 {
private Object instance;
public Object getInstance() {
if (instance != null) {
return instance; // early return instead of if-null
}
synchronized (this) {
if (instance == null) {
instance = new Object();
}
return instance; // return inside synchronized block
}
}
}PosCase1_Var5.java
Moving the inner null check into initSingleton() only hides the same unsafe initialization pattern behind a helper.
// Double-checked locking on non-volatile, non-immutable field should be flagged as thread-unsafe.
package scensct.var.pos;
public class PosCase1_Var5 {
private Object instance;
private void initSingleton() {
if (instance == null) {
instance = new Object();
}
}
public Object getInstance() {
if (instance == null) {
synchronized (this) {
// inner check moved into helper but still inside sync
initSingleton();
}
}
return instance;
}
}PosCase2_Var3.java
The helper methods separate the outer check, the synchronized initialization, and the final read, but the field is still accessed outside synchronization and remains non-volatile.
// Double-checked locking on non-volatile immutable-type field with field access outside synchronized block should be flagged.
package scensct.var.pos;
public class PosCase2_Var3 {
private String instance;
public String getInstance() {
if (isNotInitialized()) { // Outer check extracted to helper
initializeSafely();
}
return retrieveInstance(); // Access extracted to helper
}
private boolean isNotInitialized() {
return instance == null;
}
private void initializeSafely() {
synchronized (this) {
if (instance == null) {
instance = "lazy";
}
}
}
private String retrieveInstance() {
return instance; // Access outside synchronized block
}
}PosCase2_Var5.java
This is still a read-outside-sync / initialize-inside-sync pattern. Restructuring it as an early return does not repair the memory-visibility problem.
// Double-checked locking on non-volatile immutable-type field with field access outside synchronized block should be flagged.
package scensct.var.pos;
public class PosCase2_Var5 {
private String instance;
public String getInstance() {
// Restructured control flow with early return for null after sync
if (instance != null) {
return instance; // Early return, but still access outside sync
}
synchronized (this) {
if (instance == null) {
instance = "done";
}
}
// Field accessed after synchronization block
return instance;
}
}Cause analysis
The query appears too tied to one canonical AST shape for double-checked locking. As soon as the outer check is cached in a local, the inner initialization moves to a helper, or the method returns early on the fast path, the match falls apart.
That is a real modeling gap. The thread-safety problem depends on non-volatile publication and unsynchronized reads, not on whether the code is written in one exact syntactic form.
References
None known.