8351230: Collections.synchronizedList returns a list that is not thread-safe

src/java.base/share/classes/java/util/Collections.java

@@ -2679,6 +2679,7 @@ public static <T> List<T> synchronizedList(List<T> list) {
                 new SynchronizedList<>(list));
     }
 
+    // used by java.util.Vector
     static <T> List<T> synchronizedList(List<T> list, Object mutex) {
         return (list instanceof RandomAccess ?
                 new SynchronizedRandomAccessList<>(list, mutex) :
@@ -2696,14 +2697,29 @@ static class SynchronizedList<E>
 
         @SuppressWarnings("serial") // Conditionally serializable
         final List<E> list;
+        transient SynchronizedList<E> reversedView = null;
+        transient final boolean isReversed;
 
+        // constructs a forward view, using 'this' as the mutex
         SynchronizedList(List<E> list) {
             super(list);
             this.list = list;
+            this.isReversed = false;
         }
+
+        // constructs a forward view, using 'mutex' as the mutex
         SynchronizedList(List<E> list, Object mutex) {
             super(list, mutex);
             this.list = list;
+            this.isReversed = false;
+        }
+
+        // constructs the reversed view; first arg should be reversed view of backing list
+        SynchronizedList(List<E> revList, Object mutex, SynchronizedList<E> forward) {
+            super(revList, mutex);
+            this.list = revList;
+            this.reversedView = forward;
+            this.isReversed = true;
         }
 
         public boolean equals(Object o) {
@@ -2714,7 +2730,6 @@ public boolean equals(Object o) {
         public int hashCode() {
             synchronized (mutex) {return list.hashCode();}
         }
-
         public E get(int index) {
             synchronized (mutex) {return list.get(index);}
         }
@@ -2727,41 +2742,97 @@ public void add(int index, E element) {
         public E remove(int index) {
             synchronized (mutex) {return list.remove(index);}
         }
-
         public int indexOf(Object o) {
             synchronized (mutex) {return list.indexOf(o);}
         }
         public int lastIndexOf(Object o) {
             synchronized (mutex) {return list.lastIndexOf(o);}
         }
-
         public boolean addAll(int index, Collection<? extends E> c) {
             synchronized (mutex) {return list.addAll(index, c);}
         }
-
         public ListIterator<E> listIterator() {
             return list.listIterator(); // Must be manually synched by user
         }
-
         public ListIterator<E> listIterator(int index) {
             return list.listIterator(index); // Must be manually synched by user
         }
-
         public List<E> subList(int fromIndex, int toIndex) {
             synchronized (mutex) {
                 return new SynchronizedList<>(list.subList(fromIndex, toIndex),
                                             mutex);
             }
         }
-
-        @Override
         public void replaceAll(UnaryOperator<E> operator) {
             synchronized (mutex) {list.replaceAll(operator);}
         }
-        @Override
         public void sort(Comparator<? super E> c) {
             synchronized (mutex) {list.sort(c);}
         }
+        public void addFirst(E element) {
+            synchronized (mutex) {list.addFirst(element);}
+        }
+        public void addLast(E element) {
+            synchronized (mutex) {list.addLast(element);}
+        }
+        public E getFirst() {
+            synchronized (mutex) {return list.getFirst();}
+        }
+        public E getLast() {
+            synchronized (mutex) {return list.getLast();}
+        }
+        public E removeFirst() {
+            synchronized (mutex) {return list.removeFirst();}
+        }
+        public E removeLast() {
+            synchronized (mutex) {return list.removeLast();}
+        }
+
+        /**
+         * Reversed view handling. The reversedView field is transient
+         * and is initialized to null upon construction of the wrapper
+         * and upon deserialization. Reversed views are not serializable.
+         * The reversed view is created on first call to the reversed()
+         * method.
+         *
+         * There are four objects at play here:
+         *
+         * L = original list
+         * Lr = reversed view of original list
+         * S = synchronized forward wrapper: its backing list is L
+         * Sr = synchronized reversed wrapper: its backing list is Lr
+         *
+         * The reversedView field of S points to Sr and vice versa.
+         * This enables the reversed() method always to return the same
+         * object, and for S.reversed().reversed() to return S. This isn't
+         * strictly necessary, because all internal locking is done on S
+         * (which is passed around as mutex). But in the case where the
+         * client does external locking, it's good to minimize the number
+         * of different instances. Note however that external locking on
+         * the reversed wrapper Sr can't be made to work properly with
+         * internal or external locking on the forward wrapper S. (Sublists
+         * of a synchronized wrapper, reversed or not, have the same issue.)
+         *
+         * An alternative would be to have a ReversedSynchronizedList view class. However,
+         * that would require two extra classes (one RandomAccess and one not) and it would
+         * complicate the serialization story.
+         */
+        public List<E> reversed() {
+            synchronized (mutex) {
+                if (reversedView == null) {
+                    reversedView = new SynchronizedList<>(list.reversed(), mutex, this);
+                }
+                return reversedView;
+            }
+        }
+
+        @java.io.Serial
+        private void writeObject(ObjectOutputStream out) throws IOException {
+            if (isReversed) {
+                throw new java.io.NotSerializableException();
+            }
+            out.defaultWriteObject();
+        }
 
         /**
          * SynchronizedRandomAccessList instances are serialized as
@@ -2798,6 +2869,19 @@ static class SynchronizedRandomAccessList<E>
             super(list, mutex);
         }
 
+        SynchronizedRandomAccessList(List<E> list, Object mutex, SynchronizedList<E> forward) {
+            super(list, mutex, forward);
+        }
+
+        public List<E> reversed() {
+            synchronized (mutex) {
+                if (reversedView == null) {
+                    reversedView = new SynchronizedRandomAccessList<>(list.reversed(), mutex, this);
+                }
+                return reversedView;
+            }
+        }
+
         public List<E> subList(int fromIndex, int toIndex) {
             synchronized (mutex) {
                 return new SynchronizedRandomAccessList<>(

test/jdk/java/util/Collections/SyncListBash.java

@@ -0,0 +1,110 @@
+/*
+ * Copyright (c) 2025, 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     8351230
+ * @summary Test that List's new SequencedCollection methods are properly
+ *          synchronized on a synchronized list.
+ * @library /test/lib
+ * @build   jdk.test.lib.Utils
+ * @run     main SyncListBash f
+ * @run     main SyncListBash r
+ */
+
+import java.util.*;
+import java.util.concurrent.*;
+import java.util.stream.*;
+
+import jdk.test.lib.Utils;
+
+public class SyncListBash {
+    static final int LOOP_COUNT = 1000;
+    static final int NUM_WORKERS = 4;
+    static List<Integer> list;
+
+    /**
+     * Test for race conditions with synchronized lists. Several worker threads
+     * add and remove elements from the front of the list, while the main thread
+     * gets the last element. The last element should never change. However, if
+     * the list isn't properly synchronized, getLast() might return the wrong
+     * element or throw IndexOutOfBoundsException.
+     *
+     * On an unsynchronized list, this fails 200-500 out of 1000 times, so this
+     * seems like a fairly reliable way to test for a race condition.
+     *
+     * @param args there must be one arg, "f" or "r", which determines whether the
+     *             forward (original) list or reversed view of the list is tested
+     * @throws InterruptedException if the main thread is interrupted
+     */
+    public static void main(String[] args) throws InterruptedException {
+        int wrongElement = 0;
+        int numExceptions = 0;
+
+        boolean reversed = switch (args[0]) {
+            case "f" -> false;
+            case "r" -> true;
+            default -> throw new IllegalArgumentException();
+        };
+
+        list = IntStream.range(0, 10)
+                        .boxed()
+                        .collect(Collectors.toCollection(ArrayList::new));
+        list = Collections.synchronizedList(list);
+        if (reversed)
+            list = list.reversed();
+        Integer expectedLast = list.getLast();
+
+        ExecutorService pool = Executors.newFixedThreadPool(NUM_WORKERS);
+        for (int i = 0; i < NUM_WORKERS; i++)
+            pool.submit(() -> {
+                while (! Thread.currentThread().isInterrupted()) {
+                    list.add(0, -1);
+                    list.remove(0);
+                }
+            });
+
+        for (int i = 0; i < LOOP_COUNT; i++) {
+            Thread.sleep(1L);
+            try {
+                Integer actualLast = list.getLast();
+                if (! expectedLast.equals(actualLast)) {
+                    ++wrongElement;
+                }
+            } catch (IndexOutOfBoundsException ioobe) {
+                ++numExceptions;
+            }
+        }
+
+        pool.shutdownNow();
+        pool.awaitTermination(Utils.adjustTimeout(60L), TimeUnit.SECONDS);
+
+        System.out.printf("LOOP_COUNT=%d wrongElement=%d numExceptions=%d%n",
+            LOOP_COUNT, wrongElement, numExceptions);
+        if (wrongElement == 0 && numExceptions == 0) {
+            System.out.println("Test passed.");
+        } else {
+            throw new AssertionError("TEST FAILED!");
+        }
+    }
+}