8189264: (sl) ServiceLoader does not wrap Errors thrown by provider classes when running with a security manager

Reviewed-by: mchung

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

@@ -747,8 +747,10 @@ public final class ServiceLoader<S>
                 // invoke factory method with permissions restricted by acc
                 try {
                     result = AccessController.doPrivileged(pa, acc);
-                } catch (PrivilegedActionException pae) {
+                } catch (Throwable x) {
-                    exc = pae.getCause();
+                    if (x instanceof PrivilegedActionException)
+                        x = x.getCause();
+                    exc = x;
                 }
             }
             if (exc != null) {
@@ -788,8 +790,10 @@ public final class ServiceLoader<S>
                 // invoke constructor with permissions restricted by acc
                 try {
                     p = AccessController.doPrivileged(pa, acc);
-                } catch (PrivilegedActionException pae) {
+                } catch (Throwable x) {
-                    exc = pae.getCause();
+                    if (x instanceof PrivilegedActionException)
+                        x = x.getCause();
+                    exc = x;
                 }
             }
             if (exc != null) {
@@ -852,8 +856,9 @@ public final class ServiceLoader<S>
             PrivilegedExceptionAction<Class<?>> pa = () -> Class.forName(module, cn);
             try {
                 clazz = AccessController.doPrivileged(pa);
-            } catch (PrivilegedActionException pae) {
+            } catch (Throwable x) {
-                Throwable x = pae.getCause();
+                if (x instanceof PrivilegedActionException)
+                    x = x.getCause();
                 fail(service, "Unable to load " + cn, x);
                 return null;
             }

src/java.base/share/classes/sun/util/cldr/CLDRLocaleProviderAdapter.java

@@ -26,6 +26,7 @@
 package sun.util.cldr;
 
 import java.security.AccessController;
+import java.security.AccessControlException;
 import java.security.PrivilegedExceptionAction;
 import java.text.spi.BreakIteratorProvider;
 import java.text.spi.CollatorProvider;
@@ -37,6 +38,7 @@ import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
 import java.util.ServiceLoader;
+import java.util.ServiceConfigurationError;
 import java.util.Set;
 import java.util.StringTokenizer;
 import java.util.concurrent.ConcurrentHashMap;
@@ -81,8 +83,11 @@ public class CLDRLocaleProviderAdapter extends JRELocaleProviderAdapter {
                     return null;
                 }
             });
-        }  catch (Exception e) {
+        } catch (Exception e) {
             // Catch any exception, and continue as if only CLDR's base locales exist.
+        } catch (ServiceConfigurationError sce) {
+            Throwable cause = sce.getCause();
+            if (!(cause instanceof AccessControlException)) throw sce;
         }
 
         nonBaseMetaInfo = nbmi;

src/java.base/share/classes/sun/util/locale/provider/JRELocaleProviderAdapter.java

@@ -26,6 +26,7 @@
 package sun.util.locale.provider;
 
 import java.security.AccessController;
+import java.security.AccessControlException;
 import java.security.PrivilegedAction;
 import java.security.PrivilegedExceptionAction;
 import java.text.spi.BreakIteratorProvider;
@@ -40,6 +41,7 @@ import java.util.List;
 import java.util.Locale;
 import java.util.ResourceBundle;
 import java.util.ServiceLoader;
+import java.util.ServiceConfigurationError;
 import java.util.Set;
 import java.util.StringTokenizer;
 import java.util.concurrent.ConcurrentHashMap;
@@ -476,8 +478,11 @@ public class JRELocaleProviderAdapter extends LocaleProviderAdapter implements R
             if (nonBaseTags != null) {
                 supportedLocaleString += " " + nonBaseTags;
             }
-        }  catch (Exception e) {
+        } catch (Exception e) {
             // catch any exception, and ignore them as if non-EN locales do not exist.
+        } catch (ServiceConfigurationError sce) {
+            Throwable cause = sce.getCause();
+            if (!(cause instanceof AccessControlException)) throw sce;
         }
 
         return supportedLocaleString;

test/jdk/java/util/ServiceLoader/security/test/module-info.java

@@ -26,8 +26,16 @@ import p.Tests.*;
 module test {
     uses S1;
     uses S2;
+    uses S3;
+    uses S4;
+    uses S5;
+    uses S6;
     provides S1 with P1;
     provides S2 with P2;
+    provides S3 with P3;
+    provides S4 with P4;
+    provides S5 with P5;
+    provides S6 with P6;
     requires testng;
     exports p to testng;
 }

test/jdk/java/util/ServiceLoader/security/test/p/Tests.java

@@ -38,14 +38,16 @@ import java.util.ServiceLoader;
 import java.util.ServiceLoader.Provider;
 import static java.security.AccessController.doPrivileged;
 
+import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 import org.testng.annotations.BeforeTest;
 import static org.testng.Assert.*;
 
 /**
- * Basic tests with a security manager to ensure that the provider code
+ * Tests ServiceLoader when running with a security manager, specifically
- * is run with permissions restricted by whatever created the ServiceLoader
+ * tests to ensure that provider code is run with permissions restricted by
- * object.
+ * the creater of ServiceLoader, and also testing of exceptions thrown
+ * when loading or initializing a provider.
  */
 
 public class Tests {
@@ -163,11 +165,35 @@ public class Tests {
         }
     }
 
+    @DataProvider(name = "failingServices")
+    public Object[][] failingServices() {
+        return new Object[][] {
+            { S3.class,    P3.Error3.class },
+            { S4.class,    P4.Error4.class },
+            { S5.class,    P5.Error5.class },
+            { S6.class,    P6.Error6.class },
+        };
+    }
+
+    @Test(dataProvider = "failingServices")
+    public void testFailingService(Class<?> service, Class<? extends Error> errorClass) {
+        ServiceLoader<?> sl = ServiceLoader.load(service);
+        try {
+            sl.iterator().next();
+            assertTrue(false);
+        } catch (ServiceConfigurationError e) {
+            assertTrue(e.getCause().getClass() == errorClass);
+        }
+    }
 
     // service types and implementations
 
     public static interface S1 { }
     public static interface S2 { }
+    public static interface S3 { }
+    public static interface S4 { }
+    public static interface S5 { }
+    public static interface S6 { }
 
     public static class P1 implements S1 {
         public P1() {
@@ -182,4 +208,36 @@ public class Tests {
             return new P2();
         }
     }
+
+    public static class P3 implements S3 {
+        static class Error3 extends Error { }
+        static {
+            if (1==1) throw new Error3();  // fail
+        }
+        public P3() { }
+    }
+
+    public static class P4 implements S4 {
+        static class Error4 extends Error { }
+        static {
+            if (1==1) throw new Error4();  // fail
+        }
+        public static S4 provider() {
+            return new P4();
+        }
+    }
+
+    public static class P5 implements S5 {
+        static class Error5 extends Error { }
+        public P5() {
+            throw new Error5();  // fail
+        }
+    }
+
+    public static class P6 implements S6 {
+        static class Error6 extends Error { }
+        public static S6 provider() {
+            throw new Error6();   // fail
+        }
+    }
 }