8248505: Unexpected NoSuchAlgorithmException when using secure random impl from BCFIPS provider

Summary: Use getService(...) call for Provider.getDefaultSecureRandomService()
Reviewed-by: weijun, coffeys, mullan

src/java.base/share/classes/java/security/Provider.java

@@ -867,7 +867,7 @@ public abstract class Provider extends Properties {
     // For backward compatibility, the registration ordering of
     // SecureRandom (RNG) algorithms needs to be preserved for
     // "new SecureRandom()" calls when this provider is used
-    private transient Set<Service> prngServices;
+    private transient Set<String> prngAlgos;
 
     // Map<ServiceKey,Service>
     // used for services added via legacy methods, init on demand
@@ -1087,7 +1087,7 @@ public abstract class Provider extends Properties {
         legacyChanged = false;
         servicesChanged = false;
         serviceSet = null;
-        prngServices = null;
+        prngAlgos = null;
         super.clear();
         putId();
     }
@@ -1219,7 +1219,7 @@ public abstract class Provider extends Properties {
                 s.className = className;
 
                 if (type.equals("SecureRandom")) {
-                    updateSecureRandomEntries(true, s);
+                    updateSecureRandomEntries(true, s.algorithm);
                 }
             } else { // attribute
                 // e.g. put("MessageDigest.SHA-1 ImplementedIn", "Software");
@@ -1381,25 +1381,25 @@ public abstract class Provider extends Properties {
         synchronized (this) {
             putPropertyStrings(s);
             if (type.equals("SecureRandom")) {
-                updateSecureRandomEntries(true, s);
+                updateSecureRandomEntries(true, s.algorithm);
             }
         }
     }
 
-    private void updateSecureRandomEntries(boolean doAdd, Service s) {
+    // keep tracks of the registered secure random algos and store them in order
+    private void updateSecureRandomEntries(boolean doAdd, String s) {
         Objects.requireNonNull(s);
         if (doAdd) {
-            if (prngServices == null) {
-                prngServices = new LinkedHashSet<Service>();
+            if (prngAlgos == null) {
+                prngAlgos = new LinkedHashSet<String>();
             }
-            prngServices.add(s);
+            prngAlgos.add(s);
         } else {
-            prngServices.remove(s);
+            prngAlgos.remove(s);
         }
 
         if (debug != null) {
-            debug.println((doAdd? "Add":"Remove") + " SecureRandom algo " +
-                s.getAlgorithm());
+            debug.println((doAdd? "Add":"Remove") + " SecureRandom algo " + s);
         }
     }
 
@@ -1409,12 +1409,15 @@ public abstract class Provider extends Properties {
         checkInitialized();
 
         if (legacyChanged) {
-            prngServices = null;
+            prngAlgos = null;
             ensureLegacyParsed();
         }
 
-        if (prngServices != null && !prngServices.isEmpty()) {
-            return prngServices.iterator().next();
+        if (prngAlgos != null && !prngAlgos.isEmpty()) {
+            // IMPORTANT: use the Service obj returned by getService(...) call
+            // as providers may override putService(...)/getService(...) and
+            // return their own Service objects
+            return getService("SecureRandom", prngAlgos.iterator().next());
         }
 
         return null;
@@ -1514,7 +1517,7 @@ public abstract class Provider extends Properties {
         synchronized (this) {
             removePropertyStrings(s);
             if (type.equals("SecureRandom")) {
-                updateSecureRandomEntries(false, s);
+                updateSecureRandomEntries(false, s.algorithm);
             }
         }
     }

test/jdk/java/security/SecureRandom/DefaultAlgo.java

@@ -26,13 +26,12 @@ import java.security.Provider;
 import java.security.Security;
 import java.security.SecureRandom;
 import java.security.Provider.Service;
-import java.util.Objects;
 import java.util.Arrays;
 import sun.security.provider.SunEntries;
 
 /**
  * @test
- * @bug 8228613 8246613
+ * @bug 8228613 8246613 8248505
  * @summary Ensure that the default SecureRandom algo used is based
  *     on the registration ordering, and falls to next provider
  *     if none are found
@@ -40,6 +39,9 @@ import sun.security.provider.SunEntries;
  */
 public class DefaultAlgo {
 
+    private static final String SR_IMPLCLASS =
+            "sun.security.provider.SecureRandom";
+
     public static void main(String[] args) throws Exception {
         String[] algos = { "A", "B", "C" };
         test3rdParty(algos);
@@ -51,7 +53,8 @@ public class DefaultAlgo {
     private static void test3rdParty(String[] algos) {
         Provider[] provs = {
             new SampleLegacyProvider(algos),
-            new SampleServiceProvider(algos)
+            new SampleServiceProvider(algos),
+            new CustomProvider(algos)
         };
         for (Provider p : provs) {
             checkDefault(p, algos);
@@ -72,9 +75,10 @@ public class DefaultAlgo {
     }
 
     private static void checkDefault(Provider p, String ... algos) {
-        out.println(p.getName() + " with " + Arrays.toString(algos));
-        int pos = Security.insertProviderAt(p, 1);
         String pName = p.getName();
+        out.println(pName + " with " + Arrays.toString(algos));
+        int pos = Security.insertProviderAt(p, 1);
+
         boolean isLegacy = pName.equals("SampleLegacy");
         try {
             if (isLegacy) {
@@ -91,7 +95,13 @@ public class DefaultAlgo {
             out.println("=> Test Passed");
         } finally {
             if (pos != -1) {
-                Security.removeProvider(p.getName());
+                Security.removeProvider(pName);
+            }
+            if (isLegacy) {
+                // add back the removed algos
+                for (String s : algos) {
+                    p.put("SecureRandom." + s, SR_IMPLCLASS);
+                }
             }
         }
     }
@@ -100,7 +110,7 @@ public class DefaultAlgo {
         SampleLegacyProvider(String[] listOfSupportedRNGs) {
             super("SampleLegacy", "1.0", "test provider using legacy put");
             for (String s : listOfSupportedRNGs) {
-                put("SecureRandom." + s, "sun.security.provider.SecureRandom");
+                put("SecureRandom." + s, SR_IMPLCLASS);
             }
         }
     }
@@ -110,8 +120,37 @@ public class DefaultAlgo {
             super("SampleService", "1.0", "test provider using putService");
             for (String s : listOfSupportedRNGs) {
                 putService(new Provider.Service(this, "SecureRandom", s,
-                        "sun.security.provider.SecureRandom", null, null));
+                        SR_IMPLCLASS, null, null));
+            }
+        }
+    }
+
+    // custom provider which overrides putService(...)/getService() and uses
+    // its own Service impl
+    private static class CustomProvider extends Provider {
+        private static class CustomService extends Provider.Service {
+            CustomService(Provider p, String type, String algo, String cName) {
+                super(p, type, algo, cName, null, null);
             }
         }
+
+        CustomProvider(String[] listOfSupportedRNGs) {
+            super("Custom", "1.0", "test provider overrides putService with " +
+                    " custom service with legacy registration");
+            for (String s : listOfSupportedRNGs) {
+                putService(new CustomService(this, "SecureRandom", s ,
+                        SR_IMPLCLASS));
+            }
+        }
+        @Override
+        protected void putService(Provider.Service s) {
+            // convert to legacy puts
+            put(s.getType() + "." + s.getAlgorithm(), s.getClassName());
+            put(s.getType() + ":" + s.getAlgorithm(), s);
+        }
+        @Override
+        public Provider.Service getService(String type, String algo) {
+            return (Provider.Service) get(type + ":" + algo);
+        }
     }
 }