8243592: Subject$SecureSet::addAll should not call contains(null)

Reviewed-by: mullan

src/java.base/share/classes/javax/security/auth/Subject.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2020, 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
@@ -203,18 +203,20 @@ public final class Subject implements java.io.Serializable {
      *          Sets.
      */
     public Subject(boolean readOnly, Set<? extends Principal> principals,
-                   Set<?> pubCredentials, Set<?> privCredentials)
+                   Set<?> pubCredentials, Set<?> privCredentials) {
-    {
+        LinkedList<Principal> principalList
-        collectionNullClean(principals);
+                = collectionNullClean(principals);
-        collectionNullClean(pubCredentials);
+        LinkedList<Object> pubCredsList
-        collectionNullClean(privCredentials);
+                = collectionNullClean(pubCredentials);
+        LinkedList<Object> privCredsList
+                = collectionNullClean(privCredentials);
 
-        this.principals = Collections.synchronizedSet(new SecureSet<>
+        this.principals = Collections.synchronizedSet(
-                                (this, PRINCIPAL_SET, principals));
+                new SecureSet<>(this, PRINCIPAL_SET, principalList));
-        this.pubCredentials = Collections.synchronizedSet(new SecureSet<>
+        this.pubCredentials = Collections.synchronizedSet(
-                                (this, PUB_CREDENTIAL_SET, pubCredentials));
+                new SecureSet<>(this, PUB_CREDENTIAL_SET, pubCredsList));
-        this.privCredentials = Collections.synchronizedSet(new SecureSet<>
+        this.privCredentials = Collections.synchronizedSet(
-                                (this, PRIV_CREDENTIAL_SET, privCredentials));
+                new SecureSet<>(this, PRIV_CREDENTIAL_SET, privCredsList));
         this.readOnly = readOnly;
     }
 
@@ -975,8 +977,9 @@ public final class Subject implements java.io.Serializable {
 
         // Rewrap the principals into a SecureSet
         try {
+            LinkedList<Principal> principalList = collectionNullClean(inputPrincs);
             principals = Collections.synchronizedSet(new SecureSet<>
-                                (this, PRINCIPAL_SET, inputPrincs));
+                                (this, PRINCIPAL_SET, principalList));
         } catch (NullPointerException npe) {
             // Sometimes people deserialize the principals set only.
             // Subject is not accessible, so just don't fail.
@@ -1001,26 +1004,18 @@ public final class Subject implements java.io.Serializable {
      * @throws NullPointerException if the specified collection is either
      *            {@code null} or contains a {@code null} element
      */
-    private static void collectionNullClean(Collection<?> coll) {
+    private static <E> LinkedList<E> collectionNullClean(
-        boolean hasNullElements = false;
+            Collection<? extends E> coll) {
 
         Objects.requireNonNull(coll,
                 ResourcesMgr.getString("invalid.null.input.s."));
 
-        try {
+        LinkedList<E> output = new LinkedList<>();
-            hasNullElements = coll.contains(null);
+        for (E e : coll) {
-        } catch (NullPointerException npe) {
+            output.add(Objects.requireNonNull(e,
-            // A null-hostile collection may choose to throw
+                    ResourcesMgr.getString("invalid.null.input.s.")));
-            // NullPointerException if contains(null) is called on it
-            // rather than returning false.
-            // If this happens we know the collection is null-clean.
-            hasNullElements = false;
-        } finally {
-            if (hasNullElements) {
-                throw new NullPointerException
-                    (ResourcesMgr.getString("invalid.null.input.s."));
-            }
         }
+        return output;
     }
 
     /**
@@ -1066,10 +1061,10 @@ public final class Subject implements java.io.Serializable {
             this.elements = new LinkedList<E>();
         }
 
-        SecureSet(Subject subject, int which, Set<? extends E> set) {
+        SecureSet(Subject subject, int which, LinkedList<E> list) {
             this.subject = subject;
             this.which = which;
-            this.elements = new LinkedList<E>(set);
+            this.elements = list;
         }
 
         public int size() {
@@ -1242,7 +1237,7 @@ public final class Subject implements java.io.Serializable {
         public boolean addAll(Collection<? extends E> c) {
             boolean result = false;
 
-            collectionNullClean(c);
+            c = collectionNullClean(c);
 
             for (E item : c) {
                 result |= this.add(item);
@@ -1252,7 +1247,7 @@ public final class Subject implements java.io.Serializable {
         }
 
         public boolean removeAll(Collection<?> c) {
-            collectionNullClean(c);
+            c = collectionNullClean(c);
 
             boolean modified = false;
             final Iterator<E> e = iterator();
@@ -1282,7 +1277,7 @@ public final class Subject implements java.io.Serializable {
         }
 
         public boolean containsAll(Collection<?> c) {
-            collectionNullClean(c);
+            c = collectionNullClean(c);
 
             for (Object item : c) {
                 if (this.contains(item) == false) {
@@ -1294,7 +1289,7 @@ public final class Subject implements java.io.Serializable {
         }
 
         public boolean retainAll(Collection<?> c) {
-            collectionNullClean(c);
+            c = collectionNullClean(c);
 
             boolean modified = false;
             final Iterator<E> e = iterator();
@@ -1443,13 +1438,7 @@ public final class Subject implements java.io.Serializable {
 
             LinkedList<E> tmp = (LinkedList<E>) fields.get("elements", null);
 
-            Subject.collectionNullClean(tmp);
+            elements = Subject.collectionNullClean(tmp);
-
-            if (tmp.getClass() != LinkedList.class) {
-                elements = new LinkedList<E>(tmp);
-            } else {
-                elements = tmp;
-            }
         }
 
     }

test/jdk/javax/security/auth/Subject/UnreliableContains.java

@@ -0,0 +1,68 @@
+/*
+ * Copyright (c) 2020, 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 8243592
+ * @summary Subject$SecureSet::addAll should not call contains(null)
+ */
+
+import javax.security.auth.Subject;
+import java.security.Principal;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Objects;
+
+public class UnreliableContains {
+
+    public static void main(String[] args) {
+        MySet<Principal> set = new MySet<>();
+        set.add(null);
+        Subject s = null;
+        try {
+            s = new Subject(false, set, Collections.emptySet(),
+                    Collections.emptySet());
+        } catch (NullPointerException e) {
+            // The correct exit
+            return;
+        }
+        // Suppose NPE was not caught. At least null was not added?
+        for (Principal p : s.getPrincipals()) {
+            Objects.requireNonNull(p);
+        }
+        // Still must fail. We don't want this Subject created
+        throw new RuntimeException("Fail");
+    }
+
+    // This is a Map that implements contains(null) differently
+    static class MySet<E> extends HashSet<E> {
+        @Override
+        public boolean contains(Object o) {
+            if (o == null) {
+                return false;
+            } else {
+                return super.contains(o);
+            }
+        }
+    }
+}