False Negative: DoubleCheckedLocking.ql misses non-volatile lazy initialization once the null checks are split across locals, helpers, or early returns.

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

```java
// 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.

```java
// 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.

```java
// 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.

```java
// 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.

```java
// 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.

Start with Likely Bugs/Concurrency/DoubleCheckedLocking.ql and compare its matching logic with PosCase1_Var2.java, PosCase1_Var3.java, PosCase1_Var5.java, PosCase2_Var3.java, and PosCase2_Var5.java. Trace how the query models non-volatile reads, synchronized initialization, locals, helpers, and early returns. Done means the checker reports all five unsafe patterns without relying on one canonical AST shape.

Scritto dal modello di indicizzazione a partire dal testo della issue.