Date: Tue, 30 May 2023 15:42:59 +0800
Subject: [PATCH 03/59] 8187408: AbstractQueuedSynchronizer wait queue corrupted when thread awaits without holding the lock

Bug url: https://bugs.openjdk.org/browse/JDK-8187408
---
 .../locks/AbstractQueuedLongSynchronizer.java |  20 +--
 .../locks/AbstractQueuedSynchronizer.java     |  52 +++---
 jdk/test/java/util/Bug8187408/Bug8187408.java | 165 ++++++++++++++++++
 3 files changed, 207 insertions(+), 30 deletions(-)
 create mode 100644 jdk/test/java/util/Bug8187408/Bug8187408.java

diff --git a/jdk/src/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java b/jdk/src/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java
index 8699fc9b8..5adc39e17 100644
--- a/jdk/src/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java
+++ b/jdk/src/share/classes/java/util/concurrent/locks/AbstractQueuedLongSynchronizer.java
@@ -64,11 +64,11 @@ public abstract class AbstractQueuedLongSynchronizer
     private static final long serialVersionUID = 7373984972572414692L;
 
     /*
-      To keep sources in sync, the remainder of this source file is
-      exactly cloned from AbstractQueuedSynchronizer, replacing class
-      name and changing ints related with sync state to longs. Please
-      keep it that way.
-    */
+     * To keep sources in sync, the remainder of this source file is
+     * exactly cloned from AbstractQueuedSynchronizer, replacing class
+     * name and changing ints related with sync state to longs. Please
+     * keep it that way.
+     */
 
     /**
      * Creates a new {@code AbstractQueuedLongSynchronizer} instance
@@ -946,8 +946,7 @@ public abstract class AbstractQueuedLongSynchronizer
     /**
      * Returns {@code true} if synchronization is held exclusively with
      * respect to the current (calling) thread.  This method is invoked
-     * upon each call to a non-waiting {@link ConditionObject} method.
-     * (Waiting methods instead invoke {@link #release}.)
+     * upon each call to a {@link ConditionObject} method.
      *
      * <p>The default implementation throws {@link
      * UnsupportedOperationException}. This method is invoked
@@ -1597,9 +1596,8 @@ public abstract class AbstractQueuedLongSynchronizer
     }
 
     /**
-     * Condition implementation for a {@link
-     * AbstractQueuedLongSynchronizer} serving as the basis of a {@link
-     * Lock} implementation.
+     * Condition implementation for a {@link AbstractQueuedLongSynchronizer}
+     * serving as the basis of a {@link Lock} implementation.
      *
      * <p>Method documentation for this class describes mechanics,
      * not behavioral specifications from the point of view of Lock
@@ -1632,6 +1630,8 @@ public abstract class AbstractQueuedLongSynchronizer
          * @return its new wait node
          */
         private Node addConditionWaiter() {
+            if (!isHeldExclusively())
+                throw new IllegalMonitorStateException();
             Node t = lastWaiter;
             // If lastWaiter is cancelled, clean out.
             if (t != null && t.waitStatus != Node.CONDITION) {
diff --git a/jdk/src/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java b/jdk/src/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java
index 9088e5894..0e2bcdfef 100644
--- a/jdk/src/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java
+++ b/jdk/src/share/classes/java/util/concurrent/locks/AbstractQueuedSynchronizer.java
@@ -192,19 +192,13 @@ import sun.misc.Unsafe;
  * represent the locked state. While a non-reentrant lock
  * does not strictly require recording of the current owner
  * thread, this class does so anyway to make usage easier to monitor.
- * It also supports conditions and exposes
- * one of the instrumentation methods:
+ * It also supports conditions and exposes some instrumentation methods:
  *
  *  <pre> {@code
  * class Mutex implements Lock, java.io.Serializable {
  *
  *   // Our internal helper class
  *   private static class Sync extends AbstractQueuedSynchronizer {
- *     // Reports whether in locked state
- *     protected boolean isHeldExclusively() {
- *       return getState() == 1;
- *     }
- *
  *     // Acquires the lock if state is zero
  *     public boolean tryAcquire(int acquires) {
  *       assert acquires == 1; // Otherwise unused
@@ -218,14 +212,27 @@ import sun.misc.Unsafe;
  *     // Releases the lock by setting state to zero
  *     protected boolean tryRelease(int releases) {
  *       assert releases == 1; // Otherwise unused
- *       if (getState() == 0) throw new IllegalMonitorStateException();
+ *       if (!isHeldExclusively())
+ *         throw new IllegalMonitorStateException();
  *       setExclusiveOwnerThread(null);
  *       setState(0);
  *       return true;
  *     }
  *
+ *     // Reports whether in locked state
+ *     public boolean isLocked() {
+ *       return getState() != 0;
+ *     }
+ *
+ *     public boolean isHeldExclusively() {
+ *       // a data race, but safe due to out-of-thin-air guarantees
+ *       return getExclusiveOwnerThread() == Thread.currentThread();
+ *     }
+ *
  *     // Provides a Condition
- *     Condition newCondition() { return new ConditionObject(); }
+ *     public Condition newCondition() {
+ *       return new ConditionObject();
+ *     }
  *
  *     // Deserializes properly
  *     private void readObject(ObjectInputStream s)
@@ -238,12 +245,17 @@ import sun.misc.Unsafe;
  *   // The sync object does all the hard work. We just forward to it.
  *   private final Sync sync = new Sync();
  *
- *   public void lock()                { sync.acquire(1); }
- *   public boolean tryLock()          { return sync.tryAcquire(1); }
- *   public void unlock()              { sync.release(1); }
- *   public Condition newCondition()   { return sync.newCondition(); }
- *   public boolean isLocked()         { return sync.isHeldExclusively(); }
- *   public boolean hasQueuedThreads() { return sync.hasQueuedThreads(); }
+ *   public void lock()              { sync.acquire(1); }
+ *   public boolean tryLock()        { return sync.tryAcquire(1); }
+ *   public void unlock()            { sync.release(1); }
+ *   public Condition newCondition() { return sync.newCondition(); }
+ *   public boolean isLocked()       { return sync.isLocked(); }
+ *   public boolean isHeldByCurrentThread() {
+ *     return sync.isHeldExclusively();
+ *   }
+ *   public boolean hasQueuedThreads() {
+ *     return sync.hasQueuedThreads();
+ *   }
  *   public void lockInterruptibly() throws InterruptedException {
  *     sync.acquireInterruptibly(1);
  *   }
@@ -1168,8 +1180,7 @@ public abstract class AbstractQueuedSynchronizer
     /**
      * Returns {@code true} if synchronization is held exclusively with
      * respect to the current (calling) thread.  This method is invoked
-     * upon each call to a non-waiting {@link ConditionObject} method.
-     * (Waiting methods instead invoke {@link #release}.)
+     * upon each call to a {@link ConditionObject} method.
      *
      * <p>The default implementation throws {@link
      * UnsupportedOperationException}. This method is invoked
@@ -1819,9 +1830,8 @@ public abstract class AbstractQueuedSynchronizer
     }
 
     /**
-     * Condition implementation for a {@link
-     * AbstractQueuedSynchronizer} serving as the basis of a {@link
-     * Lock} implementation.
+     * Condition implementation for a {@link AbstractQueuedSynchronizer}
+     * serving as the basis of a {@link Lock} implementation.
      *
      * <p>Method documentation for this class describes mechanics,
      * not behavioral specifications from the point of view of Lock
@@ -1852,6 +1862,8 @@ public abstract class AbstractQueuedSynchronizer
          * @return its new wait node
          */
         private Node addConditionWaiter() {
+            if (!isHeldExclusively())
+                throw new IllegalMonitorStateException();
             Node t = lastWaiter;
             // If lastWaiter is cancelled, clean out.
             if (t != null && t.waitStatus != Node.CONDITION) {
diff --git a/jdk/test/java/util/Bug8187408/Bug8187408.java b/jdk/test/java/util/Bug8187408/Bug8187408.java
new file mode 100644
index 000000000..fee6c730d
--- /dev/null
+++ b/jdk/test/java/util/Bug8187408/Bug8187408.java
@@ -0,0 +1,165 @@
+/*
+ * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+ /*
+ * @test
+ * @bug 8187408
+ * @summary AbstractQueuedSynchronizer wait queue corrupted when thread awaits without holding the lock
+ */
+
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import java.util.ArrayList;
+import java.util.Date;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ThreadLocalRandom;
+import java.util.concurrent.locks.Condition;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+public class Bug8187408 {
+    static final long LONG_DELAY_MS = 10000L;
+
+    public static void main(String[] args) throws InterruptedException {
+        final ThreadLocalRandom rnd = ThreadLocalRandom.current();
+        final AwaitMethod awaitMethod = randomAwaitMethod();
+        final int nThreads = rnd.nextInt(2, 10);
+        final ReentrantLock lock = new ReentrantLock();
+        final Condition cond = lock.newCondition();
+        final CountDownLatch done = new CountDownLatch(nThreads);
+        final ArrayList<Thread> threads = new ArrayList<>();
+
+        Runnable rogue = () -> {
+            while (done.getCount() > 0) {
+                try {
+                    // call await without holding lock?!
+                    await(cond, awaitMethod);
+                    throw new AssertionError("should throw");
+                }
+                catch (IllegalMonitorStateException success) {}
+                catch (Throwable fail) { threadUnexpectedException(fail); }}};
+        Thread rogueThread = new Thread(rogue, "rogue");
+        threads.add(rogueThread);
+        rogueThread.start();
+
+        Runnable waiter = () -> {
+            lock.lock();
+            try {
+                done.countDown();
+                cond.await();
+            } catch (Throwable fail) {
+                threadUnexpectedException(fail);
+            } finally {
+                lock.unlock();
+            }};
+        for (int i = 0; i < nThreads; i++) {
+            Thread thread = new Thread(waiter, "waiter");
+            threads.add(thread);
+            thread.start();
+        }
+
+        assertTrue(done.await(LONG_DELAY_MS, MILLISECONDS));
+        lock.lock();
+        try {
+            assertEquals(nThreads, lock.getWaitQueueLength(cond));
+        } finally {
+            cond.signalAll();
+            lock.unlock();
+        }
+        for (Thread thread : threads) {
+            thread.join(LONG_DELAY_MS);
+            assertFalse(thread.isAlive());
+        }
+    }
+
+    private static void assertTrue(boolean expr) {
+        if (!expr)
+            throw new RuntimeException("assertion failed");
+    }
+
+    private static void assertFalse(boolean expr) {
+        if (expr)
+            throw new RuntimeException("assertion failed");
+    }
+
+    private static void assertEquals(int i, int j) {
+        if (i != j)
+            throw new AssertionError(i + " != " + j);
+    }
+
+    /**
+     * Records the given exception using {@link #threadRecordFailure},
+     * then rethrows the exception, wrapping it in an AssertionError
+     * if necessary.
+     */
+    private static void threadUnexpectedException(Throwable t) {
+        t.printStackTrace();
+        if (t instanceof RuntimeException)
+            throw (RuntimeException) t;
+        else if (t instanceof Error)
+            throw (Error) t;
+        else
+            throw new AssertionError("unexpected exception: " + t, t);
+    }
+
+    enum AwaitMethod { await, awaitTimed, awaitNanos, awaitUntil }
+    private static AwaitMethod randomAwaitMethod() {
+        AwaitMethod[] awaitMethods = AwaitMethod.values();
+        return awaitMethods[ThreadLocalRandom.current().nextInt(awaitMethods.length)];
+    }
+
+    /**
+     * Returns a new Date instance representing a time at least
+     * delayMillis milliseconds in the future.
+     */
+    private static Date delayedDate(long delayMillis) {
+        // Add 1 because currentTimeMillis is known to round into the past.
+        return new Date(System.currentTimeMillis() + delayMillis + 1);
+    }
+
+    /**
+     * Awaits condition "indefinitely" using the specified AwaitMethod.
+     */
+    private static void await(Condition c, AwaitMethod awaitMethod)
+            throws InterruptedException {
+        long timeoutMillis = 2 * LONG_DELAY_MS;
+        switch (awaitMethod) {
+        case await:
+            c.await();
+            break;
+        case awaitTimed:
+            assertTrue(c.await(timeoutMillis, MILLISECONDS));
+            break;
+        case awaitNanos:
+            long timeoutNanos = MILLISECONDS.toNanos(timeoutMillis);
+            long nanosRemaining = c.awaitNanos(timeoutNanos);
+            assertTrue(nanosRemaining > timeoutNanos / 2);
+            assertTrue(nanosRemaining <= timeoutNanos);
+            break;
+        case awaitUntil:
+            assertTrue(c.awaitUntil(delayedDate(timeoutMillis)));
+            break;
+        default:
+            throw new AssertionError();
+        }
+    }
+}
\ No newline at end of file
-- 
2.22.0