Merge pull request #786 from aschackmull/java/double-checked-locking

Java: Fix FP in DoubleCheckedLocking.ql

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 and more true positive results | Results that use safe publication through a `final` field are no longer reported. Results that initialize immutable types like `String` incorrectly are now 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,26 @@
 import java
 import DoubleCheckedLocking
 
+predicate allFieldsFinal(Class c) { forex(Field f | c.inherits(f) | f.isFinal()) }
+
+predicate immutableFieldType(Type t) {
+  allFieldsFinal(t) or
+  t instanceof ImmutableType
+}
+
 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.
+    immutableFieldType(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/DoubleCheckedLocking.qll

@@ -38,6 +38,5 @@ predicate doubleCheckedLocking(IfStmt if1, IfStmt if2, SynchronizedStmt sync, Fi
   if1.getThen() = sync.getParent*() and
   sync.getBlock() = if2.getParent*() and
   if1.getCondition() = getANullCheck(f) and
-  if2.getCondition() = getANullCheck(f) and
-  not f.getType() instanceof ImmutableType
+  if2.getCondition() = getANullCheck(f)
 }

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 object you construct is immutable (that is, the
+object declares all fields as <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,14 @@ Java Language Specification:
 <li>
 Wikipedia: <a href="https://en.wikipedia.org/wiki/Double-checked_locking">Double-checked locking</a>.
 </li>
+<li>
+Aleksey Shipilëv:
+<a href="https://shipilev.net/blog/2014/safe-public-construction/">Safe Publication and Safe Initialization in Java</a>.
+</li>
+<li>
+Aleksey Shipilëv:
+<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

@@ -6,16 +6,31 @@ public class A {
     }
   }
 
-  private String s;
-  public String getString() {
-    if (s == null) {
+  private String s1;
+  public String getString1() {
+    if (s1 == null) {
       synchronized(this) {
-        if (s == null) {
-          s = "string"; // OK, immutable
+        if (s1 == null) {
+          s1 = "string"; // BAD, immutable but read twice outside sync
         }
       }
     }
-    return s;
+    return s1;
+  }
+
+  private String s2;
+  public String getString2() {
+    String x = s2;
+    if (x == null) {
+      synchronized(this) {
+        x = s2;
+        if (x == null) {
+          x = "string"; // OK, immutable and read once outside sync
+          s2 = x;
+        }
+      }
+    }
+    return x;
   }
 
   private B b1;
@@ -72,4 +87,40 @@ public class A {
     }
     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,3 @@
-| 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:12:7:16:7 | stmt | Double-checked locking on the non-volatile field $@ is not thread-safe. | A.java:9:18:9:19 | s1 | s1 |
+| A.java:40:7:45:7 | stmt | Double-checked locking on the non-volatile field $@ is not thread-safe. | A.java:36:13:36:14 | b1 | b1 |
+| A.java:101:7:106:7 | stmt | Double-checked locking on the non-volatile field $@ is not thread-safe. | A.java:98:26:98:27 | b5 | b5 |

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

@@ -1,2 +1,2 @@
-| A.java:55:11:55:22 | ...=... | Potential race condition. This assignment to $@ is visible to other threads before the subsequent statements are executed. | A.java:50:22:50:23 | b3 | b3 |
-| A.java:68:11:68:22 | ...=... | Potential race condition. This assignment to $@ is visible to other threads before the subsequent statements are executed. | A.java:63:22:63:23 | b4 | b4 |
+| A.java:70:11:70:22 | ...=... | Potential race condition. This assignment to $@ is visible to other threads before the subsequent statements are executed. | A.java:65:22:65:23 | b3 | b3 |
+| A.java:83:11:83:22 | ...=... | Potential race condition. This assignment to $@ is visible to other threads before the subsequent statements are executed. | A.java:78:22:78:23 | b4 | b4 |