8177474: Do not emit warnings when illegal access is allowed by --add-exports/--add-opens
authoralanb
Fri, 24 Mar 2017 16:35:51 +0000
changeset 44364 9cc9dc782213
parent 44363 bdc8543c97f5
child 44365 15a9dd7386b1
8177474: Do not emit warnings when illegal access is allowed by --add-exports/--add-opens Reviewed-by: chegar, mchung
jdk/src/java.base/share/classes/jdk/internal/module/IllegalAccessLogger.java
jdk/src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java
jdk/src/java.base/share/classes/sun/launcher/LauncherHelper.java
jdk/src/java.base/share/classes/sun/launcher/resources/launcher.properties
jdk/test/tools/launcher/modules/permit/PermitIllegalAccess.java
--- a/jdk/src/java.base/share/classes/jdk/internal/module/IllegalAccessLogger.java	Fri Mar 24 09:16:58 2017 -0700
+++ b/jdk/src/java.base/share/classes/jdk/internal/module/IllegalAccessLogger.java	Fri Mar 24 16:35:51 2017 +0000
@@ -25,6 +25,7 @@
 
 package jdk.internal.module;
 
+import java.io.PrintStream;
 import java.lang.invoke.MethodHandles;
 import java.lang.reflect.Module;
 import java.net.URL;
@@ -42,6 +43,9 @@
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
+import jdk.internal.loader.BootLoader;
+import sun.security.action.GetPropertyAction;
+
 /**
  * Supports logging of access to members of API packages that are exported or
  * opened via backdoor mechanisms to code in unnamed modules.
@@ -49,16 +53,25 @@
 
 public final class IllegalAccessLogger {
 
-    // true to print stack trace
-    private static final boolean PRINT_STACK_TRACE;
-    static {
-        String s = System.getProperty("sun.reflect.debugModuleAccessChecks");
-        PRINT_STACK_TRACE = "access".equals(s);
+    /**
+     * Holder class to lazily create the StackWalker object and determine
+     * if the stack trace should be printed
+     */
+    static class Holder {
+        static final StackWalker STACK_WALKER;
+        static final boolean PRINT_STACK_TRACE;
+
+        static {
+            PrivilegedAction<StackWalker> pa = () ->
+                StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE);
+            STACK_WALKER = AccessController.doPrivileged(pa);
+
+            String name = "sun.reflect.debugModuleAccessChecks";
+            String value = GetPropertyAction.privilegedGetProperty(name, null);
+            PRINT_STACK_TRACE = "access" .equals(value);
+        }
     }
 
-    private static final StackWalker STACK_WALKER
-        = StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE);
-
     // the maximum number of frames to capture
     private static final int MAX_STACK_FRAMES = 32;
 
@@ -72,10 +85,15 @@
     private final Map<Module, Map<String, String>> exported;
     private final Map<Module, Map<String, String>> opened;
 
