[GR-61457] Warn about unused language allow list entries in language permissions tool.

PullRequest: graal/19829

Author: tzezula

substratevm/src/com.oracle.svm.truffle.tck/src/META-INF/native-image/native-image.properties

@@ -23,4 +23,5 @@ ProvidedHostedOptions = \
   TruffleTCKPermissionsReportFile= \
   TruffleTCKPermissionsExcludeFiles= \
   TruffleTCKPermissionsMaxStackTraceDepth= \
-  TruffleTCKPermissionsMaxErrors=
+  TruffleTCKPermissionsMaxErrors= \
+  TruffleTCKUnusedAllowListEntriesAction=

substratevm/src/com.oracle.svm.truffle.tck/src/com/oracle/svm/truffle/tck/WhiteListParser.java renamed to substratevm/src/com.oracle.svm.truffle.tck/src/com/oracle/svm/truffle/tck/AllowListParser.java

@@ -53,32 +53,27 @@
 import jdk.vm.ci.meta.ResolvedJavaType;
 import jdk.vm.ci.meta.Signature;
 
-final class WhiteListParser extends ConfigurationParser {
+final class AllowListParser extends ConfigurationParser {
 
     private static final String CONSTRUCTOR_NAME = "<init>";
 
     private final ImageClassLoader imageClassLoader;
     private final BigBang bb;
-    private Set<AnalysisMethodNode> whiteList;
+    private final Set<AnalysisMethodNode> allowList;
 
-    WhiteListParser(ImageClassLoader imageClassLoader, BigBang bb) {
+    AllowListParser(ImageClassLoader imageClassLoader, BigBang bb) {
         super(true);
         this.imageClassLoader = Objects.requireNonNull(imageClassLoader, "ImageClassLoader must be non null");
         this.bb = Objects.requireNonNull(bb, "BigBang must be non null");
+        this.allowList = new HashSet<>();
     }
 
-    Set<AnalysisMethodNode> getLoadedWhiteList() {
-        if (whiteList == null) {
-            throw new IllegalStateException("Not parsed yet.");
-        }
-        return whiteList;
+    Set<AnalysisMethodNode> getLoadedAllowList() {
+        return allowList;
     }
 
     @Override
     public void parseAndRegister(Object json, URI origin) {
-        if (whiteList == null) {
-            whiteList = new HashSet<>();
-        }
         parseClassArray(castList(json, "First level of document must be an array of class descriptors"));
     }
 
@@ -211,33 +206,33 @@ private boolean registerMethod(AnalysisType type, String methodName, List<Analys
         Predicate<ResolvedJavaMethod> p = (m) -> methodName.equals(m.getName());
         p = p.and(new SignaturePredicate(type, formalParameters, bb));
         Set<AnalysisMethodNode> methods = PermissionsFeature.findMethods(bb, type, p);
-        whiteList.addAll(methods);
+        allowList.addAll(methods);
         return !methods.isEmpty();
     }
 
     private boolean registerAllMethodsWithName(AnalysisType type, String name) {
         Set<AnalysisMethodNode> methods = PermissionsFeature.findMethods(bb, type, (m) -> name.equals(m.getName()));
-        whiteList.addAll(methods);
+        allowList.addAll(methods);
         return !methods.isEmpty();
     }
 
     private boolean registerConstructor(AnalysisType type, List<AnalysisType> formalParameters) {
         Predicate<ResolvedJavaMethod> p = new SignaturePredicate(type, formalParameters, bb);
         Set<AnalysisMethodNode> methods = PermissionsFeature.findConstructors(bb, type, p);
-        whiteList.addAll(methods);
+        allowList.addAll(methods);
         return !methods.isEmpty();
     }
 
     private boolean registerDeclaredConstructors(AnalysisType type) {
         for (AnalysisMethod method : type.getDeclaredConstructors(false)) {
-            whiteList.add(new AnalysisMethodNode(method));
+            allowList.add(new AnalysisMethodNode(method));
         }
         return true;
     }
 
     private boolean registerDeclaredMethods(AnalysisType type) {
         for (AnalysisMethod method : type.getDeclaredMethods(false)) {
-            whiteList.add(new AnalysisMethodNode(method));
+            allowList.add(new AnalysisMethodNode(method));
         }
         return true;
     }

substratevm/src/com.oracle.svm.truffle.tck/src/com/oracle/svm/truffle/tck/PermissionsFeature.java

@@ -49,10 +49,12 @@
 import java.util.Objects;
 import java.util.Set;
 import java.util.function.BooleanSupplier;
+import java.util.function.Function;
 import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
 import com.oracle.svm.hosted.code.FactoryMethod;
+import com.oracle.svm.util.LogUtils;
 import org.graalvm.nativeimage.ImageSingletons;
 import org.graalvm.nativeimage.hosted.Feature;
 import org.graalvm.polyglot.io.FileSystem;
@@ -110,6 +112,12 @@ public class PermissionsFeature implements Feature {
 
     private static final String CONFIG = "truffle-language-permissions-config.json";
 
+    public enum ActionKind {
+        Ignore,
+        Warn,
+        Throw
+    }
+
     public static class Options {
         @Option(help = "Path to file where to store report of Truffle language privilege access.")//
         public static final HostedOptionKey<String> TruffleTCKPermissionsReportFile = new HostedOptionKey<>(null);
@@ -124,6 +132,13 @@ public static class Options {
 
         @Option(help = "Maximum number of erroneous privileged accesses reported.", type = OptionType.Expert)//
         public static final HostedOptionKey<Integer> TruffleTCKPermissionsMaxErrors = new HostedOptionKey<>(100);
+
+        @Option(help = {"Specifies how unused methods in the language allow list should be handled.",
+                        "Available options are:",
+                        "  \"Ignore\": Do not report unused methods in the allow list.",
+                        "  \"Warn\": Log a warning message to stderr.",
+                        "  \"Throw\" (default): Throw an exception and abort the native-image build process."}, type = OptionType.Expert)//
+        public static final HostedOptionKey<ActionKind> TruffleTCKUnusedAllowListEntriesAction = new HostedOptionKey<>(ActionKind.Throw);
     }
 
     /**
@@ -178,17 +193,22 @@ public boolean getAsBoolean() {
     /**
      * Methods which should not be found.
      */
-    private Set<BaseMethodNode> deniedMethods = new HashSet<>();
+    private final Set<BaseMethodNode> deniedMethods = new HashSet<>();
 
     /**
      * Path to store report into.
      */
     private Path reportFilePath;
 
     /**
-     * Methods which are allowed to do privileged calls without being reported.
+     * JDK methods which are allowed to do privileged calls without being reported.
      */
-    private Set<? extends BaseMethodNode> whiteList;
+    private Set<? extends BaseMethodNode> platformAllowList;
+
+    /**
+     * Language methods which are allowed to do privileged calls without being reported.
+     */
+    private Map<BaseMethodNode, Boolean> languageAllowList;
 
     private Set<CallGraphFilter> contextFilters;
 
@@ -229,12 +249,13 @@ public void beforeAnalysis(BeforeAnalysisAccess access) {
                         new SafeServiceLoaderRecognizer(bb, accessImpl.getImageClassLoader()), new SafeSetThreadNameRecognizer(bb));
 
         /*
-         * Ensure methods which are either deniedMethods or on the whiteList are never inlined into
+         * Ensure methods which are either deniedMethods or on the allow list are never inlined into
          * methods. These methods are important for identifying violations.
          */
         Set<AnalysisMethod> preventInlineBeforeAnalysis = new HashSet<>();
         deniedMethods.stream().map(BaseMethodNode::getMethod).forEach(preventInlineBeforeAnalysis::add);
-        whiteList.stream().map(BaseMethodNode::getMethod).forEach(preventInlineBeforeAnalysis::add);
+        platformAllowList.stream().map(BaseMethodNode::getMethod).forEach(preventInlineBeforeAnalysis::add);
+        languageAllowList.keySet().stream().map(BaseMethodNode::getMethod).forEach(preventInlineBeforeAnalysis::add);
         contextFilters.stream().map(CallGraphFilter::getInspectedMethods).forEach(preventInlineBeforeAnalysis::addAll);
 
         accessImpl.getHostVM().registerNeverInlineTrivialHandler((caller, callee) -> {
@@ -257,14 +278,19 @@ private void initializeDeniedMethods(FeatureImpl.BeforeAnalysisAccessImpl access
         if (sunMiscUnsafe != null) {
             inlinedUnsafeCall = new InlinedUnsafeMethodNode(bb.getMetaAccess().lookupJavaType(sunMiscUnsafe));
         }
-        WhiteListParser parser = new WhiteListParser(accessImpl.getImageClassLoader(), bb);
-        ConfigurationParserUtils.parseAndRegisterConfigurations(parser,
-                        accessImpl.getImageClassLoader(),
-                        ClassUtil.getUnqualifiedName(getClass()),
-                        Options.TruffleTCKPermissionsExcludeFiles,
-                        new ResourceAsOptionDecorator(getClass().getPackage().getName().replace('.', '/') + "/resources/jre.json"),
-                        CONFIG);
-        whiteList = parser.getLoadedWhiteList();
+        String featureName = ClassUtil.getUnqualifiedName(getClass());
+        AllowListParser parser = new AllowListParser(accessImpl.getImageClassLoader(), bb);
+        ConfigurationParserUtils.parseAndRegisterConfigurations(parser, accessImpl.getImageClassLoader(), featureName,
+                        CONFIG,
+                        List.of(),
+                        List.of(getClass().getPackage().getName().replace('.', '/') + "/resources/jre.json"));
+        platformAllowList = parser.getLoadedAllowList();
+        parser = new AllowListParser(accessImpl.getImageClassLoader(), bb);
+        ConfigurationParserUtils.parseAndRegisterConfigurations(parser, accessImpl.getImageClassLoader(), featureName,
+                        CONFIG,
+                        Options.TruffleTCKPermissionsExcludeFiles.getValue().values(),
+                        List.of());
+        languageAllowList = parser.getLoadedAllowList().stream().collect(Collectors.toMap(Function.identity(), key -> false));
         deniedMethods.addAll(findMethods(bb, SecurityManager.class, (m) -> m.getName().startsWith("check")));
         if (sunMiscUnsafe != null) {
             deniedMethods.addAll(findMethods(bb, sunMiscUnsafe, ModifiersProvider::isPublic));
@@ -321,6 +347,21 @@ public void afterAnalysis(AfterAnalysisAccess access) {
                                     pw.print(builder);
                                 });
             }
+            List<BaseMethodNode> unusedLanguageAllowListEntries = languageAllowList.entrySet().stream().filter((e) -> !e.getValue()).map(Map.Entry::getKey).toList();
+            if (!unusedLanguageAllowListEntries.isEmpty()) {
+                StringBuilder errorMessageBuilder = new StringBuilder(
+                                "The following methods in the language allow list were not statically reachable during points-to analysis. " + "Please review and remove them from the allow list:\n");
+                for (BaseMethodNode unused : unusedLanguageAllowListEntries) {
+                    errorMessageBuilder.append(" - ").append(unused.getMethod().format("%H.%n(%p)")).append("\n");
+                }
+                switch (Options.TruffleTCKUnusedAllowListEntriesAction.getValue()) {
+                    case Ignore -> {
+                    }
+                    case Warn -> LogUtils.warning("[%s] %s", ClassUtil.getUnqualifiedName(getClass()), errorMessageBuilder);
+                    case Throw -> throw UserError.abort(errorMessageBuilder.toString());
+                    default -> throw new AssertionError(Options.TruffleTCKUnusedAllowListEntriesAction.getValue());
+                }
+            }
         }
     }
 
@@ -533,7 +574,7 @@ private int collectViolations(
             if (isSafeClass(mNode)) {
                 return numReports;
             }
-            // The denied method can be excluded by a white list
+            // The denied method can be excluded by a allow list
             if (isExcludedClass(mNode)) {
                 return numReports;
             }
@@ -618,12 +659,15 @@ private static boolean isClassInPackage(String javaName, Collection<? extends St
     }
 
     /**
-     * Tests if the given {@link BaseMethodNode} is excluded by white list.
+     * Tests if the given {@link BaseMethodNode} is excluded by allow list.
      *
      * @param methodNode the {@link BaseMethodNode} to check
      */
     private boolean isExcludedClass(BaseMethodNode methodNode) {
-        return whiteList.contains(methodNode);
+        if (platformAllowList.contains(methodNode)) {
+            return true;
+        }
+        return languageAllowList.computeIfPresent(methodNode, (n, v) -> true) != null;
     }
 
     /**
@@ -783,7 +827,7 @@ public boolean test(BaseMethodNode methodNode, BaseMethodNode callerNode, List<B
                     if (args.isEmpty()) {
                         return false;
                     }
-                    ValueNode arg0 = args.get(0);
+                    ValueNode arg0 = args.getFirst();
                     ResolvedJavaType newType = null;
                     if (arg0 instanceof NewInstanceNode newInstanceNode) {
                         newType = newInstanceNode.instanceClass();
@@ -938,7 +982,7 @@ public boolean test(BaseMethodNode methodNode, BaseMethodNode callerNode, List<B
             for (Invoke invoke : graph.getInvokes()) {
                 if (method.equals(invoke.callTarget().targetMethod())) {
                     NodeInputList<ValueNode> args = invoke.callTarget().arguments();
-                    ValueNode arg0 = args.get(0);
+                    ValueNode arg0 = args.getFirst();
                     boolean isTruffleThread = false;
                     if (arg0 instanceof PiNode piNode) {
                         arg0 = piNode.getOriginalNode();
@@ -962,16 +1006,6 @@ public Collection<AnalysisMethod> getInspectedMethods() {
         }
     }
 
-    /**
-     * Options facade for a resource containing the JRE white list.
-     */
-    private static final class ResourceAsOptionDecorator extends HostedOptionKey<AccumulatingLocatableMultiOptionValue.Strings> {
-
-        ResourceAsOptionDecorator(String defaultValue) {
-            super(AccumulatingLocatableMultiOptionValue.Strings.buildWithDefaults(defaultValue));
-        }
-    }
-
     abstract static class BaseMethodNode {
         abstract StackTraceElement asStackTraceElement();
 

vm/mx.vm/mx_vm_gate.py

@@ -652,6 +652,7 @@ def _collect_excludes(suite, suite_import, excludes):
         options = mx.get_runtime_jvm_args(dists, exclude_names=['substratevm:SVM']) + [
             '--features=com.oracle.svm.truffle.tck.PermissionsFeature',
         ] + mx_sdk_vm_impl.svm_experimental_options([
+            '-H:TruffleTCKUnusedAllowListEntriesAction=Warn', # GR-61487: Clean JavaScript allow list
             '-H:ClassInitialization=:build_time',
             '-H:+EnforceMaxRuntimeCompileMethods',
             '-H:-FoldSecurityManagerGetter',

wasm/mx.wasm/truffle.tck.permissions/unsafe_excludes.json

@@ -262,27 +262,6 @@
             }, {
                 "name" : "atomic_rmw_xchg_i64",
                 "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
-            }, {
-                "name" : "atomic_rmw_cmpxchg_i32_8u",
-                "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
-            }, {
-                "name" : "atomic_rmw_cmpxchg_i32_16u",
-                "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
-            }, {
-                "name" : "atomic_rmw_cmpxchg_i32",
-                "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
-            }, {
-                "name" : "atomic_rmw_cmpxchg_i64_8u",
-                "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
-            }, {
-                "name" : "atomic_rmw_cmpxchg_i64_16u",
-                "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
-            }, {
-                "name" : "atomic_rmw_cmpxchg_i64_32u",
-                "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
-            }, {
-                "name" : "atomic_rmw_cmpxchg_i64",
-                "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
             }, {
                 "name" : "initialize",
                 "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
@@ -307,9 +286,6 @@
             }, {
                 "name" : "copyToBuffer",
                 "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
-            }, {
-                "name" : "getObjectFieldOffset",
-                "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
             }
         ]
     }, {
@@ -564,27 +540,6 @@
             }, {
                 "name" : "atomic_rmw_xchg_i64",
                 "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
-            }, {
-                "name" : "atomic_rmw_cmpxchg_i32_8u",
-                "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
-            }, {
-                "name" : "atomic_rmw_cmpxchg_i32_16u",
-                "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
-            }, {
-                "name" : "atomic_rmw_cmpxchg_i32",
-                "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
-            }, {
-                "name" : "atomic_rmw_cmpxchg_i64_8u",
-                "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
-            }, {
-                "name" : "atomic_rmw_cmpxchg_i64_16u",
-                "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
-            }, {
-                "name" : "atomic_rmw_cmpxchg_i64_32u",
-                "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
-            }, {
-                "name" : "atomic_rmw_cmpxchg_i64",
-                "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."
             }, {
                 "name" : "duplicate",
                 "justification" : "Allow using sun.misc.Unsafe methods behind the --wasm.UseUnsafeMemory flag."