Java: Fix FP in DoubleCheckedLocking.ql

Author: aschackmull

change-notes/1.20/analysis-java.md

@@ -14,6 +14,7 @@
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
+| Double-checked locking is not thread-safe (`java/unsafe-double-checked-locking`) | Fewer false positive results | Results that use safe publication through a `final` field are no longer reported. |
 | Result of multiplication cast to wider type (`java/integer-multiplication-cast-to-long`) | Fewer results | Results involving conversions to `float` or `double` are no longer reported, as they were almost exclusively false positives. |
 
 ## Changes to QL libraries

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

@@ -15,9 +15,21 @@
 import java
 import DoubleCheckedLocking
 
+predicate allFieldsFinal(Class c) { forex(Field f | c.inherits(f) | f.isFinal()) }
+
 from IfStmt if1, IfStmt if2, SynchronizedStmt sync, Field f
 where
   doubleCheckedLocking(if1, if2, sync, f) and
-  not f.isVolatile()
+  not f.isVolatile() and
+  not (
+    // Non-volatile double-checked locking is ok when the object is immutable and
+    // there is only a single non-synchronized field read.
+    allFieldsFinal(f.getType()) and
+    1 = strictcount(FieldAccess fa |
+        fa.getField() = f and
+        fa.getEnclosingCallable() = sync.getEnclosingCallable() and
+        not fa.getEnclosingStmt().getParent*() = sync.getBlock()
+      )
+  )
 select sync, "Double-checked locking on the non-volatile field $@ is not thread-safe.", f,
   f.toString()

java/ql/src/Likely Bugs/Concurrency/DoubleCheckedLockingBad3.java

@@ -0,0 +1,13 @@
+private Object lock = new Object();
+private MyImmutableObject f = null;
+
+public MyImmutableObject getMyImmutableObject() {
+  if (f == null) {
+    synchronized(lock) {
+      if (f == null) {
+        f = new MyImmutableObject();
+      }
+    }
+  }
+  return f; // BAD
+}

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

@@ -66,6 +66,26 @@ variable can be used to avoid reading the field more times than neccessary.
 </p>
 <sample src="DoubleCheckedLockingGood.java"/>
 
+<p>
+As a final note, it is possible to use double-checked locking correctly without
+<code>volatile</code> if the constructed object is immutable in the sense that
+all its fields are declared <code>final</code> and the double-checked field is
+read exactly once outside the synchronized block.
+</p>
+<p>
+Given that all fields in <code>MyImmutableObject</code> are declared
+<code>final</code> then the following example is protected against exposing
+uninitialized fields to another thread. However, since there are two reads of
+<code>f</code> without synchronization, it is possible that these are
+reordered, which means that this method can return <code>null</code>.
+</p>
+<sample src="DoubleCheckedLockingBad3.java"/>
+<p>
+In this case, using a local variable to minimize the number of field reads is
+no longer a performance improvement, but rather a crucial detail that is
+necessary for correctness.
+</p>
+
 </example>
 
 <references>
@@ -80,6 +100,12 @@ Java Language Specification:
 <li>
 Wikipedia: <a href="https://en.wikipedia.org/wiki/Double-checked_locking">Double-checked locking</a>.
 </li>
+<li>
+<a href="https://shipilev.net/blog/2014/safe-public-construction/">Safe Publication and Safe Initialization in Java</a>.
+</li>
+<li>
+<a href="https://shipilev.net/blog/2016/close-encounters-of-jmm-kind/">Close Encounters of The Java Memory Model Kind</a>.
+</li>
 
 </references>
 

java/ql/test/query-tests/DoubleCheckedLocking/A.java

@@ -72,4 +72,40 @@ public B getter4() {
     }
     return b4;
   }
+
+  static class FinalHelper<T> {
+    public final T x;
+    public FinalHelper(T x) {
+      this.x = x;
+    }
+  }
+
+  private FinalHelper<B> b5;
+  public B getter5() {
+    if (b5 == null) {
+      synchronized(this) {
+        if (b5 == null) {
+          B b = new B();
+          b5 = new FinalHelper<B>(b); // BAD, racy read on b5 outside synchronized-block
+        }
+      }
+    }
+    return b5.x; // Potential NPE here, as the two b5 reads may be reordered
+  }
+
+  private FinalHelper<B> b6;
+  public B getter6() {
+    FinalHelper<B> a = b6;
+    if (a == null) {
+      synchronized(this) {
+        a = b6;
+        if (a == null) {
+          B b = new B();
+          a = new FinalHelper<B>(b);
+          b6 = a; // OK, published through final field with a single non-synced read
+        }
+      }
+    }
+    return a.x;
+  }
 }

java/ql/test/query-tests/DoubleCheckedLocking/DoubleCheckedLocking.expected

@@ -1 +1,2 @@
 | A.java:25:7:30:7 | stmt | Double-checked locking on the non-volatile field $@ is not thread-safe. | A.java:21:13:21:14 | b1 | b1 |
+| A.java:86:7:91:7 | stmt | Double-checked locking on the non-volatile field $@ is not thread-safe. | A.java:83:26:83:27 | b5 | b5 |