+    // the print stream to send the warnings
+    private final PrintStream warningStream;
+
     private IllegalAccessLogger(Map<Module, Map<String, String>> exported,
-                                Map<Module, Map<String, String>> opened) {
+                                Map<Module, Map<String, String>> opened,
+                                PrintStream warningStream) {
         this.exported = deepCopy(exported);
         this.opened = deepCopy(opened);
+        this.warningStream = warningStream;
     }
 
     /**
@@ -168,7 +186,7 @@
      */
     private void log(Class<?> caller, String what, Supplier<String> msgSupplier) {
         // stack trace without the top-most frames in java.base
-        List<StackWalker.StackFrame> stack = STACK_WALKER.walk(s ->
+        List<StackWalker.StackFrame> stack = Holder.STACK_WALKER.walk(s ->
             s.dropWhile(this::isJavaBase)
              .limit(MAX_STACK_FRAMES)
              .collect(Collectors.toList())
@@ -184,13 +202,13 @@
         // log message if first usage
         if (firstUsage) {
             String msg = msgSupplier.get();
-            if (PRINT_STACK_TRACE) {
+            if (Holder.PRINT_STACK_TRACE) {
                 synchronized (OUTPUT_LOCK) {
-                    System.err.println(msg);
-                    stack.forEach(f -> System.err.println("\tat " + f));
+                    warningStream.println(msg);
+                    stack.forEach(f -> warningStream.println("\tat " + f));
                 }
             } else {
-                System.err.println(msg);
+                warningStream.println(msg);
             }
         }
     }
@@ -265,8 +283,10 @@
      * A builder for IllegalAccessLogger objects.
      */
     public static class Builder {
+        private final Module UNNAMED = BootLoader.getUnnamedModule();
         private Map<Module, Map<String, String>> exported;
         private Map<Module, Map<String, String>> opened;
+        private PrintStream warningStream = System.err;
 
         public Builder() { }
 
@@ -276,30 +296,37 @@
             this.opened = deepCopy(opened);
         }
 
-        public void logAccessToExportedPackage(Module m, String pn, String how) {
-            if (!m.isExported(pn)) {
+        public Builder logAccessToExportedPackage(Module m, String pn, String how) {
+            if (!m.isExported(pn, UNNAMED)) {
                 if (exported == null)
                     exported = new HashMap<>();
                 exported.computeIfAbsent(m, k -> new HashMap<>()).putIfAbsent(pn, how);
             }
+            return this;
         }
 
-        public void logAccessToOpenPackage(Module m, String pn, String how) {
+        public Builder logAccessToOpenPackage(Module m, String pn, String how) {
             // opens implies exported at run-time.
             logAccessToExportedPackage(m, pn, how);
 
-            if (!m.isOpen(pn)) {
+            if (!m.isOpen(pn, UNNAMED)) {
                 if (opened == null)
                     opened = new HashMap<>();
                 opened.computeIfAbsent(m, k -> new HashMap<>()).putIfAbsent(pn, how);
             }
+            return this;
+        }
+
+        public Builder warningStream(PrintStream warningStream) {
+            this.warningStream = Objects.requireNonNull(warningStream);
+            return this;
         }
 
         /**
          * Builds the logger.
          */
         public IllegalAccessLogger build() {
-            return new IllegalAccessLogger(exported, opened);
+            return new IllegalAccessLogger(exported, opened, warningStream);
         }
     }
 
--- a/jdk/src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java	Fri Mar 24 09:16:58 2017 -0700
+++ b/jdk/src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java	Fri Mar 24 16:35:51 2017 +0000
@@ -515,44 +515,42 @@
      * additional packages specified on the command-line.
      */
     private static void addExtraExportsAndOpens(Layer bootLayer) {
-        IllegalAccessLogger.Builder builder = new IllegalAccessLogger.Builder();
-
         // --add-exports
         String prefix = "jdk.module.addexports.";
         Map<String, List<String>> extraExports = decode(prefix);
         if (!extraExports.isEmpty()) {
-            addExtraExportsOrOpens(bootLayer, extraExports, false, builder);
+            addExtraExportsOrOpens(bootLayer, extraExports, false);
         }
 
         // --add-opens
         prefix = "jdk.module.addopens.";
         Map<String, List<String>> extraOpens = decode(prefix);
         if (!extraOpens.isEmpty()) {
-            addExtraExportsOrOpens(bootLayer, extraOpens, true, builder);
+            addExtraExportsOrOpens(bootLayer, extraOpens, true);
         }
 
         // --permit-illegal-access
         if (getAndRemoveProperty("jdk.module.permitIllegalAccess") != null) {
             warn("--permit-illegal-access will be removed in the next major release");
+            IllegalAccessLogger.Builder builder = new IllegalAccessLogger.Builder();
+            Module unnamed = BootLoader.getUnnamedModule();
             bootLayer.modules().stream().forEach(m -> {
                 m.getDescriptor()
                  .packages()
                  .stream()
-                 .filter(pn -> !m.isOpen(pn))
+                 .filter(pn -> !m.isOpen(pn, unnamed))  // skip if opened by --add-opens
                  .forEach(pn -> {
                      builder.logAccessToOpenPackage(m, pn, "--permit-illegal-access");
                      Modules.addOpensToAllUnnamed(m, pn);
                  });
             });
+            IllegalAccessLogger.setIllegalAccessLogger(builder.build());
         }
-
-        IllegalAccessLogger.setIllegalAccessLogger(builder.build());
     }
 
     private static void addExtraExportsOrOpens(Layer bootLayer,
                                                Map<String, List<String>> map,
-                                               boolean opens,
-                                               IllegalAccessLogger.Builder builder)
+                                               boolean opens)
     {
         String option = opens ? ADD_OPENS : ADD_EXPORTS;
         for (Map.Entry<String, List<String>> e : map.entrySet()) {
@@ -600,10 +598,8 @@
                 }
                 if (allUnnamed) {
                     if (opens) {
-                        builder.logAccessToOpenPackage(m, pn, option);
                         Modules.addOpensToAllUnnamed(m, pn);
                     } else {
-                        builder.logAccessToExportedPackage(m, pn, option);
                         Modules.addExportsToAllUnnamed(m, pn);
                     }
                 } else {
--- a/jdk/src/java.base/share/classes/sun/launcher/LauncherHelper.java	Fri Mar 24 09:16:58 2017 -0700
+++ b/jdk/src/java.base/share/classes/sun/launcher/LauncherHelper.java	Fri Mar 24 16:35:51 2017 +0000
@@ -85,7 +85,6 @@
 import java.util.stream.Stream;
 
 import jdk.internal.misc.VM;
-import jdk.internal.module.IllegalAccessLogger;
 import jdk.internal.module.Modules;
 
 
@@ -429,20 +428,14 @@
                 abort(null, "java.launcher.jar.error3", jarname);
             }
 
-            // Add-Exports and Add-Opens to allow illegal access
+            // Add-Exports and Add-Opens
             String exports = mainAttrs.getValue(ADD_EXPORTS);
             if (exports != null) {
-                String warn = getLocalizedMessage("java.launcher.permitaccess.warning",
-                                                  jarname, ADD_EXPORTS);
-                System.err.println(warn);
-                addExportsOrOpens(exports, false, ADD_EXPORTS);
+                addExportsOrOpens(exports, false);
             }
             String opens = mainAttrs.getValue(ADD_OPENS);
             if (opens != null) {
-                String warn = getLocalizedMessage("java.launcher.permitaccess.warning",
-                                                   jarname, ADD_OPENS);
-                System.err.println(warn);
-                addExportsOrOpens(opens, true, ADD_OPENS);
+                addExportsOrOpens(opens, true);
             }
 
             /*
@@ -467,15 +460,7 @@
      * Process the Add-Exports or Add-Opens value. The value is
      * {@code <module>/<package> ( <module>/<package>)*}.
      */
-    static void addExportsOrOpens(String value, boolean open, String how) {
-        IllegalAccessLogger.Builder builder;
-        IllegalAccessLogger logger = IllegalAccessLogger.illegalAccessLogger();
-        if (logger == null) {
-            builder = new IllegalAccessLogger.Builder();
-        } else {
-            builder = logger.toBuilder();
-        }
-
+    static void addExportsOrOpens(String value, boolean open) {
         for (String moduleAndPackage : value.split(" ")) {
             String[] s = moduleAndPackage.trim().split("/");
             if (s.length == 2) {
@@ -485,18 +470,14 @@
                 Layer.boot().findModule(mn).ifPresent(m -> {
                     if (m.getDescriptor().packages().contains(pn)) {
                         if (open) {
-                            builder.logAccessToOpenPackage(m, pn, how);
                             Modules.addOpensToAllUnnamed(m, pn);
                         } else {
-                            builder.logAccessToExportedPackage(m, pn, how);
                             Modules.addExportsToAllUnnamed(m, pn);
                         }
                     }
                 });
             }
         }
-
-        IllegalAccessLogger.setIllegalAccessLogger(builder.build());
     }
 
     // From src/share/bin/java.c:
--- a/jdk/src/java.base/share/classes/sun/launcher/resources/launcher.properties	Fri Mar 24 09:16:58 2017 -0700
+++ b/jdk/src/java.base/share/classes/sun/launcher/resources/launcher.properties	Fri Mar 24 16:35:51 2017 +0000
@@ -211,6 +211,4 @@
 java.launcher.module.error3=\
     Error: Unable to load main class {0} from module {1}\n\
     \t{2}
-java.launcher.permitaccess.warning=\
-    WARNING: Main manifest of {0} contains {1} attribute to permit illegal access
 
--- a/jdk/test/tools/launcher/modules/permit/PermitIllegalAccess.java	Fri Mar 24 09:16:58 2017 -0700
+++ b/jdk/test/tools/launcher/modules/permit/PermitIllegalAccess.java	Fri Mar 24 16:35:51 2017 +0000
@@ -29,7 +29,10 @@
  * @summary Basic test for java --permit-illegal-access
  */
 
+import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
+import java.util.stream.Stream;
 
 import jdk.testlibrary.ProcessTools;
 import jdk.testlibrary.OutputAnalyzer;
@@ -58,10 +61,13 @@
      * Launches AttemptAccess to execute an action, returning the OutputAnalyzer
      * to analyze the output/exitCode.
      */
-    private OutputAnalyzer tryAction(String action, int count) throws Exception {
-        String arg = "" + count;
-        return ProcessTools
-                .executeTestJava("-cp", TEST_CLASSES, TEST_MAIN, action, arg)
+    private OutputAnalyzer tryAction(String action, int count, String... args)
+        throws Exception
+    {
+        Stream<String> s1 = Stream.of(args);
+        Stream<String> s2 = Stream.of("-cp", TEST_CLASSES, TEST_MAIN, action, "" + count);
+        String[] opts = Stream.concat(s1, s2).toArray(String[]::new);
+        return ProcessTools.executeTestJava(opts)
                 .outputTo(System.out)
                 .errorTo(System.out);
     }
@@ -70,16 +76,10 @@
      * Launches AttemptAccess with --permit-illegal-access to execute an action,
      * returning the OutputAnalyzer to analyze the output/exitCode.
      */
-    private OutputAnalyzer tryActionPermittingIllegalAccess(String action,
-                                                            int count)
+    private OutputAnalyzer tryActionPermittingIllegalAccess(String action, int count)
         throws Exception
     {
-        String arg = "" + count;
-        return ProcessTools
-                .executeTestJava("-cp", TEST_CLASSES, "--permit-illegal-access",
-                                 TEST_MAIN, action, arg)
-                .outputTo(System.out)
-                .errorTo(System.out);
+        return tryAction(action, count, "--permit-illegal-access");
     }
 
     /**
@@ -195,6 +195,61 @@
     }
 
     /**
+     * Permit access to succeed with --add-exports. No warning should be printed.
+     */
+    public void testAccessWithAddExports() throws Exception {
+        tryAction("access", 1, "--add-exports", "java.base/sun.security.x509=ALL-UNNAMED")
+                .stdoutShouldNotContain(WARNING)
+                .stdoutShouldNotContain("IllegalAccessException")
+                .stderrShouldNotContain(WARNING)
+                .stderrShouldNotContain("IllegalAccessException")
+                .shouldHaveExitValue(0);
+    }
+
+    /**
+     * Permit access to succeed with --add-exports and --permit-illegal-access.
+     * The only warning emitted should be the startup warning.
+     */
+    public void testAccessWithePermittedAddExports() throws Exception {
+        tryAction("access", 1, "--permit-illegal-access",
+                    "--add-exports", "java.base/sun.security.x509=ALL-UNNAMED")
+                .stdoutShouldNotContain(WARNING)
+                .stdoutShouldNotContain("IllegalAccessException")
+                .stderrShouldContain(STARTUP_WARNING)
+                .stderrShouldNotContain("IllegalAccessException")
+                .stderrShouldNotContain(ILLEGAL_ACCESS_WARNING)
+                .shouldHaveExitValue(0);
+    }
+
+    /**
+     * Permit setAccessible to succeed with --add-opens. No warning should be printed.
+     */
+    public void testSetAccessibleWithAddOpens() throws Exception {
+        tryAction("setAccessible", 1, "--add-opens", "java.base/java.lang=ALL-UNNAMED")
+                .stdoutShouldNotContain(WARNING)
+                .stdoutShouldNotContain("InaccessibleObjectException")
+                .stderrShouldNotContain(WARNING)
+                .stderrShouldNotContain("InaccessibleObjectException")
+                .shouldHaveExitValue(0);
+    }
+
+    /**
+     * Permit setAccessible to succeed with both --add-opens and --permit-illegal-access.
+     * The only warning emitted should be the startup warning.
+     */
+    public void testSetAccessiblePermittedWithAddOpens() throws Exception {
+        tryAction("setAccessible", 1, "--permit-illegal-access",
+                    "--add-opens", "java.base/java.lang=ALL-UNNAMED")
+                .stdoutShouldNotContain(WARNING)
+                .stdoutShouldNotContain("InaccessibleObjectException")
+                .stderrShouldContain(STARTUP_WARNING)
+                .stderrShouldNotContain("InaccessibleObjectException")
+                .stderrShouldNotContain(ILLEGAL_ACCESS_WARNING)
+                .shouldHaveExitValue(0);
+    }
+
+
+    /**
      * Returns the number of lines in the given input that contain the
      * given char sequence.
      */